add X-Registry-Auth header support #6207

vrothberg · 2020-05-13T12:35:14Z

Support the X-Registry-Auth http-request header.
The content of the header is a base64 encoded JSON payload which can
either be a single auth config or a map of auth configs (user+pw or
token) with the corresponding registries being the keys. Vanilla
Docker, projectatomic Docker and the bindings are transparantly
supported.
Add a hidden --registries-conf flag. Buildah exposes the same
flag, mostly for testing purposes.
Do all credential parsing in the client (i.e., cmd/podman) pass
the username and password in the backend instead of unparsed
credentials.
Add a pkg/auth which handles most of the heavy lifting.
Go through the authentication-handling code of most commands, bindings
and endpoints. Migrate them to the new code and fix issues as seen.
A final evaluation and more tests is still required after this
change.
The manifest-push endpoint is missing certain parameters and should
use the ABI function instead. Adding auth-support isn't really
possible without these parts working.
The container commands and endpoints (i.e., create and run) have not
been changed yet. The APIs don't yet account for the authfile.
Add authentication tests to pkg/bindings.

Fixes: #6384
Signed-off-by: Valentin Rothberg [email protected]

vrothberg · 2020-05-13T12:35:44Z

Manual tests were successful. I am now working on testing that in the apiv2 tests.

vrothberg · 2020-05-13T14:25:20Z

Manual tests were successful. I am now working on testing that in the apiv2 tests.

I looked into running a local registry and found a blocker, unfortunately pretty late in the process. The Docker compat endpoints don't expose a tlsVerify parameter that we could set to false. Hence, the server fails pulling because we're creating our own certificate. One workaround would be to use a custom registries.conf where we're setting the local registry as insecure and that's where I am stuck.

There is no way to point Podman to a custom registries.conf. The e2e tests are using a REGISTRIES_CONF_PATH env variable but that's a NOP as there's no code using that at all.

For now, I suggest to merge the PR as is and to trust that my local smoke tests work.

However, I believe we need a way to point Podman to a custom registries.conf. Buildah exposes a --registries-conf which I suggest to add to Podman as well.

vrothberg · 2020-05-13T14:39:54Z

@baude @jwhonce @edsantiago @mheon PTAL

edsantiago · 2020-05-13T14:44:44Z

How hard would it be to add our own, undocumented, unsupported, tlsVerify to the API?

vrothberg · 2020-05-13T14:46:59Z

How hard would it be to add our own, undocumented, unsupported, tlsVerify to the API?

A simple 1 minute change. If that's acceptable, this would certainly be the quickest way forward.

mheon · 2020-05-13T14:54:12Z

Can we restrict it so it's not available outside of the tests?

vrothberg · 2020-05-13T15:03:41Z

Can we restrict it so it's not available outside of the tests?

We could via an env var, for instance. But I think this would make it sufficiently complex to justify adding a --registries-conf flag.

mheon · 2020-05-13T15:14:26Z

We have plenty of basically-undocumented unit-testing-only flags like that, so I'm on board

jwhonce

Just one improvement for the API usage.

pkg/api/handlers/utils/images.go

openshift-ci-robot · 2020-05-14T14:49:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [vrothberg]

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

vrothberg · 2020-05-14T14:52:19Z

Back to WIP:

We now support both, single-auth headers and multi-auth headers. Multi-auth headers are the same as the ones from projectatomic docker. This way, we are compatible with vanilla Docker and the projectatomic/RHEL Docker which we can/will also use for the new libpod endpoints.

It's now wired into the compat endpoints. Libpod endpoints are still missing along with unit, apiv2 and integration tests.

vrothberg · 2020-05-15T15:31:48Z

Dev update:

All encoding and decoding of the authentication headers is working now.
Bindings have been updated for pull, push along with CLI parts. We can now also set headers in the bindings requests which accounts for the high amount of changed files.
We can send credentials AND entire auth files now. This required some massaging of c/image and lead to docker/config - get all credentials image#928.
Note that there's a new pkg/auth that the bindings and the sever share.

I'll continue on Monday and wire in support for all commands/endpoints that support --authfile and or specifying credentials. Once that's done, I will have a look at testing. I tried to get a local registry running in apiv2-test but failed miserably. I may need @edsantiago's bash skills to get that part done in a sane way.

