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

Check pull access when tagging imagestreams #10109

Merged
merged 1 commit into from Aug 2, 2016

Conversation

Projects
None yet
5 participants
@liggitt
Contributor

liggitt commented Jul 29, 2016

When tagging across namespaces, a user must have pull permission on the source image stream. This means they need get access on the imagestreams/layers resource in the source namespace. The admin, edit, and system:image-puller roles all grant this permission.

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Jul 29, 2016

Contributor

@smarterclayton, shouldn't we be checking pull access for an image stream layer, not just view access on the image stream object?

@deads2k for scoped usage

Contributor

liggitt commented Jul 29, 2016

@smarterclayton, shouldn't we be checking pull access for an image stream layer, not just view access on the image stream object?

@deads2k for scoped usage

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Jul 30, 2016

Member
Member

smarterclayton commented Jul 30, 2016

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Jul 30, 2016

Contributor

imagestreams/layers is what the registry checks before letting you pull that image stream

Contributor

liggitt commented Jul 30, 2016

imagestreams/layers is what the registry checks before letting you pull that image stream

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Jul 30, 2016

Contributor

[test]

Contributor

liggitt commented Jul 30, 2016

[test]

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Jul 31, 2016

Member

Hope this doesn't break too many people. Release note please?

Member

smarterclayton commented Jul 31, 2016

Hope this doesn't break too many people. Release note please?

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Jul 31, 2016

Member

Agree with change (logically tag and pull/push are identical permissions), is there an easy test case to add?

Does this have any implications for third party clients?

Member

smarterclayton commented Jul 31, 2016

Agree with change (logically tag and pull/push are identical permissions), is there an easy test case to add?

Does this have any implications for third party clients?

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Jul 31, 2016

Member
Member

smarterclayton commented Jul 31, 2016

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Jul 31, 2016

Contributor

Hope this doesn't break too many people. Release note please?

The only role we have that allows someone to get imagestreams but not pull is the view role. Remind me where we're collecting release notes again?

Deployments can tag images in their hook so the deployer SA needs this permission.

This check is only done when tagging from another namespace... no deployer SA will ever automatically have a permission in another namespace.

Contributor

liggitt commented Jul 31, 2016

Hope this doesn't break too many people. Release note please?

The only role we have that allows someone to get imagestreams but not pull is the view role. Remind me where we're collecting release notes again?

Deployments can tag images in their hook so the deployer SA needs this permission.

This check is only done when tagging from another namespace... no deployer SA will ever automatically have a permission in another namespace.

@@ -433,19 +432,18 @@ func (v *TagVerifier) Verify(old, stream *api.ImageStream, user user.Info) field
continue
}
subjectAccessReview := authorizationapi.SubjectAccessReview{
// Make sure this user can pull the specified image before allowing them to tag it into another imagestream
subjectAccessReview := authorizationapi.AddUserToSAR(user, &authorizationapi.SubjectAccessReview{

This comment has been minimized.

@deads2k

deads2k Aug 1, 2016

Contributor

AddUserToSAR usage is correct.

@deads2k

deads2k Aug 1, 2016

Contributor

AddUserToSAR usage is correct.

@stevekuznetsov

This comment has been minimized.

Show comment
Hide comment
@stevekuznetsov

stevekuznetsov Aug 1, 2016

Contributor

re[test]

Contributor

stevekuznetsov commented Aug 1, 2016

re[test]

@openshift-bot

This comment has been minimized.

Show comment
Hide comment
@openshift-bot

openshift-bot Aug 2, 2016

Evaluated for origin test up to 8115614

openshift-bot commented Aug 2, 2016

Evaluated for origin test up to 8115614

@openshift-bot

This comment has been minimized.

Show comment
Hide comment
@openshift-bot

openshift-bot Aug 2, 2016

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

openshift-bot commented Aug 2, 2016

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

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Aug 2, 2016

Contributor

[merge]

Contributor

liggitt commented Aug 2, 2016

[merge]

@openshift-bot

This comment has been minimized.

Show comment
Hide comment
@openshift-bot

openshift-bot Aug 2, 2016

Evaluated for origin merge up to 8115614

openshift-bot commented Aug 2, 2016

Evaluated for origin merge up to 8115614

@openshift-bot

This comment has been minimized.

Show comment
Hide comment
@openshift-bot

openshift-bot Aug 2, 2016

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

openshift-bot commented Aug 2, 2016

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

@openshift-bot openshift-bot merged commit ea16ac6 into openshift:master Aug 2, 2016

3 checks passed

continuous-integration/openshift-jenkins/merge Passed
Details
continuous-integration/openshift-jenkins/test Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@liggitt liggitt deleted the liggitt:imagestreamtag branch Aug 3, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment