Bug 67628 - OpenSSLCipherConfigurationParser#parse() produces misleading false positive cipher warnings
Summary: OpenSSLCipherConfigurationParser#parse() produces misleading false positive c...
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Connectors (show other bugs)
Version: 8.5.x-trunk
Hardware: All All
: P2 major (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-10-06 16:29 UTC by Michael Osipov
Modified: 2023-11-28 17:07 UTC (History)
1 user (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Osipov 2023-10-06 16:29:03 UTC
This likely happens in all maintained versions I have just observed this in 8.5.94-dev. This one tooks me some hours to understand and analyze, after 7129db33aa2797b8da17a9aeffeedfafdc725e7a I see false positive warnings which are almost impossible to analyze for many users.

I am running off Java 8 and OpenSSL 1.1.1t (HP-UX), 1.1.1w-freebsd/3.0.11 (FreeBSD).

Consider the following config in server.xml:
> <SSLHostConfig hostName="..." protocols="TLSv1.2+TLSv1.3"
>   honorCipherOrder="true" disableSessionTickets="true"
>   ciphers="HIGH:!aNULL:!eNULL:!EXPORT:!DES:!RC4:!3DES:!MD5:!PSK:!DSS:!SHA1:!SHA256:!SHA384">
>     <Certificate certificateFile="..." certificateKeyFile="..." certificateKeyPassword="..." type="RSA" />
> </SSLHostConfig>

Suddenly I see the following warning:
> 2023-10-05T21:36:05.274 WARNUNG [main] org.apache.tomcat.util.net.SSLUtilBase.getEnabled Some of the specified [ciphers] are not supported by the SSL engine and have been skipped: [[TLS_DH_DSS_WITH_AES_256_GCM_SHA384, TLS_DH_RSA_WITH_AES_256_GCM_SHA384, TLS_ECDH_RSA_WITH_AES_256_GCM_SHA384, TLS_ECDH_ECDSA_WITH_AES_256_GCM_SHA384, TLS_AES_128_CCM_SHA256, TLS_DH_DSS_WITH_AES_128_GCM_SHA256, TLS_DH_RSA_WITH_AES_128_GCM_SHA256, TLS_ECDH_RSA_WITH_AES_128_GCM_SHA256, TLS_ECDH_ECDSA_WITH_AES_128_GCM_SHA256]]

I have started diffing my config back and forth, but wasn't able to spot the issue comparing my cipher expression compared to ALL. Added the following diff to better understand the issue:
> diff --git a/java/org/apache/tomcat/util/net/SSLUtilBase.java b/java/org/apache/tomcat/util/net/SSLUtilBase.java
> index d300737f69..7f62a18ca7 100644
> --- a/java/org/apache/tomcat/util/net/SSLUtilBase.java
> +++ b/java/org/apache/tomcat/util/net/SSLUtilBase.java
> @@ -175,0 +176,1 @@ public abstract class SSLUtilBase implements SSLUtil {
> +        log.info("[" + name + "] with configured: " + configured + ", implemented: " + implemented + ", enabled: " + enabled);

Still doesn't work out for me. Looking at SslUtilBase:
>        List<String> configuredCiphers = sslHostConfig.getJsseCipherNames();
>        Set<String> implementedCiphers = getImplementedCiphers();

Returns false data! While #getImplementedCiphers() truly returns the implemented ciphers by the underlying OpenSSL version, sslHostConfig.getJsseCipherNames() does NOT invoke OpenSSL at all. It invokes "OpenSSLCipherConfigurationParser.parse(getCiphers());" which gives me:
> TLS_AES_128_CCM_SHA256
> TLS_AES_128_GCM_SHA256
> TLS_AES_256_GCM_SHA384
> TLS_CHACHA20_POLY1305_SHA256
> TLS_DHE_RSA_WITH_AES_128_CCM
> TLS_DHE_RSA_WITH_AES_128_GCM_SHA256
> TLS_DHE_RSA_WITH_AES_256_CCM
> TLS_DHE_RSA_WITH_AES_256_GCM_SHA384
> TLS_DHE_RSA_WITH_ARIA_128_GCM_SHA256
> TLS_DHE_RSA_WITH_ARIA_256_GCM_SHA384
> TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256
> TLS_DH_DSS_WITH_AES_128_GCM_SHA256
> TLS_DH_DSS_WITH_AES_256_GCM_SHA384
> TLS_DH_RSA_WITH_AES_128_GCM_SHA256
> TLS_DH_RSA_WITH_AES_256_GCM_SHA384
> TLS_ECDHE_ECDSA_WITH_AES_128_CCM
> TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
> TLS_ECDHE_ECDSA_WITH_AES_256_CCM
> TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
> TLS_ECDHE_ECDSA_WITH_ARIA_128_GCM_SHA256
> TLS_ECDHE_ECDSA_WITH_ARIA_256_GCM_SHA384
> TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
> TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
> TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
> TLS_ECDHE_RSA_WITH_ARIA_128_GCM_SHA256
> TLS_ECDHE_RSA_WITH_ARIA_256_GCM_SHA384
> TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
> TLS_ECDH_ECDSA_WITH_AES_128_GCM_SHA256
> TLS_ECDH_ECDSA_WITH_AES_256_GCM_SHA384
> TLS_ECDH_RSA_WITH_AES_128_GCM_SHA256
> TLS_ECDH_RSA_WITH_AES_256_GCM_SHA384
> TLS_RSA_WITH_AES_128_CCM
> TLS_RSA_WITH_AES_128_GCM_SHA256
> TLS_RSA_WITH_AES_256_CCM
> TLS_RSA_WITH_AES_256_GCM_SHA384
> TLS_RSA_WITH_ARIA_128_GCM_SHA256
> TLS_RSA_WITH_ARIA_256_GCM_SHA384

The parsing and IANA mapping is done by Tomcat, NOT OpenSSL. Now let's invoke OpenSSL:
> # openssl version
> OpenSSL 1.1.1t  7 Feb 2023
> # openssl ciphers -stdname 'HIGH:!aNULL:!eNULL:!EXPORT:!DES:!RC4:!3DES:!MD5:!PSK:!DSS:!SHA1:!SHA256:!SHA384' | cut -d ' ' -f 1 | sort
> TLS_AES_128_GCM_SHA256
> TLS_AES_256_GCM_SHA384
> TLS_CHACHA20_POLY1305_SHA256
> TLS_DHE_RSA_WITH_AES_128_CCM
> TLS_DHE_RSA_WITH_AES_128_CCM_8
> TLS_DHE_RSA_WITH_AES_128_GCM_SHA256
> TLS_DHE_RSA_WITH_AES_256_CCM
> TLS_DHE_RSA_WITH_AES_256_CCM_8
> TLS_DHE_RSA_WITH_AES_256_GCM_SHA384
> TLS_DHE_RSA_WITH_ARIA_128_GCM_SHA256
> TLS_DHE_RSA_WITH_ARIA_256_GCM_SHA384
> TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256
> TLS_ECDHE_ECDSA_WITH_AES_128_CCM
> TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8
> TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
> TLS_ECDHE_ECDSA_WITH_AES_256_CCM
> TLS_ECDHE_ECDSA_WITH_AES_256_CCM_8
> TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
> TLS_ECDHE_ECDSA_WITH_ARIA_128_GCM_SHA256
> TLS_ECDHE_ECDSA_WITH_ARIA_256_GCM_SHA384
> TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
> TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
> TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
> TLS_ECDHE_RSA_WITH_ARIA_128_GCM_SHA256
> TLS_ECDHE_RSA_WITH_ARIA_256_GCM_SHA384
> TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
> TLS_RSA_WITH_AES_128_CCM
> TLS_RSA_WITH_AES_128_CCM_8
> TLS_RSA_WITH_AES_128_GCM_SHA256
> TLS_RSA_WITH_AES_256_CCM
> TLS_RSA_WITH_AES_256_CCM_8
> TLS_RSA_WITH_AES_256_GCM_SHA384
> TLS_RSA_WITH_ARIA_128_GCM_SHA256
> TLS_RSA_WITH_ARIA_256_GCM_SHA384
> $ openssl version
> OpenSSL 1.1.1w-freebsd  11 Sep 2023
> $ openssl ciphers -stdname 'HIGH:!aNULL:!eNULL:!EXPORT:!DES:!RC4:!3DES:!MD5:!PSK:!DSS:!SHA1:!SHA256:!SHA384' | cut -d ' ' -f 1 | sort
> TLS_AES_128_GCM_SHA256
> TLS_AES_256_GCM_SHA384
> TLS_CHACHA20_POLY1305_SHA256
> TLS_DHE_RSA_WITH_AES_128_CCM
> TLS_DHE_RSA_WITH_AES_128_CCM_8
> TLS_DHE_RSA_WITH_AES_128_GCM_SHA256
> TLS_DHE_RSA_WITH_AES_256_CCM
> TLS_DHE_RSA_WITH_AES_256_CCM_8
> TLS_DHE_RSA_WITH_AES_256_GCM_SHA384
> TLS_DHE_RSA_WITH_ARIA_128_GCM_SHA256
> TLS_DHE_RSA_WITH_ARIA_256_GCM_SHA384
> TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256
> TLS_ECDHE_ECDSA_WITH_AES_128_CCM
> TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8
> TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
> TLS_ECDHE_ECDSA_WITH_AES_256_CCM
> TLS_ECDHE_ECDSA_WITH_AES_256_CCM_8
> TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
> TLS_ECDHE_ECDSA_WITH_ARIA_128_GCM_SHA256
> TLS_ECDHE_ECDSA_WITH_ARIA_256_GCM_SHA384
> TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
> TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
> TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
> TLS_ECDHE_RSA_WITH_ARIA_128_GCM_SHA256
> TLS_ECDHE_RSA_WITH_ARIA_256_GCM_SHA384
> TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
> TLS_RSA_WITH_AES_128_CCM
> TLS_RSA_WITH_AES_128_CCM_8
> TLS_RSA_WITH_AES_128_GCM_SHA256
> TLS_RSA_WITH_AES_256_CCM
> TLS_RSA_WITH_AES_256_CCM_8
> TLS_RSA_WITH_AES_256_GCM_SHA384
> TLS_RSA_WITH_ARIA_128_GCM_SHA256
> TLS_RSA_WITH_ARIA_256_GCM_SHA384
> $ openssl version
> OpenSSL 3.0.11 19 Sep 2023 (Library: OpenSSL 3.0.11 19 Sep 2023)
> $ openssl ciphers -stdname 'HIGH:!aNULL:!eNULL:!EXPORT:!DES:!RC4:!3DES:!MD5:!PSK:!DSS:!SHA1:!SHA256:!SHA384' | cut -d ' ' -f 1 | sort
> TLS_AES_128_GCM_SHA256
> TLS_AES_256_GCM_SHA384
> TLS_CHACHA20_POLY1305_SHA256
> TLS_DHE_RSA_WITH_AES_128_CCM
> TLS_DHE_RSA_WITH_AES_128_CCM_8
> TLS_DHE_RSA_WITH_AES_128_GCM_SHA256
> TLS_DHE_RSA_WITH_AES_256_CCM
> TLS_DHE_RSA_WITH_AES_256_CCM_8
> TLS_DHE_RSA_WITH_AES_256_GCM_SHA384
> TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256
> TLS_ECDHE_ECDSA_WITH_AES_128_CCM
> TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8
> TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
> TLS_ECDHE_ECDSA_WITH_AES_256_CCM
> TLS_ECDHE_ECDSA_WITH_AES_256_CCM_8
> TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
> TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
> TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
> TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
> TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
> TLS_RSA_WITH_AES_128_CCM
> TLS_RSA_WITH_AES_128_CCM_8
> TLS_RSA_WITH_AES_128_GCM_SHA256
> TLS_RSA_WITH_AES_256_CCM
> TLS_RSA_WITH_AES_256_CCM_8
> TLS_RSA_WITH_AES_256_GCM_SHA384

On Windows, compiled according to our instruction and patches:
> PS> .\openssl version
> OpenSSL 1.1.1w  11 Sep 2023
> PS> .\openssl ciphers -stdname  'HIGH:!aNULL:!eNULL:!EXPORT:!DES:!RC4:!3DES:!MD5:!PSK:!DSS:!SHA1:!SHA256:!SHA384' | foreach-object { $_.split(" ")[0]} | sort-object
> TLS_AES_128_GCM_SHA256
> TLS_AES_256_GCM_SHA384
> TLS_CHACHA20_POLY1305_SHA256
> TLS_DHE_RSA_WITH_AES_128_CCM
> TLS_DHE_RSA_WITH_AES_128_CCM_8
> TLS_DHE_RSA_WITH_AES_128_GCM_SHA256
> TLS_DHE_RSA_WITH_AES_256_CCM
> TLS_DHE_RSA_WITH_AES_256_CCM_8
> TLS_DHE_RSA_WITH_AES_256_GCM_SHA384
> TLS_DHE_RSA_WITH_ARIA_128_GCM_SHA256
> TLS_DHE_RSA_WITH_ARIA_256_GCM_SHA384
> TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256
> TLS_ECDHE_ECDSA_WITH_AES_128_CCM
> TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8
> TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
> TLS_ECDHE_ECDSA_WITH_AES_256_CCM
> TLS_ECDHE_ECDSA_WITH_AES_256_CCM_8
> TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
> TLS_ECDHE_ECDSA_WITH_ARIA_128_GCM_SHA256
> TLS_ECDHE_ECDSA_WITH_ARIA_256_GCM_SHA384
> TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
> TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
> TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
> TLS_ECDHE_RSA_WITH_ARIA_128_GCM_SHA256
> TLS_ECDHE_RSA_WITH_ARIA_256_GCM_SHA384
> TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
> TLS_RSA_WITH_AES_128_CCM
> TLS_RSA_WITH_AES_128_CCM_8
> TLS_RSA_WITH_AES_128_GCM_SHA256
> TLS_RSA_WITH_AES_256_CCM
> TLS_RSA_WITH_AES_256_CCM_8
> TLS_RSA_WITH_AES_256_GCM_SHA384
> TLS_RSA_WITH_ARIA_128_GCM_SHA256
> TLS_RSA_WITH_ARIA_256_GCM_SHA384
> PS> .\openssl version
> OpenSSL 3.0.11 19 Sep 2023 (Library: OpenSSL 3.0.11 19 Sep 2023)
> PS> .\openssl ciphers -stdname  'HIGH:!aNULL:!eNULL:!EXPORT:!DES:!RC4:!3DES:!MD5:!PSK:!DSS:!SHA1:!SHA256:!SHA384' | foreach-object { $_.split(" ")[0]} | sort-object
> TLS_AES_128_GCM_SHA256
> TLS_AES_256_GCM_SHA384
> TLS_CHACHA20_POLY1305_SHA256
> TLS_DHE_RSA_WITH_AES_128_CCM
> TLS_DHE_RSA_WITH_AES_128_CCM_8
> TLS_DHE_RSA_WITH_AES_128_GCM_SHA256
> TLS_DHE_RSA_WITH_AES_256_CCM
> TLS_DHE_RSA_WITH_AES_256_CCM_8
> TLS_DHE_RSA_WITH_AES_256_GCM_SHA384
> TLS_DHE_RSA_WITH_ARIA_128_GCM_SHA256
> TLS_DHE_RSA_WITH_ARIA_256_GCM_SHA384
> TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256
> TLS_ECDHE_ECDSA_WITH_AES_128_CCM
> TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8
> TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
> TLS_ECDHE_ECDSA_WITH_AES_256_CCM
> TLS_ECDHE_ECDSA_WITH_AES_256_CCM_8
> TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
> TLS_ECDHE_ECDSA_WITH_ARIA_128_GCM_SHA256
> TLS_ECDHE_ECDSA_WITH_ARIA_256_GCM_SHA384
> TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
> TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
> TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
> TLS_ECDHE_RSA_WITH_ARIA_128_GCM_SHA256
> TLS_ECDHE_RSA_WITH_ARIA_256_GCM_SHA384
> TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
> TLS_RSA_WITH_AES_128_CCM
> TLS_RSA_WITH_AES_128_CCM_8
> TLS_RSA_WITH_AES_128_GCM_SHA256
> TLS_RSA_WITH_AES_256_CCM
> TLS_RSA_WITH_AES_256_CCM_8
> TLS_RSA_WITH_AES_256_GCM_SHA384
> TLS_RSA_WITH_ARIA_128_GCM_SHA256
> TLS_RSA_WITH_ARIA_256_GCM_SHA384

OpenSSL gives us less ciphers than the Tomcat parser. So whetever I have requested from OpenSSL and verified on the command line is not what is passed to OpenSSL. This is confusing and not documented (?). Especially because I have excluded DSS explicitly, but it is back with a warning.

----------

Ideally

(a) either the data is retrieved live from OpenSSL,
(b) or documentation and log message somehow depict that this might not always be correct.
Comment 1 Michael Osipov 2023-10-06 16:41:15 UTC
Looking at apps/ciphers.c we could the same from Java, almost identical to:
https://github.com/apache/tomcat/blob/eb884fff4670ca7294442d0a196fbc7d6ec576ad/java/org/apache/tomcat/util/net/openssl/OpenSSLEngine.java#L71-L99

I bet that this can be moved to a static method which can take in ciphers method arg instead of "ALL" literal.
Comment 2 Michael Osipov 2023-10-06 16:42:09 UTC
Here is the gist with the data: https://gist.github.com/michael-o/0b4a5c4a9362a5477c1dbca6c8ba28cb
Comment 3 Mark Thomas 2023-10-25 18:56:17 UTC
I think this is a documentation issue.

The intention was to:
- allow OpenSSL notation to be used with JSSE
- track ciphers and behaviour of latest OpenSSL development branch
- have consistent (as possible) behaviour between JSSE and OpenSSL for the same cipher definition

It does this by converting the notation to a list of ciphers and then passing that to JSSE or OpenSSL.

That behaviour changes if you use a different version of OpenSSL is something that I think is good to highlight.

We could better document this by:
- adding most of the above (not necessarily exactly in that form) to the docs for ciphers
- amend the log message to note that this is expected if you run on older JDKs and/or older OpenSSL and reference the cipher docs

Thoughts?
Comment 4 Michael Osipov 2023-10-27 07:31:53 UTC
(In reply to Mark Thomas from comment #3)
> I think this is a documentation issue.
> 
> The intention was to:
> - allow OpenSSL notation to be used with JSSE
> - track ciphers and behaviour of latest OpenSSL development branch
> - have consistent (as possible) behaviour between JSSE and OpenSSL for the
> same cipher definition

This makes sense and I totally understand that.

> It does this by converting the notation to a list of ciphers and then
> passing that to JSSE or OpenSSL.
> 
> That behaviour changes if you use a different version of OpenSSL is
> something that I think is good to highlight.
> 
> We could better document this by:
> - adding most of the above (not necessarily exactly in that form) to the
> docs for ciphers
> - amend the log message to note that this is expected if you run on older
> JDKs and/or older OpenSSL and reference the cipher docs

I think we can do better by supplying "ciphers" to an SSL handle instead of decrypting it on our own and then match. This would reduce the false positive. If SunJSSE/OpenJSSE is used then this remain the same. The mismatch for me does not happen because it matches the latest OpenSSL dev branch, but the comparison is not faire because sources are incorrect.
Though, improving docs for people to better understand the warning is always good.
Comment 5 Mark Thomas 2023-11-01 15:33:14 UTC
If we pass ciphers directly to OpenSSL then we get different behaviour between JSSE and OpenSSL. This is the issue the current design is trying to avoid - or at least highlight when it happens.

What you are seeing is intended behaviour.

I remain of the view that better docs and a better log message is the way to address this.
Comment 6 Michael Osipov 2023-11-01 16:32:19 UTC
(In reply to Mark Thomas from comment #5)
> If we pass ciphers directly to OpenSSL then we get different behaviour
> between JSSE and OpenSSL. This is the issue the current design is trying to
> avoid - or at least highlight when it happens.

But we both know that JSSE and OpenSSL are different no matter what we do. Why don't we put that into consider and be fair about that fact.

> What you are seeing is intended behaviour.
> 
> I remain of the view that better docs and a better log message is the way to
> address this.

I agree with you that behavior should be consistent wherever possible, but documentation has to mention where it differs for obvious reasons.

Let's work on docs first.
Comment 7 Mark Thomas 2023-11-01 20:21:45 UTC
Fixed in:
- 11.0.x for 11.0.0-M14 onwards
- 10.1.x for 10.1.16 onwards
-  9.0.x for  9.0.83 onwards
-  8.5.x for  8.5.96 onwards

The description of ciphers and the log message should be a lot clearer now.

I couldn't reproduce the DSS issue so there may be another issue here that needs a new bug report.

Generally, I'd expect to see two types of bugs with this code.

1. Interpretation errors. These take the form of a test added to TestOpenSSLCipherConfigurationParser with a valid ciphers string that fails when run with the latest OpenSSL dev buld.

2. Test errors. Any test in TestOpenSSLCipherConfigurationParser that passes with the latest OpenSSL dev build but fails with the latest build of one or more older branches.This usually means we haven;t taken full account of a change between OpenSSL versions in the tests.
Comment 8 Michael Osipov 2023-11-03 07:56:24 UTC
(In reply to Mark Thomas from comment #7)
> Fixed in:
> - 11.0.x for 11.0.0-M14 onwards
> - 10.1.x for 10.1.16 onwards
> -  9.0.x for  9.0.83 onwards
> -  8.5.x for  8.5.96 onwards
> 
> The description of ciphers and the log message should be a lot clearer now.

Just tested. From a documentation PoV, this is fine now, though I wonder how many people will run OpenSSL from main instead of the LTS branch.

> I couldn't reproduce the DSS issue so there may be another issue here that
> needs a new bug report.
> 
> Generally, I'd expect to see two types of bugs with this code.
> 
> 1. Interpretation errors. These take the form of a test added to
> TestOpenSSLCipherConfigurationParser with a valid ciphers string that fails
> when run with the latest OpenSSL dev buld.
> 
> 2. Test errors. Any test in TestOpenSSLCipherConfigurationParser that passes
> with the latest OpenSSL dev build but fails with the latest build of one or
> more older branches.This usually means we haven;t taken full account of a
> change between OpenSSL versions in the tests.

I will take a look at these with my setup the next couple of weeks.

Thank your for your attention here.
Comment 9 Mark Thomas 2023-11-03 08:30:17 UTC
(In reply to Michael Osipov from comment #8)

> Just tested. From a documentation PoV, this is fine now, though I wonder how
> many people will run OpenSSL from main instead of the LTS branch.

I suspect very few, if any. I did consider aligning with the most recent LTS release but concluded it was better to align with main as that reflects the latest thinking regarding how secure a cipher or family of ciphers is. In reality, the differences have been minimal in the last few years. If they become problematic, we can always review which branch we track.
Comment 10 Markus Schlegel 2023-11-28 08:23:59 UTC
We are also facing this strange log entry since we upgraded Tomcat recently.
I have read through this issue's description and comments, but the changed text in 8.5.96 alone does not help in my opinion. I really required to debug and read through the respective code sections in order to get an understanding of this log statement. 
Now I understand the reasoning behind it, but I still have a problem with that. Let me explain why.
We are configuring our (embedded) Tomcat's SSL since years with the following code:

...
Connector sslConnector = new Connector("org.apache.coyote.http11.Http11Nio2Protocol");
sslConnector.setPort(sslPort);
sslConnector.setSecure(true);
sslConnector.setScheme("https");
sslConnector.setProperty("ciphers", "HIGH:!aNULL:!eNULL:!EXPORT:!DES:!RC4:!MD5:!kRSA:-DH:+ECDH");
sslConnector.setProperty("sslEnabledProtocols", "TLSv1.2");
sslConnector.setProperty("useServerCipherSuiteOrder", "true");
...

We explicitly set the ciphers configuration since the default config which comes with Tomcat still includes the (normal) Diffie-Helman ciphers which are considered to be insecure (but not the ECDH's!). 
There is still nothing wrong with that config as far as I could understand.
Nevertheless, there is now a warning in the logfile which we CAN'T TURN OFF since we use our custom ciphers configuration, which leds "warnOnSkip" being set to true.
Those skipped ciphers are of no interest for us or our customers since they appear only because Tomcat - as of my understanding - uses the ciphers-set from OpenSSL to build the complete list of theoretically available ciphers. 

It would help us with explaining this to the customers if the log statement would be logged on level "debug" rather than as a "warning" or if we had a way to turn off logging it.
Comment 11 Michael Osipov 2023-11-28 08:39:35 UTC
(In reply to Markus Schlegel from comment #10)
> We are also facing this strange log entry since we upgraded Tomcat recently.
> I have read through this issue's description and comments, but the changed
> text in 8.5.96 alone does not help in my opinion. I really required to debug
> and read through the respective code sections in order to get an
> understanding of this log statement. 
> Now I understand the reasoning behind it, but I still have a problem with
> that. Let me explain why.
> We are configuring our (embedded) Tomcat's SSL since years with the
> following code:
> 
> ...
> Connector sslConnector = new
> Connector("org.apache.coyote.http11.Http11Nio2Protocol");
> sslConnector.setPort(sslPort);
> sslConnector.setSecure(true);
> sslConnector.setScheme("https");
> sslConnector.setProperty("ciphers",
> "HIGH:!aNULL:!eNULL:!EXPORT:!DES:!RC4:!MD5:!kRSA:-DH:+ECDH");
> sslConnector.setProperty("sslEnabledProtocols", "TLSv1.2");
> sslConnector.setProperty("useServerCipherSuiteOrder", "true");
> ...
> 
> We explicitly set the ciphers configuration since the default config which
> comes with Tomcat still includes the (normal) Diffie-Helman ciphers which
> are considered to be insecure (but not the ECDH's!). 
> There is still nothing wrong with that config as far as I could understand.
> Nevertheless, there is now a warning in the logfile which we CAN'T TURN OFF
> since we use our custom ciphers configuration, which leds "warnOnSkip" being
> set to true.
> Those skipped ciphers are of no interest for us or our customers since they
> appear only because Tomcat - as of my understanding - uses the ciphers-set
> from OpenSSL to build the complete list of theoretically available ciphers. 
> 
> It would help us with explaining this to the customers if the log statement
> would be logged on level "debug" rather than as a "warning" or if we had a
> way to turn off logging it.

I have raised more or less the same concern and how it can be solved...
Comment 12 Mark Thomas 2023-11-28 09:27:47 UTC
@Markus - suggestions on improving the text of the docs and or the message welcome.

I don't think logging this at debug is an option. That the actual ciphers used change depending on which TLS implementation is used potentially has security implications so I think we have to visibly log something.

We can log any combination of:
- the requested configuration
- the list of ciphers the requested configuration maps to
- the list of ciphers actually used
- the list of ciphers requested but not supported

If you want to silence the warning, then you can explicitly list the ciphers you want to use but that has its own drawbacks.

I haven't run the default Tomcat TLS configuration against the SSL Labs scanner for a while. I'll do that and see if adjustments are required.
Comment 13 Markus Schlegel 2023-11-28 13:27:26 UTC
> I haven't run the default Tomcat TLS configuration against the SSL Labs scanner
> for a while. I'll do that and see if adjustments are required.

SSL-Labs still gives rating "B" if DH ciphers are enabled.
For information about DH ciphers, see https://weakdh.org/
Comment 14 Mark Thomas 2023-11-28 17:07:03 UTC
Hmm. I think we need to move the ciphers part of this discussion to the users list.

With a recent version of OpenSSL, Tomcat's default returns 112 ciphers. Adding ":-DH" reduces that to 83 and adding ":-DH:+ECDH" makes no difference compared to just to ":-DH". Looking at what "DH" and "ECDH" return on their own, I think you might want to look at your cipher configuration.

I get an SSL Labs rating of "A" with Tomcat's default ciphers for Tomcat 11 + Java 21 (OpenSSl and JSSE), Tomcat 8 + Java 11 (OpenSSL and JSSE), Tomcat 8 + Java 8 (JSSE)

Looking at the results from SSL Labs, adding ":-CBC" looks like something worth considering.