[JWE] Added support for `A256GCM` and `A128GCM` #278

tobihagemann · 2022-08-12T15:59:48Z

After considering the comments in #251, I've added support for content encryption using AES GCM according to JWE standard. As I've mentioned there, I increased the minimum iOS target to 13.0 because of CryptoKit.

istarp · 2022-11-01T21:28:55Z

Thanks for this PR, any thoughts on completing it and releasing new lib version?

tobihagemann · 2022-11-02T10:50:17Z

From my perspective, it's completed. I'm just waiting for a review.

istarp · 2022-11-02T14:22:10Z

I hope someone will take a look, seems like most of the PR's are not reviewed at all :(. It would be awesome to have this functionality as we are currently integrating Play Services Integrity check (Google version of device check from apple) and need this to decrypt token as it uses A256GCM

antoniopantaleo · 2023-06-12T08:07:04Z

Hello 👋🏽
Are any chances this PR will be merged?

kiransable279 · 2023-08-25T11:26:42Z

After considering the comments in #251, I've added support for content encryption using AES GCM according to JWE standard. As I've mentioned there, I increased the minimum iOS target to 13.0 because of CryptoKit.

Any update on this. Please include AES128GCM and AES256GCM content encryption algorithm. It would be very helpful

lsolniczek · 2023-09-25T13:20:26Z

Hi, any update on this? It will be very useful to have this PR merged.

Thireus · 2023-10-20T09:58:18Z

Looks like it fails lint tests - @tobihagemann

tobihagemann · 2023-10-24T13:19:59Z

Fixed linter warning and brought branch up-to-date. But I'm not sure if these factors were actually blockers for a review. 😅

sfostermc · 2024-01-23T20:58:59Z

Is there anything we can do to help get this PR merged in?

…tandard

…l input paths

…nd inline them into the appropriate encrypter

daniel-moh

Very nice! Couple of comments, let me know what you think!

daniel-moh · 2024-05-15T09:32:22Z

JOSESwift/Sources/AESCBCEncryption.swift

+        switch contentEncryptionAlgorithm {
+        case .A256CBCHS512:
+            self.hmacKey = contentEncryptionKey.subdata(in: 0..<32)
+            self.contentEncryptionKey = contentEncryptionKey.subdata(in: 32..<64)


Naming wise I think it's potentially a bit dangerous (or at the very least confusing) to have the same name contentEncryptionKey for two different things.

What comes in via the initializer parameter is the two combined keys (message authentication and encryption key) while what is set here as a property of the AESCBCEncryption struct is only the message encryption key (a subset of the other one).

Just to be safe I would say we should distinguish that more clearly by using different naming for the two different keys. Note, for example, that the previous contentEncryptionKey struct property represented the two combined keys while now it only represents the subset that is used for message encryption (but not message authentication). Everything is correctly implemented (because we made the necessary changes in the encryption function below), but let's just be more explicit about it.

In the JWA specification of AES CBC encryption algorithms, they use the following naming:

secret key for the combination of the two keys (authentication and encryption)

for us we could call that secretKey

MAC_KEY/ENC_KEY for the keys derived from the secret key.

for us, we could call them authenticationKey and encryptionKey.

What do you think?

Very valid and valuable feedback. I like the naming secret key for the combined one and will use macKey and encryptionKey for the split ones. Will make the adaptions.

daniel-moh · 2024-05-15T11:38:25Z

JOSESwift/Sources/AESCBCEncryption.swift

+            hmacAlgorithm = .SHA256
+        case .A256GCM, .A128GCM:
+            throw JWEError.contentEncryptionAlgorithmMismatch
+        }


Totally a taste thing, not a change request:

Should we move these two switches to separate functions just like the getAuthenticationTag below? For me personally I feel like it's easier to keep an overview that way. But Both options work for me. Again, it's a taste thing.

externalised both of them to functions. I like it better as well.

