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

feat: add age plugin support #1641

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

brianmcgee
Copy link

@brianmcgee brianmcgee commented Oct 2, 2024

Another attempt at #1465 without bringing in so much code from age.

Instead, I created FiloSottile/age#591 upstream to expose PluginTerminalUI.

TODO

- [ ] update go.mod once FiloSottile/age#591 is merged

@felixfontein felixfontein marked this pull request as draft October 2, 2024 17:16
@felixfontein
Copy link
Contributor

Thank you very much for this! I've marked it as a draft (we can put it back to ready once the upstream change is merged).

@felixfontein felixfontein added this to the 3.10.0 milestone Oct 2, 2024
@Mic92 Mic92 mentioned this pull request Oct 3, 2024
go.mod Outdated
@@ -134,3 +134,5 @@ require (
google.golang.org/genproto/googleapis/api v0.0.0-20240903143218-8af14fe29dc1 // indirect
gopkg.in/yaml.v2 v2.4.0 // indirect
)

replace filippo.io/age => github.com/brianmcgee/age v0.0.0-20241002093043-152b6edfe56a
Copy link

@loa loa Oct 3, 2024

Choose a reason for hiding this comment

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

I guess this slipped in (or required to get the tests to pass), thanks for pushing this forward @brianmcgee !

Copy link
Author

Choose a reason for hiding this comment

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

That's what the todo is about above. Once upstream merges, I can remove that.

@Ma27
Copy link

Ma27 commented Nov 20, 2024

Using this, works fine for sops -d. However if I specify public keys for creation_rules in a .sops.yaml, sops will break when editing these files since parseRecipients doesn't know what to do with age-plugin-yubikey pubkeys.

I fixed this with the following patch on top:

From 64e77bd60f8ebc8a0a5c7f8602a6f5855c892fd3 Mon Sep 17 00:00:00 2001
From: Maximilian Bosch <maximilian@mbosch.me>
Date: Wed, 20 Nov 2024 22:44:49 +0100
Subject: [PATCH] age/keysource: parse recipients using plugin system

Otherwise I get

    failed to parse input as Bech32-encoded age public key: malformed recipient "age1yubikey1...": invalid type "age1yubikey"

for `sops secrets/.../secrets.sops.yaml` in a directory with a
`.sops.yaml` that has creation rules with age1yubikey1* keys in its
creation rules.
---
 age/keysource.go | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/age/keysource.go b/age/keysource.go
index f04e4aff8..a9051c926 100644
--- a/age/keysource.go
+++ b/age/keysource.go
@@ -304,12 +304,21 @@ func (key *MasterKey) loadIdentities() (ParsedIdentities, error) {
 
 // parseRecipient attempts to parse a string containing an encoded age public
 // key.
-func parseRecipient(recipient string) (*age.X25519Recipient, error) {
-	parsedRecipient, err := age.ParseX25519Recipient(recipient)
-	if err != nil {
-		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:
+		parsedRecipient, err := plugin.NewRecipient(recipient, tui.PluginTerminalUI)
+		if err != nil {
+			return nil, fmt.Errorf("failed to parse input as age key from age plugin: %w", err)
+		}
+		return parsedRecipient, nil
+	default:
+		parsedRecipient, err := age.ParseX25519Recipient(recipient)
+		if err != nil {
+			return nil, fmt.Errorf("failed to parse input as Bech32-encoded age public key: %w", err)
+		}
+		return parsedRecipient, nil
 	}
-	return parsedRecipient, nil
 }
 
 // parseIdentities attempts to parse the string set of encoded age identities.
-- 
2.47.0

Feel free to pick this to your branch @brianmcgee :)

@brianmcgee brianmcgee force-pushed the feat/age-plugins branch 3 times, most recently from 81f1b4a to 0607eae Compare November 21, 2024 09:03
@brianmcgee
Copy link
Author

@Ma27 applied the patch, thanks 👍

@visualphoenix
Copy link

+1 would be great to have this in sops

@felixfontein
Copy link
Contributor

