feat: implement ECDH-ES key agreement (ECDH-ES, ECDH-ES+A*KW)#13
feat: implement ECDH-ES key agreement (ECDH-ES, ECDH-ES+A*KW)#13ahmednfwela wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR introduces ECDH-ES (Elliptic Curve Diffie-Hellman Ephemeral Static) key agreement support to the library. It implements elliptic curve arithmetic, Concat KDF derivation, and AES key wrap/unwrap for secure key establishment, integrating these primitives into the JWE encryption and decryption pipelines with comprehensive test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant Sender
participant Recipient
participant ECDH as ECDH Module
participant KDF as Concat KDF
participant Wrapper as Key Wrapper
Sender->>ECDH: ecdhEsDerive(recipientPublicKey, algId, keyLen)
ECDH->>ECDH: Generate ephemeral EC keypair
ECDH->>ECDH: Compute shared secret Z via scalar mult
ECDH->>KDF: runKdf(Z, algId, apu, apv)
KDF->>KDF: SHA-256 with counter, structured inputs
KDF-->>ECDH: Derived key material
ECDH->>Wrapper: Wrap CEK with derived key (if A*KW variant)
Wrapper-->>ECDH: Encrypted key
ECDH-->>Sender: {derivedKey, ephemeralPublicKey}
sequenceDiagram
participant Recipient
participant ECDH as ECDH Module
participant KDF as Concat KDF
participant Unwrapper as Key Unwrapper
participant Decryption as Decryption Engine
Recipient->>ECDH: ecdhEsDecrypt(privKey, ephemeralPubKey, algId, keyLen)
ECDH->>ECDH: Validate key types & curves match
ECDH->>ECDH: Compute shared secret Z via scalar mult
ECDH->>KDF: runKdf(Z, algId, apu, apv)
KDF->>KDF: SHA-256 with counter, structured inputs
KDF-->>ECDH: Derived key material
ECDH->>Unwrapper: Unwrap encrypted key (if A*KW variant)
Unwrapper-->>ECDH: Content encryption key
ECDH-->>Recipient: CEK
Recipient->>Decryption: Decrypt payload with CEK
Decryption-->>Recipient: Plaintext
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (3)
lib/src/jwe.dart (1)
297-328: Missingapu/apv(Agreement PartyInfo) support in encryption.The
ecdhEsDerivecall omitsapuandapvparameters, andJsonWebEncryptionBuilderhas no mechanism to set these values. While optional per RFC 7518 §4.6.1, omitting them could cause interoperability issues when decrypting JWEs from other libraries that include these values.Consider adding
apu/apvsetters to the builder for completeness:♻️ Proposed enhancement to support apu/apv
Add to
JsonWebEncryptionBuilder:/// Agreement PartyUInfo for ECDH key agreement (base64url-encoded) Uint8List? agreementPartyUInfo; /// Agreement PartyVInfo for ECDH key agreement (base64url-encoded) Uint8List? agreementPartyVInfo;Then update the
ecdhEsDerivecall:final result = ecdhEsDerive( recipientPublicKey: key, algorithmId: algId, keyDataLen: keyLen, + apu: agreementPartyUInfo, + apv: agreementPartyVInfo, ); +// Add apu/apv to header if present +if (agreementPartyUInfo != null) { + unprotectedHeaderParams['apu'] = encodeBase64EncodedBytes(agreementPartyUInfo!); +} +if (agreementPartyVInfo != null) { + unprotectedHeaderParams['apv'] = encodeBase64EncodedBytes(agreementPartyVInfo!); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/jwe.dart` around lines 297 - 328, Add optional Agreement PartyInfo support by exposing two fields on JsonWebEncryptionBuilder (e.g., Uint8List? agreementPartyUInfo and Uint8List? agreementPartyVInfo) and pass them into ecdhEsDerive when computing the ECDH-ES key; also, when non-null, base64url-encode and include apu/apv in the unprotectedHeaderParams alongside 'epk'. Update the ecdhEsDerive call site in the ECDH-ES branch (the block that computes algId/keyLen and calls ecdhEsDerive) to accept and forward these values, and ensure ecdhEsDerive signature accepts apu/apv, and that ecdhEsWrapKey/CEK logic remains unchanged.lib/src/ecdh.dart (1)
406-409: Missing validation:cek['k']could be null for non-oct keys.The
ecdhEsWrapKeyfunction assumescek['k']exists. If called with a non-symmetric key, this will throw a less helpful error fromdecodeBase64EncodedBytes.♻️ Add defensive validation
List<int> ecdhEsWrapKey(Uint8List derivedKey, JsonWebKey cek) { + final kValue = cek['k']; + if (kValue == null) { + throw ArgumentError('CEK must be a symmetric key with "k" parameter'); + } - final cekBytes = Uint8List.fromList(decodeBase64EncodedBytes(cek['k'])); + final cekBytes = Uint8List.fromList(decodeBase64EncodedBytes(kValue)); return _aesKeyWrap(derivedKey, cekBytes); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/ecdh.dart` around lines 406 - 409, ecdhEsWrapKey currently assumes cek['k'] exists and will pass null into decodeBase64EncodedBytes; add defensive validation at the start of ecdhEsWrapKey to verify that cek contains a non-null, non-empty 'k' entry (and optionally that cek['k'] is a String) and throw a clear ArgumentError (or other domain-specific exception) if missing/invalid; after validation proceed to call decodeBase64EncodedBytes and _aesKeyWrap as before so errors are explicit and handled.lib/src/jwa.dart (1)
172-186: Hardcodedcurve: 'P-256'is inconsistent with actual multi-curve support.RFC 7518 §4.6 specifies that ECDH-ES supports P-256, P-384, and P-521 curves. The runtime implementation in
ecdh.dartcorrectly derives the curve dynamically from the key'scrvfield, and tests verify all three curves work. However, hardcodingcurve: 'P-256'here is misleading because:
generateRandomKey()will always generate P-256 keys even when other curves are valid- The
curvemetadata doesn't reflect actual algorithm capabilitiesConsider either:
- Removing the
curveparameter for ECDH-ES algorithms (let the key determine the curve)- Or documenting that P-256 is merely the default for key generation
♻️ Option: Remove hardcoded curve to reflect actual behavior
/// ECDH-ES using Concat KDF static const ecdh_es = - JsonWebAlgorithm('ECDH-ES', type: 'EC', use: 'key', curve: 'P-256'); + JsonWebAlgorithm('ECDH-ES', type: 'EC', use: 'key'); /// ECDH-ES using Concat KDF and A128KW wrapping static const ecdh_es_a128kw = - JsonWebAlgorithm('ECDH-ES+A128KW', type: 'EC', use: 'key', curve: 'P-256'); + JsonWebAlgorithm('ECDH-ES+A128KW', type: 'EC', use: 'key'); /// ECDH-ES using Concat KDF and A192KW wrapping static const ecdh_es_a192kw = - JsonWebAlgorithm('ECDH-ES+A192KW', type: 'EC', use: 'key', curve: 'P-256'); + JsonWebAlgorithm('ECDH-ES+A192KW', type: 'EC', use: 'key'); /// ECDH-ES using Concat KDF and A256KW wrapping static const ecdh_es_a256kw = - JsonWebAlgorithm('ECDH-ES+A256KW', type: 'EC', use: 'key', curve: 'P-256'); + JsonWebAlgorithm('ECDH-ES+A256KW', type: 'EC', use: 'key');Note: This would require updating
generateCryptoKeyPair()(line 284-285) to handlecurve == nullfor EC types, defaulting to P-256 there explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/jwa.dart` around lines 172 - 186, The ECDH-ES constants (ecdh_es, ecdh_es_a128kw, ecdh_es_a192kw, ecdh_es_a256kw) currently hardcode curve: 'P-256', which misrepresents multi-curve support; remove the curve parameter from these JsonWebAlgorithm declarations (or set it to null) so the key's crv drives behavior, and update generateCryptoKeyPair / generateRandomKey (the functions that read algorithm.curve) to explicitly default to 'P-256' only when algorithm.curve is null for EC types—ensure generateRandomKey no longer always forces P-256 and that generateCryptoKeyPair handles null curve by using P-256 as the default at generation time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/src/ecdh.dart`:
- Around line 49-62: The multiply(BigInt k, BigInt p, BigInt a) method uses a
branching double-and-add loop that leaks scalar bits via timing; replace it with
a constant-time scalar multiplication (e.g., Montgomery ladder) to remove branch
dependence on n.isOdd. Update _ECPoint.multiply to implement a constant-time
ladder (or a constant-time windowed algorithm) that uses only fixed-time
conditional swaps and uniform operations on _ECPoint (refer to the _ECPoint.add
and _ECPoint.double_ helpers), ensuring no data-dependent branches or early loop
exits and that the loop runs a fixed number of iterations based on the scalar
bit-length.
---
Nitpick comments:
In `@lib/src/ecdh.dart`:
- Around line 406-409: ecdhEsWrapKey currently assumes cek['k'] exists and will
pass null into decodeBase64EncodedBytes; add defensive validation at the start
of ecdhEsWrapKey to verify that cek contains a non-null, non-empty 'k' entry
(and optionally that cek['k'] is a String) and throw a clear ArgumentError (or
other domain-specific exception) if missing/invalid; after validation proceed to
call decodeBase64EncodedBytes and _aesKeyWrap as before so errors are explicit
and handled.
In `@lib/src/jwa.dart`:
- Around line 172-186: The ECDH-ES constants (ecdh_es, ecdh_es_a128kw,
ecdh_es_a192kw, ecdh_es_a256kw) currently hardcode curve: 'P-256', which
misrepresents multi-curve support; remove the curve parameter from these
JsonWebAlgorithm declarations (or set it to null) so the key's crv drives
behavior, and update generateCryptoKeyPair / generateRandomKey (the functions
that read algorithm.curve) to explicitly default to 'P-256' only when
algorithm.curve is null for EC types—ensure generateRandomKey no longer always
forces P-256 and that generateCryptoKeyPair handles null curve by using P-256 as
the default at generation time.
In `@lib/src/jwe.dart`:
- Around line 297-328: Add optional Agreement PartyInfo support by exposing two
fields on JsonWebEncryptionBuilder (e.g., Uint8List? agreementPartyUInfo and
Uint8List? agreementPartyVInfo) and pass them into ecdhEsDerive when computing
the ECDH-ES key; also, when non-null, base64url-encode and include apu/apv in
the unprotectedHeaderParams alongside 'epk'. Update the ecdhEsDerive call site
in the ECDH-ES branch (the block that computes algId/keyLen and calls
ecdhEsDerive) to accept and forward these values, and ensure ecdhEsDerive
signature accepts apu/apv, and that ecdhEsWrapKey/CEK logic remains unchanged.
🪄 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: e5619368-6a74-4fe6-b161-821e9275d2f3
📒 Files selected for processing (5)
lib/src/ecdh.dartlib/src/jose.dartlib/src/jwa.dartlib/src/jwe.darttest/ecdh_test.dart
| /// Scalar multiplication using double-and-add. | ||
| _ECPoint multiply(BigInt k, BigInt p, BigInt a) { | ||
| var result = _ECPoint.infinity(); | ||
| var base = this; | ||
| var n = k; | ||
| while (n > BigInt.zero) { | ||
| if (n.isOdd) { | ||
| result = result.add(base, p, a); | ||
| } | ||
| base = base.double_(p, a); | ||
| n >>= 1; | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
❓ Verification inconclusive
What are constant-time scalar multiplication algorithms for elliptic curves?
Constant-time (a.k.a. regular/uniform) elliptic-curve scalar multiplication means computing ([k]P) with a control-flow and memory-access pattern that does not depend on the secret scalar bits/digits (k) (to resist timing/simple power/cache attacks). Common constant-time algorithm families include:
-
Double-and-add-always (regular binary method): per bit, always do one doubling and one addition, then use a constant-time select to keep the correct result. This produces a fixed-length, uniform trace. [1]
-
Ladder methods (one add + one double per bit)
- Montgomery ladder: maintains two related points and, for each bit, performs one differential addition and one doubling with constant-time conditional swaps (
cswap) to avoid secret-dependent branches; widely used for X25519-style x-coordinate-only arithmetic. [1][2] - Variants / generalized ladders: many “ladder-like” designs exist (e.g., for different coordinate systems or with “data-veiling” to avoid secret-dependent memory behavior) while preserving the per-bit uniform operation mix. [2]
- Montgomery ladder: maintains two related points and, for each bit, performs one differential addition and one doubling with constant-time conditional swaps (
-
Fixed-window / windowed methods implemented regularly: precompute a table of small multiples and process the scalar in fixed-size windows, using constant-time table selection (no secret-indexed loads). These can be faster than ladders while remaining regular. [3][4]
-
Multi-scalar (Shamir/Straus/interleaving) in fixed windows: for computing (\sum [k_i]P_i), interleaved fixed-window approaches can be made constant-time with constant-time table access and a fixed schedule. (Often used in signature verification.) [5]
-
Uniform “pseudomultiplication + recovery” frameworks: compute on an x-line/Kummer surface with fast, uniform differential operations, then recover the full point—giving constant-time scalar multiplication beyond pure Diffie–Hellman use-cases. [4]
In practice, constant-time scalar multiplication also relies on exception-free curve formulas (e.g., complete addition laws on Edwards-type models) so the arithmetic itself doesn’t trigger special-case branches on rare inputs. [6]
Sources:
[1] https://www.hyperelliptic.org/tanja/teaching/crypto21/ecc-8.pdf
[2] https://eprint.iacr.org/2017/350.pdf
[3] https://eprint.iacr.org/2011/338
[4] https://arxiv.org/abs/1510.03174
[5] https://doc.cuprate.org/curve25519_dalek/backend/serial/scalar_mul/straus/struct.Straus.html
[6] https://cr.yp.to/newelliptic/edwards2-20080611.pdf
Script executed:
# Search for usage of multiply() method in the codebase
rg "multiply\(" lib/src/ecdh.dart -A 2 -B 2
# Check for any documentation about security implications
rg "timing|constant-time|side.channel" lib/src/ecdh.dart
# Look for the overall context of the file
head -20 lib/src/ecdh.dartRepository: Bdaya-Dev/jose
Repository: Bdaya-Dev/jose
Exit code: 0
stdout:
/// Scalar multiplication using double-and-add.
_ECPoint multiply(BigInt k, BigInt p, BigInt a) {
var result = _ECPoint.infinity();
var base = this;
--
}
final result = q.multiply(privateKey.eccPrivateKey, curve.p, curve.a);
if (result.isInfinity) {
/// ECDH-ES key agreement per [RFC 7518 §4.6](https://tools.ietf.org/html/rfc7518#section-4.6)
library;
import 'dart:typed_data';
import 'package:crypto_keys_plus/crypto_keys.dart';
import 'jwk.dart';
import 'util.dart';
// ---------------------------------------------------------------------------
// EC point arithmetic for Weierstrass curves y² = x³ + ax + b (mod p)
// ---------------------------------------------------------------------------
class _ECPoint {
final BigInt x;
final BigInt y;
final bool isInfinity;
const _ECPoint(this.x, this.y) : isInfinity = false;
Script executed:
# Search for how this library is documented and used
find . -name "README*" -o -name "*.md" | head -10
# Check for any security warnings or advisories in the codebase
rg "warning|WARNING|security|Security" lib/src/ecdh.dart -iRepository: Bdaya-Dev/jose
Repository: Bdaya-Dev/jose
Exit code: 1
stdout:
./CHANGELOG.md
./README.md
Script executed:
# Look for how multiply is called and what keys are being used
rg "multiply" lib/src/ecdh.dart -B 5 -A 5Repository: Bdaya-Dev/jose
Repository: Bdaya-Dev/jose
Exit code: 0
stdout:
final ry = (lambda * (x - rx) - y) % p;
return _ECPoint(rx, ry);
}
/// Scalar multiplication using double-and-add.
_ECPoint multiply(BigInt k, BigInt p, BigInt a) {
var result = _ECPoint.infinity();
var base = this;
var n = k;
while (n > BigInt.zero) {
if (n.isOdd) {
--
final rhs = (q.x * q.x * q.x + curve.a * q.x + curve.b) % curve.p;
if (lhs != rhs) {
throw ArgumentError('Public key point is not on the curve');
}
final result = q.multiply(privateKey.eccPrivateKey, curve.p, curve.a);
if (result.isInfinity) {
throw StateError('ECDH produced point at infinity');
}
return result.x;
Script executed:
# Search for privateKey definition and context
rg "privateKey.*eccPrivateKey" lib/src/ecdh.dart -B 10 -A 5
# Look for the ECDH function signature
rg "perform.*ecdh|ecdh.*agreement" lib/src/ecdh.dart -i -B 2 -A 10
# Check for test files that show usage
find . -name "*test*" -o -name "*_test.dart" | head -5Repository: Bdaya-Dev/jose
Repository: Bdaya-Dev/jose
Exit code: 0
stdout:
final q = _ECPoint(publicKey.xCoordinate, publicKey.yCoordinate);
// Validate that the public key point lies on the curve: y² ≡ x³ + ax + b (mod p)
final lhs = (q.y * q.y) % curve.p;
final rhs = (q.x * q.x * q.x + curve.a * q.x + curve.b) % curve.p;
if (lhs != rhs) {
throw ArgumentError('Public key point is not on the curve');
}
final result = q.multiply(privateKey.eccPrivateKey, curve.p, curve.a);
if (result.isInfinity) {
throw StateError('ECDH produced point at infinity');
}
return result.x;
/// ECDH-ES key agreement per [RFC 7518 §4.6](https://tools.ietf.org/html/rfc7518#section-4.6)
library;
import 'dart:typed_data';
import 'package:crypto_keys_plus/crypto_keys.dart';
import 'jwk.dart';
import 'util.dart';
// ---------------------------------------------------------------------------
--
// ---------------------------------------------------------------------------
BigInt _ecdhAgreement(EcPrivateKey privateKey, EcPublicKey publicKey) {
final curveName = privateKey.curve.name.split('/').last;
final curve = _curveForName(curveName);
final q = _ECPoint(publicKey.xCoordinate, publicKey.yCoordinate);
// Validate that the public key point lies on the curve: y² ≡ x³ + ax + b (mod p)
final lhs = (q.y * q.y) % curve.p;
final rhs = (q.x * q.x * q.x + curve.a * q.x + curve.b) % curve.p;
if (lhs != rhs) {
throw ArgumentError('Public key point is not on the curve');
--
}
/// Sender side: generate ephemeral key, perform ECDH, derive key material.
EcdhEsResult ecdhEsDerive({
required JsonWebKey recipientPublicKey,
required String algorithmId,
required int keyDataLen,
Uint8List? apu,
Uint8List? apv,
KeyPair? ephemeralKeyPair,
}) {
final recipientPublic = recipientPublicKey.cryptoKeyPair.publicKey;
if (recipientPublic is! EcPublicKey) {
--
final ephemeralPublic = ephemeral.publicKey as EcPublicKey;
final z = _ecdhAgreement(ephemeralPrivate, recipientPublic);
final fieldSize = _fieldSizeForCurve(curveName);
final zBytes = _bigIntToBytes(z, fieldSize);
final derivedKey = concatKdf(
zBytes,
keyDataLen: keyDataLen,
algorithmId: algorithmId,
apu: apu,
apv: apv,
);
--
}
final z = _ecdhAgreement(recipientPrivate, ephemeralPublic);
final curveName = recipientCurve;
final fieldSize = _fieldSizeForCurve(curveName);
final zBytes = _bigIntToBytes(z, fieldSize);
return concatKdf(
zBytes,
keyDataLen: keyDataLen,
algorithmId: algorithmId,
apu: apu,
apv: apv,
./test
./test/ecdh_test.dart
./test/jwa_test.dart
./test/jwe_test.dart
./test/jwk_test.dart
Script executed:
# Check the full ECDH function context to understand the flow
sed -n '65,120p' lib/src/ecdh.dartRepository: Bdaya-Dev/jose
Repository: Bdaya-Dev/jose
Exit code: 0
stdout:
class _ECCurve {
final BigInt p;
final BigInt a;
final BigInt b;
final int fieldSize; // in bytes
const _ECCurve(this.p, this.a, this.b, this.fieldSize);
}
// SEC 2 curve parameters
// https://www.secg.org/sec2-v2.pdf
final _p256 = _ECCurve(
BigInt.parse(
'FFFFFFFF00000001000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFF',
radix: 16),
BigInt.parse(
'FFFFFFFF00000001000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFC',
radix: 16),
BigInt.parse(
'5AC635D8AA3A93E7B3EBBD55769886BC651D06B0CC53B0F63BCE3C3E27D2604B',
radix: 16),
32,
);
final _p384 = _ECCurve(
BigInt.parse(
'FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFE'
'FFFFFFFF0000000000000000FFFFFFFF',
radix: 16),
BigInt.parse(
'FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFE'
'FFFFFFFF0000000000000000FFFFFFFC',
radix: 16),
BigInt.parse(
'B3312FA7E23EE7E4988E056BE3F82D19181D9C6EFE8141120314088F5013875A'
'C656398D8A2ED19D2A85C8EDD3EC2AEF',
radix: 16),
48,
);
final _p521 = _ECCurve(
BigInt.parse(
'01FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF'
'FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF'
'FFFF',
radix: 16),
BigInt.parse(
'01FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF'
'FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF'
'FFFC',
radix: 16),
BigInt.parse(
'0051953EB9618E1C9A1F929A21A0B68540EEA2DA725B99B315F3B8B489918EF1'
'09E156193951EC7E937B1652C0BD3BB1BF073573DF883D2C34F1EF451FD46B50'
'3F00',
Script executed:
# Look at how private keys are used in the JOSE/ECDH context
rg "eccPrivateKey" lib/src/ -B 3 -A 3Repository: Bdaya-Dev/jose
Repository: Bdaya-Dev/jose
Exit code: 0
stdout:
lib/src/jwk.dart-
lib/src/jwk.dart- return JsonWebKey.ec(
lib/src/jwk.dart- curve: toCurveName(privateKey.curve),
lib/src/jwk.dart: privateKey: privateKey.eccPrivateKey,
lib/src/jwk.dart- xCoordinate: (publicKey as EcPublicKey?)?.xCoordinate,
lib/src/jwk.dart- yCoordinate: publicKey?.yCoordinate,
lib/src/jwk.dart- keyId: keyId,
--
lib/src/jwa.dart- (keyPair.privateKey as RsaPrivateKey).secondPrimeFactor),
lib/src/jwa.dart- },
lib/src/jwa.dart- if (type == 'EC') ...{
lib/src/jwa.dart: 'd': encodeBigInt((keyPair.privateKey as EcPrivateKey).eccPrivateKey),
lib/src/jwa.dart- 'x': encodeBigInt((keyPair.publicKey as EcPublicKey).xCoordinate),
lib/src/jwa.dart- 'y': encodeBigInt((keyPair.publicKey as EcPublicKey).yCoordinate),
lib/src/jwa.dart- 'crv': curve,
--
lib/src/ecdh.dart- throw ArgumentError('Public key point is not on the curve');
lib/src/ecdh.dart- }
lib/src/ecdh.dart-
lib/src/ecdh.dart: final result = q.multiply(privateKey.eccPrivateKey, curve.p, curve.a);
lib/src/ecdh.dart-
lib/src/ecdh.dart- if (result.isInfinity) {
lib/src/ecdh.dart- throw StateError('ECDH produced point at infinity');
Scalar multiplication is not constant-time (timing side-channel risk).
The double-and-add algorithm branches on bits of the scalar k, making it vulnerable to timing attacks. While this implementation is used with both ephemeral keys (sender side) and long-term/static keys (receiver side), the timing vulnerability is particularly relevant for the static key case, where the same key is reused across multiple ECDH operations.
Consider using Montgomery ladder or windowed methods for constant-time execution if constant-time properties are required.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/src/ecdh.dart` around lines 49 - 62, The multiply(BigInt k, BigInt p,
BigInt a) method uses a branching double-and-add loop that leaks scalar bits via
timing; replace it with a constant-time scalar multiplication (e.g., Montgomery
ladder) to remove branch dependence on n.isOdd. Update _ECPoint.multiply to
implement a constant-time ladder (or a constant-time windowed algorithm) that
uses only fixed-time conditional swaps and uniform operations on _ECPoint (refer
to the _ECPoint.add and _ECPoint.double_ helpers), ensuring no data-dependent
branches or early loop exits and that the loop runs a fixed number of iterations
based on the scalar bit-length.
There was a problem hiding this comment.
Pull request overview
Implements JWE key management via ECDH-ES (direct agreement) and ECDH-ES+A*KW (agreement + AES Key Wrap) and adds tests validating ECDH-ES roundtrips and header structure.
Changes:
- Added an
ecdh.dartimplementation of ECDH-ES (Concat KDF) plus AES Key Wrap/Unwrap helpers. - Integrated ECDH-ES / ECDH-ES+A*KW into JWE build + decrypt flows, including
epkheader support. - Added a new test suite covering compact/JSON roundtrips, curves, and header expectations.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/ecdh_test.dart | Adds ECDH-ES and ECDH-ES+A*KW roundtrip + header verification tests. |
| lib/src/jwe.dart | Adds ECDH-ES CEK derivation / unwrap logic and emits epk during build. |
| lib/src/jwa.dart | Registers new JWA algorithm identifiers for ECDH-ES and ECDH-ES+A*KW. |
| lib/src/jose.dart | Extends JoseHeader with epk, apu, apv accessors. |
| lib/src/ecdh.dart | New ECDH-ES implementation: curve math, Concat KDF, AES-KW wrap/unwrap. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final result = ecdhEsDerive( | ||
| recipientPublicKey: key, | ||
| algorithmId: algId, | ||
| keyDataLen: keyLen, | ||
| ); |
There was a problem hiding this comment.
ecdhEsDerive(...) is always called without apu/apv, but decryption does incorporate apu/apv from the JOSE header. If a caller sets apu/apv via setProtectedHeader, encryption will derive a different key than decryption expects and roundtrips/interop will fail. Please plumb apu/apv (decoded from the header’s base64url values) into ecdhEsDerive and ensure the same values are written to the header.
| while (n > BigInt.zero) { | ||
| if (n.isOdd) { | ||
| result = result.add(base, p, a); | ||
| } | ||
| base = base.double_(p, a); |
There was a problem hiding this comment.
_ECPoint.multiply uses a classic double-and-add loop with a conditional branch on each bit of the scalar (n.isOdd). Because this is used with private scalars during ECDH, it is not constant-time and can leak key-dependent timing information. Prefer a constant-time ECDH implementation from a vetted crypto provider (or ensure scalar multiplication is constant-time) rather than custom BigInt-based point arithmetic.
| final curve = _curveForName(curveName); | ||
|
|
||
| final q = _ECPoint(publicKey.xCoordinate, publicKey.yCoordinate); | ||
|
|
There was a problem hiding this comment.
The public key validation in _ecdhAgreement reduces coordinates modulo p when checking the curve equation, but it does not reject non-canonical coordinates (e.g., x >= p or y >= p). For stricter input validation and to reduce attack surface, explicitly check that 0 <= x < p and 0 <= y < p before the on-curve test (and reject otherwise).
| if (q.x < BigInt.zero || | |
| q.x >= curve.p || | |
| q.y < BigInt.zero || | |
| q.y >= curve.p) { | |
| throw ArgumentError('Public key coordinates are out of range'); | |
| } |
| JsonWebAlgorithm('ECDH-ES', type: 'EC', use: 'key', curve: 'P-256'); | ||
|
|
||
| /// ECDH-ES using Concat KDF and A128KW wrapping | ||
| static const ecdh_es_a128kw = | ||
| JsonWebAlgorithm('ECDH-ES+A128KW', type: 'EC', use: 'key', curve: 'P-256'); | ||
|
|
||
| /// ECDH-ES using Concat KDF and A192KW wrapping | ||
| static const ecdh_es_a192kw = | ||
| JsonWebAlgorithm('ECDH-ES+A192KW', type: 'EC', use: 'key', curve: 'P-256'); | ||
|
|
||
| /// ECDH-ES using Concat KDF and A256KW wrapping | ||
| static const ecdh_es_a256kw = | ||
| JsonWebAlgorithm('ECDH-ES+A256KW', type: 'EC', use: 'key', curve: 'P-256'); |
There was a problem hiding this comment.
The new ECDH-ES algorithm constants hardcode curve: 'P-256', which means JsonWebKey.generate('ECDH-ES') (and the +A*KW variants) will always generate P-256 keys even though the feature is intended to be compatible with P-384 and P-521 as well. Consider not pinning a specific curve for these algorithms (or add a way to choose the curve when generating ECDH keys) so callers can generate P-384/P-521 ECDH keys without workarounds.
| JsonWebAlgorithm('ECDH-ES', type: 'EC', use: 'key', curve: 'P-256'); | |
| /// ECDH-ES using Concat KDF and A128KW wrapping | |
| static const ecdh_es_a128kw = | |
| JsonWebAlgorithm('ECDH-ES+A128KW', type: 'EC', use: 'key', curve: 'P-256'); | |
| /// ECDH-ES using Concat KDF and A192KW wrapping | |
| static const ecdh_es_a192kw = | |
| JsonWebAlgorithm('ECDH-ES+A192KW', type: 'EC', use: 'key', curve: 'P-256'); | |
| /// ECDH-ES using Concat KDF and A256KW wrapping | |
| static const ecdh_es_a256kw = | |
| JsonWebAlgorithm('ECDH-ES+A256KW', type: 'EC', use: 'key', curve: 'P-256'); | |
| JsonWebAlgorithm('ECDH-ES', type: 'EC', use: 'key'); | |
| /// ECDH-ES using Concat KDF and A128KW wrapping | |
| static const ecdh_es_a128kw = | |
| JsonWebAlgorithm('ECDH-ES+A128KW', type: 'EC', use: 'key'); | |
| /// ECDH-ES using Concat KDF and A192KW wrapping | |
| static const ecdh_es_a192kw = | |
| JsonWebAlgorithm('ECDH-ES+A192KW', type: 'EC', use: 'key'); | |
| /// ECDH-ES using Concat KDF and A256KW wrapping | |
| static const ecdh_es_a256kw = | |
| JsonWebAlgorithm('ECDH-ES+A256KW', type: 'EC', use: 'key'); |
| ..addRecipient(JsonWebKey.fromJson({'kty': 'oct', 'k': ''})!, | ||
| algorithm: 'none'); |
There was a problem hiding this comment.
For the inner JWS using alg: 'none', you can pass null as the recipient key instead of constructing an oct JWK with an empty k value. Using a dummy empty symmetric key is unnecessary here and may become brittle if JWK parsing tightens validation.
| ..addRecipient(JsonWebKey.fromJson({'kty': 'oct', 'k': ''})!, | |
| algorithm: 'none'); | |
| ..addRecipient(null, algorithm: 'none'); |
Summary by CodeRabbit
Release Notes