Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The implementation omits the existing GCP provenance TTL and weakens TCB schema validation before PCCS caching.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates attested TLS caching to expire at the earliest TLS, collateral, certificate-chain, or Azure AK dependency deadline.
Changes:
- Retains verified evidence for lazy expiry calculation.
- Adds certificate and collateral deadline parsing.
- Updates attestation and cache tests/API plumbing.
File summaries
| File | Description |
|---|---|
crates/pccs/src/lib.rs |
Exposes collateral expiry calculation. |
crates/attested-tls/src/lib.rs |
Applies dependency deadlines to TLS cache entries. |
crates/attestation/src/lib.rs |
Retains evidence and computes cache expiry. |
crates/attestation/src/cache_expiry.rs |
Adds certificate-chain expiry parsing. |
crates/attestation/src/dcap.rs |
Returns retained DCAP evidence. |
crates/attestation/src/azure/verify.rs |
Retains Azure AK certificate evidence. |
crates/attestation/src/azure/mod.rs |
Defines Azure verified evidence. |
crates/attestation/src/gcp/provenance.rs |
Updates quote borrowing. |
crates/attestation/src/gcp/firmware.rs |
Adapts verification result destructuring. |
crates/attestation/Cargo.toml |
Adds test certificate dependency. |
Cargo.lock |
Locks the new dependency. |
Review details
- Files reviewed: 10/11 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+434
to
+436
| /// This excludes the local GCP provenance cache lifetime. Consumers | ||
| /// must also apply their own freshness limits and TLS certificate | ||
| /// expiry. Evidence and endorsements must remain as returned by |
Collaborator
Author
There was a problem hiding this comment.
Yes, but the seven day cache period for GCP provenance does not come from Google. Its is a design choice of this repo and quite arbitrary. I feel like including it would complicate things for little gain - rather let callers choose this via the TLS cert validity period.
Comment on lines
+706
to
+709
| let tcb_info: CollateralNextUpdate = | ||
| serde_json::from_str(&collateral.tcb_info).map_err(|e| { | ||
| PccsError::PccsCollateralParse(format!("Failed to parse TCB info JSON: {e}")) | ||
| })?; |
ameba23
marked this pull request as draft
September 17, 2026 11:49
ameba23
changed the title
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
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.
The
attested-tlscrate in this repo currently caches a validated attested TLS certificate until the certificate expiry date.It is possible that the related collateral or other associated trust assumptions will expire before this deadline.
This PR changes the behavior to use the earliest associated date. That is, the soonest of the following dates:
notAfter.nextUpdate, and both CRLs’nextUpdate.notAfteracross the collateral issuer chains and PCK certificate chain.notAfter.However this does add some performance overhead in terms of additional parsing to compute this soonest date.
Whether this makes sense depends on the chosen validity period for attested TLS.
The default is currently 5 minutes, with renewal at 2/3 of the validity period - so 3 min and 20 seconds.
With this default, its unlikely much security will be gained by merging this vs the additional parsing overhead.
So im a little unsure if we want this. It might make more sense to put a maximum allowed validity period (eg: 24 hours).
This is an API breaking change (adds an additional field to
VerifiedAttestation).