tokio-rs/tokio-tls

Bad API even worse than thought: `danger_connect_async_without_providing_domain_for_certificate_verification_and_server_name_indication`

briansmith opened this issue · 7 comments

briansmith commented

The choice of whether to send SNI should be separated from the choice of whether to validate the certificate for the (SNI) host name.

In particular, a server might use the SNI to route a connection to a backend, and/or it might fail to do any routing if SNI isn't provided, so SNI is needed in some cases even when certificate validation isn't done.

Also, I imagine some users might want to avoid putting SNI in the client hello but still want to validate the certificate for a particular host name, e.g. if they're trying to hide the hostname from the plaintext part of the TLS connection.

Some users of this API are already doing the wrong thing. For example, this one is disabling SNI when only disabling certificate verification is intended: https://github.com/hyperium/hyper-tls/blob/da69ab8b9897f3b2bce25e5afce02ff94b750985/src/client.rs#L93-L100

Also, IMO, the scope of this crate should be "connect Tokio and rust-native-tls" together. tokio-tls should have a way of accepting a configuration that is configured using the rust-native-tls APIs, and that should be the way configuration of SNI, certificate validation, etc. is done, without any such APIs in tokio-tls itself.

Therefore, I propose we remove this API.

See the related discussion at quininer/tokio-rustls#14 (comment)

alexcrichton commented

cc @sfackler, thoughts?

sfackler commented

Removing the API from here would require making some changes to native-tls and openssl's APIs but I'm not opposed to splitting these up.

sfackler commented

This will be complex on OSX since SNI and hostname verification both use the same method: https://developer.apple.com/documentation/security/1393047-sslsetpeerdomainname.

briansmith commented

See SCH_CRED_NO_SERVERNAME_CHECK and SCH_CRED_SNI_CREDENTIAL. My hypothesis is that they work like this:

  • Neither flag set - SNI with server name checking when name is provided.
  • SCH_CRED_NO_SERVERNAME_CHECK = no SNI and no server name checking.
  • SCH_CRED_NO_SERVERNAME_CHECK | SCH_CRED_SNI_CREDENTIAL = SNI with no server name checking.

However, I've not found any evidence that this hypothesis is true; it's just speculation after 10 minutes of Googling.

Maybe macOS has something similar.

Regardless, I think that all of this should be handled outside tokio-tls and inside rust-native-tls instead.

sfackler commented

I'm confused - the method here directly forwards to the equivalent method in native-tls: https://github.com/tokio-rs/tokio-tls/blob/master/src/lib.rs#L193-L203.