-
Notifications
You must be signed in to change notification settings - Fork 159
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
Avoid re-encrypting key for all existing clients #269
Conversation
Before this patch, symetrical key was encrypted for all clients on refresh. With this patch, we only encrypt symetrical key for new clients and re-use previously encrypted key. This patch requires to improve "vault remove" scenario to clear encrypted data before re-encrypting. Change-Id: I0abccb32d45deb6ae51a1afdaff54c1e7c994e29 Signed-off-by: Grégoire Seux <g.seux@criteo.com>
88cd8c0
to
50d669a
Compare
Main improvement of this patch is speed for refresh scenario. On a node with ~500 clients, refresh goes from 30s to 7 seconds (when there is no change at all in the search_query result) |
cc @aboten |
38154e3
to
50d669a
Compare
To be fair, this patch breaks a corner case where a node would change its default chef key. You would have to update secret now to make sure such a node regain access. If you don't think that's an issue and have no further comments, I'd be happy to get this merged and released :) |
Sparse mode is marked in xxx_keys item with: > mode: "sparse" but when decrypting secrets, each node is trying to read the sparse format first (xxx_key_[node]) and then fallback to normal xxx_keys. This adds a performance penalty both on reading secrets and during refresh. With this patch, sparse format is checked only when secret is marked as sparse. This makes refresh a fast no-op (it was already a no-op with chef#269 now it is faster) since the only cost is now searching nodes matching search_query. Change-Id: I38f511b9f590240775085a386b387c476d3a1f5c
Sparse mode is marked in xxx_keys item with: > mode: "sparse" but when decrypting secrets, each node is trying to read the sparse format first (xxx_key_[node]) and then fallback to normal xxx_keys. This adds a performance penalty both on reading secrets and during refresh. With this patch, sparse format is checked only when secret is marked as sparse. This makes refresh a fast no-op (it was already a no-op with chef#269 now it is faster) since the only cost is now searching nodes matching search_query. Change-Id: I38f511b9f590240775085a386b387c476d3a1f5c Signed-off-by: Grégoire Seux <g.seux@criteo.com>
Hi, As there is not build-in (as far as I know) function to clean unknown clients after node deletion, this requires some custom, post delete action to clean old client if you want to reuse the name. Sure I could run knife vault refresh X Y --clean-unknown-clients, but then I need to wrap it in a loop for each data bag item, as unfortunately there is no knife vault refresh all keys command. For me, I like more previous behavior with refreshing existing clients. |
Thanks for the report.
I think you should open a new issue about this.
How about we try to add an opt-in flag to restore the re-encrypt at each
run situation?
…On Wed, Jul 5, 2017 at 2:06 PM krzkowalczyk ***@***.***> wrote:
Hi,
Starting from today, when new chef-vault gem was published, our
environment broke down. Our luck is to be in described corner case. We
regenerate nodes daily, using terraform and chef provisioner, reusing the
same node name. As client key is new each time, vaults were refreshed each
time. From now vault refresh does not refresh keys in vault, resulting
failing to access secrets.
As there is not build-in (as far as I know) function to clean unknown
clients after node deletion, this requires some custom, post delete action
to clean old client if you want to reuse the name. Sure I could run knife
vault refresh X Y --clean-unknown-clients, but then I need to wrap it in a
loop for each data bag item, as unfortunately there is no knife vault
refresh all keys command.
For me, I like more previous behavior with refreshing existing clients.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#269 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAeu8cFUpIM3BebAwu6eA73n8BcD3zHgks5sK3wngaJpZM4NBWB9>
.
|
@krzkowalczyk, have you found a work-around for this? I've been experiencing this exact same issue. The biggest issue is that the "knife vault update" command returns 200 responses from chef-server, even though the vault is never updated with the new public client key. This, IMO, is a bug. I've been trying to figure out a way to pin the chef-vault version to 3.0.3 (the version I believe to be the last-known-good), but it seems that there's no way to do that using Terraform's Chef provisioner. |
@bryward just a workaround As you said "knife vault update" won't update the key as well. To do that you need to first delete the node from client or admin list (knife vault remove), and then "knife update". As for terraform my colleague did a patch which first remove clients from vault before adding them, and it did solve the problem for us. Here is a PR Anyway I would go for opt-in flag "--force" or similar which would re-encrypt all keys. |
In my environment (~1500 clients), this takes about 5 minutes to do using the search (-S "name:[node_name]") flag. Is that your experience as well or have you found a quicker method? EDIT: |
Before this patch, symetrical key was encrypted for all clients on refresh.
With this patch, we only encrypt symetrical key for new clients and re-use previously encrypted key.
This patch requires to improve "vault remove" scenario to clear encrypted data before re-encrypting.
Change-Id: I0abccb32d45deb6ae51a1afdaff54c1e7c994e29