[87] Implement direct encryption and symmetric JWKs #92

ghost · 2018-07-17T11:42:50Z

Resolves #87.

This implements direct encryption for JWEs as well as symmetric JWKs.

Usage

This is a made up example to show off all new functionality.

let data = "🔥🌵🕶".data(using: .utf8)!

let random = try! SecureRandom.generate(count: ...)
let jwk = SymmetricKey(key: random)
let keyData = jwk.converted(to: Data.self)

let header = JWEHeader(algorithm: .direct, encryptionAlgorithm: .A256CBCHS512)
let payload = Payload(data)
let encrypter = Encrypter(keyEncryptionAlgorithm: .direct, encryptionKey: keyData, contentEncyptionAlgorithm: .A256CBCHS512)!

let jwe = try! JWE(header: header, payload: payload, encrypter: encrypter)

let serialization = jwe.compactSerializedString // eyJlbmMiOiJBMjU2Q0JDLUhTNTEyIiwiYWxnIjoiZGlyIn0..HUTNQ9m2Z8Q77tQJhLs5gg.DWQCCkrCPFeZ2-65L9__z83N1exh4oVIk4rOO2_v1eE.8sOW54Soupo_-TdXg5A9qXvokaHzS8cGb__ca3MvuEo

API Changes

The main Encrypter and Decrypter initializers changed from

init?(keyEncryptionAlgorithm: AsymmetricKeyAlgorithm, keyEncryptionKey kek: KeyType, contentEncyptionAlgorithm: SymmetricKeyAlgorithm)

// to

