CryptoModule #117

mohitpubnub · 2023-09-27T07:25:41Z

fix: Improve security of crypto implementation.

Improved security of crypto implementation by adding enhanced AES-CBC cryptor.

feat: Add crypto module

Add crypto module that allows configure SDK to encrypt and decrypt messages.

Draft PR

pubnub/lib/src/crypto/aesCbcCryptor.dart

mohitpubnub · 2023-10-02T07:37:20Z

pubnub/lib/src/core/crypto/cipher_key.dart

@@ -21,4 +21,15 @@ class CipherKey {
  factory CipherKey.fromList(List<int> key) {
    return CipherKey._(key);
  }
+
+  @override
+  bool operator ==(dynamic other) {


@jguz-pubnub for comparison and to distinguish explicitly provided keyset/using keyset v/s default. Till the time we support cipherKey at configuration level.

jguz-pubnub · 2023-10-02T16:43:42Z

pubnub/lib/src/crypto/aesCbcCryptor.dart

+
+  AesCbcCryptor(this.cipherKey);
+  @override
+  List<int> decrypt(EncryptedData encryptedData) {


That's a nitpick from developer to developer, so you don't have to fix it 🙂 You can enable code formatting on save. I know it from Android Studio through:

Preferences -> Languages & Frameworks -> Flutter -> Format code on save

I'm almost sure a similar option may also be available for Visual Studio you use. Moreover, the whole code structure doesn't wrap if you add some trailing commas at the end and save a source file. For example, something like this:

@override List<int> decrypt(EncryptedData encryptedData) { var encrypter = crypto.Encrypter( crypto.AES(_getKey(), mode: crypto.AESMode.cbc), ); return encrypter.decryptBytes( crypto.Encrypted(Uint8List.fromList(encryptedData.data.toList())), iv: crypto.IV(Uint8List.fromList(encryptedData.metadata!.toList())), ); }

setting it up. this will look elegant.
Thanks!

jguz-pubnub · 2023-10-02T16:45:44Z

pubnub/lib/src/crypto/crypto.dart

+    if (encrypted.metadata == null) return encrypted.data;
+
+    var header =
+        CryptorHeader.from(defaultCryptor.identifier, encrypted.metadata!);


Is it safe to access .metadata using an exclamation mark here? The same goes for header! below

EDIT: Regarding metadata, perhaps you could declare it as non-optional List<int> in the EncryptedData class. The empty list might indicate there's no metadata and you could avoid handling such an optional variable every time you want to access it. Let's leave it as it is now if this would require significant effort or may break your unit/contract tests

jguz-pubnub · 2023-10-02T16:50:26Z

pubnub/lib/src/crypto/crypto.dart

-    } else {
-      return crypto.Key(Uint8List.fromList(cipherKey.data));
-    }
+  factory CryptoModule.aescbcCryptoModule(CipherKey cipherKey,


Nitpick: typo in method's name (could be camel-cased aesCbcCryptoModule)

yeah! naming!

jguz-pubnub · 2023-10-02T17:02:09Z

pubnub/lib/src/crypto/crypto.dart

-  /// Decrypts [input] based on the [key] and [configuration].
-  ///
-  /// If [configuration] is `null`, then [CryptoModule.defaultConfiguration] is used.
+  List<ICryptor> _getAllCryptor() {


Nitpick: _getAllCryptors

jguz-pubnub · 2023-10-02T17:07:51Z

pubnub/lib/src/crypto/crypto.dart

-        var iv = crypto.IV.fromUtf8('0123456789012345');
-        return encrypter.encrypt(input, iv: iv).base64;
-      }
+      var allCryptors = _getAllCryptor();


Perhaps you could simplify instantiating local cryptor variable in this way:

var cryptor = allCryptors.firstWhere( (element) => element.identifier == (header?.identifier ?? ''), );

jguz-pubnub · 2023-10-02T17:13:03Z

pubnub/lib/src/crypto/cryptorHeader.dart

+    if (encryptedData.length >= 5) {
+      version = encryptedData[4];
+    } else {
+      throw CryptoException('decryption error');


Don't you want to provide more detailed messages that clearly indicate what's missing? For example, missing sentinel bytes, version byte, etc. Let's leave it as it is if you think it brings no value

That would be helpful.
WIP: update along with tests in next commit.

jguz-pubnub · 2023-10-02T17:14:13Z

pubnub/lib/src/crypto/legacyCryptor.dart

+
+  /// Decrypts [input] with [key] based on [configuration].
+  ///
+  /// If [configuration] is `null`, then [CryptoModule.defaultConfiguration] is used.


I really like those documentation comments! 👏

* Added more information in exception string for reason fo failure. * naming convention fix for CryptoModule factory. * code formatting.

jguz-pubnub · 2023-10-04T12:05:13Z

pubnub/lib/src/core/crypto/crypto.dart

@@ -12,7 +12,7 @@ class CryptoException extends PubNubException {
 }

 /// @nodoc
-abstract class ICryptoModule {
+abstract class ILegacyCryptor {


Android Studio shows me this class is never used. Is it true? :)

Yeah! We do not need it. Earlier I was thinking to have a different interface for LegacyCryptor. But then I was able to figure out that we can unify the signature and have only one interface that's ICryptor.

removing it.

jguz-pubnub · 2023-10-04T12:08:03Z

pubnub/lib/src/crypto/crypto.dart

+
+    var header =
+        CryptorHeader.from(defaultCryptor.identifier, encrypted.metadata);
+    var headerData = List<int>.filled(header!.length, 0);


Are you sure that header is not null here?

So, This is encryption code. Before attempting CryptoHeader.from(...), Here is what we did:

var encrypted = defaultCryptor.encrypt(data); if (encrypted.metadata.isEmpty) return encrypted.data;

Which means If the default cryptor is not giving metadata(that means legacy one) we return from there only.

If we haven't return that means It's not legacy cryptor and If it's not legacy cryptor then we must have header.[going forward we won't ever support cryptor which does not conforms our header requirement] So header! will never throw.

I think I know the root of confusion:
from(..) has this if (id == LEGACY_IDENTIFIER) return null;
This was there for testing purpose(If CryptoHeader.from() called with legacy id then it should work as expected), while consuming this method in encrypt method. We are sure that execution will reach to Header.from(...) only when we have non-legacy cryptor.

I think I can remove if (id == LEGACY_IDENTIFIER) return null; condition. Because from method will be called only when we deal with non legacy cryptor.

jguz-pubnub · 2023-10-04T12:09:27Z

pubnub/lib/src/crypto/crypto.dart

+    var cryptor = _getCryptor(header);
+    var headerLength = header != null ? header.length : 0;
+    var metadata = headerLength > 0
+        ? data.sublist((headerLength - header!.metadataLength), headerLength)


The same, are you sure that header is not null here?

Yes, Because we already performed check :
var headerLength = header != null ? header.length : 0;

we go with sublist only when we have headerLength>0 that means header is not null.

jguz-pubnub · 2023-10-04T12:14:33Z

pubnub/lib/src/dx/channel/channel_history.dart

@@ -104,10 +106,14 @@ class ChannelHistory {
      _cursor = result.endTimetoken;
      _messages.addAll(await Future.wait(result.messages.map((message) async {
        if (_keyset.cipherKey != null) {
-          message['message'] = await _core.parser.decode(_core.crypto
-              .decrypt(_keyset.cipherKey!, message['message'] as String));
+          message['message'] = _keyset.cipherKey ==


Could you explain to me what's the logic here? Perhaps the idea is the same for other changes below

Possibility 1: User has provided a custom cipherKey(or a keyset or one of the keyset from KeysetStore via using param) while calling history.
In this case _keyset(current one from param).cipherKey == _core.keysets.defaultKeyset.cipherKey => false
That means we need to use custom cipherKey - and so we perform encryptWithKey

Possibility 2: User hasn't provided key which means _keyset.cipherKey is already defaulted to defaultKeyset's cipherkey
In this case _keyset.cipherKey == _core.keysets.defaultKeyset.cipherKey => true
That means we can use our decrypt() methods of cryptoModule because that cryptoModule is already initialised with defaultKeyset's cipherKey.

…ecryption, * refactoring to fix broken format issue with decrypted hostory messages, * files encryption/ decryption conditions refined

mohitpubnub · 2023-10-16T11:09:43Z

@pubnub-release-bot release dart as v4.3.0

parfeon

LGTM!

pubnub-release-bot · 2023-10-16T11:17:04Z

🚀 Release successfully completed 🚀

mohitpubnub added 3 commits September 27, 2023 10:36

cryptorHeader for new cryptoModule

6378d0c

format!

29f034f

added AesCbcCryptor, ICryptor

430326d

mohitpubnub requested a review from parfeon as a code owner September 27, 2023 07:25

mohitpubnub marked this pull request as draft September 27, 2023 07:25

mohitpubnub requested a review from jguz-pubnub September 27, 2023 07:26

format!

35fd7a4

jguz-pubnub previously approved these changes Sep 27, 2023

View reviewed changes

jguz-pubnub reviewed Sep 27, 2023

View reviewed changes

pubnub/lib/src/crypto/aesCbcCryptor.dart Outdated Show resolved Hide resolved

mohitpubnub added 3 commits September 28, 2023 13:32

* LegacyCryptor, * added: newCryptoModule

3d5a59a

new crytoModule

fcbf895

test: crypto Module

fb6ba49

mohitpubnub marked this pull request as ready for review October 2, 2023 06:54

mohitpubnub self-assigned this Oct 2, 2023

dart fix

b390625

mohitpubnub commented Oct 2, 2023

View reviewed changes

mohitpubnub and others added 5 commits October 2, 2023 13:07

format!! again

2d57627

* expose CryptoModule for users, * deprecation warning for cipherKey

9de18f1

code cleanup

ca15732

show CryptoConfiguration for legacy cryptor configuration support

2cbf57f

Update CODEOWNERS

f4822c6

jguz-pubnub reviewed Oct 2, 2023

View reviewed changes

* Added empty content handling while encry and decrypt,

138b140

* Added more information in exception string for reason fo failure. * naming convention fix for CryptoModule factory. * code formatting.

jguz-pubnub reviewed Oct 4, 2023

View reviewed changes

removed unused class ILegacyCryptor

9c6edc2

jguz-pubnub approved these changes Oct 4, 2023

View reviewed changes

mohitpubnub added 4 commits October 9, 2023 19:31

fix: encrypt/decrypt File methods

fe02a01

fix: explicit cipherKey at method params defaulting to legacy cryptor

36bf022

added condition to apply user specified cryptoModule for encryption/d…

abc7824

…ecryption, * refactoring to fix broken format issue with decrypted hostory messages, * files encryption/ decryption conditions refined

update LICENSE

70a553d

PubNub SDK v4.3.0 release.

926c90f

pubnub-release-bot requested review from techwritermat and kazydek as code owners October 16, 2023 11:10

parfeon approved these changes Oct 16, 2023

View reviewed changes

techwritermat approved these changes Oct 16, 2023

View reviewed changes

mohitpubnub merged commit ba5935d into master Oct 16, 2023
5 checks passed

mohitpubnub deleted the clen-1585 branch October 16, 2023 11:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CryptoModule #117

CryptoModule #117

mohitpubnub commented Sep 27, 2023 •

edited

Loading

mohitpubnub Oct 2, 2023

jguz-pubnub Oct 2, 2023 •

edited

Loading

mohitpubnub Oct 3, 2023

jguz-pubnub Oct 2, 2023 •

edited

Loading

jguz-pubnub Oct 2, 2023

mohitpubnub Oct 3, 2023

jguz-pubnub Oct 2, 2023

jguz-pubnub Oct 2, 2023

mohitpubnub Oct 3, 2023

jguz-pubnub Oct 2, 2023

mohitpubnub Oct 3, 2023

jguz-pubnub Oct 2, 2023

jguz-pubnub Oct 4, 2023

mohitpubnub Oct 4, 2023

jguz-pubnub Oct 4, 2023

mohitpubnub Oct 4, 2023

jguz-pubnub Oct 4, 2023

mohitpubnub Oct 4, 2023

jguz-pubnub Oct 4, 2023 •

edited

Loading

mohitpubnub Oct 4, 2023

mohitpubnub commented Oct 16, 2023

parfeon left a comment

pubnub-release-bot commented Oct 16, 2023

CryptoModule #117

CryptoModule #117

Conversation

mohitpubnub commented Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

jguz-pubnub Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jguz-pubnub Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jguz-pubnub Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohitpubnub commented Oct 16, 2023

parfeon left a comment

Choose a reason for hiding this comment

pubnub-release-bot commented Oct 16, 2023

mohitpubnub commented Sep 27, 2023 •

edited

Loading

jguz-pubnub Oct 2, 2023 •

edited

Loading

jguz-pubnub Oct 2, 2023 •

edited

Loading

jguz-pubnub Oct 4, 2023 •

edited

Loading