Add support for RSA PSS and RS384 signatures #188
Conversation
airsidemobile#175 Throws if iOS version < 11.0
|
Hey @JohanObrink, thanks for your contribution. We will have a look and give feedback as soon as possible. |
|
Oh... I forgot: Also adds support for RS384 Supported formats are: RS256, RS384, RS512, PS256, PS384, PS512 |
| } else { | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Lines should not have trailing whitespace.trailing_whitespace RSA.swift:52 |
| } else { | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Lines should not have trailing whitespace.trailing_whitespace RSA.swift:59 |
Tests/JWSRSATests.swift
Outdated
| @@ -103,5 +132,27 @@ class JWSRSATests: RSACryptoTestCase { | |||
| let signature = try! signer.sign(header: JWSHeader(algorithm: algorithm), payload: Payload(message.data(using: .utf8)!)) | |||
| XCTAssertEqual(jws.signature.data(), signature) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Lines should not have trailing whitespace.trailing_whitespace JWSRSATests.swift:135 |
Tests/RSACryptoTestCase.swift
Outdated
| @@ -45,6 +45,15 @@ class RSACryptoTestCase: CryptoTestCase { | |||
| XZa_Aj3LkNWn1GSw5B4WQueb8E0uJVAzLSNbxA-ZNowlOgDtKHOEkwbZu6zj7WvLEm8xovgmAha_y7HssoXnH26Nu-8RMUYw-LXUJz6Fny1F_xc\ | |||
| v_TA | |||
| """ | |||
|
|
|||
There was a problem hiding this comment.
Lines should not have trailing whitespace.trailing_whitespace RSACryptoTestCase.swift:48 |
Generated by 🚫 Danger |
...to satisfy our Jose-ElRobot overlord
There was a problem hiding this comment.
This is awesome, great job @JohanObrink and thanks for the contribution. 🎉
I only had a couple of small comments. See below and let me know what you think!
| @@ -63,6 +77,21 @@ class JWSRSATests: RSACryptoTestCase { | |||
| self.performTestRSADeserialization(algorithm: .RS512, compactSerializedJWS: compactSerializedJWSRS512Const) | |||
| } | |||
|
|
|||
| @available(*, deprecated) | |||
| func testSignVerifyAndDeserializeForPS256() { | |||
| performTestRSASerializationValidationAndDeserialization(algorithm: .PS256) | |||
There was a problem hiding this comment.
Please also add an example serialization for each of the three RSASSA-PSS algorithms, similar to what we have for the RSASSA-PKCS1-v1_5 algorithms. Then you should be able to reuse the self.performTestRSASign and self.performTestRSADeserialization helper functions. I'd prefer that instead of this new testSignVerifyAndDeserializeForPS256 helper. Let me know if you disagree!
While looking at your code for performTestRSASerializationValidationAndDeserialization I noticed that we have an issue in self.performTestRSASign. See here: #189. You can just ignore that issue and use the function as you did for the RS384 test. I will fix #189 after merging your pull request.
There was a problem hiding this comment.
The thing is that the signature will differ every time since PSS isn't determinate (it stands for Probabilistic Signing Signature). This means that it cannot be tested by comparison - only by validation. My understanding is that this is part of why it is recommended over the old signing scheme.
There was a problem hiding this comment.
Oh stupid me, you're right of course.
In this case, please just remove this line from performTestRSASerializationValidationAndDeserialization:
XCTAssertEqual(compactSerializedJWS, compactSerializedJWS)as it doesn't really do anything (this is the same issue as in #189).
There was a problem hiding this comment.
Thanks for the quick responses @JohanObrink!
Please just remove that one line mentioned above, and we should be good to go! 🚀
|
@daniel-mohemian I've been away for a couple of days so I couldn't address that. It's done now :) |
|
Nice that's perfect @JohanObrink. Somehow the build system is having some issues. I'll have a look into that tomorrow so that we can merge your pull request and release the changes. Thanks for your contribution, I'll keep you posted on the status of the release. 👍 |
|
I will fix the build issues through another pull request before the release. Thanks again for you contribution @JohanObrink. 🎉 |
ghost
changed the title
ghost
changed the title
ghost
merged commit
ghost
mentioned this pull request
|
A bit late, but there we go: 2.0.0. 🎉 |
ghost
mentioned this pull request
#175
Throws if iOS version < 11.0