Skip to content

Conversation

@miminar
Copy link

@miminar miminar commented Jun 6, 2016

The new images/signatures resource allows to update image' signatures. There's a new images API client call images.UpdateSignatures() for this purpose.

A cluster-wide role system:image-signer is necessary to add/remove/modify signatures.

A follow-up for #8371

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 6, 2016
@miminar miminar force-pushed the image-signatures-rest branch from d19f2fc to 7ad1c59 Compare June 6, 2016 17:33
@miminar
Copy link
Author

miminar commented Jun 6, 2016

/cc @mtrmac @deads2k

[test]

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 6, 2016
@miminar miminar force-pushed the image-signatures-rest branch 2 times, most recently from 39bcb8f to fa3fa4b Compare June 7, 2016 11:10
@soltysh
Copy link
Contributor

soltysh commented Jun 7, 2016

Will review once the first part is in.

@mtrmac
Copy link
Contributor

mtrmac commented Jun 7, 2016

Thanks, I have successfully used this from an independently written client in containers/skopeo#93 ; but I ran into a fairly serious difficulty about the permissions design.

The way things work now, the first user uploading a specific image can include any signatures they want in the original imagestreammapping object; any other user uploading the image, even in a different namespace and completely unaware of the first user, has signatures in their imagestreammapping ignored, and when they are not a member of the system:image-signer role, they can not add their signatures at all.

This is bad for multitenancy or for the stand-alone Atomic Registry; everyone should be able to upload any images they please in their own namespace, and add any signatures they please to images in their own namespace.

It is fine if removing signatures is restricted; adding a signature should never break anything or cause the image to be removed, so removing signatures is not needed except for an “approval/revoking approval” workflow.

