Add secret support to podman login #19200

rhatdan · 2023-07-11T20:21:32Z

Does this PR introduce a user-facing change?

Podman login can now read the secret for a registry from its secret database created with podman secret create.

Fixes: #18667

rhatdan · 2023-07-11T20:21:51Z

@kriansa PTAL

rhatdan · 2023-07-11T20:22:02Z

@ashley-cui @vrothberg PTAL

rhatdan · 2023-07-11T20:29:02Z

@alexlarsson @ygalblum This is my idea for how quadlet could handle registry secrets, In pre section of the quadlet, do a ExecStartPre=podman login --secret secret registryfor.image.in.quadlet
ExecStopPost=podman logout registryfor.image.in.quadlet

TomSweeneyRedHat · 2023-07-11T21:18:05Z

test/system/150-login.bats

+       "output from podman logout"
+    run_podman secret rm $secret
+
+    # test using secred id as --username


nit of a nit

Suggested change

# test using secred id as --username

# test using secret id as --username

test/system/150-login.bats

edsantiago · 2023-07-12T00:25:38Z

test/system/150-login.bats

+
+      run_podman login --tls-verify=false \
+               --username ${PODMAN_LOGIN_USER} \
+               --secret ${output} \


This squicks me out a little: first, it's not immediately obvious what $output is, second, it would be so easy to accidentally insert a run_podman load or run or something. Suggestion: add secretid=$output immediately after the secret create. Otherwise LGTM. with @TomSweeneyRedHat's suggestions.

vrothberg · 2023-07-12T07:32:52Z

cmd/podman/login.go

