feat: Add ES256, ES384 and ES512 support #40
Conversation
README.md
Outdated
| @@ -176,6 +178,11 @@ The supported algorithms for signing and verifying JWTs are: | |||
| * HS512 - HMAC using using SHA-512 | |||
| * none - Don't sign or verify the JWT | |||
|
|
|||
| ECDSA is available from **Swift 4.1** | |||
There was a problem hiding this comment.
Move this below the ECDSA list, and I'd word it as:
Note: ECDSA algorithms require a minimum Swift version of 4.1.
README.md
Outdated
| @@ -176,6 +178,11 @@ The supported algorithms for signing and verifying JWTs are: | |||
| * HS512 - HMAC using using SHA-512 | |||
| * none - Don't sign or verify the JWT | |||
|
|
|||
| ECDSA is available from **Swift 4.1** | |||
| * ES256 - ECDSA using using SHA-256 and a P-256 Curve | |||
| * ES384- ECDSA using using SHA-384 and a P-384 Curve | |||
There was a problem hiding this comment.
missing space after ES384, and Curve should probably not be capitalized.
Sources/SwiftJWT/BlueECDSA.swift
Outdated
| @@ -0,0 +1,108 @@ | |||
| /** | |||
| * Copyright IBM Corporation 2018 | |||
Sources/SwiftJWT/BlueECDSA.swift
Outdated
| private let key: Data | ||
| private let curve: String | ||
|
|
||
| init(key: Data, curve: String) { |
There was a problem hiding this comment.
Since there are only a small number of valid values for curve, could this be an internal enum instead?
Sources/SwiftJWT/BlueECDSA.swift
Outdated
| } | ||
| if #available(OSX 10.13, *) { | ||
| let privateKey = try ECPrivateKey(key: keyString) | ||
| guard privateKey.curveId == curve else { |
There was a problem hiding this comment.
Damn, ECPrivateKey.curveId should have been an enum (or struct equivalent) - is it too late to change it?
Tests/SwiftJWTTests/TestJWT.swift
Outdated
| @@ -196,6 +204,26 @@ class TestJWT: XCTestCase { | |||
| else { | |||
| XCTFail("Failed to sign") | |||
| } | |||
|
|
|||
| // ECDSA key | |||
There was a problem hiding this comment.
Could this be a separate test? Maybe split this function into four
Tests/SwiftJWTTests/TestJWT.swift
Outdated
| @@ -556,6 +624,30 @@ class TestJWT: XCTestCase { | |||
| } | |||
| } | |||
|
|
|||
| // From jwt.io | |||
There was a problem hiding this comment.
Maybe flesh this comment out ie. test that we can interop with a JWT issued by jwt.io
Sources/SwiftJWT/JWTSigner.swift
Outdated
| @@ -49,35 +66,62 @@ public struct JWTSigner { | |||
| } | |||
|
|
|||
| /// Initialize a JWTSigner using the RSA 256 bits algorithm and the provided privateKey. | |||
| /// - Parameter privateKey: The UTF8 encoded PEM private key. | |||
There was a problem hiding this comment.
You added the words "including the BEGIN PUBLIC KEY header" in the case of JWTVerifier - whilst I understand this is to disambiguate between key and certificate, I think it would be nice to include the statement about the header here too (and ditto for the EC keys for sign/verify)
This pull request used the new BlueECC repo to add support for signing and verifying using ECDSA.