fix(verifier): align ACME with upstream proto & defer ITA to final step#39
Merged
kingsleydon merged 4 commits intomainfrom Apr 3, 2026
Merged
fix(verifier): align ACME with upstream proto & defer ITA to final step#39kingsleydon merged 4 commits intomainfrom
kingsleydon merged 4 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+281
to
+285
| collector.updateObjectFields(cpuObjectId, { | ||
| intel_trust_authority: JSON.stringify(itaResult), | ||
| }) | ||
| console.log(`[VerifierChain] ITA result added to ${cpuObjectId}`) | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Comment on lines
+309
to
+311
| if (this.cachedBaseDomain) { | ||
| return this.cachedBaseDomain | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Comment on lines
+178
to
+184
| .transform((data) => ({ | ||
| ...data, | ||
| // Derive hist_keys from quoted_hist_keys if not provided by legacy API | ||
| hist_keys: | ||
| data.hist_keys ?? | ||
| data.quoted_hist_keys.map((k) => k.public_key), | ||
| })) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
kingsleydon
changed the title
… final step - Update AcmeInfoSchema to match upstream gateway proto: remove required active_cert/base_domain (legacy optional), add account_attestation and quoted_hist_keys[].attestation, derive hist_keys from quoted_hist_keys - Add GatewayInfoSchema and getBaseDomain() to fetch base_domain from /.dstack/info endpoint when not available in legacy ACME response - Extract ITA from verifyHardware() in all 3 verifiers (KMS, Gateway, App) and run it as the final step in executeVerifiers via runDeferredIta() - Skip ITA entirely if prior verification steps produced errors (early drop) - Add DataObjectCollector.updateObjectFields() for partial field updates to inject ITA results into existing CPU DataObjects Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Cache getAcmeInfo() and getBaseDomain() results to avoid redundant HTTP calls during a single verification run (6-8 calls → 1 each) - ITA skip condition now checks both errors AND failures, not just errors - Parallelize ITA calls across verifiers with Promise.allSettled() Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- ITA now runs independently per verifier based on whether it produced a quote (lastQuoteHex), not on global zero-failure condition. A failure in Gateway verification no longer blocks ITA for KMS/App verifiers. - Only skip ITA entirely on hard errors (thrown exceptions in the chain) - Check updateObjectFields() return value and warn if CPU DataObject is missing instead of silently dropping the ITA result Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
kingsleydon
force-pushed
the
fix/verifier-error-handling
branch
from
ebebca4 to
bd27458
Compare
Comment on lines
+175
to
+176
| active_cert: z.string().optional(), | ||
| base_domain: z.string().optional(), |
There was a problem hiding this comment.
Bug: The active_cert field was made optional, but the verifyCertificateKey function doesn't handle its absence, causing verification to always fail for new gateways that don't provide this field.
Severity: HIGH
Suggested Fix
Update the verification logic to handle cases where active_cert is not provided. A conditional check should be added to skip the verifyCertificateKey step if acmeInfo.active_cert is absent, as this is a valid state for new gateways.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/verifier/src/schemas.ts#L175-L176
Potential issue: The PR updated the ACME info schema to make the `active_cert` field
optional to support new gateways. However, the verification logic in the
`verifyCertificateKey` function was not updated to handle this change. The function
still requires `active_cert` to be present and returns an error if it is `undefined`. As
a result, when the system processes information from a new gateway that legitimately
omits the `active_cert`, the `verifyCertificateKey` function will incorrectly fail,
causing the entire certificate key verification to fail for these valid new gateways.
…eful degradation - Remove global chainErrors gate from runDeferredIta — each verifier's ITA eligibility is determined solely by whether it produced a quote - Skip certificateKey verification gracefully on new gateways where active_cert is absent, instead of producing a false failure - Wrap getAcmeInfo() in try-catch within getBaseDomain() so ACME endpoint failure does not block /.dstack/info fallback - Include verifier name in ITA failure logs for easier debugging Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 tasks
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.
Summary
AcmeInfoResponseremovedactive_cert,base_domain, andhist_keysfields, and addedaccount_attestation. UpdatedAcmeInfoSchemato accept both old and new formats, withhist_keysauto-derived fromquoted_hist_keysvia Zod transform.base_domainresolution: AddedgetBaseDomain()that tries the legacy ACME field first, then falls back to the/.dstack/infoendpoint for newer gateways.verifyHardware()in all 3 verifiers (KMS, Gateway, App) and moved it to the end of the verification chain. ITA runs in parallel viaPromise.allSettled()and is evaluated per-verifier — a failure in one verifier does not block ITA for others. Skipped entirely on hard errors (thrown exceptions).Changes
AcmeInfoSchema— aligned with upstream proto,.transform()deriveshist_keysGatewayInfoSchema— new schema for/.dstack/inforesponseGatewayVerifier— addedgetBaseDomain(), cachedgetAcmeInfo()andgetBaseDomain()to avoid redundant HTTP callsKmsVerifier/PhalaCloudVerifier— removed inline ITA, exposedlastQuoteHexverifierChain.ts— addedrunDeferredIta()with parallel execution and per-verifier eligibilityDataObjectCollector— addedupdateObjectFields()for partial field updatesTest plan
active_cert/base_domain) no longer throw ACME parse errorsintel_trust_authorityfield🤖 Generated with Claude Code