misc updates and improvements#20
Conversation
ameba23
left a comment
There was a problem hiding this comment.
🥇 Great to have these fixes.
Re: passing in keypair - i would lean towards having its as an Option and generating if not given. But don't feel too strongly.
Re: blocking API for attestation generation - i agree this makes sense as things are right now. But as mentioned here i would consider actually switching to async quote generation via configfs-tsm. Happy to merge this as-is and have that discussion later.
56281b9 to
530838f
Compare
530838f to
56281b9
Compare
12ad663 to
8799225
Compare
implemented builder patterns for cert resolver and verifier. using predefined key-pair is now optional (as well as bunch of other things) |
ccf73a0 to
b513778
Compare
ameba23
left a comment
There was a problem hiding this comment.
💯 Great improvements.
The builder pattern especially is a big improvement.
We should document that if calling attestation generation from a tokio runtime users of the library should use spawn_blocking. But since that was already the case i consider it not directly relevant to this PR.
| let response = reqwest::get(url) | ||
| .await | ||
| let mut response = ureq::get(&url) | ||
| .timeout(Duration::from_millis(1000)) |
There was a problem hiding this comment.
Maybe put the duration in a constant at the top of the file
| let server_verifier = WebPkiServerVerifier::builder_with_provider( | ||
| self.root_cert_store.clone().unwrap_or_else(|| { | ||
| let mut root_certs = rustls::RootCertStore::empty(); | ||
| root_certs.extend(webpki_roots::TLS_SERVER_ROOTS.to_owned()); |
There was a problem hiding this comment.
I think this is a slight behavior change - the default path is to use public webpki roots as well as self-signed, whereas before it was only self-signed.
I don't think this is bad, but maybe worth mentioning in a doccomment somewhere.
There was a problem hiding this comment.
I updated this with b322582:
- we now have separate toggle
with_accepting_self_signed_certs()that allows use of self-signed certs - and
with_root_cert_store()that allows setting up private CA
by default the verifier uses webpki-roots's store with public CAs and has self-signed certs disabled => this is useful when the server/client use public CA signed certs.
with_accepting_self_signed_certs() and with_root_cert_store() are mutually exclusive
so, basically there are 3 modes:
- default, public CA based
- private CA
- self-signed
b.t.w. all 3 can also allow-list leaf-cert's pubkeys.
l.m.k. w.d.y.t.
61b7d58 to
923ef25
Compare
8b21d95 to
67128e4
Compare
(if in the future we add a new attestation type, the catch-all case will backfire)
^ Conflicts: ^ crates/attested-tls/src/lib.rs
67128e4 to
fb259cf
Compare
this PR:
detect()functions (that auto-determine platform) andAttestedCertificateResolver::new()non-async so that they could be used outside tokioverify_attestation_sync()in the context of blockingverify_attestation_binding()primary_nametosubjectin the context of TLS cert related functionsrcgento avoid version collisions when the downstream needs to accessrcgen