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

plugin: new framework to implement plugins #580

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

Conversation

FiloSottile
Copy link
Owner

No description provided.

Copy link

@AnomalRoil AnomalRoil left a comment

Choose a reason for hiding this comment

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

I'm not entirely done with my tests but I can confirm both the standard and the interactive mode worked for me.

// HandleRecipient registers a function to parse recipients of the form
// age1name1... into [age.Recipient] values. data is the decoded Bech32 payload.
//
// It must be called before [Plugin.Main], and can be called at most once.

Choose a reason for hiding this comment

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

Suggested change
// It must be called before [Plugin.Main], and can be called at most once.
// It must be called before [Plugin.Main], and can be called at most once, it panics otherwise.

// form AGE-PLUGIN-NAME-1... into [age.Recipient] values, for when identities
// are used as recipients. data is the decoded Bech32 payload.
//
// It must be called before [Plugin.Main], and can be called at most once.

Choose a reason for hiding this comment

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

Suggested change
// It must be called before [Plugin.Main], and can be called at most once.
// It must be called before [Plugin.Main], and can be called at most once, it panics otherwise

In general I think calling out panic in doc is a good idea, no ?

@AnomalRoil
Copy link

Okay, a first set of issues when trying to use plugin when using age as a library willing to support arbitrary age plugin recipients:

  • the currently exposed age.ParseRecipient and age.ParseIdentity are both only returning ones of type *X25519Identity only.
  • there exist a parseIdentities function supporting plugin ones, but it seems the only way to use it is to rely on EncryptedIdentities and there's no such things for recipients.

The rest of the library code is fairly agnostic of whether this is a plugin recipient or identity, so having an easy way of parsing plugin identities would be nice.

Currently #518 is providing a way to detect "regular" identities from plugin identities through error handling, which could work as well but isn't as convenient I guess, although it's more in line with the rest of the currently exposed parsing functions (one is supposed to parse SSH vs age recipients and identities using the specific exported functions for each currently, meaning everyone ends up re-implementing this).

In other notes:

  • the plugin.NewIdentity and plugin.NewRecipient are both expecting a plugin.ClientUI but there isn't any exported "default" ClientUI. This might be a decision to force people to implement their own?
  • The existing plugin.ClientUI interface does not expect any context to be passed to any of the prompts, whereas a library would be typically carrying these around, especially for such prompts that we might want to timeout on. It might be good to refactor the ClientUI interface to expect contexts in each of the function calls?

@olastor
Copy link

olastor commented Jul 25, 2024

It works great in my tests so far 🎉

One thing I noticed though is that the age client that communicates with the plugins currently still seems to spawn separate processes. For example, age -d -i identities.txt -o test somefile.enc (two identities in identities.txt) will do something like this (the binary I used is v1.2.0, but I think it would be the same case with this branch):

-> add-identity AGE-PLUGIN-SSS-1R79SSQQQQQQQQQ8LQNQVZZKZXQX
-> grease-1331f5cec305b37b

-> recipient-stanza 0 X25519 DsW/n
xxxx
-> done

-> msg
SGFuZGxlZCBpZGVudGl0eSBpbiAxNzEzNQ
age: sss plugin: Handled identity in 17135
-> ok

-> grease-7cada51a126db013 78
gBr1Q7wtAl4/a8vPQe/OLdZyc0z5hVVuf06OuIxigh7vNBlTn07r8PTPCqAE0bN3
ZaljVNGkPNxFyYgaIwrejyID6rk
-> unsupported

-> done

-> add-identity AGE-PLUGIN-SSS-1R79SSQQQQQQQQQ8L4FTV5NPF26

-> grease-236eb6a331bae717

-> recipient-stanza 0 X25519 DsW
xxx
-> done

-> msg
SGFuZGxlZCBpZGVudGl0eSBpbiAxNzE0MQ
age: sss plugin: Handled identity in 17141
-> ok

-> done

age: error: no identity matched any of the recipients

The debug messages I added show that each identity is handled in a different process/pid (each also gets the same stanza separately), so I think the recipients/identities arrays might currently not get longer than 1. It works, but sorting identities won't be possible like this. This is basically #526 , which I closed because it seemed to be intentional at the time. I think having only one plugin process with all the data could open more opportunities (more advance logic, ordering, maybe batching encrypts/decrypts in one request/operation, caching pins).

@AnomalRoil
Copy link

AnomalRoil commented Oct 2, 2024

After further testing in gopass, I can confirm that for a library trying to support Plugin Recipients and Identities, the current interfaces are not enough yet:

  • I had to copy the parseIdentity and parseIdentities functions to support plugin identities from cmd/age/parse.go
  • I had to copy the func identitiesToRecipients(ids []age.Identity) ([]age.Recipient, error) from cmd/age/age.go
  • I had to create my own wrappedIdentity in order to have a way to output the encoding of an identity, it might be best to provide a way of serializing identities easily? (Maybe implement the Stringer interface for both plugin.Identity and plugin.Recipient?)
  • While plugin.Identity has a func (i *Identity) Recipient() *Recipient, it sadly can easily contains secret key material, since it basically contains the plugin.Identity, it would be nice to have a mapping from plugin.Identity to plugin.Recipient that is "safe", but this probably requires changing the plugin spec and interface to add a new type of interaction, so might not be ideal.

On the plus side:

  • implementing my own terminalUI was very easy with the current ClientUI, my previous comment about passing it contextes still holds since I had to put context.Background() everywhere in mine for now.
  • just passing the plugin identities to age.Decrypt does work nicely.

One last comment for now, it might be good in age.Decrypt, when ranging over stanzas/identities to first sort them to prioritize native identities over plugin ones?

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.

3 participants