Since so many tests require a local registry, I would love to have make local-registry target which runs a registry:2 container with "known" credentials that may be dumped at a known location. This would be nice for local development but it would also prevent us from having duplicate code for setting up a registry across the e2e, system, api and potentially bindings tests. @edsantiago, is that something you could do (assuming there's time)?

In response to containers#6207: this is a helper script intended for use in starting and stopping a local container registry. It takes care of port, username, password assignments; generates a self-signed certificate; and starts the container in an isolated podman root/runroot to avoid conflicting with the caller's environment. Intended usage: invoke from shell script, using 'eval' to get results into calling process environment. See help message (-h) for invocation details. This will work for shell scripts but will be difficult if called from Go or C - if that is likely to happen, I'd love to hear suggestions for alternate ways to get the settings back to the caller. Signed-off-by: Ed Santiago <[email protected]>

vrothberg · 2020-05-20T12:47:52Z

Note that we first need to get containers/image#933 in.

vrothberg · 2020-05-20T12:50:51Z

Also, once #6269 is merged, we can add auth tests.

jwhonce

Is identitytoken going to be implemented in a follow on PR?

cmd/podman/manifest/push.go

pkg/domain/infra/abi/play.go

pkg/domain/infra/abi/images.go

jwhonce · 2020-05-20T16:15:54Z

pkg/bindings/play/play.go

 	"github.com/containers/libpod/pkg/domain/entities"
+	"github.com/ghodss/yaml"
+	"github.com/pkg/errors"
+	v1 "k8s.io/api/core/v1"


How much does this grow the remote client?

This would effectively revert the changes from @baude and rebump from 33M to 41M.

I want to go back to the initial approach of sending all credentials over to the server (see containers/image#942 (comment)). The amount of work and complexity is getting out of hand.

This would avoid repulling the k8s stuff back in :)

pkg/auth/auth.go

pkg/api/server/register_registries.go

vrothberg · 2020-05-25T13:30:34Z

Is identitytoken going to be implemented in a follow on PR?

Can you elaborate on that? Identity tokens are already supported via the c/image backends and accounted for in the header.

vrothberg · 2020-05-27T11:31:45Z

Looking at the podman stats with json output flakes now.

vrothberg · 2020-05-27T14:58:00Z

Looking at the podman stats with json output flakes now.

Fixed in #6403

* Support the `X-Registry-Auth` http-request header. * The content of the header is a base64 encoded JSON payload which can either be a single auth config or a map of auth configs (user+pw or token) with the corresponding registries being the keys. Vanilla Docker, projectatomic Docker and the bindings are transparantly supported. * Add a hidden `--registries-conf` flag. Buildah exposes the same flag, mostly for testing purposes. * Do all credential parsing in the client (i.e., `cmd/podman`) pass the username and password in the backend instead of unparsed credentials. * Add a `pkg/auth` which handles most of the heavy lifting. * Go through the authentication-handling code of most commands, bindings and endpoints. Migrate them to the new code and fix issues as seen. A final evaluation and more tests is still required *after* this change. * The manifest-push endpoint is missing certain parameters and should use the ABI function instead. Adding auth-support isn't really possible without these parts working. * The container commands and endpoints (i.e., create and run) have not been changed yet. The APIs don't yet account for the authfile. * Add authentication tests to `pkg/bindings`. Fixes: containers#6384 Signed-off-by: Valentin Rothberg <[email protected]>

vrothberg · 2020-05-29T13:41:30Z

Note: I vendored a commit of c/image (non release). I agreed with @rhatdan to do this until c/image has new release. It's acceptable for an RC at least.

rhatdan · 2020-05-29T14:15:24Z

LGTM

mheon · 2020-05-29T14:34:59Z

/lgtm

In response to containers#6207: this is a helper script intended for use in starting and stopping a local container registry. It takes care of port, username, password assignments; generates a self-signed certificate; and starts the container in an isolated podman root/runroot to avoid conflicting with the caller's environment. Intended usage: invoke from shell script, using 'eval' to get results into calling process environment. See help message (-h) for invocation details. This will work for shell scripts but will be difficult if called from Go or C - if that is likely to happen, I'd love to hear suggestions for alternate ways to get the settings back to the caller. Signed-off-by: Ed Santiago <[email protected]>

vrothberg changed the title ~~WIP - compat handlers: add X-Registry-Auth header support~~ compat handlers: add X-Registry-Auth header support May 13, 2020

jwhonce reviewed May 13, 2020

View reviewed changes

pkg/api/handlers/utils/images.go Outdated Show resolved Hide resolved

vrothberg force-pushed the auth-header branch from e3f8bb3 to 4d3da57 Compare May 14, 2020 14:49

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 14, 2020

vrothberg changed the title ~~compat handlers: add X-Registry-Auth header support~~ WIP - compat handlers: add X-Registry-Auth header support May 14, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 14, 2020

vrothberg force-pushed the auth-header branch from 4d3da57 to 6136cfb Compare May 15, 2020 15:21

vrothberg changed the title ~~WIP - compat handlers: add X-Registry-Auth header support~~ WIP - add X-Registry-Auth header support May 15, 2020

vrothberg force-pushed the auth-header branch from 6136cfb to a7a6649 Compare May 18, 2020 12:10

edsantiago mentioned this pull request May 18, 2020

New tool: hack/podman-registry, manages local registry #6269

Closed

vrothberg force-pushed the auth-header branch from a7a6649 to 6486847 Compare May 20, 2020 12:43

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2020

vrothberg force-pushed the auth-header branch from 6486847 to 3840cb8 Compare May 20, 2020 12:46

vrothberg force-pushed the auth-header branch from 3840cb8 to 05eb3c5 Compare May 20, 2020 12:50

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2020

jwhonce requested changes May 20, 2020

View reviewed changes

vrothberg force-pushed the auth-header branch 2 times, most recently from 8c5ab02 to 36c5f57 Compare May 25, 2020 15:37

vrothberg mentioned this pull request May 26, 2020

[APIv2] push: unauthorized: authentication required #6384

Closed

Edward5hen mentioned this pull request May 26, 2020

[APIv2] push: query parameter tag does not work #6388

Closed

vrothberg force-pushed the auth-header branch 3 times, most recently from 3af8dfc to ccc0869 Compare May 27, 2020 09:36

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 27, 2020

vrothberg force-pushed the auth-header branch from ccc0869 to 77913e7 Compare May 27, 2020 09:39

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 27, 2020

vrothberg force-pushed the auth-header branch from 77913e7 to 779904a Compare May 29, 2020 07:23

vrothberg force-pushed the auth-header branch from 779904a to dc80267 Compare May 29, 2020 13:39

vrothberg changed the title ~~WIP - add X-Registry-Auth header support~~ add X-Registry-Auth header support May 29, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 29, 2020

openshift-ci-robot assigned mheon May 29, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 29, 2020

openshift-merge-robot merged commit 0c750a9 into containers:master May 29, 2020

cdoern mentioned this pull request Jun 7, 2021

[CI:DOCS] converted query parameter for credentials to header parameter. #10588

Merged

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 Sep 25, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add X-Registry-Auth header support #6207

add X-Registry-Auth header support #6207

vrothberg commented May 13, 2020 •

edited

Loading

vrothberg commented May 13, 2020

vrothberg commented May 13, 2020

vrothberg commented May 13, 2020

edsantiago commented May 13, 2020

vrothberg commented May 13, 2020

mheon commented May 13, 2020

vrothberg commented May 13, 2020

mheon commented May 13, 2020

jwhonce left a comment

openshift-ci-robot commented May 14, 2020

vrothberg commented May 14, 2020

vrothberg commented May 15, 2020

vrothberg commented May 20, 2020

vrothberg commented May 20, 2020

jwhonce left a comment

jwhonce May 20, 2020

vrothberg May 25, 2020

vrothberg May 25, 2020

vrothberg May 25, 2020

vrothberg commented May 25, 2020

vrothberg commented May 27, 2020

vrothberg commented May 27, 2020

vrothberg commented May 29, 2020

rhatdan commented May 29, 2020

mheon commented May 29, 2020

add X-Registry-Auth header support #6207

add X-Registry-Auth header support #6207

Conversation

vrothberg commented May 13, 2020 • edited Loading

vrothberg commented May 13, 2020

vrothberg commented May 13, 2020

vrothberg commented May 13, 2020

edsantiago commented May 13, 2020

vrothberg commented May 13, 2020

mheon commented May 13, 2020

vrothberg commented May 13, 2020

mheon commented May 13, 2020

jwhonce left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented May 14, 2020

vrothberg commented May 14, 2020

vrothberg commented May 15, 2020

vrothberg commented May 20, 2020

vrothberg commented May 20, 2020

jwhonce left a comment

Choose a reason for hiding this comment

jwhonce May 20, 2020

Choose a reason for hiding this comment

vrothberg May 25, 2020

Choose a reason for hiding this comment

vrothberg May 25, 2020

Choose a reason for hiding this comment

vrothberg May 25, 2020

Choose a reason for hiding this comment

vrothberg commented May 25, 2020

vrothberg commented May 27, 2020

vrothberg commented May 27, 2020

vrothberg commented May 29, 2020

rhatdan commented May 29, 2020

mheon commented May 29, 2020

vrothberg commented May 13, 2020 •

edited

Loading