feat(deployment): check AMD CRL revocation in attestation validation#52
Merged
Conversation
Attestation validation proved AMD SEV-SNP hardware genuine and nonce-bound but never checked AMD's CRL, so a genuine-but-revoked signing key still read as valid. Add a node:crypto-only CRL parser (manual ASN.1 walk; node has no CRL type) that verifies the ARK-signed CRL (RSA-PSS or ECDSA), enforces nextUpdate freshness, and reports whether the ASK or ARK serial is revoked. The SEV-SNP verifier now fetches the per-product CRL from KDS and resolves: revoked to invalid, fresh and clear to valid, and unretrievable/unverifiable/stale to unverifiable (revocation unknown, never a silent pass). It populates the reserved notRevoked verdict check and drops the "Revocation not checked" detail. Refs CON-596
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Warning
|
| Layer / File(s) | Summary |
|---|---|
CRL parser: types, ASN.1 helpers, and core functions ...amd-snp/amd-crl.parser.ts |
Defines the parsed CRL shape and parsing error, implements DER and time helpers, parses CRL contents, verifies signatures, checks revocation, and evaluates expiration. |
KDS client: CRL fetching and caching ...amd-snp/amd-kds.client.ts |
Adds CRL TTL state, per-product CRL caching, and a CRL fetch path that handles cache hits, 404 responses, and transport errors. |
AmdSnpService: revocation integration ...amd-snp/amd-snp.service.ts |
Imports CRL helpers, derives product names from ARK subjects, propagates product through chain resolution, and updates verification to return invalid, unverifiable, or valid based on CRL revocation state. |
CRL parser unit tests and fixtures ...amd-snp/amd-crl.parser.spec.ts, ...amd-snp/amd-crl.test-fixtures.ts |
Builds hermetic OpenSSL CRL fixtures and covers parsing, signature verification, revocation lookup, serial normalization, and expiration behavior. |
AmdSnpService revocation integration tests ...amd-snp/amd-snp.service.spec.ts |
Extends the service spec with CRL fixtures, revoked and unavailable CRL scenarios, updated valid-chain assertions, and product-matching certificate subjects. |
Estimated code review effort: 4 (Complex) | ~50 minutes
Sequence Diagram(s)
sequenceDiagram
participant AmdSnpService
participant AmdKdsClient
participant AMD CRL Parser
AmdSnpService->>AmdKdsClient: getCrl(product)
AmdKdsClient-->>AmdSnpService: CRL Buffer | null
AmdSnpService->>AMD CRL Parser: parseAmdCrl / verifyCrlSignature
AMD CRL Parser-->>AmdSnpService: revoked status / expiry result
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly summarizes the main change: adding AMD CRL revocation checks to attestation validation. |
| Description check | ✅ Passed | The description includes the required Why and What sections and thoroughly explains the change, rationale, and testing. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
feat/amd-crl-revocation-check
Comment @coderabbitai help to get the list of available commands.
![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
🧹 Nitpick comments (2)
apps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-crl.parser.spec.ts (1)
13-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the console-tests
setup()pattern here.This spec shares suite-global fixtures through
beforeAll, which is the one repo convention this file currently skips. A localsetup()helper would keep each case explicit and avoid hidden cross-test coupling as more CRL variants get added. As per coding guidelines,apps/deploy-web/**/*.spec.{ts,tsx}should follow the console-testssetup()pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-crl.parser.spec.ts` around lines 13 - 20, This spec is using suite-global fixture setup via beforeAll/afterAll, but it should follow the repo’s console-tests setup() pattern. Refactor amd-crl.parser.spec.ts to use a local setup() helper that creates fixtures for each case and returns the needed values, then have the individual tests call it explicitly rather than sharing hidden state through the top-level fixtures variable. Keep the cleanup tied to the helper or each test so amd-crl.parser.spec.ts stays explicit and consistent with the existing spec convention.Source: Coding guidelines
apps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-snp.service.spec.ts (1)
207-236: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a stale-CRL fixture path to this helper.
generateCrl()only emits fresh CRLs, so this spec never drives the service’s “expired CRL => unverifiable” branch end-to-end. The parser spec covers expiry in isolation, but the verdict mapping changed inAmdSnpService.verify()too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-snp.service.spec.ts` around lines 207 - 236, The generateCrl helper currently only creates fresh CRLs, so the AMD-SNP service spec does not exercise the expired-CRL verification path end-to-end. Update generateCrl() to support generating a stale/expired CRL fixture (for example by parameterizing the validity dates or adding a dedicated branch), then add a test case in the amd-snp.service.spec.ts flow that uses it to drive AmdSnpService.verify() into the “expired CRL => unverifiable” result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-crl.parser.ts`:
- Around line 76-93: Make missing or malformed nextUpdate fail closed instead of
being treated as valid: update parseAsn1Time so it only returns a Date when the
ASN.1 time is fully well-formed, and have isCrlExpired treat a null/invalid
nextUpdate as unverifiable (not current). Use the existing parseAsn1Time and
isCrlExpired logic to ensure an ARK-signed CRL without a usable freshness bound
is rejected rather than accepted.
In
`@apps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-snp.service.spec.ts`:
- Around line 144-153: The amd-snp.service.spec.ts setup mock for
AmdKdsClient.getCrl is too permissive because it ignores the requested product,
so the revocation tests do not verify the AmdSnpService product mapping. Update
the setup helper and the getCrl mock to behave product-sensitively like
getCaChain/getVcek, keyed off input.resolvingProduct (or equivalent), so only
the expected AMD product returns a CRL and wrong product derivations fail the
tests.
---
Nitpick comments:
In
`@apps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-crl.parser.spec.ts`:
- Around line 13-20: This spec is using suite-global fixture setup via
beforeAll/afterAll, but it should follow the repo’s console-tests setup()
pattern. Refactor amd-crl.parser.spec.ts to use a local setup() helper that
creates fixtures for each case and returns the needed values, then have the
individual tests call it explicitly rather than sharing hidden state through the
top-level fixtures variable. Keep the cleanup tied to the helper or each test so
amd-crl.parser.spec.ts stays explicit and consistent with the existing spec
convention.
In
`@apps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-snp.service.spec.ts`:
- Around line 207-236: The generateCrl helper currently only creates fresh CRLs,
so the AMD-SNP service spec does not exercise the expired-CRL verification path
end-to-end. Update generateCrl() to support generating a stale/expired CRL
fixture (for example by parameterizing the validity dates or adding a dedicated
branch), then add a test case in the amd-snp.service.spec.ts flow that uses it
to drive AmdSnpService.verify() into the “expired CRL => unverifiable” result.
🪄 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: 2b5ad7c7-9e9f-4af6-9787-94b354cd2b38
📒 Files selected for processing (5)
apps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-crl.parser.spec.tsapps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-crl.parser.tsapps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-kds.client.tsapps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-snp.service.spec.tsapps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-snp.service.ts
…duct wiring Address CodeRabbit review on the AMD CRL revocation path: - parseAsn1Time now rejects malformed times and isCrlExpired treats a missing or invalid nextUpdate as expired, so a CRL with no usable freshness bound is unverifiable rather than trusted as current. - The amd-snp.service.spec getCrl mock is product-sensitive, so the revocation tests actually exercise the product-derivation wiring. Refs CON-596
Collaborator
Author
CodeRabbit feedback addressed (commit
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-crl.parser.ts`:
- Around line 76-87: parseAsn1Time currently accepts structurally valid strings
but lets Date.UTC normalize out-of-range components, so malformed CRL times can
still parse. Update parseAsn1Time in amd-crl.parser.ts to round-trip validate
the extracted year/month/day/hour/minute/second after constructing the Date, and
throw AmdCrlParseError unless the normalized Date matches the original
components exactly. Keep the existing RFC 5280 Zulu-format checks and apply the
same fail-closed behavior for both TAG.UTC_TIME and non-UTC time tags.
🪄 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: 3d15bb1e-2866-4053-9214-981b1b91fc92
📒 Files selected for processing (3)
apps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-crl.parser.spec.tsapps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-crl.parser.tsapps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-snp.service.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-snp.service.spec.ts
Address CodeRabbit follow-up: parseAsn1Time validated digit structure but Date.UTC silently rolled over out-of-range parts (month 13, day 00), yielding a plausible wrong date that isCrlExpired would not catch. Round-trip the parsed components and throw on mismatch so a malformed nextUpdate fails closed. Refs CON-596
…tures Quality cleanup of the AMD SEV-SNP CRL revocation path; no behavior change beyond the verifier tightening below: - verifyCrlSignature now accepts only RSA-PSS/SHA-384, AMD's actual algorithm. The ECDSA branch existed solely so a test fixture could avoid RSA; the service spec now mints an RSA ARK/ASK (P-384 VCEK) so its CRL is genuinely RSA-PSS-signed. - Build the verdict checks object per branch instead of annotating then mutating it. - Drop the unreachable non-finite nextUpdate guard in isCrlExpired (parseAsn1Time only ever yields null or a valid Date). - Extract the duplicated openssl CRL-generation scaffold from both specs into a shared amd-crl.test-fixtures helper. Refs CON-596
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-crl.parser.spec.ts (1)
121-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePositional DER byte-patching is brittle but acceptable for test fixtures.
Locating the "second UTCTime occurrence" and throwing when not found is a reasonable safety net; just be aware this depends on OpenSSL's CRL field ordering staying stable across environments/versions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-crl.parser.spec.ts` around lines 121 - 157, The CRL fixture helper in withNextUpdateMonth relies on positional DER byte-patching, so keep the second-UTCTime lookup and the explicit throw as the safety net, but add a brief note near generateCrlFixtures/withNextUpdateMonth that this test-only approach depends on OpenSSL’s CRL field ordering remaining stable across versions and environments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@apps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-crl.parser.spec.ts`:
- Around line 121-157: The CRL fixture helper in withNextUpdateMonth relies on
positional DER byte-patching, so keep the second-UTCTime lookup and the explicit
throw as the safety net, but add a brief note near
generateCrlFixtures/withNextUpdateMonth that this test-only approach depends on
OpenSSL’s CRL field ordering remaining stable across versions and environments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 786b6e87-1325-46db-b0d4-b2758b5dae92
📒 Files selected for processing (5)
apps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-crl.parser.spec.tsapps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-crl.parser.tsapps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-crl.test-fixtures.tsapps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-snp.service.spec.tsapps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-snp.service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-snp.service.ts
Cover getVcek/getCaChain/getCrl 200, 404-to-null, and transport-error paths plus the per-instance chain/CRL cache, closing the KDS client coverage gap.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-kds.client.spec.ts (1)
71-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering the cached-
null(404) path too.Both caching tests only verify that a successful response is cached; a repeat lookup after a 404 (cached
null) isn't asserted to skip the fetch as well, even though#cacheCaChain/#cacheCrlcachenullthe same way.♻️ Suggested addition
it("caches the CRL so a repeat lookup skips the HTTP fetch", async () => { const { client, httpClient } = setup(); httpClient.get.mockResolvedValue({ data: new Uint8Array([1]).buffer }); await client.getCrl("Turin"); await client.getCrl("Turin"); expect(httpClient.get).toHaveBeenCalledTimes(1); }); + + it("caches a 404 (null) result so a repeat lookup skips the HTTP fetch", async () => { + const { client, httpClient } = setup(); + httpClient.get.mockRejectedValue(httpError(404)); + + await client.getCrl("Turin"); + await client.getCrl("Turin"); + + expect(httpClient.get).toHaveBeenCalledTimes(1); + });Also applies to: 108-116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-kds.client.spec.ts` around lines 71 - 79, Add a test in amd-kds.client.spec.ts that verifies a cached null result from a 404 also short-circuits repeated lookups, not just the success path. Update the existing cache coverage around getCaChain (and the analogous getCrl test mentioned in the comment) to mock a 404 or null-returning fetch, call the method twice, and assert the underlying httpClient.get is only invoked once. Use the existing client setup and method names like getCaChain, getCrl, `#cacheCaChain`, and `#cacheCrl` to keep the new assertion aligned with the current caching behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@apps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-kds.client.spec.ts`:
- Around line 71-79: Add a test in amd-kds.client.spec.ts that verifies a cached
null result from a 404 also short-circuits repeated lookups, not just the
success path. Update the existing cache coverage around getCaChain (and the
analogous getCrl test mentioned in the comment) to mock a 404 or null-returning
fetch, call the method twice, and assert the underlying httpClient.get is only
invoked once. Use the existing client setup and method names like getCaChain,
getCrl, `#cacheCaChain`, and `#cacheCrl` to keep the new assertion aligned with the
current caching behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37428630-294b-44a8-97be-692e80787b14
📒 Files selected for processing (1)
apps/deploy-web/src/lib/nextjs/confidential-compute/services/amd-snp/amd-kds.client.spec.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Console Air's Confidential Compute attestation validation (CON-552) proves an AMD SEV-SNP report came from genuine, nonce-bound AMD hardware, but it never checked AMD's certificate revocation list (CRL) —
node:cryptohas no CRL support. So a chip whose signing key AMD has revoked (e.g. a known-compromised part) still read asvalid, and the verdict only said "Revocation not checked". This closes that documented blind spot (CON-596).What
amd-crl.parser.ts— anode:crypto-only manual ASN.1 parser for AMD KDS CRLs (no new dependency, matching the module's existing design). Extracts revoked serials +nextUpdateand verifies the ARK-signed CRL signature. AMD signs these RSA-PSS/SHA-384 (the ARK is RSA-4096); an ECDSA issuer is also accepted so test fixtures need not be RSA.amd-kds.client.ts—getCrl(product)fetching/vcek/v1/{product}/crlwith a 6h cache (mirrors the existing VCEK/CA-chain fetch + 404→null, transport-error→throw contract).amd-snp.service.ts— once authenticity passes, checks the per-product CRL. Verdict resolves to:invalid(notRevoked: false)valid(notRevoked: true)unverifiable(revocation unknown — never a silent pass)notRevokedverdict check and drops the "Revocation not checked" detail.The contract was verified against live AMD KDS: the CRL is ARK-signed RSA-PSS and revokes ASK-level serials (Genoa's CRL revokes an old ASK
020001, since rotated to020002— exactly the case this closes).Acceptance criteria (CON-596)
validnotRevokedpopulated, not omitted)unverifiable(unknown), never a silentvalidTesting
amd-crl.parser.spec.ts— RSA-PSS CRL fixtures generated via openssl; covers parse, signature verify (+ tamper / wrong-issuer rejection), revoked-serial detection, serial normalisation, and expiry.amd-snp.service.spec.tsextended with revoked / unretrievable / errored-CRL scenarios; the existing valid-path test now assertsnotRevoked: trueand no "Revocation not checked".npm run test:unit— 62/62 confidential-compute tests pass; ESLint clean; type-check clean for changed files.Note / follow-up
AMD_SNP_PRODUCTSdefaults toMilan,Genoa,Turin. If "all currently supported AMD products" should include Bergamo/Siena, that's a smallenv.config.tschange, independent of this revocation logic.Refs CON-596
Summary by CodeRabbit
New Features
nextUpdateas expired.Bug Fixes
Tests