[DO NOT MERGE] Handle cross repository mount #9582

miminar · 2016-06-27T11:57:23Z

During a push with a cross-repo mount, authorize only against target
repository, not against source repository.

In OpenShift we allow to pull any blob stored in the registry as long as
the user is authorized to pull from repository he requested.

Closes #9540
Not sure whether #6841 is close-able with this. The issue mentions proper association of credentials with repositories (and especially the remote once if a pull through is in play). I'll verify.

miminar · 2016-06-27T11:58:07Z

/cc @legionus @soltysh

[test]

soltysh · 2016-06-27T12:14:05Z

pkg/dockerregistry/server/auth.go

@@ -90,7 +91,7 @@ var (
 )

 func newAccessController(options map[string]interface{}) (registryauth.AccessController, error) {
-	log.Info("Using Origin Auth handler")
+	log.Info("using Origin Auth handler")


Why changing that, only errors start with small letter.

Well, most of the upstream log messages are lower-cased. In v2.4.1 it's 33 messages starting with upper-cased letter vs 81 starting with lower-cased (all log levels).

But ok, keeping the messages same won't do any harm. I'll lower-case the new messages though.

This code is part of origin code in which the patterns I've mentioned apply.

This code is part of origin code in which the patterns I've mentioned apply.

Yes, but the message appears only in registry's logs. Another reason for having registry in its own repository 😄.

soltysh · 2016-06-27T12:22:41Z

Nits, but overall LGTM.

legionus · 2016-06-27T12:39:09Z

LGTM

soltysh · 2016-06-27T13:33:16Z

LGTM, squash and you're good to go.

miminar · 2016-06-27T13:36:09Z

@soltysh thanks! squashed.

miminar · 2016-06-27T13:36:54Z

(I'd still rather wait for a clean pass though)

soltysh · 2016-06-27T13:59:06Z

Your wish is my command! I'll merge once this turns green.

liggitt · 2016-06-27T14:11:43Z

pkg/dockerregistry/server/auth.go

+//
+// This function always grants access in the first case because OpenShift registry treats all the local blobs
+// as accessible once authorized to access given repository (target one).
+func isCrossRepoMountFromSourceRepository(ctx context.Context, namespace, imageRepo, verb string) bool {


how are we guaranteeing that the access check has already been performed on the imageRepo?

It's upstream's responsibility to do auth check for both repositories (source and target). See here: https://github.com/openshift/origin/blob/master/Godeps/_workspace/src/github.com/docker/distribution/registry/handlers/app.go#L771. The source check is done first. The second check will decide whether the user is authorized to push.

bparees · 2016-06-27T14:35:05Z

i've added extended test requests for tests that i think were broken by the recent registry changes.

liggitt · 2016-06-27T15:20:20Z

pkg/dockerregistry/server/auth.go

@@ -196,7 +197,7 @@ func (ac *AccessController) Authorized(ctx context.Context, accessRecords ...reg
 				}
 				verifiedPrune = true
 			default:
-				if err := verifyImageStreamAccess(ctx, imageStreamNS, imageStreamName, verb, osClient); err != nil {
+				if err := authorizeRepositoryAccess(ctx, imageStreamNS, imageStreamName, verb, osClient); err != nil {


I'd rather see this loop be aware of access records being skipped because they appear to be cross-repository mounts... if we skip an access record for that reason, this loop should ensure it is paired with an authorized push access record check

That's reasonable, I'll change it.

liggitt · 2016-06-27T15:41:48Z

actually, if I understand this correctly, this would allow a client to do the following:

push a manifest M referencing layers L1,L2,L3 where L1 is their own layer in their own namespace, and L2 and L3 are layers from other namespaces they do not have access to
this code (even with the adjustments described so far) would appear to skip access checks for L2, and L3
The user could then pull the manifest and get all three layers, including the ones they were not supposed to have access to

liggitt · 2016-06-27T19:26:29Z

pkg/dockerregistry/server/pullthroughblobstore.go

+func (r *pullthroughBlobStore) Create(ctx context.Context, options ...distribution.BlobCreateOption) (distribution.BlobWriter, error) {
+	options = append(options, storage.WithRepositoryMiddlewareWrapper(
+		func(ctx context.Context, repo distribution.Repository, name reference.Named) (distribution.Repository, error) {
+			nameParts := strings.SplitN(name.Name(), "/", 2)


do we want to artificially limit the segments to 2, or fail if they give us something with more than 2 segments?

We're doing it the same way on most other places. And AFAIK upstream registry doesn't support more than 2 segments either. We could create a utility function to do this. But I wouldn't clobber this with such a refactor.

miminar · 2016-06-27T19:39:47Z

I changed the behavior to the old one. Now the first auth check for source repository is skipped in the auth module. The auth check has been moved to repository middleware. If the check fails (most of the time), the registry falls back to a blob upload (sends 202 to client as documented upstream).

If we were doing both checks in the auth module, the registry would return unathorized which would cancel whole push.

For this, we need to propagate our middleware code to upstream blob store during a Create() call. Which is not possibly without modifying upstream code. This brings a dependency on distribution/distribution#1757 (and a carry patch). Feel free to push it upstream. The same patch is needed to allow remote layer federation for remote registries when dealing with v1 manifest schema.

This doesn't attempt to fix the cross-repo mount by checking for blob presence in source repository. The blob checking needs to be done at several other places. We should fix it together with #7792. Until then, let's keep the old behavior.

openshift-bot · 2016-06-28T00:00:33Z

continuous-integration/openshift-jenkins/test Running (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/5501/)

liggitt · 2016-06-28T05:15:43Z

rather than skipping errors during auth, what if we store ones that look like cross-mount errors in the context. then, in the repository impls, we can first check to ensure auth ran, then short-circuit any operations for a given repo if there were deferred errors. see #9593 for a quick POC.

that would make me a lot more comfortable deferring enforcement of some auth errors, and being confident that no API path was going to hit a repository interface method we didn't expect

pweil- · 2016-06-28T12:15:41Z

@smarterclayton

openshift-bot · 2016-06-28T17:00:56Z

Evaluated for origin test up to fb49049

…pper during cross-repo mounts This allows to use pullthroughBlobStore middleware wrapper during a cross-repository mount. This is an upstream part of fix to enable remote layer federation for the push. Signed-off-by: Michal Minar <[email protected]>

This allows us to: - do additional authorization checks during cross-repo mount on source repository - remote layer federation Signed-off-by: Michal Minar <[email protected]>

openshift-bot · 2016-06-28T19:25:47Z

continuous-integration/openshift-jenkins/testextended Running (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/253/) (Extended Tests: core(images), core(builds))

bparees · 2016-06-28T19:33:38Z

[testextended][extended:core(images)]

bparees · 2016-06-28T19:34:07Z

(bot does not seem to respect two testextended tags in one comment?)

openshift-bot · 2016-06-28T19:35:49Z

Evaluated for origin testextended up to 3199a57

bparees · 2016-06-28T19:37:47Z

[testextended][extended:core(builds)]

liggitt · 2016-06-28T19:44:57Z

closing in favor of #9593

miminar · 2016-06-28T19:51:00Z

closing in favor of #9593

soltysh reviewed Jun 27, 2016
View reviewed changes

miminar force-pushed the cross-repo-push-registry-fix branch from ab83d21 to 7940966 Compare June 27, 2016 13:35

liggitt reviewed Jun 27, 2016
View reviewed changes

miminar force-pushed the cross-repo-push-registry-fix branch from 7940966 to c78781b Compare June 27, 2016 15:00

liggitt reviewed Jun 27, 2016
View reviewed changes

miminar force-pushed the cross-repo-push-registry-fix branch from c78781b to c823e8c Compare June 27, 2016 19:22

liggitt reviewed Jun 27, 2016
View reviewed changes

miminar force-pushed the cross-repo-push-registry-fix branch from c823e8c to 9892109 Compare June 27, 2016 19:34

This was referenced Jun 28, 2016

Fix auth checks for cross-repo mount requests #9593

Merged

Docker registry v2.4.0 #8938

Merged

miminar force-pushed the cross-repo-push-registry-fix branch from 9892109 to fb49049 Compare June 28, 2016 16:56

miminar changed the title ~~Handle cross repository mount~~ [DO NOT MERGE] Handle cross repository mount Jun 28, 2016

Michal Minar added 2 commits June 28, 2016 19:22

Use our repository middleware during cross-repo mount

3199a57

This allows us to: - do additional authorization checks during cross-repo mount on source repository - remote layer federation Signed-off-by: Michal Minar <[email protected]>

miminar force-pushed the cross-repo-push-registry-fix branch from fb49049 to 3199a57 Compare June 28, 2016 17:25

bparees mentioned this pull request Jun 28, 2016

Adding dumplogs pattern to extended build tests #9592

Merged

liggitt closed this Jun 28, 2016

miminar deleted the cross-repo-push-registry-fix branch November 10, 2016 15:41

[DO NOT MERGE] Handle cross repository mount #9582

[DO NOT MERGE] Handle cross repository mount #9582

Uh oh!

Conversation

miminar commented Jun 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miminar commented Jun 27, 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!

soltysh commented Jun 27, 2016

Uh oh!

legionus commented Jun 27, 2016

Uh oh!

soltysh commented Jun 27, 2016

Uh oh!

miminar commented Jun 27, 2016

Uh oh!

miminar commented Jun 27, 2016

Uh oh!

soltysh commented Jun 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bparees commented Jun 27, 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!

liggitt commented Jun 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miminar commented Jun 27, 2016

Uh oh!

openshift-bot commented Jun 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liggitt commented Jun 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pweil- commented Jun 28, 2016

Uh oh!

openshift-bot commented Jun 28, 2016

Uh oh!

openshift-bot commented Jun 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bparees commented Jun 28, 2016

Uh oh!

bparees commented Jun 28, 2016

Uh oh!

openshift-bot commented Jun 28, 2016

Uh oh!

bparees commented Jun 28, 2016

Uh oh!

liggitt commented Jun 28, 2016

Uh oh!

miminar commented Jun 28, 2016

Uh oh!

Uh oh!

miminar commented Jun 27, 2016 •

edited

Loading

openshift-bot commented Jun 28, 2016 •

edited

Loading

liggitt commented Jun 28, 2016 •

edited

Loading

openshift-bot commented Jun 28, 2016 •

edited

Loading