Skip to content

Add TLS version changes to deprecation checks#37793

Merged
tvernum merged 10 commits intoelastic:6.xfrom
tvernum:deprecation-api-tls10
Feb 5, 2019
Merged

Add TLS version changes to deprecation checks#37793
tvernum merged 10 commits intoelastic:6.xfrom
tvernum:deprecation-api-tls10

Conversation

@tvernum
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

Pinging @elastic/es-security

@tvernum
Copy link
Copy Markdown
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
Copy Markdown
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
@AthenaEryma
Copy link
Copy Markdown
Contributor

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.

@AthenaEryma
Copy link
Copy Markdown
Contributor

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

@jaymode
Copy link
Copy Markdown
Member

jaymode commented Jan 25, 2019

Nice catch Tim.

Copy link
Copy Markdown
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
@AthenaEryma
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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
Copy Markdown

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