Implement a lookaside storage for signatures of images in Docker registries #52

mtrmac · 2016-08-17T21:29:19Z

Allows configuring sigstore (for RW) and sigstore-write (write-only) for images/repositories/namespaces/registries in atomic.conf.

If configured, uses them to store signatures in the docker: transport.

This works fine for file:// sigstore URLs, for read-only http:// and https:// sigstore URLs, and is at best a skeleton for writing to http:// / https:// (no authentication, to start). So, with this, pushing signatures to remote Docker registries is still not reasonably possible, but at least you can push to a file:// pointing to a webroot of a sigstore served to the public over HTTP. (Or you can configure sigstore to point to HTTP to pull from the same sources as your users, and sigstore-write to the webroot.)

Does not deal with any kind of discovery, the lookaside needs to be manually configured in advance.

This pulls in #49 for types.SystemContext and significantly extends it, adding a generic RootForImplicitAbsolutePaths and adding a SystemContext parameter to types.NewImage{,Source,Destination} as tentatively discussed in #41 . (Also note that we now implicitly use the ATOMIC_CONF environment variable, for compatibility with projectatomic/atomic). I will rebase as necessary.

Parsing atomic.conf here in generic is admittedly fairly ugly, but perhaps still better than defining an interface for this and requiring skopeo to implement that…

More details are in commit messages.

mtrmac · 2016-08-22T14:28:58Z

