WIP: Add RSAES-OAEP support #95

garrefa · 2018-07-18T13:14:56Z

Still working on it guys. Will also add tests but would be nice to get a quick review.

Wondering about the Encrypter.swift and Decrypter.swift changes. If something else is needed there, can you point me to some documentation?

Thanks

…cipher length check

carol-mohemian

Hey @garrefa, this one was quick 😁👌

Thanks for your contribution, looks really good already 👍
You will add tests for this as well, right?

I also added an issue #96 (read in the RFC that the error messages for decrypting shouldn't state what exactly failed) that we need to change our error handling. Thanks for "indirectly" pointing this out.

carol-mohemian · 2018-07-18T14:15:05Z

JOSESwift/Sources/Algorithms.swift

 public enum AsymmetricKeyAlgorithm: String {
    case RSA1_5 = "RSA1_5"
+    case RSAES_OAEP = "RSAES-OAEP"


A little picky but, I think we should name it like in the JWA rfc, as the underscore in RSA1_5 is only because the algorithm parameter is named like this.

hum... in this case whats your suggestion for the name? The RFC uses RSAES-OAEP or RSAES OAEP, both are not possible in swift enums. RSAESOAEP? just OAEP?

Thanks

Sorry 🤦‍♂️ forgot about that. Looking at the SymmetricKeyAlgorithm, A256CBC-HS512 is written as A256CBCHS512, so RSAESOAEP it is.

You're right @garrefa 🙂 - is not possible in Swift enums.

Guys, the "alg" parameter value is RSA-OAEP not RSAES-OAEP according to the RFC. What do you think of RSA_OAEP of RSAOAEP?

@garrefa If you have any suggestion for a more consistent naming scheme, we'd be happy to adopt it. 😉

Would go for RSAOAEP 😁

Hello guys. I think it's amazing that you decided to fill the gap and create this library. It is super useful for newbies in cartography like me.

I just want to let you know @garrefa that I did some testing of this branch and it seems that JwtSecurityTokenHandler class from .NET framework is not able to decode tokens with "alg": "RSAES-OAEP" parameter in the header, yet it works fine with RSA-OAEP version. I think that it may be connected with @daniel-mohemian's suggestion that according to RFC standard this param should has value RSA-OAEP.

carol-mohemian · 2018-07-18T14:22:50Z

JOSESwift/Sources/Decrypter.swift

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


As there is a PR for direct encryption (see #92) I think this should remain here since the keyDecryptionKey can be of type type Data as well. (Just to avoid that you need to resolve any merge conflicts.)

carol-mohemian · 2018-07-18T14:24:57Z

JOSESwift/Sources/Encrypter.swift

        switch (keyEncryptionAlgorithm, contentEncyptionAlgorithm) {
-        case (.RSA1_5, .A256CBCHS512) :
-            guard type(of: kek) is RSAEncrypter.KeyType.Type else {


Same here for the type check.

carol-mohemian · 2018-07-18T14:25:21Z

JOSESwift/Sources/Encrypter.swift

-            guard type(of: kek) is RSAEncrypter.KeyType.Type else {
-                return nil
-            }
+        case (.RSA1_5, .A256CBCHS512), (.RSAES_OAEP, .A256CBCHS512) :


Whitespace before :

Sorry man, the whitespace before : was already there in the Encrypter.swift file, line 80 and somehow I assumed that was the convention. Im happy to remove it :)

carol-mohemian · 2018-07-18T14:25:36Z

JOSESwift/Sources/Decrypter.swift

-            guard type(of: kdk) is RSADecrypter.KeyType.Type else {
-                return nil
-            }
+        case (.RSA1_5, .A256CBCHS512), (.RSAES_OAEP, .A256CBCHS512) :


Whitespace before :

ghost

Hi @garrefa 👋

Great job!

I think that should be all good in the Decrypter and Encrypter. Those switch statements may be a little bit cumbersome and we'll probably refactor those as more algorithms come along. But for right now, I think they're fine.

I tried creating a JWE with your patch but got a plainTextLengthNotSatisfied error. Does encrypting already work for you?

Regarding the tests: A test suite with some test data from the RFCs or from a large JOSE library would be awesome. 🙏 🎉

garrefa · 2018-07-19T08:16:07Z

Thanks @carol-mohemian, @daniel-mohemian

For sure Ill add the tests mentioned in the RFC.

@daniel-mohemian I did not try to use it yet but I'll check. Maybe related to the change I did in the other MR to add the missing = in the length check? I was wondering how is it possible that it was working before, I thought the blocks should always have the max length and the last block should be padded to match that length. Is it not the case?

Also, I don't want to disturb the reason of this MR but since we are talking about the switch statements Im going to ask. In parallel I'm trying to implement A256-GCM but it seems very complicated because almost all the logic is done considering HMAC will be used and now I need to make things optional and handle the different logics. Any tips? Do you think its worth to try and support A256-GCM or this library was basically meant to be used with HMAC algorithms?

Thanks

JOSESwift/Sources/CryptoImplementation/RSA.swift

ghost · 2018-07-19T09:02:05Z

@garrefa The reason it was working before was that the specific length check that you fixed only applies to the plaintext before it is padded. That's why we do that length check to ensure there is enough room for the padding to be applied. So the bug we had would have only manifested itself if the plaintext would have been 2018 - 11 = 2037 bits long. Just as a sidenote: For the actual encryption, we rely on Apple's Security framework, so the length check before the encryption is just to catch errors early on.

Regarding the RSA-OAEP length check, see my inline comment.

Let me know if you need something!

Regarding A256-GCM: We'd definitely appreciate the addition of this encryption method. 🎉 We're not trying to limit the library to HMAC algorithms. But you're right, it would need some refactoring to support algorithms that don't rely on HMAC for computing the digest. I'd be happy to discuss some strategies once this pull request is merged. Or if you feel like it, you can always go ahead and open another pull request with a suggestion on how to approach it. 🙂

JOSESwift/Sources/CryptoImplementation/RSA.swift

garrefa · 2018-07-25T14:09:50Z

Guys, just a quick update. The company I work for decided to go with CommonCrypto only so Im having a hard time here implementing this stuff. I'll still use the next weekend to finish this MR.

ghost · 2018-08-14T10:34:41Z

Hi @garrefa, 👋

as mentioned, your progress looks really good so far.

Do you have any plans on finishing this pull request?

garrefa · 2018-08-22T15:02:19Z

@daniel-mohemian hey man, just got back to it. Plan is to work this week and next week on this + A256GCM support. Ill close this first as you suggested and open a new one for A256GCM where we can discuss strategy.

Merge origin master in

ghost · 2018-08-22T15:07:53Z

@garrefa Sounds good. If you add a small test suite and work in the comments above we're good to go with merging this pull request. For A256GCM, as you said, a new pull request is the way to go.

ghost · 2018-08-30T07:23:07Z

Awesome @garrefa 👏

If you could add a small test suite, that would be awesome! Then we're good to go with merging. Let me know if you need help or assistance with that. 🎉

ghost · 2018-08-30T15:22:11Z

I might have a little time later this week to add some tests, so if you're ok with that, let me know. 🙂

garrefa · 2018-09-02T22:02:05Z

@daniel-mohemian this week all I have to do are this MR and the other to add GCM, but feel free to also add tests. I know this should have being merged already and the only thing pending are the tests.

ghost · 2018-09-03T12:10:01Z

It's fine @garrefa. We're not in a hurry. Take your time. Looking forward to reviewing the test suite next week. 🙂 If you run out of time, we can still have a look at it ourselves then.

garrefa · 2018-09-04T18:54:08Z

JOSESwift/Sources/CryptoImplementation/RSA.swift

            return .rsaEncryptionOAEPSHA1
+        case .direct:


Guys, I'm adding a explicity case to direct here instead of using default: return nil. Idea is to make obvious when you add a new AsymetricKeyAlgorithm that you need to update this method. Let me know if you think the default clause would make more sense.

garrefa · 2018-09-04T18:54:34Z

JOSESwift/Sources/CryptoImplementation/RSA.swift

            return mLen <= (k - 2 * hLen - 2)
+        case .direct:


Same thing here... left me know if you prefer default: return false.

garrefa · 2018-09-04T18:54:40Z

JOSESwift/Sources/CryptoImplementation/RSA.swift

+            // C: ciphertext to be decrypted, an octet string of length k, where k >= 2hLen + 2
+            return cipherText.count == SecKeyGetBlockSize(privateKey)
+        case .direct:
+            return false


Same thing here... left me know if you prefer default: return false.

garrefa · 2018-09-05T02:01:54Z

@daniel-mohemian Please let me know if those tests are suficient. I duplicated the RSA1_5 to RSAOAEP

garrefa · 2018-09-05T16:29:12Z

Those failed unit tests pass locally for me. It seems like some difference in the environment. Any idea?

[02:06:57]: ▸ {:file_path=>"/Users/travis/build/airsidemobile/JOSESwift/Tests/RSADecrypterTests.swift:145", :reason=>"XCTAssertEqual failed: (\"decryptingFailed(\"The operation couldn’t be completed. (OSStatus error -50 - RSAdecrypt wrong input (err -3))\")\") is not equal to (\"decryptingFailed(\"The operation couldn’t be completed. (OSStatus error -50 - RSAdecrypt wrong input (err 26))\")\") - ", :test_case=>"testDecryptingAliceSecretWithBobKey_RSAOAEP"}
[02:06:57]: ▸ {:file_path=>"/Users/travis/build/airsidemobile/JOSESwift/Tests/RSADecrypterTests.swift:173", :reason=>"XCTAssertEqual failed: (\"decryptingFailed(\"The operation couldn’t be completed. (OSStatus error -50 - RSAdecrypt wrong input (err -3))\")\") is not equal to (\"decryptingFailed(\"The operation couldn’t be completed. (OSStatus error -50 - RSAdecrypt wrong input (err 26))\")\") - ", :test_case=>"testDecryptingBobSecretWithAliceKey_RSAOAEP"}

I get 26 error back when running locally, but on CI we get -3.

ghost · 2018-09-06T14:54:27Z

@garrefa Thanks for adding tests. I'll be offline the next two weeks and will have a look at your work when I'm back. Feel free to ping @carol-mohemian or @gigi-mohemian if you need something in the meantime.

ghost · 2019-02-10T18:24:53Z

Hi, @garrefa sorry for the huge delay. I should've taken care of this pull request much earlier.

Some things have changed in the past weeks in our test code base. I'll take care of resolving the merge conflicts in the resolve-conflicts-95 branch and will then notify you once RSA-OAEP hits master.

Sorry again and thanks for your contribution! 👍

Edit: See #142

* WIP: Add RSAES-OAEP support (#95) * Add RSAES_OAEP type to AsymmetricKeyAlgorithm * Adds AsymmetricKeyAlgorithm.RSAES_OAEP and implements plain text and cipher length check * Add encrypter for (RSAES_OAEP, A256CBCHS512) * Add decrypter for (RSAES_OAEP, A256CBCHS512) * Rename new AsymmetricKeyAlgorithm case to RSAOAEP * Fix length checks * Add Encrypter Tests * Add Decrypter Tests * Add check to readme * Fix algorithm naming in doc * Fix doc mistakes * Update changelog * Update Tests/JWERSATests.swift Co-Authored-By: daniel-mohemian <daniel-mohemian@users.noreply.github.com> * Update Tests/JWERSATests.swift Co-Authored-By: daniel-mohemian <daniel-mohemian@users.noreply.github.com> * Update Tests/JWERSATests.swift Co-Authored-By: daniel-mohemian <daniel-mohemian@users.noreply.github.com> * Update MMA doc * Fix typos and inconsistencies in comments

giladreich · 2022-07-21T13:43:45Z

Hi @garrefa, sorry for commenting on an old merged PR, but I was looking for GCM implementation, but unfortunately this library doesn't support it yet.
I saw there is a fork here that implements it: https://github.com/sreekanthps/JOSESwift-AES256GCM
I haven't tested it, but maybe we can combine some of this work here?

Alexandre Garrefa and others added 5 commits July 18, 2018 13:40

Add RSAES_OAEP type to AsymmetricKeyAlgorithm

78c8754

Adds AsymmetricKeyAlgorithm.RSAES_OAEP and implements plain text and …

83ad051

…cipher length check

Add encrypter for (RSAES_OAEP, A256CBCHS512)

81981f9

Add decrypter for (RSAES_OAEP, A256CBCHS512)

39ade5f

Merge branch 'master' into add-RSAES-OAEP-support

06a476a

mohemian-92817281 requested a review from a team July 18, 2018 13:55

carol-mohemian reviewed Jul 18, 2018

View reviewed changes

ghost reviewed Jul 18, 2018

View reviewed changes

ghost reviewed Jul 19, 2018

View reviewed changes

JOSESwift/Sources/CryptoImplementation/RSA.swift Outdated Show resolved Hide resolved

ghost reviewed Jul 19, 2018

View reviewed changes

JOSESwift/Sources/CryptoImplementation/RSA.swift Outdated Show resolved Hide resolved

ghost reviewed Jul 19, 2018

View reviewed changes

JOSESwift/Sources/CryptoImplementation/RSA.swift Outdated Show resolved Hide resolved

michael-mohemian assigned garrefa Jul 20, 2018

Merge pull request #1 from airsidemobile/master

9f5f898

Merge origin master in

garrefa added 2 commits August 22, 2018 17:31

Rename new AsymmetricKeyAlgorithm case to RSAOAEP

352bd19

Fix length checks

6762d40

ghost added the WIP label Aug 30, 2018

Alexandre Garrefa added 3 commits September 4, 2018 15:42

Merge remote-tracking branch 'upstream/master'

76e74aa

Merge latest master in

7d54f6f

Add Encrypter Tests

7454a3d

garrefa commented Sep 4, 2018

View reviewed changes

Add Decrypter Tests

f342566

ghost changed the base branch from master to merge-95 February 10, 2019 18:05

ghost changed the base branch from merge-95 to resolve-conflicts-95 February 10, 2019 18:23

ghost merged commit c4d3253 into airsidemobile:resolve-conflicts-95 Feb 10, 2019

ghost mentioned this pull request Feb 12, 2019

Add RSA-OAEP via #95 #142

Merged

This pull request was closed.

WIP: Add RSAES-OAEP support #95

WIP: Add RSAES-OAEP support #95

Conversation

garrefa commented Jul 18, 2018 • edited by mohemian-92817281 Loading

carol-mohemian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrefa Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jul 19, 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

lkomorowski Sep 27, 2018 • 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

ghost left a comment

Choose a reason for hiding this comment

garrefa commented Jul 19, 2018

ghost commented Jul 19, 2018 • edited by ghost Loading

garrefa commented Jul 25, 2018

ghost commented Aug 14, 2018

garrefa commented Aug 22, 2018

ghost commented Aug 22, 2018

ghost commented Aug 30, 2018

ghost commented Aug 30, 2018

garrefa commented Sep 2, 2018

ghost commented Sep 3, 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

garrefa commented Sep 5, 2018

garrefa commented Sep 5, 2018

ghost commented Sep 6, 2018

ghost commented Feb 10, 2019 • edited by ghost Loading

giladreich commented Jul 21, 2022 • edited Loading

garrefa commented Jul 18, 2018 •

edited by mohemian-92817281

Loading

garrefa Jul 19, 2018 •

edited

Loading

ghost Jul 19, 2018 •

edited by ghost

Loading

lkomorowski Sep 27, 2018 •

edited

Loading

ghost commented Jul 19, 2018 •

edited by ghost

Loading

ghost commented Sep 3, 2018 •

edited by ghost

Loading

ghost commented Feb 10, 2019 •

edited by ghost

Loading

giladreich commented Jul 21, 2022 •

edited

Loading