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

[DOCS] Adds common definitions for security settings #51017

Merged
merged 10 commits into from
Mar 6, 2020

Conversation

lcawl
Copy link
Contributor

@lcawl lcawl commented Jan 15, 2020

Related to #50984

There are currently similar SSL/TLS settings for monitoring, alerting, and security features (e.g. listed in https://www.elastic.co/guide/en/elasticsearch/reference/master/security-settings.html#http-tls-ssl-settings, https://www.elastic.co/guide/en/elasticsearch/reference/master/monitoring-settings.html#ssl-monitoring-settings, and https://www.elastic.co/guide/en/elasticsearch/reference/master/notification-settings.html#ssl-notification-settings). Some of them are generated from a shared file https://github.com/elastic/elasticsearch/blob/master/docs/reference/settings/ssl-settings.asciidoc.

There are discrepancies in how some of the settings are defined, however, and whether their default values are listed.

This PR attempts to address those problems as follows:

I have left in some clauses where there are slight variations in the definitions for now (e.g. setting X says it cannot be used at the same time as setting Y). I think for the most part those should ultimately go away (i.e. they're likely applicable to all occurrences of that setting, not just one or two).

Preview:

@lcawl lcawl added >docs General docs changes WIP :Security/TLS SSL/TLS, Certificates v8.0.0 labels Jan 15, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-docs (>docs)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security (:Security/Network)

@lcawl lcawl removed the WIP label Jan 22, 2020
@lcawl lcawl marked this pull request as ready for review January 22, 2020 20:01
@lcawl lcawl requested a review from tvernum January 22, 2020 20:01
@lcawl
Copy link
Contributor Author

lcawl commented Jan 24, 2020

@elasticmachine update branch

@elasticmachine
Copy link
Collaborator

user doesn't have permission to update head repository

@tvernum
Copy link
Contributor

tvernum commented Jan 28, 2020

@elasticmachine update branch

@elasticmachine
Copy link
Collaborator

user doesn't have permission to update head repository

Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

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

This is a big step forwards, so I'd like us to merge it when we can, even if we don't sort out some of the finer points of about combining the different variants and the FIPS content.

tag::ssl-certificate[]
Specifies the path for the PEM encoded certificate (or certificate chain) that is
associated with the key.
//TBD: This setting can be used only if `ssl.key` is set.
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the situation with these //TBD parts? Are you looking for a review as to whether that content is correct, or trying to decide how to represent them in the docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tvernum Sorry for the lack of clarity. These "TBD" pieces are bits of information that only existed in a subset of the places where these definitions were used. For now, I've left them as-is and the TBD is to move that info into the shared definition if indeed they're applicable to all contexts (likely in a subsequent PR).

`TLS_RSA_WITH_AES_256_CBC_SHA256`, `TLS_RSA_WITH_AES_128_CBC_SHA256`,
`TLS_RSA_WITH_AES_256_CBC_SHA`, `TLS_RSA_WITH_AES_128_CBC_SHA`.
end::ssl-cipher-suites-values[]
//TBD: Are these two different definitions (with different Oracle URLs) for cipher_suites required?
Copy link
Contributor

Choose a reason for hiding this comment

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

On master, no because we don't support Java 8 any more.
On 7.x, I think so. The set of options (and defaults) are different depend in the Java version.

Although, strictly speaking we have different values between Java11 and Java12 as well (since #42155) so we may need to think this through a bit more.

@jkakavas What do we want to do with FIPS here? BC JSSE doesn't support TLS1.3, so some of these ciphers aren't supported, but I think technically the default is still correct (we don't change the default cipher suite list based on the supported protocols list).

Copy link
Member

Choose a reason for hiding this comment

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

Yes the default is still that one so I agree we shouldn't add anything here (We could add something in the FIPS docs though, but this is part of the effort to make FIPS docs better ).

The actual list of unusable ones while running with fips mode ( we do print this in the logs on startup too)
JDK11:

TLS_AES_256_GCM_SHA384
TLS_AES_128_GCM_SHA256
TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384
TLS_RSA_WITH_AES_256_GCM_SHA384
TLS_RSA_WITH_AES_128_GCM_SHA256

JDK13:

TLS_AES_256_GCM_SHA384
TLS_AES_128_GCM_SHA256
TLS_CHACHA20_POLY1305_SHA256
TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384
TLS_RSA_WITH_AES_256_GCM_SHA384
TLS_RSA_WITH_AES_128_GCM_SHA256

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @jkakavas I've drafted those changes in #53138

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The set of options (and defaults) are different depend in the Java version. Although, strictly speaking we have different values between Java11 and Java12 as well (since #42155) so we may need to think this through a bit more.

@tvernum For now I have (1) deleted the "ssl-cipher-suites-values-java8" definition, (2) moved the Java11 info into a "ssl-cipher-suites-values-java11" definition, (3) put a Java12 URL in the "ssl-cipher-suites-values" definition and added the three ChaCha20 ciphers, (4) updated all occurrences of *-values-java8 and *-values-java11 to use the generic *-values. I'm just keeping the *-java11 for now in case we backport this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @lcawl that sounds right to me.

Path to a PEM encoded file containing the private key.
//TBD: You cannot use this setting and `ssl.keystore.path` at the same time.
end::ssl-key-pem[]
//TBD: Is it correct that this setting only applies to PEM files or can it support other types?
Copy link
Contributor

Choose a reason for hiding this comment

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

It only applies to PEM files, but crypto file formats are a complex topic.
The PEM encoding format supports a variety of different key type and formats, so it's possible to have a EC private key in PKCS8 format inside a PEM file.

We don't want to go into that detail here, but "PEM" is a lot less specific than it sounds.

docs/reference/settings/common-defs.asciidoc Outdated Show resolved Hide resolved
docs/reference/settings/common-defs.asciidoc Outdated Show resolved Hide resolved

--
end::ssl-supported-protocols[]
//TBD: Are these three different definitions for supported protocols valid?
Copy link
Contributor

Choose a reason for hiding this comment

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

No, I don't think there's a reason to have all 3 variants. The main (first) one should be accurate everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks!

end::ssl-truststore-path[]

tag::ssl-truststore-path-jks[]
The path for the Java keystore file that contains the certificates to trust.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this. If it's used somewhere it's probably out of date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The format of the keystore file. It must be either `jks` or `PKCS12`. If the
keystore path ends in ".p12", ".pfx", or "pkcs12", this setting defaults
to `PKCS12`. Otherwise, it defaults to `jks`.
end::ssl-keystore-type-pkcs12[]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we probably want to merge the pkcs11 and pkcs12 variants. I don't believe there is any context in which the beahviour of this setting is different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@tvernum tvernum Mar 4, 2020

Choose a reason for hiding this comment

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

I'll admit my PKCS#11 knowledge is pretty thin - maybe @jkakavas can chime in here, but ...

No, we still want that section, I think (but am not 100% sure) that our PKCS#11 support is consistent across all SSL contexts, so anything we say for xpack.security.http.ssl.keystore.type could technically be said about monitoring ssl, etc.

However, there is a limit of having only 1 PKCS#11 provider per JVM, so setting this up & describing it is pretty complex. Maybe we leave this for now so we can merge it, and then come back to it. I think we'd need to setup a PKCS#11 environment for testing, and I don't have cycles for that right now.

Copy link
Member

Choose a reason for hiding this comment

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

Tim's comment is right. PKCS#11 support is consistent across all SSL contexts, but there can only be 1 PKCS#11 provider/keystore and we don't offer a way to configure which alias in a keystore will be used. So that means that of PKCS#11 is used, then there is a single key/certificate pair that needs to be shared across all SSL contexts ( more details are shared in #33459 ) . I agree we can handle adding a few more details in PKCS11 in another PR

@lcawl
Copy link
Contributor Author

lcawl commented Mar 6, 2020

Thanks for all the feedback @jkakavas and @tvernum! I'm going to rebase now to hopefully address the CI errors then I think we're good to go.

@lcawl lcawl merged commit cd5910b into elastic:master Mar 6, 2020
@lcawl lcawl deleted the watcher-tls branch March 6, 2020 19:28
lcawl added a commit to lcawl/elasticsearch that referenced this pull request Mar 6, 2020
@lcawl lcawl added the v7.7.0 label Mar 6, 2020
lcawl added a commit to lcawl/elasticsearch that referenced this pull request Mar 6, 2020
@lcawl lcawl added the v7.6.2 label Mar 6, 2020
lcawl added a commit that referenced this pull request Mar 7, 2020
lcawl added a commit that referenced this pull request Mar 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>docs General docs changes :Security/TLS SSL/TLS, Certificates v7.6.2 v7.7.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants