Support to download remote OCI layers (with TLS) #351

danail-branekov · 2017-09-28T10:31:17Z

This is a follow up of #348, which added the URL property to the BlobInfo descriptor.

This will allow GetBlob to fetch external blobs when the "urls" property is present. The download is performed by an HTTP client that has its Transport configured depending on the newly introduced SytemContext.OCICertPath and SystemContext.OCIInsecureSkipTLSVerify properties.

BlobInfo.URLs is an array and therefore the implementation would iterate over the URLs until HTTP OK response is returned.

When the SytemContext.OCICertPath is specified the implementation would look for CA, private key and public key certificates the same way the Docker client does. As a matter of fact they both use one and the same certificate handling implementation that is extracted in the newly introduced tlsclientconfig package.

(this is part of a CloudFoundry/Grootfs story to support OCI images based buildpacks in the future)

tscolari · 2017-09-28T11:09:00Z

So that test broke because of the use of go < 1.8 on the tests (where the http.Server.Close method didn't exist).

That's a bit misleading because the tests output says

$ go version
go version go1.9 linux/amd64

but that doesn't seem to be the case.

We tried to reproduce the error with different versions of go and could only reproduce it with 1.7 and bellow. (1.9 and 1.8 was all green)

tscolari · 2017-09-28T15:20:31Z

The PR #352 should update the version of go in the docker image

mtrmac · 2017-09-28T19:53:33Z

Thanks. I will take a detailed look later (hopefully tomorrow), this seem reasonable at a first glance.

(Note to self: Previous discussion in #147 ).

mtrmac · 2017-09-28T19:55:12Z

oci/layout/assets/certs/accepted/cacert.crt

@@ -0,0 +1,13 @@
+-----BEGIN CERTIFICATE-----


Nit: Please use fixtures for the name of the subdirectory containing test fixtures, to be consistent with the other subpackages.

👍
going to update it

mtrmac

ACK overall, just various minor things.

Should ociImageDestination.AcceptsForeignLayerURLs now return true? It’s, strictly speaking, orthogonal.

mtrmac · 2017-09-30T01:26:10Z

docker/docker_image_src.go

 }

+// TODO? Fix bug here: looping over urls continues even after successful request...
+// this is slow, and also leaks response bodies which don't get closed.


Good catch; please file a GitHub issue instead. Using the source code as a bug tracker does not really work.

mtrmac · 2017-09-30T01:26:16Z

oci/layout/assets/manifest/index.json

@@ -0,0 +1 @@
+{"schemaVersion":2,"manifests":[{"mediaType":"application/vnd.oci.image.manifest.v1+json","digest":"sha256:84afb6189c4d69f2d040c5f1dc4e0a16fed9b539ce9cfb4ac2526ae4e0576cc0","size":496,"annotations":{"org.opencontainers.image.ref.name":"v0.1.1"},"platform":{"architecture":"amd64","os":"linux"}}]}


Move this to fixtures as well, please.

mtrmac · 2017-09-30T01:31:14Z

oci/layout/oci_src.go

+	client := &http.Client{}
+	if ctx != nil {
+		tr := tlsclientconfig.NewTransport()
+		tr.TLSClientConfig = tlsconfig.ServerDefault()


ctx == nil is defined to be equal to ctx == &types.SystemContext{}, but this code treats them differently.

The ServerDefault should probably be always used.

mtrmac · 2017-09-30T01:33:25Z

oci/layout/oci_src.go

+		tr := tlsclientconfig.NewTransport()
+		tr.TLSClientConfig = tlsconfig.ServerDefault()
+		if ctx.OCICertPath != "" {
+			tlsclientconfig.SetupCertificates(ctx.OCICertPath, tr.TLSClientConfig)


This needs error handling.

mtrmac · 2017-09-30T01:37:33Z

oci/layout/oci_src.go

+func (s *ociImageSource) getExternalBlob(urls []string) (io.ReadCloser, int64, error) {
+	var (
+		resp *http.Response
+		err  error


resp does not need to be pre-declared.

err might be better initialized to errors.New("Internal error: getExternalBlob called with no URLs") or something like that, to be defensive and strictly correct.

(Both non-blocking.)

mtrmac · 2017-09-30T01:46:18Z

oci/layout/oci_src_test.go

+		ClientCAs: clientCertPool,
+	}
+
+	tlsConfig.BuildNameToCertificate()


This seems to read tlsConfig.Certificates, which is empty at this point?!

mtrmac · 2017-09-30T02:02:30Z

oci/layout/oci_src_test.go

+	http.HandleFunc("/", remoteLayerContent)
+
+	httpServer := &http.Server{
+		Addr:      ":12000",


Nothing guarantees that the port is available. Could this use :0 to automatically choose an available one?

(There is also net/http/httptest; would it simplify things?)

Yeah, I think so, I'm going to give it a try here ^^

mtrmac · 2017-09-30T02:06:08Z

oci/layout/oci_src_test.go

+	layerInfo := types.BlobInfo{
+		URLs: []string{
+			"brokenurl",
+			"https://example.com",


This is not really blocking, but it would be nice for an unit test not to depend on an external service like this.

Could this use something like https://golang.org/src/net/http/httptest/example_test.go:ExampleServer?

mtrmac · 2017-09-30T02:17:41Z

oci/layout/oci_src_test.go

+
+func TestGetBlobForRemoteLayers(t *testing.T) {
+	imageSource := createImageSource(t, &types.SystemContext{})
+	layerInfo := types.BlobInfo{


(Non-blocking: Strictly speaking, the Digest field is mandatory and Size should be -1 if unknown. OTOH I realize that computing the digests would be fairly pointless pain.)

mtrmac · 2017-09-30T02:19:09Z

oci/layout/oci_src_test.go

+func createImageSource(t *testing.T, context *types.SystemContext) types.ImageSource {
+	imageSource, err := newImageSource(context, ociReference{
+		dir: "assets/manifest",
+	})


(Non-blocking: Calling NewReference like a public user would make it obvious that there’s nothing special about constructing this reference.)

So on this one it seems that go doesn't allow the NewReference return (type.ImageReference) to be used as an ociReference.

cannot use imageRef (type types.ImageReference) as type ociReference in argument to newImageSource: need type assertion

casting seems to work

The public ociReference.NewImageSource can be called instead.

mtrmac · 2017-10-02T21:28:59Z

oci/layout/oci_src_test.go

+	hash.Write([]byte("Hello world"))
 	imageSource := createImageSource(t, &types.SystemContext{})
 	layerInfo := types.BlobInfo{
+		Digest: digest.NewDigest("sha256", hash),


(FWIW this could be digest.FromBytes([]byte(…)). This is fine as well.)

williammartin · 2017-10-03T11:30:50Z

@mtrmac PTAL

* Support for custom certs and keys when downloading OCI layers Signed-off-by: Will Martin <[email protected]>

mtrmac · 2017-10-03T13:22:28Z

👍

runcom · 2017-10-03T13:24:38Z

LGTM

Part of the fixes from containers#351 got undone Signed-off-by: umohnani8 <[email protected]>

tscolari force-pushed the tls-layer-downloads branch from 8a00b54 to fc6165a Compare September 28, 2017 10:38

tscolari force-pushed the tls-layer-downloads branch from fc6165a to 790798d Compare September 28, 2017 14:48

mtrmac reviewed Sep 28, 2017

View reviewed changes

mtrmac reviewed Sep 30, 2017

View reviewed changes

tscolari force-pushed the tls-layer-downloads branch 2 times, most recently from c4d3ace to c0f8e4b Compare October 2, 2017 10:32

mtrmac reviewed Oct 2, 2017

View reviewed changes

tscolari force-pushed the tls-layer-downloads branch 2 times, most recently from 3b01125 to 4849f5e Compare October 3, 2017 10:57

Support for remote OCI layers if they have a URL

1ed776f

* Support for custom certs and keys when downloading OCI layers Signed-off-by: Will Martin <[email protected]>

tscolari force-pushed the tls-layer-downloads branch from 4849f5e to 1ed776f Compare October 3, 2017 12:41

runcom merged commit 7434c9c into containers:master Oct 3, 2017

mtrmac mentioned this pull request Oct 11, 2017

Add authentication package to containers/image/pkg #333

Merged

umohnani8 added a commit to umohnani8/image that referenced this pull request Oct 11, 2017

Remove duplicate code for setupCertificates

4ebdec8

Part of the fixes from containers#351 got undone Signed-off-by: umohnani8 <[email protected]>

umohnani8 mentioned this pull request Oct 11, 2017

Remove duplicate code for setupCertificates #356

Merged

		@@ -0,0 +1 @@
		{"schemaVersion":2,"manifests":[{"mediaType":"application/vnd.oci.image.manifest.v1+json","digest":"sha256:84afb6189c4d69f2d040c5f1dc4e0a16fed9b539ce9cfb4ac2526ae4e0576cc0","size":496,"annotations":{"org.opencontainers.image.ref.name":"v0.1.1"},"platform":{"architecture":"amd64","os":"linux"}}]} No newline at end of file

Support to download remote OCI layers (with TLS) #351

Support to download remote OCI layers (with TLS) #351

Uh oh!

Conversation

danail-branekov commented Sep 28, 2017

Uh oh!

tscolari commented Sep 28, 2017

Uh oh!

tscolari commented Sep 28, 2017

Uh oh!

mtrmac commented Sep 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

williammartin commented Oct 3, 2017

Uh oh!

mtrmac commented Oct 3, 2017 • edited by runcom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Oct 3, 2017 • edited 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!

5 participants

mtrmac commented Oct 3, 2017 •

edited by runcom

Loading

runcom commented Oct 3, 2017 •

edited

Loading