Bug 64141

Summary: APR SSL: Required certificate verification uses -Djavax.net.ssl.trustStore instead of caCertificateFile
Product: Tomcat 8 Reporter: Martin Wegner <martin.wegner>
Component: ConnectorsAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED FIXED    
Severity: critical CC: martin.wegner
Priority: P2    
Version: 8.5.51   
Target Milestone: ----   
Hardware: PC   
OS: All   

Description Martin Wegner 2020-02-13 18:25:05 UTC
I have the followong server.xml configuration to enforce a client certificate verification:

>    <Connector port="27247" server="Apache" protocol="org.apache.coyote.http11.Http11AprProtocol"
>               connectionTimeout="20000"
>               compression="force"
>               maxThreads="400"
>               scheme="https" secure="true" SSLEnabled="true">
>      <SSLHostConfig caCertificateFile="D:\Program Files\Apache Software Foundation\Tomcat8_BackEnd_Test\cert\ca.pem"
>                     disableCompression ="true" disableSessionTickets="true"
>                     certificateVerification="require" protocols="TLSv1.2">
>        <Certificate certificateFile="D:\Program Files\Apache Software Foundation\Tomcat8_BackEnd_Test\cert\server.pem"
>                     certificateKeyFile="D:\Program Files\Apache Software Foundation\Tomcat8_BackEnd_Test\cert\server.key"
>                     certificateKeyPassword="mysecret" />
>      </SSLHostConfig>
>    </Connector>

The Tomcat Java options contain:

>-Djavax.net.ssl.trustStore=D:\Program Files\Apache Software Foundation\Tomcat8_BackEnd_Test\cert\truststore.jks
>-Djavax.net.ssl.trustStorePassword=mysecret
>-Djavax.net.ssl.trustStoreType=JKS
>-Djavax.net.ssl.keyStore=D:\Program Files\Apache Software Foundation\Tomcat8_BackEnd_Test\cert\client.pfx
>-Djavax.net.ssl.keyStorePassword=mysecret
>-Djavax.net.ssl.keyStoreType=PKCS12

Suppose the ca.pem contains a CA named A, so I would expect that the client certificate chain must be approved by A.
But here all client certificate chains are accepted which are covered by the truststore.jks in the Tomcat Java options.

In my opinion only the caCertificateFile from the server.xml should be used as the trust anchor.

I have not tested if caCertificateFile and -Djavax.net.ssl.trustStore are used together (mixed) or if -Djavax.net.ssl.trustStore simply overwrites caCertificateFile.
Comment 1 Christopher Schultz 2020-02-13 21:43:52 UTC
Are you using OpenSSL? The caCertificateFile configuration option is only applicable if you are using the OpenSSL crypto engine.

From your description, I believe you have an incorrect configuration. You want to set:

truststoreFile="D:\Program Files\Apache Software Foundation\Tomcat8_BackEnd_Test\cert\ca.pem" (but convert this file into a keystore)
Comment 2 Remy Maucherat 2020-02-13 22:02:26 UTC
The configuration looks ok to me in theory as APR will use caCertificateFile.

However, if you look at SSLHostConfig, you can notice that:
private String truststoreFile = System.getProperty("javax.net.ssl.trustStore");
So the system property sets truststoreFile, which is then used to get the trust managers (and caCertificateFile is then not used at all).

IMO: bad luck, this may be a WONTFIX.

The workaround mentioned by Chris by creating a keystore is correct, since setting truststoreFile will override the value from the system property.
Comment 3 Martin Wegner 2020-02-14 11:51:57 UTC
(In reply to Christopher Schultz from comment #1)
> Are you using OpenSSL? The caCertificateFile configuration option is only
> applicable if you are using the OpenSSL crypto engine.
> 
> From your description, I believe you have an incorrect configuration. You
> want to set:
> 
> truststoreFile="D:\Program Files\Apache Software
> Foundation\Tomcat8_BackEnd_Test\cert\ca.pem" (but convert this file into a
> keystore)

I use the Windows binaries, so it is Tomcat Native APR and OpenSSL.
This is the reason why I only use OpenSSL configuration parameters and no JSSE configuration parameters. truststoreFile is a JSSE only configuration parameter (https://tomcat.apache.org/tomcat-8.5-doc/config/http.html).
Comment 4 Martin Wegner 2020-02-14 13:11:23 UTC
(In reply to Remy Maucherat from comment #2)
> The configuration looks ok to me in theory as APR will use caCertificateFile.
> 
> However, if you look at SSLHostConfig, you can notice that:
> private String truststoreFile =
> System.getProperty("javax.net.ssl.trustStore");
> So the system property sets truststoreFile, which is then used to get the
> trust managers (and caCertificateFile is then not used at all).
> 
> IMO: bad luck, this may be a WONTFIX.
> 
> The workaround mentioned by Chris by creating a keystore is correct, since
> setting truststoreFile will override the value from the system property.

I checked the source code and you are right. When the JSSE only parameters truststoreFile and truststorePassword are set, then the caCertificateFile would be ignored and the provided truststore would be used. If this is intended, then the documentation should be updated.
Comment 5 Remy Maucherat 2020-02-14 13:24:17 UTC
(In reply to Martin Wegner from comment #4)
> I checked the source code and you are right. When the JSSE only parameters
> truststoreFile and truststorePassword are set, then the caCertificateFile
> would be ignored and the provided truststore would be used. If this is
> intended, then the documentation should be updated.

This is by accident right now, so we need to decide if this is labelled as "intended" [I hope you don't have a problem with retcons if your shows, because we do it here as well :) ] or if it should be fixed. The two attributes caCertificateFile and truststoreFile could reset the other to null (if it not null) and WARN about it since they are mutually exclusive.
Comment 6 Remy Maucherat 2020-02-15 12:32:39 UTC
Since the TLS configuration types for the properties are not the same, it seemed possible to fix it. The fix will be in 10.0.0-M2, 9.0.32 and 8.5.52.
Comment 7 Martin Wegner 2020-02-17 10:06:20 UTC
(In reply to Remy Maucherat from comment #6)
> Since the TLS configuration types for the properties are not the same, it
> seemed possible to fix it. The fix will be in 10.0.0-M2, 9.0.32 and 8.5.52.

Thank you, looks good to me. I will test it after the release of 8.5.52.