tomcat

tomcat copied to clipboard

This PR is a followup of the following earlier PR https://github.com/apache/tomcat/pull/673 Although that pull request didn't get merged, the code changes has been comitted to the main branch by the main developer, see here for the specific commit: https://github.com/apache/tomcat/commit/b0df9819c8d130adab0490b89dce1ab4ca6a3448

Context With the earlier commit it is now possible to programatically configure the ssl configuration of tomcat instead of using properties and delegating to tomcat to construct the ssl configuration. This opens the possibility of reloading the ssl configuration or other customizations as shown also here: sslcontext-kickstart

I want to provide an example with my own code to give a better understanding, the project is also available here: Instant SSL Reloading with Tomcat

SSLContext wrapper

import javax.net.ssl.KeyManager;
import javax.net.ssl.SSLEngine;
import javax.net.ssl.SSLParameters;
import javax.net.ssl.SSLServerSocketFactory;
import javax.net.ssl.SSLSessionContext;
import javax.net.ssl.TrustManager;
import javax.net.ssl.X509KeyManager;
import javax.net.ssl.X509TrustManager;
import java.security.KeyManagementException;
import java.security.SecureRandom;
import java.security.cert.X509Certificate;

public class SSLContextWrapper implements SSLContext {

    private final javax.net.ssl.SSLContext sslContext;
    private final X509KeyManager keyManager;
    private final X509TrustManager trustManager;

    public SSLContextWrapper(javax.net.ssl.SSLContext sslContext, X509KeyManager keyManager, X509TrustManager trustManager) {
        this.sslContext = sslContext;
        this.keyManager = keyManager;
        this.trustManager = trustManager;
    }

    @Override
    public void init(KeyManager[] kms, TrustManager[] tms, SecureRandom sr) throws KeyManagementException {
        // not needed to initialize as it is already initialized
    }

    @Override
    public void destroy() {

    }

    @Override
    public SSLSessionContext getServerSessionContext() {
        return sslContext.getServerSessionContext();
    }

    @Override
    public SSLEngine createSSLEngine() {
        return sslContext.createSSLEngine();
    }

    @Override
    public SSLServerSocketFactory getServerSocketFactory() {
        return sslContext.getServerSocketFactory();
    }

    @Override
    public SSLParameters getSupportedSSLParameters() {
        return sslContext.getSupportedSSLParameters();
    }

    @Override
    public X509Certificate[] getCertificateChain(String alias) {
        return keyManager.getCertificateChain(alias);
    }

    @Override
    public X509Certificate[] getAcceptedIssuers() {
        return trustManager.getAcceptedIssuers();
    }
}

import org.apache.catalina.connector.Connector;
import org.apache.coyote.http11.AbstractHttp11Protocol;
import org.apache.tomcat.util.net.SSLHostConfig;
import org.apache.tomcat.util.net.SSLHostConfigCertificate;
import org.apache.tomcat.util.net.SSLHostConfigCertificate.Type;
import org.apache.tomcat.util.net.SSLUtil;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.boot.web.embedded.tomcat.TomcatConnectorCustomizer;
import org.springframework.context.annotation.Configuration;

@Configuration
public class SSLConnectorCustomizer implements TomcatConnectorCustomizer {

    private final int port;

    public SSLConnectorCustomizer(@Value("${server.port}") int port) {
        this.port = port;
    }

    @Override
    public void customize(Connector connector) {
        connector.setScheme("https");
        connector.setSecure(true);
        connector.setPort(port);

        AbstractHttp11Protocol<?> protocol = (AbstractHttp11Protocol<?>) connector.getProtocolHandler();
        protocol.setSSLEnabled(true);

        SSLHostConfig sslHostConfig = new SSLHostConfig();
        SSLHostConfigCertificate certificate = new SSLHostConfigCertificate(sslHostConfig, Type.UNDEFINED);
        certificate.setSslContext(new TomcatSSLContext(sslContext, keyManager, trustManager));
        sslHostConfig.addCertificate(certificate);
        protocol.addSslHostConfig(sslHostConfig);
    }

}

Problem statement Boilerplate code is needed by the end-user to provide a custom ssl configuration. Tomcat takes a custom SSLContext, the full name is org.apache.tomcat.util.net.SSLContext while the end-user has javax.net.ssl.SSLContext. So the end-user is required to create an implementation of org.apache.tomcat.util.net.SSLContext which acts as a wrapper. This sslcontext needs to be passed to SSLHostConfigCertificate to further configure the server.

