Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SSL Configuration for Netty, Jetty, and Embedded #2494

Merged
merged 2 commits into from
Jun 9, 2022

Conversation

devinrsmith
Copy link
Member

The minimum-viable configuration for enabling SSL.

Can run a server w/ self-signed certs:

./gradlew server-jetty-app:run -Pgroovy -PdevCerts

or

./gradlew server-netty-app:run -Pgroovy -PdevCerts

Can verify the the server via:

./java-client/session-examples/build/install/java-client-session-examples/bin/connect-check --target dh://localhost:8443 --ssl server/dev-certs/client.json

SSL configuration can be applied manually with the properties "ssl.identity.type", "ssl.identity.certChainPath", and
"ssl.identity.privateKeyPath".

builder.maxInboundMessageSize(maxInboundMessageSize);
}
if (httpWebsockets != null) {
builder.websockets(Boolean.parseBoolean(httpWebsockets));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that Boolean.parseBoolean will correctly return false if you pass it a null, no need to guard with a null check.

Copy link
Member Author

@devinrsmith devinrsmith Jun 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The defaults are defined by the server object - in this example, websockets defaults to true, so Boolean.parseBoolean(null) would be the wrong behavior.

server/jetty-app/build.gradle Outdated Show resolved Hide resolved
server/jetty/build.gradle Outdated Show resolved Hide resolved
int DEFAULT_MAX_INBOUND_MESSAGE_SIZE_MiB = 100;

/**
* The host. Defaults to {@value DEFAULT_HOST}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this suggested to be the host ip (possibly more accurately the "bind address", or interface to bind to)? or are there reasons you would want a dns name (i.e. http host) to be passed here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming was chosen based on how jetty chooses to call it:

    @ManagedAttribute("The network interface this connector binds to as an IP address or a hostname.  If null or 0.0.0.0, then bind to all interfaces.")
    public String getHost()
    {
        return _host;
    }

Specifically, I think the "using a hostname" is very important for certain types of deployments.

I'll update the javadocs - and also, I think it makes sense to have host as optional with the behavior defined as "all interfaces" when not specified.

public abstract class NettyConfig implements ServerConfig {

public static final int DEFAULT_SSL_PORT = 443;
public static final int DEFAULT_PLAINTEXT_PORT = 8080;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why force 8080, why not the actual default plaintext port of 80?

if we allow port numbers <1024 (i.e. 443), we already require that we can run as root, so 80 should be fine?

Copy link
Member Author

@devinrsmith devinrsmith Jun 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose it this way to leave the current default behavior the same and working out-of-the-box for localhost - no config -> plaintext + 8080. ssl without an explicit port -> ssl + 443.

I think when you are going through the act of ssl configuration, you'll be aware that you need privs to run at 443 (or, you'll know soon enough after startup), or you can do the small additional configuration for a port >= 1024.

I don't think it's "proper" to default to port 80 for plaintext case, since the default config should just "work" IMO.

// Defaults defined in JettyConfig
int httpSessionExpireMs = config.getIntegerWithDefault("http.session.durationMs", -1);
String httpHost = config.getStringWithDefault("http.host", null);
int httpPort = config.getIntegerWithDefault("http.port", -1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am leery of divorcing the default configs from either the .prop file or the actual config.getFoo() invocation, at least as a matter of changing conventions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is much of a coherent convention in how we currently use config. At least, in a lot of cases we tell the "objects" that they are responsible themselves for decoded how to parse their own properties...

I'm hoping this PR will at least present some code that we can agree is "better than before", and I think the code can stand alone (but is a nod towards #2471). For example, nowhere do we document that you can configure the property scheduler.poolSize, nor that the default is 4.

I will concede though, due to this prompting, I think the code can be consolidated and documented better, which I will do... hoping to motivate more that it is "better than before".

@devinrsmith devinrsmith requested a review from niloc132 June 9, 2022 21:19
default String host() {
return DEFAULT_HOST;
static <B extends Builder<?, B>> B buildFromConfig(B builder, Configuration config) {
int httpSessionExpireMs = config.getIntegerWithDefault(HTTP_SESSION_DURATION_MS, -1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I hate providing "null sentinel" default values just so we can read them and apply real defaults, but I agree this is the clearest way to do it with the current api, and it does keep the string and its default closer together to track down defaults that aren't expressly documented (... like most of our configs).

}

/**
* The network interface this server binds to as an IP address or a hostname. If not set, then bind to all
* interfaces.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stupid/pedantic question: will this bind to ipv6 interfaces as part of "all of the interfaces"? or do we need to be more explicit about that. I seem to recall that in java >=9 you can specify * in some spots, but this might not be one of them...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested both netty and jetty - they bind ipv4 and ipv6 as part of "all interfaces". Didn't look into wildcard * at all - if we want some alternative syntaxes we should be able to adapt them.

@devinrsmith devinrsmith merged commit d52367f into deephaven:main Jun 9, 2022
@devinrsmith devinrsmith deleted the ssl-server-configuration branch June 9, 2022 23:14
@github-actions github-actions bot locked and limited conversation to collaborators Jun 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants