Reject misconfigured/ambiguous SSL server config #45892
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit makes it an error to start a node where either of the server contexts (xpack.security.transport.ssl and xpack.security.http.ssl) meet either of these conditions: 1. The server lacks a certificate/key pair (i.e. neither ssl.keystore.path not ssl.certificate are configured) 2. The server has some ssl configuration, but ssl.enabled is not specified. This new validation does not care whether ssl.enabled is true or false (though other validation might), it simply makes it an error to configure server SSL without being explicit about whether to enable that configuration.
|
Pinging @elastic/es-security |
|
This is 8.0 only, as it is a breaking change. I will raise a separate PR to backport to 7.x as a deprecation. |
|
@elasticmachine update branch |
jkakavas
approved these changes
Aug 27, 2019
| `xpack.security.http.ssl` without also configuring | ||
| `xpack.security.http.ssl.enabled`. | ||
|
|
||
| For example, the following configuration is invalid: |
There was a problem hiding this comment.
Maybe do this example with PEM files to further visually indicate that this doesn't apply only to keystores/truststores?
| final SSLConfigurationSettings configurationSettings = SSLConfigurationSettings.withPrefix(prefix + "."); | ||
| if (isConfigurationValidForServerUsage(configuration) == false) { | ||
| throw new ElasticsearchSecurityException("invalid configuration for " + prefix + | ||
| " - a server context requires a key and certificate, but these have not been configured; either [" + |
There was a problem hiding this comment.
Unsure how clear "server context" is. We could try "server SSL context" or "server SSL configuration" ?
| throw new ElasticsearchSecurityException("invalid configuration for " + prefix + | ||
| " - a server context requires a key and certificate, but these have not been configured; either [" + | ||
| configurationSettings.x509KeyPair.keystorePath.getKey() + "] or [" + | ||
| configurationSettings.x509KeyPair.certificatePath.getKey() + "] should be set"); |
There was a problem hiding this comment.
Suggested change
| configurationSettings.x509KeyPair.certificatePath.getKey() + "] should be set"); | |
| configurationSettings.x509KeyPair.keyPath.getKey() + " and " + configurationSettings.x509KeyPair.certificatePath.getKey() + "] should be set"); |
| configurationSettings.x509KeyPair.certificatePath.getKey() + "] should be set"); | ||
| } | ||
| } else if (settings.hasValue(enabledSetting) == false) { | ||
| final List<String> sslKeys = settings.keySet().stream() |
There was a problem hiding this comment.
Suggested change
| final List<String> sslKeys = settings.keySet().stream() | |
| final List<String> sslConfigKeys = settings.keySet().stream() |
to make it immediately obvious when looking at the code these are not SSL Keys but configuration keys
We added a few tests with invalid config while this PR was stagnant
37 tasks
|
@elasticmachine update branch |
tvernum
added a commit
to tvernum/elasticsearch
that referenced
this pull request
Nov 19, 2019
This commit adds a deprecation warning when starting a node where either of the server contexts (xpack.security.transport.ssl and xpack.security.http.ssl) meet either of these conditions: 1. The server lacks a certificate/key pair (i.e. neither ssl.keystore.path not ssl.certificate are configured) 2. The server has some ssl configuration, but ssl.enabled is not specified. This new validation does not care whether ssl.enabled is true or false (though other validation might), it simply makes it an error to configure server SSL without being explicit about whether to enable that configuration. Backport of: elastic#45892
tvernum
added a commit
that referenced
this pull request
Nov 22, 2019
This commit adds a deprecation warning when starting a node where either of the server contexts (xpack.security.transport.ssl and xpack.security.http.ssl) meet either of these conditions: 1. The server lacks a certificate/key pair (i.e. neither ssl.keystore.path not ssl.certificate are configured) 2. The server has some ssl configuration, but ssl.enabled is not specified. This new validation does not care whether ssl.enabled is true or false (though other validation might), it simply makes it an error to configure server SSL without being explicit about whether to enable that configuration. Backport of: #45892
This was referenced Feb 3, 2020
80 tasks
masseyke
added a commit
that referenced
this pull request
Sep 3, 2021
…settings (#77092) In 8.0 we prevent the server from starting up if certain SSL properties are misconfigured or ambiguous. Specifically: 1) The server lacks a certificate/key pair (i.e. neither ssl.keystore.path nor ssl.key/ssl.certificate are configured) 2) The server has some ssl configuration, but ssl.enabled is not specified. This commit adds a check to the deprecation info API for these changes. Relates #42404 #45892
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit makes it an error to start a node where either of the
server contexts (xpack.security.transport.ssl and
xpack.security.http.ssl) meet either of these conditions:
ssl.keystore.path nor ssl.certificate are configured)
specified. This new validation does not care whether ssl.enabled is
true or false (though other validation might), it simply makes it
an error to configure server SSL without being explicit about
whether to enable that configuration.
Relates to #49280