Could the permission design be modified so that adding signatures (through the new /images//signatures endpoint at least, possibly also through an imagestreammapping) did not require any additional permissions?

(Even with that, the signatures would not be namespaced, and the existence a tenant’s signature would be visible to other tenants. That is a slight privacy leak, but perhaps not a showstopper.)

@liggitt
Copy link
Contributor

liggitt commented Jun 7, 2016

the first user uploading a specific image can include any signatures they want in the original imagestreammapping object

@miminar, is that intentional?

Could the permission design be modified so that adding signatures (through the new /images/…/signatures endpoint at least, possibly also through an imagestreammapping)...

We wouldn't let normal users interact with unnamespaced objects in that way. If we wanted regular users to add signatures, there would need to be a namespaced API to allow that.

...did not require any additional permissions?

I would expect it to be a subresource, which would allow granular control over whether signing was enabled. Access to that subresource could possibly be added to existing roles if that makes sense.

@deads2k
Copy link
Contributor

deads2k commented Jun 7, 2016

I think we can resolve many use-cases by having signatures live as a subresource on images. The status-y parts would be stripped when POSTing to the subresource. The status-y parts could be set via a direct image update/patch or via a status endpoint (if we split the signature, but right now nothing in an image is split along spec/status lines). This would mean that image signatures start out as only available to people more trusted that project-admins and editors.

I agree that we'll end up wanting to allow any editor to add a signature to an image, but the "scope" of that signature is less clear. Because there are size limits for an etcd record, allowing any editor to add any number of signatures won't scale out for large environments. There are definitely cases where you want to share the signature (QA signs an image and the production namespace should get that sig when they retag) and there are definitely cases where you don't want to share it ("bob signed his istag of the openshift/rhel 7.2, but I don't know who bob is and don't care"). If we stored these signatures in a different etcd hierarchy, we could automatically inject signatures from your namespace. If you want to see other signatures, maybe you need a different API call with a special filter.

Either way, I think I see the images/signatures endpoint as a good starting point for "special" components to use for signing.

@miminar
Copy link
Author

miminar commented Jun 8, 2016

the first user uploading a specific image can include any signatures they want in the original imagestreammapping object

@miminar, is that intentional?

It is. An uploader of a new image should be allowed to upload any signatures he wants. New image is tagged just in one namespace so the uploader may be considered as the sole owner of the image at that time.

Modification of signatures of existing image is another story because the image may be tagged in multiple namespaces with different owners. That's why changing the existing signatures is restricted.

Could the permission design be modified so that adding signatures (through the new /images/…/signatures endpoint at least, possibly also through an imagestreammapping) did not require any additional permissions?

This could be done. I'm however reluctant to allow removal of existing signatures while creating images using imagestreammapping. In this case we could just append all the new signatures to the old ones.

However, I don't consider imagestreammapping to be the right resource for updating image signatures. It's designed for registry to create a new image entry and tag it into existing image stream at the same time.

For this purpose, I'd rather add a new subresource imagestreamimages/signatures or perhaps just add an AddSignatures() call toimagestreamimages. It would permit just additions. It would be scoped to user's namespace, so he would be able to add signatures to any image tagged into his namespace.

Any signature removal of existing image should be done through the unnamespaced images/signatures resource requiring cluster-wide image-signer role.

Either way, I think I see the images/signatures endpoint as a good starting point for "special" components to use for signing.

👍

@liggitt
Copy link
Contributor

liggitt commented Jun 8, 2016

An uploader of a new image should be allowed to upload any signatures he wants. New image is tagged just in one namespace so the uploader may be considered as the sole owner of the image at that time.

I'm not sure whether I'd expect the first person to push an image to be able to surface their signatures into any other namespace they want. @smarterclayton, was that what you expected?

@miminar
Copy link
Author

miminar commented Jun 8, 2016

I'm not sure whether I'd expect the first person to push an image to be able to surface their signatures into any other namespace they want. @smarterclayton, was that what you expected?

@liggitt shall we rather allow for per-project image signatures? And allow to pin a signature to an image by a cluster image-signer. I don't disagree. My only concern is signature redundancy and possible complexity.

@smarterclayton
Copy link
Contributor

With pull through present, it's not sufficient to say "first pusher gets to
push signatures". For now, only admins can push signatures. David and I
went through some options here, but for the short term signatures are for
admins and infrastructure, and these APIs are targeted towards that.

On Wed, Jun 8, 2016 at 9:41 AM, Michal Minar notifications@github.com
wrote:

I'm not sure whether I'd expect the first person to push an image to be
able to surface their signatures into any other namespace they want.
@smarterclayton https://github.com/smarterclayton, was that what you
expected?

@liggitt https://github.com/liggitt shall we rather allow for
per-project image signatures? And allow to pin a signature to an image
by a cluster image-signer. I don't disagree. My only concern is
signature redundancy and possible complexity.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9181 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p5JpexFFrnZ9rUVBnCdYBx36Mou2ks5qJsZtgaJpZM4IvEFx
.

@mtrmac
Copy link
Contributor

mtrmac commented Jun 8, 2016

Either way, I think I see the images/signatures endpoint as a good starting point for "special" components to use for signing.

👍

Yeah, let's get this merged and then figure out the per-namespace aspect; it does need to be figured out eventually.

@miminar miminar changed the title Added REST storage for images/signatures resource [Blocked] Added REST storage for images/signatures resource Jun 9, 2016
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2016
@miminar miminar force-pushed the image-signatures-rest branch from fa3fa4b to 9ff4bf4 Compare June 13, 2016 07:26
@miminar miminar changed the title [Blocked] Added REST storage for images/signatures resource Added REST storage for images/signatures resource Jun 13, 2016
@miminar miminar mentioned this pull request Jun 13, 2016
3 tasks
@miminar
Copy link
Author

miminar commented Jun 13, 2016

Rebased. Removed an option to set signatures during Create(image). Signatures can now be updated only using images/signatures subresource.

We can add imagestream-scoped modification of iimage signatures (additions only) in a follow-up PR.

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2016
@smarterclayton
Copy link
Contributor

Let's hold off on adding user focused signing until we have a design
proposal for securing it and limiting the impact on the cluster. There
were enough questions raised that I'm not sure I want to go forward until
we have agreement. Let's focus on admin and infra signing for now.

On Mon, Jun 13, 2016 at 4:38 AM, OpenShift Bot notifications@github.com
wrote:

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


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9181 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_px6o_GqN14L_RUQsfgFzOYto_8dJks5qLRchgaJpZM4IvEFx
.

OpenshiftExposedGroupName: {BuildGroupName, ImageGroupName, DeploymentGroupName, TemplateGroupName, "routes"},
OpenshiftAllGroupName: {OpenshiftExposedGroupName, UserGroupName, OAuthGroupName, PolicyOwnerGroupName, SDNGroupName, PermissionGrantingGroupName, OpenshiftStatusGroupName, "projects",
"clusterroles", "clusterrolebindings", "clusterpolicies", "clusterpolicybindings", "images" /* cluster scoped*/, "projectrequests", "builds/details", "imagestreams/secrets",
"clusterroles", "clusterrolebindings", "clusterpolicies", "clusterpolicybindings", "images", "images/signatures" /* cluster scoped*/, "projectrequests", "builds/details", "imagestreams/secrets",
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't add anything else to this file. See line 7.

@openshift-bot
Copy link
Contributor

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

@deads2k
Copy link
Contributor

deads2k commented Jul 22, 2016

lgtm [merge]

@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 22, 2016

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

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to cf163c2

@miminar
Copy link
Author

miminar commented Jul 22, 2016

@deads2k, @mtrmac thank you! I'm glad we came to settlement ... finally 😄

Thanks to all the involved for valuable input.

@openshift-bot openshift-bot merged commit d2a687d into openshift:master Jul 23, 2016
mtrmac added a commit to mtrmac/skopeo that referenced this pull request Jul 25, 2016
mtrmac added a commit to mtrmac/skopeo that referenced this pull request Jul 30, 2016
This can only work against the image-signatures-rest branch for
openshift/origin#9181 .

Note the need for openshiftCluster.relaxImageSignerPermissions.
mtrmac added a commit to mtrmac/skopeo that referenced this pull request Aug 1, 2016
This reverts commit 7f9c56a.

Also fixes build with updated OpenShift hack/common.ss: The computation
of OS_GIT_MAJOR and OS_GIT_MINOR was failing because miminar/origin
didn’t carry the expected recent tags.
mtrmac added a commit to mtrmac/skopeo that referenced this pull request Aug 1, 2016
This can only work against the image-signatures-rest branch for
openshift/origin#9181 .

Note the need for openshiftCluster.relaxImageSignerPermissions.
mtrmac added a commit to mtrmac/skopeo that referenced this pull request Aug 1, 2016
This reverts commit 7f9c56a.

Also fixes build with updated OpenShift hack/common.sh: The computation
of OS_GIT_MAJOR and OS_GIT_MINOR was failing because miminar/origin
didn’t carry the expected recent tags.
mtrmac added a commit to mtrmac/skopeo that referenced this pull request Aug 4, 2016
This reverts commit 7f9c56a.

Also fixes build with updated OpenShift hack/common.sh: The computation
of OS_GIT_MAJOR and OS_GIT_MINOR was failing because miminar/origin
didn’t carry the expected recent tags.
mtrmac added a commit to mtrmac/skopeo that referenced this pull request Aug 5, 2016
This reverts commit 7f9c56a.

Also fixes build with updated OpenShift hack/common.sh: The computation
of OS_GIT_MAJOR and OS_GIT_MINOR was failing because miminar/origin
didn’t carry the expected recent tags.
@miminar miminar deleted the image-signatures-rest branch August 8, 2016 13:13
mtrmac added a commit to mtrmac/skopeo that referenced this pull request Aug 10, 2016
This reverts commit 7f9c56a.

Also fixes build with updated OpenShift hack/common.sh: The computation
of OS_GIT_MAJOR and OS_GIT_MINOR was failing because miminar/origin
didn’t carry the expected recent tags.
runcom pushed a commit to containers/skopeo that referenced this pull request Aug 25, 2016
Uses a tag created after merging that PR.  (git clone -b …) does not
work with commit IDs, and we like to use a released version anyway.
Jamesjaj2dc6j added a commit to Jamesjaj2dc6j/lucascalsilvaa that referenced this pull request Jun 6, 2022
This builds from the image-signatures-rest branch for
openshift/origin#9181 .

Testing push, pull, streaming.

Does not test working with the other Docker registries built in
Dockerfile; I will leave that to the author of that code :)

Note that this relies on an internet connection for pulling from the
Docker Hub (which is incidentally tested by that); pushing to no Docker
Registry, neither local nor Hub, is tested by this.

The tests only run in a container because the (oc login) / (docker
login)-like code modifies files in a home directory; the new
SKOPEO_CONTAINER_TESTS environment variable should protect against
accidental non-container runs.
mrhyperbit23z0d added a commit to mrhyperbit23z0d/thomasdarimont that referenced this pull request Jun 6, 2022
This builds from the image-signatures-rest branch for
openshift/origin#9181 .

Testing push, pull, streaming.

Does not test working with the other Docker registries built in
Dockerfile; I will leave that to the author of that code :)