Since FiloSottile/age#591 doesn't seem to move forward at all (there isn't even any comment by the age maintainer, not even something like "this willl never happen" or "I still have to think about this"), so I think we have to proceed differently.

I just merged #1692, which actually vendors some of the code from https://github.com/FiloSottile/age/pull/591/files (basically ReadSecret with WithTerminal inlined). I think we should proceed similarly here, by modifying the tui.go added there to basically contain https://github.com/FiloSottile/age/blob/266c0940916da6bfa310fdd23156fbd70f9d90a0/tui/tui.go (resp. its original form).

I'd like to get #1400 resolved before merging this one, which can also use the readPassphrase function added by #1692. That way all three PRs (#1692, #1400, and this one) can share the same code vendored from age.

WDYT?

@visualphoenix
Copy link

I dream of a day I could use https://github.com/Foxboron/ssh-tpm-agent with sops…

@Foxboron
Copy link
Contributor

Foxboron commented Feb 21, 2025

@felixfontein An alternative is for sops to use the filippo/plugin branch as it contains the required changes for this to work.

I've moved this over in age-plugin-tpm and considering the low velocity of the project I suspect it should be fine.

EDIT: Reading more of the code. I don't think it solves anything :/

@brianmcgee
Copy link
Author

Since FiloSottile/age#591 doesn't seem to move forward at all (there isn't even any comment by the age maintainer, not even something like "this willl never happen" or "I still have to think about this"), so I think we have to proceed differently.

Agreed. I think the approach you laid out seems sensible.

@felixfontein
Copy link
Contributor

I've merged the other PR (as well as some dependabot PRs).

@brianmcgee
Copy link
Author

Ok cool, I'll have a look at rebasing this one today.

Signed-off-by: Brian McGee <brian@bmcgee.ie>
Co-authored-by: Maximilian Bosch <maximilian@mbosch.me>
@brianmcgee
Copy link
Author

@felixfontein I manually tested this with the age-plugin-fido2-hmac plugin.

@brianmcgee brianmcgee marked this pull request as ready for review February 25, 2025 12:30
@Ramblurr
Copy link

I'd like to test this PR too! Is this feature added transparently to the user? Or are there flags I need to pass to make it work?

@brianmcgee
Copy link
Author

I'd like to test this PR too! Is this feature added transparently to the user? Or are there flags I need to pass to make it work?

It should be relatively transparent, you just need to pass an appropriate age key per the plugin you're using.

Here's an example of how I'm using it with age-plugin-fido2-hmac:

SOPS_AGE_KEY=$(age-plugin-fido2-hmac -m) sops test.yml

My .sops.yaml file contains a key entry of the form age1fido2-hmac1xxxxxxxxxxx.

@felixfontein
Copy link
Contributor

I'll take a closer look at this during the next days. The issues reported by CodeQL can likely be ignored (I think they're the same as in #1692, I'll have to take a cloesr look).

@felixfontein felixfontein requested a review from a team February 25, 2025 21:16
// WithTerminal runs f with the terminal input and output files, if available.
// WithTerminal does not open a non-terminal stdin, so the caller does not need
// to check stdinInUse.
func WithTerminal(f func(in, out *os.File) error) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you replace the copy of this function's body in readPassphrase() with a call to WithTerminal?

@@ -1,5 +1,6 @@
// These functions have been copied from the age project
// https://github.com/FiloSottile/age/blob/v1.0.0/cmd/age/encrypted_keys.go
// https://github.com/FiloSottile/age/blob/266c0940916da6bfa310fdd23156fbd70f9d90a0/tui/tui.go
Copy link
Contributor

Choose a reason for hiding this comment

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

Commit 266c0940916da6bfa310fdd23156fbd70f9d90a0 is not part of FiloSottile/age. Can you use https://github.com/FiloSottile/age/blob/3d91014ea095e8d70f7c6c4833f89b53a96e0832/cmd/age/tui.go instead (and also replace the code with the code from that commit)? It is also preferable to use lower-case identifiers (as in the original tui.go) so that the symbols defined in here are private to SOPS' age package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants