Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TLSA types that will be used to implement DANE #4

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wez
Copy link

@wez wez commented Sep 4, 2023

I put these in a separate module to avoid clutter, but that can be moved if you prefer to use a different style for this.

One thing I'm not totally sure about is this paragraph at the bottom of https://datatracker.ietf.org/doc/html/rfc6698#section-2.1.1:

The certificate usages defined in this document explicitly only apply
to PKIX-formatted certificates in DER encoding [X.690]. If TLS
allows other formats later, or if extensions to this RRtype are made
that accept other formats for certificates, those certificates will
need their own certificate usage values.

For the sake of simplicity in constructing the TlsaRecord I've encoded the data field as Der, but the above suggests that perhaps the CertificateUsage variants should become "load bearing" members that have Der data inside them.

Thoughts?

refs: rustls/rustls#816

@@ -0,0 +1,120 @@
//! This types in this module relate to the TLSA and DANE
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some typos in these docs; will make a pass to fix these up

@wez
Copy link
Author

wez commented Sep 4, 2023

A couple of problems with this, if TlsaRecord is to be embedded in rustls::ServerName:

  • ServerName is Hash+Eq but Der is not. I can make Der Hash+Eq, seemingly without consequence. I'm not sure that it is valid to relax ServerName to be just PartialEq; I'm not sure how/where ServerName is hashed.
  • The lifetime in Der<'a> either needs to infect ServerName<'a> or requires the use of 'static when embedded in there.

/// certificate can be used by a given service on a host. The target
/// certificate MUST pass PKIX certification path validation and MUST
/// match the TLSA record.
ServiceCertificateConstraint = 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To the extent that this documentation is quoting directly from the spec, perhaps make that clear by using quotes of some kind and linking to the originating section. It feels like there is a lot of boilerplate/repetition here that is making it harder to actually understand the core of the difference between these variants?

#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub enum MatchingType {
/// Exact match on selected content
ExactMatch = 0,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd probably avoid repeating the Match suffix from the type name.

/// for matching type 0, or the hash of the raw data for matching types 1
/// and 2. The data refers to the certificate in the association, not to
/// the TLS ASN.1 Certificate object.
pub association_data: Der<'a>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you alluded to, I don't think this is always Der<'_>, at least I assume that in the hash cases these would just be straight up bytes?

How do you feel about modeling it more like this?

enum CertificateUsage {
    CaConstraint(Selector),
    ..
}

enum Selector {
    FullCertificate(MatchingType),
}

enum MatchingType {
     Exact(Der<'_>),
     Sha256([u8; 32]),
     Sha512([u8; 64]),
}

@djc
Copy link
Member

djc commented Sep 6, 2023

A couple of problems with this, if TlsaRecord is to be embedded in rustls::ServerName:

  • ServerName is Hash+Eq but Der is not. I can make Der Hash+Eq, seemingly without consequence. I'm not sure that it is valid to relax ServerName to be just PartialEq; I'm not sure how/where ServerName is hashed.

Try removing the Hash bound and see what fails? I think it's probably okay to make Der Hash, too.

  • The lifetime in Der<'a> either needs to infect ServerName<'a> or requires the use of 'static when embedded in there.

I think we'll want to let it infect ServerName? IIRC ServerName so far is a rustls concept, might be worth seeing how we represent this in rustls-webpki (where, IIRC, we avoid allocating on the common paths).

@cpu
Copy link
Member

cpu commented Sep 6, 2023

The path forward here feels uncertain enough that it might make sense to make this a draft PR and consider it for merging only after having used it in downstream draft pull requests that specify this branch as a dependency override. I suspect that pursuing the webpki and rustls updates will affect the shape of the common types.

@wez wez marked this pull request as draft September 6, 2023 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants