User can get only blobs he's able to see #9819

miminar · 2016-07-13T16:38:34Z

Closes #9755, closes #6841

This PR fixes several issues:

The most important is a fix for User can get only blobs he's able to see #9819. Where a user authorized to a single repository can pull any blob stored in the registry.
This also enables cross-repo mount feature. It allows a client to mount a blob from another repository in the registry to the destination repository (the target of the push). Docker client remembers source registry/repository for each layer it pulled. During a push of a blob B to a repository D, it instructs the registry to mount B from another repository S in the same registry if it previously pulled the blob from there. This speeds up push time a lot. This PR enables this functionality only for local repositories.
It fixes a bug in digestToRepositoryCache.RememberDigest where the first association inserted was lost immediately.
It ensures that DockerImageManifestMediaType gets set for all the new and existing images. This requires no migration.

The 1st issue is addressed by replacing regular layer link Stat call done in our blobDescriptorService with a fall-back to image stream images in etcd. The blob must exist either locally or remotely if a remote image is tagged in the image stream. If it's tagged locally, the blob<->repository cache will be checked before the etcd query.

It's blobDescriptorService is nearly a lowest possible level where we can intercept Stat calls on local blobs. Unfortunately blob descriptor service doesn't know, which repository is being accessed without being told via context. That's why upper layers need to set repository on the context for every method that may invoke Stat or Clear on a local layer link.

The fix for 2nd issue is applies only to local repositories. Mounting from remotes needs to be addressed specifically. There's a follow-up #10120.

miminar · 2016-07-13T16:39:48Z

Quick&dirty. I'll polish tomorrow.

[test]

miminar · 2016-07-13T16:57:25Z

/cc @liggitt @smarterclayton

miminar · 2016-07-14T13:36:19Z

/cc @soltysh especially for the e2e test part
/cc @legionus for the blob store part

smarterclayton · 2016-07-14T14:52:27Z

pkg/image/api/helper.go

@@ -463,6 +463,8 @@ func ImageWithMetadata(image *Image) error {
 			return nil
 		}

+		image.DockerImageManifestMediaType = schema1.MediaTypeManifest


We'll need the migrator in order to make this work then? How will a user upgrading get the correct behavior? Or is this just extra data we forgot?

It should be already set for all the schema2 manifests. It's set by the registry on a push and by importer on import. All the code using this field is written with this in mind (empty value is considered as schema v1).

smarterclayton · 2016-07-14T16:01:57Z

test/end-to-end/core.sh

+os::cmd::expect_success "oc tag --source docker centos/ruby-22-centos7:latest -n custom ruby-22-centos7:latest"
+os::cmd::expect_success 'oc policy add-role-to-user registry-viewer pusher -n custom'
+
+os::cmd::expect_success_and_text "curl -I -X HEAD -u 'pusher:${pusher_token}' '${DOCKER_REGISTRY}/v2/crossmount/repo/blobs/$rubyimageblob'" "404 Not Found"


Can you do a test of cross mount of a pull through image? If it's currently broken due to the lookup problem that's ok, I just want a test either way.

Can you do a test of cross mount of a pull through image? If it's currently broken due to the lookup problem that's ok, I just want a test either way.

See the last curl. I'll add a pullthrough comment so it's obvious.

smarterclayton · 2016-07-14T23:36:03Z

pkg/dockerregistry/server/blobdescriptorservice.go

