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

DNS-Based Authentication of Named Entities (DANE) #816

Open
Zash opened this issue Sep 11, 2021 · 13 comments
Open

DNS-Based Authentication of Named Entities (DANE) #816

Zash opened this issue Sep 11, 2021 · 13 comments

Comments

@Zash
Copy link

Zash commented Sep 11, 2021

Support for RFC 6698 would be nice to have. It has some usage in email and XMPP.

This would require a way to validate certificates against some bytes retrieved securely from DNS (this retrieval would be out of scope). Usually those bytes are a hash of the certificate public key (SubjectPublicKeyInfo structure).

@djc
Copy link
Member

djc commented Sep 15, 2021

What exactly do you think we should implement to enable DANE support?

As far as I can tell you could use Session::get_peer_certificates() with something like x509-parser to get the SubjectPublicKeyInfo out; I'm unconvinced rustls wants to be in the business of parsing X509 certificates more than strictly necessary to do the authentication steps needed to bootstrap a secure connection.

@horazont
Copy link
Contributor

horazont commented Sep 15, 2021

As far as I could tell, there's no way to hand arbitrary data to verify_server_cert. That means to verify DANE during the handshake, it would require using a separate instance of a ServerCertVerifier implementation for each connection, which AFAICT means having separate ClientConfigs which is not the goal of ClientConfig.

Of course, I could do the verification after the handshake, but that seems a bit dangerous.

I think what this boils down to is that the dns_name passed to verify_server_cert is in fact an implementation detail of the webpki-based verification. Instead, we'd need something a generic authentication data which matches the verifier class used, which is just passed through the ClientSession in some way (just like hostname is passed through).

I can't think of a good design off the top of my head for that though :)

Edit: Full disclosure: I'm working on a rust-based network backend for a thing where Zash is a main developer, so we'll probably be intermixing our participation here somewhat :).

@djc
Copy link
Member

djc commented Sep 15, 2021

On current main, ClientConnection::new() takes a ServerName enum. We plan to use this to allow callers to pass an IP instead of a hostname and also to enable support for ECH at some point. Maybe DANE could be supported as yet another variant of the ServerName enum?

@horazont
Copy link
Contributor

Thanks for the reply.

It does not fit the ServerName enum name to 100%, but it might do. Given that there may be fallbacks in place when using DANE, what do you think about passing a slice or Vec of ServerNames? Then one could pass the DNSName as well as for instance the TLSA records. The ServerName would then become a generic "server identity" thing and verifiers would use whichever identifier type they are comfortable dealing with.

That obviously only works as long as each ServeName variant on its own is strong enough to verify a server's identity, otherwise a verifier which is unable to handle a specific (but crucial hypothetic) ServerName variant could cause a security issue.

Am I making sense here?

@djc
Copy link
Member

djc commented Sep 18, 2021

I think you should come up with a ServerName::Dane variant and insert into it whatever you think we need; verifiers can probably just raise an error if they encounter a variant they don't know how to deal with. I think the intention is already that ServerName is a generic "server identity" thing, but would be good to come up with a specific design for how a Dane variant would look.

@Zash
Copy link
Author

Zash commented Sep 19, 2021

Sounds reasonable. Presumably such a variant would consist of at least a list of TLSA records and an optional server name.

@wez
Copy link

wez commented Sep 3, 2023

I'm taking a run at implementing this. My game plan is:

  • Add ServerName::Dane variant that includes the DnsName and TLSA records
  • Add a TLSARecord type (alternative, better name suggestions welcomed!) that is something like this, borrowing the CertUsage, Selector and Matching types from the trust-dns repo. I'm assuming that defining the struct here is preferable to adding a dep on trust-dns-proto and importing their definition of the type.
struct TLSARecord {
   cert_usage: CertUsage,
   selector: Selector,
   matching: Matching,
   cert_data: Vec<u8>,
}

