Closed janbrasna closed 1 month ago
The linting commit should be a separate PR. I am glad that it is a separate commit from the rest, but this is a Tomcat PR and the changes and ultimate merge should be for Tomcat alone.
The linting commit actually is linting and removing defaults, which ought to be separate commits, but if you cherry-pick it to a different PR, I'll merge it as-is.
Besides splitting the linting commit to a separate PR, the Tomcat changes here LGTM.
Thanks. Please submit the removed commit with linting changes and removing defaults to another PR.
To support not only OpenSSL, but also JSSE fully, whenever ciphers are configured, TLSv1.3 suites must be included, otherwise TLSv1.3–only clients won't be able to connect if not using OpenSSL.
Fixes https://github.com/mozilla/server-side-tls/issues/280 (similar to #226, but with more possible implementations treating the config different ways)
Rationale
The output template doesn't set any Connector details, so the config works for whatever implementation the consumers use (JSSE Java SSL or OpenSSL, depending on tcnative or APR/native lib):
https://tomcat.apache.org/tomcat-9.0-doc/config/http.html#SSL_Support
We kinda imply OpenSSL due to naming in the configs, but thanks to how the value is parsed JSSE will accept
openssl
naming instead of itsiana
and happily apply that — so we can use one cipherstring for both and not limit what library can be used.Currently works over OpenSSL but fails for JSSE unless RFC 8446 suites are also defined with the rest, given the API differences when using different crypto connectors. (APR won't configure TLSv1.3 at all and apply defaults, while JSSE will adhere to the cipher list for all protocols, needing the TLSv1.3 defaults explicitly enumerated in the allowed cipherstring.)
Significant changes and points to review
It is slightly complicated as the cipherstring is parsed first, checking allowlists, env configs etc., comparing the suites set with some internal list first, and raising warnings for mismatches — so it has to first pass internal checks before being set by the connector/library.
The addition is necessary for JSSE that treats the list as exhaustive (applied to all TLS versions) and won't configure anything what's not on it on its own, but it won't bother OpenSSL as it won't configure what it doesn't have in allowlists (for ≤TLSv1.2, and using defaults for TLSv1.3); so this can be safely used with both implementations.
["TLS_AES_128_GCM_SHA256", "TLS_AES_256_GCM_SHA384", "TLS_CHACHA20_POLY1305_SHA256"]
will be configured and allowed.Testing
https://fix-tomcat-tls13--mozsslconf-dev.netlify.app/#server=tomcat