fix input/output streams for gpg validation #1057

cdoern · 2022-06-02T20:28:51Z

currently, our gpg function which uses exec.Command to gpg --encrypt and
gpg --decrypt does not allow for stdin or out to be anything but a buffer stream directly used in the program
this causes gpg to error out since it cannot ask for validation on locked keys.
Fix this by passing stdin to --decrypt as the "in" var and stdout as the "out" var to --encrypt

resolves containers/podman#13539

Signed-off-by: cdoern cdoern@redhat.com

openshift-ci · 2022-06-02T20:28:57Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cdoern
To complete the pull request process, please assign baude after the PR has been reviewed.
You can assign the PR to them by writing /assign @baude in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cdoern · 2022-06-02T20:29:01Z

@ashley-cui PTAL

ashley-cui

Code LGTM! Just seeing if we can test with a locked gpg key. Going to have to trust you on the hardware token part since I don't have a hardware key

ashley-cui · 2022-06-02T20:32:03Z

pkg/secrets/passdriver/passdriver_test.go

@@ -28,6 +28,9 @@ func setupDriver(t *testing.T) (driver *Driver, cleanup func()) {
 	err = driver.gpg(context.TODO(), nil, nil, "--batch", "--passphrase", "--quick-generate-key", "testing@passdriver")
 	require.NoError(t, err)

+	err = driver.gpg(context.TODO(), os.Stdin, os.Stderr, "--batch", "--passphrase", "--quick-generate-key", "testing@passdriver")
+	require.NoError(t, err)
+


Is there a way to test this with a key that has a password?

yeah, I was not sure how to approach this. if we could somehow ensure that pass is installed i could test it really easily but it was not installed on my fedora machine. Is there a way to dnf install stuff in these tests?

cc/ @edsantiago

I think you don't need pass installed for the passdriver to work, only gpg. Maybe look into generating a key that has a password? And maybe a gpg flag to pass in the password when decrypting the key?

I don't understand the full context here, but (1) no you can't dnf-install, (2) have you considered --passphrase-file?

cdoern · 2022-06-03T13:57:29Z

@ashley-cui PTAL I think thats the most applicable test I can make for this.

edsantiago · 2022-06-06T14:29:47Z

pkg/secrets/passdriver/passdriver_test.go

@@ -25,12 +25,15 @@ func setupDriver(t *testing.T) (driver *Driver, cleanup func()) {
 	})
 	require.NoError(t, err)

-	err = driver.gpg(context.TODO(), nil, nil, "--batch", "--passphrase", "--quick-generate-key", "testing@passdriver")
+	f, err := ioutil.TempFile("", "pass.txt")


Sorry, I'm just taking a quick look. Did you mean to actually write some sort of passphrase into the file? If so have I missed where that happens?

thanks @edsantiago I think I missed a line here. I will circle back to this today

currently, our gpg function which uses exec.Command to `gpg --encrypt` and `gpg --decrypt` does not allow for stdin or out to be anything but a buffer stream directly used in the program this causes `gpg` to error out since it cannot ask for validation on locked keys. Fix this by passing stdin to --decrypt as the "in" var and stdout as the "out" var to --encrypt resolves containers/podman#13539 Signed-off-by: cdoern <cdoern@redhat.com>

edsantiago · 2022-06-13T18:43:48Z

CI failure seems persistent (i.e., not a flake)

rhatdan · 2022-06-21T18:33:45Z

@cdoern whats up with this one?

cdoern · 2022-06-21T20:22:46Z

I am working on this one now, trying to figure out how to give the TestLookup test a dummy stdin

cdoern · 2022-06-21T20:50:48Z

@edsantiago any idea here how to get the test to acknowledge stdin? I tried just writing "123" to a file and setting os.Stdin = f but that just times out and the commands do not have the proper input

edsantiago · 2022-06-21T20:56:47Z

@cdoern I can't see the code you're referring to; I assume it's unpushed and will guess that your question is: "how do I run gpg, from within go, such that its stdin is something of my choice". I don't know enough go to answer that, but the traditional approach has been to fork(), switch filehandles (e.g. reopen os.Stdin), then exec(). There is almost certainly an idiomatic go way to do this but that's a question for those more literate.

cdoern · 2022-06-21T20:58:03Z

@edsantiago sorry for the vague question. I figured out a way to modify the code here to pass lookup a byte array and if that is nil, default to stdin.

cdoern · 2022-06-22T14:26:36Z

been working on this for a bit, keep walking in circles with trying to wire up the stdin/out @ashley-cui is there any reason we would not want actual stdin/out here? the code seems to be fighting me a lot on this change.

ashley-cui · 2022-06-22T20:38:47Z

I'm not sure, since it looks like the gpg() function is just a cmd.exec, so it makes sense that. a regular os.stdin/os.stdout would work. Have you looked at if its specifically stdin or stdout that's not behaving, or do both have issues?

