X5CVerifier: support more signature algorithms #238
Conversation
0xTim
left a comment
0xTim
left a comment
There was a problem hiding this comment.
Think this is the right approach, just some nits to add
Sources/JWTKit/X5C/X5CVerifier.swift
Outdated
| let eddsaKey = try EdDSA.PublicKey(pem: certificate.publicKey.serializeAsPEM().pemString) | ||
| return JWTSigner(algorithm: EdDSASigner(key: eddsaKey), parser: parser) | ||
| default: | ||
| throw JWTError.invalidX5CChain(reason: "Unsupported algorithm: \(String(describing: alg))") |
There was a problem hiding this comment.
I don't think this is the correct error - the chain could be valid but we don't support that algorithm so I think we should migrate it to something else
There was a problem hiding this comment.
Let me know if the new error I added makes sense!
Tests/JWTKitTests/X5CTests.swift
Outdated
| } | ||
| } | ||
|
|
||
| @Test("Test signing with EdDSA x5c chain") |
There was a problem hiding this comment.
It would be good to add tests for the other new algorithms
There was a problem hiding this comment.
Done, however I'm not sure how you guys feel about me replacing the previously hardcoded key and certs with runtime-generated ones.
ptoffy
left a comment
ptoffy
left a comment
There was a problem hiding this comment.
Thanks for this and sorry for the wait!
This feature is long overdue. It looks like tests are failing because the other hardcoded certs are expired. I'll fix those up in a separate PR. In the meantime could you please switch the tests you added to be parameterised using swift-testing so we don't duplicate the same code over various tests? And could you also please run this through a swift-format round when done? Thanks!
3 participants
Noticed jwt-kit's
X5CVerifieronly supports ES256 while trying to use Ed25519 certs and signatures.This PR tries to add support for ES384, ES512 and EdDSA (did not bother with RSA yet). Would that be an okay way of doing so?