feat(deployment): check AMD CRL revocation in attestation validation#3381
Merged
Conversation
AMD SEV-SNP attestation proved a report came from genuine, nonce-bound AMD hardware but never checked AMD's certificate revocation list (CRL), so a chip whose ASK/ARK signing key AMD has revoked still read as `valid`, with the verdict only noting "Revocation not checked". Add a `node:crypto`-only manual ASN.1 CRL parser (no new dependency), an `AmdKdsClient.getCrl` fetch cached via @memoize (6h; verifier also enforces the CRL's own nextUpdate), and wire the check into the verdict: revoked ASK/ARK -> `invalid` (notRevoked: false); fresh ARK-signed CRL omitting them -> `valid` (notRevoked: true); CRL unretrievable / unverifiable / stale -> `unverifiable` (never a silent pass). Fixes CON-596
Contributor
📝 WalkthroughWalkthroughAdds AMD CRL parsing and verification, fetches product CRLs through ChangesAMD CRL revocation support
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3381 +/- ##
==========================================
- Coverage 69.80% 68.68% -1.13%
==========================================
Files 1100 1012 -88
Lines 26926 24716 -2210
Branches 6457 6030 -427
==========================================
- Hits 18797 16977 -1820
+ Misses 7135 6780 -355
+ Partials 994 959 -35
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/api/src/confidential-compute/services/amd-snp/amd-crl.parser.spec.ts (1)
6-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap fixture lifecycle in a
setup()helper.This spec uses file-level mutable
fixtureswithbeforeAll/afterAll. Please move fixture creation/cleanup behind asetup()helper to match the repo’s unit-test pattern.Proposed direction
-let fixtures: ReturnType<typeof generateCrlFixtures>; +let fixtures: ReturnType<typeof generateCrlFixtures>; +let cleanupFixtures: (() => void) | undefined; + +function setup() { + const fixtures = generateCrlFixtures(); + return { + fixtures, + cleanup: () => rmSync(fixtures.dir, { recursive: true, force: true }) + }; +} beforeAll(() => { - fixtures = generateCrlFixtures(); + const context = setup(); + fixtures = context.fixtures; + cleanupFixtures = context.cleanup; }); afterAll(() => { - if (fixtures) rmSync(fixtures.dir, { recursive: true, force: true }); + cleanupFixtures?.(); });As per coding guidelines,
**/*.spec.ts: “Usesetupfunction instead ofbeforeEachin unit and service level tests.” As per path instructions,**/*.spec.ts: “Check that tests use a setup() function.”🤖 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/api/src/confidential-compute/services/amd-snp/amd-crl.parser.spec.ts` around lines 6 - 21, Wrap the AMD CRL fixture setup/teardown in a local setup() helper instead of using file-level mutable state with beforeAll/afterAll. Update amd-crl.parser.spec.ts so generateCrlFixtures and rmSync are owned by setup(), and have the tests consume the returned fixtures through that helper to match the repo’s spec pattern. Keep the existing test symbols like generateCrlFixtures, fixtures, and the cleanup logic, but move their lifecycle management behind setup().Sources: Coding guidelines, Path instructions
apps/api/src/confidential-compute/services/amd-snp/amd-snp.service.spec.ts (1)
176-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConstrain the CRL mock to the resolved product.
getCrlcurrently returns a CRL for any product, so these tests would not catch a regression that fetches the wrong product’s CRL.Suggested test hardening
- kdsClient.getCrl.mockImplementation(async () => { + kdsClient.getCrl.mockImplementation(async product => { + if (product !== resolving) return null; if (input.crl === "missing") return null; if (input.crl === "error") throw new Error("AMD KDS unavailable"); return input.crl === "revoked" ? revokedCrlDer : cleanCrlDer; });As per path instructions, verify meaningful assertions in specs.
🤖 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/api/src/confidential-compute/services/amd-snp/amd-snp.service.spec.ts` around lines 176 - 180, The CRL mock in amd-snp.service.spec.ts is too permissive because getCrl returns a CRL regardless of the requested product, so it won’t catch regressions that fetch the wrong one. Tighten the mock tied to kdsClient.getCrl by asserting the requested product matches the resolved product used by the service, and only returning the CRL for that product while rejecting or returning null for others; keep the existing missing/error/revoked paths but bind them to the expected product so the spec verifies the correct lookup behavior.Source: Path instructions
🤖 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/api/src/confidential-compute/services/amd-snp/amd-crl.parser.ts`:
- Around line 171-173: The `isCrlExpired` helper currently treats a missing
`nextUpdate` as non-expired, which lets stale CRLs remain valid indefinitely.
Update `isCrlExpired(crl, now)` in `amd-crl.parser.ts` so a `null`
`crl.nextUpdate` is considered expired/stale instead of fresh, while still
comparing timestamps normally when `nextUpdate` is present. Keep the behavior
aligned with the “fresh CRL” contract by making the function return true when
`nextUpdate` is absent.
In `@apps/api/src/confidential-compute/services/amd-snp/amd-snp.service.spec.ts`:
- Around line 235-261: The generateCrl helper leaves temporary CRL directories
behind when execFileSync or readFileSync throws. Update generateCrl in
amd-snp.service.spec.ts to wrap the OpenSSL/file-read flow in a try/finally and
always call rmSync on the mkdtempSync-created dir in the finally block, keeping
the cleanup tied to generateCrl and its ossl helper.
---
Nitpick comments:
In `@apps/api/src/confidential-compute/services/amd-snp/amd-crl.parser.spec.ts`:
- Around line 6-21: Wrap the AMD CRL fixture setup/teardown in a local setup()
helper instead of using file-level mutable state with beforeAll/afterAll. Update
amd-crl.parser.spec.ts so generateCrlFixtures and rmSync are owned by setup(),
and have the tests consume the returned fixtures through that helper to match
the repo’s spec pattern. Keep the existing test symbols like
generateCrlFixtures, fixtures, and the cleanup logic, but move their lifecycle
management behind setup().
In `@apps/api/src/confidential-compute/services/amd-snp/amd-snp.service.spec.ts`:
- Around line 176-180: The CRL mock in amd-snp.service.spec.ts is too permissive
because getCrl returns a CRL regardless of the requested product, so it won’t
catch regressions that fetch the wrong one. Tighten the mock tied to
kdsClient.getCrl by asserting the requested product matches the resolved product
used by the service, and only returning the CRL for that product while rejecting
or returning null for others; keep the existing missing/error/revoked paths but
bind them to the expected product so the spec verifies the correct lookup
behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d9a1f804-8b34-4dfc-89e8-5752c1c32813
📒 Files selected for processing (5)
apps/api/src/confidential-compute/services/amd-snp/amd-crl.parser.spec.tsapps/api/src/confidential-compute/services/amd-snp/amd-crl.parser.tsapps/api/src/confidential-compute/services/amd-snp/amd-kds.client.tsapps/api/src/confidential-compute/services/amd-snp/amd-snp.service.spec.tsapps/api/src/confidential-compute/services/amd-snp/amd-snp.service.ts
Fail closed when a CRL has no nextUpdate (treat as stale), reject out-of-range DER time components instead of silently normalizing, and accept only RSA-PSS CRL signatures as AMD KDS issues. Rework the service test chain to a realistic RSA ARK/ASK + P-384 VCEK so RSA-only verification holds, and add AmdKdsClient tests covering the getVcek/getCaChain/getCrl 404 and transport-error paths. Resolves CodeRabbit review on CON-596.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/api/src/confidential-compute/services/amd-snp/amd-crl.parser.spec.ts (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a
setup()helper instead of suite-level fixture state.This spec imports
beforeAll/afterAlland relies on shared fixtures; repo test guidance asks unit specs to use asetup()function and avoid shared mutable setup state. As per coding guidelines,**/*.spec.ts: “Usesetupfunction instead ofbeforeEachin unit and service level tests”. As per path instructions,**/*.spec.ts: “Check that tests use a setup() function (not beforeEach with shared mutable state).”🤖 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/api/src/confidential-compute/services/amd-snp/amd-crl.parser.spec.ts` at line 6, The spec is using suite-level fixture setup via beforeAll/afterAll instead of the required local setup pattern. Refactor amd-crl.parser.spec.ts to define a setup() helper that creates and returns the test fixtures used by the describe/it blocks, and have each test call setup() rather than relying on shared mutable state. Remove the unused beforeAll/afterAll imports from the spec and keep the test state isolated through the setup() function.Sources: Coding guidelines, Path instructions
apps/api/src/confidential-compute/services/amd-snp/amd-kds.client.spec.ts (1)
14-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove cache-clearing into
setup()rather thanbeforeEach/afterEach.The suite already has a
setup()helper (line 120); the memoization-cache reset could live there instead ofbeforeEach/afterEach, keeping test setup centralized per convention.♻️ Suggested change
- // getCaChain and getCrl are `@Memoize`'d against a module-level cache; clear it so results don't bleed between tests. - beforeEach(() => cacheEngine.clearAllKeyInCache()); - afterEach(() => cacheEngine.clearAllKeyInCache()); - describe("getVcek", () => { @@ function setup() { + // getCaChain and getCrl are `@Memoize`'d against a module-level cache; clear it so results don't bleed between tests. + cacheEngine.clearAllKeyInCache(); const httpClient = mock<HttpClient>(); const logger = mock<LoggerService>(); const client = new AmdKdsClient(httpClient, logger); return { client, httpClient, logger }; }As per coding guidelines, "Use
setupfunction instead ofbeforeEachin unit and service level tests" for files matching**/*.spec.ts.🤖 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/api/src/confidential-compute/services/amd-snp/amd-kds.client.spec.ts` around lines 14 - 16, The memoization cache reset for `getCaChain` and `getCrl` is currently wired through `beforeEach`/`afterEach`, but this spec should follow the project convention of using `setup()`. Move the `cacheEngine.clearAllKeyInCache()` call into the existing `setup()` helper in `amd-kds.client.spec.ts`, and remove the `beforeEach`/`afterEach` hooks so test initialization stays centralized and consistent.Source: Coding guidelines
🤖 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/api/src/confidential-compute/services/amd-snp/amd-crl.parser.spec.ts`:
- Line 6: The spec is using suite-level fixture setup via beforeAll/afterAll
instead of the required local setup pattern. Refactor amd-crl.parser.spec.ts to
define a setup() helper that creates and returns the test fixtures used by the
describe/it blocks, and have each test call setup() rather than relying on
shared mutable state. Remove the unused beforeAll/afterAll imports from the spec
and keep the test state isolated through the setup() function.
In `@apps/api/src/confidential-compute/services/amd-snp/amd-kds.client.spec.ts`:
- Around line 14-16: The memoization cache reset for `getCaChain` and `getCrl`
is currently wired through `beforeEach`/`afterEach`, but this spec should follow
the project convention of using `setup()`. Move the
`cacheEngine.clearAllKeyInCache()` call into the existing `setup()` helper in
`amd-kds.client.spec.ts`, and remove the `beforeEach`/`afterEach` hooks so test
initialization stays centralized and consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f4cb7bb9-739f-4744-82f3-c52acfef5dcd
📒 Files selected for processing (5)
apps/api/src/confidential-compute/services/amd-snp/amd-crl.parser.spec.tsapps/api/src/confidential-compute/services/amd-snp/amd-crl.parser.tsapps/api/src/confidential-compute/services/amd-snp/amd-crl.test-fixtures.tsapps/api/src/confidential-compute/services/amd-snp/amd-kds.client.spec.tsapps/api/src/confidential-compute/services/amd-snp/amd-snp.service.spec.ts
stalniy
approved these changes
Jul 6, 2026
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
2 participants
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
AMD SEV-SNP attestation validation ([CON-552]) proves a 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 ASK/ARK signing key AMD has revoked (e.g. a known-compromised part) still read asvalid, with the verdict only noting "Revocation not checked". This closes that documented blind spot.This is a back-port of console-air#52 into this repo's production attestation service (
apps/api/src/confidential-compute/services/amd-snp/) — console-air's copy was itself ported fromapps/api, so this brings the fix home and adapts it to the API's conventions (tsyringe DI,@Memoize,LoggerService).Fixes 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 hand-rolled design). Extracts revoked serials +nextUpdateand verifies the ARK-signed CRL signature (RSA-PSS/SHA-384; an ECDSA issuer is also accepted so test fixtures need not be RSA).amd-kds.client.ts—getCrl(product)fetching/vcek/v1/{product}/crl, cached via the existing@Memoizedecorator (6h; the verifier additionally enforces the CRL's ownnextUpdate), reusing the shared 404→null/ transport-error→throwhandling.amd-snp.service.ts— once authenticity passes, checks the per-product CRL and resolves the verdict:invalid(notRevoked: false)valid(notRevoked: true)unverifiable(revocation unknown — never a silent pass)notRevokedverdict check and drops the "Revocation not checked" detail.Scope is backend-only: the deploy-web
AttestationEvidenceModalrendersstatus/detail(not the per-check breakdown) andnotRevokedalready exists in the generated API types, so a revoked key flips the badge to "invalid" with the new detail automatically. No schema change (notRevokedwas already defined).Testing
amd-crl.parser.spec.ts— hermetic 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 valid-path test now assertsnotRevoked: trueand no "Revocation not checked".npx vitest run src/confidential-compute/→ 66/66 pass; ESLint clean;tscclean for changed files.Summary by CodeRabbit
New Features
Bug Fixes
Tests