Allow for TLS certificate checks to be skipped#224
Conversation
evanrichter
left a comment
evanrichter
left a comment
There was a problem hiding this comment.
I personally like this PR, but I think it needs a few changes before merging (just my opinion, I'm not a maintainer)
I like having the option to skip TLS cert checking, but the option needs to be safe by default, and variable naming should be consistent and straight forward.
| impl Default for SStreamConfig { | ||
| fn default() -> Self { | ||
| SStreamConfig { | ||
| tls_check_certificates: false, |
There was a problem hiding this comment.
The default should definitely be to enforce certificate checking...
I think that may be what's happening here, but the wording is super confusing.
I would expect tls_check_certificates to be true if certificates are going to be checked...
There was a problem hiding this comment.
Good catch, I originally had this variable called 'skip_certificate_check' and didn't exactly invert the default here.
| if let Some(config) = config { | ||
| config.tls_check_certificates | ||
| } else { | ||
| false |
There was a problem hiding this comment.
again, I think the variable wording is opposite what I expect, and a cursory code review would suggest that the service does not check certs by default
| .add_server_trust_anchors(&webpki_roots::TLS_SERVER_ROOTS); | ||
|
|
||
| if !tls_check_certificates { | ||
| tls_config.client_auth_cert_resolver = Arc::new(NoVerifyTLS); |
There was a problem hiding this comment.
I'm honestly confused again what this variable does and what it's default behavior is. My understanding is that this variable is by default false and therefore NoVerifyTLS is by default the configuration. If that is the case, then I suggest:
- leave the variable name as is
- make the variable
trueby default - make all default configurations check the certificate by default
There was a problem hiding this comment.
Fully agree that strict checks should be the default.
There was a problem hiding this comment.
Yes, we definitely want this true by default. Thanks for updating.
| "ws" => { | ||
| if addr.is_ipv4() { | ||
| SStream::new_v4(None) | ||
| SStream::new_v4(None, None) |
There was a problem hiding this comment.
It's not always the case that connecting to the server over plaintext means that certificates should not be enforced. I run synapse on my local network, so I use HTTP, but I still want certs enforced by default.
There was a problem hiding this comment.
I think a nice feature (though doesn't have to be done now) would be adding a flag which enables skipping tls verification in sycli now. This seems like it's going to enforce by default which should be ok.
|
|
||
| [dependencies] | ||
| rustls = "0.18.0" | ||
| rustls = { version = "0.19", features = ["default", "dangerous_configuration"] } |
There was a problem hiding this comment.
you might not need the dangerous_configuration feature since you don't call rustls::client::ClientConfig::dangerous()
There was a problem hiding this comment.
Unfortunately ServerCertVerifier is only exported with this feature enabled: https://github.com/rustls/rustls/blame/0f17531c6fb898e564c7d6ffbfec183ce66db508/rustls/src/lib.rs#L377-L381.
There was a problem hiding this comment.
ah I missed that, agreed!
|
@evanrichter thanks for the review! |
Luminarys
left a comment
Luminarys
left a comment
There was a problem hiding this comment.
Thanks for making this change! And thanks for the review @evanrichter. Just a couple minor things and I think it can be merged.
| ) -> SStreamConfig { | ||
| SStreamConfig { | ||
| tls_check_certificates: tls_no_verify, | ||
|
|
There was a problem hiding this comment.
Can we remove this empty line?
| let mut config = rustls::ClientConfig::new(); | ||
| config | ||
| let tls_check_certificates = | ||
| if let Some(config) = config { |
There was a problem hiding this comment.
Let's convert this to an optional mapping, so something like config.map(|config| config.tls_check_certificates).unwrap_or(true);
| .add_server_trust_anchors(&webpki_roots::TLS_SERVER_ROOTS); | ||
|
|
||
| if !tls_check_certificates { | ||
| tls_config.client_auth_cert_resolver = Arc::new(NoVerifyTLS); |
There was a problem hiding this comment.
Yes, we definitely want this true by default. Thanks for updating.
| "ws" => { | ||
| if addr.is_ipv4() { | ||
| SStream::new_v4(None) | ||
| SStream::new_v4(None, None) |
There was a problem hiding this comment.
I think a nice feature (though doesn't have to be done now) would be adding a flag which enables skipping tls verification in sycli now. This seems like it's going to enforce by default which should be ok.
3 participants
Allows user to skip TLS certificate checks when connecting to trackers.
Rationale:
When seeding exclusively to private trackers, trust is generally higher than with public trackers. LetsEncrypt certificates expire every three months and require operators to intervene. This disrupts the torrent infrastructure unnecessarily.
Proposal:
Add new
verify_certificatesswitch to the tracker-specific configuration, which defaults to true, but if set to false, configures SStream to use a rustls verifier that skips the server authentication.