age-plugin support #1465
age-plugin support #1465
Conversation
Mic92
force-pushed
the
age-plugins
branch
2 times, most recently
from
54c64d5
to
aa8b289
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Mic92
force-pushed
the
age-plugins
branch
2 times, most recently
from
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.
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.
Out of curiosity, why isn't this the first if at the top of this function?
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
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.