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

Add support for age. #688

Merged
merged 16 commits into from
Sep 23, 2020
Merged

Add support for age. #688

merged 16 commits into from
Sep 23, 2020

Conversation

jimmycuadra
Copy link
Contributor

@jimmycuadra jimmycuadra commented Jul 4, 2020

Implements support for age as I requested in #603. I'm not a Go programmer and was learning the sops code as I went, so there's likely a lot I missed here. But the basic encrypt and decrypt operations are working for me! I used #569 as a reference for how to do this, as suggested by @jvehent.

Example (this has been simplified in more recent commits. See #688 (comment)):

$ mkdir -p ~/.sops/age/
$ age-keygen | tee age.key
Public key: age1hhpw5e07syc00jwtwxs64e6nqv9exhmmjrlztcxkw9vgnjqev47qllp3ry
# created: 2020-07-04T13:55:01-07:00
# public key: age1hhpw5e07syc00jwtwxs64e6nqv9exhmmjrlztcxkw9vgnjqev47qllp3ry
AGE-SECRET-KEY-1ZWFCS8JEND5M8T7VYRW8VVWF2K7MLS6LQWDDG53NYHTJHNHTCYVQQTK7EG
$ mv age.key ~/.sops/age/age1hhpw5e07syc00jwtwxs64e6nqv9exhmmjrlztcxkw9vgnjqev47qllp3ry.key
$ echo 'foo: bar' > secret.yaml
$ sops --age age1hhpw5e07syc00jwtwxs64e6nqv9exhmmjrlztcxkw9vgnjqev47qllp3ry -e secret.yaml > secret.enc.yaml
$ sops -d secret.enc.yaml
foo: bar

Things that I know are missing:

  • Tests
  • Documentation
  • Support for scrypt keys which would allow for password-derived keys (only X25519 is implemented right now)
  • Support for SSH keys (this is likely bigger than I'm interested in working on myself)
  • Ability to customize where sops looks for private key material.
  • A subcommand to generate a new key and place it in ~/.sops/age, which would remove the need for the manual steps I took in the example above.

Which of these things would be necessary to get basic support merged? I can't guarantee a lot of time to work on this so I'm hoping to scope it down as small as possible. Alternatively, if someone wants to use my branch as a starting point and take over, that'd be great.

@codecov-commenter
Copy link

codecov-commenter commented Jul 4, 2020

Codecov Report

Merging #688 into develop will decrease coverage by 3.22%.
The diff coverage is 10.12%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #688      +/-   ##
===========================================
- Coverage    36.74%   33.51%   -3.23%     
===========================================
  Files           22       23       +1     
  Lines         3217     3076     -141     
===========================================
- Hits          1182     1031     -151     
+ Misses        1916     1913       -3     
- Partials       119      132      +13     
Impacted Files Coverage Δ
config/config.go 64.39% <0.00%> (-7.04%) ⬇️
keyservice/keyservice.go 0.00% <0.00%> (ø)
keyservice/server.go 4.06% <0.00%> (-1.21%) ⬇️
stores/stores.go 0.00% <0.00%> (ø)
keyservice/keyservice.pb.go 6.02% <7.02%> (+1.89%) ⬆️
age/keysource.go 34.78% <34.78%> (ø)
pgp/keysource.go 30.25% <0.00%> (-3.83%) ⬇️
gcpkms/keysource.go 21.87% <0.00%> (-2.81%) ⬇️
aes/cipher.go 65.34% <0.00%> (-1.95%) ⬇️
stores/ini/store.go 27.52% <0.00%> (-1.80%) ⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 334be56...e9acafc. Read the comment docs.

age/keysource.go Outdated Show resolved Hide resolved
keyservice/server.go Outdated Show resolved Hide resolved
@Mic92
Copy link
Contributor

Mic92 commented Jul 6, 2020

I have not tested but from a basic code review it looks good to me. I am not affiliated with the project but I assume that the maintainers also want to see some unit tests before they merge this.

@autrilla
Copy link
Contributor

autrilla commented Jul 6, 2020

Thanks for the work :)

Two concerns:

  • As pointed out already, lack of tests
  • Key management: from a usability standpoint, this looks like it could be even worse than GPG to me. At least GPG has standardized locations to put the keys in. Right now, you're putting the keys in a sops-specific directory. Sharing keys with other people seems hard. I'm sure we get some security gains from AGE vs PGP, but still.

@Mic92
Copy link
Contributor

Mic92 commented Jul 6, 2020

Thanks for the work :)

