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 TLS version changes to deprecation checks #37793

Merged
merged 10 commits into from
Feb 5, 2019

Conversation

tvernum
Copy link
Contributor

@tvernum tvernum commented Jan 24, 2019

This adds a warning to the deprecations API for any SSL contexts that
rely on the default supported_protocols list. This list will change
in ES 7.0 and will no longer include TLS 1.0 by default.

Relates: #37512

This adds a warning to the deprecations API for any SSL contexts that
rely on the default `supported_protocols` list. This list will change
in ES 7.0 and will no longer include TLS 1.0 by default.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@tvernum
Copy link
Contributor Author

tvernum commented Jan 24, 2019

@gwbrown @jaymode
We have a problem here (and as best I can tell #36847 has the same issue).
It looks like the Settings that get passed into the deprecation check are filtered so none of the SSL settings are there.

A config like this:

xpack.security.transport.ssl.enabled: true
xpack.security.transport.ssl.supported_protocols: [ "TLSv1.2", "TLSv1.1", "TLSv1" ]
xpack.ssl.certificate_authorities: [ "tls/ca/ca.crt" ]
xpack.ssl.certificate: tls/server/server.crt
xpack.ssl.key: tls/server/server.key

should generate 2 deprecation warnings:

  1. due to the use of xpack.ssl.* which will be removed
  2. the use of xpack.ssl.* with the default protocols.

It should not generate a default protocols warning for xpack.security.transport.ssl, as the protocols are explicitly configured.

However, it does the exact opposite. The deprecation check sees filtered settings, so it only gets:

xpack.security.transport.ssl.enabled: true

and it doesn't generate any xpack.ssl.* warnings, but does generate one for xpack.security.transport.ssl.

@gwbrown Is it intentional that the deprecation checks run on filtered settings? Any thoughts on how to resolve this?

@tvernum
Copy link
Contributor Author

tvernum commented Jan 24, 2019

This is because the deprecation checks rely on NodeInfo and NodeService always filters the returned settings (which makes sense, that's the point of the filter) but it means we can't write node level deprecation checks that depend on filtered settings.
https://github.com/elastic/elasticsearch/blob/0227260/server/src/main/java/org/elasticsearch/node/NodeService.java#L90

We want the default deprecation rest tests to run without any
deprecations. This means we need to configure "supported_protocols"
for transport.ssl

Note: This doesn't currently work because supported_protocols is a
filtered setting, and filtered settings are not available in
deprecation checks
@gwbrown
Copy link
Contributor

gwbrown commented Jan 25, 2019

Thanks for doing this, and for catching the issue @tvernum.

You're absolutely correct that it does work on filtered settings, and I don't think there's a way to get around that without significantly reworking the deprecations API, which probably isn't going to happen before the next release.

Given that the problem with filtered settings doesn't show up with the way we've been testing these, I'm going to go through all of the deprecation checks and check for any others - I believe there is at least one other check that may look for filtered settings. I'm going to have to think about what to do about this, because there isn't a clear answer.

@gwbrown
Copy link
Contributor

gwbrown commented Jan 25, 2019

I've opened #37845 to track the problem, as it's not confined to this check.

@jaymode
Copy link
Member

jaymode commented Jan 25, 2019

Nice catch Tim.

Copy link
Member

@jaymode jaymode left a comment

Choose a reason for hiding this comment

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

The changes here LGTM. I guess merging depends on what we're going to do with these that need filtered settings

@jaymode jaymode mentioned this pull request Jan 29, 2019
3 tasks
@gwbrown
Copy link
Contributor

gwbrown commented Feb 2, 2019

@tvernum The fixes for the bug discussed above are merged to master - I went ahead and merged master into this PR and made the required changes on a branch, you can take a look here: 6.x...gwbrown:pr/37793

A https monitoring exporter may not have any "ssl.*" settings, but
should still issue a deprecation warning as target cluster (or
infrastructure in between) may require TLSv1.0
@tvernum
Copy link
Contributor Author

tvernum commented Feb 4, 2019

Thanks @gwbrown I'ved merged in your changes.

@jaymode I've also added a change to explicitly check for "https" monitoring exporters that don't have ssl.supported_protocols (similar to LDAP & SAML realms).

@tvernum tvernum merged commit 5d9c040 into elastic:6.x Feb 5, 2019
@EvanXiaa
Copy link

Hello everyone!
I recently came across this discussion and wanted to highlight that TLSv1.1 is now deprecated as well.
Please refer to issue #108057 for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants