Added cleaner SSL support to HTTPServer

[dhoard]

Added cleaner SSL support to HTTPServer.

This will allow downstream projects to set up an SSLContext and use it with the HTTPServer.Builder via the withSSLContext(SSLContext sslContext) method to support SSL.

Added public static helper method HTTPServer.createSSLContext(String sslContextType, String keyStoreType, String keyStorePath, String keyStorePassword) to create an SSLContext for the most common use-cases.

[fstab]

Hi, thanks a lot for the PR, and sorry for the delay. I have this on the top of my TODO list, and I am planning to get back to it this week.

[fstab]

Here are a few initial thoughts: First of all, thank you very much for digging into this, SSL is really an ugly part of Java's API.

I think it's worthwhile to step back a bit and look at how SSL should be configured from a user's perspective, for example in jmx_exporter.

Compatibility with node_exporter

My first thought was, let's make SSL configuration similar to node_exporter, so that user's don't need to learn different ways how to configure our exporters. In node_exporter you have a configuration like this:

tls_server_config:
  cert_file: node_exporter.crt
  key_file: node_exporter.key

It would be great if there was an easy way to use these crt and key files in the Java configuration as well. However, I did not find an obvious way how to do this.

I will research this a bit more, because it would be great if we could provide a consistent way of configuring SSL across different exporters.

Compatibility with Java's default behavior

If we cannot follow the node_exporter's convention, it would be good to follow Java's conventions. My understanding is that by default you can configure Java's SSL with the following system properties:

-Djavax.net.ssl.keyStore=/path/to/my/keystore.jks
-Djavax.net.ssl.keyStorePassword=my_secret_password

It turns out, if you use the default HttpsConfigurator and the default SSLContext this seems to work. Replace line 359 with

((HttpsServer) httpServer).setHttpsConfigurator(new HttpsConfigurator(SSLContext.getDefault()));

Then create a key store with

keytool -genkeypair -keyalg RSA -alias selfsigned -keystore keystore.jks -storepass my_secret_password -dname "CN=localhost, OU=Developers, O=NA, L=Ufa, C=RB"

Then start the Server with -Djavax.net.ssl.keyStore=/path/to/my/keystore.jks -Djavax.net.ssl.keyStorePassword=my_secret_password.

As this works I'm wondering: Do we need the custom SSLContext and HttpsConfigurator? I don't have an opinion yet, I'm just trying to understand the implications.

[dhoard]

I'll do some more research as well on the use of a .crt file. It maybe possible to dynamically build the keystore in memory.

Regarding the use of system properties, I feel...

• it's insecure. Anyone on the machine (non-root) can see the process arguments.

• it may force the code using client_java and the application to use the same certificate.

I can see scenarios where there are requirements (InfoSec) to use different certificates.

Example -

Given a machine with two hostnames/IP addresses, I want the application to use hostname 1/IP address 1/certificate 1 and the jmx_exporter to use hostname 2/IP address 2/certificate 2.

[fstab]

Hi @dhoard, thanks for the update.

Regarding security: I don't think that system properties are necessarily insecure, because if you want to avoid command line parameters you can set them programmatically as well with System.setProperty().

However, it's true that if SSL is configured via system properties, the configuration will apply to the rest of the application as well. Maybe we should offer both ways, a simple withSSL() that uses the default system properties, and an advanced withHttpsConfigurator(HttpsConfigurator) for custom configuration.

I'm wondering how much of createHttpsConfigurator() and createSSLContext() should actually be part of client_java. I tried to create an HttpsConfigurator without overriding configure() linke this:

new HttpsConfigurator(sslContext);

It works (at least the test is green when I do this), so I'm wondering if overriding configure() is really needed.

Regarding the createSSLContext() method, I find it hard to decide which settings should be configurable via parameters, and which settings should be hard-coded. For example, it is likely that sslContextType is always SSL for most users, however, it is configurable. On the other hand the algorithm name might not be SunX509 for some users (I found a tutorial with IbmISeriesX509), however it is hard-coded.

I'm not sure whether there is a good solution for an SSL configuration API. I'm wondering if we should remove createHttpsConfigurator() and createSSLContext(), and just provide a withHttpsConfigurator(HttpsConfigurator) method, and let the users create the HttpsConfigurator themselves. This would avoid mixing SSL configuration with the client_java API (I guess there are many special cases where whatever API we provide does not fit), and it would give users full flexibility.

What do you think?

[dhoard]

@fstab all good points.

I think we should add the withHttpsConfigurator() method and remove the withSSLContext() method from the HTTPServer.Builder() class.

Then, I think we should remove the createSSLContext() method from the HTTPServer.Builder() class and introduce a new class KeyStoreSSLContext to encapsulate the logic to use a keystore.

This would allow creating a PEMCertificateSSLContext class so that a client_java user could use either a keystore or PEM certificate.

[fstab]

The question is how much value our own KeyStoreSSLContext would provide over the built-in SSLContext. If there are no good reason for introducing our own abstraction we could just work with SSLContext directly.

[dhoard]

The current value is encapsulating all the boilerplate code that every developer would duplicate if they want to use SSL with a keystore.

Once we can determine how to use a PEM certificate(s) it will allow that encapsulation as well.

[fstab]

There are some libraries out there for creating SSLContext, like https://github.com/Hakky54/sslcontext-kickstart.

I fear if we make our API sufficiently flexible we will end up writing our own library like this, which would be out of scope for client_java and probably not as good as a dedicated library. So maybe it would be cleaner to just rely on the built-in Java classes in client_java and let users use something like https://github.com/Hakky54/sslcontext-kickstart if they want a dedicated API.

[dhoard]

@fstab I understand your point of view. To me, SSL support seems like a core functionality when using HTTPServer.

There are really two thoughts/approaches...

1. provide a hook in HTTPServer (client_java) and implement a basic implementation.

2. provide a hook for HTTPServer (client_java) and defer to the downstream consumer of client_java to add the functionality.

We need to reconcile the approach with jmx_exporter ( prometheus/jmx_exporter#442 ) and other client_java consumers such as the Cassandra exporter ( prometheus/jmx_exporter#630 )

[fstab]

I feel we should stick with the built-in HttpsConfigurator for now and leave the rest to the user. If we implement our own API, we will either end up with something that's too simplistic for many users, or with something that's as complicated as HttpsConfigurator itself. If it turns out that many users configure HttpsConfigurator in the same way, we can still add an API later based on how users are actually using it.

[dhoard]

@fstab fair enough. I'll try to make the changes this week.

[dhoard]

@fstab changes made per our converstation. Please review.

[dhoard]

@fstab Thoughts on merging this PR?

[fstab]

Thanks a lot and sorry for the delay. It's looking good, I just commented two nitpicks.
Please also rebase it to the current master.
I'm happy to merge and release it so we can use it in jmx_exporter.

Thanks you for your patience!

[fstab]

Thanks a lot! Would you mind rebasing the branch to the current master? Automatic rebase fails because of merge conflicts. This branch has 10 commits, and I think for later reference it would be easier to squash them into one and rebase them on the current master.

BTW I just published a release so that people don't have the vulnerable log4j dependency in their dependency anymore. However, that does not mean that we need to wait for long until we create a new release with the SSL support. I'd say as soon as we want to merge code in jmx_exporter that needs the SSL support, we create a new client_java release and use it in jmx_exporter.

[fstab]

Thanks a lot!

[fstab]

It's released with 0.14.0 🎉.