Split internal key structure into public and private #128
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
gwynne
requested changes
Nov 25, 2023
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## jwtkit-5 #128 +/- ##
============================================
- Coverage 70.37% 68.90% -1.47%
============================================
Files 57 56 -1
Lines 1367 1386 +19
============================================
- Hits 962 955 -7
- Misses 405 431 +26
|
0xTim
reviewed
Nov 27, 2023
There was a problem hiding this comment.
Overall this is shaping up nicely.
I am however not a fan of the use of package everywhere - we're kind of abusing the purpose of it for testing which poses a challenge as we can't test what the actual public API is. I'd rather have everything as either internal or public and the majority of tests using the publicly defined API and @testable import in the case where we need to verify the internals work in the limited cases where this can't be done via a public API (and there's a good justification for it)
Sources/JWTKit/RSA/RSA.swift
Outdated
|
|
||
| /// Namespace encompassing functionality related to the RSA (Rivest–Shamir–Adleman) cryptographic algorithm. | ||
| /// Relatively to other algorithms such as ECDSA and EdDSA, RSA is considered slow and should be avoided when possible. | ||
| public enum RSA: Sendable {} |
There was a problem hiding this comment.
I'd be tempted to shove this under an Unsafe namespace similar to Swift Crypto - thoughts @gwynne?
|
@0xTim my thoughts on the use of |
|
I'll disagree with that - the modifier is for when you have multiple modules in a single package and want to be able to access code between without making it public to those using your package as a dependency. For example, making Vapor internals available to XCTest or Bcrypt internals to Vapor. Since JWTKit only has a single module, we shouldn't need The canonical way of doing this is to move those tests to an |
|
That's sensible, I'm convinced 🫂 |
gwynne
requested changes
Nov 27, 2023
Sources/JWTKit/RSA/RSA.swift
Outdated
|
|
||
| /// Namespace encompassing functionality related to the RSA (Rivest–Shamir–Adleman) cryptographic algorithm. | ||
| /// Relatively to other algorithms such as ECDSA and EdDSA, RSA is considered slow and should be avoided when possible. | ||
| public enum RSA: Sendable {} |
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
* 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
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 PR updates the internal structure to (hopefully) closely follow the one in swift-crypto