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

support passphrase callback #3

Closed
wants to merge 1 commit into from

Conversation

vrothberg
Copy link

Add a SetPassphrase(string) method to create a passphrase callback
using the specified string as passphrase to prevent gpg from prompting
the user.

Signed-off-by: Valentin Rothberg rothberg@redhat.com

Add a `SetPassphrase(string)` method to create a passphrase callback
using the specified string as passphrase to prevent gpg from prompting
the user.

Signed-off-by: Valentin Rothberg <rothberg@redhat.com>
@vrothberg
Copy link
Author

Skopeo PR: containers/skopeo#1540

Copy link
Owner

@mtrmac mtrmac 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 sure any of this is truly necessary.

Either way, this repo is only a friendly fork of https://github.com/proglottis/gpgme that exists only because of proglottis#6 , and I’d very much prefer not to deviate from the upstream repo any further. So I think any new functionality should be proposed there.

#include <errno.h>
#include <string.h>

gpgme_error_t passphrase_cb (void *opaque, const char *uid_hint, const char *passphrase_info, int last_was_bad, int fd) {
Copy link
Owner

Choose a reason for hiding this comment

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

The callback should probably fail if last_was_bad instead of trying again.

callback := C.gpgme_passphrase_cb_t(C.passphrase_cb)
cPass := C.CString(passphrase)
C.gpgme_set_pinentry_mode(c.ctx, C.GPGME_PINENTRY_MODE_LOOPBACK);
C.gpgme_set_passphrase_cb(c.ctx, callback, unsafe.Pointer(cPass))
Copy link
Owner

Choose a reason for hiding this comment

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

These are all unsafe WRT CGo rules, per proglottis#23 .

There already is a Context.SetCallback exposing gpgme_set_passphrase_cb (and see how different that implementation needs to be), isn’t that sufficient already?

To the extent actually using SetCallback with a fixed string requires some code that could be widely reused, it might well make sense to provide that in this gpgme package, but I think it would be cleaner to just wrap SetCallback instead of reimplementing it, and it’s up to the upstream maintainer whether to add it.

logrus.Debugf("Setting GPGME passphrase callback", passphrase)
callback := C.gpgme_passphrase_cb_t(C.passphrase_cb)
cPass := C.CString(passphrase)
C.gpgme_set_pinentry_mode(c.ctx, C.GPGME_PINENTRY_MODE_LOOPBACK);
Copy link
Owner

Choose a reason for hiding this comment

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

This won’t compile with the GPGME 1.3.2 version this repo is currently targeting.

We should almost certainly just give up on that version, and move on, sure. If I’m reading https://access.redhat.com/downloads/content/package-browser correctly, neither of the changes in this repo are necessary for RHEL ≥ 8.

Copy link
Owner

Choose a reason for hiding this comment

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

I.e., to be a bit more explicit, if I’m not mistaken about the supported versions, we can just revert c/image to use the upstream proglottis/gpgme package, and deprecate/abandon this fork.

@vrothberg
Copy link
Author

Either way, this repo is only a friendly fork of https://github.com/proglottis/gpgme that exists only because of proglottis#6 , and I’d very much prefer not to deviate from the upstream repo any further. So I think any new functionality should be proposed there.

Are you proposing to just move to https://github.com/proglottis/gpgme entirely? As you mentioned, it already support callbacks.

@mtrmac
Copy link
Owner

mtrmac commented Jan 19, 2022

Are you proposing to just move to https://github.com/proglottis/gpgme entirely?

I think that would be cleanest overall, but I didn’t research whether any relevant distribution is still holding us back to support older GPGME versions, past that bit on the RHEL package browser.

@vrothberg
Copy link
Author

I like that proposal very much! I will browse a bit around and see what other distributions ship but I think they'll be more recent than RHEL 7.

@vrothberg
Copy link
Author

Closing this PR as it is not needed anymore.

@vrothberg vrothberg closed this Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants