Skip to content

Improve PCCS configuation and support Intel PCS API keys - #88

Open
ameba23 wants to merge 7 commits into
mainfrom
peg/pccs-modes
Open

Improve PCCS configuation and support Intel PCS API keys#88
ameba23 wants to merge 7 commits into
mainfrom
peg/pccs-modes

Conversation

@ameba23

@ameba23 ameba23 commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

This does two things:

Improve PCCS config options

Improves PCCS configuration options to addresses a bug found by @samlaf - see #70 (comment)

Previously, with no internal PCCS cache configured, a custom remote one cannot be used - we always default to intel PCS.

In some cases we would want a remote pccs rather than the internal in-memory cache. For example when we have multiple instances of the attested TLS proxy behind a load balancer (as we do on Builderhub) they can share a common cache.

This PR refactors things to make the Pccs on AttestationVerifier mandatory (not Option) but add a 'remote mode' which always fetches from the remote resource.

Add support for user-supplied Intel PCS API keys

Closes #66 - see that issue for explanation as to why this might be useful.

Base automatically changed from peg/attestation-verifier-builder to main August 27, 2026 09:19

@samlaf samlaf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only did a superficial pass but LGTM. Appreciate the docs :)

Comment thread crates/pccs/src/lib.rs Outdated
Comment on lines +47 to +60
pub enum PccsMode {
/// Fetch collateral from the configured endpoint for every asynchronous
/// lookup, without keeping an internal cache.
///
/// Synchronous lookups are unavailable in this mode because fetching
/// collateral requires asynchronous I/O.
Remote,
/// Start pre-warming an internal cache when [`Pccs`] is constructed.
///
/// Call [`Pccs::ready`] to wait for the initial pre-warm to complete.
Prewarmed,
/// Start with an empty internal cache and fetch collateral on demand.
Lazy,
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious why you chose to use PccsMode::Remote instead of PccsCache::None. Is it because not having a cache doesnt necessarily imply that you are fetching from a remote..?

Otherwise Prewarmed and Lazy are describing a cache strategy whereas remote doesnt (arguably indirectly). Very possible Im just misunmderstanding because I havent reviewed super closely and dont fully understand the subtleties in this PR.

@ameba23 ameba23 Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because you can set this to be a remote PCCS instance. That is, rather than having an internal cache, you can run one on a remote server and use this instead.

Why would you want to do this?

  • For one-shot verifications where you wont keep an instance of AttestationVerifier running over multiple verifications but still want fast fetch.
  • For multiple instances behind load balancer that should share a common cache located on same zone or host.
  • Intel PCS is rate limited for anonymous users. We have an open issue for adding the option to add an API key here PCCS - allow configuring an API key for Intel PCS #66 - but in some cases this adds friction as all your users need to create one. Providing a PCCS with API key exclusively for your service speeds things up. dcap-qvl actually defaults to https://pccs.phala.network run by Phala.

So 'None' implies no cache, wheres it might actually be a remote cache.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right but isn't that just a matter of changing the pccs_url? So you'd just turn the cache off and then point to some local shared pccs endpoint that acts as the cache, and in that case "remote" is kind of wrong right given that it's actually inside your own local network.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Yeah i see what you mean. But to me "None" also seems kind of wrong if there is a PCCS somewhere.

How about "External"?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will start off by saying that I hope this discussion is not blocking merging anything more important on top. I tend to be very nitpicky and like to keep digging until I find the perfect or as close as perfect as possible interface, so appreciate you going back and forth with me! But I also dont want this to block other work.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a comment on #66 (comment) related to this discussion. In the context of that switch, I think the API surface that I'd prefer is something like:

pub enum CollateralSource {
    /// Intel PCS. A subscription key lifts the anonymous rate limit.
    IntelPcs { subscription_key: Option<String> },
    /// Any PCCS-compatible service: self-hosted, Phala, a local sidecar.
    Pccs { url: String },
}

/// Whether and how `Pccs` keeps an in-process collateral cache.
pub enum CachePolicy {
    /// No in-process cache. Every lookup goes straight to the endpoint.
    Passthrough,
    /// Cache starts empty and fills on demand.
    OnDemand,
    /// Cache fills at construction, then refreshes proactively.
    Prewarmed,
}

and then Pccs' constructor would become:

pub fn new(source: CollateralSource, cache: CachePolicy) -> Self

WDYT of this? Does this fit with your mental model?

@ameba23
ameba23 requested a review from samlaf September 4, 2026 07:28
@ameba23

ameba23 commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

@samlaf updated this based on your suggestions let me know what you think

@ameba23 ameba23 changed the title Make Pccs mandatory but add a remote mode with no internal cache Improve PCCS configuation and support Intel PCS API keys Sep 4, 2026

@samlaf samlaf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I like it

Comment thread crates/pccs/src/lib.rs
Comment on lines +251 to +254
let Some(inner) = &self.inner else {
let collateral = fetch_collateral(&self.collateral_client, fmspc, ca).await?;
return Ok((collateral, true));
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude is highlighting this pre-existing bug that seems easy to fix:

Passthrough skips the collateral freshness check that the caching policies enforce. crates/pccs/src/lib.rs:251

let Some(inner) = &self.inner else {
    let collateral = fetch_collateral(&self.collateral_client, fmspc, ca).await?;
    return Ok((collateral, true));
};
let now = i64::try_from(now).map_err(|_| PccsError::TimeStampExceedsI64)?;

The early return bails out before now is even converted, so Passthrough never calls extract_next_update. The cached policies do, and that function rejects collateral where now >= min(tcb, qe, root_ca_crl, pck_crl).

Downstream, dcap_qvl's verify checks tcb_info.next_update and qe_identity.next_update itself (dcap-qvl-0.5.2/src/verify.rs:316,374) — but not the two CRLs. So the delta is real: under PassthrougCA CRL is accepted, and under OnDemand/Prewarmed the samecollateral is rejected. Stale revocation data is exactly the thing a CRL nextUpdate exists to prevent, and Passthrough is now the default
policy.

Same gap existed on main via the old pccs: None path, so than introduced — but the PR is what promotes Passthrough to a documented first-class mode, which makes it the right moment to close. One line:

let Some(inner) = &self.inner else {
    let now = i64::try_from(now).map_err(|_| PccsError::
    let collateral = fetch_collateral(&self.collateral_client, fmspc, ca).await?;
    extract_next_update(&collateral, now)?;
    return Ok((collateral, true));
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PCCS - allow configuring an API key for Intel PCS

2 participants