+		return false
+	}
+
+	if managed := image.Annotations[imageapi.ManagedByOpenShiftAnnotation]; requireManaged && managed != "true" {


If you've tagged in an image successfully, you've proved you have access to it. All this case guards against is pullthrough (for now). Can you add a comment describing why this is needed?

Comment added.

soltysh · 2016-07-18T14:08:23Z

pkg/dockerregistry/server/errorblobstore.go

+	isCrossMount, err := checkPendingCrossMountErrors(ctx, opts)
+	if isCrossMount {
+		if err != nil {
+			context.GetLogger(ctx).Infof("disabling cross-repo mount because o error: %v", err)


s/o error/of an error

soltysh · 2016-07-18T14:19:33Z

Why we want to carry a patch to distribution? Have we tried upstreaming the fix?

smarterclayton · 2016-07-19T00:23:33Z

test/end-to-end/core.sh

+os::cmd::expect_success_and_text "curl -I -X HEAD -u 'pusher:${pusher_token}' '${DOCKER_REGISTRY}/v2/crossmount/repo/blobs/$rubyimageblob'" "404 Not Found"
+os::cmd::expect_success_and_text "curl -I -X HEAD -u 'pusher:${pusher_token}' '${DOCKER_REGISTRY}/v2/cache/ruby-22-centos7/blobs/$rubyimageblob'" "200 OK"
+# 202 means that cross-repo mount has failed (in this case because of blob doesn't exist in the source repository), client needs to reupload the blob
+os::cmd::expect_success_and_text "curl -I -X POST -u 'pusher:${pusher_token}' '${DOCKER_REGISTRY}/v2/crossmount/repo/blobs/uploads/?mount=$rubyimageblob&from=cache/hello-world'" "202 Accepted"


Add negative tests that verify a user who doesn't have access gets an explicit error on push (when nothing is mounted). Also that they can't access the blob directly, and that users don't have access to the blobs. Tests probably need to be both here and in the unit tests.

Tests probably need to be both here and in the unit tests.

Auth unit tests seem sufficient to me. Additional negative tests have been just added by @liggitt's d179662#diff-4ea7c8f0aaeae849c3760842d6b42c60R194

I don't think they cover it, we need blob tests for authenticated users,
and users with similar / overlapping namespace permissions. We also need
to explicitly call out the cached permissions behavior with a test.

On Wed, Jul 20, 2016 at 8:31 AM, Michal Minar [email protected]
wrote:

In test/end-to-end/core.sh
#9819 (comment):

echo "[INFO] Back to 'default' project with 'admin' user..."
os::cmd::expect_success "oc project ${CLUSTER_ADMIN_CONTEXT}"
os::cmd::expect_success_and_text 'oc whoami' 'system:admin'
+os::cmd::expect_success "oc tag --source docker centos/ruby-22-centos7:latest -n custom ruby-22-centos7:latest"
+os::cmd::expect_success 'oc policy add-role-to-user registry-viewer pusher -n custom'
+
+os::cmd::expect_success_and_text "curl -I -X HEAD -u 'pusher:${pusher_token}' '${DOCKER_REGISTRY}/v2/crossmount/repo/blobs/$rubyimageblob'" "404 Not Found"
+os::cmd::expect_success_and_text "curl -I -X HEAD -u 'pusher:${pusher_token}' '${DOCKER_REGISTRY}/v2/cache/ruby-22-centos7/blobs/$rubyimageblob'" "200 OK"
+# 202 means that cross-repo mount has failed (in this case because of blob doesn't exist in the source repository), client needs to reupload the blob
+os::cmd::expect_success_and_text "curl -I -X POST -u 'pusher:${pusher_token}' '${DOCKER_REGISTRY}/v2/crossmount/repo/blobs/uploads/?mount=$rubyimageblob&from=cache/hello-world'" "202 Accepted"

Tests probably need to be both here and in the unit tests.

Auth unit tests
https://github.com/openshift/origin/blob/master/pkg/dockerregistry/server/auth_test.go
seem sufficient to me. Additional negative tests have been just added by
@liggitt https://github.com/liggitt's d179662
#diff-4ea7c8f0aaeae849c3760842d6b42c60R194
d179662#diff-4ea7c8f0aaeae849c3760842d6b42c60R194

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/9819/files/1742dd1c7e2f56e7e52207e5c024ed830ea84fca..29c2108e28e979ea31cba31a000179b8c1073bd5#r71515054,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p2Dy62mVK6zrq3BOM54Vd-LWd_0_ks5qXhUIgaJpZM4JLnQb
.

we need blob tests for authenticated users, and users with similar / overlapping namespace permissions.

I'm not sure what is needed here. Is a scenario where:

bob creates a namespace N and tags image there, he can pull and push

alice cannot pull, nor push to N

admin gives alice an admin role in N, she can pull and push there

admin gives joe a view role in N, he can only pull

admin gives joe an edit role as will, he can now pull and push

Enough? Or do you have something more complex in mind?

We also need to explicitly call out the cached permissions behavior with a test.

The cache part is problematic or rather broken. Our cache evicts with configured timeout. Upstream inmemory cache, however, does not. So when a stat on a blob X in a repository R happens after a manual untag of an image containing it:

our blobDescriptorService finds out that the X<->R association is too old and removes it

it passes the request down to cachedBlobStatter that finds the association and succeeds

With the manual untag, the blob won't ever be cleared from the cachedBlobStatter until registry's restart.

During the image pruning, the blob will be cleared thoroughly both from the cache and the storage.

I can add a test case for the pruning case. The former case is unfortunately broken unless we disable or implement our own replacement for inmemory cache or until we store the links in etcd.

liggitt · 2016-08-05T04:15:55Z

pkg/dockerregistry/server/digestcache.go

+// ForgetDigest removes an association between given digest and repository from the cache.
+func (c digestToRepositoryCache) ForgetDigest(dgst digest.Digest, repo string) {
+	key := dgst.String()
+	value, ok := c.Get(key)


not sure we want to bump to the front of the LRU cache when removing a repo... maybe use Peek() instead?

not sure we want to bump to the front of the LRU cache when removing a repo... maybe use Peek() instead?

Good catch. Fixed.

liggitt · 2016-08-05T05:01:56Z

nits on digestcache, can address in a follow-up if you want, LGTM

miminar · 2016-08-05T08:36:27Z

nits on digestcache, can address in a follow-up if you want, LGTM

Thanks a lot! It's totally different PR now with your suggestions applied. But each one made it a lot better.

For security reasons, evict stale pairs of (blob, repository) from cache. So when an image is untagged from image stream, registry will deny access to its blobs. Also fixed a bug in digestcache where the first association for particular digest was lost and once. Signed-off-by: Michal Minář <[email protected]>

Turned blob repository ttl into a config option. Also allowed for overrides using env var REGISTRY_MIDDLEWARE_REPOSITORY_OPENSHIFT_BLOBREPOSITORYCACHETTL. Signed-off-by: Michal Minar <[email protected]>

Use much smaller image for pulls. Also Deal with multiple image candidates for deletion. Signed-off-by: Michal Minář <[email protected]>

Signed-off-by: Michal Minář <[email protected]>

miminar · 2016-08-05T09:39:25Z

Flake #10008

openshift-bot · 2016-08-05T09:52:23Z

Evaluated for origin test up to db956d8

openshift-bot · 2016-08-05T11:12:52Z

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

liggitt · 2016-08-05T13:04:21Z

[merge]

openshift-bot · 2016-08-06T01:57:23Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7543/) (Image: devenv-rhel7_4779)

smarterclayton · 2016-08-07T19:05:43Z

[merge] #9938

openshift-bot · 2016-08-07T19:06:05Z

Evaluated for origin merge up to db956d8

miminar force-pushed the remote-layer-federation branch 2 times, most recently from 31602ff to cf0b321 Compare July 14, 2016 13:33

miminar changed the title ~~[WIP] User can get only blobs he's able to see~~ User can get only blobs he's able to see Jul 14, 2016

smarterclayton reviewed Jul 14, 2016
View reviewed changes

miminar force-pushed the remote-layer-federation branch from cf0b321 to 66991bf Compare July 14, 2016 15:42

smarterclayton reviewed Jul 14, 2016
View reviewed changes

miminar force-pushed the remote-layer-federation branch 3 times, most recently from 614f3bb to 1bb6fba Compare July 14, 2016 16:31

smarterclayton reviewed Jul 14, 2016
View reviewed changes

miminar force-pushed the remote-layer-federation branch 3 times, most recently from 67e7975 to 29c2108 Compare July 18, 2016 09:31

soltysh reviewed Jul 18, 2016
View reviewed changes

smarterclayton reviewed Jul 19, 2016
View reviewed changes

miminar force-pushed the remote-layer-federation branch 3 times, most recently from 5efda24 to 82fc7c9 Compare August 4, 2016 19:41

liggitt reviewed Aug 5, 2016
View reviewed changes

miminar force-pushed the remote-layer-federation branch from 82fc7c9 to b8326dc Compare August 5, 2016 08:32

miminar force-pushed the remote-layer-federation branch from b8326dc to 25f06a6 Compare August 5, 2016 08:38

Michal Minar added 6 commits August 5, 2016 10:45

Configurable blobrepositorycachettl value

4ac83cb

Turned blob repository ttl into a config option. Also allowed for overrides using env var REGISTRY_MIDDLEWARE_REPOSITORY_OPENSHIFT_BLOBREPOSITORYCACHETTL. Signed-off-by: Michal Minar <[email protected]>

e2e: speed-up docker repository pull tests

ddf1151

Use much smaller image for pulls. Also Deal with multiple image candidates for deletion. Signed-off-by: Michal Minář <[email protected]>

e2e: added tests for cross-repo mounting

3f1c7a9

Signed-off-by: Michal Minář <[email protected]>

Allow to mock default registry client

f4f98fa

Signed-off-by: Michal Minář <[email protected]>

Added unit tests for repository and blobdescriptorservice

db956d8

Signed-off-by: Michal Minář <[email protected]>

miminar force-pushed the remote-layer-federation branch from 25f06a6 to db956d8 Compare August 5, 2016 08:45

miminar mentioned this pull request Aug 5, 2016

Registry config update openshift/openshift-docs#2609

Merged

openshift-bot merged commit 7998ae4 into openshift:master Aug 7, 2016

miminar deleted the remote-layer-federation branch August 8, 2016 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User can get only blobs he's able to see #9819

User can get only blobs he's able to see #9819

miminar commented Jul 13, 2016 •

edited

Loading

miminar commented Jul 13, 2016

miminar commented Jul 13, 2016

miminar commented Jul 14, 2016

smarterclayton Jul 14, 2016

miminar Jul 14, 2016

smarterclayton Jul 14, 2016

miminar Jul 14, 2016

smarterclayton Jul 14, 2016

miminar Jul 16, 2016

soltysh Jul 18, 2016

soltysh commented Jul 18, 2016

smarterclayton Jul 19, 2016

miminar Jul 20, 2016

smarterclayton Jul 20, 2016

miminar Jul 20, 2016

liggitt Aug 5, 2016

miminar Aug 5, 2016

liggitt commented Aug 5, 2016

miminar commented Aug 5, 2016

miminar commented Aug 5, 2016

openshift-bot commented Aug 5, 2016

openshift-bot commented Aug 5, 2016

liggitt commented Aug 5, 2016 •

edited

Loading

openshift-bot commented Aug 6, 2016 •

edited

Loading

smarterclayton commented Aug 7, 2016

openshift-bot commented Aug 7, 2016

User can get only blobs he's able to see #9819

User can get only blobs he's able to see #9819

Conversation

miminar commented Jul 13, 2016 • edited Loading

miminar commented Jul 13, 2016

miminar commented Jul 13, 2016

miminar commented Jul 14, 2016

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

soltysh commented Jul 18, 2016

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

liggitt commented Aug 5, 2016

miminar commented Aug 5, 2016

miminar commented Aug 5, 2016

openshift-bot commented Aug 5, 2016

openshift-bot commented Aug 5, 2016

liggitt commented Aug 5, 2016 • edited Loading

openshift-bot commented Aug 6, 2016 • edited Loading

smarterclayton commented Aug 7, 2016

openshift-bot commented Aug 7, 2016

miminar commented Jul 13, 2016 •

edited

Loading

liggitt commented Aug 5, 2016 •

edited

Loading

openshift-bot commented Aug 6, 2016 •

edited

Loading