Two concerns:

* As pointed out already, lack of tests

* Key management: from a usability standpoint, this looks like it could be even worse than GPG to me. At least GPG has standardized locations to put the keys in. Right now, you're putting the keys in a sops-specific directory. Sharing keys with other people seems hard. I'm sure we get some security gains from AGE vs PGP, but still.

My main motivation for having this is an alternative for deploying sops on servers in none-cloud environments (no azure, no aws, no GCP) because key generation/provision with GPG is overly complex:

  • gnupg does fancy things with your tty, has weird exit codes when doing scripting
  • It has a complex database where as with age one can just deploy a single file
  • gnupg requires a long scripts with under-documented options that you have to steal from stackoverflow where in age its a oneliner
  • gnupg always spawns a daemon which has to be killed manually in your $configurationmanagement (chef, puppet, ansible in my case nixos)
  • gnupg can have a lot of dependencies i.e. gui libraries where as age would be just a library inside sops (No, the embedded pgp is not enough as it breaks with gpg 2.1 databases)

Key sharing is actually something I don't want to do in this case because unlike gpg keys those are machines rather than persons.

@autrilla
Copy link
Contributor

autrilla commented Jul 6, 2020

I understand AGE has some benefits, and I'm unfortunately painfully aware of the issues with GPG.

Maybe documentation saying "put your AGE keys here" would be enough. I don't know if AGE has a standard location to put keys in. I want to have a better look at AGE before fully reviewing this PR regardless.

@Mic92
Copy link
Contributor

Mic92 commented Jul 6, 2020

I understand AGE has some benefits, and I'm unfortunately painfully aware of the issues with GPG.

Maybe documentation saying "put your AGE keys here" would be enough. I don't know if AGE has a standard location to put keys in. I want to have a better look at AGE before fully reviewing this PR regardless.

On servers one could even use the ssh host key, which is already present on most Linux servers. Also hosters like github already expose ssh public keys, which can be used: https://github.com/FiloSottile.keys

@jimmycuadra
Copy link
Contributor Author

The location of the private keys I chose is based on a similar pattern in the PR I was referencing, as noted in the description. The setup here is a little odd in that we don't save any sort of path or identifier for the private key in the encrypted file—it has to be derived from the public key somehow.

I suspect that age is intentionally agnostic on key management so that systems that use it can do whatever is most appropriate, but in our case it is unfortunate not to have a convention for where to find private keys that we could leverage. Perhaps @FiloSottile can share if there is any particular guidance on key management.

Also, I don't think age is an acronym, so it doesn't need to be capitalized. 😛

@Mic92
Copy link
Contributor

Mic92 commented Jul 9, 2020

Seems to be a bit relevant: #692

@jimmycuadra
Copy link
Contributor Author

jimmycuadra commented Jul 18, 2020

Made some changes:

  • Added support for specifying the directory where sops will look for age key files with the SOPS_AGE_KEY_DIR environment variable. If not specified, defaults to what it was before (~/.sops/age/$PUBLIC_KEY.key).
  • Platform-agnostic path handling for the key directory.
  • Some basic tests for encrypting and decrypting with an age key. I'm not thrilled with setting the environment variable in the test to point at the test key I created (and assuming what $PWD will be). Again I'm not very familiar with Go idioms so I'll need some advice on how to do this better. Normally I'd have the function accept the key dir as an argument, but the decrypt function is part of an interface I can't change.