daniel-moh · 2024-05-15T11:40:05Z

JOSESwift/Sources/AESGCMEncryption.swift

+    }
+
+    func encrypt(_ plaintext: Data, initializationVector: Data, additionalAuthenticatedData: Data) throws -> ContentEncryptionContext {
+        return try AESGCM.encrypt(plaintext: plaintext, encryptionKey: contentEncryptionKey, initializationVector: initializationVector, additionalAuthenticatedData: additionalAuthenticatedData)


Should we split these calls to multiple lines for easier readability?

Suggested change

return try AESGCM.encrypt(plaintext: plaintext, encryptionKey: contentEncryptionKey, initializationVector: initializationVector, additionalAuthenticatedData: additionalAuthenticatedData)

return try AESGCM.encrypt(

plaintext: plaintext,

encryptionKey: contentEncryptionKey,

initializationVector: initializationVector,

additionalAuthenticatedData: additionalAuthenticatedData

)

done for both calls in the class

daniel-moh · 2024-05-15T11:44:54Z

JOSESwift/Sources/CryptoImplementation/AESGCM.swift

+        return ContentEncryptionContext(
+            ciphertext: encrypted.ciphertext,
+            authenticationTag: encrypted.tag,
+            initializationVector: encrypted.nonce.withUnsafeBytes({ Data(Array($0)) })


Is

encrypted.nonce.withUnsafeBytes({ Data(Array($0)) })

the same as

initializationVector

?

Yes it is. This way all the context was built out of encrypted output.
Changed it to initializationVector for readability now. Also involves fewer conversions.

This way all the context was built out of encrypted output.

That somehow sounds better to me I must admit. Just to double check, does it have any relevance at all if we use the incoming iv or the one from the encryption output?

Refactor switch cases out of initializer

daniel-moh · 2024-05-15T12:27:24Z

JOSESwift/Sources/AESCBCEncryption.swift

+    static func retrieveKeys(for contentEncryptionAlgorithm: ContentEncryptionAlgorithm,
+                             from inputKey: Data) throws -> (hmacKey: Data, encryptionKey: Data) {


Suggested change

static func retrieveKeys(for contentEncryptionAlgorithm: ContentEncryptionAlgorithm,

from inputKey: Data) throws -> (hmacKey: Data, encryptionKey: Data) {

static func retrieveKeys(

for contentEncryptionAlgorithm: ContentEncryptionAlgorithm,

from inputKey: Data

) throws -> (hmacKey: Data, encryptionKey: Data) {

IIRC this is the formatting we were using in other parts of the codebase

😅 I wonder if I can install a formatter to match styles automatically.
Is there something you would recommend?
We could go with https://github.com/apple/swift-format

Yeah we should definitely automate this. IIRC we did some research in the past but I'm sure things changed in the meantime. Defaulting to Apple's formatter surely isn't a bad idea. Probably what Xcode uses under the hood too (I guess) so that might make it easier as well.

daniel-moh

🎉

daniel-moh · 2024-05-15T12:27:38Z

JOSESwift/Sources/AESCBCEncryption.swift

+    static func retrieveKeys(for contentEncryptionAlgorithm: ContentEncryptionAlgorithm,
+                             from inputKey: Data) throws -> (hmacKey: Data, encryptionKey: Data) {


IIRC this is the formatting we were using in other parts of the codebase

tobihagemann mentioned this pull request Apr 12, 2023

Project Maintenance #284

Closed

haeser force-pushed the feature/GCM branch from 22920d4 to 670232c Compare May 7, 2024 14:59

daniel-moh mentioned this pull request May 7, 2024

Clarification on Project Maintenance #305

Closed

tobihagemann added 2 commits May 10, 2024 12:04

Added support for content encryption using AES GCM according to JWE s…

8518bf6

…tandard

Fixed linter warning

5c21a17

haeser force-pushed the feature/GCM branch from 670232c to 5c21a17 Compare May 10, 2024 10:04

haeser added 4 commits May 10, 2024 14:43

Fix some typos across the repo

4742a9e

Refactor nonce generation for readability

6f2c672

Add Tests for asserting AESGCM is not usable in the CommonCrypto impl

a3530c6

Refactor AESGCM test class for DRYness, adding more tests to cover al…

c493377

…l input paths

haeser requested a review from daniel-moh May 10, 2024 12:53

haeser assigned tobihagemann and haeser May 10, 2024

haeser added 8 commits May 10, 2024 14:56

Disable linting rule for test file

7c7bc0e

Merge branch 'master' into feature/GCM

b3de721

Externalize actual crypto operations into dedicated class

9c31aad

Refactor first round of removing specifics from AlgorithmExtensions a…

dd59d47

…nd inline them into the appropriate encrypter

Bubble error up

c34f21e

Add tests for updated initializer

23e26c6

Fix lint issue

2f5f044

Fix linting indentation error

70801ba

daniel-moh reviewed May 15, 2024

View reviewed changes

haeser added 2 commits May 15, 2024 14:18

Refactor to multiline method call for readability

ea81bda

Rename contentEncryptionKey to secretKey AESCBC

af37d86

Refactor switch cases out of initializer

haeser requested a review from daniel-moh May 15, 2024 12:24

daniel-moh reviewed May 15, 2024

View reviewed changes

daniel-moh approved these changes May 15, 2024

View reviewed changes

haeser merged commit 56cbae8 into airsidemobile:master May 15, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JWE] Added support for `A256GCM` and `A128GCM` #278

[JWE] Added support for `A256GCM` and `A128GCM` #278

tobihagemann commented Aug 12, 2022

istarp commented Nov 1, 2022

tobihagemann commented Nov 2, 2022

istarp commented Nov 2, 2022 •

edited

Loading

antoniopantaleo commented Jun 12, 2023 •

edited

Loading

kiransable279 commented Aug 25, 2023 •

edited

Loading

lsolniczek commented Sep 25, 2023

Thireus commented Oct 20, 2023

tobihagemann commented Oct 24, 2023

sfostermc commented Jan 23, 2024

daniel-moh left a comment

daniel-moh May 15, 2024

haeser May 15, 2024

daniel-moh May 15, 2024

haeser May 15, 2024

daniel-moh May 15, 2024

haeser May 15, 2024

daniel-moh May 15, 2024

haeser May 15, 2024

daniel-moh May 15, 2024

daniel-moh May 15, 2024

daniel-moh May 15, 2024

haeser May 15, 2024

daniel-moh May 15, 2024

daniel-moh left a comment

daniel-moh May 15, 2024

		static func retrieveKeys(for contentEncryptionAlgorithm: ContentEncryptionAlgorithm,
		from inputKey: Data) throws -> (hmacKey: Data, encryptionKey: Data) {

[JWE] Added support for A256GCM and A128GCM #278

[JWE] Added support for A256GCM and A128GCM #278

Conversation

tobihagemann commented Aug 12, 2022

istarp commented Nov 1, 2022

tobihagemann commented Nov 2, 2022

istarp commented Nov 2, 2022 • edited Loading

antoniopantaleo commented Jun 12, 2023 • edited Loading

kiransable279 commented Aug 25, 2023 • edited Loading

lsolniczek commented Sep 25, 2023

Thireus commented Oct 20, 2023

tobihagemann commented Oct 24, 2023

sfostermc commented Jan 23, 2024

daniel-moh 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

Choose a reason for hiding this comment

daniel-moh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[JWE] Added support for `A256GCM` and `A128GCM` #278

[JWE] Added support for `A256GCM` and `A128GCM` #278

istarp commented Nov 2, 2022 •

edited

Loading

antoniopantaleo commented Jun 12, 2023 •

edited

Loading

kiransable279 commented Aug 25, 2023 •

edited

Loading