Allow checking credentials for image imports against repo url, not just passed url #14851

soltysh · 2017-06-23T10:48:54Z

Fixes #9584 and https://bugzilla.redhat.com/show_bug.cgi?id=1462606.

Here's the story. Currently our credentials store (during image import) is passed a url, which we then match against secrets. Unfortunately, in some cases the url of the repository and the one passed to that credentials store are different, which causes frustration, because people first of all need to know the 2nd url and most importantly create it. This situation happens when authentication is redirecting to a different url. Whereas pull secrets don't require this, because they are matching secrets against repository url, not the authn (even if it's different). This simple fix passes the repository url down to the credentials store, which can then try to get the proper credentials using either one or the other url.

@smarterclayton @miminar wdyt of this approach? I guess some tests are needed, but I don't want to spend time working on tests when the approach will be bad. I couldn't find a better way of passing the original repo url, other than this somehow nasty hack.

soltysh · 2017-06-23T10:48:59Z

[test]

openshift-bot · 2017-06-23T10:55:29Z

Evaluated for origin test up to d236486

mfojtik · 2017-06-23T11:56:42Z

@soltysh need unit test

openshift-bot · 2017-06-23T12:47:26Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2547/) (Base Commit: c3c286b) (PR Branch Commit: d236486)

soltysh · 2017-06-23T13:58:17Z

@soltysh need unit test

I know, but I don't want to spend time working on it, unless I get an approval from Michal and Clayton about the approach.

miminar · 2017-06-29T12:00:39Z

pkg/image/importer/credentials.go

