NACL BOX master key support #569
Conversation
There was a problem hiding this comment.
I like this idea and it looks good to me at this point (didn't do a full "code review" at this point). My main concern is that I don't think we should ship two binaries. Subcommands work great and the functionality of makenaclboxkey imo does not call to be a separate binary.
cmd/makenaclboxkey/main.go
Outdated
| PublicKey, PrivateKey string | ||
| } | ||
|
|
||
| func main() { |
There was a problem hiding this comment.
Why not have this as a subcommand under the sops binary? Could be sops naclbox-gen or similar?
There was a problem hiding this comment.
Agreed, but I'd prefer to keep the longer makenaclboxkey name.
cmd/makenaclboxkey/main.go
Outdated
| if err != nil { | ||
| panic(err) | ||
| } | ||
| os.MkdirAll(os.Getenv("HOME")+"/.sops/naclbox", 0750) |
There was a problem hiding this comment.
We will need to make sure this is compliant with Windows before we ship.
|
I wasn't sure if you wanted this in a subcommand, so thanks for that piece of information. |
There was a problem hiding this comment.
Looks pretty good. I don't know how to best handle the encrypted private key on disk. Pinentry is okay, but is this something sops should handle? Could we tell people "make sure your home directory is properly encrypted" instead? My concern is that we're never going to be able to implement the authentication mechanisms people are going to want for decryption. E.g. can I use my fingerprint sensor?
cmd/makenaclboxkey/main.go
Outdated
| PublicKey, PrivateKey string | ||
| } | ||
|
|
||
| func main() { |
There was a problem hiding this comment.
Agreed, but I'd prefer to keep the longer makenaclboxkey name.
naclbox/keysource.go
Outdated
| // When used in Sops, we don't care much about the sender keypair, but it is | ||
| // required for the protocol to work, so we issue an ephemeral keypair instead | ||
| // and store its pubkey alongside the encrypted data key, so it can later | ||
| // be decrypted. |
There was a problem hiding this comment.
Should this say "so it can later be reencrypted"? Unless I'm misunderstanding, you're generating a key pair and throwing away the priv key (since it's not going to decrypt anything), and we need to keep it so we can reencrypt the data key in the future if needed without generating a completely new keypair.
| // $HOME/.sops/naclbox/<sha256(pubkey)>.key | ||
| // where <sha256(pubkey)> is the sha256 hash of the raw public key | ||
| h := sha256.Sum256(key.pub[:]) | ||
| path := fmt.Sprintf("%s/.sops/naclbox/%x.key", os.Getenv("HOME"), h[:]) |
There was a problem hiding this comment.
I'd split this into several functions to make it a bit easier to understand. There seems to be a few steps going on, e.g. loading the keys, validating them (base64 decoding and such), and actually decrypting
There was a problem hiding this comment.
Please also add an environment variable that allows to relocate they key to a different directory than $HOME.
|
@autrilla Quick question about the protobuf of keyservice: I noticed it hasn't been updated in a while, and when I added the new key type and regenerated the pb.go file, it changed it dramatically. In particular, Do you know what's going on there? Was it manually updated at some point? |
|
@jvehent I think it's kind of the opposite? Looks like the generated Go code was checked in, but the updated proto definition was not. 4 is the tag previously used for that field, and it's the one you're adding in this PR, so we're good on that side. If you make the name: -string awsprofile = 4;
+string aws_profile = 4;It'll also go back to the old generated code name, which I think we should keep. |
|
Hopefully this will create a lot less headaches than gnupg's over-complicated design. |
|
deprecated in favor of age |
This is work in progress and not yet functional.
The goal is to provide a new type of master key using the NaCl Box protocol that uses local private keys. It's meant to provide an alternative to PGP for those who want to encrypt files locally, without relying on hosted KMS.
I added a new cmdline tool called
makenaclboxkeythat generates a keypair under$HOME/.sops/naclbox/.Most of the logic is in place, but I need to finish the integration with the store. Data key encryption works, but
nonceandephemeralpubkeyaren't captured in the file metadata.Early review/feedback much appreciated.