Improve PCCS configuation and support Intel PCS API keys - #88
Conversation
samlaf
left a comment
There was a problem hiding this comment.
Only did a superficial pass but LGTM. Appreciate the docs :)
| 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, | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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-qvlactually defaults tohttps://pccs.phala.networkrun by Phala.
So 'None' implies no cache, wheres it might actually be a remote cache.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) -> SelfWDYT of this? Does this fit with your mental model?
|
@samlaf updated this based on your suggestions let me know what you think |
| let Some(inner) = &self.inner else { | ||
| let collateral = fetch_collateral(&self.collateral_client, fmspc, ca).await?; | ||
| return Ok((collateral, true)); | ||
| }; |
There was a problem hiding this comment.
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));
};
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
PccsonAttestationVerifiermandatory (notOption) 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.