-
Notifications
You must be signed in to change notification settings - Fork 64
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
Add chef-vault implementation #minor #101
base: master
Are you sure you want to change the base?
Conversation
It would be great to replace these shell commands : https://github.com/hashicorp/terraform/blob/c0793c84fdbea8c4f3f7597cad57bcca38b21ebf/builtin/provisioners/chef/resource_provisioner.go#L569-L617 |
Looks nice. It's taking me while to get up to speed on the code base. I'll try to get this evaluated over the next month. |
@MarkGibbons great! LMK what you think and I'll fix the conflicts. |
|
Nice code, I'm writing examples and testing against a chef server. See https://github.com/go-chef/chef/tree/vault if you want to see. I merged your branch into my vault branch and will merge it once testing and maybe a couple other feature get added. |
@@ -31,7 +31,6 @@ func TestSearch_Get(t *testing.T) { | |||
"users": "http://localhost:4000/search/users", | |||
} | |||
if !reflect.DeepEqual(indexes, wantedIdx) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self. Figure out if the deleted error message is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
const algorithm string = "aes-256-gcm" | ||
const supportedVersion int = 3 | ||
|
||
var errUnsupportedVersion = errors.New("Only Version 3 encrypted values are supported") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version 3 of what? Comment needs to be clearer.
V3 of the encrypted data bag support from chef. PR comments have a nice link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update comments in my version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
Keys *VaultItemKeys | ||
Name string | ||
Vault string | ||
VaultService *VaultService |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to understand why vault service is part of the vault item structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass the client connection to Item processing. Need to be clear that get is required before decrypt or update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
} | ||
|
||
// CreateItem creates a vault item and keys databag | ||
func (vs *VaultService) CreateItem(vaultName, itemName string) (*VaultItem, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like Create Item creates an empty vault and then Update adds the content. Note to self, verify this and add doc to make it clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also need to create the data bag before creating the vault items.
OK
return item, nil | ||
} | ||
|
||
// UpdateItem sets the item data, encrypts with a shared key, and then encrypts the shared key with each authorized client key in the <item>_keys data bag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this function a total replacement of the contents of the vault item? Figure it out and doc. Any local data concept (knife -m solo concept) or does it always hit the server (knife -m client)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what mode does. Looks like the code is always aimed at the server.
return nil, err | ||
} | ||
|
||
keysItem := map[string]interface{}{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are the admins, client, search_query set and used? Figure that out, create code examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding user to an item needs to be clearer.
Also may need to handle bad users (old keys).
keys := map[string]bool{} | ||
|
||
for name := range *databags { | ||
if vaultName := strings.TrimSuffix(name, "_keys"); vaultName != name { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this code finds chef vault data bags. The vaults don't have the _keys name, the items within the vaults have those names.
databagData := DataBagItem(itemData) | ||
|
||
item.DataBagItem = &databagData | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to reencrypt the shared secret for the users in admins and clients as is we can't specify admins or clients which makes the feature hard to use. The search feature is also not implemented. I'm looking at adding admins and clients.
Corrected a few spelling mistakes along the way.
Add a compatible client implementation to the chef-vault ruby version.
The testing could be much better. I will throw the crypto tests I have in next. This testing is much more useful with a live chef server.
I rebased master but it seems that your other branch is pretty far along. This code base has come long way since I opened this PR. Let me know if I can help with anything. |
I'm noodling at adding users, clients and getting the search function to work. In this line
in the CreateItem method. I think that is supposed to be a PublicKey. The encode and decode methods in crypt both use the PrivateKey. Both of those things and adding code to deal with multiple users and nodes need to be there IMO. If you have time, cool. Otherwise I'll likely get it. Might take me a while. You can see my branch, local vault branch, to see what I've done already. I've been adding tests against a chef server that gets spun up. thanks, |
not sure if this is still an active effort but noticed only v3 supported. i did some similar work on https://github.com/bhoriuchi/go-chef-crypto and have v1 and v2 if interested in adding to this. also one issue i ran into to note is that the ruby base64 package adds new lines every 60 chars when you use knife so https://github.com/bhoriuchi/go-chef-crypto/blob/master/crypto.go#L211-L216 is used to replicate that. |
I'll look at this one again over the next month. The last time I looked at this I went down some really nasty rabbit holes. |
I'm just looking to support encryption/decryption (not vault nec.) |
Adding support for chef vault to this api may be too much effort for too little return. |
I don't see why not. Haven't started, but initial thought would be to add
go-chef-encrypt as dependency and submit PR with it integrated into go-chef
API. If this sounds ok to you, I'll open a separate issue and post PR
there.
…On Thu, Sep 8, 2022 at 9:27 PM Mark Gibbons ***@***.***> wrote:
Adding support for chef vault to this api may be too much effort for too
little return.
@dhubler <https://github.com/dhubler> & @bhoriuchi
<https://github.com/bhoriuchi> adding better encryption support is
something that makes sense. Could the code to do that be turned in to a
pull request?
—
Reply to this email directly, view it on GitHub
<#101 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACA7T4ODCX6ECNPRCSXULV5KHALANCNFSM4FONAKCA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Currently only supports version 3 of the encryption/decryption scheme: See https://github.com/chef/chef/blob/7c9178e5dd4ec7b5a75993c148b087bac6e5b53d/lib/chef/encrypted_data_bag_item/encryptor.rb#L162-L222
Fairly minimal and the testing is weak but I thought I'd share before progressing further.
This change is