Would also like feedback on this from @jsha (concerning FFIability) and @complexspaces (regarding usability within rustls-platform-verifier).

I think this is looking good so far -- my main comment is that we should be making clear somewhere that these are relatively behaviour-less newtypes, so nobody should end up with an expectation that (for example) CertificateDer contains any particular encoding.

For convenient reference I've pushed docs from this PR to https://rustdoc.crud.net/jsha/pki-types/rustls_pki_types/.

One thing I'm wondering: are these newtypes even necessary? Can we pass primitive slices like &[u8] in their place? In general newtypes are nice for preventing mistakes (like passing a key instead of a certificate). But I think for these people just convert bytes to the newtype and gain no real safety. For instance, look at the example for Certificate:

fn load_certificates_from_pem(path: &str) -> std::io::Result<Vec<Certificate>> {
    let file = File::open(path)?;
    let mut reader = BufReader::new(file);
    let certs = rustls_pemfile::certs(&mut reader)?;

    Ok(certs.into_iter().map(Certificate).collect())
}

The map(Certificate) here doesn't seem to add very much safety or clarity. Though I suppose the idea is that rustls_pemfile::certs would return Vec<pki_types::CertificateDer<'a>> instead of Vec<Vec<u8>> as it does now?

Also to make sure I understand correctly: The Cow-alike implementation here (DerInner::Slice and DerInner::Vec) is not a performance optimization, it's to allow operation in a no_std environment that also does not have the alloc library, because Vec is not available in that environment? And in that environment only the DerInner::Slice variant would be available? As a concrete example: on an embedded system, someone might know that some specific 4kB region of memory is available, make a slice out of it (unsafely but with caution), read some bytes into it, and then make a CertificateDer that references that slice?

Assuming all the above is correct, I feel like I've talked myself into understanding why these are necessary, but I'll still include the ruminating since it may be useful to have it written out (and I could be wrong).

From an FFI standpoint I think this should be fine. We currently expose the Certificate type as rustls_certificate: https://github.com/rustls/rustls-ffi/blob/db40a7b8cee883b131d41741135e440a98a0b0eb/src/cipher.rs#L26-L62

That's a simple struct that has one accessor, to get a pointer and length to the contained data. It would probably keep the same interface. It's returned from rustls_connection_get_peer_certificate and rustls_certified_key_get_certificate. These both document the returned pointer's lifetime to be limited by some parent object, so they should be okay with the slice variant.

The other place I would expect to see the rustls_certificate type is rustls_verify_server_cert_params, passed into certificate verification callbacks. But actually the current version just contains a rustls_slice_bytes, which seems reasonable; in the callback, there's always a bounding lifetime (that is, the duration of the callback).

rustls-ffi doesn't current expose TrustAnchor at all. Instead it exposes RootCertStore as rustls_root_cert_store, and offers rustls_root_cert_store_add_pem, which calls add_parseable_certificates.

One slight reservation I have on the FFI front is: will there ever be a case when we want non-Rust code to be able to generate the Slice / borrowed variant of CertificateDer or others? I worry about that scenario because it requires the non-Rust code to carefully manage lifetimes. But I think if it does become necessary we can always document the lifetime requirements, which are not really any more onerous than what C code deals with all the time. And if we want to eventually support no_std + no alloc crate + FFI, it would be necessary.

Oh and one thing to be aware of on the FFI front: &[u8] is nice and easy because we don't worry about #[repr(C)]. We break it down into a pointer and len and give those to the C side as needed. &[Vec<u8>] and &[CertificateDer<'a>] are challenging because we can't just pass a pointer and len; the representation of Vec and CertificateDer are not guaranteed. For rustls_verify_server_cert_callback in rustls-ffi we handled this in a hacky way by collecting a &[&[u8]] in the Rust function before calling the callback. However, I think a more general solution is possible, and preferred: any time we have, e.g. &[CertificateDer<'a>], we need to treat that as its own FFI type. For instance *const rustls_slice_certificate_der. And then to get any given element from that slice, you would call an accessor, e.g. rustls_slice_certificate_der_get_ith(i). That would return *rustls_certificate_der, which would have its own accessor rustls_certificate_der_as_slice() (returning rustls_slice_bytes).

So, overall after chewing it over for a bit: 👍🏻

