Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

Add SSL/TLS support #620

Merged
merged 43 commits into from
Nov 19, 2019
Merged

Add SSL/TLS support #620

merged 43 commits into from
Nov 19, 2019

Conversation

pemontto
Copy link
Contributor

This PR adds the ability to configure password protected key and certificate, or keystore and truststore transport and HTTP encryption.

If both are defined it will preference keystores and truststore based on the preference in the Elasticsearch documentation. It doesn't yet support having different keys for HTTP and trasport, though that won't be hard to add.

Currently a WIP because it's not documented and I have yet to define tests to cover various use cases. I'm having trouble getting converge running, it seems to pick up some strange paths when "preparing roles".

The role attempts to add or remove passwords to/from the keystore only when the cert/truststore copy operation is changed. This might be a non-issue but it could lead to an edge case where if alternate key/keystores have been uploaded already, swapping between them won't update the password to unlock them in the keystore.

Failed to complete #converge action: [No such file or directory @ rb_sysopen - /Users/test/git/<some other git repo>/.venv/.Python] on oss-centos-7

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@pemontto
Copy link
Contributor Author

This builds on the work of @AevaOnline from #597 and @kcm from #479

@pemontto pemontto changed the title WIP: Add SSL/TLS support Add SSL/TLS support Oct 18, 2019
@pemontto
Copy link
Contributor Author

@jmlrt this is ready to look at, are you able to kick off a jenkins build?

I had to do my testing with the new trial task to test security under version 7.1.0.

TODO: documentation

@jmlrt
Copy link
Member

jmlrt commented Oct 18, 2019

jenkins test this please

@pemontto
Copy link
Contributor Author

@jmlrt I've moved the cert files along with the template files and updated the integration tests:

files/certs -> test/integration/files/certs
files/templates-6.x -> test/integration/files/templates-6.x
files/templates-7.x -> test/integration/files/templates-7.x

@jmlrt
Copy link
Member

jmlrt commented Oct 31, 2019

jenkins test this please

@jmlrt
Copy link
Member

jmlrt commented Oct 31, 2019

@jmlrt I've moved the cert files along with the template files and updated the integration tests:

Nice catch for the templates 👍

@pemontto
Copy link
Contributor Author

devops failure looks to be a transient debian-8 issue

@jmlrt
Copy link
Member

jmlrt commented Oct 31, 2019

devops failure looks to be a transient debian-8 issue

Yes that's not related to your changes so not a blocker.

Local tests are OK for me.

I would prefer removing the callback_whitelist of the role.

I will also ask for a second opinion from elastic people, especially for the idea of generating tls configuration in elasticsearch.yml.j2 versus letting users add it in es_config.

@Crazybus Crazybus self-requested a review October 31, 2019 13:16
jmlrt
jmlrt previously approved these changes Oct 31, 2019
@jmlrt jmlrt mentioned this pull request Nov 6, 2019
@jmlrt
Copy link
Member

jmlrt commented Nov 14, 2019

jenkins test this please

Crazybus
Crazybus previously approved these changes Nov 15, 2019
Copy link
Contributor

@Crazybus Crazybus left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you so much for all of the work you have done here. Including adding testing and updating the docs it really is amazing.

My comment around making the auto generated certificate configuration optional isn't blocking. While there are edge cases that could prevent users from using these auto generated values, right now they can't do it at all :P So the ability to disable it can always come in a future PR.

@@ -33,6 +33,39 @@ action.auto_create_index: {{ es_action_auto_create_index }}

{% if es_enable_xpack and es_api_basic_auth_username is defined and es_api_basic_auth_password is defined %}
xpack.security.enabled: true

{% if es_enable_transport_ssl | bool %}
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 its ok to leave these autogenerated values in as long as there is a way to opt out of it by adding some kind of es_enable_auto_ssl_configuration: false. Right now if someone wanted to add multiple certificate_authorities (which is an array/list) they wouldn't be able to override it in es_config because those values are being provided first and assume that you only have one CA. There are probably a bunch of other edge cases that could prevent a user from enabling security if they were forced to have these exact values set.

@pemontto pemontto dismissed stale reviews from Crazybus and jmlrt via 8156ab4 November 15, 2019 12:24
Copy link
Member

@jmlrt jmlrt left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@Crazybus Crazybus left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for adding in that extra option to disable the auto generated config 👍

@jmlrt
Copy link
Member

jmlrt commented Nov 19, 2019

jenkins test this please

@jmlrt
Copy link
Member

jmlrt commented Nov 19, 2019

One failing test not related to PR code (timeout during ES package install).

Let's merge this PR!

@pemontto but also @AevaOnline (#597), @itsmed & @kcm (#479), @strootman (#302), @xocasdashdash (#635), @Justbkuz (#442), @cdahlqvist (#331), thanks you all for your amazing work to make this Ansible role supporting with TLS 👍 👍 👍

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

Successfully merging this pull request may close these issues.

6 participants