(Tests failing due to API changes, corresponding update is containers/skopeo#170 .)

mtrmac · 2016-08-25T18:20:38Z

Rebased, has no dependencies now.

mtrmac · 2016-08-31T18:41:15Z

Note that this calls os.LookupEnv("ATOMIC_CONF") if the caller does not explicitly set an atomic.conf path through SystemContext.AtomicConfPath or SystemContext.RootForImplicitAbsolutePaths. Per the conversation in containers/skopeo#178 we should probably allow overriding that.

(Should the default be to use the environment or to ignore it? I am leaning towards “use” but not sure.)

mtrmac · 2016-09-10T01:20:16Z

OK, updated with the configuration per projectatomic/atomic#594, now includes documentation and high unit test converage for lookaside.go.

@runcom PTAL sometime (not urgent enough to be done over the weekend).

runcom · 2016-09-10T14:42:29Z

If configured, uses them to store signatures in the docker: transport.

should this also be with docker-daemon:? just curious and a bit confused maybe

runcom · 2016-09-10T14:45:22Z

docker/docker_client.go

-	var registry string
-	if refHostname == dockerHostname {
+// forWriting specifies whether the client will be used for "write" access (in particular passed to lookaside.go:toplevelFromSection)
+func newDockerClient(ctx *types.SystemContext, ref dockerReference, forWriting bool) (*dockerClient, error) {


can we name this just write bool and document that it'll be used for writing? This is not in the Go idioms where variables,arguments and such are just small names. Maybe mode or mask and have other values to maybe be future proof?

can we name this just write bool and document that it'll be used for writing?

I can’t see that having a short name + “for writing” documentation is any shorter or easier to understand than a self-documenting forWriting, but, sure, changed.

Maybe mode or mask and have other values to maybe be future proof?

I don’t want to get all Design Patterns on this; the underlying configuration file syntax is not future proof that way, and that is the ABI commitment; even if we turned mode into an interface we wouldn’t know what to do with it in registryNamespace.signatureTopLevel. The parameter is an internal implementation detail which we can generalize at any time if needed.

I don’t want to get all Design Patterns on this; the underlying configuration file syntax is not future proof that way, and that is the ABI commitment; even if we turned mode into an interface we wouldn’t know what to do with it in registryNamespace.signatureTopLevel. The parameter is an internal implementation detail which we can generalize at any time if needed.

yeah I pushed too much there :)

runcom · 2016-09-10T14:52:18Z

docker/docker_image_dest.go

+			return false, err
+		}
+		defer res.Body.Close()
+		if res.StatusCode == http.StatusNotFound {


question, is there an API to follow when implementing a remove sigstore? sorry if I missed it but I was just wondering this

forget this one...

Actually, thanks for pointing this out. Without authentication this is basically pointless; we kind of are defining an API here, but we don’t really want to—e.g. if we decided to later implement WebDAV or something, having the attempt an an implementation above could lead to somebody implementing such a server.

Because we are not truly serious about the above code as an API, let’s just drop it for now.

runcom · 2016-09-10T14:56:23Z

LGTM

This is awesome @mtrmac , I'd like to know more about the API a remote sigstore should implement though.

runcom · 2016-09-11T19:16:39Z

Forget about the lookaside API doc, I just checked the emails :)

mtrmac · 2016-09-12T14:11:21Z

If configured, uses them to store signatures in the docker: transport.

should this also be with docker-daemon:? just curious and a bit confused maybe

No; when add a lookaside for docker-daemon, it will be a hard-coded directory somewhere in /var, with no need to configure it. (Though the structure, and parts of the implementation, may be very similar or shared.)

mtrmac · 2016-09-12T14:23:53Z

docker/lookaside.go

+		return nil, err
+	}
+	for _, configName := range configNames {
+		configPath := filepath.Join(dirPath, configName)


Forgot a .yaml suffix check here, will add it.

mtrmac · 2016-09-12T14:50:41Z

Updated:

s/forWriting/write/g + comment changes
Dropped the pitiful attempts at writing to http/https; modified the docs to use file:/// examples
Files without .yaml suffix are ignored, + a new test fixture
Used require instead of assert in a few tests to avoid panics

@runcom Do you want to take another look?

runcom · 2016-09-12T14:53:52Z

s/forWriting/write/g + comment changes
Dropped the pitiful attempts at writing to http/https; modified the docs to use file:/// examples
Files without .yaml suffix are ignored, + a new test fixture
Used require instead of assert in a few tests to avoid panics

LGTM

Allows configuring "sigstore" (for RW) and "sigstore-write" (write-only) for images/repositories/namespaces/registries in /etc/containers/repositories.d/*.yaml . If configured, uses them to store signatures in the docker: transport. Also includes documentation in docs/registries.d.md. Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac · 2016-09-12T19:08:03Z

👍

There have been redundant calls here since two ref.ref.Hostname() calls were added in aaedc64 (Implement lookaside storage for signatures for Docker registries, 2016-08-11, containers#52). At that point the two calls were separated by a dockerHostname check which could have been shifted by two lines to avoid the doubled function calls. But in f28367e (Add docker/config package to containers/image/pkg, 2017-08-29, containers#333) the dockerHostname check moved to a separate function entirely (newDockerClientWithDetails) while the Domain() calls remained together in newDockerClientFromRef. So now there is no longer any reason for the second call, and this commit drops it. Signed-off-by: W. Trevor King <[email protected]>

mtrmac mentioned this pull request Aug 17, 2016

Implement a lookaside storage for signatures of images in Docker registries containers/skopeo#170

Merged

mtrmac force-pushed the docker-lookaside branch 2 times, most recently from 37723f2 to 273d3c8 Compare August 25, 2016 17:33

mtrmac force-pushed the docker-lookaside branch 5 times, most recently from bc25077 to 3032b0c Compare August 30, 2016 18:59

This was referenced Aug 31, 2016

HTTP Proxy support containers/skopeo#178

Closed

Add support for containers/storage #63

Merged

API changes #64

Merged

mtrmac force-pushed the docker-lookaside branch 7 times, most recently from f10b136 to cf2789d Compare September 6, 2016 18:18

mtrmac force-pushed the docker-lookaside branch 3 times, most recently from ec19c57 to 0d027ce Compare September 10, 2016 00:39

mtrmac mentioned this pull request Sep 10, 2016

(atomic sign) and (skopeo copy) are inconsistent WRT Docker sigstore configuration projectatomic/atomic#594

Closed

runcom reviewed Sep 10, 2016
View reviewed changes

mtrmac reviewed Sep 12, 2016
View reviewed changes

mtrmac force-pushed the docker-lookaside branch from 0d027ce to c286c99 Compare September 12, 2016 14:50

mtrmac force-pushed the docker-lookaside branch from c286c99 to 89a28a6 Compare September 12, 2016 15:00

mtrmac force-pushed the docker-lookaside branch from 89a28a6 to aaedc64 Compare September 12, 2016 15:51

mtrmac merged commit 6310e69 into containers:master Sep 12, 2016

mtrmac deleted the docker-lookaside branch September 12, 2016 19:08

wking mentioned this pull request Mar 9, 2019

docker/docker_client: Drop redundant Domain(ref.ref) call #598

Merged

Implement a lookaside storage for signatures of images in Docker registries #52

Implement a lookaside storage for signatures of images in Docker registries #52

Uh oh!

Conversation

mtrmac commented Aug 17, 2016

Uh oh!

mtrmac commented Aug 22, 2016

Uh oh!

mtrmac commented Aug 25, 2016

Uh oh!

mtrmac commented Aug 31, 2016

Uh oh!

mtrmac commented Sep 10, 2016

Uh oh!

runcom commented Sep 10, 2016

Uh oh!

runcom Sep 10, 2016

Choose a reason for hiding this comment

Uh oh!

mtrmac Sep 12, 2016

Choose a reason for hiding this comment

Uh oh!

runcom Sep 12, 2016

Choose a reason for hiding this comment

Uh oh!

runcom Sep 10, 2016

Choose a reason for hiding this comment

Uh oh!

runcom Sep 12, 2016

Choose a reason for hiding this comment

Uh oh!

mtrmac Sep 12, 2016

Choose a reason for hiding this comment

Uh oh!

runcom commented Sep 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Sep 11, 2016

Uh oh!

mtrmac commented Sep 12, 2016

Uh oh!

mtrmac Sep 12, 2016

Choose a reason for hiding this comment

Uh oh!

mtrmac commented Sep 12, 2016

Uh oh!

runcom commented Sep 12, 2016

Uh oh!

mtrmac commented Sep 12, 2016 • edited by runcom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

runcom commented Sep 10, 2016 •

edited

Loading

mtrmac commented Sep 12, 2016 •

edited by runcom

Loading