@@ -102,14 +102,15 @@ func NewLazyCredentialsForSecrets(secretsFn func() ([]kapi.Secret, error)) *Secr

 type SecretCredentialStore struct {
 	lock      sync.Mutex
+	origin    *url.URL


This should be a map[string /* origin */]string /* realm */.
With the single value, the origin may be overwritten by other concurrent auth routines.

miminar · 2017-06-29T12:11:49Z

pkg/image/importer/credentials.go

+			return basicCredentialsFromKeyring(keyring, &url.URL{Host: "docker.io"}, nil)
+		}
+		// try origin url, if it's different than target
+		if origin != nil && origin.String() != target.String() {


There is no guarantee that origin really corresponds to the target realm. In docker-registry, single context can be used for different registries, each having possibly different auth server. The association origin <-> target needs to be verified first. Otherwise, credentials belonging to one server may be sent to another.

The verification may be simple like extracting auth realm from a simple http GET request to the origin:

curl -I https://index.docker.io/v2/ HTTP/1.1 401 Unauthorized Content-Type: application/json; charset=utf-8 Docker-Distribution-Api-Version: registry/2.0 Www-Authenticate: Bearer realm="https://auth.docker.io/token",service="registry.docker.io" Date: Thu, 29 Jun 2017 11:53:49 GMT

Thread-safety needs to be ensured though. I would welcome the verification being done lazily. However, I'm not sure about polling remote registries in this function which is supposed to be fast.

@liggitt this is your domain, do you have better idea?

I'm also concerned about secret exposure. We would need to be really sure that we aren't vulnerable to handing out a secret to a malicious registry.

smarterclayton · 2017-06-29T14:25:38Z

Fairly concerned about merging this this close to release. If this is only usability, would prefer to discuss for 3.6

smarterclayton · 2017-06-29T14:25:46Z

3.7

openshift-merge-robot · 2017-07-25T19:02:30Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: soltysh

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/image/OWNERS~~ [soltysh]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

aweiteka · 2017-07-28T16:26:45Z

Workaround documented:
openshift/openshift-docs#4863

openshift-merge-robot · 2017-07-28T22:09:38Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: soltysh

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/image/OWNERS~~ [soltysh]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-merge-robot · 2017-07-28T22:36:46Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: soltysh

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/image/OWNERS~~ [soltysh]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

soltysh · 2017-08-04T10:07:08Z

@smarterclayton all in all, is this approach you're ok with or we need to discuss this?

soltysh · 2017-08-18T11:35:30Z

@miminar @smarterclayton I've went with a realm cache and based on that I've added a fallback logic authing against the url from before the realm one.

soltysh · 2017-08-21T11:59:20Z

/test unit

soltysh · 2017-08-21T14:13:28Z

Flake: #13966

/test unit

soltysh · 2017-08-21T14:31:55Z

/retest

soltysh · 2017-08-23T13:02:34Z

Fixed that failing unit test and added another for the realm part.

soltysh · 2017-08-23T14:32:27Z

/retest

smarterclayton · 2017-08-23T22:37:25Z

pkg/image/importer/client.go

+			if realm, ok := ch.Parameters["realm"]; ok {
+				if url, err := url.Parse(realm); err == nil {
+					if scs, ok := r.credentials.(*SecretCredentialStore); ok {
+						scs.AddRealm(&registry, url)


We should log at v(4) if we encounter this.

smarterclayton · 2017-08-23T22:39:16Z

pkg/image/importer/client_test.go

+
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Docker-Distribution-API-Version", "registry/2.0")
+		w.Header().Set("WWW-Authenticate", `Bearer realm="https://auth.example.com/token"`)


So assume I'm a malicious docker registry at https://badserver.com. You hit me, I say, "oh, i need credentials for www.redhat.com". Will we then send credentials for www.redhat.com to https://badserver.com, just because badserver.com told us?

No, we won't send credentials. The flow looks like this:

OpenShift sends ping to https://badserver.com, in response we MUST get 401 Unauthorized with header WWW-Authenticate: <scheme> realm="https://www.redhat.com",service="<servicename>".

OpenShift sends credentials TO realm, here https://www.redhat.com with information about service. https://www.redhat.com verifies service (needs to be known for it, otherwise you'll get rejected) and credentials. In response sends back a token (usually short lived one).

OpenShift uses token against https://baserver.com to perform necessary actions.

This is all described in details in RFC 7235.

soltysh · 2017-08-29T13:11:06Z

@smarterclayton any other doubts, or we're good to ship it. @miminar is out still

smarterclayton · 2017-08-29T13:45:44Z

pkg/image/importer/client_test.go

+	}
+	if ru.registry.String() != server.URL {
+		t.Errorf("Unexpected registry url, expected %s, got %v", server.URL, ru.registry)
+	}


Can this test do more to prove the credentials for the realm would not be offered to the endpoint?

Not within this test. I'd have to create the entire client flow to verify if the original server is not getting those creds. Seems like a lot of effort for such a reasonable small thing. Is it worth it?

Small security thing sometimes turns out to be a big nightmare. I'd like having this covered.

Sigh... will update.

miminar · 2017-09-05T14:14:41Z

pkg/image/importer/client.go

+	if challenges, err := r.context.Challenges.GetChallenges(*resp.Request.URL); err == nil {
+		for _, ch := range challenges {
+			// TODO: support multiple realm headers
+			if realm, ok := ch.Parameters["realm"]; ok {


Challenges with ch.Scheme other than basic and bearer should be skipped.

See here:

origin/pkg/image/importer/client.go

Line 114 in 724c7d2

auth.NewBasicHandler(r.credentials),

We don't support any other schemes but bearer and basic.

Roger that, will update.

miminar · 2017-09-05T14:57:21Z

pkg/image/importer/credentials.go

+func (s *SecretCredentialStore) AddRealm(registry, realm *url.URL) {
+	ru := &realmURL{
+		realm:    *realm,
+		registry: *registry,


If there are parallel requests using the same credential store to different registries having the same realm, one entry will overwrite the other. To avoid the race, the registry should be a list and access to these entries needs to be guarded.

Will update.

miminar · 2017-09-05T15:03:16Z

pkg/image/importer/client_test.go

+	}
+	if ru.registry.String() != server.URL {
+		t.Errorf("Unexpected registry url, expected %s, got %v", server.URL, ru.registry)
+	}


Small security thing sometimes turns out to be a big nightmare. I'd like having this covered.

miminar · 2017-09-05T15:11:33Z

pkg/image/importer/client_test.go

+	if !ok {
+		t.Errorf("Unexpected object type, expected realmURL, got %T", obj)
+	}
+	if ru.realm.String() != "https://auth.example.com/token" {


nit: a good candidate for const

I didn't bother since this is just test.

openshift-merge-robot · 2017-09-08T08:03:28Z

@soltysh PR needs rebase

…st passed url

soltysh · 2017-09-14T09:44:37Z

@smarterclayton I've spent great deal of time to think through this solution (especially after your recent changes) and I think this approach is not good. I've additionally confirmed my suspicions wrt to the insecure nature of this approach with @miminar, while in Brno yesterday. Mostly wrt to having the authorization decoupled from the credential store, like we have right now. We need to implement appropriate authorization handlers in image imports that will have the full access to the both the request and the credential store so that it matches appropriate secrets when needed. Additionally, having a proper authhandler in place will allow us to simplify the dockerhub credentials matching code which will also be supported when we have a proper auth delegation implemented.

Having said that, I've created a trello card in devexp board (@bparees fyi) to have a proper fix instead of hacky approach.

openshift-ci-robot · 2017-09-14T10:06:37Z

@soltysh: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/verify	`788a5f9`	link	`/test verify`
ci/openshift-jenkins/integration	`788a5f9`	link	`/test integration`
ci/openshift-jenkins/unit	`788a5f9`	link	`/test unit`
ci/openshift-jenkins/cmd	`788a5f9`	link	`/test cmd`
ci/openshift-jenkins/extended_networking_minimal	`788a5f9`	link	`/test extended_networking_minimal`
ci/openshift-jenkins/end_to_end	`788a5f9`	link	`/test end_to_end`
ci/openshift-jenkins/extended_conformance_gce	`788a5f9`	link	`/test extended_conformance_gce`
ci/openshift-jenkins/extended_conformance_install_update	`788a5f9`	link	`/test extended_conformance_install_update`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

miminar suggested changes Jun 29, 2017

View reviewed changes

openshift-merge-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 24, 2017

openshift-merge-robot assigned liggitt and deads2k Jul 25, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2017

openshift-merge-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 30, 2017

soltysh added this to the 3.7.0 milestone Aug 4, 2017

soltysh force-pushed the bug_1462606 branch from d236486 to 9fd05f2 Compare August 18, 2017 11:31

soltysh added the priority/P1 label Aug 21, 2017

soltysh force-pushed the bug_1462606 branch from 9fd05f2 to fb6ba6c Compare August 23, 2017 13:02

openshift-merge-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 23, 2017

smarterclayton reviewed Aug 23, 2017

View reviewed changes

soltysh force-pushed the bug_1462606 branch from fb6ba6c to 724c7d2 Compare August 25, 2017 12:02

smarterclayton reviewed Aug 29, 2017

View reviewed changes

openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 30, 2017

miminar suggested changes Sep 5, 2017

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 8, 2017

soltysh added 2 commits September 12, 2017 10:14

Remove old debug messages

612caae

Allow checking credentials for image imports against repo url, not ju…

788a5f9

…st passed url

soltysh force-pushed the bug_1462606 branch from 724c7d2 to 788a5f9 Compare September 14, 2017 09:44

soltysh closed this Sep 14, 2017

soltysh mentioned this pull request Sep 14, 2017

accessing ImageStream from private repository with auth delegated to a different server requires creating two secrets instead of one #9584

Closed

soltysh deleted the bug_1462606 branch September 14, 2017 09:50

Allow checking credentials for image imports against repo url, not just passed url #14851

Allow checking credentials for image imports against repo url, not just passed url #14851

Conversation

soltysh commented Jun 23, 2017

soltysh commented Jun 23, 2017

openshift-bot commented Jun 23, 2017

mfojtik commented Jun 23, 2017

openshift-bot commented Jun 23, 2017

soltysh commented Jun 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Jun 29, 2017

smarterclayton commented Jun 29, 2017

openshift-merge-robot commented Jul 25, 2017

aweiteka commented Jul 28, 2017

openshift-merge-robot commented Jul 28, 2017

openshift-merge-robot commented Jul 28, 2017

soltysh commented Aug 4, 2017

soltysh commented Aug 18, 2017

soltysh commented Aug 21, 2017

soltysh commented Aug 21, 2017

soltysh commented Aug 21, 2017

soltysh commented Aug 23, 2017

soltysh commented Aug 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented Aug 29, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-merge-robot commented Sep 8, 2017

soltysh commented Sep 14, 2017

openshift-ci-robot commented Sep 14, 2017

soltysh Aug 25, 2017 •

edited

Loading