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

Avoid re-encrypting key for all existing clients #269

Merged
merged 1 commit into from
Jun 20, 2017

Conversation

kamaradclimber
Copy link
Contributor

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

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>
@kamaradclimber
Copy link
Contributor Author

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)

@kamaradclimber
Copy link
Contributor Author

cc @aboten

@kamaradclimber
Copy link
Contributor Author

kamaradclimber commented Apr 25, 2017

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 :)

kamaradclimber added a commit to criteo-forks/chef-vault that referenced this pull request Apr 25, 2017
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
kamaradclimber added a commit to criteo-forks/chef-vault that referenced this pull request Apr 25, 2017
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>
@thommay thommay added the Type: Enhancement Adds new functionality. label Jun 7, 2017
@kamaradclimber kamaradclimber merged commit 895fb67 into chef:master Jun 20, 2017
@krzkowalczyk
Copy link

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.

@kamaradclimber
Copy link
Contributor Author

kamaradclimber commented Jul 5, 2017 via email

@bryward
Copy link

bryward commented Jul 12, 2017

@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.

@krzkowalczyk
Copy link

@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.

@bryward
Copy link

bryward commented Jul 13, 2017

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.

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:
I don't know if this is the correct place to post this or not, but I'm going to share my work-around, which uses Terraform's remote-exec/destroy provisioner and the "-C" (client) flag available for Linux clients:
# Destroy provisioner to clean up Chef Vault.
provisioner "remote-exec" {
when = "destroy"
connection {
type = "ssh"
user = "${var.ssh_user}"
password = "${var.ssh_password}"
}
inline = [
"knife vault remove ${var.vault} ${var.vault_item} -c /etc/chef/client.rb -M client -C ${var.vmname}.${var.domain} -u ${var.chefuser} --key ${var.keyfile}",
]
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Adds new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants