Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix auth checks for cross-repo mount requests #9593

Merged
merged 2 commits into from
Jun 29, 2016
Merged

Fix auth checks for cross-repo mount requests #9593

merged 2 commits into from
Jun 29, 2016

Conversation

liggitt
Copy link
Contributor

@liggitt liggitt commented Jun 28, 2016

Fixes #9540

When pushing blobs to a target repo targetRepo, if the client has knowledge that the blob exists in another repo sourceRepo on the same server, clients may request the server cross-mount a blob from another repo sourceRepo as an optimization.

Before allowing that, the server ensures the client has the following authorizations:

  • pull and push on targetRepo
  • pull on sourceRepo

In cases where the user is not authorized to pull from sourceRepo, rather than an authorization error, the blob push should just proceed unoptimized (requiring the user to push the full content of the blob to targetRepo, which proves they have access to it, since they just pushed it)

This PR:

  • detects if pull access check failures are likely to be related to cross-repo mounts:
    • there was no push access request for the repo which failed the pull access check
    • there was at least one push access request for some other repo (which is required to pass)
  • adds errors related to those pull access checks to the request context, keyed by repo (ns+name)
  • updates the blobstore, tagregistry, and manifestregistry to check the request context for access errors for the relevant repo before performing any action
  • updates the blobstore Create method to disable cross-repo mounting if the request context contains an access error for the source repo

@miminar
Copy link

miminar commented Jun 28, 2016

I like it! Thanks. I'll cherry-pick to my #9582.

@miminar miminar mentioned this pull request Jun 28, 2016
@@ -197,7 +237,11 @@ func (ac *AccessController) Authorized(ctx context.Context, accessRecords ...reg
verifiedPrune = true
default:
if err := verifyImageStreamAccess(ctx, imageStreamNS, imageStreamName, verb, osClient); err != nil {
return nil, ac.wrapErr(err)
if verb == "pull" {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a possibility for the verb be set to "pull".

@miminar
Copy link

miminar commented Jun 28, 2016

LGTM

@liggitt
Copy link
Contributor Author

liggitt commented Jun 28, 2016

[test]

@liggitt liggitt changed the title [DO NOT MERGE] POC for deferred auth errors Fix auth checks for cross-repo mount requests Jun 28, 2016
@liggitt
Copy link
Contributor Author

liggitt commented Jun 28, 2016

[testextended][extended:core(builds)]

@liggitt
Copy link
Contributor Author

liggitt commented Jun 28, 2016

[testextended][extended:core(images)]

@openshift-bot
Copy link
Contributor

Evaluated for origin testextended up to 860fa7f

@liggitt
Copy link
Contributor Author

liggitt commented Jun 28, 2016

[merge]

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 28, 2016

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

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 860fa7f

@liggitt
Copy link
Contributor Author

liggitt commented Jun 28, 2016

fyi @smarterclayton

@openshift-bot
Copy link
Contributor

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

@liggitt
Copy link
Contributor Author

liggitt commented Jun 28, 2016

Re[test] #9512

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 860fa7f

@openshift-bot
Copy link
Contributor

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

@openshift-bot openshift-bot merged commit c805ff4 into openshift:master Jun 29, 2016
@@ -197,7 +237,11 @@ func (ac *AccessController) Authorized(ctx context.Context, accessRecords ...reg
verifiedPrune = true
default:
if err := verifyImageStreamAccess(ctx, imageStreamNS, imageStreamName, verb, osClient); err != nil {
return nil, ac.wrapErr(err)
if access.Action == "pull" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: invert and return early

@liggitt liggitt deleted the registry-deferred-auth-error branch June 29, 2016 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants