feat: add endorsements.skip_validation setting for disaster recovery#11
Merged
Conversation
When enabled, endorsement retrieval failures are demoted to warnings instead of hard errors, allowing the server to start and serve attestation when endorsement infrastructure is unavailable. Only retrieval failures are skipped — if endorsements are successfully fetched, measurement comparison is always enforced regardless of this flag, ensuring modified images cannot pass attestation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughA new configuration toggle Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant EndorsementFetcher
participant Verifier
Client->>Server: Request attestation
Server->>EndorsementFetcher: fetch endorsements (parallel)
alt fetch succeeds
EndorsementFetcher-->>Server: endorsements
Server->>Verifier: verify signatures & compare measurements
alt verification succeeds
Verifier-->>Server: verified
Server-->>Client: attestation response (endorsed)
else verification fails
Verifier-->>Server: error (verification/mismatch)
Server-->>Client: attestation error (fatal)
end
else fetch fails
EndorsementFetcher-->>Server: wrapped errEndorsementRetrieval
alt EndorsementSkipValidation = true
Server--xServer: log warning, skip endorsement validation
Server-->>Client: attestation response (without endorsement verification)
else
Server-->>Client: attestation error (fatal)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/endorsements_test.go (1)
817-878: Add a regression test for post-fetch resolve failures under skip mode.The new tests validate canceled-context retrieval failures, but not failures that happen after successful fetch (e.g., invalid cosign bundle/verification error). Please add one case asserting those errors are not skipped when
endorsements.skip_validation=true.Also applies to: 1562-1635
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/endorsements_test.go` around lines 817 - 878, Add a new test in endorsements_test.go (alongside TestValidateOwnEndorsements_SkipValidation_*) that exercises a post-fetch resolution/verification failure (fetch succeeds but cosign bundle / verification or resolve fails) and asserts that endorsements.skip_validation=true does NOT suppress that error. Reuse newFetcherCache/cache.setGroup to return a fetched payload that will fail during resolution/verification (e.g., malformed cosign bundle or bad signature) and construct a Server with validateOwnEndorsements, endorsements.skip_validation=true, and a valid selfAttestation; call s.validateOwnEndorsements(context.Background()) and assert err != nil and that err.Error() contains a verification/resolve-related substring (e.g., "verify", "cosign", or "resolve") to ensure post-fetch failures are not skipped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Line 167: Update the sentence describing NewServer behavior to clarify that
endorsements.skip_validation only relaxes failures that occur while
retrieving/validating endorsement data, not device open or Attest failures;
specifically mention that NewServer still exits on device open/Attest errors and
that parsedSelfAttestation is used for endorsement validation while only
endorsement retrieval/validation can be bypassed when
endorsements.skip_validation is enabled.
In `@docs/security.md`:
- Line 113: Change the phrase describing the infrastructure to use a hyphenated
compound modifier: update the text mentioning endorsements.skip_validation to
read "endorsement-serving infrastructure" (hyphenate endorsement-serving) so the
sentence reads "when the endorsement-serving infrastructure is completely
unavailable"; ensure the surrounding sentence and the startup warning reference
remain unchanged.
In `@internal/endorsements.go`:
- Around line 198-204: The current handling treats any error from
resolveEndorsements as skippable when s.cfg.EndorsementSkipValidation is true;
change the logic to only skip when the error represents a retrieval/network
failure (not verification/parsing failures). Update resolveEndorsements to
return or wrap a sentinel/typed error for retrieval problems (e.g.,
ErrEndorsementRetrieval or a RetrievalError type) and then replace the
unconditional skip check around the call that assigns doc, cr, err :=
s.resolveEndorsements(...) so it uses errors.Is/As to detect retrieval-only
errors: if skip is enabled and errors.Is(err, ErrEndorsementRetrieval) then log
warn and return nil, otherwise log/return the original err for non-retrieval
errors (verification/parsing). Apply the same change to the other identical call
site that currently uses s.cfg.EndorsementSkipValidation to ensure only
retrieval errors are bypassed.
---
Nitpick comments:
In `@internal/endorsements_test.go`:
- Around line 817-878: Add a new test in endorsements_test.go (alongside
TestValidateOwnEndorsements_SkipValidation_*) that exercises a post-fetch
resolution/verification failure (fetch succeeds but cosign bundle / verification
or resolve fails) and asserts that endorsements.skip_validation=true does NOT
suppress that error. Reuse newFetcherCache/cache.setGroup to return a fetched
payload that will fail during resolution/verification (e.g., malformed cosign
bundle or bad signature) and construct a Server with validateOwnEndorsements,
endorsements.skip_validation=true, and a valid selfAttestation; call
s.validateOwnEndorsements(context.Background()) and assert err != nil and that
err.Error() contains a verification/resolve-related substring (e.g., "verify",
"cosign", or "resolve") to ensure post-fetch failures are not skipped.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc571454-240e-4480-86c9-f2c2063ae70f
📒 Files selected for processing (9)
CLAUDE.mdcmd/root.goconfig/config.tomldocs/architecture.mddocs/security.mdinternal/config.gointernal/endorsements.gointernal/endorsements_test.gointernal/server.go
Add errEndorsementRetrieval sentinel type to distinguish network fetch failures from verification/parsing errors within resolveEndorsements. Only g.Wait() errors (network) and fetchCosignSignatures errors are wrapped; byte-for-byte mismatch, JSON parse, and cosign bundle verification errors are not. validateOwnEndorsements and validateDependencyEndorsements now use errors.As to skip only retrieval errors, ensuring tampered or malformed content is never silently accepted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.
When enabled, endorsement retrieval failures are demoted to warnings instead of hard errors, allowing the server to start and serve attestation when endorsement infrastructure is unavailable. Only retrieval failures are skipped — if endorsements are successfully fetched, measurement comparison is always enforced regardless of this flag, ensuring modified images cannot pass attestation.
Summary by CodeRabbit
New Features
Documentation
Tests