Support cipher suite selection #167
Conversation
| cipher_suites.bulk_encryption.iter().map(|alg| match alg { | ||
| TlsBulkEncryptionAlgorithm::Aes128 => "AES128", | ||
| TlsBulkEncryptionAlgorithm::Aes256 => "AES256", | ||
| TlsBulkEncryptionAlgorithm::Des => "DES", |
There was a problem hiding this comment.
Do we really need to offer historical curiosities like DES and RC2?
There was a problem hiding this comment.
I lean towards not being opinionated about what is available. I can remove them if that's your preference, but I can imagine that someone might have some use for them for connecting to old, insecure servers.
| TlsBulkEncryptionAlgorithm::Des => "DES", | ||
| TlsBulkEncryptionAlgorithm::Rc2 => "RC2", | ||
| TlsBulkEncryptionAlgorithm::Rc4 => "RC4", | ||
| TlsBulkEncryptionAlgorithm::TripleDes => "3DES", |
There was a problem hiding this comment.
This should include ChaCha20 IMO.
There was a problem hiding this comment.
That would be nice, but AFAICT Schannel doesn't have any ChaCha20 ciphersuites, and I've just included the GCD of the available algorithms across the different backends.
There was a problem hiding this comment.
I think it's fine that schannel doesn't support it yet - many versions of OpenSSL don't either. As long as it's paired with another algorithm that is supported it'll work.
There was a problem hiding this comment.
Sure, what's the experience you had in mind, though, if the user chooses to only specify ChaCha20 on Windows? As currently written, nothing here returns a Result, and from some testing, it looks passing no bulk encryption algorithms to the supported_algorithms call in Schannel will result in Windows choosing some (most likely system-wide) defaults.
On Windows 10 19h1 where I'm trying this out, it looks like it chose to allow AES-128, AES-256, and 3DES, which seems consistent with https://docs.microsoft.com/en-us/windows/win32/secauthn/tls-cipher-suites-in-windows-10-v1903, but probably not ideal as far as the behavior of this API.
There was a problem hiding this comment.
It would need to fail to handshake, yeah. If that's not easily doable though we can leave it out for now.
| cipher_suite_strings = cartesian_product( | ||
| cipher_suite_strings, | ||
| cipher_suites.signature.iter().map(|alg| match alg { | ||
| TlsSignatureAlgorithm::Dss => "aDSS", |
There was a problem hiding this comment.
Like below, do we really need DSS?
src/imp/security_framework.rs
Outdated
| CipherSuite::TLS_DH_RSA_WITH_3DES_EDE_CBC_SHA, | ||
| CipherSuite::TLS_DHE_DSS_WITH_3DES_EDE_CBC_SHA, | ||
| CipherSuite::TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA, | ||
| // CipherSuite::TLS_PSK_WITH_3DES_EDE_CBC_SHA, |
There was a problem hiding this comment.
Why are some of these commented out?
There was a problem hiding this comment.
All the commented out ones are PSK. I can remove them, they're mostly left in as I was checking my work while manually compiling the lists, and for anyone else checking it over to see that it wasn't forgotten, but explicitly excluded.
src/imp/security_framework.rs
Outdated
| .iter() | ||
| .flat_map(key_exchange_alg_to_cipher_suites) | ||
| .copied(); | ||
| let rest: &[HashSet<_>] = &[ |
There was a problem hiding this comment.
It might be a bit better to load the key exchange ciphers into a hashset and then iterate through the the signature, bulk_encryption, etc ciphers to progressively remove bad ciphers from the set, and then load the remainder into the returned vec.
There was a problem hiding this comment.
That's a good point, I can change this.
There was a problem hiding this comment.
Made an attempt at this, although I'm not entirely convinced that it's better that it was before.
| #[test] | ||
| fn badssl_cipher_suites_no_rsa() { | ||
| let builder = p!(TlsConnector::builder().supported_cipher_suites( | ||
| // Oddly, on Windows, allowing RSA key exchange, but not RSA signature algorithms still |
There was a problem hiding this comment.
This is the kind of edge case I'm worried about in exposing this level of control over cipher suites.
There was a problem hiding this comment.
FWIW, RSA is the only one that's valid in two different parts of the ciphersuite specification, so it's the only one this can happen to. Did some searching and it seems like it might be an expected quirk of Schannel that RSA_KEYX implies RSA_SIGN, although all the discussions that I found were on tangential topics (1, 2).
|
Picking this up again. Added some handling for AES-GCM ciphersuites for OpenSSL specifically, as they aren't included in AES128/AES256, and specifying AESGCM doesn't allow you to choose the bitwidth. FWIW, I've been using a locally patched version of this for a while (with that change), and have had no problems so far. |
|
Can this be merged? I'm getting flagged by my site security department because I'm advertising 3DES. It would be nice to be able to restrict the ciphers. (I didn't see any way, in |
|
I'm interested in this getting merged as well. For the insecure ciphers, perhaps it would be a good idea to call them |
|
any updates on this ? |
The exported API is implemented as an explicit whitelist with a consistent default across platforms based on the allowed ciphersuites in rust-openssl, which is itself based on the list from Python.
The implementation for each backend is roughly what you'd expect.
+.SHA512 is not included as a possible hash algorithm because it appears to be only available in the Security Framework.
Closes #4.