host-ctr: add support for authenticated image pulls from ECR Public #1296

etungsten · 2021-01-29T01:02:24Z

Issue number:
N/A

Description of changes:

Author: Erikson Tung <[email protected]>
Date:   Thu Jan 28 15:25:47 2021 -0800

    host-ctr: add support for authenticated image pulls from ECR Public
    
    Add support for authenticated image pulls from ECR Public registries.
    Unlike ECR private registries, we have to use Docker's registry API for
    resolving ECR Public image references. This means having to manage
    ECR Public registry auth credentials ourselves

Testing done:

Verified that ecrpublic::GetAuthorizationToken calls from host-ctr are successful and I get credentials back without problems.

Verified the request header includes the base64-encoded authorization token and the image gets pulled successfully:

time="2021-02-01T10:02:25-08:00" level=debug msg="do request" digest="sha256:6c72cf34ed39ff40a5ad8abd57c66e6766897f311a28f6b993f4b87545195204" mediatype=application/vnd.docker.distribution.manifest.list.v2+json 
request.header.accept="application/vnd.docker.distribution.manifest.list.v2+json, */*" 
request.header.authorization="Bearer  <BASE64-ENCODED AUTHORIZATION TOKEN>"
request.header.user-agent=containerd/1.3.7+unknown 
request.method=GET 
size=743 
url="https://public.ecr.aws/v2/nginx/nginx/manifests/sha256:6c72cf34ed39ff40a5ad8abd57c66e6766897f311a28f6b993f4b87545195204"
time="2021-02-01T10:02:25-08:00" level=debug msg="fetch response received" host=public.ecr.aws response.header.content-length=743 response.header.content-type=application/vnd.docker.distribution.manifest.list.v2+json response.header.date="Mon, 01 Feb 2021 18:02:25 GMT" response.header.docker-content-digest="sha256:6c72cf34ed39ff40a5ad8abd57c66e6766897f311a28f6b993f4b87545195204" response.header.docker-distribution-api-version=registry/2.0 response.status="200 OK" url="https://public.ecr.aws/v2/nginx/nginx/manifests/latest"
....

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

sources/host-ctr/cmd/host-ctr/main.go

etungsten · 2021-01-29T19:02:54Z

Push above and below addresses all of @samuelkarp 's comments.

etungsten · 2021-02-01T19:00:15Z

Push above fixes a small issue with double checking the host part of the url

zmrow

🤗

webern · 2021-02-01T20:43:45Z

sources/host-ctr/cmd/host-ctr/main.go

+			if !strings.HasPrefix(host, "public.ecr.aws") {
+				return "", "", errors.New("ecr-public: expected image to start with public.ecr.aws")
+			}
+			session := session.Must(session.NewSession())


What are the pre-conditions for this to work? The instance needs an instance role? Same as for the old resolver, right?

Yes, the instance needs to be launched with an instance role profile that has the ECRPublic:GetAuthorizationToken permission. For the normal resolver, the role just needs the managed policy for ECR (not public). All other registries just uses the default resolver.

I think this brings up a good point on how we should make host-ctr more cloud-agnostic. I don't think it's easy to make a determination at runtime to check if we're running in EC2 (or at least not in a hacky way). So this is something that needs to be conditionally compiled... I can follow up with a issue to investigate that further if that's alright.

Or maybe it just makes sense to fallback to using the default resolver if we fail a call to ecrpublic:GetAuthorizationToken. I think that's probably better and makes things easier. I'll go ahead and do that.

Since ECR Public specifically allows unauthenticated requests (albeit with more throttling), a lack of credentials/permission should fall-through to an anonymous pull.

etungsten · 2021-02-02T00:24:07Z

Push above and below makes it so that we fallback to using the default resolver with no creds if we can't get creds for ECR Public.

Tested things and they work as they should. If I don't have permission to get authorization creds, host-ctr correctly falls back to using the default resovler:

Feb 02 00:22:51 host-ctr[4640]: time="2021-02-02T00:22:51Z" level=warning msg="ecr-public: failed to get authorization token, falling back to default resolver (unauthenticated pull)"
Feb 02 00:22:52 host-ctr[4640]: time="2021-02-02T00:22:52Z" level=info msg="pulled image successfully" img="public.ecr.aws/nginx/nginx:latest"

samuelkarp · 2021-02-02T02:46:02Z

If I don't have permission to get authorization creds, host-ctr correctly falls back to using the default resovler

Can you also explicitly test the case where no AWS credentials are available as opposed to the case where AWS credentials are available but don't have permission?

sources/host-ctr/cmd/host-ctr/main.go

etungsten · 2021-02-02T04:38:03Z

Push above addresses @samuelkarp 's latest comments.

If I don't have permission to get authorization creds, host-ctr correctly falls back to using the default resovler

Can you also explicitly test the case where no AWS credentials are available as opposed to the case where AWS credentials are available but don't have permission?

I moved my ~/.aws/ directory to a random location and tried running host-ctr locally. It was not able to fetch credentials and fell back using the default resolver as expected:

time="2021-02-01T20:33:17-08:00" level=warning msg="ecr-public: failed to get authorization token, falling back to default resolver (unauthenticated pull)"
time="2021-02-01T20:33:17-08:00" level=debug msg=resolving host=public.ecr.aws
time="2021-02-01T20:33:17-08:00" level=debug msg="do request" host=public.ecr.aws request.header.accept="application/vnd.docker.distribution.manifest.v2+json, application/vnd.docker.distribution.manifest.list.v2+json, application/vnd.oci.image.manifest.v1+json, application/vnd.oci.image.index.v1+json, */*" request.header.user-agent=containerd/1.3.7+unknown request.method=HEAD url="https://public.ecr.aws/v2/nginx/nginx/manifests/latest"