Solution Provide a helper class which acts as a wrapper to reduce the boilerplate code. The utility interface is able to provide a method to wrap the required objects, in this case javax.net.ssl.SSLContext, KeyManager, TrustManager in a org.apache.tomcat.util.net.SSLContext

What do you think, would it be a nice addition to the code changes we did previously? It will atleast make it more developer friendlier. Looking forward to your feedback.

Hi @markt-asf

What do you think of this PR, would it make sense to have this kind of wrapper, or does it needs to be adjusted or would you like me to close it and disregard it? Looking forward to get your feedback on this

Your previous PR, which was integrated, unfortunately caused a very high number of regressions that had to be fixed over multiple Tomcat releases. This PR similarly seems uninteresting to me, so as a result I will not integrate it.

Ah so your feeling is that this also might cause some regression while this wrapper does not add that much value to the project itself. I can understand that. Okay, thank you for your time for reviewing this PR. Let's close it then.

I had left the PR open since others could have been willing to go through with it (or not, I don't know).

Hi guys, any news regarding this topic?

This wrapper class appears to specifically discard the key managers and trust managers. The constructor captures its arguments and then ignores them forever after that. Why bother capturing them in the first place?

If the goal is to allow "instant" reloading of the SSL configuration... that capability already exists in Tomcat.

This wrapper class appears to specifically discard the key managers and trust managers.

I used my own example from my own project to demonstrate the usage of it within the PR description, however the code changes are slightly different. It has a constructor with a SSLContext, KeyManager and TrustManager.

The constructor captures its arguments and then ignores them forever after that. Why bother capturing them in the first place?

Well actually it is using the KeyManager and TrustManager, see here: https://github.com/apache/tomcat/pull/706/files#diff-8ed2a43a8b2f354b707c0fdb8cd5b794e5a476ecbf603b2ba69af5eea18b3cc4R73-R81 So it is using all of the objects from the constructor even the SSLContext itself. It just acts as a wrapper to simplifies the usage of a custom org.apache.tomcat.util.net.SSLContext

If the goal is to allow "instant" reloading of the SSL configuration... that capability already exists in Tomcat.

In my pull request description I mentioned the usage of ssl reloading, but this was an example to demonstrate how I used it with the resulting wrapper which I needed to add to provide a custom ssl configuration. So I wanted to point out there that the developer who want's to have a custom ssl configurationn always needs to create a wrapper on their side or else it won't work.

So the reloading of tomcat was just an example but I use it also for different use cases, such as:

• Combining custom truststore, cacert and System keystore as a TrustManager
• Fetching certificates as pem from a database and constructing the KeyManager and TrustManager
• Using a custom TrustManager which can prompt when the certificate is not trusted yet and whether it needs to be trusted, ss it can be added to the exusting list of trusted certificates
• Managing ssl sessions

It might be that I am the only developer which is working in this kind of edge cases... So all of these scenario's are working for me already actually while using the wrapper class, I thought it would be nice to move it to apache tomcat to simplify the usage of it.

The constructor captures its arguments and then ignores them forever after that. Why bother capturing them in the first place?

Well actually it is using the KeyManager and TrustManager, see here: https://github.com/apache/tomcat/pull/706/files#diff-8ed2a43a8b2f354b707c0fdb8cd5b794e5a476ecbf603b2ba69af5eea18b3cc4R73-R81

Oops, I seem to have totally missed that part of the code. My apologies.

So the reloading of tomcat was just an example but I use it also for different use cases, such as:

* Combining custom truststore, cacert and System keystore as a TrustManager
* Fetching certificates as pem from a database and constructing the KeyManager and TrustManager
* Using a custom TrustManager which can prompt when the certificate is not trusted yet and whether it needs to be trusted, ss it can be added to the exusting list of trusted certificates

Really, using any custom SSLContext for whatever reason is a valid use case. It's not reasonable for Tomcat to provide all of these various combinations of features, so extensibility is certainly useful.

I think the only question is whether this wrapper is really useful to ship with Tomcat. It's certainly not useful outside of Tomcat since it uses Tomcat's internal interface. But it does bridge the gap between Java-provided APIs and Tomcat's APIs.

The SSLHostConfigCertificate has the option to pass the tomcat SSLContext interface. Not quite sure whether that is part of the public api. Next to that the wrapper class is package protected, so no one can access it. It can only be created using the sslutil which returns the interface. I am not sure whether sslutil is the right place and whether the sslutil is part of the public api. What do you think?

After couple of review rounds it is not clear whether it would get approved/merged, so I decided to close this PR. Please ping me if you guys still consider it and and I will create a new PR.