Thanks for the thorough review!

The map(Certificate) here doesn't seem to add very much safety or clarity. Though I suppose the idea is that rustls_pemfile::certs would return Vec<pki_types::CertificateDer<'a>> instead of Vec<Vec<u8>> as it does now?

Yes, it would return a Vec<CertificateDer<'static>> -- the same would go for rustls_native_certs::load_native_certs().

Also to make sure I understand correctly: The Cow-alike implementation here (DerInner::Slice and DerInner::Vec) is not a performance optimization, it's to allow operation in a no_std environment that also does not have the alloc library, because Vec is not available in that environment? And in that environment only the DerInner::Slice variant would be available? As a concrete example: on an embedded system, someone might know that some specific 4kB region of memory is available, make a slice out of it (unsafely but with caution), read some bytes into it, and then make a CertificateDer that references that slice?

I think there are two goals:

• Make it possible to make these data types work in a no-alloc environment
• Abstract over the use of the data type (that is, the difference between Vec<u8> and &'static [u8]

For the latter, I'm thinking of scenarios where a TrustAnchor or Certificate might be embedded in a library or application as a const (as in webpki-roots), and allocating for it is actually unnecessary.

rustls-ffi doesn't current expose TrustAnchor at all. Instead it exposes RootCertStore as rustls_root_cert_store, and offers rustls_root_cert_store_add_pem, which calls add_parseable_certificates.

So one potential future direction that I think might be interesting is removing the RootCertStore abstraction in favor of just Vec<TrustAnchor<'static>> -- because I think the RootCertStore abstraction isn't providing a whole lot of value.

Abstract over the use of the data type (that is, the difference between Vec and &'static [u8]

For the latter, I'm thinking of scenarios where a TrustAnchor or Certificate might be embedded in a library or application as a const (as in webpki-roots), and allocating for it is actually unnecessary.

Ah, nice. I think the case of embedding a CertificateDer in an application would be quite unusual for a couple of reasons. It means rebuilding and redeploying is required to rotate certificates. Also, the CertificateDer is not much good without a corresponding private key, and it would be bad practice to embed a private key in an application.

The TrustAnchor case is of course sensible and we have a current example of embedding a list of TrustAnchors in a library (webpki-roots). I increasingly worry about the reliability hazard posed by applications that bake in a list of TrustAnchors and don't get updated, but that's not really an issue for this thread (and hopefully we'll solve it by promoting rustls-platform-verifier as the best way).

I increasingly worry about the reliability hazard posed by applications that bake in a list of TrustAnchors and don't get updated, but that's not really an issue for this thread (and hopefully we'll solve it by promoting rustls-platform-verifier as the best way).

Yup, I'm still on board with promoting rustls-platform-verifier as the better verifier, but I also think there are use cases where webpki-roots are quite nice. In general, I would argue rustls-platform-verifier is especially useful on Windows and macOS as (a) on Linux it falls back to the rustls-native-certs approach anyway which (b) depends on distribution trust store management, which might well be less reliable than the Mozilla roots. Obviously that still depends on timely upgrades of webpki-roots and regular build + deployments, but I think that scenario still covers a lot of ground in SaaS applications.

Update:

• Added Debug implementations for the private key types (which do not print the private key bytes)
• Derived PartialEq for all the types
• Added From and Deref impls for CertificateRevocationListDer

Of course the PartialEq implementation for private key types effectively also leaks the private key, but in a less bad way than accidentally leaking it through a derived Debug impl of a downstream type containing it. The PartialEq requirement comes from rustls_pemfile::Item -- it was added in the Initial commit so not sure how load-bearing that is.

One of the things I took from the rustls no-std RFC was whether a unix timestamp newtype would be good, to hoist up that type from webpki into here. Do you think that's useful? I suppose don't need to make that decision now in any case.

Yes, that's definitely something that I think would make sense, but trying to deal with the initial push here first, which is complicated enough when working across 6 different repos.

Addressed a bunch of documentation issues, added a README and crate root documentation, added #![deny(missing_docs)] to make sure all public API is documented and #![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))] to make sure the alloc-based API are documented as such. I think this is ready to publish as a 0.1.0 release (for iteration, then we should probably bump it to 1.0 before we release rustls 0.22?). Once that is published, we can move forward with the webkpi and pemfile PRs.