Add --compat-auth-file to login/logout #1731

mtrmac · 2023-11-07T18:38:54Z

See individual commit messages for details.

Depends on an unmerged c/image PR, and was not yet tested in consumers.

openshift-ci · 2023-11-07T18:39:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtrmac

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~~ [mtrmac]

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

rhatdan · 2023-11-08T12:31:23Z

pkg/auth/auth.go

+		// If the Docker configuration exists in the default ~/.docker/config.json location,
+		// we DO NOT write to it; instead, we update auth.json in the default path.
+		// Only if the user explicitly sets DOCKER_CONFIG, we write to that config.json.
+		if dockerConfig := os.Getenv("DOCKER_CONFIG"); dockerConfig != "" {


If REGISTRY_AUTH_FILE and DOCKER_CONFIG is set, then we use DOCKER_CONFIG? We should probably at least logrus_debug this.

Oops, I’ll fix that.

Hopefully fixed.

rhatdan · 2023-11-08T12:32:22Z

pkg/auth/cli.go

@@ -45,7 +45,7 @@ type LogoutOptions struct {
 // GetLoginFlags defines and returns login flags for containers tools
 func GetLoginFlags(flags *LoginOptions) *pflag.FlagSet {
 	fs := pflag.FlagSet{}
-	fs.StringVar(&flags.AuthFile, "authfile", GetDefaultAuthFile(), "path of the authentication file. Use REGISTRY_AUTH_FILE environment variable to override")
+	fs.StringVar(&flags.AuthFile, "authfile", "", "path of the authentication file. Use REGISTRY_AUTH_FILE environment variable to override")


I like to reveal this information to the user via

podman login --help

Why are you removing this information?

We need to detect whether the user incorrectly passed --authfile … --compat-auth-file …, and setting a default value here interferes with that.

Also, GetDefaultAuthFile does not do what it suggests; on almost all systems it returns "".

The idea is revealing the default, if user specifies the --authfile or --docker-auth-file, then they modified the defaults.
This should be showing $XDG_RUNTIME_DIR/UID/auth.json almost everywhere unless there is a bug.

It would also show the correct value if DOCKER_AUTH_FILE or REGISTRY_AUTH_FILE environment is set.

But that’s not how it works.

On the read paths (not login/logout, this has always been incorrect because “no --authfile” and --authfile=$the_default_from_--help” don’t do the same thing: the former searches for auth.json in /run, and in ~/.config, and for two formats of Docker-defined files. The latter only reads that one file. (I think fairly strongly that this behavior is correct.)

And, on both read paths and login/logout, if a credential helper is configured in registries.conf, the value of --authfile (and of the new --compat-auth-file) might be effectively ignored. (I don’t know whether that’s desirable, it’s just what the code does.)

I see some value in displaying the paths that will be used in --help; but doing that by setting “the default value of the flag”, and in effect having the flag always set and non-empty, is EDIT not an accurate way to do that.

Change

fs.StringVar(&flags.AuthFile, "authfile", GetDefaultAuthFile(), "path of the authentication file. Use REGISTRY_AUTH_FILE environment variable to override")

To
fs.String("authfile", GetDefaultAuthFile(), "path of the authentication file. Use REGISTRY_AUTH_FILE environment variable to override")

Then during the main function set the values only if they changed, otherwise rely on the regular processioning as if they were "".

That doesn’t actually implement the feature you want: GetDefaultAuthFile returns "" on most systems. (and it must continue to do so, given how it is currently called in Buildah/Podman, otherwise Podman would stop searching in any of the ~3 other locations.)

And a description of the default behavior into --help as a default value of --authfile is actively confusing, as described above. The default behavior is a credential helper user and/or a file search, passing any --authfile value disables that search (and not the credential helper use)

As a special case of that confusion, see how $DOCKER_CONFIG is handled. GetDefaultAuthFile must continue to read that environment variable, because Podman and Buildah “credential reader” commands rely on that and removing that behavior would be a regression, but login/logout must *EDIT not use that value in --authfile, because they need to write in the compatibility format.

I don’t object to idea of showing default locations in --help. But that feature mostly doesn’t exist right now; it is not the topic of this PR; and I don’t think showing the default locations (note the plural!) in the default value of --authfile can work.

rhatdan · 2023-11-08T20:07:09Z

These flags should be ignored if the customer did not set it. We should add accessors to see if the fields are set

if flags().Changed {
Set the value.
}

mtrmac · 2023-11-08T20:11:02Z

Yes, that is an approach to display a “default” value for --authfile and to still detect whether the user passed a value — but it doesn’t resolve the confusion in that the “default” value does not accurately represent the default behavior, as in #1731 (comment) .

> go get github.com/containers/image/v5@main > go mod tidy && go mod vendor Signed-off-by: Miloslav Trmač <mitr@redhat.com>

... so that it can be used with a future --compat-auth-file option. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

We _need_ to know whether the user explicitly set an option or not, because --authfile and --compat-auth-file conflict. Also, --authfile= (unset) and --authfile=$the_default_auth_json have different effects, so setting the default value of the option is _not_ a practical way to show what the system would do if the option were not specified. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac · 2023-11-13T19:03:41Z

Rebased and now no longer depends on c/image from a fork.

PRs in users all pass tests, and have been manually tested to interoperate with Docker:

Please review & merge.

rhatdan · 2023-11-13T19:51:45Z

LGTM

rhatdan · 2023-11-13T19:52:12Z

@vrothberg @Luap99 @vrothberg @umohnani8 @mheon PTAL

Luap99 · 2023-11-14T10:35:42Z

pkg/auth/auth.go

+	case authFile != "" && dockerCompatAuthFile != "":
+		return nil, errors.New("--authfile and --compat-auth-file can not be set simultaneously")
+	case authFile != "":
+		if authFile == defaultDockerConfigPath {
+			logrus.Warn("--authfile points to ~/.docker/config.json, but the file format is not fully compatible; use --compat-auth-file instead")


I am not sure how this package is used everywhere but pointing to cli options in error messages may not be optimal here. I see it used on the podman server API side (without any auth file option there so not a real issue but it might be an issue in the future). It is also used in the bindings test with an authfile so pointing to a cli option there would not make any sense.

You’re completely right that conceptually, auth.go can be used without cli.go, so it should not assume any specific option names.

Can you suggest a practical option-name-independent wording? Especially in this message, my first stab is credential file is set to ~/.docker/config.json, but the file format is not fully compatible; specify it using the (Docker?) compatibility credential file option instead , which is way too long, and not all that helpful for users looking for the option name.

Looking at the callers, most users users of pkg/auth just call GetDefaultAuthFile and CheckAuthFile; the non-trivial callers are just the top-level CLI login/logout, except:

Podman’s pkg/api/handlers/compat/auth.go. In that case neither AuthFile nor CompatAuthFile is specified, so these messages should not be reached.

(From a quick read without looking at the history, it seems to me that this API handler should not deal with auth files at all (like auth.Login always reads them), and instead of the CLI login it should just call docker.CheckAuth directly; and LoginOptions.NoWriteBack could then be removed.)

Podman’s pkg/bindings/test/auth_test.go. Awkward, but those errors should not happen with code as is, and future maintainers can hopefully deal with it.

So I think that referring to the cli.go option names directly is a a clearly imperfect but hopefully-acceptable compromise, but that’s just because I can’t think of good wording; I’ll be happy to change it to any other suggested text.

Perhaps "saving credentials to ~/.docker/config.json but not using Docker-compatible credential format"?

Thanks for that suggestion!

@Luap99 @mheon Texts updated; PTAL.

Luap99 · 2023-11-14T10:38:12Z

pkg/auth/cli.go

@@ -45,7 +47,8 @@ type LogoutOptions struct {
 // GetLoginFlags defines and returns login flags for containers tools
 func GetLoginFlags(flags *LoginOptions) *pflag.FlagSet {
 	fs := pflag.FlagSet{}
-	fs.StringVar(&flags.AuthFile, "authfile", GetDefaultAuthFile(), "path of the authentication file. Use REGISTRY_AUTH_FILE environment variable to override")
+	fs.StringVar(&flags.AuthFile, "authfile", "", "path of the authentication file. Use REGISTRY_AUTH_FILE environment variable to override")
+	fs.StringVar(&flags.DockerCompatAuthFile, "compat-auth-file", "", "path of a Docker-compatible config file to update instead")


Should this be named compat-authfile? Ii is inconsistent to have authfile as one word and then another option split it as auth-file

My very weak preference is to keep the dash, and to think of the previous authfile as a mistake.

The spelling with the dash was also suggested by someone else in the design doc, and widely approved there — though I’m not 100% clear all of the reviewers were focused on the dash question.

Yeah I can live with either one as well, I just noticed the inconsistency when looking at this here.
If people didn't complain in the design doc then I am fine keeping it like this.

can we rename authfile to auth-file and add an alias to maintain authfile? Otherwise, I find it confusing too that one has the dash and one hasn't

Migrating to --auth-file is a good idea.

Sadly this is not the only location, --authfile is hard-coded all over the place in commands that read credentials (e.g. https://github.com/containers/podman/blob/1d49773bb82a9cb93dbcf652fa613fb9295fd609/cmd/podman/images/push.go#L85 / https://github.com/containers/podman/blob/1d49773bb82a9cb93dbcf652fa613fb9295fd609/cmd/podman/images/push.go#L178 , 28 other instances in Podman outside of this subpackage).

I’m fine with doing that work but I’d prefer to do that after 4.8; --compat-auth-file is a release blocker and I’m worried that changing the flag name everywhere would require several CI iterations (to adjust tests and to deal with possible bugs, or even to add tests for the alias), and Buildah/Podman CI can manage about 2 iterations a day.

I am OK with the plan of migrating after 4.8 given our current timing issues

I have filed containers/podman#20693 to make sure the --auth-file renaming does not get lost.

mheon · 2023-11-14T21:14:49Z

Code LGTM; I think all remaining concerns are about exact wording of error/warning messages and the name of the option.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Note that the warning can't be disabled. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

rhatdan · 2023-11-16T11:34:57Z

/lgtm

... to include containers/image#2173 and containers/common#1731 . Signed-off-by: Miloslav Trmač <mitr@redhat.com>

... to include containers/image#2173, containers/common#1731 and containers/buildah#5143 . Signed-off-by: Miloslav Trmač <mitr@redhat.com>

openshift-ci bot added the do-not-merge/work-in-progress label Nov 7, 2023

openshift-ci bot added the approved label Nov 7, 2023

mtrmac marked this pull request as draft November 7, 2023 18:39

rhatdan reviewed Nov 8, 2023

View reviewed changes

mtrmac force-pushed the docker-compat-login branch from 77dd5ce to c36a4fc Compare November 8, 2023 17:50

openshift-merge-robot added the needs-rebase label Nov 10, 2023

mtrmac force-pushed the docker-compat-login branch from c36a4fc to 2e22005 Compare November 10, 2023 18:11

openshift-merge-robot added needs-rebase and removed needs-rebase labels Nov 10, 2023

mtrmac added 3 commits November 13, 2023 19:58

Update c/image after containers/image#2173

11ed0d5

> go get github.com/containers/image/v5@main > go mod tidy && go mod vendor Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Generalize CheckAuthFile

2e16fde

... so that it can be used with a future --compat-auth-file option. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac force-pushed the docker-compat-login branch from 2e22005 to 44e6c07 Compare November 13, 2023 19:00

openshift-merge-robot removed the needs-rebase label Nov 13, 2023

mtrmac changed the title ~~WIP: Add --compat-auth-file to login/logout~~ Add --compat-auth-file to login/logout Nov 13, 2023

mtrmac marked this pull request as ready for review November 13, 2023 19:26

openshift-ci bot removed the do-not-merge/work-in-progress label Nov 13, 2023

Luap99 reviewed Nov 14, 2023

View reviewed changes

mtrmac added 2 commits November 15, 2023 17:47

Add DockerCompatAuthFile options to ...Options, and --compat-auth-file

19b71a1

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Add a warning if --authfile points to .docker/config.json

eb481dc

Note that the warning can't be disabled. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac force-pushed the docker-compat-login branch from 44e6c07 to eb481dc Compare November 15, 2023 16:48

mtrmac mentioned this pull request Nov 15, 2023

Turn --authfile into an alias for --auth-file containers/podman#20693

Open

openshift-ci bot assigned rhatdan Nov 16, 2023

openshift-ci bot added the lgtm label Nov 16, 2023

openshift-merge-bot bot merged commit 88c1e55 into containers:main Nov 16, 2023
7 checks passed

mtrmac deleted the docker-compat-login branch November 16, 2023 14:58

mtrmac added a commit to mtrmac/buildah that referenced this pull request Nov 16, 2023

Update c/image and c/common to latest

f52f09d

... to include containers/image#2173 and containers/common#1731 . Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac added a commit to mtrmac/skopeo that referenced this pull request Nov 16, 2023

Update c/image and c/common to latest

518181e

... to include containers/image#2173 and containers/common#1731 . Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac added a commit to mtrmac/libpod that referenced this pull request Nov 16, 2023

Update c/image and c/common to latest

3e3f803

... to include containers/image#2173 and containers/common#1731 . Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac added a commit to mtrmac/buildah that referenced this pull request Nov 16, 2023

Update c/image and c/common to latest

5fc64af

... to include containers/image#2173 and containers/common#1731 . Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac added a commit to mtrmac/libpod that referenced this pull request Nov 17, 2023

Update c/image and c/common to latest

9d49d90

... to include containers/image#2173 and containers/common#1731 . Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac added a commit to mtrmac/libpod that referenced this pull request Nov 17, 2023

Update c/image and c/common to latest, c/buildah to main

ced07a5

... to include containers/image#2173, containers/common#1731 and containers/buildah#5143 . Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --compat-auth-file to login/logout #1731

Add --compat-auth-file to login/logout #1731

mtrmac commented Nov 7, 2023 •

edited

openshift-ci bot commented Nov 7, 2023

rhatdan Nov 8, 2023

mtrmac Nov 8, 2023

mtrmac Nov 8, 2023

rhatdan Nov 8, 2023

mtrmac Nov 8, 2023 •

edited

rhatdan Nov 8, 2023

mtrmac Nov 8, 2023 •

edited

rhatdan Nov 8, 2023

mtrmac Nov 8, 2023 •

edited

rhatdan commented Nov 8, 2023

mtrmac commented Nov 8, 2023

mtrmac commented Nov 13, 2023

rhatdan commented Nov 13, 2023

rhatdan commented Nov 13, 2023

Luap99 Nov 14, 2023

mtrmac Nov 14, 2023 •

edited

mheon Nov 14, 2023

mtrmac Nov 15, 2023

Luap99 Nov 14, 2023

mtrmac Nov 14, 2023

Luap99 Nov 14, 2023

giuseppe Nov 14, 2023

mtrmac Nov 14, 2023

mheon Nov 14, 2023

mtrmac Nov 15, 2023

mheon commented Nov 14, 2023

rhatdan commented Nov 16, 2023

Add --compat-auth-file to login/logout #1731

Add --compat-auth-file to login/logout #1731

Conversation

mtrmac commented Nov 7, 2023 • edited

openshift-ci bot commented Nov 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac Nov 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac Nov 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac Nov 8, 2023 • edited

Choose a reason for hiding this comment

rhatdan commented Nov 8, 2023

mtrmac commented Nov 8, 2023

mtrmac commented Nov 13, 2023

rhatdan commented Nov 13, 2023

rhatdan commented Nov 13, 2023

Choose a reason for hiding this comment

mtrmac Nov 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mheon commented Nov 14, 2023

rhatdan commented Nov 16, 2023

mtrmac commented Nov 7, 2023 •

edited

mtrmac Nov 8, 2023 •

edited

mtrmac Nov 8, 2023 •

edited

mtrmac Nov 8, 2023 •

edited

mtrmac Nov 14, 2023 •

edited