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

Enable TLS based communication with Zookeeper Backend #4856

Merged

Conversation

palsaurabh2005
Copy link
Contributor

@palsaurabh2005 palsaurabh2005 commented Jul 1, 2018

With this PR I am pushing changes that will allow Vault to communicate with the Zookeeper backend using TLS.
Description for Configuration changes is added to the readme

Saurabh Pal added 3 commits July 1, 2018 10:14
@palsaurabh2005
Copy link
Contributor Author

palsaurabh2005 commented Jul 5, 2018

Hello @jefferai ,
Please suggest if this Pull request requires a new Issue attached to it? The Original issue(#1652), is marked as closed.
Thanks!

@Andrewg-674
Copy link

Andrewg-674 commented Aug 1, 2018

@jefferai @briankassouf @chrishoffman @palsaurabh2005
As advised on this thread we have built Vault from source and can confirm that Mutual Auth works as expected with the Zookeeper backend. We are running a clustered Zookeeper setup that requires a client certificate verification.

@jefferai jefferai modified the milestones: 0.10.5 , 0.11 Aug 13, 2018
@palsaurabh2005
Copy link
Contributor Author

@jefferai @briankassouf @chrishoffman
Admins, I am curious to know if this would ever be reviewed or integrated?

@jefferai
Copy link
Member

Generally speaking it would be nice if other zk storage users reviewed this. You may want to poke some of the people that have contributed to the backend in the past.

@palsaurabh2005
Copy link
Contributor Author

Thanks Jeff. I see there is just one user @Andrewg-674 who has validated this PR.
I have already marked other zk users who had originally made this request or commented on it (#1652).

I am mentioning them here again, hoping to catch their attention:
@devth @kenbreeman @reegz @vixns @sherzberg @elupu

- `tls_enabled` `(bool: false)` – Specifies if TLS communication with the Zookeeper
backend has to be enabled.

- `tls_ca_file` `(string: "")` – Specifies the path to the CA certificate used
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth noting that this supports multiple CA's in a single file which is useful for rotation before they expire.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added additional description with Commit:
palsaurabh2005@3749e91

@kenbreeman
Copy link
Contributor

Looks good to me, one comment on documentation around the CA file.

…ion indicating support for multiple Root CAs in a single file has been added
@palsaurabh2005
Copy link
Contributor Author

Thanks for reviewing this @kenbreeman. As per your comment I have added some more info for the CA cert file usage and its ability to support multiple CA certs in a single file.
The Lib ref for RootCAs property is documented here: https://golang.org/pkg/crypto/tls/ #RootCAs

cc: @jefferai @chrishoffman @briankassouf @devth @reegz @vixns @sherzberg @elupu

@@ -66,16 +66,11 @@ znodes and, potentially, take Vault out of service.
ip:70.95.0.0/16
```

- `auth_info` `(string: "")` – Specifies an authentication string in Zookeeper
Copy link
Contributor Author

Choose a reason for hiding this comment

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

auth_info is removed in this commit since it was incorrectly committed as a duplicate in a previous commit.

@palsaurabh2005
Copy link
Contributor Author

Hello @jefferai
We have the PR verified by 2 members. Would more reviews be needed?

Thanks.

@vishalnayak vishalnayak added this to the 0.11.2 milestone Sep 17, 2018
@palsaurabh2005
Copy link
Contributor Author

palsaurabh2005 commented Sep 18, 2018

@jefferai @vishalnayak
One more thing on the regression impact of this PR:
The flow to establish a TLS connection is only triggered when a user explicitly sets the property "tls_enabled" to true. If it is not set or set to false, the same pre-PR code will execute:

if isTlsEnabled {
 		      // Create a custom Dialer with cert configuration for TLS handshake.
 		     tlsDialer := customTLSDial(conf, machines)
 		     options := zk.WithDialer(tlsDialer)
 		     return zk.Connect(strings.Split(machines, ","), timeout, options)
 	 } else {
                   // **pre-PR -->** 	
                   return zk.Connect(strings.Split(machines, ","), timeout)
 	 }  

So existing integrations that do not have this "tls_enabled" will work as expected without any impact from this change.

@briankassouf briankassouf merged commit 239f201 into hashicorp:master Oct 1, 2018
@palsaurabh2005
Copy link
Contributor Author

@briankassouf
Thank you for Merging the PR!

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

Successfully merging this pull request may close these issues.

7 participants