Add RSA CRT fields and extractable support to JWK generation#15
Conversation
…added protected header; refactor fromJson and add focused tests (fallback, AAD, key wrap, error paths).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds RSA CRT parameters (dp, dq, qi) to JWK serialization and introduces an ChangesRSA CRT Parameters and Extractable JWK Fields
JWE JSON Parsing Fallback Test Coverage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/src/jwk.dart (1)
393-399:⚠️ Potential issue | 🟡 MinorInconsistent field name:
keyOperationsshould bekey_ops.The
unwrapKeymethod uses'keyOperations'but RFC 7517 specifies'key_ops'. This is inconsistent withjwkFromCryptoKeyPairinjwa.dart(line 246) and thekeyOperationsgetter (line 279) which reads from'key_ops'. Keys created byunwrapKeywon't have their operations recognized by the getter.🐛 Proposed fix
return JsonWebKey.fromJson({ 'kty': 'oct', 'k': encodeBase64EncodedBytes(v), 'use': 'enc', - 'keyOperations': ['encrypt', 'decrypt'] + 'key_ops': ['encrypt', 'decrypt'] })!;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/jwk.dart` around lines 393 - 399, The unwrapKey return builds a JWK with the wrong field name; change the 'keyOperations' property to the RFC-compliant 'key_ops' so the getter and other code (e.g., jwkFromCryptoKeyPair in jwa.dart and the keyOperations getter) can read it; update the map in unwrapKey (the JsonWebKey.fromJson call that uses encodeBase64EncodedBytes(v)) to use 'key_ops': ['encrypt', 'decrypt'] instead of 'keyOperations'.
🧹 Nitpick comments (5)
test/jwk_test.dart (1)
10-17: Consider using the library'sdecodeBigIntutility.The test helper
_decodeBase64UrlBigIntduplicates the logic fromlib/src/util.dart:decodeBigInt. While both implementations are equivalent (<< 8vs* b256), using the library's utility would validate that the actual decode function works correctly and reduce test maintenance.💡 Optional: Import and use library utility
+import 'package:jose_plus/src/util.dart' show decodeBigInt; + -BigInt _decodeBase64UrlBigInt(String value) { - final bytes = base64Url.decode(value + '=' * ((4 - value.length % 4) % 4)); - var out = BigInt.zero; - for (final b in bytes) { - out = (out << 8) + BigInt.from(b); - } - return out; -}Then replace
_decodeBase64UrlBigIntcalls withdecodeBigInt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/jwk_test.dart` around lines 10 - 17, The test helper _decodeBase64UrlBigInt duplicates library logic; remove that helper and import and use the existing decodeBigInt utility instead—update test/jwk_test.dart to import the module that exposes decodeBigInt, replace calls to _decodeBase64UrlBigInt with decodeBigInt, and delete the _decodeBase64UrlBigInt function to avoid duplication and validate the real implementation.lib/src/jwk.dart (1)
83-100: Consider using the extension methods to avoid duplication.The CRT calculation logic here duplicates
RsaPrivateKeyCRTCalcExtensionfromjwa.dart. Using the extension methods would reduce duplication and centralize the calculation.♻️ Optional: Use extension methods
final k = privateKey; - // Calculate CRT coefficients - // dp = d mod (p-1) - BigInt dp = k.privateExponent % (k.firstPrimeFactor - BigInt.one); - // dq = d mod (q-1) - BigInt dq = k.privateExponent % (k.secondPrimeFactor - BigInt.one); - // qi = q^(-1) mod p - BigInt qi = k.secondPrimeFactor.modInverse(k.firstPrimeFactor); return JsonWebKey.rsa( modulus: privateKey.modulus, exponent: (publicKey as RsaPublicKey?)?.exponent, privateExponent: privateKey.privateExponent, firstPrimeFactor: privateKey.firstPrimeFactor, secondPrimeFactor: privateKey.secondPrimeFactor, - firstFactorCRTExponent: dp, - secondFactorCRTExponent: dq, - firstCRTCoefficient: qi, + firstFactorCRTExponent: k.calculateDp(), + secondFactorCRTExponent: k.calculateDq(), + firstCRTCoefficient: k.calculateQi(), keyId: keyId, extractable: extractable, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/jwk.dart` around lines 83 - 100, The CRT coefficients are being recomputed here (dp, dq, qi) duplicating logic in RsaPrivateKeyCRTCalcExtension from jwa.dart; replace the manual calculations by calling the extension methods on the RsaPrivateKey (use the extension provided by RsaPrivateKeyCRTCalcExtension to get the firstFactorCRTExponent, secondFactorCRTExponent, and firstCRTCoefficient) and pass those values into JsonWebKey.rsa (rather than computing dp, dq, qi locally), keeping the existing variables privateKey/publicKey and preserving the same JsonWebKey.rsa parameter order.lib/src/jwa.dart (1)
304-328: Consider adding guards for missing RSA private key fields.The
JsonWebKeyCRTCalcExtensionmethods assume the JWK containsd,p, andqfields. Calling these on a public-only RSA key or non-RSA key will throw a runtime error due to theas Stringcasts. While this may be intentional (caller responsibility), adding precondition checks or documenting the requirement would improve safety.💡 Optional: Add defensive checks
extension JsonWebKeyCRTCalcExtension on JsonWebKey { /// Calculate dp = d mod (p-1) /// First factor CRT exponent BigInt calculateDp() { + if (this['d'] == null || this['p'] == null) { + throw StateError('RSA private key fields (d, p) required for CRT calculation'); + } final d = decodeBigInt(this['d'] as String); final p = decodeBigInt(this['p'] as String); return d % (p - BigInt.one); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/jwa.dart` around lines 304 - 328, The extension JsonWebKeyCRTCalcExtension assumes presence of RSA private fields ('d','p','q') and will throw on public-only/non-RSA keys; update calculateDp, calculateDq, and calculateQi to validate that this['d'], this['p'], and this['q'] are non-null and are Strings before decoding and either throw a clear ArgumentError/StateError (e.g., "missing RSA private field 'd'") or return a nullable BigInt, so callers get a deterministic error path; keep the same decodeBigInt/modInverse usage after the checks and include the field names in the error messages to aid debugging.lib/src/jwe.dart (1)
69-130: Consider handling missingprotectedheader in general JSON serialization.The code at line 130 unconditionally calls
JsonObject.decode(json['protected']), which will throw ifprotectedisnull. While RFC 7516 states thatprotectedis REQUIRED for compact serialization, it may be absent in certain general JSON serialization scenarios where all header parameters are in unprotected headers. Currently, this would fail with a base64 decode error rather than a clear message.The fallback branch (lines 100-103) correctly validates this, but the general/flattened branches don't. If
protectedbeing null is invalid for all serializations, consider adding a clearer error message.💡 Optional: Add explicit validation
+ final protectedValue = json['protected']; + if (protectedValue == null && !json.containsKey('recipients')) { + throw ArgumentError('Missing protected header for JWE'); + } protectedHeader: JsonObject.decode(json['protected']),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/jwe.dart` around lines 69 - 130, The code unconditionally calls JsonObject.decode(json['protected']) when constructing protectedHeader, which will throw a confusing error if json['protected'] is null; update the construction logic (around the List.unmodifiable(...) return and the protectedHeader assignment) to first check whether json.containsKey('protected') and json['protected'] != null and, if missing, either set protectedHeader to null (if your model permits) or throw an explicit ArgumentError('Missing protected header for JWE') with a clear message; ensure this validation covers both the general JSON (json['recipients']) and flattened (json['header']) branches and keep the _JweRecipient._ and derivedRecipientHeader behavior unchanged.test/jwe_test.dart (1)
299-308: Consider extracting fallback JSON assembly into a small helper.The repeated compact-parts-to-map logic is consistent but duplicated; a helper would reduce maintenance overhead in this test file.
♻️ Optional refactor sketch
+Map<String, dynamic> _fallbackJsonFromCompactParts( + List<String> parts, { + bool includeEncryptedKey = true, +}) { + final json = <String, dynamic>{ + 'protected': parts[0], + 'iv': parts[2], + 'ciphertext': parts[3], + 'tag': parts[4], + }; + if (includeEncryptedKey) { + json['encrypted_key'] = parts[1]; + } + return json; +}- var fallbackJson = { - 'protected': compact[0], - // intentionally no 'encrypted_key' - 'iv': compact[2], - 'ciphertext': compact[3], - 'tag': compact[4] - }; + var fallbackJson = + _fallbackJsonFromCompactParts(compact, includeEncryptedKey: false);Also applies to: 325-331, 370-376
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/jwe_test.dart` around lines 299 - 308, Extract the repeated logic that converts a JWE's compact serialization into the fallback JSON map into a small helper (e.g., compactToFallbackJson or buildFallbackJsonFromJwe) that accepts the JWE instance (or its compact string) and returns a Map with 'protected', optional omission of 'encrypted_key', 'iv', 'ciphertext', and 'tag'; replace the inline code that builds fallbackJson (the block using jwe.toCompactSerialization().split('.') and assigning compact[0], compact[2], compact[3], compact[4]) in the occurrences around the existing fallbackJson usages so tests call the helper instead to remove duplication and keep behavior identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 1-5: The CHANGELOG entry for 0.4.8 is missing the PR's key
features: add notes that JWK generation now supports RSA CRT parameters (dp, dq,
qi) and that the `extractable` flag (`ext`) is now handled; update the 0.4.8
list to include lines mentioning "RSA CRT parameters (`dp`, `dq`, `qi`) support
in JWK generation" and "support for `extractable`/`ext` flag in
JsonWebKey.fromCryptoKeys(), rsa(), ec(), and symmetric()" so reviewers can see
these features alongside the JWE parsing and tests changes.
In `@pubspec.yaml`:
- Line 12: Update the pubspec constraint decision for crypto_keys_plus: either
restore the minimum to ">=0.5.0" if the downgrade to ">=0.4.0" was accidental,
or keep ">=0.4.0" but update the changelog entry that currently states "Updated
to use crypto_keys_plus 0.5.0" to reflect the intentional change; reference the
package name crypto_keys_plus in pubspec.yaml and the changelog entry for
version 0.4.7 when making the correction so the minimum-version rationale is
consistent across files.
---
Outside diff comments:
In `@lib/src/jwk.dart`:
- Around line 393-399: The unwrapKey return builds a JWK with the wrong field
name; change the 'keyOperations' property to the RFC-compliant 'key_ops' so the
getter and other code (e.g., jwkFromCryptoKeyPair in jwa.dart and the
keyOperations getter) can read it; update the map in unwrapKey (the
JsonWebKey.fromJson call that uses encodeBase64EncodedBytes(v)) to use
'key_ops': ['encrypt', 'decrypt'] instead of 'keyOperations'.
---
Nitpick comments:
In `@lib/src/jwa.dart`:
- Around line 304-328: The extension JsonWebKeyCRTCalcExtension assumes presence
of RSA private fields ('d','p','q') and will throw on public-only/non-RSA keys;
update calculateDp, calculateDq, and calculateQi to validate that this['d'],
this['p'], and this['q'] are non-null and are Strings before decoding and either
throw a clear ArgumentError/StateError (e.g., "missing RSA private field 'd'")
or return a nullable BigInt, so callers get a deterministic error path; keep the
same decodeBigInt/modInverse usage after the checks and include the field names
in the error messages to aid debugging.
In `@lib/src/jwe.dart`:
- Around line 69-130: The code unconditionally calls
JsonObject.decode(json['protected']) when constructing protectedHeader, which
will throw a confusing error if json['protected'] is null; update the
construction logic (around the List.unmodifiable(...) return and the
protectedHeader assignment) to first check whether json.containsKey('protected')
and json['protected'] != null and, if missing, either set protectedHeader to
null (if your model permits) or throw an explicit ArgumentError('Missing
protected header for JWE') with a clear message; ensure this validation covers
both the general JSON (json['recipients']) and flattened (json['header'])
branches and keep the _JweRecipient._ and derivedRecipientHeader behavior
unchanged.
In `@lib/src/jwk.dart`:
- Around line 83-100: The CRT coefficients are being recomputed here (dp, dq,
qi) duplicating logic in RsaPrivateKeyCRTCalcExtension from jwa.dart; replace
the manual calculations by calling the extension methods on the RsaPrivateKey
(use the extension provided by RsaPrivateKeyCRTCalcExtension to get the
firstFactorCRTExponent, secondFactorCRTExponent, and firstCRTCoefficient) and
pass those values into JsonWebKey.rsa (rather than computing dp, dq, qi
locally), keeping the existing variables privateKey/publicKey and preserving the
same JsonWebKey.rsa parameter order.
In `@test/jwe_test.dart`:
- Around line 299-308: Extract the repeated logic that converts a JWE's compact
serialization into the fallback JSON map into a small helper (e.g.,
compactToFallbackJson or buildFallbackJsonFromJwe) that accepts the JWE instance
(or its compact string) and returns a Map with 'protected', optional omission of
'encrypted_key', 'iv', 'ciphertext', and 'tag'; replace the inline code that
builds fallbackJson (the block using jwe.toCompactSerialization().split('.') and
assigning compact[0], compact[2], compact[3], compact[4]) in the occurrences
around the existing fallbackJson usages so tests call the helper instead to
remove duplication and keep behavior identical.
In `@test/jwk_test.dart`:
- Around line 10-17: The test helper _decodeBase64UrlBigInt duplicates library
logic; remove that helper and import and use the existing decodeBigInt utility
instead—update test/jwk_test.dart to import the module that exposes
decodeBigInt, replace calls to _decodeBase64UrlBigInt with decodeBigInt, and
delete the _decodeBase64UrlBigInt function to avoid duplication and validate the
real implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c84f562-c7d5-4eca-a71c-9db3aabb1905
📒 Files selected for processing (9)
CHANGELOG.mdlib/src/jwa.dartlib/src/jwe.dartlib/src/jwk.dartlib/src/util.dartpubspec.yamltest/jwa_test.darttest/jwe_test.darttest/jwk_test.dart
| ## 0.4.8 | ||
| - **FIX**: JWE JSON parsing now handles missing `recipients` / `header` by deriving a single recipient from the protected header and validates absent `encrypted_key` for non-`dir` algorithms. | ||
| - **FEAT**: Support unpadded Base64URL protected header (new parsing test). | ||
| - **TEST**: Added fallback, AAD, key wrap, and error path tests. | ||
| - **CHORE**: Refactored `JsonWebEncryption.fromJson` for clarity. |
There was a problem hiding this comment.
Changelog entry is incomplete - missing RSA CRT and extractable features.
The 0.4.8 entry documents JWE parsing changes but omits the main features from this PR:
- RSA CRT parameters (
dp,dq,qi) support in JWK generation extractableflag support (extfield) inJsonWebKey.fromCryptoKeys(),rsa(),ec(), andsymmetric()
Consider adding these to the changelog for completeness.
📝 Suggested additions
## 0.4.8
- **FIX**: JWE JSON parsing now handles missing `recipients` / `header` by deriving a single recipient from the protected header and validates absent `encrypted_key` for non-`dir` algorithms.
- **FEAT**: Support unpadded Base64URL protected header (new parsing test).
+ - **FEAT**: RSA JWK generation now includes CRT parameters (`dp`, `dq`, `qi`) for private keys.
+ - **FEAT**: Added `extractable` parameter to `JsonWebKey.fromCryptoKeys()`, `rsa()`, `ec()`, and `symmetric()` factory methods.
- **TEST**: Added fallback, AAD, key wrap, and error path tests.
- **CHORE**: Refactored `JsonWebEncryption.fromJson` for clarity.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## 0.4.8 | |
| - **FIX**: JWE JSON parsing now handles missing `recipients` / `header` by deriving a single recipient from the protected header and validates absent `encrypted_key` for non-`dir` algorithms. | |
| - **FEAT**: Support unpadded Base64URL protected header (new parsing test). | |
| - **TEST**: Added fallback, AAD, key wrap, and error path tests. | |
| - **CHORE**: Refactored `JsonWebEncryption.fromJson` for clarity. | |
| ## 0.4.8 | |
| - **FIX**: JWE JSON parsing now handles missing `recipients` / `header` by deriving a single recipient from the protected header and validates absent `encrypted_key` for non-`dir` algorithms. | |
| - **FEAT**: Support unpadded Base64URL protected header (new parsing test). | |
| - **FEAT**: RSA JWK generation now includes CRT parameters (`dp`, `dq`, `qi`) for private keys. | |
| - **FEAT**: Added `extractable` parameter to `JsonWebKey.fromCryptoKeys()`, `rsa()`, `ec()`, and `symmetric()` factory methods. | |
| - **TEST**: Added fallback, AAD, key wrap, and error path tests. | |
| - **CHORE**: Refactored `JsonWebEncryption.fromJson` for clarity. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` around lines 1 - 5, The CHANGELOG entry for 0.4.8 is missing
the PR's key features: add notes that JWK generation now supports RSA CRT
parameters (dp, dq, qi) and that the `extractable` flag (`ext`) is now handled;
update the 0.4.8 list to include lines mentioning "RSA CRT parameters (`dp`,
`dq`, `qi`) support in JWK generation" and "support for `extractable`/`ext` flag
in JsonWebKey.fromCryptoKeys(), rsa(), ec(), and symmetric()" so reviewers can
see these features alongside the JWE parsing and tests changes.
|
|
||
| dependencies: | ||
| crypto_keys_plus: ">=0.5.0 <1.0.0" | ||
| crypto_keys_plus: ">=0.4.0 <1.0.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check crypto_keys_plus versions and their RsaPrivateKey API
echo "Checking crypto_keys_plus package info on pub.dev..."
curl -s "https://pub.dev/api/packages/crypto_keys_plus" | jq '.versions | map(.version) | .[-10:]'Repository: Bdaya-Dev/jose
Length of output: 135
🏁 Script executed:
# Search for RsaPrivateKeyCRTCalcExtension usage in the codebase
rg "RsaPrivateKeyCRTCalcExtension|privateExponent|firstPrimeFactor|secondPrimeFactor" -A 2 -B 2Repository: Bdaya-Dev/jose
Length of output: 3369
🏁 Script executed:
# Check the actual pubspec.yaml to confirm the constraint
cat pubspec.yaml | head -20Repository: Bdaya-Dev/jose
Length of output: 513
🏁 Script executed:
# Search for references to crypto_keys_plus in the codebase
rg "crypto_keys_plus" -B 2 -A 2Repository: Bdaya-Dev/jose
Length of output: 1826
🌐 Web query:
crypto_keys_plus 0.4.0 RsaPrivateKey API changelog
💡 Result:
In crypto_keys_plus 0.4.0, there are no documented, direct API changes to RsaPrivateKey itself (its constructor, fields, and methods appear unchanged in the 0.4.0 API docs). [2]
What did change in 0.4.0 that can affect RSA key handling:
- Behavior change when parsing/recognizing key material: the package now returns
nullinstead of throwing when it encounters an unknown curve/key type (this can surface when constructing keys/keypairs from serialized formats such as JWK, depending on input). [1] - Internal refactor: removed the dependency on quiver (not an
RsaPrivateKeyAPI change, but relevant for dependency/changelog tracking). [1]
Sources: crypto_keys_plus 0.4.0 changelog [1], RsaPrivateKey 0.4.0 API docs [2].
Clarify the rationale for lowering the crypto_keys_plus constraint.
The constraint was lowered from >=0.5.0 to >=0.4.0 in this version, but the changelog for 0.4.7 explicitly states "Updated to use crypto_keys_plus 0.5.0". The required properties (privateExponent, firstPrimeFactor, secondPrimeFactor) do exist in crypto_keys_plus 0.4.0, so API compatibility is not a concern. However, the version downgrade contradicts the previous upgrade decision. Please confirm whether this constraint change is intentional and update the changelog entry if needed to reflect this shift in minimum version requirements.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pubspec.yaml` at line 12, Update the pubspec constraint decision for
crypto_keys_plus: either restore the minimum to ">=0.5.0" if the downgrade to
">=0.4.0" was accidental, or keep ">=0.4.0" but update the changelog entry that
currently states "Updated to use crypto_keys_plus 0.5.0" to reflect the
intentional change; reference the package name crypto_keys_plus in pubspec.yaml
and the changelog entry for version 0.4.7 when making the correction so the
minimum-version rationale is consistent across files.
…d-extractable # Conflicts: # lib/src/jwa.dart
|
@ahmednfwela Hi, thanks for supporting this package! |
|
Sorry, this PR is kind of mixup, feel free to mention what you'd like to improve or toss out. |
…d-extractable # Conflicts: # CHANGELOG.md # lib/src/jwe.dart # pubspec.yaml
1 participant
Title
Add RSA CRT fields and extractable support to JWK generation
Summary
This PR improves JWK generation by including full RSA CRT parameters for private keys and adding support for the JWK
ext(extractable) property across key constructors.Changes
dp(first factor CRT exponent)dq(second factor CRT exponent)qi(first CRT coefficient)extractablesupport and serialize it asextin:JsonWebKey.fromCryptoKeys(...)JsonWebKey.rsa(...)JsonWebKey.ec(...)JsonWebKey.symmetric(...)extractablegetter onJsonWebKey.decodeBigInt(String)utility to support BigInt decoding for CRT-related values.ext: truewhere algorithm-driven key generation applies.Why
extaligns JWK output with extractability metadata used in WebCrypto-style workflows.Scope / Compatibility
extpropagation.extractableis optional).Testing
test/jwk_test.darttest/jwa_test.dartSummary by CodeRabbit
New Features
extractableparameter for key generation across RSA, EC, and symmetric keys.Bug Fixes
encrypted_keyis properly handled for different key management algorithms.Tests