init?(keyEncryptionAlgorithm: AsymmetricKeyAlgorithm, encryptionKey key: KeyType, contentEncyptionAlgorithm: SymmetricKeyAlgorithm) {

and from

init?<KeyType>(keyDecryptionAlgorithm: AsymmetricKeyAlgorithm, keyDecryptionKey kdk: KeyType, contentDecryptionAlgorithm: SymmetricKeyAlgorithm)

// to

init?<KeyType>(keyDecryptionAlgorithm: AsymmetricKeyAlgorithm, decryptionKey key: KeyType, contentDecryptionAlgorithm: SymmetricKeyAlgorithm)

respectively.

The old initializers are still available (altough marked as deprecated), so we can release this as a minor version bump with no breaking API changes.

Regarding the weird Xcode file: Apparently it should be commited.

Xcode 9.3 adds a new IDEWorkspaceChecks.plist file to a workspace's shared data, to store the state of necessary workspace checks. Committing this file to source control will prevent unnecessary rerunning of those checks for each user opening the workspace. (37293167

carol-mohemian

Nice work @daniel-mohemian 👏 Looks really good! Found some typos and have 1-2 questions.

carol-mohemian · 2018-07-18T07:56:14Z

JOSESwift/Sources/CryptoImplementation/DataSymmetricKey.swift

@@ -0,0 +1,34 @@
+//
+//  DataRSAPublicKey.swift


DataSymmetricKey

carol-mohemian · 2018-07-18T07:57:01Z

JOSESwift/Sources/CryptoImplementation/SecureRandom.swift

@@ -24,17 +24,17 @@
 import Foundation
 import Security

-internal enum SecureRandomError: Error {
+public enum SecureRandomError: Error {


carol-mohemian · 2018-07-18T08:41:40Z

JOSESwift/Sources/RSADecrypter.swift


    func decrypt(_ ciphertext: Data) throws -> Data {
+        guard let privateKey = privateKey else {
+            return Data()


This is awesome 👌 Couldn't remember why we returned plain Data(). Maybe write a comment that we need a empty octet for direct encryption and therefore Data() is returned. What do you think?

carol-mohemian · 2018-07-18T08:42:23Z

JOSESwift/Sources/SymmetricKeyCodable.swift

@@ -0,0 +1,78 @@
+//
+//  RSAKeyCodable.swift


SymmetricKeyCodable

carol-mohemian · 2018-07-18T08:45:07Z

JOSESwift/Sources/SymmetricKeys.swift

@@ -0,0 +1,120 @@
+//
+//  RSAKeys.swift


SymmetricKeys

carol-mohemian · 2018-07-18T09:03:05Z

Tests/SymmetricKeyTests.swift

+
+class SymmetricKeyTests: XCTestCase {
+
+    func testCreatingSymetricKeyFromData() {


typo: testCreatingSymmetricKeyFromData

carol-mohemian · 2018-07-18T09:04:25Z

Tests/SymmetricKeyTests.swift

+        let json = try! SymmetricKey(
+            key: key,
+            additionalParameters: [ "alg": SymmetricKeyAlgorithm.A256CBCHS512.rawValue ]
+            ).jsonData()!


Formatting.

carol-mohemian · 2018-07-18T09:06:21Z

Tests/SymmetricKeyTests.swift

+    }
+
+    func testDecodingFromJSONWithMissingKeyType() {
+        let json = "{\"alg\":\"A256CBC-HS512\",\"k\":\"+++==notbase64url==---\"}".data(using: .utf8)!


Maybe use a correct key otherwise you don't know if it throws because of the missing keytype or the not base64URL key.

Good catch!

carol-mohemian · 2018-07-18T09:06:34Z

Tests/SymmetricKeyTests.swift

+    }
+
+    func testDecodingFromJSONWithWrongKeyType() {
+        let json = "{\"kty\":\"RSA\",\"alg\":\"A256CBC-HS512\",\"k\":\"+++==notbase64url==---\"}".data(using: .utf8)!


Same as above.

carol-mohemian · 2018-07-18T09:06:55Z

Tests/SymmetricKeyTests.swift

+    }
+
+    func testParsingSymmetricKeyFromOtherKeyRepresentation() {
+        let key: ExpressibleAsSymmetricKeyComponents = Data(bytes: [


Why is this different to the above test? 🤔

The first tests initializing the key directly from Data. The second from something that is ExpressibleAsSymmetricKeyComponents. Currently, only Data conforms to that protocol so it's basically the same, but uses two different initializers.

If you compare it to the RSA keys, there SecKey and Data are ExpressibleAsRSAPublicKeyComponents but the key can only be initialized directly from modulus/exponent as Strings.

# Conflicts: # JOSESwift/Sources/CryptoImplementation/RSA.swift

ghost · 2018-07-18T15:47:25Z

@carol-mohemian Thanks for the feedback! I addressed all your comments (I hope 😄).

carol-mohemian · 2018-07-19T07:59:31Z

Nice 👍 Would love to see a review by @gigi-mohemian as well.

mohemian-92817281

Awesome job @daniel-mohemian 👏

I have some very minor comments, none of them critical in any way.

mohemian-92817281 · 2018-07-19T11:57:32Z

JOSESwift/Sources/Decrypter.swift

+    ///   - key: The key used to perform the decryption. If the `keyDecryptionAlgorithm` is `.direct`, the
+    ///          `decryptionKey` is the shared symmetric content encryption key. Otherwise the `decryptionKey` is the
+    ///           private key of the receiver. See [RFC-7516](https://tools.ietf.org/html/rfc7516#section-5.2) for
+    ///           details.


mohemian-92817281 · 2018-07-19T12:01:21Z

JOSESwift/Sources/Decrypter.swift

        switch (keyDecryptionAlgorithm, contentDecryptionAlgorithm) {
        case (.RSA1_5, .A256CBCHS512):
-            guard type(of: kdk) is RSADecrypter.KeyType.Type else {
+            guard type(of: key) is RSADecrypter.KeyType.Type else {


Could this also be a guard let ... as? so that we don't need the force-cast below?

That's a very good point. I did some testing and yes, normally that would work:

func printType<T>(of key: T) { if let _ = key as? Data { print("Data") } } printType(of: Data()) // prints "Data"

However, doing the same with SecKey (if let _ = key as? SecKey) gives the following error:

error: conditional downcast to CoreFoundation type 'SecKey' will always succeed

After more testing, this seems to be the situation:

For normal Swift types, we could do optional let bindings as you suggested

The current check type(of: key) is SecKey will succeed for all CoreFoundation types (SecKey, CFString, ...) but not for Swift types.

So if one passes Data() as RSA key it will detect the wrong type, but if one passes e.g. a CFString, it won't detect the error. The correct way of checking CoreFoundation types would be to compare CFGetTypeID(key as CFTypeRef to stuff like SecKeyGetTypeID(). However, I currently don't know how we should solve that without knowing what *GetTypeID() function to call (since the KeyType is generic and CFGetTypeID() only seems to work on AnyObjects and not on types itself).

Since that's a bug not directly introduced by this pull request and it does not affect direct encryption, are you guys ok with fixing it separately? If yes, I'll track an issue. cc @carol-mohemian, @gigi-mohemian

Thanks a lot for the thorough explanation!

Fixing this separately as you suggested is the way to go I'd say 👍

mohemian-92817281 · 2018-07-19T12:02:24Z

JOSESwift/Sources/Decrypter.swift

            self.symmetric = AESDecrypter(algorithm: contentDecryptionAlgorithm)
+        case (.direct, .A256CBCHS512):
+            guard type(of: key) is AESDecrypter.KeyType.Type else {


Same here: could this be a guard let ... as??

Also two more times in Encrypter.swift

See above ☝️

mohemian-92817281 · 2018-07-19T12:11:38Z

JOSESwift/Sources/JWK.swift

@@ -41,6 +41,7 @@ internal enum JWKError: Error {
 /// - RSA
 public enum JWKKeyType: String, Codable {
    case RSA = "RSA"
+    case SYM = "oct"


mohemian-92817281 · 2018-07-19T12:13:59Z

JOSESwift/Sources/JWKSetCodable.swift

            }

-            keys.append(rsaKey)
+            throw DecodingError.dataCorruptedError(in: keyContainer, debugDescription: """
+                No RSAPrivateKey, RSAPublicKey, or SymmetricKey found to decode.


mohemian-92817281 · 2018-07-19T12:31:46Z

JOSESwift/Sources/SymmetricKeyCodable.swift

+                JWKParameter.keyType,
+                DecodingError.Context.init(
+                    codingPath: [JWKParameter.keyType],
+                    debugDescription: "Key Type parameter wrong."


I would opt for "Wrong parameter: key type" - but the above is okay as well 👍

The title case of Key Type is what caught my eye.

mohemian-92817281 · 2018-07-19T12:44:28Z

JOSESwift/Sources/SymmetricKeys.swift

+    ///   - key: The octet sequence containing the key data.
+    ///   - parameters: Additional JWK parameters.
+    public init(key: Data, additionalParameters parameters: [String: String] = [:]) {
+        self.keyType = .SYM


Are we sure about the naming here? While I don't actually disagree with the naming I was confused for a short while because SYM = "oct" 🤔

What do you think about going full on Symmetric so it's not confused with an abbreviation or using OCT directly? Just thinking out aloud here 😅

Good point, I agree. 👍

I changed it to .OCT. (Nimbus also uses OCT btw.)

mohemian-92817281 · 2018-07-19T13:05:59Z

Tests/JWEDirectEncryptionTests.swift

+          4, 122,  29, 230, 151,  12, 244, 127,
+        121,  25,   4,  85, 220, 144, 215, 110,
+        130,  17,  68, 228, 129, 138,   7, 130
+    ])


Awesome formatting 👆

👌

ghost · 2018-07-22T19:18:13Z

Thx @gigi-mohemian for the review.

* master: [87] Implement direct encryption and symmetric JWKs (#92) # Conflicts: # .gitignore # Tests/JWERSATests.swift

daniel-moh added 23 commits July 4, 2018 16:12

Add direct encryption sketch

cb81fcb

Change encryptor initializer parameter name

2e197f4

Add symmetric decryption

983c60a

Make base64url and secure random public

175eff0

[87] Implement symmetric JWKs

fa6abc0

Merge branch 'master' into feature/87-direct-encryption

c66b2f3

[87] Update gitignore

3885490

[87] Remove shared data file

766d28b

[87] Commit workspace checks

7534685

[87] Add basic test suite for direct encryption

8803df5

[87] Clean up encrypter

b8c97a6

[87] Add swiftlint exception

f7fe8e7

[87] Update decryption

463d0ee

[87] Extend direct encryption tests

1612515

[87] Add old encrypter initializer for compatability

c72cbf6

[87] Update tests fro encrypter init

e98b98c

[87] Add old decrypter init for comapatibility

93b5358

[87] Update decrypter tests

57905c2

[87] Update header tests for direct

73ccb00

[87] Update sym key documentation

ca6c51f

[87] Add symmetric JWK tests

b3d4f39

[87] Implement symmetirc jek set support and tests

9d26774

Update readme

098032d

ghost self-assigned this Jul 17, 2018

ghost requested review from mohemian-92817281, carol-mohemian, haeser and markus-mohemian July 17, 2018 11:42

ghost added JWE labels Jul 17, 2018

ghost added the feature label Jul 17, 2018

carol-mohemian suggested changes Jul 18, 2018

View reviewed changes

carol-mohemian mentioned this pull request Jul 18, 2018

WIP: Add RSAES-OAEP support #95

Merged

daniel-moh added 8 commits July 18, 2018 17:22

[87] Fix comment headers

83e695e

[87] Fix more comments and some typos

9a32193

[87] Fix formatting

477aaab

[87] Formatting

f277ba7

[87] Add empty encrypted key commetn

16352e2

[87] Formatting

fb2bc6d

[87] Add json to key doc

f30390f

Merge branch 'master' into feature/87-direct-encryption

ebac1d7

# Conflicts: # JOSESwift/Sources/CryptoImplementation/RSA.swift

mohemian-92817281 reviewed Jul 19, 2018

View reviewed changes

daniel-moh added 2 commits July 22, 2018 21:06

[87] Update error message

2fafa92

[87] Rename key type to OCT

706658b

carol-mohemian approved these changes Jul 30, 2018

View reviewed changes

mohemian-92817281 approved these changes Jul 30, 2018

View reviewed changes

ghost merged commit f390e05 into master Jul 30, 2018

ghost deleted the feature/87-direct-encryption branch July 30, 2018 09:11

mohemian-92817281 added a commit that referenced this pull request Jul 30, 2018

Merge branch 'master' into feature/JOSE-80-improve-tests

f307b16

* master: [87] Implement direct encryption and symmetric JWKs (#92) # Conflicts: # .gitignore # Tests/JWERSATests.swift

ghost mentioned this pull request Aug 30, 2018

Prepare 1.2.0 #104

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[87] Implement direct encryption and symmetric JWKs #92

[87] Implement direct encryption and symmetric JWKs #92

ghost commented Jul 17, 2018 •

edited by mohemian-92817281

Loading

carol-mohemian left a comment

carol-mohemian Jul 18, 2018

carol-mohemian Jul 18, 2018

carol-mohemian Jul 18, 2018

carol-mohemian Jul 18, 2018

carol-mohemian Jul 18, 2018

carol-mohemian Jul 18, 2018

carol-mohemian Jul 18, 2018

carol-mohemian Jul 18, 2018

ghost Jul 18, 2018

carol-mohemian Jul 18, 2018

carol-mohemian Jul 18, 2018

ghost Jul 18, 2018

ghost commented Jul 18, 2018

carol-mohemian commented Jul 19, 2018

mohemian-92817281 left a comment

mohemian-92817281 Jul 19, 2018

mohemian-92817281 Jul 19, 2018

ghost Jul 22, 2018 •

edited by ghost

Loading

mohemian-92817281 Jul 30, 2018

mohemian-92817281 Jul 19, 2018

ghost Jul 21, 2018

mohemian-92817281 Jul 19, 2018

mohemian-92817281 Jul 19, 2018

mohemian-92817281 Jul 19, 2018

mohemian-92817281 Jul 19, 2018

ghost Jul 22, 2018

mohemian-92817281 Jul 19, 2018

ghost commented Jul 22, 2018


		class SymmetricKeyTests: XCTestCase {

		func testCreatingSymetricKeyFromData() {

[87] Implement direct encryption and symmetric JWKs #92

[87] Implement direct encryption and symmetric JWKs #92

Conversation

ghost commented Jul 17, 2018 • edited by mohemian-92817281 Loading

Usage

API Changes

carol-mohemian left a 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

ghost commented Jul 18, 2018

carol-mohemian commented Jul 19, 2018

mohemian-92817281 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jul 22, 2018 • edited by ghost 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

ghost commented Jul 22, 2018

ghost commented Jul 17, 2018 •

edited by mohemian-92817281

Loading

ghost Jul 22, 2018 •

edited by ghost

Loading