CryptorModule #144

jguz-pubnub · 2023-09-25T15:24:55Z

fix(crypto): Improve security of crypto implementation

Improved security of crypto implementation by adding AES-CBC cryptor

feat(crypto): Add CryptorModule

Add CryptorModule that allows configuring SDK to encrypt and decrypt messages

* Reusing existing CryptoStream + CryptoInputStream by removing Crypto dependency from them * Moved old encryption/decryption to LegacyCryptor * Introduced new AESCBCCryptor * Introduced new CryptorModule instead of Crypto * Fixing tests

…readable error descriptions

… less parameter

…oder

* Prevent from decoding & encoding empty Data & Files * Added inline documentation for public items

parfeon · 2023-10-02T17:47:19Z

Sources/PubNub/Errors/PubNubError.swift

+    case encryptionError
+    case decryptionError
+    case unknownCryptorError


@jguz-pubnub Should we try to be consistent with other cases in class? The class itself already specifies that it will be errors, and the *Error suffix won't have much meaning. Other cases which report any troubles have *Failure suffix.

parfeon · 2023-10-02T17:55:20Z

Tests/PubNubContractTest/Steps/CryptorModule/PubNubCryptoModuleContractTestSteps.swift

+  public override func setup() {
+    startCucumberHookEventsListening()
+
+    var cryptorKind: String = ""
+    var cipherKey: String = ""
+    var randomIV: Bool = true
+    var otherCryptors: [String] = []


setup called only once, and those variables won't be reset for each scenario. I suggest moving them into handleBeforeHook:

Suggested change

public override func setup() {

startCucumberHookEventsListening()

var cryptorKind: String = ""

var cipherKey: String = ""

var randomIV: Bool = true

var otherCryptors: [String] = []

var cryptorKind: String!

var cipherKey: String!

var randomIV: Bool = true

var otherCryptors: [String] = []

override public func handleBeforeHook() {

cryptorKind = ""

cipherKey = ""

randomIV = true

otherCryptors = []

super.handleBeforeHook()

}

public override func setup() {

startCucumberHookEventsListening()

parfeon · 2023-10-02T18:01:43Z

Tests/PubNubContractTest/Steps/CryptorModule/PubNubCryptoModuleContractTestSteps.swift

+    When("I decrypt '(.*)' file") { args, userInfo in
+      let fileName = args?.first ?? ""
+      let localUrl = self.localUrl(for: fileName)
+      let inputStream = InputStream(url: localUrl)!
+      let outputUrl = self.generateTestOutputUrl()
+
+      let cryptorModule = self.createCryptorModule(cryptorKind, key: cipherKey, withRandomIV: randomIV)
+      let encryptedStreamData = EncryptedStreamResult(stream: inputStream, contentLength: localUrl.sizeOf)
+      let decryptingRes = cryptorModule.decrypt(stream: encryptedStreamData, to: outputUrl)
+
+      Then("I receive '(.*)'") { thenArgs, _ in
+        switch thenArgs?.first ?? "" {
+        case "unknown cryptor error":
+          XCTAssertTrue(self.failureIfAny(from: decryptingRes)?.reason == .unknownCryptorError)
+        case "decryption error":
+          XCTAssertTrue(self.failureIfAny(from: decryptingRes)?.reason == .decryptionError)
+        case "success":
+          XCTAssertNotNil(try? decryptingRes.get())
+        default:
+          XCTFail("Unsupported outcome")
+        }
+      }
+    }


Shouldn't it be like follows and decryptingRes be declared as instance variable (as above):

Suggested change

When("I decrypt '(.*)' file") { args, userInfo in

let fileName = args?.first ?? ""

let localUrl = self.localUrl(for: fileName)

let inputStream = InputStream(url: localUrl)!

let outputUrl = self.generateTestOutputUrl()

let cryptorModule = self.createCryptorModule(cryptorKind, key: cipherKey, withRandomIV: randomIV)

let encryptedStreamData = EncryptedStreamResult(stream: inputStream, contentLength: localUrl.sizeOf)

let decryptingRes = cryptorModule.decrypt(stream: encryptedStreamData, to: outputUrl)

Then("I receive '(.*)'") { thenArgs, _ in

switch thenArgs?.first ?? "" {

case "unknown cryptor error":

XCTAssertTrue(self.failureIfAny(from: decryptingRes)?.reason == .unknownCryptorError)

case "decryption error":

XCTAssertTrue(self.failureIfAny(from: decryptingRes)?.reason == .decryptionError)

case "success":

XCTAssertNotNil(try? decryptingRes.get())

default:

XCTFail("Unsupported outcome")

}

}

}

When("I decrypt '(.*)' file") { args, userInfo in

let fileName = args?.first ?? ""

let localUrl = self.localUrl(for: fileName)

let inputStream = InputStream(url: localUrl)!

let outputUrl = self.generateTestOutputUrl()

let cryptorModule = self.createCryptorModule(cryptorKind, key: cipherKey, withRandomIV: randomIV)

let encryptedStreamData = EncryptedStreamResult(stream: inputStream, contentLength: localUrl.sizeOf)

let decryptingRes = cryptorModule.decrypt(stream: encryptedStreamData, to: outputUrl)

}

Then("I receive '(.*)'") { thenArgs, _ in

switch thenArgs?.first ?? "" {

case "unknown cryptor error":

XCTAssertTrue(self.failureIfAny(from: decryptingRes)?.reason == .unknownCryptorError)

case "decryption error":

XCTAssertTrue(self.failureIfAny(from: decryptingRes)?.reason == .decryptionError)

case "success":

XCTAssertNotNil(try? decryptingRes.get())

default:

XCTFail("Unsupported outcome")

}

}

parfeon · 2023-10-02T18:05:24Z

Tests/PubNubContractTest/Steps/CryptorModule/PubNubCryptoModuleContractTestSteps.swift

+    When("I encrypt '(.*)' file as 'binary'") { args, userInfo in
+      let fileName = args?.first ?? ""
+      let cryptorModule = self.createCryptorModule(cryptorKind, key: cipherKey, withRandomIV: randomIV)
+      let localFileUrl = self.localUrl(for: fileName)
+      let inputData = try! Data(contentsOf: localFileUrl)
+      let encryptedDataRes = cryptorModule.encrypt(data: inputData)
+
+      Then("Successfully decrypt an encrypted file with legacy code") { _, _ in
+        let decryptedData = try! cryptorModule.decrypt(data: try! encryptedDataRes.get()).get()
+        XCTAssertEqual(inputData, decryptedData)
+      }
+      Then("I receive 'encryption error'") { _, _ in
+        guard case .failure(let failure) = encryptedDataRes else {
+          XCTFail("Encryption error is expected"); return;
+        }
+        XCTAssertTrue(failure.reason == .encryptionError)
+      }
+    }


I know that it is possible to declare it this way, but it makes create false impression that nested steps definitions will be related only to this When:

Suggested change

When("I encrypt '(.*)' file as 'binary'") { args, userInfo in

let fileName = args?.first ?? ""

let cryptorModule = self.createCryptorModule(cryptorKind, key: cipherKey, withRandomIV: randomIV)

let localFileUrl = self.localUrl(for: fileName)

let inputData = try! Data(contentsOf: localFileUrl)

let encryptedDataRes = cryptorModule.encrypt(data: inputData)

Then("Successfully decrypt an encrypted file with legacy code") { _, _ in

let decryptedData = try! cryptorModule.decrypt(data: try! encryptedDataRes.get()).get()

XCTAssertEqual(inputData, decryptedData)

}

Then("I receive 'encryption error'") { _, _ in

guard case .failure(let failure) = encryptedDataRes else {

XCTFail("Encryption error is expected"); return;

}

XCTAssertTrue(failure.reason == .encryptionError)

}

}

When("I encrypt '(.*)' file as 'binary'") { args, userInfo in

let fileName = args?.first ?? ""

let cryptorModule = self.createCryptorModule(cryptorKind, key: cipherKey, withRandomIV: randomIV)

let localFileUrl = self.localUrl(for: fileName)

let inputData = try! Data(contentsOf: localFileUrl)

let encryptedDataRes = cryptorModule.encrypt(data: inputData)

}

Then("Successfully decrypt an encrypted file with legacy code") { _, _ in

let decryptedData = try! cryptorModule.decrypt(data: try! encryptedDataRes.get()).get()

XCTAssertEqual(inputData, decryptedData)

}

Then("I receive 'encryption error'") { _, _ in

guard case .failure(let failure) = encryptedDataRes else {

XCTFail("Encryption error is expected"); return;

}

XCTAssertTrue(failure.reason == .encryptionError)

}

if a suggestion will be included, it will require that encryptedDataRes to be declared as instance variable.

parfeon · 2023-10-02T18:16:52Z

Sources/PubNub/PubNubConfiguration.swift

-  public var cipherKey: Crypto?
+  public var cryptorModule: CryptorModule?


Wouldn't this change be a breaking change?
Can we mark it as deprecated and, if users provide it, use Crypto field values to construct CryptoModule under the hood and use it further with endpoints?

This should be possible I think, will restore Crypto and its fields

parfeon · 2023-10-02T19:26:55Z

Sources/PubNub/Helpers/Crypto/Cryptors/LegacyCryptor.swift

+        iv = try CryptorVector.fixed.data()
+        cipherText = data.data
+      }
+


Should we have some more check here for case, when cipherText is empty after we extracted IV from it?

parfeon · 2023-10-02T19:30:54Z

Sources/PubNub/Helpers/Crypto/Header/CryptorHeaderWithinStreamFinder.swift

+    default:
+      continuationStream = MultipartInputStream(
+        inputStreams: [
+          InputStream(data: possibleHeaderBytes.suffix(from: noOfBytesProcessedByParser)),


Can we just use noOfBytesProcessedByParser here instead of suffix offset computation? Shouldn't this variable store data which hasn't been processed as header?

parfeon · 2023-10-02T19:37:11Z

Sources/PubNub/Helpers/Crypto/Miscellaneous/CryptoInputStream.swift


 /// A stream that provides read-only stream functionality while performing crypto operations
 public class CryptoInputStream: InputStream {
  // swiftlint:disable:previous type_body_length
-
+  public struct DataSource {


So, right now CrytoInputStream will work only for our cryptors? In future, users also would like to have some help from use in handling stream chunks reading.

I still preserved this class as public so customers can use it. What's changed is the input that this class takes. I decoupled "old" Crypto from this class so now customers may create its instances without any dependencies to struct/classes we're going to deprecate.

parfeon · 2023-10-02T19:38:19Z

Sources/PubNub/Helpers/Crypto/Miscellaneous/CryptoInputStream.swift

+    let blockSize: Int
+
+    func outputSize(from inputBytes: Int) -> Int {
+      inputBytes + (blockSize - (inputBytes % blockSize))


CCCrypto actually can tell you what resulting size you will get (depending on from operation type and source data size).

parfeon · 2023-10-02T19:40:38Z

Sources/PubNub/Helpers/Crypto/Miscellaneous/CryptoInputStream.swift

+    if includeInitializationVectorInContent {
+      rawDataLength = contentLength - (operation == .decrypt ? crypto.cipher.blockSize : 0)
+      // The estimated content length is the IV length plus the crypted length
+      estimatedCryptoCount = crypto.cipher.blockSize + crypto.cipher.outputSize(from: rawDataLength)


As mentioned above, CCCrypto can help with estimated output size computation.

I moved it from the current master implementation that's available here. The reason I moved this code was not to alter/modify CryptoStream or CryptoInputStream already present before the more it's necessary.

CryptoStream looks like a wrapper for Swift's CommonCrypto operations. In general, I agree with you it's fine to use CommonCrypto's operations directly in the CryptoInputStream implementation - since CryptoInputStream is already tightly coupled with Swift's CommonCrypto, so IMHO there's nothing that CryptoStream abstracts away.

I can try to use CCCOperations directly in CryptoInputStream, but I'm not sure yet what would be the scope of changes after introducing this idea.

* Fixing readability of contract tests * Fixing reasons (PubNubError) for Crypto errors * Fixed representation for the CryptorHeader v1 * Restoring PubNub's extension for encryption/decryption * Restoring original Crypto

* Returning MultipartInputStream as the result of stream encryption * Removing EncryptedStreamDataResult

* Fixed encrypt() and decrypt() methods for PubNub class * Removed encoding from CryptorModule

Sources/PubNub/Helpers/Crypto/CryptoError.swift

Sources/PubNub/Helpers/Crypto/CryptorModule.swift

parfeon · 2023-10-05T07:25:39Z

Sources/PubNub/Helpers/Crypto/CryptorModule.swift

-  /// - Returns: A success, storing an ``EncryptedStreamResult`` value if operation succeeds. Otherwise, a failure storing `PubNubError` is returned
-  public func encrypt(stream: InputStream, contentLength: Int) -> Result<EncryptedStreamResult, PubNubError> {
+  /// - Returns: A success, storing a `MultipartInputStream` value if operation succeeds. Otherwise, a failure storing `PubNubError` is returned
+  public func encrypt(stream: InputStream, contentLength: Int) -> Result<MultipartInputStream, PubNubError> {


Is it possible in case of success to return InputStream (caller shouldn't be aware about actual implementation)?

This method is also used for File uploading, so we also need to have encrypted stream length to set proper value for HTTP Content-Header field. Where do you store such length?

parfeon · 2023-10-05T09:47:42Z

Sources/PubNub/Helpers/Streams/MultipartInputStream.swift

@@ -28,16 +28,18 @@
 import Foundation

 /// An `InputStream` that can combine multiple streams into a single stream
-class MultipartInputStream: InputStream {
+public class MultipartInputStream: InputStream {


Do we need to make it public? Will be there any benefits for users to using it as MultipartInputStream?

Earlier, this class was created to provide support of multipart-form body, which may consist of sections represented by separate streams with data.

I thought it was your idea during the last review 🙂 The whole issue is about setting Content-Length when uploading a File, so CryptorModule method responsible for stream encryption has to return something more than InputStream only. Where do you prefer having encrypted stream length stored?

parfeon · 2023-10-05T09:53:28Z

Sources/PubNub/Helpers/Crypto/Header/CryptorHeaderWithinStreamFinder.swift

+      guard headerLength + dataLength < buffer.count else {
+        throw PubNubError(.decryptionFailure, additional: ["Cannot extract metadata for CryptorHeader v1"])
+      }
+      // Extracts Cryptor-defined data from the supplied buffer
+      cryptorDefinedData = buffer.subdata(in: headerLength..<headerLength + dataLength)


Just a note, that there can be no cryptor-defined data at all or custom implementation will handle it in a different way.

I think I will rework that when implementing another Cryptor. Fortunately, that's the internal implementation so it won't break customers in terms of any API. The question is where do you extract metadata for V1 header? Is it inside CryptorModule or somewhere else?

Since CryptoModule after data pre-processing (header extraction) calls to actual cryptor implementations which expect specific types (encrypt expects Data and decrypt expects EncrpyptedData which contains cryptor-defined portion of data and encrypted data). It is module who extracts metadata and puts it into EncrpyptedData.

Co-authored-by: Serhii Mamontov <[email protected]>

* Provide minimum input for old Crypto to stay backward-compatible * InputStream instead of MultipartInputStream

parfeon

LGTM!

…ctor

jguz-pubnub · 2023-10-16T11:02:47Z

@pubnub-release-bot release as v6.2.0

pubnub-release-bot · 2023-10-16T16:33:22Z

🛑 Build failed. Please check release build for details 🛑

jguz-pubnub added 2 commits September 21, 2023 19:17

CryptorModule

78da753

* Reusing existing CryptoStream + CryptoInputStream by removing Crypto dependency from them * Moved old encryption/decryption to LegacyCryptor * Introduced new AESCBCCryptor * Introduced new CryptorModule instead of Crypto * Fixing tests

Contract tests implemented

988f8ae

jguz-pubnub requested a review from parfeon as a code owner September 25, 2023 15:24

jguz-pubnub added 4 commits September 26, 2023 09:02

Added comments for CryptorHeaderWithinStreamFinder and provides more …

b57f156

…readable error descriptions

Method for decrypting an InputStream from CryptorModule now takes one…

7726ea6

… less parameter

Prevent from appending CryptorHeader for LegacyCryptor used as an enc…

83ba71f

…oder

CryptorModule

2cb46f9

* Prevent from decoding & encoding empty Data & Files * Added inline documentation for public items

jguz-pubnub requested a review from techwritermat October 2, 2023 16:21

parfeon reviewed Oct 2, 2023

View reviewed changes

jguz-pubnub added 3 commits October 4, 2023 09:12

Fixes according to review (in progress)

e82f02f

* Fixing readability of contract tests * Fixing reasons (PubNubError) for Crypto errors * Fixed representation for the CryptorHeader v1 * Restoring PubNub's extension for encryption/decryption * Restoring original Crypto

Fixes according to review (in progress #2)

c5186ec

* Returning MultipartInputStream as the result of stream encryption * Removing EncryptedStreamDataResult

Fixes

71416bc

* Fixed encrypt() and decrypt() methods for PubNub class * Removed encoding from CryptorModule

jguz-pubnub requested a review from josh-lubliner October 5, 2023 08:26

parfeon requested changes Oct 5, 2023

View reviewed changes

jguz-pubnub and others added 3 commits October 5, 2023 12:06

Update Sources/PubNub/Helpers/Crypto/CryptorModule.swift

5d2868f

Co-authored-by: Serhii Mamontov <[email protected]>

Fixes

d814f5d

* Provide minimum input for old Crypto to stay backward-compatible * InputStream instead of MultipartInputStream

POC for improving CryptoInputStream and computing output length

facc3c4

jguz-pubnub removed the request for review from josh-lubliner October 5, 2023 12:52

parfeon approved these changes Oct 5, 2023

View reviewed changes

jguz-pubnub added 3 commits October 6, 2023 12:44

* Provide both cipherKey: and cryptorModule: for the PNConfig constru…

bdc2c1e

…ctor

* Preserves setting cipherKey for backward compatibility

1876e56

LICENSE + minor fixes for inline documentation

5d2c954

jguz-pubnub force-pushed the feat/cryptors branch from 0b4787e to 5d2c954 Compare October 16, 2023 10:13

PubNub SDK v6.2.0 release.

97bb528

jguz-pubnub merged commit 56c8687 into master Oct 16, 2023
10 checks passed

jguz-pubnub deleted the feat/cryptors branch October 16, 2023 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CryptorModule #144

CryptorModule #144

jguz-pubnub commented Sep 25, 2023 •

edited

Loading

parfeon Oct 2, 2023

parfeon Oct 2, 2023

parfeon Oct 2, 2023

parfeon Oct 2, 2023

parfeon Oct 2, 2023

jguz-pubnub Oct 3, 2023

parfeon Oct 2, 2023

parfeon Oct 2, 2023

parfeon Oct 2, 2023

jguz-pubnub Oct 3, 2023

parfeon Oct 2, 2023

parfeon Oct 2, 2023

jguz-pubnub Oct 3, 2023

parfeon Oct 5, 2023

jguz-pubnub Oct 5, 2023

parfeon Oct 5, 2023

jguz-pubnub Oct 5, 2023

parfeon Oct 5, 2023

jguz-pubnub Oct 5, 2023

parfeon Oct 5, 2023

parfeon left a comment

jguz-pubnub commented Oct 16, 2023

pubnub-release-bot commented Oct 16, 2023

		public var cipherKey: Crypto?
		public var cryptorModule: CryptorModule?

CryptorModule #144

CryptorModule #144

Conversation

jguz-pubnub commented Sep 25, 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

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

parfeon left a comment

Choose a reason for hiding this comment

jguz-pubnub commented Oct 16, 2023

pubnub-release-bot commented Oct 16, 2023

jguz-pubnub commented Sep 25, 2023 •

edited

Loading