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

Teach reindex to honor xpack.ssl.certificate_authorities #29755

Closed
elasticmachine opened this issue Feb 10, 2017 · 14 comments
Closed

Teach reindex to honor xpack.ssl.certificate_authorities #29755

elasticmachine opened this issue Feb 10, 2017 · 14 comments
Assignees
Labels
>enhancement :Security/TLS SSL/TLS, Certificates

Comments

@elasticmachine
Copy link
Collaborator

Original comment by @nik9000:

Right now reindex is fine with ssl but doesn't read xpack.ssl.certificate_authorities. It probably should, but I'm not really sure how to make that work because reindex and x-pack don't know about each other. I'm sure we could get it to work by making some common thing inside core that x-pack implements and reindex uses, but I'm not sure it is worth the effort. Maybe? Thus, discuss label.

@elasticmachine
Copy link
Collaborator Author

Original comment by @jaymode:

I'm sure we could get it to work by making some common thing inside core that x-pack implements and reindex uses

I wonder if we should have the notion of a default keystore/truststore in core for things that need SSL. For core, this would point to the jdk defaults that are configured using system properties. Plugins would need to be allowed to provide a default and then xpack would set it.

but I'm not sure it is worth the effort

I am also on the fence about the effort and usefulness vs asking the user to configure the default JDK one (or if reindex has other settings).

A simpler and possibly cleaner idea is to have xpack set the default SSLContext based on what has been configured using xpack.ssl.*, under the assumption that this is what reindex will use for reindex to remote.

@elasticmachine
Copy link
Collaborator Author

Original comment by @nik9000:

A simpler and possibly cleaner idea is to have xpack set the default SSLContext based on what has been configured using xpack.ssl.*, under the assumption that this is what reindex will use for reindex to remote.

It is certainly worth testing that assumption, but if it proves true I think it is fine.

@elasticmachine
Copy link
Collaborator Author

Original comment by @joshbressers:

This just showed up on my radar. I assume that with the coming TLS everywhere, we will want this working properly.

@jaymode @tvernum Any thoughts on how to best fix this?

@elasticmachine
Copy link
Collaborator Author

Original comment by @tvernum:

I'll do some of tests on this and see whether setting the default SSL context does what we want.

@elasticmachine
Copy link
Collaborator Author

Original comment by @tvernum:

This doesn't work out of the box.
The REST client uses Apache HTTP client, which by default doesn't use the default SSL Context, and the REST client doesn't configure an SSL context (or enable the default context).

It's possible to make it work, but it would require changes to the TransportReindexAction to configure the SSLContext on the REST client.

@elasticmachine
Copy link
Collaborator Author

Original comment by @jaymode:

I opened a core issue for the rest client aspect previously LINK REDACTED I'll go ahead and take a look at that since no one else has gotten around to it yet.

@elasticmachine
Copy link
Collaborator Author

Original comment by @eskibars:

I see #23231 closed. Is there anything here still outstanding?

@elasticmachine
Copy link
Collaborator Author

Original comment by @jaymode:

The core issue allowed for us to use the system default context, but I think the item left to do here is to consider making the SSLContext defined by the settings xpack.ssl.* the default system context. I'm on the fence on whether we should actually do this; it provides a nicer experience but then we are changing system defaults. Maybe we only do this if no default context is set and/or provide a setting to disable it.

@elasticmachine
Copy link
Collaborator Author

Original comment by @ppf2:

+1 on making it work with xpack.ssl.*. On 5.0+, we made an effort to actively promote all users to use the xpack.ssl.* settings to directly reference the pem certs instead of using JKS - and our documentation for setting up encryption in x-pack has since been rewritten with all examples referencing pem (with jks information hidden away only in the x-pack settings section, not in the actual installation instructions). I have a customer who dislikes JKS so much that they are very happy with our move away from JKS, but the restclient + reindexing api are the pieces that are forcing them to go back to set up JKS stores. So allowing users to use xpack.ssl.* (as an option) for both the low level restclient + reindexing API will allow users to control this only via xpack.ssl.* and be consistent everywhere.

@elasticmachine
Copy link
Collaborator Author

Original comment by @ppf2:

Related: #25604

@elasticmachine
Copy link
Collaborator Author

Original comment by @PhaedrusTheGreek:

Related: #27267

@tvernum
Copy link
Contributor

tvernum commented May 10, 2018

Note, this issue is specifically about how to make X-Pack based SSL config available in _reindex, not general reindex/SSL questions, but the two questions can't be separated perfectly

I propose that we do something based around the following

  • Not use xpack.ssl.* settings because:

    1. these tend to interfere with other X-Pack features (like watcher connections to https URLs), which prompted us to change the docs in 42f9a99
    2. we're looking to remove those fallback settings anyway (Remove all fallback in SSL settings #29797)
  • Not add overwrite the system trust config (SSLContext.setDefault) because configuring the default system trust incorrectly will also break other features (like watcher) and experience shows that configure SSL well is tricky, and customers often don't completely understand the implication of changes they're making. Making changes to the default SSL context seems trappy. Even if we only supported adding additional CAs, that still has the risk that the JVM would start trusting certs that it shouldn't.

  • As part of a fix I'm working on for Monitoring Exporter attempts to read closed SecureSettings #30344, we're planning to introduce named ssl contexts. These allow code that uses the X-Pack SSL Service to ask for a context by name, without needing to have a copy of the actual settings.

  • We now have an SPI concept via Plugins: Add plugin extension capabilities #27881. This would allow the reindex module and x-pack module to share code in ways that weren't possible when this issue was raised.

Putting those together, would mean that

  • X-Pack can add some settings to configure an SSL context for "_reindex"
  • We can implement a basic SSL provider concept, either in server (core) or in the reindex module through SPI
  • X-Pack implements this provider interface, to make SSL contexts available to non X-Pack code
  • Reindex uses the discovered service providers to configure an appropriate SSL context.

There's some open questions there like:

  • do you need to tell reindex that it should look for a named SSL context?
  • is it too lenient to: use x-pack if configured, otherwise use the system default? (We typically lean towards explicit over implicit)
  • is SPI the correct approach, or would a standard plugin make more sense?
  • do we want to support multiple SSL providers, or require that there be only one?

but all those questions are relatively minor and could be resolved as part of the implementation.

@tvernum
Copy link
Contributor

tvernum commented May 10, 2018

ping: @elastic/es-security , @joshbressers , @eskibars

@jkelastic
Copy link

@eskibars Do you know if there's an ETA on this? If so, approximately when?

@tvernum tvernum self-assigned this Nov 26, 2018
tvernum added a commit that referenced this issue Jan 31, 2019
Adds reindex.ssl.* settings for reindex from remote.

This uses the ssl-config/ internal library to parse and load SSL
configuration and files. This is applied when using the low level
rest client to connect to a remote ES node

Relates: #37287
Resolves: #29755
tvernum added a commit to tvernum/elasticsearch that referenced this issue Feb 4, 2019
Adds reindex.ssl.* settings for reindex from remote.

This uses the ssl-config/ internal library to parse and load SSL
configuration and files. This is applied when using the low level
rest client to connect to a remote ES node

Backport of: elastic#37527
Relates: elastic#37287
Resolves: elastic#29755
tvernum added a commit that referenced this issue Feb 4, 2019
Adds reindex.ssl.* settings for reindex from remote.

This uses the ssl-config/ internal library to parse and load SSL
configuration and files. This is applied when using the low level
rest client to connect to a remote ES node

Backport of: #37527
Relates: #37287
Resolves: #29755
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Security/TLS SSL/TLS, Certificates
Projects
None yet
Development

No branches or pull requests

5 participants