-
Notifications
You must be signed in to change notification settings - Fork 855
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
age-plugin support #1465
base: main
Are you sure you want to change the base?
age-plugin support #1465
Conversation
54c64d5
to
aa8b289
Compare
fd06c45
to
5d8afa9
Compare
Signed-off-by: Jörg Thalheim <joerg@thalheim.io>
What work needs to be done still to get age plugin support merged? |
Ideally an age release unless sops is ok with pointing to a master release of age. |
The issue creation flow at the age repository points into discussion threads; FiloSottile/age#562 |
FYI: this works perfectly fine in my nixos configs: essentially just applied this PR without @Mic92 could you rebase? |
@Mic92 any progress on this? I tried looking at implementing support for TPM keyfiles directly, but I suspect utilizing |
@Foxboron the integration works but I haven't received any feedback from upstream, so a rebase is not high on my priority list |
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.
Thanks for your contribution! There are two things that make me worry:
- Quite a lot of the code here is basically copied from https://github.com/FiloSottile/age/blob/main/cmd/age/tui.go and https://github.com/FiloSottile/age/blob/main/cmd/age/parse.go, without indicating that the code originates from there. While it's in some cases not feasible to implement something quite differently (like
parseIdentity()
), in other cases this is a problem and basically violation of the age license (https://github.com/FiloSottile/age/blob/main/LICENSE). - There is quite some age-specific code in here that would be better suited to be kept as part of age itself, or as a library on top of age that makes that code available for other users (like sops). Having to maintain these parts here doesn't sound like a good idea to me. (In particular since they are basically a copy of the code from age, see 1.)
} else if tty, err := os.OpenFile("/dev/tty", os.O_RDWR, 0); err == nil { | ||
defer tty.Close() | ||
return f(tty, tty) | ||
} else if term.IsTerminal(int(os.Stdin.Fd())) { |
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.
Out of curiosity, why isn't this the first if
at the top of this function?
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.
Because /dev/tty is preferred over standard file descriptor. The standard file descriptor might be not connected to a tty.
func readSecret(prompt string) (s []byte, err error) { | ||
err = withTerminal(func(in, out *os.File) error { | ||
fmt.Fprintf(out, "%s ", prompt) | ||
defer clearLine(out) |
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.
Why having clearLine
here and in readCharacter
? Why not keep the prompt and simply continue in the next line (maybe after adding some placeholder, like (...)
)? In the existing code in SOPS that reads a password from the terminal (in the PGP keysource) it doesn't clear the prompt either.
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.
My thinking was here that most age plugins are most likely only tested with age as the implementation, I don't think it's a good idea to diverge to much from the original behavior.
func readCharacter(prompt string) (c byte, err error) { | ||
err = withTerminal(func(in, out *os.File) error { | ||
fmt.Fprintf(out, "%s ", prompt) | ||
defer clearLine(out) |
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.
Same here. If programs expect a yes/no (or generic one-letter input) on the terminal they usually echo the final selection and go to the next line, instead of removing the prompt.
return | ||
} | ||
|
||
// readCharacter reads a single character from the terminal with no echo. The |
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.
Why read the character without echo?
return nil, fmt.Errorf("failed to parse input as Bech32-encoded age public key: %w", err) | ||
func parseRecipient(recipient string) (age.Recipient, error) { | ||
switch { | ||
case strings.HasPrefix(recipient, "age1") && strings.Count(recipient, "1") > 1: |
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.
TBH this condition strings.Count(recipient, "1") > 1
looks rather strange to me. This looks like an implementation detail of age that programs using age as a library really shouldn't know about.
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.
Because the plugins use 'age1someplugin1' as a prefix. This is the same logic as the age cli. Do you think we should reject prefixes that the age cli accepts?
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.
No, I'm mainly saying that this looks like things that the age code should check, and not our code.
This is trivially solved by using REUSE. I don't think the license is the main problem. |
Updated version of #1335
I wrote the plugin integration in the first place.