Uh oh!
There was an error while loading. Please reload this page.
feat: key binding certs instead of direct public keys - #568
Conversation
WalkthroughThis PR introduces JWT-based key binding certificates to secure public key exchange between subsystems. It converts the single Changes
Sequence DiagramsequenceDiagram
actor User
participant Client as Client App
participant EV as eVault Core
participant Registry
participant SigValidator as Signature Validator
rect rgb(200, 220, 255)
note over User,Registry: Key Binding Certificate Generation & Whois
Client->>EV: GET /whois?ename=user@example.com
EV->>EV: getPublicKeys(eName) → string[]
alt publicKeys exist and registry configured
EV->>Registry: GET /jwks
Registry-->>EV: JWK set (public keys)
EV->>Registry: POST /key-binding-certificate<br/>(ename, publicKey) for each key
Registry->>Registry: generateKeyBindingCertificate<br/>(ES256 JWT sign)
Registry-->>EV: token (JWT)
EV->>EV: Collect keyBindingCertificates[]
end
EV-->>Client: { w3id, keyBindingCertificates[] }
end
rect rgb(220, 240, 220)
note over Client,SigValidator: Signature Verification with Key Binding
Client->>SigValidator: verify(signature, multikey, eName)
SigValidator->>EV: GET /whois?ename=eName
EV-->>SigValidator: { keyBindingCertificates[] }
SigValidator->>Registry: GET /jwks
Registry-->>SigValidator: JWK set
SigValidator->>SigValidator: Loop keyBindingCertificates
loop For each certificate JWT
SigValidator->>SigValidator: Verify JWT signature (JWKS)
SigValidator->>SigValidator: Check ename matches
SigValidator->>SigValidator: Extract publicKey from JWT
SigValidator->>SigValidator: Verify multikey signature<br/>with publicKey
alt Match found
SigValidator-->>Client: ✓ Valid (publicKey, eName)
end
end
alt No match
SigValidator-->>Client: ✗ Error (aggregated failures)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 2 inconclusive)
✅ Passed checks (1 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 |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infrastructure/evault-core/src/core/http/server.ts (1)
70-76: Critical: API schema doesn't match actual response structure.The Swagger schema at lines 70-76 declares the response has a
publicKeyfield of type string (nullable), but the actual response at line 155 returnskeyBindingCertificates(an array of strings) instead. This breaks the API contract, will generate incorrect documentation, and will cause client validation failures.Apply this diff to update the schema to match the actual response:
response: { type: "object", properties: { w3id: { type: "string" }, - publicKey: { type: "string", nullable: true },+ keyBindingCertificates: { + type: "array",+ items: { type: "string" }+ }, }, },
♻️ Duplicate comments (1)
infrastructure/eid-wallet/package.json (1)
45-45: Verify the jose library version for security advisories.Ensure that version ^5.2.0 of the jose library is current, secure, and free from known vulnerabilities.
This is the same concern as flagged for infrastructure/signature-validator/package.json. The verification script from that review applies here as well.
🧹 Nitpick comments (6)
infrastructure/eid-wallet/src/lib/global/controllers/evault.ts (5)
135-147: DuplicategetPublicKeycall can be consolidated.The same
getPublicKey(KEY_ID, context)call is made here and again at lines 224-228. If the first call fails but we "continue to sync anyway," the second call will likely fail too.Consider reusing
currentPublicKey(if successfully retrieved) to avoid the redundant call:- // Get public key using the same method as getApplicationPublicKey() in onboarding/verify- let publicKey: string | undefined;- try {- publicKey = await this.#keyService.getPublicKey(- KEY_ID,- context,- );+ // Reuse currentPublicKey if already retrieved, otherwise fetch it+ const publicKey = currentPublicKey ?? await (async () => {+ try {+ return await this.#keyService.getPublicKey(KEY_ID, context);+ } catch (error) {+ console.error(+ `Failed to get public key for ${KEY_ID} with context ${context}:`,+ error,+ );+ return undefined;+ }+ })();Alternatively, simply reuse
currentPublicKeydirectly since it's the same key:- let publicKey: string | undefined;- try {- publicKey = await this.#keyService.getPublicKey(- KEY_ID,- context,- );- console.log(- `Public key retrieved: ${publicKey?.substring(0, 60)}...`,- );- console.log(`Public key (full): ${publicKey}`);- } catch (error) {- console.error(- `Failed to get public key for ${KEY_ID} with context ${context}:`,- error,- );- return;- }+ const publicKey = currentPublicKey;+ if (publicKey) {+ console.log(+ `Public key retrieved: ${publicKey.substring(0, 60)}...`,+ );+ console.log(`Public key (full): ${publicKey}`);+ }
158-167: Validate JWKS response structure before creating JWKSet.If
jwksResponse.datadoesn't have the expected{ keys: [...] }structure,createLocalJWKSetmay throw or behave unexpectedly. Consider adding validation:const jwksResponse = await axios.get(jwksUrl, { timeout: 10000, }); + if (!jwksResponse.data?.keys || !Array.isArray(jwksResponse.data.keys)) {+ console.warn("Invalid JWKS response structure");+ throw new Error("Invalid JWKS response");+ } const JWKS = jose.createLocalJWKSet(jwksResponse.data);
182-195: Consider explicit validation of JWT payload fields.The
publicKeyis extracted via type assertion without validation. While the comparison will safely fail ifpublicKeyis undefined, explicit validation makes the intent clearer and helps catch malformed certificates:// Extract publicKey from JWT payload - const extractedPublicKey =- payload.publicKey as string;+ const extractedPublicKey = payload.publicKey;+ if (typeof extractedPublicKey !== "string") {+ console.warn("Certificate payload missing publicKey");+ continue;+ } if (extractedPublicKey === currentPublicKey) {
205-212: Clarify security implications of error fallback.When JWKS fetch or certificate verification fails, the code continues to sync the public key anyway. This is likely intentional for resilience (allowing sync even if registry is temporarily unavailable), but it's worth documenting:
} catch (error) { console.error( "Error checking existing public keys:", error, ); - // Continue to sync anyway+ // Continue to sync anyway - verification failures shouldn't block+ // new key registration. Server-side validation ensures security. }If strict verification is required (per PR objectives about certificate chain validation), consider whether this fallback should instead abort the sync when
keyBindingCertificatesexist but verification fails.
229-232: Consider removing verbose public key logging for production.Logging the full public key (line 232) is fine for debugging but adds noise in production. Consider using a debug flag or removing before release:
- console.log(- `Public key retrieved: ${publicKey?.substring(0, 60)}...`,- );- console.log(`Public key (full): ${publicKey}`);+ console.log(`Public key retrieved for sync: ${publicKey?.substring(0, 20)}...`);infrastructure/evault-core/src/core/http/server.ts (1)
111-151: Consider improving observability for certificate generation failures.The code generates key binding certificates for each public key, with per-key error handling that logs errors and continues. However, if the registry is unavailable or all certificate generations fail, the response will contain an empty
keyBindingCertificatesarray with no indication to the caller that something went wrong.Consider one of the following approaches:
- Include a
certificateGenerationErrorsfield in the response to indicate partial failures- Add metrics/telemetry to track certificate generation success/failure rates
- Return a warning in the response when certificates couldn't be generated but publicKeys exist
This would help diagnose issues where signature verification fails due to missing certificates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
infrastructure/control-panel/src/routes/+layout.svelte(1 hunks)infrastructure/control-panel/src/routes/actions/+page.svelte(2 hunks)infrastructure/eid-wallet/package.json(1 hunks)infrastructure/eid-wallet/src/lib/global/controllers/evault.ts(2 hunks)infrastructure/evault-core/src/core/db/db.service.ts(2 hunks)infrastructure/evault-core/src/core/db/migrations/migrate-publickey-to-array.ts(1 hunks)infrastructure/evault-core/src/core/http/server.ts(2 hunks)infrastructure/evault-core/src/index.ts(1 hunks)infrastructure/signature-validator/package.json(1 hunks)infrastructure/signature-validator/src/index.ts(5 hunks)platforms/blabsy/package.json(1 hunks)platforms/registry/src/index.ts(2 hunks)platforms/registry/src/jwt.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
infrastructure/eid-wallet/src/lib/global/controllers/evault.ts (3)
infrastructure/eid-wallet/src/lib/global/controllers/user.ts (2)
isFake(88-98)isFake(100-113)infrastructure/eid-wallet/src/env.d.ts (1)
PUBLIC_REGISTRY_URL(6-6)platforms/pictique/src/env.d.ts (1)
PUBLIC_REGISTRY_URL(7-7)
infrastructure/evault-core/src/index.ts (1)
infrastructure/evault-core/src/core/db/migrations/migrate-publickey-to-array.ts (1)
migratePublicKeyToArray(13-47)
platforms/registry/src/jwt.ts (1)
infrastructure/evault-core/generate-test-token.js (2)
token(39-50)privateKey(36-36)
platforms/registry/src/index.ts (1)
platforms/registry/src/jwt.ts (1)
generateKeyBindingCertificate(70-81)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: test
🔇 Additional comments (22)
infrastructure/control-panel/src/routes/+layout.svelte (2)
18-18: LGTM!The single-line expression is clean and preserves the original logic correctly.
22-22: LGTM!Class token reordering has no functional impact.
infrastructure/control-panel/src/routes/actions/+page.svelte (3)
70-75: LGTM!Class reordering and text formatting changes are cosmetic with no functional impact.
91-94: LGTM!Class reordering and text wrapping are cosmetic adjustments.
101-101: LGTM!Position class reordering has no impact on the rendered output.
platforms/blabsy/package.json (2)
64-64: Trailing newline fix.The trailing newline addition is a non-functional formatting adjustment.
1-64: This review comment is incorrect.The
blabsypackage is a Next.js frontend application that does not implement JWT-based key binding certificates or ePassports. JWT/jose functionality is implemented ininfrastructure/evault-coreandinfrastructure/signature-validator, both of which already declare thejosedependency (^5.2.2and^5.2.0respectively). No changes toplatforms/blabsy/package.jsonare required for this feature.Likely an incorrect or invalid review comment.
infrastructure/eid-wallet/src/lib/global/controllers/evault.ts (2)
9-9: LGTM!The
joselibrary is a solid choice for JWT/JWKS operations, providing secure and spec-compliant implementations.
126-128: LGTM!Clean extraction with optional chaining. The type is validated later with
Array.isArray()before use.platforms/registry/src/jwt.ts (1)
84-94: LGTM! Good security practice.The implementation correctly returns the JWK in JWKS format while omitting the private key component. This is a secure approach for exposing the public verification key.
platforms/registry/src/index.ts (1)
6-6: LGTM!The import of
generateKeyBindingCertificateis correctly added alongside existing JWT functions.infrastructure/evault-core/src/index.ts (1)
104-110: Migration failure handling is appropriate but requires monitoring.The migration error is caught and logged as a warning without failing startup. This is a pragmatic approach for backward compatibility, but be aware that a failed migration could lead to inconsistent behavior if the database still has the old
publicKeyfield structure while the new code expectspublicKeysarrays.Consider monitoring migration execution in production to ensure all instances complete successfully. You may want to add telemetry or alerting for migration failures.
infrastructure/evault-core/src/core/db/migrations/migrate-publickey-to-array.ts (1)
13-47: LGTM! Migration is idempotent and handles errors correctly.The migration properly:
- Checks for
publicKeys IS NULLto ensure idempotency- Handles both existing publicKey values and NULL cases
- Closes the session in a finally block
- Logs progress and errors appropriately
infrastructure/evault-core/src/core/db/db.service.ts (2)
683-704: LGTM! Defensive handling of publicKeys array.The method correctly:
- Returns an empty array when the User node doesn't exist
- Handles null/undefined and non-array values gracefully
- Maintains consistent return type of
string[]
816-842: LGTM! User node copy correctly handles publicKeys array.The copy logic properly:
- Checks that publicKeys is an array with length > 0
- Copies the entire array to the target
- Includes appropriate logging
infrastructure/signature-validator/src/index.ts (3)
111-144: LGTM! Certificate retrieval flow is well-structured.The function correctly:
- Resolves the eVault URL from the registry
- Fetches key binding certificates from the /whois endpoint
- Returns an empty array when certificates are missing (handled appropriately by caller)
212-227: LGTM! Efficient signature verification setup.The code correctly:
- Fetches the JWKS from the registry for JWT verification
- Creates a local JWK set for validation
- Decodes the signature once to avoid redundant work in the loop
228-294: LGTM! Certificate verification loop is thorough and secure.The verification loop correctly:
- Verifies each JWT signature against the JWKS
- Validates that the ename in the JWT matches the expected ename
- Extracts and decodes the public key from the verified JWT payload
- Imports the key and verifies the signature
- Returns immediately on first successful verification
- Aggregates errors for debugging when all certificates fail
This implements the security requirements from issue #465 by verifying the certificate chain before trusting the public key.
infrastructure/evault-core/src/core/http/server.ts (3)
97-109: LGTM! Robust error handling for public key retrieval.The code correctly:
- Retrieves all public keys using the new
getPublicKeysmethod- Handles errors gracefully by continuing with an empty array
- Maintains consistent array type
153-159: LGTM! Response structure correctly returns key binding certificates.The response properly returns the
keyBindingCertificatesarray instead of a single public key, aligning with the PR's objective to use certificate-based key exchange.
492-506: LGTM! Correctly uses addPublicKey for append-only behavior.The PATCH endpoint properly:
- Uses
addPublicKeyto append rather than overwrite- Updates the success message to reflect "added" instead of "saved"
- Maintains appropriate error messaging
infrastructure/signature-validator/package.json (1)
17-17: jose@^5.2.0 is secure with no known vulnerabilities.Version 5.2.0 is free from known CVEs. Recent advisories (CVE-2021-29443, CVE-2024-28176) do not affect v5.x, and CVE-2023-25653 targets a different package (node-jose). The version constraint allows updates within the v5 range; latest stable v6.1.2 is available if you choose to upgrade, but no security action is required.
Uh oh!
There was an error while loading. Please reload this page.
Description of change
Issue Number
closes#465
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
Release Notes
New Features
Database Updates
Style
✏️ Tip: You can customize this high-level summary in your review settings.