Enforce SEV-SNP cert-chain/quote-signature and fix TDX fixture aging - #127
Enforce SEV-SNP cert-chain/quote-signature and fix TDX fixture aging#127ai-13rac1 wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens Teep’s hardware-attestation boundary by ensuring SEV-SNP quote authentication (cert-chain + signature) is enforced by default for tinfoil_v3_cloud, while also making replay/fixture verification stable over time by pinning collateral currency checks to capture-time instead of wall-clock time.
Changes:
- Split SEV-SNP “online verification” into fetch vs crypto-verify phases, add
SEVCertCacheto tolerate AMD KDS flakiness without weakening enforcement. - Remove
tee_cert_chain/tee_quote_signaturefromTinfoilCloudDefaultAllowFail, add them toOnlineFactors, and add aBuildReportoverride to force-enforce definitive crypto-auth failures even if operator misconfiguresallow_fail. - Introduce
verifyTimeplumbing for TDX (and SEV verifier construction) so replay fixtures don’t age/expire as wall-clock advances; update tests and integration fixtures accordingly.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/verify/verify.go | Passes a per-run verification time into TDX/SEV verifier construction to support replay-time pinning. |
| internal/verify/attest_test.go | Adapts offline TDX verifier signature for tests after VerifyTDXQuoteOffline signature change. |
| internal/proxy/relay_internal_test.go | Updates proxy tests for the new NewTDXVerifier constructor signature. |
| internal/proxy/proxy.go | Uses the new verifier constructors and updates offline TDX parsing call sites with explicit verifyTime. |
| internal/provider/neardirect/pinned.go | Updates TDX verifier construction signature for the NearDirect pinned handler. |
| internal/provider/neardirect/pinned_test.go | Updates tests to use an adapter for offline TDX verification with explicit verifyTime. |
| internal/provider/nearcloud/pinned.go | Updates TDX verifier construction signature for the NearCloud pinned handler. |
| internal/integration/venice_test.go | Pins TDX online verification to fixture capture-time. |
| internal/integration/tinfoil_test.go | Asserts tee_cert_chain / tee_quote_signature now pass (not merely “not enforced”) for Tinfoil fixtures. |
| internal/integration/neardirect_test.go | Pins TDX online verification to fixture capture-time. |
| internal/integration/nearcloud_test.go | Pins both model and gateway TDX online verification to fixture capture-time. |
| internal/integration/chutes_test.go | Pins TDX online verification to fixture capture-time. |
| internal/config/config_test.go | Updates allow-fail expectations to reflect enforced-by-default SEV crypto-auth factors for cloud. |
| internal/attestation/tdx.go | Adds verifyTime-pinned time sets for TDX offline/online verification and updates verifier constructors. |
| internal/attestation/tdx_test.go | Updates tests for the new TDX verifier function signatures and verifyTime parameter. |
| internal/attestation/sevcert.go | Introduces a concurrency-safe SEV VCEK cert-chain cache with TTL + capacity bound. |
| internal/attestation/sevcert_test.go | Adds unit tests for cache keying, cloning isolation, expiry, eviction, nil-receiver safety, and concurrency. |
| internal/attestation/sev.go | Splits SEV online verification into fetch vs crypto attribution, adds FetchErr, enables revocation checks, adds sevTimeNow. |
| internal/attestation/sev_test.go | Adds coverage for fetch-vs-crypto behavior and end-to-end crypto verification with committed go-sev-guest testdata. |
| internal/attestation/report.go | Enforces SEV crypto-auth factors by default, adds isCryptoAuthFactor force-enforcement override, improves SEV factor rendering (FetchErr vs crypto failures). |
| internal/attestation/report_test.go | Adds regression tests ensuring forged signatures block even if allow-listed; validates offline/online behaviors for SEV + TDX. |
| func (c *SEVCertCache) Put(key string, chain *pb.CertificateChain) { | ||
| if c == nil || chain == nil { | ||
| return | ||
| } | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
| for k, e := range c.entries { | ||
| if time.Since(e.addedAt) > c.ttl { | ||
| delete(c.entries, k) | ||
| } | ||
| } | ||
| if len(c.entries) >= maxSEVCertEntries { | ||
| c.evictOldestLocked() | ||
| } | ||
| c.entries[key] = sevCertEntry{ | ||
| chain: cloneCertChain(chain), | ||
| addedAt: time.Now(), | ||
| } | ||
| } |
There was a problem hiding this comment.
I am actually against this change. The KDS cache here is the cert chain for the inference provider, not revocation information. It does not need hard expiry. In fact, we should even cache it on disk when we implement supply_chain_caching.md.
The CRL revocation list (which we do not cache at all, and can still cause hard failures upon endpoint failure) would need more aggressive TTL enforcement like this.
|
From a human usage POV, this does not really solve the root problem. As a user, the new defaults still will cause my inference to be randomly unavailable due to KDS downtime. With this PR, my only choice to have reliable inference despite KDS downtime is to either use --offline or go back to placing tee_cert_chain and tee_quote_signature in my allow_fail. Both are more broad validation exemptions than just permitting the KDS to fail. Or at least, would appear to be. Instead, we have this weird special-case FetchErr sub-exception where placing these two in allow_fail doesn't really allow those validations to fail, but instead only allows the KDS validation piece to fail. Architecturally, I would say this whole PR blocks on supply_chain_caching.md. AMD defines expiry for VCEKs (CPU key certs) at 7 years. It defines ARK and ASK key expiry at 25 years. We can hardcode the ARK and ASK certs in our code, but we need fresh VCEKs for each attestation endpoint host. These may change more frequently than reasonable package build and update timescales, simply because fleet hardware may change, esp if GCP or other cloud hosting is the actual hardware owner. It also turns out Tinfoil provides VCEKs in its V2 ATC endpoint, so really we could use that and avoid the KDS hit for this information. We can obtain this key via https://atc.tinfoil.sh/attestation and just ignore the V2 info, extracting only the VCEK. They also provide a live KDS proxy at https://kds-proxy.tinfoil.sh, which might provide CRL (I am less concerned about revocation; such a revocation would hit global tech news I bet). The flow would be:
This all could be done via |
f07f1cc to
5a740c9
Compare
VerifySEVReportOnline previously called go-sev-guest's RawSnpReportContext, which fetches the VCEK from AMD KDS and verifies the chain/signature in one call. Any failure — including KDS simply being unreachable — was recorded as both CertChainErr and SignatureErr, making an availability outage indistinguishable from a cryptographically forged report. This splits the call into an explicit fetch phase (sevFetchChain, using the new SEVCertCache or a live KDS fetch via GetAttestationFromReportContext) and an offline crypto-verify phase (sevAttributeCryptoResult, using SnpAttestationContext with DisableCertFetching so no network is touched). A fetch failure now sets the new SEVVerifyResult.FetchErr (availability); a crypto failure still sets SignatureErr/CertChainErr (forgery), attributed by re-checking the signature alone via SnpProtoReportSignature. SEVCertCache (sevcert.go) caches successfully-verified VCEK chains per chip-ID+TCB for 24h, mirroring spki.go: struct-owned, sync.RWMutex-guarded, nil-receiver-safe, size-capped with oldest-eviction, and returns/stores proto clones so callers can't mutate cached state. A cache hit only supplies certificate bytes — the crypto phase still re-runs every time, so eviction or a cold cache can only force a re-fetch, never unattested pass-through. NewSEVVerifier now owns one cache per verifier instance (closed over, no package-level state) and also threads a verifyNow time.Time through to both phases so replay/fixture verification isn't pinned to the wall clock; call sites in internal/verify/verify.go and internal/proxy/proxy.go are updated accordingly. sevcert_test.go adds cache coverage (TTL expiry, eviction, clone isolation, nil-receiver, concurrent access under -race). sev_test.go adds two genuine-crypto tests against go-sev-guest's real committed AMD-signed test attestation (verify/testdata): one proving a fresh fetch verifies and warms the cache so a subsequent KDS outage still passes from cache, and one proving a corrupted report signature is attributed to SignatureErr (not CertChainErr), with the failed verification never cached. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
TinfoilCloudDefaultAllowFail exempted tee_cert_chain and tee_quote_signature — the two factors that cryptographically authenticate the SEV-SNP hardware quote. Every measurement and key-binding factor derives its trust from that same quote, so exempting the signature check removed the only thing making them trustworthy: a forged quote could pass with Blocked()==false, collapsing hardware attestation to bare TLS trust. Now that VerifySEVReportOnline distinguishes a KDS-availability failure (FetchErr) from a genuine cryptographic failure (SignatureErr/CertChainErr), report.go can enforce the crypto factors without letting a KDS outage block all cloud traffic: - evalSEVCertChainFactor / evalSEVQuoteSignatureFactor replace the fused rendering in evalSEVParseDependent with FetchErr-aware logic: a crypto error still renders Fail; a FetchErr also renders Fail (failing closed on an unobtainable VCEK) but is a distinct, non-secret diagnostic from a crypto failure; a cross-fault (the other check failed but this one wasn't reached) renders Skip rather than a misleading Fail. - BuildReport's enforcement loop gains an isCryptoAuthFactor override: a definitive cryptographic failure of tee_cert_chain/tee_quote_signature (or the gateway equivalents) is force-enforced even if an operator re-adds the factor to their own allow_fail. This deliberately covers only the crypto errors, not FetchErr, so a pure availability failure still follows the normal allow_fail/--offline waiver path. - tee_cert_chain/tee_quote_signature are removed from TinfoilCloudDefaultAllowFail (enforced online by default) and added to OnlineFactors (so --offline still waives the KDS network dependency, same idiom as intel_pcs_collateral). TestMergedAllowFailTinfoilCloudAllowsKDSAndGPUFactorsByDefault is renamed and updated: it now asserts the two factors are absent from (i.e. enforced in) the default online allow_fail list. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
report_test.go: - TestBuildReport_SEVForgedSignatureBlocksEvenIfAllowFailListed: the core regression test — a forged SEV-SNP report signature blocks tinfoil_v3_cloud even if an operator re-adds tee_quote_signature to their own allow_fail, proving the isCryptoAuthFactor override is unconditional for a definitive crypto failure. - TestBuildReport_SEVColdCacheFetchErrFailsClosed: a cold-cache KDS-unreachable result (FetchErr, no crypto error) fails closed by default, distinct from the forgery case above. - TestBuildReport_SEVWarmCachePasses / TestBuildReport_SEVOfflineSkipsViaOnlineFactors: a genuine online verification renders Pass+Enforced, and the sanctioned --offline skip renders Skip+not-Enforced via the OnlineFactors union — neither can contribute to Blocked() (defined as Fail && Enforced). - TestBuildReport_TDXCryptoOverrideBlocksEvenIfAllowFailListed: the symmetric TDX case (tinfoil_v3_direct), since TDX cert/sig verification is offline and the override covers it unconditionally via in.TDX != nil. - TestEvalSEVParseDependent_FetchErr: pins the FetchErr rendering's Fail status and "AMD KDS unreachable" diagnostic directly. internal/integration/tinfoil_test.go: adds tee_cert_chain/tee_quote_signature to TestIntegration_Tinfoil_Fixture's assertMustPass list. This fixture replays genuine AMD KDS responses, so asserting these PASS (rather than merely "not exempted") proves the split fetch/crypto verifier has no false positive against a real attestation — mirroring the pre-existing hard assertion in verify_run_test.go's TestVerifyRun_Tinfoil_Fixture. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Enable CheckRevocations in both sevFetchChain and sevAttributeCryptoResult so revoked VCEK/ASK certificates are checked against AMD's CRL, matching TDX which already had CheckRevocations: true. - Add sevTimeNow helper to convert zero time.Time to time.Now() before passing to sevverify.Options.Now. go-sev-guest does not actually fall back to wall clock for zero values despite its godoc claiming so. Matches the TDX tdxTimeSet nil-guard pattern. - Add in.GatewayTDX to isCryptoAuthFactor override condition so gateway TDX crypto-auth factors are force-enforced regardless of allow_fail. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
5a740c9 to
8f472df
Compare
|
Agreed on splitting this up — the fixture-aging fix is now merged separately as #128. Moving this to draft while we work through VCEK sourcing, but wanted to share some analysis on the security architecture before we proceed. The ATC-based VCEK flow is soundVCEKs are X.509 certificates signed by AMD's PKI — they're self-authenticating regardless of delivery channel. Steps 4–6 in your proposal (validate against embedded AMD roots, match HWID+TCB extensions to the fresh V3 report, verify report signature) are the actual trust anchors, not the transport. Whether the VCEK bytes arrive from AMD KDS, Tinfoil's ATC endpoint, or a local cache file, the cryptographic verification is identical. The ideal fallback chain would be: local cache → ATC (Tinfoil only) → AMD KDS. This maximizes availability while maintaining the same cryptographic guarantees. Note that ATC is Tinfoil-specific — if we add other SEV-SNP providers, we still need KDS for those. These two concerns are orthogonalYour proposal addresses where VCEKs come from (availability). This PR addresses what happens when verification fails (security). They don't need to block each other. The CRL revocationRe: "such a revocation would hit global tech news" — TCB recovery events (firmware patches that require new VCEKs) are routine, not headline news. The window between AMD issuing a CRL update and a sysadmin manually noticing could be days. That said, per-request CRL checking is an availability risk. A reasonable middle ground: periodic CRL re-validation aligned with Path forwardThe enforcement changes here (split fetch/crypto phases, |
Summary
Fixes the highest-severity finding (H1) from the 2026-07-02 security code review:
tinfoil_v3_cloudexemptedtee_cert_chainandtee_quote_signatureby default, collapsing hardware attestation to bare TLS trust. Every other enforced factor (measurement, boot config, reportdata binding) derives its values from the same unverified quote — an exemptible signature/cert-chain check lets a forged quote pass.The stated rationale was AMD KDS flakiness, but TDX cert/sig verification is fully offline (embedded PCK chain) with no KDS dependency. Two independent review passes converged on this as the top finding.
Changes
FetchErr) are distinguished from cryptographic forgery signals (SignatureErr/CertChainErr). A KDS outage with a cold cache is waivable viaallow_fail/--offline; a definitive crypto failure is force-enforced and can never be exempted.SEVCertCache(24h TTL, per-verifier, concurrency-safe) to absorb AMD KDS flakiness after first contact — the cache, not theallow_faillist, handles routine KDS downtime.tee_cert_chainandtee_quote_signaturefromTinfoilCloudDefaultAllowFail; add them toOnlineFactorsso--offlineremains the one sanctioned way to skip network-dependent checks.isCryptoAuthFactoroverride inBuildReportthat force-enforces a definitive cryptographic failure regardless ofallow_fail, preventing operator misconfiguration from weakening quote authentication.tcbInfovalidity doesn't rot as wall-clock time advances past the collateral's expiry window. Fixes thetee_tcb_not_revokedintegration test failure on the 4 Venice fixtures.CheckRevocations: true) in both fetch and crypto-verify phases, matching TDX which already had it enabled.in.GatewayTDX != nilto theisCryptoAuthFactoroverride so gateway TDX crypto-auth factors are force-enforced regardless ofallow_fail.Result
allow_fail/--offlineTest plan
make checkpasses (includes integration fixture replay)make integrationwith live Tinfoil providertee_cert_chainandtee_quote_signaturenow pass (not just non-enforced) in Tinfoil integration testsallow_fail