-
Notifications
You must be signed in to change notification settings - Fork 879
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 SSH support for age #1134
base: main
Are you sure you want to change the base?
Add SSH support for age #1134
Conversation
Thanks for submitting. Took it for a spin; works splendidly over here. Here's approximately what I used to test: #!/bin/bash
# review https://github.com/mozilla/sops/pull/1134
set -euo pipefail
# Make tempdir for test
d="$(mktemp -d)"
cd "$d" || exit 1
ssh_pubkey="$(cat ~/.ssh/id_ed25519.pub)"
cat <<END > .sops.yaml
---
creation_rules:
- age: "$ssh_pubkey"
END
echo "Created sops config:"
cat .sops.yaml
cat <<END2 > example.yaml
---
foo: bar
END2
echo "Created vars file:"
cat example.yaml
sops -e -i example.yaml
echo "Encrypted vars file via ssh pubkey:"
cat example.yaml
echo "Attempting decryption of vars file:"
sops -d example.yaml
echo "SUCCESS!" Tried it with a 4096-bit RSA key, and that worked fine, too. Would love to see this PR land. |
Hi! |
Hey, I would also be interested in using ssh key :) |
Thank you for your contribution! 🥑 Due to the size of this PR, I have scheduled it for next-next minor. As we do not want to unleash a flood of issues do to newly introduced functionalities on us while we have just taken over the project. I'll come back to this once In the meantime, it would be helpful if you could rebase your work and sign-off your commits. Thanks! 🙇 |
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.
It took me some time (and I hope you are still around, @mstrangfeld!), but here is an initial round of review. Overall, it appears to be headed in the right direction, except for some minor nits and picks around details.
Thanks a lot for spending time on this, it appears to be the most requested thing at the moment. I promise my next review won't take this long. 🍇
// SopsAgeKeyUserConfigPath). It will load all found references, and expects | ||
// at least one configuration to be present. | ||
// SopsAgeSshPrivateKeyEnv, SopsAgeKeyUserConfigPath). It will load all | ||
// found references, and expects at least one configuration to be present. | ||
func (key *MasterKey) loadIdentities() (ParsedIdentities, 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.
As this is a method, it should continue to be above the newly introduced private functions.
age/keysource.go
Outdated
sshKeyFilePath, ok := os.LookupEnv(SopsAgeSshPrivateKeyEnv) | ||
if ok { | ||
return getAgeSshIdentityFromPrivateKeyFile(sshKeyFilePath) | ||
} | ||
|
||
userHomeDir, err := os.UserHomeDir() | ||
if err != nil || userHomeDir == "" { | ||
log.Warnf("could not determine the user home directory: %v", err) | ||
return nil, nil | ||
} | ||
|
||
sshEd25519PrivateKeyPath := filepath.Join(userHomeDir, ".ssh", "id_ed25519") | ||
if _, err := os.Stat(sshEd25519PrivateKeyPath); err == nil { | ||
return getAgeSshIdentityFromPrivateKeyFile(sshEd25519PrivateKeyPath) | ||
} | ||
|
||
sshRsaPrivateKeyPath := filepath.Join(userHomeDir, ".ssh", "id_rsa") | ||
if _, err := os.Stat(sshRsaPrivateKeyPath); err == nil { | ||
return getAgeSshIdentityFromPrivateKeyFile(sshRsaPrivateKeyPath) | ||
} | ||
|
||
return nil, nil |
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.
AFAIK, age lazily attempts to make use of the keys. Which makes me wonder if it would not be better to collect any found key which would match the behavior of loadIdentities()
.
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.
If so, it may be better to collect existing paths here and delegate the loading of them to loadIdentities()
. Which would iterate over the returned paths, while calling getAgeSshIdentityFromPrivateKeyFile()
.
Hi @hiddeco, thanks for the review! I will rebase, sign-off the commits and try to implement your suggestions! |
I hate to look like I'm trying rush this, but I'm really just very excited for this feature! 😅 So if I could just bump this...? |
Reasonable behavior. What was a bit confusing for me was just when I've set SOPS_AGE_SSH_PRIVATE_KEY=file.key it automatically assumed there is also a |
Signed-off-by: Marvin Strangfeld <[email protected]>
I finally had some time to do this. I am no longer actively using sops at the moment, so if anyone else wants to take over from here, please feel free to use and modify my code as you wish. |
I wrote a blog post on using sops with ssh keys and github, might be relevant to someone here. https://taras.glek.net/post/github-to-sops-lighter-weight-secret-management/ |
Hi @hiddeco , would it be possible to take a look at this PR once again? Maybe it would make sense to merge this now and then work on any potential refactoring in future versions, so that we already have something that's working that we can use. |
@overfl0 You can convert ssh-ed25519 key using |
This is a valuable feature - +1 - it would be great to see it merged and usable. |
@felixfontein what would be needed for this PR to be merged? It has had milestone tags changed for a while, without any comment whether it's just waiting for someone to look at it and it's purely due to the lack of time or if some additional work is needed. I'm tagging you because hiddeco seems to have stopped contributing in February. |
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.
@overfl0 I guess someone needs to reboot this PR by creating a new one and resolving conflicts. I personally don't have very much time, but I can try to give this another review, but it probably makes more sense to wait for a new PR (one comment below which I noticed while glancing over it).
Right now (well, I guess since the beginning of this year) it seems that the maintainer team has very limited time, so not much happened... I tried to get 3.9.0 out in summer with all changes done until then (and some more that should really go in); everything that wasn't looking like it will get done very very soon at that point got moved to the next milestone.
@@ -24,6 +26,9 @@ const ( | |||
// SopsAgeKeyFileEnv can be set as an environment variable pointing to an | |||
// age keys file. | |||
SopsAgeKeyFileEnv = "SOPS_AGE_KEY_FILE" | |||
// SopsAgeSshPrivateKeyEnv can be set as an environment variable pointing to | |||
// a private SSH key file. | |||
SopsAgeSshPrivateKeyEnv = "SOPS_AGE_SSH_PRIVATE_KEY" |
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 would include File
here to avoid possible confusion with the contents of the private key.
SopsAgeSshPrivateKeyEnv = "SOPS_AGE_SSH_PRIVATE_KEY" | |
SopsAgeSshPrivateKeyEnvFile = "SOPS_AGE_SSH_PRIVATE_KEY_FILE" |
Thanks for your response @felixfontein ! I'm sure that if you acknowledge that PR and say there that there is an actual chance of it being merged in, that people will then be willing to help and iron it out. |
This MR implements #692
I realize there is already a MR #898 but it implements the ssh encryption as a completely new method.
I just patched the existing age module to also support ssh recipients and identities.
The behavior is the same as the original MR:
If there is no
SOPS_AGE_SSH_PRIVATE_KEY
env variable given, sops will check~/.ssh/id_ed25519
and fallbacks to~/.ssh/id_rsa
.Would love to hear some thoughts on this.