Certificate implicit PEM/DER conversion assumption

Security issue notifications

If you discover a potential security issue in s2n-quic we ask that you notify AWS Security via our vulnerability reporting page. Please do not create a public github issue.

Problem:

IntoCertificate provides conversion from a set of Rust standard library types into the opaque Certificate struct. This works fine, but the APIs aren't documented for whether they expect PEM or DER encoding, and the user has to look at the source code to find out. Additionally, in user code the necessary conversion to a &str type if you have PEM data in &[u8] currently looks pretty awkward, and missing it means you silently do the wrong thing.

Solution:

Certificate should have public functions for creating it, something like:

fn from_pem(certificate: &[u8]) -> Result<Certificate, Error>
fn from_der(certificate: &[u8]) -> Result<Certificate, Error>

This wouldn't really change the available APIs, but it would IMO make the code more readable than what currently happens, which looks something like this, as an example, when working with existing APIs. (Obviously those can change too, but it seems like the proposed from_pem/from_der would just be a win. Maybe taking impl AsRef<[u8]> would be an easy way to let users pass either &str or &[u8] out of the box.

.with_client_identity(
    // S2N's impl for &[u8] assumes DER, while &str assume PEM.
    std::str::from_utf8(tls.current_peer_cert_and_key().0.as_ref()).unwrap(),
    std::str::from_utf8(tls.current_peer_cert_and_key().1.as_ref()).unwrap(),
)

Does this change what s2n-quic sends over the wire? no
Does this change any public APIs? yes

Yeah, I ran into this kind of API design problem on 1.36.0 too. I changed my code from .with_tls((Path::new("cert.pem"), Path::new("key.pem"))) to .with_tls((cert_pem_bytes, key_pem_bytes)) that were being loaded from the same files as before and was surprised when the library panicked withe a message about not supporting DER, even though I wasn't using DER. Then I clicked through all the relevant rustdoc pages to see if maybe the types mattered, somehow:

Turned out that the types did indeed matter.

Later, for unrelated reasons, I was looking at the RFC describing the PEM format and found quotes like:

As these textual encodings can be used on many different systems as well as on long-term archival storage media such as paper or engravings, an implementer ought to use the spirit rather than the letter of the rules when generating or parsing these formats in environments that are not strictly limited to US-ASCII.

Files MAY contain multiple textual encoding instances.

which tell me that not all PEM files can be represented as &str/String, because those require UTF-8. AFAICT, s2n-quic unconditionally interprets &[u8] as DER, which means s2n-quic currently makes it impossible to use non-UTF-8 PEM data, even though such data is valid.

aws / s2n-quic