Add Elliptic Curve variants for JWS and JWK utilities #88
Conversation
|
👋 Hi Jarrod That's great news! 🎉 We were actually planning on supporting EC keys in our JWK implementation and consequently using them for signing and/or encryption for the mid-term future, so any help in that domain is greatly appreciated. @carol-mohemian and I will have a look at your code over the next days and provide some feedback. At a first glance, it already looks really great! Once we have some progress on the test suite, we'd be happy to merge this to master and put it in the next release of JOSESwift. 😎 Thanks again, you're awesome |
ghost
requested review from
|
Hi @daniel-mohemian 👋 Thank you for the warm welcome. I'll be pecking away at this in my downtime, so it might be a 4-8 week turn-around for the full test suite. Let me know if that conflicts with you, and I'll see what I can do. Cheers, Jarrod |
There was a problem hiding this comment.
Hey @jarrodmoldrich from my side as well 👋
Had a first look at your PR and it already looks really amazing, well done 👏 I only had a look at the structure and didn't get my head around the logic yet.
Just found a few typos and commented some structural improvements, looking forward to your replies 😁
JOSESwift/Sources/Algorithms.swift
Outdated
| public enum SignatureAlgorithm: String { | ||
| case RS256 = "RS256" | ||
| case RS512 = "RS512" | ||
| case ES256 = "ES256" | ||
| case ES384 = "ES384" | ||
| case ES521 = "ES521" |
There was a problem hiding this comment.
This should say ES512 for key and value.
There was a problem hiding this comment.
Ah yes, got my P-521s and ES512s mixed up 🤦♂️
| // | ||
| // Created by Jarrod Moldrich on 02.07.18. | ||
| // | ||
|
|
| case yNotBase64URLUIntEncoded | ||
| case privateKeyNotBase64URLUIntEncoded | ||
| case invalidCurveType | ||
| case compressedCurvePointsUnsuported |
There was a problem hiding this comment.
Typo Unsuported -> Unsupported.
Would either rename this error to unsupportedCompressedCurvePoints or the error below to curvePointSizeUnsupported for consistency.
There was a problem hiding this comment.
Good catch, I'll fix the typo.
We're specifically not supporting compressed curve points at all, so I'm not sure the other two suggestions effectively convey this limitation.
There was a problem hiding this comment.
I see what you mean about the "error below". GitHub doesn't provide surrounding context, but I'd agree that the second error could be invalidCurvePointOctetLength.
|
|
||
| static func fromKeyBitLength(_ length: Int) -> ECCurveType? { | ||
| switch length { | ||
| case 256: |
There was a problem hiding this comment.
I think you should use ECCurveType.P256.keyBitLength, etc.
|
|
||
| static func fromCoordinateOctetLength(_ length: Int) -> ECCurveType? { | ||
| switch length { | ||
| case 32: |
There was a problem hiding this comment.
Same as above with ECCurveType.P256.coordinateOctetLength, etc.
There was a problem hiding this comment.
I'm not sure I understand what you want. They are static functions that map a value to an enum value, not a property of an enum value. There's probably a Swiftier way to implement this, so if you could provide an example, I'd be more than happy to oblige.
There was a problem hiding this comment.
I just thought that you can use the ECCurveType enum and it's values for keyBitLength/coordinateOctetLength to not hardcode those length values again.
There was a problem hiding this comment.
Yes, sorry, I misunderstood. That's done 👍
| let status = SecKeyRawVerify(publicKey, .sigRaw, digest, digest.count, signatureBytes, curveType.signatureOctetLength) | ||
| if status != 0 { | ||
| throw ECError.verifyingFailed( | ||
| description: "Error validating signature. (OSStatus: \(status))" |
There was a problem hiding this comment.
Doesn't this fit in one line?
JOSESwift/Sources/ECKeys.swift
Outdated
|
|
||
| // MARK: Protocols | ||
|
|
||
| /// The components of an EC public key. |
There was a problem hiding this comment.
One whitespace too much between "EC" and "public".
JOSESwift/Sources/ECVerifier.swift
Outdated
|
|
||
| import Foundation | ||
|
|
||
| /// A `Verifier` to verify a signature created with a elliptic curve algorithm. |
There was a problem hiding this comment.
an elliptic curve..
JOSESwift/Sources/Signer.swift
Outdated
| @@ -51,6 +52,8 @@ public struct Signer<KeyType> { | |||
| } | |||
| // swiftlint:disable:next force_cast | |||
| self.signer = RSASigner(algorithm: signingAlgorithm, privateKey: privateKey as! RSASigner.KeyType) | |||
| default: | |||
There was a problem hiding this comment.
Why you don't add the ECSigner here?
JOSESwift/Sources/Verifier.swift
Outdated
| return nil | ||
| } | ||
| self.verifier = ECVerifier(algorithm: verifyingAlgorithm, publicKey: publicKey as! ECVerifier.KeyType) | ||
|
|
|
Sure, sounds great @jarrodmoldrich. 👍 If you want, we can also help out with some tests. It would be cool if you could create a basic test suite and maybe use some examples from the relevant RFCs as the test data. We can then still expand the test suite after merging this PR. As said, the implementation looks really great already. 🎉 Let me know if you need anything! |
JOSESwift/Sources/Algorithms.swift
Outdated
| @@ -26,9 +27,13 @@ import Foundation | |||
| /// An algorithm for signing and verifying. | |||
| /// | |||
| /// - RS512: [RSASSA-PKCS1-v1_5 using SHA-512](https://tools.ietf.org/html/rfc7518#section-3.3) | |||
| /// - ES512: [ECDSA P-512 using SHA-512](https://tools.ietf.org/html/rfc7518#section-3.4) | |||
There was a problem hiding this comment.
I think you have another typo here. The curve should be P-521. 😉
|
@daniel-mohemian It probably makes sense that I create a minimal test suite. I think the sooner this PR gets merged, the better, as there's a lot of opportunity to refactor out much of the shared functionality between the key types. The Apache license seemed to me a little ambiguous about the copyright notice, but on a second read, a PR classes more as a 'Contribution' than a 'Derivative Work' and so I've changed to your copyright notice I'll be AFK for the next couple of weeks, so I probably won't start on the test suite until August. Feel free to leave feedback in the meantime 🙂 |
jarrodmoldrich
force-pushed
the
add_ec_variants
branch
from
fa21e48 to
b47545e
Compare
|
Hi @jarrodmoldrich 👋 I just wanted to check in with you to see if you need any assistance with the tests. If you want we can also take over a part of the test suite. If you're back online already, let me know what you think, otherwise, enjoy your time off. 🙂 |
|
Thanks for checking in, @daniel-mohemian 🙂 I'm actually back online, though my schedule was pushed back by about a month. From next week I'll be in a situation where I'll be able to work on the test suite again, so I should be able to deliver a minimal suite by the end of September. If you need something sooner for your own purposes, I'm happy for you to take the reins, otherwise there should be a trickle of check-ins next month. |
|
That sounds great. Just let me know if you need anything. We're not in a hurry, end of September sounds like a plan! 😄 |
|
@daniel-mohemian It's a good thing you're not in a hurry! Things were a bit hectic for me this past month, but I should finally have some free time at the end of this coming week. I was going to make a start by adding test data to |
|
That's fine, @jarrodmoldrich. Refactoring and expanding the base class for crypto tests -
Let me know if you need anything! |
|
I see you added quite some tests @jarrodmoldrich. 🎉 Please ping me once they are ready to be looked at. PS: Don't worry about the merge conflicts, I can take care of that for you before merging. |
|
Sorry for the delay, @daniel-mohemian . I've been occupied the last few months, but I'll have this ready for your review at my self-imposed deadline of Jan 14. |
|
Don't worry @jarrodmoldrich, we've been quite packed with work as well over the last months, so we're not in a hurry. Once you think your work is ready, just ping me and I'll take a look as soon as I can. I can also take care of the merge conflicts for you if you want. Thanks again for your efforts. 😄 |
|
Happy New Year @daniel-mohemian and the Airside crew 🎉 The test suite has been updated to test all EC features, with close to 100% coverage. I just realized I haven't pulled down from upstream. I'll do that now and try merging. In the meantime it might be worth reviewing before I pollute the diff with merges. |
Upstream needed to be integrated with the split in the `CryptoTestCase` test data class, as well as trivially merged with new entries in various enums.
|
@daniel-mohemian Is |
|
Hey @jarrodmoldrich 👋 Please see the PR description of #126. TL;DR
|
|
Thanks @carol-mohemian . I think it's finally ready for review. |
|
Awesome @jarrodmoldrich, I hope we can provide feedback soon! We're also quite packed with work but I'll do my best to get the pull request merged over the next weeks. At the very latest in February I'll have some more time on my hands. I hope this is ok for you. We already looked over it when you first opened the pull request and it looked really good back then. 👌 Oh and happy new year to you too! 🎉 |
ghost
mentioned this pull request
There was a problem hiding this comment.
@jarrodmoldrich I just looked over your implementation once again, and it still looks great. 👌 I only had some minor comments. Nothing in the way of merging. ✅
I will look over the tests first thing next week so expect the pull request to be merged latest by the end of next week. I'll ping you if I have questions regarding the tests.
Until then, have a nice weekend. 😎
| case typeIsUnknown | ||
| } | ||
|
|
||
| class JWKBase: Decodable { |
There was a problem hiding this comment.
I think I see what JWKBase is supposed to do - finding out what type of encoded key we have by throwing specific "errors".
But is it used somewhere? It looks like it is not needed because further down below, we just try to decode all key types and take the first one that we can successfully decode. I think both implementations are somewhat valid approaches to determine the key type (even if they both are a bit hacky) but it looks like we just need one of them.
There was a problem hiding this comment.
Looking back at an earlier commit I had modified init(from decoder: Decoder) to look like it is below, but during the integration I must have reverted it to its original style. I think it looks a little neater the way I've done it, so I could either revert the approach once more, or remove JWKBase. I'll defer to your judgement.
do {
_ = try keyContainer.decode(JWKBase.self)
} catch JWKTypeError.typeIsECPublic {
key = try keyContainer.decode(ECPublicKey.self)
} catch JWKTypeError.typeIsECPrivate {
key = try keyContainer.decode(ECPrivateKey.self)
} catch JWKTypeError.typeIsRSAPublic {
key = try keyContainer.decode(RSAPublicKey.self)
} catch JWKTypeError.typeIsRSAPrivate {
key = try keyContainer.decode(RSAPrivateKey.self)
}
There was a problem hiding this comment.
I like your approach, let's go for that!
One suggestion from me would be to use the raw values of JWKKeyType in the cases in JWKBase like this:
// ...
switch key {
case JWKKeyType.RSA.rawValue:
if d == nil {
throw JWKTypeError.typeIsRSAPublic
} else {
throw JWKTypeError.typeIsRSAPrivate
}
}
// ...What do you think?
There was a problem hiding this comment.
Good point, yes, I should be doing that. Updated, along with a fix for the catch mechanism I just noticed. (Not using optional try?.)
There was a problem hiding this comment.
@jarrodmoldrich Awesome, your tests cover pretty much all there is to cover. 👍 I'd say we're ready to merge ✅ Thanks for sticking around and providing this great test coverage!
Feel free to push the one JWKBase decoding trick we talked about here if you want, I'll leave it up to you.
Otherwise, let's merge. ↩️ 🚀 🎉
|
|
||
| // As any two signing invocations will have different nonces, it is impossible to use pre-generated data from a | ||
| // trusted implementation (e.g. openssl) to verify the signature. Instead we will validate by verifying the | ||
| // signature. This assumes that the verification implementation is correct. |
| let dict = checkAndGetDictionary(jsonData: jsonData) | ||
|
|
||
| checkRegularParameters(dict: dict, keyData: keyData) | ||
| XCTAssertEqual(dict["d"] as? String ?? "", keyData.expectedPrivateBase64Url) |
There was a problem hiding this comment.
| XCTAssertEqual(dict["d"] as? String ?? "", keyData.expectedPrivateBase64Url) | |
| XCTAssertEqual(dict["d"] as? String ?? "", keyData.expectedPrivateBase64Url) | |
| XCTAssertNil(dict["breeze"]) |
jarrodmoldrich
force-pushed
the
add_ec_variants
branch
from
97b8d69 to
4050684
Compare
|
@daniel-mohemian Implemented your suggestions and integrated the decoding pattern with a couple of changes above what we discussed, hopefully they are all good 🤞 |
There was a problem hiding this comment.
Awesome @jarrodmoldrich let's merge this! ✅ We'll release throughout the week, probably as 1.5.0. I'll ping you here once it's out.
Thanks again, it was great to work with you.
|
Likewise, @daniel-mohemian 🙂 |
|
Your contribution is now released as part of 1.5.0. 🎉 |
Hello Airside/Mohemian team,
I needed EC algorithms for signing/verifying JWTs, and for decoding/encoding JWKs, so I thought it prudent to fork JOSESwift. The functionality for JWS and JWK should be complete and working, but I've yet to mirror your comprehensive test suite for RSA to the EC domain. I will be doing so over the coming weeks 🙂
Anecdotally, JWK decoding/encoding and JWS verification are working for
ES256public keys for the project I'm working on.This is a WIP, tell me if there's any issues with it. It would be good to see this eventually hit upstream so we can retarget to master.
Cheers,
Jarrod