samuelkarp

🚢

jahkeup

Looks great! 👍🏽

I would like to see the error messages slightly tweaked to avoid the potential of misleading users in some situations before merging - let me know what you think.

sources/host-ctr/cmd/host-ctr/main.go

Add support for authenticated image pulls from ECR Public registries. Unlike ECR private registries, we have to use Docker's registry API for resolving ECR Public image references. This means having to manage ECR Public registry auth credentials ourselves

etungsten · 2021-02-02T19:12:24Z

Push above addresses @jahkeup 's comments.

etungsten changed the title ~~host-ctr: add support for pulling images from ECR Public~~ host-ctr: add support for pulling images from ECR Public with creds Jan 29, 2021

etungsten force-pushed the host-ctr-ecr-public branch from 6bb0461 to e0934fa Compare January 29, 2021 01:03

etungsten changed the title ~~host-ctr: add support for pulling images from ECR Public with creds~~ host-ctr: add support for authenticated image pulls from ECR Public Jan 29, 2021

samuelkarp reviewed Jan 29, 2021

View reviewed changes

etungsten force-pushed the host-ctr-ecr-public branch from e0934fa to 7f5853a Compare January 29, 2021 19:02

etungsten force-pushed the host-ctr-ecr-public branch 2 times, most recently from 453b0de to 7c33500 Compare February 1, 2021 18:57

etungsten marked this pull request as ready for review February 1, 2021 19:00

etungsten requested a review from jahkeup February 1, 2021 19:00

zmrow approved these changes Feb 1, 2021

View reviewed changes

etungsten requested a review from samuelkarp February 1, 2021 20:50

srgothi92 approved these changes Feb 1, 2021

View reviewed changes

webern reviewed Feb 1, 2021

View reviewed changes

etungsten force-pushed the host-ctr-ecr-public branch from 7c33500 to 1c264df Compare February 2, 2021 00:15

etungsten force-pushed the host-ctr-ecr-public branch from 1c264df to d7da695 Compare February 2, 2021 00:24

etungsten requested a review from webern February 2, 2021 00:24

samuelkarp reviewed Feb 2, 2021

View reviewed changes

sources/host-ctr/cmd/host-ctr/main.go Outdated Show resolved Hide resolved

etungsten force-pushed the host-ctr-ecr-public branch from d7da695 to 4b37ccb Compare February 2, 2021 04:34

samuelkarp approved these changes Feb 2, 2021

View reviewed changes

jahkeup suggested changes Feb 2, 2021

View reviewed changes

sources/host-ctr/cmd/host-ctr/main.go Outdated Show resolved Hide resolved

sources/host-ctr/cmd/host-ctr/main.go Outdated Show resolved Hide resolved

webern approved these changes Feb 2, 2021

View reviewed changes

etungsten force-pushed the host-ctr-ecr-public branch from 4b37ccb to f994c53 Compare February 2, 2021 19:11

etungsten requested a review from jahkeup February 2, 2021 19:12

jahkeup approved these changes Feb 2, 2021

View reviewed changes

etungsten merged commit b890eca into bottlerocket-os:develop Feb 2, 2021

etungsten deleted the host-ctr-ecr-public branch February 2, 2021 19:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

host-ctr: add support for authenticated image pulls from ECR Public #1296

host-ctr: add support for authenticated image pulls from ECR Public #1296

etungsten commented Jan 29, 2021 •

edited

Loading

etungsten commented Jan 29, 2021 •

edited

Loading

etungsten commented Feb 1, 2021

zmrow left a comment

webern Feb 1, 2021

etungsten Feb 1, 2021 •

edited

Loading

etungsten Feb 1, 2021

samuelkarp Feb 1, 2021

etungsten commented Feb 2, 2021 •

edited

Loading

samuelkarp commented Feb 2, 2021

etungsten commented Feb 2, 2021

samuelkarp left a comment

jahkeup left a comment

etungsten commented Feb 2, 2021

host-ctr: add support for authenticated image pulls from ECR Public #1296

host-ctr: add support for authenticated image pulls from ECR Public #1296

Conversation

etungsten commented Jan 29, 2021 • edited Loading

etungsten commented Jan 29, 2021 • edited Loading

etungsten commented Feb 1, 2021

zmrow left a comment

Choose a reason for hiding this comment

webern Feb 1, 2021

Choose a reason for hiding this comment

etungsten Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

etungsten Feb 1, 2021

Choose a reason for hiding this comment

samuelkarp Feb 1, 2021

Choose a reason for hiding this comment

etungsten commented Feb 2, 2021 • edited Loading

samuelkarp commented Feb 2, 2021

etungsten commented Feb 2, 2021

samuelkarp left a comment

Choose a reason for hiding this comment

jahkeup left a comment

Choose a reason for hiding this comment

etungsten commented Feb 2, 2021

etungsten commented Jan 29, 2021 •

edited

Loading

etungsten commented Jan 29, 2021 •

edited

Loading

etungsten Feb 1, 2021 •

edited

Loading

etungsten commented Feb 2, 2021 •

edited

Loading