Bug 62032 - NPE caused by Connector and SSLHostConfig
Summary: NPE caused by Connector and SSLHostConfig
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 9
Classification: Unclassified
Component: Util (show other bugs)
Version: 9.0.2
Hardware: PC Linux
: P2 normal (vote)
Target Milestone: -----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-01-22 20:33 UTC by Coty Sutherland
Modified: 2018-01-25 02:47 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Coty Sutherland 2018-01-22 20:33:10 UTC
This behavior was noted on a users list thread, but I think a BZ is in order to make a more useful message for users.

Here is the Connector and SSLHostConfig that causes the problem:

~~~
    <Connector port="8443" protocol="org.apache.coyote.http11.Http11NioProtocol"
               maxThreads="150" SSLEnabled="true" sslProtocol="TLS">
        <SSLHostConfig hostName="test">
            <Certificate certificateKeystoreFile="conf/localhost-rsa.jks"
                         type="RSA" />
        </SSLHostConfig>
    </Connector>
~~~

The issue is that we're using sslProtocol on the Connector (which creates a _default_ SSLHostConfig object; this could be any SSLHostConfig attribute) AND defining an SSLHostConfig (with a name other than _default_ so they don't clash). This causes the _default_ SSLHostConfig object to be created which then tries to use a non-existent default PEM file and throws the NPE. The user knows nothing of this and is confused because they provided a certificateKeystoreFile in the configuration above, not a PEM file.

We need to catch and wrap this NPE in something more informative.
Comment 1 Remy Maucherat 2018-01-23 13:12:57 UTC
Should we catch this sort of configuration as an error even in cases where it would kind of work ? Like if there are SSL config bits on the Connector element *and* one or more SSLHostConfig elements, then fail the connector ?
The user will then have to choose between the deprecated single SSL config or the new one, and there's a lower error risk.
Comment 2 Coty Sutherland 2018-01-23 13:53:44 UTC
(In reply to Remy Maucherat from comment #1)
> Should we catch this sort of configuration as an error even in cases where
> it would kind of work ? Like if there are SSL config bits on the Connector
> element *and* one or more SSLHostConfig elements, then fail the connector ?

If I understand you correctly, that does happen now in cases that they're both named _default_. I think the problem is that we're defaulting to an empty PEM file with the auto-generated default SSLHostConfig instead of throwing an error. The logic in JSSEUtil is interesting, we're checking for a keystore and if it's null we default to a PEMFile without verifying it exists. Is there some reason for doing that instead of throwing an exception? I don't think an SSL Connector with no keystore or key/cert pair is usable :)
Comment 3 Mark Thomas 2018-01-24 09:57:36 UTC
No good reason I can recall.

We should check that the behaviour is consistent for:
- no keystore or PEM file specified
- keystore specified but file does not exist
- PEM file specified but file doe snot exist
Comment 4 Coty Sutherland 2018-01-24 13:53:40 UTC
(In reply to Mark Thomas from comment #3)
> We should check that the behaviour is consistent for:
> - no keystore or PEM file specified

This behaves the same as the bugzilla description (throws an NPE).

> - keystore specified but file does not exist

SSLUtilBase.getStore() throws a FNFE with a nice message and stack trace which is logged again by StandardService.initInternal() when the Connector fails to init. I think we should remove the stack trace from SSLUtilBase.getStore()'s log message (or maybe make it debug?) and keep the message so that the stack is only printed once for the exception here.

> - PEM file specified but file does not exist

This one behaves really weirdly. Two (duplicate) warning messages are logged at the same time from SSLHostConfig.adjustRelativePath() stating that the PEM file does not exist. Then another warning from OpenSSLContext.init() saying that it can't init the SSL context because there's no such file...but, the Connector init doesn't fail and it binds as usual (albeit unusable). When you try and access the Connector over https it gets rejected and the following is logged:

SEVERE [https-openssl-nio-8443-exec-1] org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun 
 java.lang.Exception: cannot create new ssl
    at org.apache.tomcat.jni.SSL.newSSL(Native Method)
    at org.apache.tomcat.util.net.openssl.OpenSSLEngine.<init>(OpenSSLEngine.java:213)
    at org.apache.tomcat.util.net.openssl.OpenSSLContext.createSSLEngine(OpenSSLContext.java:514)
    at org.apache.tomcat.util.net.AbstractJsseEndpoint.createSSLEngine(AbstractJsseEndpoint.java:162)
    ....

If you try the Connector over http, you'll get and 400 status response with a "Bad Request" body.

I think all of these scenarios should be consistent experiences and behave like scenario two above (throw a FNFE), with the amendment of only dumping the stack once per exception.
Comment 5 Coty Sutherland 2018-01-24 19:56:24 UTC
I caught the NPE and reported it as a configuration error (missing certificateFile attribute on the SSLHostConfig) and caught/handled the FileNotFoundException for the other two scenarios above in a way that is consistent across file types (I threw the exception up to the Connector creation to be handled). There was some message/stack trace duplication that I removed in there too, which reduces unnecessary logging and creates a better user experience.

Fixed in:
- trunk for 9.0.5 onwards
- 8.5.x for 8.5.28 onwards
Comment 6 Christopher Schultz 2018-01-24 21:33:29 UTC
Catching NPE/FNFE is pretty ugly. Is there a better way to do it? I'd prefer to pro-actively validate the configuration and tell the user something is wrong than catch these exceptions.
Comment 7 Coty Sutherland 2018-01-25 02:47:07 UTC
Actually using "caught" was the wrong word. I prevented the NPE by checking the certificate a bit earlier in the stack and threw a FNFE (where a warn was previously logged) up to the Connector init. Take a look at the revision (r1822150) and let me know if you see a better way to handle those.