Add registry endpoint to read and write image signatures #12504

mfojtik · 2017-01-16T17:04:46Z

This endpoint handles writing and reading of Image signatures in Docker Registry. It talks to OpenShift API for storing/reading the signatures.

mfojtik · 2017-01-16T17:05:55Z

@smarterclayton @miminar @legionus @pweil- PTAL at the idea.

This seems simple enough to not do and instead just require ansible to distribute master api endpoint to all nodes for Docker.

miminar

I like the idea. Thanks for the PR.

miminar · 2017-01-17T08:10:04Z

pkg/cmd/dockerregistry/dockerregistry.go

+	app.Config.HTTP.Headers.Set("X-Registry-Supports-Signatures", "1")
+
+	app.RegisterRoute(
+		// GET /admin/signatures/<reference>


There may be another resources we may want to expose in the future. Can we nest signatures under /atomic/ or something similar?

signatures alone can be confused with signatures of manifest V2 schema 1. With atomic qualifier it's harder to mix up.

Also, another route should be repository-scoped so that non-admins can GET signatures of images in their own repositories.

@miminar I think we don't need to make this repository scoped since you just re-use the token you use for pulling the images. I'm just passing the auth down to Origin API and I'm not doing any complex auth stuff that involve registry service account.

@miminar i'm fine to scope this under /atomic (or other route, like /extensions or /openshift since this is openshift specific).

@miminar It's just PoC. The admin prefix was used to make it smaller. For full implementation we may have to change this endpoint for greater integration into the other code.

I don't think we want to patch the upstream code (and I don't think this is something that upstream will benefit from).

We don't need to patch it. We already have everything for this.

as far I can see it use the token of the registry service account to make requests.

If we ever want to make endpoint to store signatures I don't think we want to give users to change images. So we also have to do it using the registry service account.

If we ever want to make endpoint to store signatures I don't think we want to give users to change images. So we also have to do it using the registry service account.

We don't have to. The user needs to have image-signerrole to do so, thus the attempt to POST to image signature endpoint in OpenShift will return 400?
I don't think we have to check this upfront in the auth module?

@legionus the auth middleware was not built do deal with this "proxy" auth, right?

@mfojtik Also our auth middleware allows to authenticate user by token.

@legionus I tried UserClientFrom(context) there but it failed :-)

Also, another route should be repository-scoped so that non-admins can GET signatures of images in their own repositories.

(I don’t know whether repository scoping is necessary for that, but exactly the set of people allowed to read the manifest should be allowed to read the signatures. Admin-only-readable signatures would be fairly useless.)

miminar · 2017-01-17T08:13:51Z

pkg/dockerregistry/server/signature.go