Note that this relies on an internet connection for pulling from the
Docker Hub (which is incidentally tested by that); pushing to no Docker
Registry, neither local nor Hub, is tested by this.

The tests only run in a container because the (oc login) / (docker
login)-like code modifies files in a home directory; the new
SKOPEO_CONTAINER_TESTS environment variable should protect against
accidental non-container runs.
easyriderk0c9g added a commit to easyriderk0c9g/rnin9 that referenced this pull request Jun 6, 2022
This builds from the image-signatures-rest branch for
openshift/origin#9181 .

Testing push, pull, streaming.

Does not test working with the other Docker registries built in
Dockerfile; I will leave that to the author of that code :)

Note that this relies on an internet connection for pulling from the
Docker Hub (which is incidentally tested by that); pushing to no Docker
Registry, neither local nor Hub, is tested by this.

The tests only run in a container because the (oc login) / (docker
login)-like code modifies files in a home directory; the new
SKOPEO_CONTAINER_TESTS environment variable should protect against
accidental non-container runs.
oguzturker8ijm8l added a commit to oguzturker8ijm8l/diwir that referenced this pull request Jun 6, 2022
This builds from the image-signatures-rest branch for
openshift/origin#9181 .

Testing push, pull, streaming.

Does not test working with the other Docker registries built in
Dockerfile; I will leave that to the author of that code :)