@Mic92
Copy link
Contributor

Mic92 commented Jul 18, 2020

Made some changes:

* Added support for specifying the directory where sops will look for age key files with the `SOPS_AGE_KEY_DIR` environment variable. If not specified, defaults to what it was before (`~/.sops/age/$PUBLIC_KEY.key`).

* Platform-agnostic path handling for the key directory.

* Some basic tests for encrypting and decrypting with an age key. I'm not thrilled with setting the environment variable in the test to point at the test key I created (and assuming what `$PWD` will be). Again I'm not very familiar with Go idioms so I'll need some advice on how to do this better. Normally I'd have the function accept the key dir as an argument, but the decrypt function is part of an interface I can't change.

One question is there a good reason not to put ed25519 keys directly into environment variables? I assume they are as long as the actual fingerprints?

Nevermind these are private keys...

age/keysource.go Outdated Show resolved Hide resolved
age/keysource.go Outdated Show resolved Hide resolved
age/keysource_test.go Outdated Show resolved Hide resolved
age/keysource.go Outdated Show resolved Hide resolved
age/keysource.go Outdated
Comment on lines 107 to 114
for scanner.Scan() {
line := scanner.Text()

if strings.HasPrefix(line, "AGE-SECRET-KEY") {
privateKey = line
break
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to just copy this function: https://github.com/FiloSottile/age/blob/189041b668629795593766bcb8d3f70ee248b842/cmd/age/parse.go#L36

Then it would not just support the age format but also ssh keys like age does.
Same goes for encrypting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FiloSottile How does making the linked code available via age's library sound? Ideally this logic wouldn't be duplicated in sops or it could get out of sync with what age does over time.

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be the ideal case.

@Mic92
Copy link
Contributor

Mic92 commented Jul 27, 2020

I think the next thing upstream would ask for is to provide a section in the README. I can also help out here.

@jimmycuadra
Copy link
Contributor Author

Thanks for the review, @Mic92! That was very helpful. I fixed some of the smaller points, but I think it would make sense to see what Filippo says about opening up more functionality through the library before proceeding with the other changes.

@Mic92
Copy link
Contributor

Mic92 commented Jul 30, 2020

Thanks for the review, @Mic92! That was very helpful. I fixed some of the smaller points, but I think it would make sense to see what Filippo says about opening up more functionality through the library before proceeding with the other changes.

That makes sense. Maybe we can fix this upstream.

@FiloSottile
Copy link

Hmm good question. The key files format is more part of the age CLI than the API, but maybe it should be exposed. Give me a couple days to think about it.

@Mic92
Copy link
Contributor

Mic92 commented Jul 31, 2020

If fundamental parts like key location is not part of age api than will see inconsistency in the ecosystem on how age is integrated into applications.

@jimmycuadra
Copy link
Contributor Author

I could see it being a separate lib that sits in between the current lib and the CLI, so that it's possible to use the base library just for encryption and decryption without anything more. The intermediate "storage" library could be consumed by both the CLI and third-party software like sops that wants to use the same conventions for storing keys as the CLI.

@FiloSottile
Copy link

If fundamental parts like key location is not part of age api than will see inconsistency in the ecosystem on how age is integrated into applications.

I don't know sops enough to tell you about your use case in particular, but I expect most application that use age to keep separate keys. age doesn't have the concept of a "user key" that is used for all applications that use age, and I don't want that kind of overloading. If you rotate your sops key you shouldn't have to worry about your password manager or backups at the same time.

What we might want to expose is the key file format, not the location.

Can I ask why the interest in supporting SSH keys? age supports them so one can encrypt to someone who doesn't have an age key without waiting a round-trip for them to generate it. Here it looks like you could just generate an age key?

@Mic92
Copy link
Contributor

Mic92 commented Jul 31, 2020

If fundamental parts like key location is not part of age api than will see inconsistency in the ecosystem on how age is integrated into applications.

I don't know sops enough to tell you about your use case in particular, but I expect most application that use age to keep separate keys. age doesn't have the concept of a "user key" that is used for all applications that use age, and I don't want that kind of overloading. If you rotate your sops key you shouldn't have to worry about your password manager or backups at the same time.

The current workflow is that developers have a gnupg key that is distributed across the team (i.e. through public key server). On the server side there is some sort of key management service provided by some cloud provider. With age I hope to have a solution that lucks less than gnupg for users and also an alternative for non-cloud servers, which do not have access to an cloud-based vault.

What we might want to expose is the key file format, not the location.

Can I ask why the interest in supporting SSH keys? age supports them so one can encrypt to someone who doesn't have an age key without waiting a round-trip for them to generate it. Here it looks like you could just generate an age key?

If you have configuration shared in a team they are likely to have some sort of version control and hence you have public keys i.e. https://github.com/Mic92.keys This makes further key exchange unnecessary. The same is true for servers where configuration is deployed. Most of them already have ssh private host keys: i.e. /etc/ssh/ssh_host_ed25519_key This ssh key is already trusted and easy to obtain.

@ajvb
Copy link
Contributor

ajvb commented Jul 31, 2020

If fundamental parts like key location is not part of age api than will see inconsistency in the ecosystem on how age is integrated into applications.

I don't know sops enough to tell you about your use case in particular, but I expect most application that use age to keep separate keys. age doesn't have the concept of a "user key" that is used for all applications that use age, and I don't want that kind of overloading. If you rotate your sops key you shouldn't have to worry about your password manager or backups at the same time.

What we might want to expose is the key file format, not the location.

I think that keeping separate keys for sops is what I would want us to recommend in most use-cases. At the same time, that doesn't mean that there cannot be a set default "age private key directory" where all of your keys are kept, even if it's 1 or more key per application.

@jimmycuadra
Copy link
Contributor Author

age doesn't have the concept of a "user key" that is used for all applications that use age, and I don't want that kind of overloading.

What we might want to expose is the key file format, not the location.

This sounds good to me. Not having a direct equivalent to a PGP key is a good thing about age, and we shouldn't be trying to shoehorn that back in for sops usage. It makes sense to me to have a sops-specific directory to store age keys that are used specifically and only for the purpose of sops. It'd definitely be nice to have a standard key file format, though!

Copy link

@FiloSottile FiloSottile left a comment

Choose a reason for hiding this comment

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

Thank you all for working on this and for your patience. I added a section to the age docs about key management, which matches what you are doing and recommends not adding support for SSH keys at all.

https://pkg.go.dev/filippo.io/[email protected]#hdr-Key_management

I also added ParseIdentities to make it easier to parse the private key file.

age/keysource.go Outdated Show resolved Hide resolved
age/keysource.go Outdated Show resolved Hide resolved
@jimmycuadra
Copy link
Contributor Author

Updated to the latest age beta and made changes suggested by Filippo. Still looking for a sops maintainer with time for final review and hopefully merge!

@autrilla
Copy link
Contributor

LGTM. I'll give it a couple of days to see if @ajvb has time to review, otherwise I'll just merge.

@ajvb
Copy link
Contributor

ajvb commented Sep 22, 2020

🎉 This is really exciting! I should be able to get to a review sometime tomorrow.

@ajvb
Copy link
Contributor

ajvb commented Sep 23, 2020

Thank you so much @jimmycuadra, @FiloSottile, @Mic92, and @colemickens - very exciting to get this in here.

@ajvb ajvb merged commit 682bff4 into getsops:develop Sep 23, 2020
@jimmycuadra jimmycuadra deleted the age branch September 24, 2020 07:54
@ajvb ajvb mentioned this pull request Mar 24, 2021
@dol
Copy link

dol commented Mar 25, 2021

@jimmycuadra Thank you for the age integration. I'll will test it soon. ❤️

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.

8 participants