RTE-839, RTE-840, RTE-841, RTE-842: Initial refactor of dcap-artifact-retrieval-tool by rui-alm · Pull Request #911 · fortanix/rust-sgx

rui-alm · 2026-03-18T10:12:54Z

Some experimental code which might make sense to use as the initial step for the refactoring of dcap-artifact-retrieval tool. It includes:

Simplification of generics affecting Client, BackoffService, PcsService.
API services are now stateless and therefore their functions are now static.
Attestation specific clients that expose only their supported APIs. Azure behavior remains the same: only pckcert is obtained while other APIs still going via Intel PCS.
Similar Rest APIs are implemented on internal methods which accept a superset of parameters to cover the possible combinations.
Some lifetime reference annotations have been removed or moved to the functions that actually need them.
Replacement of build_input with explicit initialization of input types.

RTE-785 contains the design spec and links to several tickets with the expected following steps, which also include some of the unfinished work from this experimental branch.

…ify generic arguments.

…ed Rest APIs

…csService::call_service function to static.

tvsfx · 2026-03-26T00:11:00Z

intel-sgx/dcap-artifact-retrieval/src/provisioning_client/azure.rs

+            azure_base_url: PckCertApi::SECONDARY_CERT_URL.to_owned(),
+            azure_client_id: PckCertApi::DEFAULT_CLIENT_ID.to_owned(),
+            azure_api_id: PckCertApi::API_VERSION_07_2021.to_owned(),
+            pckcert_service: CachedService::new(


We don't actually seem to care about configuring cache_capacity and cache_self_time, right? We should probably introduce a shorthand to create a wrapped service. Like a function that can produce any CachedService<T: ProvisioningServiceApi>

tvsfx · 2026-03-26T00:42:26Z

intel-sgx/dcap-artifact-retrieval/src/provisioning_client/pccs.rs

-        Ok((url, Vec::new()))
-    }
-
-    fn validate_response(&self, status_code: StatusCode) -> Result<(), Error> {


Easy refactor; validate_response can just return a statically defined map of error codes and corresponding output strings (also, errors shouldnt be capitalized). We can do the boilerplate in common code.

tvsfx · 2026-03-26T00:53:23Z

intel-sgx/dcap-artifact-retrieval/src/provisioning_client/mod.rs


-struct BackoffService<T: for<'a> ProvisioningServiceApi<'a> + Sync + ?Sized> {
-    service: PcsService<T>,
+struct BackoffService {


This might be nitpicky, but if you want to structure your services like this, it feels wrong to me to not bind a BackOffService to a specific T, same for PcsService.

The fact that the call_service function for BackoffService now has a different signature for the one for CachedService is a symptom of this; this asymmetry is confusing to users and makes it so you could not define a CallableService trait and define CachedService using another generic C: CallableService with field service: C. Not saying this design is better but the fact that you cannot do this seems odd.

The reason I've gone with this approach is because PcsService is now stateless, which meant BackoffService no longer needed to have a service member. This means none of BackoffService members need the type T anymore. Only the function call_service needs to be aware of the type to call the corresponding (static) T::call_service.

I see CachedService differently because it actually has a member cache which requires the type T, has the cache is returning a value of type T::Output.

Maybe BackoffService is an over-simplistic example, but if there were other functions, including static ones, that don't required any use of type T, their invocation would still require one to be passed. With this approach, only call_service needs to be parameterized with T and other functions wouldn't.

In some quick sandboxed test I've done, the compiler even warns me that type T is unused if I don't have a member variable that uses it, even if there a member function that does.

I understand why you want to do this from a technical perspective :) I just don't think it's the correct thing to do from a domain modeling perspective. See my other comment for why modeling things this way can get awkward, especially if you have multiple methods that rely on your generic (you'll have to specify what the generic is each time): https://github.com/fortanix/rust-sgx/pull/911/changes#r2991883027

but if there were other functions, including static ones, that don't required any use of type T, their invocation would still require one to be passed.

So what I'm arguing is that the concept of a Service doesn't make sense without it being tied to a specific API; since we say that a CachedService needs to be tied to a specific API, I think it's weird to then also say that a BackoffService does not need to be tied to a specific API, even though it's also a service. I'm hence implicitly also saying that I don't want to model such static functions as part of the Service API.

In some quick sandboxed test I've done, the compiler even warns me that type T is unused if I don't have a member variable that uses it, even if there a member function that does.

Did you not use PhantomData? That's what that's for.

tvsfx · 2026-03-26T00:56:58Z

intel-sgx/dcap-artifact-retrieval/src/provisioning_client/mod.rs

+    ) -> Result<T::Output, Error> {
        if let Some(retry_timeout) = self.retry_timeout {
-            let op = || match self.service.call_service::<F>(fetcher, input) {
+            let op = || match PcsService::call_service::<F, T>(fetcher, base_url, input) {


cfr. my comment above: note how you're forced to specify T here, even though we know which service we want to call at the time when our top-level cached service is created. Technically, the reason this doesnt work is because the compiler doesnt reason backwards from T::Input to the ProvisioningServiceApi we need. That's why its more convenient to bind this instance into the struct when we create it.

tvsfx · 2026-03-26T01:01:56Z

intel-sgx/dcap-artifact-retrieval/src/provisioning_client/mod.rs

 }

-struct PcsService<T: for<'a> ProvisioningServiceApi<'a> + Sync + ?Sized> {
-    service: Box<T>,


Now that we don't need trait objects anymore, we can get rid of the ?Sized (and maybe Sync? not sure why thats here) bounds here, removing them from all places that depend on this.

I think I've gotten rid of all of those, but if you've found some left over, let me know. This comment is on the removed code and I haven't found any more of those on the remaining code.

Yes, I wrote this assuming we might resurrect the T parameter on the other service types. If we don't then this comment might not be relevant.

tvsfx · 2026-03-26T01:02:46Z

intel-sgx/dcap-artifact-retrieval/src/provisioning_client/azure.rs

+        pce_isvsvn: PceIsvsvn,
+        qe_id: Option<&QeId>,
+    ) -> Result<PckCert<Unverified>, Error> {
+        let input = PckCertIn { encrypted_ppid, pce_id, cpu_svn, pce_isvsvn, qe_id, api_version: self.client.client.api_version, api_key, azure_client_id: &self.azure_client_id, azure_api_version: &self.azure_api_id };


I think this is a good change; this feels like the right place to massage input parameters. Made possible by the fact that you got rid of the gnarly trait objects :)

tvsfx · 2026-03-26T01:05:58Z

intel-sgx/dcap-artifact-retrieval/src/provisioning_client/azure.rs

+        let input = PckCertIn { encrypted_ppid, pce_id, cpu_svn, pce_isvsvn, qe_id, api_version: self.client.client.api_version, api_key, azure_client_id: &self.azure_client_id, azure_api_version: &self.azure_api_id };
+        self.pckcert_service.call_service(&self.client.client.fetcher, &self.azure_base_url, &input)
+    }
+    fn pckcerts(&self, pck_id: &PckID) -> Result<PckCerts, Error> {


Do I understand correctly that the old pckcerts_with_fallback implementation was basically a way for the Azure client to emulate the pckcerts API, meaning we can just use it as the pckcerts implementation now that the clients have been split?

Yes. It also applies to PCCS. In fact this branch has introduced the duplication of the fallback code, but I have a ticket to deal with it already.

Rather than having a pckcerts_with_fallback that was implementing 2 behaviours,

the call to the actual PCS pckcerts and

the fallback behavior (based on pckcert) that would only be used for Azure and PCCS

I've split the logic so that PCS implements the supported pckcerts REST API call, and Azure and PCCS implement only the "fallback" behavior.

The fallback pckcerts is slightly different from the PCS pckcerts, because we use the PckId instead of the EncPpid and PceId.

tvsfx · 2026-03-26T01:07:36Z

intel-sgx/dcap-artifact-retrieval/src/provisioning_client/azure.rs

+    }
+
+    fn sgx_tcbinfo(&self, fmspc: &pcs::Fmspc, evaluation_data_number: Option<u16>) -> Result<pcs::TcbInfo<platform::SGX>, Error> {
+        self.client.sgx_tcbinfo(fmspc, evaluation_data_number)


No need to refactor this now, but FYI, there's a crate to make this type of boilerplate more concise if you have a lot of it: https://crates.io/crates/delegate

Since the azure client just contains a regular client, I don't think you need this though; you can just have e.g. AsRef<Client> bounds where you need Client functionality and implement AsRef<Client> for AzureClient

tvsfx · 2026-03-26T01:28:15Z

intel-sgx/dcap-artifact-retrieval/src/provisioning_client/common.rs

 pub const ENCLAVE_ID_ISSUER_CHAIN_HEADER: &'static str = "SGX-Enclave-Identity-Issuer-Chain";
 pub const TCB_EVALUATION_DATA_NUMBERS_ISSUER_CHAIN: &'static str = "TCB-Evaluation-Data-Numbers-Issuer-Chain";

-pub struct PckCertsApiNotSupported;


yay for not introducing spurious abstractions :)

tvsfx · 2026-03-26T01:36:50Z

intel-sgx/dcap-artifact-retrieval/src/provisioning_client/azure.rs

+impl<F: for<'a> Fetcher<'a>> ProvisioningClient for AzureClient<F> {
+    fn pckcert(
+        &self,
+        api_key: &Option<String>,


I do feel it might not be necessary to have the api_key as an argument here; if we dont expect to use different keys, I think it was fine being part of the client's state?

The reason I've put the api_key here is because it's only necessary for pckcert (and PCS pckcerts). Other REST API calls don't have it. When I say necessary, it was only necessary for V3 and it's optional for V4. Given the nature of this field, I'd assume it's a leftover from V3, perhaps a way to not break V3 compatible code, but probably makes no difference to pass an api_key or not when using V4, as usually this is used to authenticate/authorize an API call. Perhaps there are some statistics on Intel's portal, but I doubt it would make difference on the API response.

Perhaps as part of the "Remove V3 support" ticket, we can discuss whether we still need to have this field or should we get rid of it altogether.

Rui Almeida added 5 commits March 6, 2026 14:55

Simplify generics on caching mechanism

89e83b0

Move PCS and PCCS common code. Remove the need for build_input. Simpl…

ba48d86

…ify generic arguments.

Introduce attestation service clients which only expose their support…

425bf0a

…ed Rest APIs

Implement ProvisioningClient on all clients

31db782

Remove generic arguments from PcsService and BackoffService. Change P…

1b3983c

…csService::call_service function to static.

rui-alm requested review from Taowyoo, raoulstrackx and tvsfx March 18, 2026 10:12

tvsfx reviewed Mar 26, 2026

View reviewed changes

Conversation

rui-alm commented Mar 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvsfx Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tvsfx Mar 27, 2026 •

edited

Loading