+	context.GetLogger(s.Context).Debugf("(*signatureHandler).Get")
+	client, ok := UserClientFrom(s.Context)
+	if !ok {
+		context.GetLogger(s.Context).Debugf("(*signatureHandler).Get: unable to get openshift client")


s/openshift/origin/

miminar · 2017-01-17T08:49:20Z

pkg/dockerregistry/server/signature.go

+
+	ref, err := imageapi.ParseDockerImageReference(s.Reference)
+	if err != nil {
+		context.GetLogger(s.Context).Debugf("(*signatureHandler).Get: %v", err)


s/Debugf/Errorf/ and the same for all the other error handling below.

miminar · 2017-01-17T08:59:46Z

pkg/dockerregistry/server/signature.go

+	}
+
+	image, err := client.ImageStreamImages(ref.Namespace).Get(ref.Name, ref.ID)
+	if err != nil {


This may be unauthorized, forbidden, not found, bad input, etc. We should return corresponding http status for all the cases.

is there any helper to do so? I'm kind of hesitant to make this complex error handing ourself.

No forbidden. Otherwise, the client will be able to obtain information about the presence of the image on which he has no rights.

miminar · 2017-01-17T09:04:36Z

pkg/dockerregistry/server/signature.go

+	if err != nil {
+		w.WriteHeader(http.StatusNotFound)
+		context.GetLogger(s.Context).Debugf("(*signatureHandler).Get: %v", err)
+		return


I'd prefer to use serialized distribution's error objects.

miminar · 2017-01-17T10:07:16Z

pkg/dockerregistry/server/signature.go

+
+	data, err := json.Marshal(versionedSignatures)
+	if err != nil {
+		handleError(s.Context, errcode.ErrorCodeUnknown.WithDetail(fmt.Sprintf("failed to serialize image signature %v", err)), w)


Please, log the errors as well.

miminar · 2017-01-17T10:09:45Z

pkg/dockerregistry/server/auth.go

+			case "get":
+				// For /signature/<reference> we pass the request to OpenShift API using the user
+				// token so the authorization happen on the OpenShift API side.
+				continue


I like the idea of handling the authorization for all the routes on one place.

The handler can then safely focus on its job.

handler is passing the user token to openshift api where the auth use the registry client to verify the access. also the URL schema used to get signatures is not standard and will require more tweaking to get the access.Resource.Name...

i'm ok improving this but I guess the current way is enough.

miminar · 2017-01-17T10:10:42Z

pkg/dockerregistry/server/signature.go

+			w.WriteHeader(http.StatusNotFound)
+			return
+		}
+		handleError(s.Context, errcode.ErrorCodeUnknown.WithDetail(fmt.Sprintf("unable to get image %q signature: %v", s.Reference.String(), err)), w)


This still needs to handle unauthorized (or leave the auth to the auth module).

@miminar I would rather check it here than in auth to not mix user token vs. registry SA

miminar · 2017-01-17T10:12:36Z

pkg/cmd/dockerregistry/dockerregistry.go

+	// openshiftRouter groups the OpenShift related registry extensions
+	openshiftRouter := app.NewRoute().PathPrefix("/openshift/").Subrouter()
+
+	signatureRouter := openshiftRouter.NewRoute().PathPrefix("/signature/").Subrouter()


Let's call it /signatures/ to comply with upstream routes, e.g.:

GET /v2/<name>/blobs/<digest> HEAD /v2/<name>/manifests/<reference>

Yes, /v1 or /v2 would be good to add to our prefix.

Also I guess, we can use handlers.NameRequired flag in app.RegisterRoute in this case.

@legionus I think this functionality is not related to /v2 or `/v1' (it should work for both since it is a custom endpoint?)

@mfojtik I meant to do versioning for our prefix to avoid problems in the future.

renamed to /openshift/signatures/<reference>

miminar · 2017-01-17T10:13:28Z

pkg/cmd/dockerregistry/dockerregistry.go

+	if app.Config.HTTP.Headers == nil {
+		app.Config.HTTP.Headers = http.Header{}
+	}
+	app.Config.HTTP.Headers.Set("X-Registry-Supports-Signatures", "1")


Great idea!

miminar · 2017-01-17T10:15:10Z

pkg/dockerregistry/server/signature.go

@@ -0,0 +1,87 @@
+package server


Let's rename the file to signaturedispatcher.go. Or it can live in its own directory e.g. pkg/dockerregistry/server/routes/signatures.go. The server directory is getting too large.

will rename for now and defer bigger refactoring as a follow up

miminar · 2017-01-17T10:26:37Z

pkg/dockerregistry/server/signature.go

+		return
+	}
+
+	image, err := client.ImageStreamImages(s.Reference.Namespace).Get(s.Reference.Name, s.Reference.ID)


So the route must contain namespace and name embedded in the <reference>? In that case, I'd use a route similar to upstream: /openshift/<namespace>/<name>/signatures/<reference> where <reference> is just sha.

@miminar I was trying to make this simple for the clients calling this endpoint. The Docker daemon will don't have to parse the namespace and name from the Docker reference.

legionus · 2017-01-17T10:48:16Z

pkg/cmd/dockerregistry/dockerregistry.go

+		// GET /openshift/signature/<reference>
+		// FIXME: Because the reference.ReferenceRegexp is anchored, we have to strip the
+		// leading '^' from the regexp in order to match the image reference in a sub-path.
+		signatureRouter.Path("/{reference:"+strings.TrimPrefix(reference.ReferenceRegexp.String(), "^")+"}").Methods("GET"),


@mfojtik @miminar do we need endpoint to write signatures ?

@legionus we might in the future. @aweiteka ?

@aweiteka I think I saw a trello card yesterday requesting this, will this be enough for skopeo? (basically you don't have to know the OpenShift Master API URL to write signatures as you can talk to just registry API, assuming you have the token and user have the signing role).

We either need an endpoint to write signatures to land at the same time, or we need an equivalent of #12480 (with the master API address) resurrected; otherwise skopeo would be writing signatures to a different place (“sigstore”) from which it would try reading (this new API).

(Having the write endpoint would be preferable.)

legionus · 2017-01-17T11:25:02Z

pkg/dockerregistry/server/signaturedispatcher.go

+			w.WriteHeader(http.StatusNotFound)
+			return
+		}
+		if kapierrors.IsUnauthorized(err) {


@mfojtik This is information leak. You must check IsUnauthorized first.

@legionus good catch, fixed.

mfojtik · 2017-01-17T11:36:57Z

@openshift/api-review since this is introducing new API to registry:

GET /openshift/signatures/<reference>

This returns []v1.ImageSignature in a JSON form.

deads2k · 2017-01-17T12:46:17Z

Is this matching an API shape that already exists for a docker registry or is this creating a new API that no "normal" client would be able to speak to as an extension to our fork of the docker registry?

mfojtik · 2017-01-17T12:48:17Z

@deads2k it is the later

deads2k · 2017-01-17T12:48:55Z

@deads2k it is the later

Assuming we were interested in doing that, why wouldn't we go with a kube compatible API?

mfojtik · 2017-01-17T12:50:45Z

@deads2k you mean wrap the response as ImageSignatureList (or 3th-party extension?).

deads2k · 2017-01-17T12:56:12Z

@deads2k you mean wrap the response as ImageSignatureList (or 3th-party extension?).

/apis/image.openshift.io/v1/imagesignature (or different group to avoid conflicts). Also, if you're building a special client to interact with this, why can't it just talk to the source of truth directly?

mfojtik · 2017-01-17T13:13:39Z

Moved to /apis/registry.openshift.io/v1/imagesignatures and the response is now:

{"kind":"ImageSignatureList","apiVersion":"v1","metadata":{},"items":[{"metadata":{"name":"sha256:4028782c08eae4a8c9a28bf661c0a8d1c2fc8e19dbaae2b018b21011197e1484@cddeb7006d914716e2728000746a0b23","uid":"12b5e7ca-dc05-11e6-91a3-a268e445cf32","creationTimestamp":"2017-01-16T16:01:42Z"},"type":"atomic","content":"owGbwMvMwMQorp341GLVgXeMpw9kJDFE1LxLq1ZKLsosyUxOzFGyqlbKTEnNK8ksqQSxU/KTs1OLdItS01KLUvOSU5WslHLygeoy8otLrEwNDAz0S1KLS8CEVU4iiFKq1VHKzE1MT0XSnpuYl5kGlNNNyUwHKbFSKs5INDI1szIxMLIwtzBKNrBITUw1SbRItkw0skhKMzMzTDZItEgxTDZKS7ZINbRMSUpMTDVKMjC0SDIyNDA0NLQ0TzU0sTABWVZSWQByVmJJfm5mskJyfl5JYmZeapFCcWZ6XmJJaVEqSFF+QUlmfh7Ef8lFqUDFRQg9BnqGega6KallSkDTMnOBLkzMLVCyAllgamFkamBUW9vJKMPCwMjEwMbKBAowBi5OAVgwSlWy/y/d9+R78teaiSeXHdy78Naj/qreCSVz55U8vrXvOceuLP09b6d5Hq946V4eby3e9qaT7ZlD2MsktcxPrtZ+eqv3nJByuz579lLVitTXbn1pYReubX7wLIm5/MiCj4r/ioPUqp6cDXNlXXJx4oPGDwkih//07T8Wc//ACo0qoQo9jterHXK7ilTNX/5RnZv8S9EwtH9O+mtza+aoGwe5VhVmLqo4y+Rr9s65ft7T+IBLN75u1U9N4FedJbmPZW/5Fu5YyRWXmeqCnid6V3HOf3W8iO3Vf4MJZ9YuDnPPEOJMD5vyikmLq+/KuclXL0kqR5dP3sDyMnh9Q11+icw3c7v3jQarHuiwHzQ3ktD1MgMA"}]}

deads2k · 2017-01-17T13:18:50Z

"apiVersion":"v1" is incorrect. you're missing a group.

mfojtik · 2017-01-17T13:22:47Z

fixed (registry.openshift.io/v1 it is)

pweil- · 2017-01-17T13:30:37Z

if you're building a special client to interact with this, why can't it just talk to the source of truth directly

I feel like this still needs a good justification. As we discussed yesterday on planning, we can embed all kinds of stuff into the registry but at some point we draw a line and say "if you want OpenShift-y stuff then talk to OpenShift". Why doesn't that line apply to signatures stored in the OpenShift API?

openshift-bot · 2017-01-23T09:04:00Z

Evaluated for origin merge up to 03d2e03

mfojtik · 2017-01-23T10:11:13Z

🎆 🍾

This is provided by the OpenShift-integrated registry. This is equivalent to the atomic: transport (in the “openshift”) subpackage, but it requires less code and notably does not require an OpenShift login context to be configured. See openshift/origin#12504 and openshift/openshift-docs#3556 for more information on this API extension. Signed-off-by: Miloslav Trmač <[email protected]>

This is provided by the OpenShift-integrated registry. This is equivalent to the atomic: transport (in the “openshift”) subpackage, but it requires less code and notably does not require an OpenShift login context to be configured. See openshift/origin#12504 and openshift/openshift-docs#3556 for more information on this API extension. To preserve compatibility, we always check for a configured lookaside sigstore first; if that is set up, we use the lookaside and ignore the registry-native signature storage. Usually the user would not bother to set up the lookaside, and use the native mechanism. Signed-off-by: Miloslav Trmač <[email protected]>

This is provided by the OpenShift-integrated registry. This is equivalent to the atomic: transport (in the “openshift”) subpackage, but it requires less code and notably does not require an OpenShift login context to be configured. See openshift/origin#12504 and openshift/openshift-docs#3556 for more information on this API extension. To preserve compatibility, we always check for a configured lookaside sigstore first; if that is set up, we use the lookaside and ignore the registry-native signature storage. Usually the user would not bother to set up the lookaside, and use the native mechanism. The code is mostly trivial; the only non-obvious aspect is the loop in putSignaturesToAPIExtension, which is a pretty direct translation of openshiftImageDestination.PutSignatures. Signed-off-by: Miloslav Trmač <[email protected]>

miminar suggested changes Jan 17, 2017

View reviewed changes

mfojtik force-pushed the poc-registry-signature-endpoint branch from ad85566 to 1109e23 Compare January 17, 2017 10:00

miminar reviewed Jan 17, 2017

View reviewed changes

legionus reviewed Jan 17, 2017

View reviewed changes

mfojtik force-pushed the poc-registry-signature-endpoint branch 3 times, most recently from b410299 to 1519052 Compare January 17, 2017 11:18

legionus reviewed Jan 17, 2017

View reviewed changes

mfojtik force-pushed the poc-registry-signature-endpoint branch from 1519052 to d0b2075 Compare January 17, 2017 11:34

mfojtik added the needs-api-review label Jan 17, 2017

mfojtik force-pushed the poc-registry-signature-endpoint branch from d0b2075 to 1637fed Compare January 17, 2017 12:55

mfojtik force-pushed the poc-registry-signature-endpoint branch from 1637fed to a10f082 Compare January 17, 2017 13:13

mfojtik force-pushed the poc-registry-signature-endpoint branch from a10f082 to c8f3e26 Compare January 17, 2017 13:20

mfojtik mentioned this pull request Jan 23, 2017

Document the experimental registry signature endpoint openshift/openshift-docs#3556

Closed

mtrmac mentioned this pull request Mar 25, 2017

Add support for the X-Registry-Supports-Signatures API extension containers/image#255

Merged

mfojtik deleted the poc-registry-signature-endpoint branch September 5, 2018 21:07

mtrmac mentioned this pull request Jan 30, 2019

update openshift API paths to groupified path containers/image#570

Closed

Add registry endpoint to read and write image signatures #12504

Add registry endpoint to read and write image signatures #12504

Conversation

mfojtik commented Jan 16, 2017 • edited Loading

mfojtik commented Jan 16, 2017

miminar left a 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

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfojtik commented Jan 17, 2017

deads2k commented Jan 17, 2017

mfojtik commented Jan 17, 2017

deads2k commented Jan 17, 2017

mfojtik commented Jan 17, 2017

deads2k commented Jan 17, 2017

mfojtik commented Jan 17, 2017 • edited Loading

deads2k commented Jan 17, 2017

mfojtik commented Jan 17, 2017

pweil- commented Jan 17, 2017

openshift-bot commented Jan 23, 2017

mfojtik commented Jan 23, 2017

mfojtik commented Jan 16, 2017 •

edited

Loading

mfojtik commented Jan 17, 2017 •

edited

Loading