Note that this relies on an internet connection for pulling from the
Docker Hub (which is incidentally tested by that); pushing to no Docker
Registry, neither local nor Hub, is tested by this.

The tests only run in a container because the (oc login) / (docker
login)-like code modifies files in a home directory; the new
SKOPEO_CONTAINER_TESTS environment variable should protect against
accidental non-container runs.
wesnowm added a commit to wesnowm/MatfOOP- that referenced this pull request Jun 6, 2022
This builds from the image-signatures-rest branch for
openshift/origin#9181 .

Testing push, pull, streaming.

Does not test working with the other Docker registries built in
Dockerfile; I will leave that to the author of that code :)

Note that this relies on an internet connection for pulling from the
Docker Hub (which is incidentally tested by that); pushing to no Docker
Registry, neither local nor Hub, is tested by this.

The tests only run in a container because the (oc login) / (docker
login)-like code modifies files in a home directory; the new
SKOPEO_CONTAINER_TESTS environment variable should protect against
accidental non-container runs.
gaveen3 added a commit to gaveen3/KimJeongHoon3x that referenced this pull request Jun 6, 2022
This builds from the image-signatures-rest branch for
openshift/origin#9181 .

Testing push, pull, streaming.

Does not test working with the other Docker registries built in
Dockerfile; I will leave that to the author of that code :)

Note that this relies on an internet connection for pulling from the
Docker Hub (which is incidentally tested by that); pushing to no Docker
Registry, neither local nor Hub, is tested by this.

The tests only run in a container because the (oc login) / (docker
login)-like code modifies files in a home directory; the new
SKOPEO_CONTAINER_TESTS environment variable should protect against
accidental non-container runs.
gaveen3 added a commit to gaveen3/KimJeongHoon3x that referenced this pull request Jun 6, 2022
This builds from the image-signatures-rest branch for
openshift/origin#9181 .

Testing push, pull, streaming.

Does not test working with the other Docker registries built in
Dockerfile; I will leave that to the author of that code :)

Note that this relies on an internet connection for pulling from the
Docker Hub (which is incidentally tested by that); pushing to no Docker
Registry, neither local nor Hub, is tested by this.

The tests only run in a container because the (oc login) / (docker
login)-like code modifies files in a home directory; the new
SKOPEO_CONTAINER_TESTS environment variable should protect against
accidental non-container runs.
Manuel-Suarez-Abascal80n9y added a commit to Manuel-Suarez-Abascal80n9y/knimeu that referenced this pull request Oct 7, 2022
This builds from the image-signatures-rest branch for
openshift/origin#9181 .

Testing push, pull, streaming.

Does not test working with the other Docker registries built in
Dockerfile; I will leave that to the author of that code :)

Note that this relies on an internet connection for pulling from the
Docker Hub (which is incidentally tested by that); pushing to no Docker
Registry, neither local nor Hub, is tested by this.

The tests only run in a container because the (oc login) / (docker
login)-like code modifies files in a home directory; the new
SKOPEO_CONTAINER_TESTS environment variable should protect against
accidental non-container runs.
straiforos8bsh5n added a commit to straiforos8bsh5n/tokingsq that referenced this pull request Oct 7, 2022
This builds from the image-signatures-rest branch for
openshift/origin#9181 .

Testing push, pull, streaming.

Does not test working with the other Docker registries built in
Dockerfile; I will leave that to the author of that code :)

Note that this relies on an internet connection for pulling from the
Docker Hub (which is incidentally tested by that); pushing to no Docker
Registry, neither local nor Hub, is tested by this.

The tests only run in a container because the (oc login) / (docker
login)-like code modifies files in a home directory; the new
SKOPEO_CONTAINER_TESTS environment variable should protect against
accidental non-container runs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants