attestation crate: Add builder pattern for constructing AttestationVerifier - #70
attestation crate: Add builder pattern for constructing AttestationVerifier#70ameba23 wants to merge 8 commits into
Conversation
2722622 to
ea6b96b
Compare
ea6b96b to
d64d3d8
Compare
923b734 to
ddfdb7b
Compare
ddfdb7b to
ecfa3b7
Compare
ecfa3b7 to
6dc3123
Compare
samlaf
left a comment
samlaf
left a comment
There was a problem hiding this comment.
Leaving comments, as requested in #84 (comment)
| pub fn dump_dcap_quotes(mut self) -> Self { | ||
| self.dump_dcap_quotes = true; | ||
| self | ||
| } | ||
|
|
||
| /// Whether to override outdated TCB when on Azure | ||
| /// | ||
| /// This provides a workaround for a known outdated FMSPC used by Azure | ||
| #[cfg(feature = "azure-verifier")] | ||
| pub fn override_azure_outdated_tcb(mut self) -> Self { | ||
| self.override_azure_outdated_tcb = true; | ||
| self | ||
| } | ||
|
|
||
| /// Do not keep an internal DCAP collateral cache | ||
| pub fn with_no_internal_pccs(mut self) -> Self { | ||
| self.internal_pccs_prewarm = None; | ||
| self | ||
| } |
There was a problem hiding this comment.
inconsistent use of with_ prefix. I'd make all of them start with with_ personally.
Also woulld make all of them take the field explicitly, even when its a boolean). Will make the building syntax cleaner (see my other comment in attestation-provider-server)
| /// Keep a DCAP collateral cache, and pre-fill it with all available | ||
| /// collateral | ||
| pub fn with_pccs_prewarmed(mut self) -> Self { | ||
| self.internal_pccs_prewarm = Some(true); | ||
| self | ||
| } | ||
|
|
||
| /// Keep a DCAP collateral cache, starting empty | ||
| pub fn with_pccs_not_prewarmed(mut self) -> Self { | ||
| self.internal_pccs_prewarm = Some(false); | ||
| self | ||
| } | ||
|
|
||
| /// Set the URL used by internal PCCS | ||
| pub fn pccs_url(mut self, pccs_url: String) -> Self { | ||
| self.pccs_url = Some(pccs_url); | ||
| self | ||
| } | ||
| } |
There was a problem hiding this comment.
these pccs functions are a bit confusing. Didn't understand the difference between internal_pccs_prewarm = None and Some(false) at first... and then having pccs_url and internal_pccs_prewarm separate is kind of weird and even allows a combination that is nonsense (passing a url but setting prewarm to None).
How about refactoring into an enum instead?
/// How the verifier obtains DCAP collateral
#[derive(Clone, Debug)]
pub enum PccsMode {
/// No internal collateral cache. DCAP/Azure verification returns
/// [AttestationError::NoPccs].
None,
/// Internal cache pre-filled with all available collateral at build
/// time. `url` defaults to Intel PCS.
Prewarmed { url: Option<String> },
/// Internal cache that starts empty and fetches on demand.
/// `url` defaults to Intel PCS.
Lazy { url: Option<String> },
}
then you can do something like:
AttestationVerifier::builder(policy)
.with_pccs(PccsMode::Prewarmed { url: cli.pccs_url })
.with_dump_dcap_quotes(cli.log_dcap_quote)
.build()
There was a problem hiding this comment.
Yep, agree this is better. Did this, but did not put the url in because we also need the URL in the 'none' case. In the async version of the verifier, no pccs mean it will always use the remote service (given url or intel pcs) on every verification.
There was a problem hiding this comment.
Oh nice! Think this is starting to make more sense to me. Then I'd consider renaming PccsMode to CollateralCache or PccsCache something. CollateralCache::None is a lot more explicit about what its doing.
Also looks like there's a bug since in the None case the pccs_url is not being used and it always defaults to intel.
There was a problem hiding this comment.
Also found a related issue while reviewing this, created #87
…ts/attested-tls into peg/attestation-verifier-builder * 'peg/attestation-verifier-builder' of github.com:flashbots/attested-tls: Add builder pattern for constructing AttestationVerifier Add builder pattern for constructing AttestationVerifier Run clippy on stable in CI to avoid issue with it failing with dependencies
There was a problem hiding this comment.
LGTM outside of the fix below and the ongoing discussion above
| #[cfg_attr(not(feature = "azure-verifier"), allow(dead_code))] | ||
| override_azure_outdated_tcb: bool, |
There was a problem hiding this comment.
| #[cfg_attr(not(feature = "azure-verifier"), allow(dead_code))] | |
| override_azure_outdated_tcb: bool, | |
| override_azure_outdated_tcb: bool, |
There was a problem hiding this comment.
think you meant to remove this?
2 participants
This is a library API breaking change.
This adds a builder pattern for constructing AttestationVerifier. It removes the old constructor and makes several fields private which were public before, so it breaks the API.