feat: Post-Quantum Extended Diffie–Hellman (PQXDH) on the Mobile #1316
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughA PQXDH (X25519 + ML-KEM-768) post-quantum key exchange implementation is added: new crypto primitives, types, and test suites in common, plus suite negotiation and dual-path proving logic in the mobile SDK proving machine and related test/config updates. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant TEE as TEE_Server
rect rgb(240,248,255)
note over Client,TEE: Hello & Suite Negotiation
Client->>TEE: WS hello (user_pubkey, uuid, supported_suites)
TEE->>Client: hello_response (attestation, selected_suite, pubkeys)
end
alt selected_suite == 'Self-PQXDH-1'
rect rgb(220,255,230)
note over Client: PQXDH path (post-quantum)
Client->>Client: kyberEncapsulate(server_kyber_pubkey) → {kyberShared, ciphertext}
Client->>Client: computeX25519SharedSecret(client_priv, server_x25519_pub) → x25519Shared
Client->>Client: deriveSessionKey(x25519Shared, kyberShared) → sessionKey
Client->>TEE: WS key_exchange (kyber_ciphertext)
TEE->>Client: key_exchange_complete
Client->>Client: Transition to CONNECT_SUCCESS
end
else selected_suite == 'legacy-p256'
rect rgb(255,240,230)
note over Client: Legacy P-256 ECDH
Client->>Client: deriveECDH(client_p256_priv, server_p256_pub) → sessionKey
Client->>Client: Transition to CONNECT_SUCCESS
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes Areas requiring focused attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
common/tests/pqxdh-crypto.test.ts (1)
16-228: LGTM: Excellent test coverage for PQXDH implementation.The test suite comprehensively validates:
- Cryptographic primitive correctness (keypair generation, encapsulation, shared secrets)
- Deterministic behavior where expected (HKDF derivation)
- Probabilistic behavior where expected (Kyber ciphertext randomization)
- Signal PQXDH specification compliance
- End-to-end key exchange agreement
Once input validation is added to the crypto functions, consider adding negative test cases to verify error handling for invalid input lengths.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
common/package.json(1 hunks)common/src/utils/proving.ts(3 hunks)common/src/utils/proving/pqxdh-crypto.ts(1 hunks)common/src/utils/proving/pqxdh-types.ts(1 hunks)common/tests/pqxdh-crypto.test.ts(1 hunks)common/vitest.config.ts(1 hunks)packages/mobile-sdk-alpha/src/constants/analytics.ts(1 hunks)packages/mobile-sdk-alpha/src/proving/provingMachine.ts(9 hunks)packages/mobile-sdk-alpha/vitest.config.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/package.json
📄 CodeRabbit inference engine (AGENTS.md)
Use Yarn only for package management (yarn install/add/remove); do not use npm or pnpm in scripts
Files:
common/package.json
packages/mobile-sdk-alpha/**/*.{ts,tsx}
📄 CodeRabbit inference engine (packages/mobile-sdk-alpha/AGENTS.md)
packages/mobile-sdk-alpha/**/*.{ts,tsx}: Use strict TypeScript type checking across the codebase
Follow ESLint TypeScript-specific rules
Avoid introducing circular dependencies
Files:
packages/mobile-sdk-alpha/src/constants/analytics.tspackages/mobile-sdk-alpha/vitest.config.tspackages/mobile-sdk-alpha/src/proving/provingMachine.ts
**/*.{js,ts,tsx,jsx,sol,nr}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,sol,nr}: NEVER log sensitive data including PII (names, DOB, passport numbers, addresses), credentials, tokens, API keys, private keys, or session identifiers.
ALWAYS redact/mask sensitive fields in logs using consistent patterns (e.g.,***-***-1234for passport numbers,J*** D***for names).
Files:
packages/mobile-sdk-alpha/src/constants/analytics.tscommon/vitest.config.tscommon/tests/pqxdh-crypto.test.tspackages/mobile-sdk-alpha/vitest.config.tscommon/src/utils/proving/pqxdh-crypto.tscommon/src/utils/proving.tscommon/src/utils/proving/pqxdh-types.tspackages/mobile-sdk-alpha/src/proving/provingMachine.ts
packages/mobile-sdk-alpha/**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
packages/mobile-sdk-alpha/**/*.{ts,tsx,js,jsx}: Review alpha mobile SDK code for:
- API consistency with core SDK
- Platform-neutral abstractions
- Performance considerations
- Clear experimental notes or TODOs
Files:
packages/mobile-sdk-alpha/src/constants/analytics.tspackages/mobile-sdk-alpha/vitest.config.tspackages/mobile-sdk-alpha/src/proving/provingMachine.ts
**/*.{test,spec}.{ts,js,tsx,jsx}
⚙️ CodeRabbit configuration file
**/*.{test,spec}.{ts,js,tsx,jsx}: Review test files for:
- Test coverage completeness
- Test case quality and edge cases
- Mock usage appropriateness
- Test readability and maintainability
Files:
common/tests/pqxdh-crypto.test.ts
packages/mobile-sdk-alpha/vitest.config.ts
📄 CodeRabbit inference engine (.cursor/rules/mobile-sdk-migration.mdc)
Use Vitest in the SDK with a Node environment configured in packages/mobile-sdk-alpha/vitest.config.ts
Files:
packages/mobile-sdk-alpha/vitest.config.ts
common/src/**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
common/src/**/*.{ts,tsx,js,jsx}: Review shared utilities for:
- Reusability and modular design
- Type safety and error handling
- Side-effect management
- Documentation and naming clarity
Files:
common/src/utils/proving/pqxdh-crypto.tscommon/src/utils/proving.tscommon/src/utils/proving/pqxdh-types.ts
packages/mobile-sdk-alpha/src/proving/**
📄 CodeRabbit inference engine (.cursor/rules/mobile-sdk-migration.mdc)
Place proof input generation in packages/mobile-sdk-alpha/src/proving/
Files:
packages/mobile-sdk-alpha/src/proving/provingMachine.ts
🧠 Learnings (4)
📚 Learning: 2025-08-24T18:54:04.809Z
Learnt from: CR
PR: selfxyz/self#0
File: .cursor/rules/mobile-sdk-migration.mdc:0-0
Timestamp: 2025-08-24T18:54:04.809Z
Learning: Applies to packages/mobile-sdk-alpha/vitest.config.ts : Use Vitest in the SDK with a Node environment configured in packages/mobile-sdk-alpha/vitest.config.ts
Applied to files:
packages/mobile-sdk-alpha/vitest.config.ts
📚 Learning: 2025-08-25T14:25:57.586Z
Learnt from: aaronmgdr
PR: selfxyz/self#951
File: app/src/providers/authProvider.web.tsx:17-18
Timestamp: 2025-08-25T14:25:57.586Z
Learning: The selfxyz/mobile-sdk-alpha/constants/analytics import path is properly configured with SDK exports, Metro aliases, and TypeScript resolution. Import changes from @/consts/analytics to this path are part of valid analytics migration, not TypeScript resolution issues.
Applied to files:
packages/mobile-sdk-alpha/vitest.config.ts
📚 Learning: 2025-08-24T18:54:04.809Z
Learnt from: CR
PR: selfxyz/self#0
File: .cursor/rules/mobile-sdk-migration.mdc:0-0
Timestamp: 2025-08-24T18:54:04.809Z
Learning: Applies to packages/mobile-sdk-alpha/tests/setup.ts : Provide Vitest setup file at packages/mobile-sdk-alpha/tests/setup.ts to suppress console noise
Applied to files:
packages/mobile-sdk-alpha/vitest.config.ts
📚 Learning: 2025-08-29T15:31:15.924Z
Learnt from: CR
PR: selfxyz/self#0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-08-29T15:31:15.924Z
Learning: Applies to packages/mobile-sdk-alpha/{**/*.test.{ts,tsx},**/__tests__/**/*.{ts,tsx}} : Use actual imports from selfxyz/mobile-sdk-alpha in tests
Applied to files:
packages/mobile-sdk-alpha/vitest.config.ts
🧬 Code graph analysis (2)
common/src/utils/proving.ts (1)
common/src/utils/proving/pqxdh-crypto.ts (1)
generateX25519Keypair(12-23)
packages/mobile-sdk-alpha/src/proving/provingMachine.ts (2)
common/src/utils/proving.ts (1)
x25519Keys(38-38)packages/mobile-sdk-alpha/src/constants/analytics.ts (1)
ProofEvents(138-203)
🔇 Additional comments (13)
common/package.json (1)
664-666: LGTM: Reputable cryptographic dependencies added.The @noble/curves and @noble/post-quantum libraries are well-maintained, audited cryptographic primitives from a trusted source. These dependencies are appropriate for implementing post-quantum key exchange.
packages/mobile-sdk-alpha/src/constants/analytics.ts (1)
163-166: LGTM: Analytics events properly defined.The new PQXDH key exchange events follow the established naming pattern and are appropriately placed within the ProofEvents group.
common/vitest.config.ts (1)
10-14: LGTM: Test module resolution fix.The alias ensures proper ESM module resolution for @noble/curves in the Vitest environment, addressing the .js extension requirement.
packages/mobile-sdk-alpha/vitest.config.ts (1)
8-12: LGTM: Consistent module resolution across packages.This mirrors the alias in common/vitest.config.ts, ensuring uniform test module resolution.
common/src/utils/proving/pqxdh-crypto.ts (2)
12-23: LGTM: X25519 keypair generation is correct.The implementation properly uses cryptographically secure random bytes and derives the public key correctly.
88-90: LGTM: Suite preference order is appropriate.Prioritizing PQXDH over legacy P-256 aligns with security best practices for post-quantum readiness.
common/src/utils/proving.ts (2)
37-38: Verify: Module-level keypair generation may limit session isolation.The X25519 keypair is generated once at module load time, similar to the legacy P-256
clientKey. Confirm whether:
- Keys should be regenerated per session/connection for better forward secrecy
- This static approach is intentional to match existing architecture
- Keys are rotated elsewhere in the lifecycle
Based on the PR objectives stating "client-side support" and the pattern matching the existing
clientKeybehavior, this may be intentional, but please verify the security model aligns with your threat model for key rotation and forward secrecy.
115-126: LGTM: Clean PQXDH API surface.The re-exports provide a well-organized public interface for PQXDH functionality with clear separation between types and implementation.
common/src/utils/proving/pqxdh-types.ts (1)
1-43: LGTM: Well-documented type definitions.The PQXDH types are clearly defined with appropriate comments explaining conditional fields. The structure aligns well with the protocol flow.
packages/mobile-sdk-alpha/src/proving/provingMachine.ts (4)
879-897: LGTM - Suite negotiation properly implemented.The WebSocket hello correctly advertises both the legacy P-256 public key (for backward compatibility) and the list of supported cryptographic suites. The TEE can then select the strongest mutually-supported suite. Logging includes only non-sensitive protocol information.
560-575: LGTM - User public key validation is security-critical and correctly implemented.The attestation validation correctly verifies that the user public key embedded in the TEE attestation matches the expected key for the selected cryptographic suite. This prevents man-in-the-middle attacks and ensures the TEE has bound the correct client identity.
979-980: LGTM - State cleanup properly includes new PQXDH fields.The new
selectedSuiteandkyberCiphertextfields are correctly reset to null during initialization and connection cleanup, preventing state leakage between proving sessions.Also applies to: 1531-1532
619-627: No performance concern—these cryptographic operations are appropriately implemented.The synchronous crypto operations (Kyber encapsulation, X25519 ECDH, and HKDF) are correctly wrapping optimized, battle-tested libraries. These execute once during PQXDH key exchange setup, not per-frame, making the 1–2ms overhead from ML-KEM-768 entirely acceptable. X25519 and HKDF are microsecond-scale operations. The code follows Signal's PQXDH specification and requires no refactoring.
| export function kyberEncapsulate(kyberPublicKey: Uint8Array): { | ||
| sharedSecret: Uint8Array; | ||
| ciphertext: Uint8Array; | ||
| } { | ||
|
|
||
| // encapsulating with the server's Kyber public key to get shared secret and ciphertext | ||
| const { cipherText, sharedSecret } = ml_kem768.encapsulate(kyberPublicKey); | ||
|
|
||
| return { | ||
| sharedSecret, | ||
| ciphertext: cipherText, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Add input validation for Kyber public key length.
ML-KEM-768 requires a 1184-byte public key, but the function doesn't validate the input length. Passing an incorrect size could lead to cryptographic failures or security vulnerabilities.
export function kyberEncapsulate(kyberPublicKey: Uint8Array): {
sharedSecret: Uint8Array;
ciphertext: Uint8Array;
} {
+ if (kyberPublicKey.length !== 1184) {
+ throw new Error(`Invalid Kyber public key length: expected 1184 bytes, got ${kyberPublicKey.length}`);
+ }
// encapsulating with the server's Kyber public key to get shared secret and ciphertext
const { cipherText, sharedSecret } = ml_kem768.encapsulate(kyberPublicKey);📝 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.
| export function kyberEncapsulate(kyberPublicKey: Uint8Array): { | |
| sharedSecret: Uint8Array; | |
| ciphertext: Uint8Array; | |
| } { | |
| // encapsulating with the server's Kyber public key to get shared secret and ciphertext | |
| const { cipherText, sharedSecret } = ml_kem768.encapsulate(kyberPublicKey); | |
| return { | |
| sharedSecret, | |
| ciphertext: cipherText, | |
| }; | |
| } | |
| export function kyberEncapsulate(kyberPublicKey: Uint8Array): { | |
| sharedSecret: Uint8Array; | |
| ciphertext: Uint8Array; | |
| } { | |
| if (kyberPublicKey.length !== 1184) { | |
| throw new Error(`Invalid Kyber public key length: expected 1184 bytes, got ${kyberPublicKey.length}`); | |
| } | |
| // encapsulating with the server's Kyber public key to get shared secret and ciphertext | |
| const { cipherText, sharedSecret } = ml_kem768.encapsulate(kyberPublicKey); | |
| return { | |
| sharedSecret, | |
| ciphertext: cipherText, | |
| }; | |
| } |
🤖 Prompt for AI Agents
In common/src/utils/proving/pqxdh-crypto.ts around lines 28 to 40, the
kyberEncapsulate function lacks validation of the kyberPublicKey length
(ML-KEM-768 requires exactly 1184 bytes); add an input check that the argument
is a Uint8Array and its length === 1184 and throw a clear Error if validation
fails (e.g., "Invalid Kyber public key length: expected 1184 bytes"); perform
this validation before calling ml_kem768.encapsulate to avoid passing malformed
input to the crypto library.
| export function computeX25519SharedSecret(privateKey: Uint8Array, serverPublicKey: Uint8Array): Uint8Array { | ||
| // computing the X25519 shared secret using ECDH | ||
| return x25519.getSharedSecret(privateKey, serverPublicKey); | ||
| } |
There was a problem hiding this comment.
Add input validation for X25519 key lengths.
X25519 requires 32-byte keys. Validate inputs to prevent cryptographic failures and improve error diagnostics.
export function computeX25519SharedSecret(privateKey: Uint8Array, serverPublicKey: Uint8Array): Uint8Array {
+ if (privateKey.length !== 32) {
+ throw new Error(`Invalid X25519 private key length: expected 32 bytes, got ${privateKey.length}`);
+ }
+ if (serverPublicKey.length !== 32) {
+ throw new Error(`Invalid X25519 public key length: expected 32 bytes, got ${serverPublicKey.length}`);
+ }
+
// computing the X25519 shared secret using ECDH
return x25519.getSharedSecret(privateKey, serverPublicKey);
}📝 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.
| export function computeX25519SharedSecret(privateKey: Uint8Array, serverPublicKey: Uint8Array): Uint8Array { | |
| // computing the X25519 shared secret using ECDH | |
| return x25519.getSharedSecret(privateKey, serverPublicKey); | |
| } | |
| export function computeX25519SharedSecret(privateKey: Uint8Array, serverPublicKey: Uint8Array): Uint8Array { | |
| if (privateKey.length !== 32) { | |
| throw new Error(`Invalid X25519 private key length: expected 32 bytes, got ${privateKey.length}`); | |
| } | |
| if (serverPublicKey.length !== 32) { | |
| throw new Error(`Invalid X25519 public key length: expected 32 bytes, got ${serverPublicKey.length}`); | |
| } | |
| // computing the X25519 shared secret using ECDH | |
| return x25519.getSharedSecret(privateKey, serverPublicKey); | |
| } |
🤖 Prompt for AI Agents
In common/src/utils/proving/pqxdh-crypto.ts around lines 46 to 49, the function
computeX25519SharedSecret lacks input validation for X25519 keys; add checks
that both privateKey and serverPublicKey are Uint8Array instances of length 32
and throw a clear, specific RangeError (or TypeError for wrong type) with a
message like "X25519 keys must be 32 bytes" before calling
x25519.getSharedSecret to fail fast and improve diagnostics.
| export function deriveSessionKey( | ||
| x25519Shared: Uint8Array, | ||
| kyberShared: Uint8Array, | ||
| ): Buffer { | ||
|
|
||
| // creating F prefix (32 0xFF bytes) per Signal PQXDH spec | ||
| // ensures the IKM is never a valid curve25519 scalar or point encoding | ||
| const F = new Uint8Array(32).fill(0xff); | ||
|
|
||
| // concatenating the two shared secrets (X25519 || Kyber) to form KM | ||
| const KM = new Uint8Array(x25519Shared.length + kyberShared.length); | ||
| KM.set(x25519Shared, 0); | ||
| KM.set(kyberShared, x25519Shared.length); | ||
|
|
||
| // combining F and KM to form the input key material (IKM = F || KM) | ||
| const ikm = new Uint8Array(F.length + KM.length); | ||
| ikm.set(F, 0); | ||
| ikm.set(KM, F.length); | ||
|
|
||
| // using zero-filled salt (32 bytes for SHA-256 output length) per Signal spec | ||
| const salt = new Uint8Array(32).fill(0); | ||
|
|
||
| // encoding the info string following the pattern "protocol_curve_hash_pqkem" | ||
| // per Signal spec: "MyProtocol_CURVE25519_SHA-512_CRYSTALS-KYBER-1024" | ||
| const info = new TextEncoder().encode('Self-PQXDH-1_X25519_SHA-256_ML-KEM-768'); | ||
|
|
||
| // deriving the final 32-byte session key using HKDF-SHA256 | ||
| const sessionKey = hkdf(sha256, ikm, salt, info, 32); | ||
|
|
||
| return Buffer.from(sessionKey); | ||
| } |
There was a problem hiding this comment.
Add input validation for shared secret lengths.
Both shared secrets must be 32 bytes for the HKDF computation to work correctly per Signal PQXDH spec. Validate to catch integration errors early.
export function deriveSessionKey(
x25519Shared: Uint8Array,
kyberShared: Uint8Array,
): Buffer {
+ if (x25519Shared.length !== 32) {
+ throw new Error(`Invalid X25519 shared secret length: expected 32 bytes, got ${x25519Shared.length}`);
+ }
+ if (kyberShared.length !== 32) {
+ throw new Error(`Invalid Kyber shared secret length: expected 32 bytes, got ${kyberShared.length}`);
+ }
// creating F prefix (32 0xFF bytes) per Signal PQXDH spec📝 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.
| export function deriveSessionKey( | |
| x25519Shared: Uint8Array, | |
| kyberShared: Uint8Array, | |
| ): Buffer { | |
| // creating F prefix (32 0xFF bytes) per Signal PQXDH spec | |
| // ensures the IKM is never a valid curve25519 scalar or point encoding | |
| const F = new Uint8Array(32).fill(0xff); | |
| // concatenating the two shared secrets (X25519 || Kyber) to form KM | |
| const KM = new Uint8Array(x25519Shared.length + kyberShared.length); | |
| KM.set(x25519Shared, 0); | |
| KM.set(kyberShared, x25519Shared.length); | |
| // combining F and KM to form the input key material (IKM = F || KM) | |
| const ikm = new Uint8Array(F.length + KM.length); | |
| ikm.set(F, 0); | |
| ikm.set(KM, F.length); | |
| // using zero-filled salt (32 bytes for SHA-256 output length) per Signal spec | |
| const salt = new Uint8Array(32).fill(0); | |
| // encoding the info string following the pattern "protocol_curve_hash_pqkem" | |
| // per Signal spec: "MyProtocol_CURVE25519_SHA-512_CRYSTALS-KYBER-1024" | |
| const info = new TextEncoder().encode('Self-PQXDH-1_X25519_SHA-256_ML-KEM-768'); | |
| // deriving the final 32-byte session key using HKDF-SHA256 | |
| const sessionKey = hkdf(sha256, ikm, salt, info, 32); | |
| return Buffer.from(sessionKey); | |
| } | |
| export function deriveSessionKey( | |
| x25519Shared: Uint8Array, | |
| kyberShared: Uint8Array, | |
| ): Buffer { | |
| if (x25519Shared.length !== 32) { | |
| throw new Error(`Invalid X25519 shared secret length: expected 32 bytes, got ${x25519Shared.length}`); | |
| } | |
| if (kyberShared.length !== 32) { | |
| throw new Error(`Invalid Kyber shared secret length: expected 32 bytes, got ${kyberShared.length}`); | |
| } | |
| // creating F prefix (32 0xFF bytes) per Signal PQXDH spec | |
| // ensures the IKM is never a valid curve25519 scalar or point encoding | |
| const F = new Uint8Array(32).fill(0xff); | |
| // concatenating the two shared secrets (X25519 || Kyber) to form KM | |
| const KM = new Uint8Array(x25519Shared.length + kyberShared.length); | |
| KM.set(x25519Shared, 0); | |
| KM.set(kyberShared, x25519Shared.length); | |
| // combining F and KM to form the input key material (IKM = F || KM) | |
| const ikm = new Uint8Array(F.length + KM.length); | |
| ikm.set(F, 0); | |
| ikm.set(KM, F.length); | |
| // using zero-filled salt (32 bytes for SHA-256 output length) per Signal spec | |
| const salt = new Uint8Array(32).fill(0); | |
| // encoding the info string following the pattern "protocol_curve_hash_pqkem" | |
| // per Signal spec: "MyProtocol_CURVE25519_SHA-512_CRYSTALS-KYBER-1024" | |
| const info = new TextEncoder().encode('Self-PQXDH-1_X25519_SHA-256_ML-KEM-768'); | |
| // deriving the final 32-byte session key using HKDF-SHA256 | |
| const sessionKey = hkdf(sha256, ikm, salt, info, 32); | |
| return Buffer.from(sessionKey); | |
| } |
🤖 Prompt for AI Agents
In common/src/utils/proving/pqxdh-crypto.ts around lines 55 to 85, the function
deriveSessionKey does not validate inputs; add explicit checks that both
x25519Shared and kyberShared are exactly 32 bytes long and throw a clear Error
(or return) if not, to fail fast on integration mistakes. Place the validations
at the top of the function before any concatenation (check both are Uint8Array
and length === 32), and include a descriptive message like "invalid shared
secret length: expected 32 bytes for x25519Shared/kyberShared" so callers can
diagnose the problem immediately.
| const helloResponse = result.result as HelloResponse; | ||
| const attestationData = helloResponse.attestation; | ||
| // extracting the selected suite from TEE response, defaulting to legacy for backward compatibility | ||
| const selectedSuite = helloResponse.selected_suite || 'legacy-p256'; | ||
|
|
||
| // storing attestation data and suite selection in state | ||
| set({ | ||
| attestation: attestationData, | ||
| selectedSuite, | ||
| }); | ||
|
|
||
| selfClient.logProofEvent('info', 'Suite selected by TEE', context, { | ||
| selected_suite: selectedSuite, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Validate HelloResponse structure before type assertion.
The type assertion result.result as HelloResponse assumes the TEE response matches the expected structure. If the TEE returns a different format or is missing fields, this could lead to runtime errors downstream (especially when accessing optional fields like x25519_pubkey and kyber_pubkey).
Consider validating critical fields before proceeding:
const helloResponse = result.result as HelloResponse;
const attestationData = helloResponse.attestation;
+
+// Validate response structure
+if (!attestationData) {
+ console.error('Missing attestation in TEE response');
+ actor!.send({ type: 'CONNECT_ERROR' });
+ return;
+}
+
// extracting the selected suite from TEE response, defaulting to legacy for backward compatibility
const selectedSuite = helloResponse.selected_suite || 'legacy-p256';📝 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.
| const helloResponse = result.result as HelloResponse; | |
| const attestationData = helloResponse.attestation; | |
| // extracting the selected suite from TEE response, defaulting to legacy for backward compatibility | |
| const selectedSuite = helloResponse.selected_suite || 'legacy-p256'; | |
| // storing attestation data and suite selection in state | |
| set({ | |
| attestation: attestationData, | |
| selectedSuite, | |
| }); | |
| selfClient.logProofEvent('info', 'Suite selected by TEE', context, { | |
| selected_suite: selectedSuite, | |
| }); | |
| const helloResponse = result.result as HelloResponse; | |
| const attestationData = helloResponse.attestation; | |
| // Validate response structure | |
| if (!attestationData) { | |
| console.error('Missing attestation in TEE response'); | |
| actor!.send({ type: 'CONNECT_ERROR' }); | |
| return; | |
| } | |
| // extracting the selected suite from TEE response, defaulting to legacy for backward compatibility | |
| const selectedSuite = helloResponse.selected_suite || 'legacy-p256'; | |
| // storing attestation data and suite selection in state | |
| set({ | |
| attestation: attestationData, | |
| selectedSuite, | |
| }); | |
| selfClient.logProofEvent('info', 'Suite selected by TEE', context, { | |
| selected_suite: selectedSuite, | |
| }); |
🤖 Prompt for AI Agents
In packages/mobile-sdk-alpha/src/proving/provingMachine.ts around lines 534 to
548, the code blindly casts result.result to HelloResponse which can cause
runtime errors if the TEE response is malformed or missing fields; add a runtime
validation step (a type guard or simple checks) to confirm result.result is an
object and contains the critical fields you rely on (at minimum attestation and
the selected_suite or other expected keys like x25519_pubkey/kyber_pubkey when
required), log and handle invalid responses (return/throw an error or set an
error state) instead of proceeding, only set the state and call logProofEvent
after the validation passes, and keep the existing default for selected_suite
when that field is absent.
| } else if (result.id === 3 && result.result === 'key_exchange_complete') { | ||
| // receiving acknowledgment that PQXDH key exchange was successful | ||
| selfClient.logProofEvent('info', 'Key exchange complete', context); | ||
| selfClient?.trackEvent(ProofEvents.PQXDH_KEY_EXCHANGE_COMPLETE); | ||
| // key exchange complete, transitioning to connected state | ||
| actor!.send({ type: 'CONNECT_SUCCESS' }); |
There was a problem hiding this comment.
Add timeout mechanism for key exchange acknowledgment.
After sending the Kyber ciphertext, the code waits indefinitely for the TEE to respond with key_exchange_complete. If the TEE fails to respond (network issue, TEE crash, etc.), the connection will hang in the init_tee_connexion state with no way to recover.
Consider implementing a timeout mechanism that transitions to CONNECT_ERROR if the acknowledgment isn't received within a reasonable timeframe (e.g., 30 seconds).
🤖 Prompt for AI Agents
In packages/mobile-sdk-alpha/src/proving/provingMachine.ts around lines 664 to
669, the handler for result.id===3 (key_exchange_complete) currently waits
forever; add a timeout started when the Kyber ciphertext is sent (or when
entering the init_tee_connexion state) that will be cleared on receipt of
key_exchange_complete and will send actor!.send({ type: 'CONNECT_ERROR' }) and
log/track an error if it fires (use ~30s); ensure the timeout reference is
stored on the actor/selfClient scope so it can be cleared here, and on timeout
call the same cleanup/metrics as a normal error branch to avoid leaking state.
ArmanKolozyan
changed the title
ArmanKolozyan
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/tests/pqxdh-cross-language.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,ts,tsx,jsx,sol,nr}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,sol,nr}: NEVER log sensitive data including PII (names, DOB, passport numbers, addresses), credentials, tokens, API keys, private keys, or session identifiers.
ALWAYS redact/mask sensitive fields in logs using consistent patterns (e.g.,***-***-1234for passport numbers,J*** D***for names).
Files:
common/tests/pqxdh-cross-language.test.ts
**/*.{test,spec}.{ts,js,tsx,jsx}
⚙️ CodeRabbit configuration file
**/*.{test,spec}.{ts,js,tsx,jsx}: Review test files for:
- Test coverage completeness
- Test case quality and edge cases
- Mock usage appropriateness
- Test readability and maintainability
Files:
common/tests/pqxdh-cross-language.test.ts
🧠 Learnings (1)
📚 Learning: 2025-08-29T15:31:15.924Z
Learnt from: CR
PR: selfxyz/self#0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-08-29T15:31:15.924Z
Learning: Applies to packages/mobile-sdk-alpha/{**/*.test.{ts,tsx},**/__tests__/**/*.{ts,tsx}} : Test isPassportDataValid() with realistic synthetic passport data (never real user data)
Applied to files:
common/tests/pqxdh-cross-language.test.ts
| console.log(` X25519 shared secret (first 8 bytes): ${Buffer.from(clientX25519Shared.slice(0, 8)).toString('hex')}`); | ||
|
|
||
| // encapsulating to server's Kyber public key | ||
| console.log('\n📦 Step 3: Kyber encapsulation...'); | ||
| const serverKyberPublic = new Uint8Array(helloResponse.kyber_pubkey); | ||
| console.log(` Server Kyber public key length: ${serverKyberPublic.length} bytes`); | ||
| console.log(` Server Kyber public key (first 16 bytes): ${Buffer.from(serverKyberPublic.slice(0, 16)).toString('hex')}`); | ||
|
|
||
| const { sharedSecret: clientKyberShared, ciphertext } = kyberEncapsulate(serverKyberPublic); | ||
|
|
||
| expect(ciphertext.length).toBe(1088); // ML-KEM-768 ciphertext | ||
| expect(clientKyberShared.length).toBe(32); | ||
|
|
||
| console.log(` Kyber ciphertext: ${ciphertext.length} bytes`); | ||
| console.log(` Kyber ciphertext (first 16 bytes): ${Buffer.from(ciphertext.slice(0, 16)).toString('hex')}`); | ||
| console.log(` Kyber shared secret (first 8 bytes): ${Buffer.from(clientKyberShared.slice(0, 8)).toString('hex')}`); | ||
|
|
||
| // sending key_exchange with Kyber ciphertext | ||
| console.log('\n📤 Step 4: Sending key_exchange...'); | ||
| const keyExchangeResponse = await sendRpcRequest( | ||
| ws, | ||
| 'openpassport_key_exchange', | ||
| { | ||
| uuid, | ||
| kyber_ciphertext: Array.from(ciphertext), | ||
| }, | ||
| 2 | ||
| ); | ||
|
|
||
| expect(keyExchangeResponse).toBe('key_exchange_complete'); | ||
| console.log('✅ Server completed key exchange'); | ||
|
|
||
| // deriving client-side session key | ||
| console.log('\n🔐 Step 5: Deriving session key...'); | ||
| const clientSessionKey = deriveSessionKey(clientX25519Shared, clientKyberShared); | ||
|
|
||
| console.log(` Client session key (first 8 bytes): ${clientSessionKey.subarray(0, 8).toString('hex')}`); | ||
|
|
||
| // getting server's session key for verification (DEBUG ONLY) | ||
| console.log('\n🔍 Step 6: Verifying keys match (DEBUG)...'); | ||
| const serverSessionKey = await sendRpcRequest( | ||
| ws, | ||
| 'openpassport_debug_get_session_key', | ||
| { uuid }, | ||
| 10 | ||
| ); | ||
|
|
||
| const serverKeyBuffer = Buffer.from(serverSessionKey); | ||
| console.log(` Server session key (first 8 bytes): ${serverKeyBuffer.subarray(0, 8).toString('hex')}`); | ||
|
|
||
| // verifying session keys match | ||
| expect(serverKeyBuffer.length).toBe(32); | ||
| expect(clientSessionKey).toEqual(serverKeyBuffer); |
There was a problem hiding this comment.
Strip secret material from logs
These console.log statements print X25519 shared secrets, Kyber shared secrets, and the derived session key (even partially). That is sensitive key material, and per our JS/TS logging policy we must never emit secrets—even in tests—because logs can leak into CI artifacts or developer machines. Please delete or redact these logs before landing.
- console.log(` X25519 shared secret (first 8 bytes): ${Buffer.from(clientX25519Shared.slice(0, 8)).toString('hex')}`);
…
- console.log(` Kyber shared secret (first 8 bytes): ${Buffer.from(clientKyberShared.slice(0, 8)).toString('hex')}`);
…
- console.log(` Client session key (first 8 bytes): ${clientSessionKey.subarray(0, 8).toString('hex')}`);
…
- console.log(` Server session key (first 8 bytes): ${serverKeyBuffer.subarray(0, 8).toString('hex')}`);As per coding guidelines
📝 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.
| console.log(` X25519 shared secret (first 8 bytes): ${Buffer.from(clientX25519Shared.slice(0, 8)).toString('hex')}`); | |
| // encapsulating to server's Kyber public key | |
| console.log('\n📦 Step 3: Kyber encapsulation...'); | |
| const serverKyberPublic = new Uint8Array(helloResponse.kyber_pubkey); | |
| console.log(` Server Kyber public key length: ${serverKyberPublic.length} bytes`); | |
| console.log(` Server Kyber public key (first 16 bytes): ${Buffer.from(serverKyberPublic.slice(0, 16)).toString('hex')}`); | |
| const { sharedSecret: clientKyberShared, ciphertext } = kyberEncapsulate(serverKyberPublic); | |
| expect(ciphertext.length).toBe(1088); // ML-KEM-768 ciphertext | |
| expect(clientKyberShared.length).toBe(32); | |
| console.log(` Kyber ciphertext: ${ciphertext.length} bytes`); | |
| console.log(` Kyber ciphertext (first 16 bytes): ${Buffer.from(ciphertext.slice(0, 16)).toString('hex')}`); | |
| console.log(` Kyber shared secret (first 8 bytes): ${Buffer.from(clientKyberShared.slice(0, 8)).toString('hex')}`); | |
| // sending key_exchange with Kyber ciphertext | |
| console.log('\n📤 Step 4: Sending key_exchange...'); | |
| const keyExchangeResponse = await sendRpcRequest( | |
| ws, | |
| 'openpassport_key_exchange', | |
| { | |
| uuid, | |
| kyber_ciphertext: Array.from(ciphertext), | |
| }, | |
| 2 | |
| ); | |
| expect(keyExchangeResponse).toBe('key_exchange_complete'); | |
| console.log('✅ Server completed key exchange'); | |
| // deriving client-side session key | |
| console.log('\n🔐 Step 5: Deriving session key...'); | |
| const clientSessionKey = deriveSessionKey(clientX25519Shared, clientKyberShared); | |
| console.log(` Client session key (first 8 bytes): ${clientSessionKey.subarray(0, 8).toString('hex')}`); | |
| // getting server's session key for verification (DEBUG ONLY) | |
| console.log('\n🔍 Step 6: Verifying keys match (DEBUG)...'); | |
| const serverSessionKey = await sendRpcRequest( | |
| ws, | |
| 'openpassport_debug_get_session_key', | |
| { uuid }, | |
| 10 | |
| ); | |
| const serverKeyBuffer = Buffer.from(serverSessionKey); | |
| console.log(` Server session key (first 8 bytes): ${serverKeyBuffer.subarray(0, 8).toString('hex')}`); | |
| // verifying session keys match | |
| expect(serverKeyBuffer.length).toBe(32); | |
| expect(clientSessionKey).toEqual(serverKeyBuffer); | |
| // encapsulating to server's Kyber public key | |
| console.log('\n📦 Step 3: Kyber encapsulation...'); | |
| const serverKyberPublic = new Uint8Array(helloResponse.kyber_pubkey); | |
| console.log(` Server Kyber public key length: ${serverKyberPublic.length} bytes`); | |
| console.log(` Server Kyber public key (first 16 bytes): ${Buffer.from(serverKyberPublic.slice(0, 16)).toString('hex')}`); | |
| const { sharedSecret: clientKyberShared, ciphertext } = kyberEncapsulate(serverKyberPublic); | |
| expect(ciphertext.length).toBe(1088); // ML-KEM-768 ciphertext | |
| expect(clientKyberShared.length).toBe(32); | |
| console.log(` Kyber ciphertext: ${ciphertext.length} bytes`); | |
| console.log(` Kyber ciphertext (first 16 bytes): ${Buffer.from(ciphertext.slice(0, 16)).toString('hex')}`); | |
| // sending key_exchange with Kyber ciphertext | |
| console.log('\n📤 Step 4: Sending key_exchange...'); | |
| const keyExchangeResponse = await sendRpcRequest( | |
| ws, | |
| 'openpassport_key_exchange', | |
| { | |
| uuid, | |
| kyber_ciphertext: Array.from(ciphertext), | |
| }, | |
| 2 | |
| ); | |
| expect(keyExchangeResponse).toBe('key_exchange_complete'); | |
| console.log('✅ Server completed key exchange'); | |
| // deriving client-side session key | |
| console.log('\n🔐 Step 5: Deriving session key...'); | |
| const clientSessionKey = deriveSessionKey(clientX25519Shared, clientKyberShared); | |
| // getting server's session key for verification (DEBUG ONLY) | |
| console.log('\n🔍 Step 6: Verifying keys match (DEBUG)...'); | |
| const serverSessionKey = await sendRpcRequest( | |
| ws, | |
| 'openpassport_debug_get_session_key', | |
| { uuid }, | |
| 10 | |
| ); | |
| const serverKeyBuffer = Buffer.from(serverSessionKey); | |
| // verifying session keys match | |
| expect(serverKeyBuffer.length).toBe(32); | |
| expect(clientSessionKey).toEqual(serverKeyBuffer); |
🤖 Prompt for AI Agents
In common/tests/pqxdh-cross-language.test.ts around lines 203 to 255, the test
currently logs X25519 shared secrets, Kyber shared secrets, and the derived
session key (even partial bytes). Remove or redact any console.log that prints
secret material; instead log only non-sensitive metadata such as byte lengths or
a deterministic constant/placeholder, or log a hash/hmac of the secret if you
need verifiability (ensure the hash key is not a secret). Ensure no
Buffer.toString of shared secrets or session keys remains in the file.
1 participant
Currently, the key exchange between the mobile and TEE uses P-256 ECDH, which is vulnerable to attacks from quantum computers ("harvest now, decrypt later"). This PR adds support for PQXDH (Post-Quantum Extended Diffie-Hellman), a hybrid protocol that combines classical X25519 with post-quantum ML-KEM-768 (Kyber) to provide quantum-resistant key exchange.
The implementation follows Signal's PQXDH specification and maintains backward compatibility with the existing P-256 flow. When a client connects, it advertises support for both
Self-PQXDH-1andlegacy-p256suites, and the TEE picks the one it supports.This PR only implements the client-side components and thus partially handles #1315. The TEE-side implementation is not included yet.
How It Works
["Self-PQXDH-1", "legacy-p256"]Self-PQXDH-1selected:legacy-p256selected, existing P-256 flow is usedChanges
New Files
common/src/utils/proving/pqxdh-crypto.tsgenerateX25519Keypair()- generates X25519 keypairs for ECDHkyberEncapsulate()- performs ML-KEM-768 encapsulationcomputeX25519SharedSecret()- computes X25519 shared secretderiveSessionKey()- derives session key from both X25519 and Kyber shared secrets using HKDFgetSupportedSuites()- returns supported cipher suites in preference ordercommon/tests/pqxdh-crypto.test.tscommon/tests/pqxdh-cross-language.test.tsModified Files
common/src/utils/proving/types.tsHelloResponsetype with optionalselected_suite,x25519_pubkey,kyber_pubkeyfieldsSelf-PQXDH-1packages/mobile-sdk-alpha/src/proving/provingMachine.tsselectedSuite,kyberCiphertext,sharedKeyto proving store state_handleWsOpen()to advertise supported suites in hello message_handleWebSocketMessage()to:_closeConnections()andinit()to clean up PQXDH stateTesting
Unit Tests
yarn workspace @selfxyz/common test pqxdh-cryptoIntegration Tests
yarn workspace @selfxyz/common test pqxdh-cross-languageSummary by CodeRabbit
New Features
Tests
Chores