Bug 59344

Created attachment 33788 [details]
PEM support implementation

Here is a patch implementing this feature:
- SSLHostConfigCertificate is modified to accept certificateFile, certificateChainFile and certificateKeyFile with JSSE
- The HTTP connector documentation is updated accordingly
- A new package private class org.apache.tomcat.util.net.jsse.PEMFile is added to handle the PEM file parsing and decoding. It supports PKCS#8 private keys only.
- JSSEUtil.getKeyManagers() is modified to create an in-memory keystore initialized with the PEM files when the certificateFile is specified.
- TesterSupport is modified to make it possible to initialize SSL with PEM files even when the APR connector isn't used.
- TestSsl is extended to test SSL with plain text and encrypted keys. It is missing a test with a certificate chain file to cover all the cases (the test certificate being self signed it has no intermediary CA).

Oh ok, that was unexpected. I'm a bit sceptical about the usefulness since JSSE is mostly useless in 9 (no ALPN), but if it works, why not.

About the documentation, more changes are needed and it shouldn't be changed this way. I would like to keep the two configuration types clearly separate, so a host configuration should use either OpenSSL or JSSE configuration types, but not a mix of both.

Note: you apparently missed the possibility to use JSSE with OpenSSL, which is basically the recommended setup now. It supports both JSSE and OpenSSL configuration types.

Created attachment 33789 [details]
PEM support implementation

Updated TestSsl to match the changes made in r1740324.

Thank you for the review Remy. My use case is mostly for HTTP/1.1, I agree this feature is less interesting for HTTP/2, until Java 9 is available and supports ALPN I guess.

The patch supports a configuration like this one:

  <Connector port="8443" protocol="org.apache.coyote.http11.Http11NioProtocol"
             SSLEnabled="true">
    <SSLHostConfig>
      <Certificate certificateKeyFile="conf/privkey-encrypted.pem"
                   certificateKeyPassword="secret"
                   certificateFile="conf/cert.pem"
                   certificateChainFile="conf/chain.pem"/>
    </SSLHostConfig>
  </Connector>

That's similar to the parameters accepted for the HTTP/2 connector. Are you suggesting a different set of parameters for this case?

Unless you have a tomcat-native phobia, I think you should just install it. Then Tomcat will use OpenSSL and you would be better off than with JSSE.

I'm quite sure you didn't try it since your testsuite changes break that.

Indeed, I've never had the need for tomcat-native so far. I'm going to check the regression with OpenSSL.

It's only a testsuite issue, so I'll look it up eventually. Otherwise the patch interacts properly with the OpenSSL support.

To run the tests with OpenSSL, you can add this in build.properties:
test.sslImplementation=org.apache.tomcat.util.net.openssl.OpenSSLImplementation

On Windows 7 I copied openssl.exe in the base directory and tcnative-1.dll into bin/native and then ran:

   ant test-apr -Dtest.openssl.path=openssl.exe -Dtest.name=**/*Ssl*

and the tests passed.

With:

   ant test-nio -Dtest.openssl.path=openssl.exe -Dtest.sslImplementation=org.apache.tomcat.util.net.openssl.OpenSSLIm
plementation -Dtest.name=**/*Ssl*

I get a SSLException "Error initializing SSL context" caused by "Invalid Server SSL Protocol (error:140A90F1:SSL routines:SSL_CTX_new:unable to load ssl2 md5 routines)". Is this the error you encountered?

Yes, that's the test error.

Created attachment 33792 [details]
PEM support implementation

TesterSupport.initSslWithPEM() now sets the sslImplementationName connector attribute when the test.sslImplementation property is set. This fixes the exception with the testSimpleSslWith*PEM() tests.

This does create a side effect on the configuration checking though, which becomes rather messy. Basically, all connectors can now use the OpenSSL style, but they cannot mix and match for the certificate definition.

So I now agree that the OpenSSL type should be removed from CertificateKeyFile, CertificateFile and CertificateChainFile, but I'll add an extra check to cause an error if for example both CertificateKeystorePassword and CertificateChainFile are defined.

I have committed r1740969, with some additions to avoid configuration errors.

I have not committed the new tests yet since the mixing with the OpenSSL flag still looks a bit suspicious. Also the documentation is not updated, so the BZ remains open.

The docs doesn't look too bad.
The feature will be in 9M5 and 8.5.1, but you really should look into using OpenSSL if you want to do something useful with these new Tomcats.

Thank you Remy, that's perfect.