Implement JWTKeyCollection and hide JWTSigner
#111
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
0xTim
reviewed
Nov 2, 2023
There was a problem hiding this comment.
Overall comment about the DocC comments - we should avoid talking about JWTSigners and other non public types in docs that will be what the end user sees to avoid confusion
Tests/JWTKitTests/JWTKitTests.swift
Outdated
| let parser = try JWTParser(token: jwt.bytes) | ||
| try XCTAssertEqual(parser.header(jsonDecoder: .defaultForJWT).typ, "JWT") | ||
| try XCTAssertEqual(parser.header(jsonDecoder: .defaultForJWT).alg, "HS256") | ||
| try XCTAssertEqual(parser.payload(as: ExpirationPayload.self, jsonDecoder: .defaultForJWT).exp, exp) | ||
| try parser.verify(using: .hs256(key: "secret".bytes)) | ||
| try await parser.verify(using: keyCollection.signer()!.algorithm) |
There was a problem hiding this comment.
One comment now I see the API, do we want signer() to throw instead of returning an optional? Having the optional here feels unnatural
There was a problem hiding this comment.
There's actually require(kid:, alg:) which I added later and wraps the signer() into a throwing function. Do you think we can use that or should we just make signer throwing and remove require?
There was a problem hiding this comment.
Just to be clear these aren't public anyways
There was a problem hiding this comment.
Do you need to test the internals then? Is it possible to rewrite the tests so that we still test the same behaviour but only via the public API (it should be).
IMO a good test suite combined with a solid API doesn't need to use @testable
There was a problem hiding this comment.
I think I solved this specific instance but there's still some stuff we need to test which is not be public, like the ECDSA parameters, the JWTParser or the PrimeGenerator. Do you think we can make this any better?
There was a problem hiding this comment.
We can add those to a test file dedicated to internal tests. Feel free to create an issue and do it in a separate PR to unblock this
0xTim
approved these changes
Nov 4, 2023
0xTim
pushed a commit
that referenced
this pull request
Feb 21, 2024
* Move away from BoringSSL (#99) * Start moving away from BoringSSL * Start converting RSA * Update RSA signer * Some fixes * Add possible RSA pubkey creation algorithm * Add prime number generator with Miller-Rabin test * Prime generation performance improvement * Attempt at private key calculation * RSA prime generation take 2 * API tidy up * Performance improvements * Even more speed * RSA tidy up * Fix JWTSigner with new RSA impl * Add RSA tests and polish some stuff * Remove unused method * Minor improvements * Add GCD test * Get ECDSA compiling * Add key gen test + fixups * Add RSA cert support + enforce bigger key sizes * Start adding ECDSA tests * Generify ECDSAKey * Abstract more and add P384 and P521 keys * Adapt curve sizes * Fix some tests * Base64URL decode raw key elements * Update byteRange names * Remove BoringSSL * Update error description * Fix wrong overload resolution * Add padding option for RSA signer * Update platform versions and start converting X5C * Convert X5CVerifier and X5CTests (SHA256) * Add certificate creation scripts * Fix comment * Address most requested issues * Add docs and replace struct with tuple * Remove valid X5C print statement * Remove `rsa_oaep_misc_test` test vectors * Performance improvements * Remove unused files * Apply suggestions * 🤦♂️ * Minor fixes * Refactor RSA init * Make RSAKey a struct and update docs * Implement `JWTKeyCollection` and hide `JWTSigner` (#111) * Implement `JWTKeyCollection` and hide `JWTSigner` * Make `JWTSigner` `Sendable` * Add comments and remove unused method * Add warning when overwriting kid * Remove `JWTSigners` * Make `JWKSigner` `Sendable` * Cleanups * Update DocC comments * Minor improvements * Add RSA pre-generated token test (#114) Add RS256 pre-generated token test * Enable full CI on the 5.x branch * Skip API breakage check for 5.x branch for now * Add `Sendable` support (#116) * Add `Sendable` support * Add Sendable conformance to tests * Make X5CVerifier a struct --------- Co-authored-by: Gwynne Raskind <gwynne@vapor.codes> * Rename ES521 to ES512 * Rename signer to algorithm * Add support for custom time validation X5Cs (#119) * Add support for custom time validation X5Cs * Clean up and move JSONDecoder settings out of X5C * Add more assertions in new X5C test * Refactor X5CTests * Add ECDSAKey PEM export (#120) * Add ECDSAKey PEM export * Use public key from private when possible * Refactor ECDSAKey init * Remove exports (#121) Start removing exports * Adopt `package` access and add RSA key PEM export (#122) * Adopt `package` access and add RSA key PEM export * Remove unused code * Make equatable conformance public * Optimise RSAKey Equatable implementation * Refactor RSAKey Equatable implementation * Remove public enums (#123) * Start removing enums * Update JWTError * Add custom decoding for new structs * Adopt a more structured JWTError * Minor improvements * Test integration with v5 of JWT * Make JWK use existing curves * Nit: spacing --------- Co-authored-by: Gwynne Raskind <gwynne@vapor.codes> * Remove use of `Data(contentsOf:)` * Add RSA-PSS signature algorithm support (#112) * Add RSA-PSS signature algorithm support * Add PSS signers and tests * Replace `Data(contentsOf:)` with `URLSession` * Remove Apple jwks test * Fix keycollection's getSigner method * Fix keycollection's getSigner method --------- Co-authored-by: Paul <paultoffoloni@gmail.com> * Adjust JWTError access modifiers * Adjust JWTError access modifiers once again * Add option to sign tokens with x5c chains (#126) * Add option to sign tokens with x5c chains * Add new test and fixes * Add option to fetch RSA primitives (#127) * Add option to fetch RSA primitives * Typo * Remove unused files * Update NOTICES * Split internal key structure into public and private (#128) * Create first idea of split RSA keys * Refactor signer and update docs * Split ECDSA keys * Remove useless parameter * Update EdDSA * Rename file * Adjust spacing * Remove useless implementations * Clean up some access modifiers * Move RSA to insecure namespace * Add customisable fields to JWTHeader (#129) * Add customisable fields to JWTHeader * Remove unused field * Fix en/decoding logic and add remove `package` use * Make customFields not optional * Add correct init to JWTHeader * Fix CodingKey mismatch * Remove `CaseIterable` conformance * Fix * Add `float` jwt header field type * Allow for custom JWT de/serialisation (#130) * Improve header structure * Allow for custom JWT de/serialisation * Make properties return nil instead of throwing * Make the new API easier to use * Add platform-agnostic de/compression algorithms * Remove unnecessary test * Update swift-certificates and add customisable policy to X5C verification * Add key initialiser for SwiftCrypto key types * Add `missingX5CHeader` error * Add RSA size boundary (#135) Add 2048 bits key size boundary for RSA keys * Update README and DocC (#136) * Start updating README * Update badges in README * Update CODEOWNERS * Update README.md Co-authored-by: Gwynne Raskind <gwynne@vapor.codes> * Update DocC * Update docs * Un-fancy JSON... * Revert README header upgrade * Add custom parsing/serialising comments --------- Co-authored-by: Gwynne Raskind <gwynne@vapor.codes> * Add some tests to get coverage up (#139) Add some test to get coverage up * Merge branch 'main' into 'jwtkit-5' --------- Co-authored-by: Gwynne Raskind <gwynne@vapor.codes> Co-authored-by: Matteo Franceschi <matteo.franceschi@flowpay.it>
ptoffy
added a commit
that referenced
this pull request
Feb 24, 2024
* Implement `JWTKeyCollection` and hide `JWTSigner` * Make `JWTSigner` `Sendable` * Add comments and remove unused method * Add warning when overwriting kid * Remove `JWTSigners` * Make `JWKSigner` `Sendable` * Cleanups * Update DocC comments * Minor improvements
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This also starts adding
Sendablesupport so we might as well implement it here directly