@@ -48,7 +51,8 @@ func init() {
 	completion.CompleteCommandFlags(loginCommand, auth.GetLoginFlagsCompletions())

 	// Podman flags.
-	flags.BoolVarP(&loginOptions.tlsVerify, "tls-verify", "", false, "Require HTTPS and verify certificates when contacting registries")
+	flags.BoolVar(&loginOptions.tlsVerify, "tls-verify", false, "Require HTTPS and verify certificates when contacting registries")
+	flags.String("secret", "", "retrieve passwd from secret file")


Suggested change

flags.String("secret", "", "retrieve passwd from secret file")

flags.String("secret", "", "Retrieve password from podman secret")

cmd/podman/login.go

vrothberg · 2023-07-12T07:34:59Z

docs/source/markdown/podman-secret-create.1.md

@@ -18,6 +18,8 @@ TLS certificates and keys, SSH keys or other important generic strings or binary

 Secrets are not committed to an image with `podman commit`, and does not get committed in the archive created by a `podman export` command.

+You can also use secrets to store secret data whcih can be used by `podman login` to authenticate to container registries.


Suggested change

You can also use secrets to store secret data whcih can be used by `podman login` to authenticate to container registries.

You can also use secrets to store passwords for `podman login` to authenticate against container registries.

test/system/150-login.bats

Luap99 · 2023-07-12T08:25:32Z

cmd/podman/login.go

@@ -48,7 +51,8 @@ func init() {
 	completion.CompleteCommandFlags(loginCommand, auth.GetLoginFlagsCompletions())

 	// Podman flags.
-	flags.BoolVarP(&loginOptions.tlsVerify, "tls-verify", "", false, "Require HTTPS and verify certificates when contacting registries")
+	flags.BoolVar(&loginOptions.tlsVerify, "tls-verify", false, "Require HTTPS and verify certificates when contacting registries")
+	flags.String("secret", "", "retrieve passwd from secret file")


missing autocompletion function, this should complete secrets AutocompleteSecrets()

ygalblum · 2023-07-12T12:31:42Z

@alexlarsson @ygalblum This is my idea for how quadlet could handle registry secrets, In pre section of the quadlet, do a ExecStartPre=podman login --secret secret registryfor.image.in.quadlet ExecStopPost=podman logout registryfor.image.in.quadlet

About the Quadlet integration, not sure I like it because the login and logout operations are not specific to the quadlet. As a result, if you have two services logging in to the same registry it can become a mess. I see two possible issues. First, different credentials to the same registry. Second, while both services are running, stopping one of them will logout from the registry.

So, for Quadlet, I think it's better to support a new unit type e.g. .registry. This unit will produce a service that does what you intended to do in the ExecStartPre and ExecStopPost, just in its ExecStart and ExecStop (maybe still ExecStopPost which I think handles error case better). Then support a new key in .container and .kube (e.g. Registry) that will create a dependency between the service units.

Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan · 2023-07-12T12:47:15Z

Sure, I was just giving an example of how you could use it. Probably would be fine to just do the ExecPreStart and never remove the secret, since on reboot the secret will be removed, and the secret is already stored on disk in a simple base64 encoding so there is no additional security risk to leaving it around.

ashley-cui

One nit, otherwise LGTM

cmd/podman/login.go

Co-authored-by: Ashley Cui <[email protected]> Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan · 2023-07-12T19:22:50Z

@vrothberg @giuseppe @Luap99 @TomSweeneyRedHat @containers/podman-maintainers PTAL

TomSweeneyRedHat · 2023-07-12T22:05:06Z

LGTM
and happy green test buttons

vrothberg

/lgtm

openshift-ci · 2023-07-13T07:03:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, vrothberg

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan,vrothberg]

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

openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 11, 2023

rhatdan force-pushed the secret1 branch from caa1e74 to 65215b2 Compare July 11, 2023 20:23

rhatdan force-pushed the secret1 branch from 65215b2 to e70f28b Compare July 11, 2023 20:48

TomSweeneyRedHat reviewed Jul 11, 2023

View reviewed changes

test/system/150-login.bats Show resolved Hide resolved

edsantiago reviewed Jul 12, 2023

View reviewed changes

vrothberg reviewed Jul 12, 2023

View reviewed changes

Luap99 reviewed Jul 12, 2023

View reviewed changes

Add secret support to podman login

d497eb3

Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the secret1 branch from e70f28b to d497eb3 Compare July 12, 2023 12:45

ashley-cui reviewed Jul 12, 2023

View reviewed changes

cmd/podman/login.go Outdated Show resolved Hide resolved

cmd/podman/login.go Outdated Show resolved Hide resolved

Update cmd/podman/login.go

d195086

Co-authored-by: Ashley Cui <[email protected]> Signed-off-by: Daniel J Walsh <[email protected]>

vrothberg approved these changes Jul 13, 2023

View reviewed changes

openshift-ci bot assigned vrothberg Jul 13, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2023

openshift-merge-robot merged commit 69f112a into containers:main Jul 13, 2023

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Oct 12, 2023

github-actions bot locked as resolved and limited conversation to collaborators Oct 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add secret support to podman login #19200

Add secret support to podman login #19200

rhatdan commented Jul 11, 2023

rhatdan commented Jul 11, 2023

rhatdan commented Jul 11, 2023

rhatdan commented Jul 11, 2023

TomSweeneyRedHat Jul 11, 2023

edsantiago Jul 12, 2023

vrothberg Jul 12, 2023

vrothberg Jul 12, 2023

Luap99 Jul 12, 2023

ygalblum commented Jul 12, 2023

rhatdan commented Jul 12, 2023

ashley-cui left a comment

rhatdan commented Jul 12, 2023

TomSweeneyRedHat commented Jul 12, 2023

vrothberg left a comment

openshift-ci bot commented Jul 13, 2023

	# test using secred id as --username
	# test using secret id as --username

	flags.String("secret", "", "retrieve passwd from secret file")
	flags.String("secret", "", "Retrieve password from podman secret")

		@@ -18,6 +18,8 @@ TLS certificates and keys, SSH keys or other important generic strings or binary

		Secrets are not committed to an image with `podman commit`, and does not get committed in the archive created by a `podman export` command.

		You can also use secrets to store secret data whcih can be used by `podman login` to authenticate to container registries.

	You can also use secrets to store secret data whcih can be used by `podman login` to authenticate to container registries.
	You can also use secrets to store passwords for `podman login` to authenticate against container registries.

Add secret support to podman login #19200

Add secret support to podman login #19200

Conversation

rhatdan commented Jul 11, 2023

Does this PR introduce a user-facing change?

rhatdan commented Jul 11, 2023

rhatdan commented Jul 11, 2023

rhatdan commented Jul 11, 2023

TomSweeneyRedHat Jul 11, 2023

Choose a reason for hiding this comment

edsantiago Jul 12, 2023

Choose a reason for hiding this comment

vrothberg Jul 12, 2023

Choose a reason for hiding this comment

vrothberg Jul 12, 2023

Choose a reason for hiding this comment

Luap99 Jul 12, 2023

Choose a reason for hiding this comment

ygalblum commented Jul 12, 2023

rhatdan commented Jul 12, 2023

ashley-cui left a comment

Choose a reason for hiding this comment

rhatdan commented Jul 12, 2023

TomSweeneyRedHat commented Jul 12, 2023

vrothberg left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 13, 2023