-
-
Notifications
You must be signed in to change notification settings - Fork 568
plugin: new framework to implement plugins #580
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
base: main
Are you sure you want to change the base?
Conversation
f84d0b5
to
0fbe2ac
Compare
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'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. |
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 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. |
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 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 ?
Okay, a first set of issues when trying to use
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:
|
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,
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). |
After further testing in
On the plus side:
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? |
I'm not sure if it is relevant to the scope of this PR, but one thing I think would be useful would be plugin support for |
@FiloSottile
I think all of the "bumps on the road" I had using this framework can be solved by additive improvements that could come later on, such as:
|
if s.Type == "fail" { | ||
return "", fmt.Errorf("client failed to request value") | ||
} | ||
if err := expectStanzaWithBody(s, 0); err != 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.
I'd argue that a common pattern in a CLI is to say "Please provide XYZ or press enter to use the default value", so expecting a Stanza with a non-0 length body is not always practical.
At least it did bite me in my testing:
-> request-public
cGxlYXNlIHByb3ZpZGUgdGhlIGNoYWluaGFzaCBvZiB0aGUgbmV0d29yayB5b3Ug
d2FudCB0byB3b3JrIHdpdGggKGFuIGVtcHR5IHZhbHVlIHdpbGwgdXNlIHRoZSBk
ZWZhdWx0IG9uZSk
please provide the chainhash of the network you want to work with (an empty value will use the default one)
this felt safe, but isn't supported with the current implementation.
I'm looking to implement an age plugin but a bit confused about what state the support is in, the docs on pkg.dev don't show really how to implement one. Am I correct in saying this PR needs to be merged for support for implementing age plugins in Go? |
When I call Also, when I press enter without entering anything I get this:
|
No description provided.