enum ServerName {
   ...
   Dane {
       dns_name: DnsName,
       tlsa: Vec<TLSARecord>,
   }
}
  • The DnsName embedded in the Dane variant will be used in the SNI extension
  • Take a stab at implementing the verification logic discussed in https://datatracker.ietf.org/doc/html/rfc6698#appendix-B.2
  • Part of that will require extracting the SubjectPublicKeyInfo info for some comparisons. I'm conscious of earlier discussion about resisting parsing certificates overly much. At the current time, the DANE RFCs all seem to refer to the certs as being DER encoded, which seems to be in alignment with the existing Certificate type, so I'm hoping that adding support for extracting that only for DER certs/selectors/usage will be acceptable.

@djc
Copy link
Member

djc commented Sep 3, 2023

This seems mostly sane from a practical architecture perspective, though others might come along who have opinions about the security/desirability of it all. Some suggestions to make an eventual review go more smoothly:

  • Our DER parsing and verification handling code all lives in rustls-webpki, so you'll probably want to plug most of that stuff into https://github.com/rustls/webpki/blob/main/src/end_entity.rs#L94. There should also be SubjectPublicKeyInfo-handling code in there somewhere.

  • We don't allow webpki types in the public rustls API, so dealing with TlsaRecord and all of its constituent parts across that API boundary might be a little tricky. For most of these things, I suggest plugging into the nascent pki-types effort -- that is, base your rustls branch on Switch to using the pki-types crate #1432 -- so you can push these basic types down into rustls-pki-types. rustls-pki-types is designed to go for a long time without semver-incompatible updates, so design your data types for that.

  • We follow the API naming guidelines, so TLSARecord would be named TlsaRecord.

(Maybe it would simplify things a little to just stuff the DnsName into the TlsaRecord instead of next to it?)

@wez
Copy link

wez commented Sep 4, 2023

(Maybe it would simplify things a little to just stuff the DnsName into the TlsaRecord instead of next to it?)

I'm not sure about this because the name is to be set in the SNI extension, but if there are multiple names that allows for the possibility of having varying names (which isn't really in the source data from DNS), and if that was really a thing, how should you reconcile which one is returned for SNI? My initial sense, before diving into the webpki part of this, is that it feels simpler to keep that separate.

@ctz
Copy link
Member

ctz commented Sep 6, 2023

I guess I have some more fundamental questions about this -- do people actually use DANE to achieve a meaningful security goal, given it is predicated on DNSSEC a) actually working reliably, b) being deployed widely, c) domain registrars having an economic reason to be competent, and d) reaching a decent cryptographic security level as a whole.

AIUI the answers to those are a) sometimes it does, b) nope, c) lol, d) 1024-bit RSA.

@wez
Copy link

wez commented Sep 7, 2023

For the trend as it applies to SMTP:
https://github.com/baknu/DANE-for-SMTP/wiki/4.-Adoption-statistics

https://list.sys4.de/hyperkitty/list/dane-users@list.sys4.de/thread/CLLDBG3UYPRXZPG33BLDZ3NR3G2TNNM5/ has the latest batch of stats.

There are currently 3.9 million domains that have correctly deployed DANE, and the trend is growing, especially in Europe: https://github.com/baknu/DANE-for-SMTP/wiki/5.-Government-and-registry-policy

@ctz
Copy link
Member

ctz commented Sep 8, 2023

How about a more general extension point:

enum ServerName {
   ...
   Custom {
       pub name_for_sni: Option<DnsName>,
       pub data: Box<dyn Any>,
   }
}

That can be used with a custom ServerCertVerifier to smuggle arbitrary data related to the server's identity (at the cost of an extra allocation, and RTTI). The same mechanism could be used for HPKP, DANE, or others equally, but naturally leaves the details of that to be agreed between the application and ServerCertVerifier implementation.

@wez
Copy link

wez commented Sep 12, 2023

I'm open to that; rustls/webpki#174 feels necessary to facilitate that. I took a quick stab at pulling that branch into rustls but there are quite a few things that need to be reconciled with the other changes there before I can make progress on that.

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

No branches or pull requests

5 participants