openshift-merge-robot · 2022-07-23T11:00:13Z

@cdoern: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mtrmac · 2022-07-30T01:34:05Z

@cdoern FWIW I wouldn’t expect just redirecting stdin to typically work. GPG is not just reading from stdin, it has a concept of pinentry which intends to talk to the user regardless of stdin. That might be using /dev/tty, or perhaps even popping a GUI window completely outside of any terminal.

That behavior needs to be explicitly opted out, in particular the documentation of --passphrase-file points at --pinentry-mode loopback.

cdoern · 2022-08-11T19:52:10Z

wouldn’t expect just redirecting stdin to typically work. GPG is not just reading from stdin, it has a concept of pinentry which intends to talk to the user regardless of stdin. That might be using /dev/tty, or perhaps even popping a GUI window completely outside
@mtrmac, strangely enough when testing this locally, changing to stdin does fix the issue. I really cant pinpoint why or hot though which is why this has languished for so long. Either way, setting to nil it definitely what is breaking this right now. Any tips?

mtrmac · 2022-08-15T14:28:20Z

That really depends on various details of the specific runtime environment of the process (e.g. is there a login session, is there a controlling TTY known to the kernel, is there an environment variable pointing at a pre-existing GPG agent).

I could well imagine that an isolated process with no session and no TTY tries to auto-start the GPG agent, and just fails because it has no way to communicate with the user; while passing a stdin TTY allows the GPG agent to run.

I don’t have much of a recommendation for understanding what’s going on beyond strace.

I would have recommended using GPGME instead of manually running GPG commands, but ultimately GPGME runs these commands as well, so that’s not helpful for the current situation — it would only add one more layer of indirection to unravel.

vrothberg · 2023-05-24T13:38:20Z

Closing due to inactivity.

ashley-cui reviewed Jun 2, 2022

View reviewed changes

cdoern force-pushed the passDriver branch 2 times, most recently from e621472 to a599662 Compare June 3, 2022 13:57

edsantiago reviewed Jun 6, 2022

View reviewed changes

cdoern force-pushed the passDriver branch 2 times, most recently from 98cfad7 to 4322730 Compare June 10, 2022 19:33

cdoern force-pushed the passDriver branch from 4322730 to 788c7fc Compare June 10, 2022 19:37

cdoern marked this pull request as draft June 23, 2022 14:27

openshift-ci bot added the do-not-merge/work-in-progress label Jun 23, 2022

openshift-merge-robot added the needs-rebase label Jul 23, 2022

vrothberg closed this May 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix input/output streams for gpg validation #1057

fix input/output streams for gpg validation #1057

cdoern commented Jun 2, 2022

openshift-ci bot commented Jun 2, 2022

cdoern commented Jun 2, 2022

ashley-cui left a comment

ashley-cui Jun 2, 2022

cdoern Jun 2, 2022

ashley-cui Jun 2, 2022

edsantiago Jun 2, 2022

cdoern commented Jun 3, 2022

edsantiago Jun 6, 2022

cdoern Jun 6, 2022

edsantiago commented Jun 13, 2022

rhatdan commented Jun 21, 2022

cdoern commented Jun 21, 2022

cdoern commented Jun 21, 2022

edsantiago commented Jun 21, 2022

cdoern commented Jun 21, 2022

cdoern commented Jun 22, 2022

ashley-cui commented Jun 22, 2022

openshift-merge-robot commented Jul 23, 2022

mtrmac commented Jul 30, 2022

cdoern commented Aug 11, 2022

mtrmac commented Aug 15, 2022

vrothberg commented May 24, 2023

fix input/output streams for gpg validation #1057

fix input/output streams for gpg validation #1057

Conversation

cdoern commented Jun 2, 2022

openshift-ci bot commented Jun 2, 2022

cdoern commented Jun 2, 2022

ashley-cui left a comment

Choose a reason for hiding this comment

ashley-cui Jun 2, 2022

Choose a reason for hiding this comment

cdoern Jun 2, 2022

Choose a reason for hiding this comment

ashley-cui Jun 2, 2022

Choose a reason for hiding this comment

edsantiago Jun 2, 2022

Choose a reason for hiding this comment

cdoern commented Jun 3, 2022

edsantiago Jun 6, 2022

Choose a reason for hiding this comment

cdoern Jun 6, 2022

Choose a reason for hiding this comment

edsantiago commented Jun 13, 2022

rhatdan commented Jun 21, 2022

cdoern commented Jun 21, 2022

cdoern commented Jun 21, 2022

edsantiago commented Jun 21, 2022

cdoern commented Jun 21, 2022

cdoern commented Jun 22, 2022

ashley-cui commented Jun 22, 2022

openshift-merge-robot commented Jul 23, 2022

mtrmac commented Jul 30, 2022

cdoern commented Aug 11, 2022

mtrmac commented Aug 15, 2022

vrothberg commented May 24, 2023