use user/pass flags #254

runcom · 2016-11-25T14:06:08Z

We already use global flags for docker specific stuff. This patch enables --username and --password to be passed down to containers/image to setup docker's registries auth.

Fixes #253

@mtrmac @cyphar PTAL

Signed-off-by: Antonio Murdaca runcom@redhat.com

mtrmac · 2016-11-25T17:44:31Z

I am not really thrilled with the generic option names: if --username and --password never worked, nothing stops us from naming them --docker-username / --docker-password.

I am worried that we really shouldn’t send the same password to several different servers. OTOH the distinction really isn’t between transport types, but between individual hostnames, and that would be a horrible UI, so a generic --username and fervently hoping that the users know what they are doing might be the best we can do.

This should also restore TestCanAuthToPrivateRegistryV2WithoutDockerCfg and perhaps some of the neighboring tests, though.

After that, I guess, LGTM.

mtrmac · 2016-11-25T17:46:51Z

(As for “we already do use generic option names for Docker-specific options”, that's partly legacy. And partly using --cert-path for an unwanted service is not a vulnerability, but sending a password to an unwanted service is. But, really, I am bad at designing UIs,)

runcom · 2016-11-25T17:46:56Z

This should also restore TestCanAuthToPrivateRegistryV2WithoutDockerCfg and perhaps some of the neighboring tests, though

Sort of.. I have to mock an empty Docker cfg first. I'll add that though, thx.

I am not really thrilled with the generic option names: if --username and --password never worked, nothing stops us from naming them --docker-username / --docker-password.

We used to have user/pass flag in the very first skopeo versions :/

mtrmac · 2016-11-25T17:47:30Z

Yeah; I am not thrilled with --password but I can live with that.

runcom · 2016-11-25T17:54:24Z

@mtrmac what if we add --src-username and --src-password and the same for dest. That's not horrible to me and IIUC should solve your concern around wrongly sending a password to a wrong server.

mtrmac · 2016-11-25T17:59:17Z

That’s great idea. We can very confidently expect that the sets of servers underlying a transport(atomic: native API + Docker registry API, docker: native API + sigstore) must be under a shared control.

Yes to --{src,dest}-{username,password}, please.

runcom · 2016-11-25T18:06:53Z

Alright, do note that I'll make these new flags available only to copy. inspect and the now deprecated layers command will continue to behave as usual (w/o any user/pass flag set).

runcom · 2016-11-25T18:07:43Z

Or wait, I can add username and password flag locally for inspect/layers commands. I'll do this way.

runcom · 2016-11-26T09:22:53Z

@mtrmac PTAL, before going ahead and add docs/tests/containers/image bits, I would like to have your ack on the direction with copy. I'll also fix layers|inspect once we agree on copy.

rhatdan · 2016-11-26T11:03:49Z

I don't like options with dashes ("-") in them. Could we look at other tools like ssh? Would

--src dwalsh/password@registry --dest mitr/foobar@remote

be better?

runcom · 2016-11-26T11:05:38Z

@rhatdan fine, but --src isn't clear enough because it's not saying we're talking about credentials

runcom · 2016-11-26T11:07:35Z

We don't need to specify the registry also, that's implied by src and dest in the command and we don't need that (something like that is how the Docker auth config file works but that's another story, here in the cli we directly honor user/pass combination without checking the registry they'll be used to authenticate against)

runcom · 2016-11-26T11:23:47Z

@rhatdan @mtrmac BTW, I'm fine compacting "username:password" in the cli as it doesn't make sense to have 2 flags for user/pass when you must always use both. I'll rework this PR once we agree on flags naming though.

rhatdan · 2016-11-26T11:29:11Z

@runcom It would help me if you gave a couple of examples of what you are proposing for a CLI.

runcom · 2016-11-26T11:40:33Z

Like --src-creds runcom:password - modulo no dash in the flag

rhatdan · 2016-11-26T12:05:45Z

I like that better

mtrmac · 2016-11-29T16:11:33Z

BTW, I'm fine compacting "username:password" in the cli as it doesn't make sense to have 2 flags for user/pass when you must always use both. I'll rework this PR once we agree on flags naming though.

That feels a bit off, as a general security habit I like to avoid formatting/parsing and in-band signalling. More importantly, it may make sense to provide an username but not a password (e.g. when authenticating using a Kerberos ticket, or a smartcard)

runcom · 2016-11-29T16:13:52Z

BTW, I'm fine compacting "username:password" in the cli as it doesn't make sense to have 2 flags for user/pass when you must always use both. I'll rework this PR once we agree on flags naming though.
That feels a bit off, as a general security habit I like to avoid formatting/parsing and in-band signalling. More importantly, it may make sense to provide an username but not a password (e.g. when authenticating using a Kerberos ticket, or a smartcard)

alright, no parsing then, any idea regarding Dan's issue on naming?

rhatdan · 2016-11-29T16:35:56Z

Why wouldn't --cred dwalsh
Cause the passwd checks to happen to use kerberos like sshd does?

I guess if you had a case where there was a password without a user then we have a problem.

mtrmac · 2016-11-29T19:06:02Z

Why wouldn't --cred dwalsh
Cause the passwd checks to happen to use kerberos like sshd does?

Good point, that works fine.

runcom · 2016-11-29T19:09:29Z

Why wouldn't --cred dwalsh
Cause the passwd checks to happen to use kerberos like sshd does?
Good point, that works fine.

I probably missed something, are we ok with --creds username:password? I didn't understand Dan's example 😕

mtrmac · 2016-11-29T19:15:27Z

I don't like options with dashes ("-") in them.

This could also be handled by providing aliases:

cli.StringFlag {
    Name: "src-username, suser"
    // …
}

and then

skopeo copy --suser mitr --spass sourcepass $source --duser Miloslav --dpass destpass $dest

(or even --su/--sp/--du/--dp but that’s probably too unreadable).

OTOH it is more typing than

skopeo copy --screds mitr:sourcepass $source --dcreds Miloslav:destpass $dest

rhatdan · 2016-11-29T19:18:08Z

runcom I am just giving a username not a password
--creds dwalsh
Means Username = dwalsh No password
--creds dwalsh:foobar
Means Username=dwalsh Password=foobar.

mtrmac · 2016-11-29T19:19:37Z

cmd/skopeo/copy.go

 		SignBy:           signBy,
 		ReportWriter:     os.Stdout,
+		SourceCtx:        sourceCtx,
+		DestinationCtx:   destinationCtx,


I guess this is unavoidable :/

mtrmac · 2016-11-29T19:21:39Z

Overall, I like the separation of username/password but it does mean quite a bit of extra typing.

WRT the too-long-option names, we can provide shortcuts but it would still be nice to at least allow using long readable ones (--dcreds is also pretty cryptic).

= overall I don’t have a strong opinion on the single/two-argument decision.

mtrmac · 2016-11-29T19:25:47Z

We can very confidently expect that the sets of servers underlying a transport(atomic: native API + Docker registry API, docker: native API + sigstore) must be under a shared control.

Sadly, we can’t, when uploading to docker.io for docker/distribution vs. company-specific sigstore.

Ouch.

(This does not immediately affect this PR because that is only setting a docker/distribution username:password. But any more such cases, and telling the user to run docker login so that we have a hostname-associated configuration stored and not a bazillion of command-line options will start to look pretty good.)

runcom · 2016-11-29T19:32:54Z

runcom I am just giving a username not a password
--creds dwalsh
Means Username = dwalsh No password
--creds dwalsh:foobar
Means Username=dwalsh Password=foobar.

Yup, thanks @rhatdan - I was asking if @mtrmac is fine with that also, Miloslav are you?

rhatdan · 2016-11-30T13:21:17Z

cmd/skopeo/utils.go

+	if err != nil {
+		return nil, err
+	}
+	fmt.Println(username)


Looks like debug outpuy

Yup I'll drop this

rhatdan · 2016-11-30T13:24:50Z

Missing changes to man pages and bash completions.

runcom · 2016-11-30T13:27:48Z

Yup, if @mtrmac ack on the changes done so far, I'll update man, docs and everything else (adding tests also)

runcom · 2016-11-30T14:45:26Z

This is ready to be reviewed fully. @mtrmac @rhatdan added man changes, README changes, tests, completions (it's auto-generated so...). What's missing is to create a PR for containers/image changes, I'll do that shortly after this has been reviewed.

rhatdan · 2016-11-30T14:48:01Z

README.md

 Private registries with authentication
 -
-When interacting with private registries, `skopeo` first looks for the Docker's cli config file (usually located at `$HOME/.docker/config.json`) to get the credentials needed to authenticate. When the file isn't available it falls back looking for `--username` and `--password` flags. The ultimate fallback, as Docker does, is to provide an empty authentication when interacting with those registries.
+When interacting with private registries, `skopeo` first looks for the Docker's cli config file (usually located at `$HOME/.docker/config.json`) to get the credentials needed to authenticate. When the file isn't available it falls back looking for `--creds` (for `skopeo inspect`) or `--src-creds|--dest-creds` (for `skopeo copy`) flags. The ultimate fallback, as Docker does, is to provide an empty authentication when interacting with those registries.


I think this is wrong. If the user specified --creds, --src-creds, --dest-creds, it should override content in config.json rather then fall back.

I think this is wrong. If the user specified --creds, --src-creds, --dest-creds, it should override content in config.json rather then fall back.

Yeah, it's already this way, this piece of the README is wrong, I'll fix it.

rhatdan · 2016-11-30T14:50:20Z

docs/skopeo.1.md


  **--sign-by=**_key-id_ add a signature using that key ID for an image name corresponding to _destination-image_

+  **--src-creds** _username:password_ for accessing the source registry


Should this be

_username[:password]_

very likely, I'll fix it

rhatdan · 2016-11-30T14:52:05Z

Still missing bash completion fixes.

runcom · 2016-11-30T14:54:42Z

Still missing bash completion fixes.

where is bash completion? hack/make/bash_autocomplete auto generates the bash completion on make install - not sure what I should fix 😕

rhatdan · 2016-11-30T15:01:25Z

Looks like it only generates one level of bash completions. Lets ignore this for now and we can open an issue to have a much more complete bach completions.

runcom · 2016-11-30T15:07:01Z

Looks like it only generates one level of bash completions. Lets ignore this for now and we can open an issue to have a much more complete bach completions.

ack, there's already an issue upstream which tracks this urfave/cli#188

mtrmac · 2016-11-30T15:05:43Z

README.md

-# passing --username and --password - we can see that everything goes fine
-$ skopeo --docker-cfg="" --username=testuser --password=testpassword inspect docker://myregistrydomain.com:5000/busybox
+# passing --creds - we can see that everything goes fine
+$ skopeo --docker-cfg="" --creds=testuser:testpassword inspect docker://myregistrydomain.com:5000/busybox


--docker-cfg seems not to exist at all

mtrmac · 2016-11-30T15:14:12Z

cmd/skopeo/delete.go

-	if err := ref.DeleteImage(contextFromGlobalOptions(context)); err != nil {
+	ctx := contextFromGlobalOptions(context)
+	if context.IsSet("creds") {
+		ctx.DockerAuthConfig, err = getDockerAuth(context.String("creds"))


If everyone is going to use it like this, shouldn't it be folded into contextFromGlobalOptions(context, creds-option-name)?

mtrmac · 2016-11-30T15:17:12Z

cmd/skopeo/copy.go

+		cli.StringFlag{
+			Name:  "src-creds, screds",
+			Value: "",
+			Usage: "Use `USERNAME:PASSWORD` for accessing the source registry",


Could this use USERNAME[:PASSWORD], i.e. does that work with the template-variable markers? (If so, please fix everywhere)

mtrmac · 2016-11-30T15:19:02Z

cmd/skopeo/utils.go

-// ParseImage converts image URL-like string to an initialized handler for that image.
+// parseImage converts image URL-like string to an initialized handler for that image.
 // The caller must call .Close() on the returned Image.
 func parseImage(c *cli.Context) (types.Image, error) {


Should parseImageSource be also updated to support --creds? (This would make it work for skopeo layers, though I am more concerned about the API symmetry than layers as such)

runcom · 2016-11-30T15:56:04Z

@mtrmac @rhatdan updated

mtrmac

Thanks!

mtrmac · 2016-11-30T16:06:17Z

cmd/skopeo/utils.go

-func contextFromGlobalOptions(c *cli.Context) *types.SystemContext {
-	tlsVerify := c.GlobalBoolT("tls-verify")
+func contextFromGlobalOptions(c *cli.Context, credsFlag string) (*types.SystemContext, error) {
+	ctx := newBaseSystemContext(c)


I don’t think we need the separate newBaseSystemContext function now that credsFlag is handled inline. (Eventually we may support credsFlag == "" to turn that off, but we don’t need that now.)

This does work just fine, feel free to merge as is.

rhatdan · 2016-11-30T16:09:51Z

Bash Completions #256

Signed-off-by: Antonio Murdaca <runcom@redhat.com>

runcom · 2016-11-30T16:13:04Z

Bash Completions #256

thanks Dan, we'll merge this one and re-iterate on #256

runcom · 2016-11-30T16:34:42Z

my bad, I have to fix containers/image and revendor here. I'll do it asap, sorry

runcom force-pushed the enable-cli-userpass branch from b005861 to 0080512 Compare November 25, 2016 16:35

runcom force-pushed the enable-cli-userpass branch from 0080512 to dda3750 Compare November 26, 2016 09:20

runcom force-pushed the enable-cli-userpass branch from dda3750 to f786369 Compare November 26, 2016 09:24

mtrmac reviewed Nov 29, 2016

View reviewed changes

runcom force-pushed the enable-cli-userpass branch 2 times, most recently from f46a027 to 3f88a84 Compare November 30, 2016 12:05

rhatdan reviewed Nov 30, 2016

View reviewed changes

runcom force-pushed the enable-cli-userpass branch 3 times, most recently from aa1dc73 to 5d246b3 Compare November 30, 2016 14:44

rhatdan reviewed Nov 30, 2016

View reviewed changes

mtrmac reviewed Nov 30, 2016

View reviewed changes

runcom force-pushed the enable-cli-userpass branch from 5d246b3 to c8fd763 Compare November 30, 2016 15:55

mtrmac approved these changes Nov 30, 2016

View reviewed changes

use user/pass flags

342ba18

Signed-off-by: Antonio Murdaca <runcom@redhat.com>

runcom force-pushed the enable-cli-userpass branch from c8fd763 to 342ba18 Compare November 30, 2016 16:10

runcom mentioned this pull request Nov 30, 2016

Complete bash completions for skopeo #256

Merged

runcom merged commit 7cca84b into containers:master Nov 30, 2016

runcom deleted the enable-cli-userpass branch November 30, 2016 16:29

runcom mentioned this pull request Nov 30, 2016

revendor containers/image #257

Merged

mtrmac mentioned this pull request Dec 1, 2016

Support layers federation #249

Merged

github-actions bot added the locked - please file new issue/PR label Oct 8, 2023

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


		--sign-by=_key-id_ add a signature using that key ID for an image name corresponding to _destination-image_

		--src-creds _username:password_ for accessing the source registry

use user/pass flags #254

use user/pass flags #254

Uh oh!

Conversation

runcom commented Nov 25, 2016

Uh oh!

mtrmac commented Nov 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac commented Nov 25, 2016

Uh oh!

runcom commented Nov 25, 2016

Uh oh!

mtrmac commented Nov 25, 2016

Uh oh!

runcom commented Nov 25, 2016

Uh oh!

mtrmac commented Nov 25, 2016

Uh oh!

runcom commented Nov 25, 2016

Uh oh!

runcom commented Nov 25, 2016

Uh oh!

runcom commented Nov 26, 2016

Uh oh!

rhatdan commented Nov 26, 2016

Uh oh!

runcom commented Nov 26, 2016

Uh oh!

runcom commented Nov 26, 2016

Uh oh!

runcom commented Nov 26, 2016

Uh oh!

rhatdan commented Nov 26, 2016

Uh oh!

runcom commented Nov 26, 2016

Uh oh!

rhatdan commented Nov 26, 2016

Uh oh!

mtrmac commented Nov 29, 2016

Uh oh!

runcom commented Nov 29, 2016

Uh oh!

rhatdan commented Nov 29, 2016

Uh oh!

mtrmac commented Nov 29, 2016

Uh oh!

runcom commented Nov 29, 2016

Uh oh!

mtrmac commented Nov 29, 2016

Uh oh!

rhatdan commented Nov 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac commented Nov 29, 2016

Uh oh!

mtrmac commented Nov 29, 2016

Uh oh!

runcom commented Nov 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Nov 30, 2016

Uh oh!

runcom commented Nov 30, 2016

Uh oh!

runcom commented Nov 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Nov 30, 2016

mtrmac commented Nov 25, 2016 •

edited

Loading