Native ECR auth #1619

squaremo · 2018-12-27T17:03:17Z

Taking inspiration (and code) from #1455, this PR builds support for fetching authorization tokens into the registry credentials code.

The design is simple: it's a wrapper for ImageCreds which keeps an authentication token, refreshing when it needs to via the AWS API. This is pretty similar to ImageCredsWithDefaults, and is composed with that in fluxd's main.go. The arguments --registry-ecr-region, --registry-ecr-include-id and --registry-ecr-exclude-id control which images it will scan:

region gives the region(s) that will be scanned; if empty, the cluster region will be assumed
include-id gives the AWS accounts (registries) to scan; if empty, any image in the allowed regions will be scanned
exclude-id excludes an AWS account (registry) from those to scan; it defaults to the account used for EKS "system" images (e.g., coredns), so images from there won't be scanned.

None of the arguments are necessary; the default will be "scan images from any registry in the cluster region, except the EKS system images".

If fluxd is not running on EC2 (or hasn't been rigged so that it can authenticate as an AWS user, to access ECR), the wrapper will fail to be initialised, a message will be logged, and image scanning will proceed as before.

Fixes #539.

(EDIT: update to reflect latest changes)

squaremo · 2018-12-31T15:38:28Z

Image pushed to quay.io/squaremo/flux:issue-539-native-ecr-auth-b76ac67 should anyone wish to try this out ...

site/daemon.md

cmd/fluxd/main.go

errordeveloper · 2019-01-02T10:28:59Z

Please don't consider my comments as blockers for the PR, those are just a few thoughts on how we can improve things in the future...

squaremo · 2019-01-02T11:30:09Z

Please don't consider my comments as blockers for the PR, those are just a few thoughts on how we can improve things in the future...

The question is, if we released this as is, would it limit our ability to improve it? Probably not -- we can always deprecate those arguments, or reinterpret them to mean "just auth this region / these account IDs". But it would be good to get it right the first time, too :-)

squaremo · 2019-01-02T11:50:02Z

Two more points:

usually we don't know all the registry URLs in play; so we wouldn't be able to authenticate against all of them at once unless we're told them explicitly. So perhaps --registry-id could mean "authenticate for these and only these ECR registries".
@errordeveloper tells me that using a registry in another region would incur data costs (of course -- everything costs, on AWS). So although, in principle, we could authenticate for whichever region is in the registry URL, it might be desirable to limit it to the region in which the cluster is running, by default.

With those in mind, I propose that I change the arguments:

--aws-region=[] -- if empty, only authenticate against the cluster region; otherwise, authenticate against all listed regions (if they come up in a registry URL)
--aws-account-id=[] -- if empty, attempt to authenticate against any account ID that is used in a registry URL; otherwise only authenticate for those given.

squaremo · 2019-01-03T14:12:12Z

Image built from the most recent commit at quay.io/squaremo/flux:issue-539-native-ecr-auth-dd7e7cd

runningman84 · 2019-01-03T16:06:50Z

@squaremo I tried the new image but I still get logs like this:
flux/flux-79477cfbbf-t5xpx[flux]: ts=2019-01-03T16:04:36.487877004Z caller=warming.go:192 component=warmer canonical_name=602401143452.dkr.ecr.eu-central-1.amazonaws.com/eks/coredns auth="{map[602401143452.dkr.ecr.eu-central-1.amazonaws.com:<registry creds for AWS@602401143452.dkr.ecr.eu-central-1.amazonaws.com, from AWS API>]}" err="requesting tags: denied: User: arn:aws:sts::1234567890:assumed-role/eks-preprod20190102102137043800000008/i-06edfb64ff3d5add4 is not authorized to perform: ecr:ListImages on resource: arn:aws:ecr:eu-central-1:602401143452:repository/eks/coredns"
Is this a known problem for eks internal repos?

squaremo · 2019-01-03T16:48:06Z

@squaremo I tried the new image

Thank you!

... but I still get logs like this:

Yep, this is the problem I was referring to in

EKS uses images from some other ECR registry for some infrastructural services, which any old IAM user doesn't have full access to

The way to shut it up is to mention your own account ID in the argument --aws-account-id, which will mean it restricts scanning to just your own registry and ignores images elsewhere in ECR.

Having said that, it looks like it's the same account ID causing log noise in eu-central-1 as it is in eu-west-1, so I wonder if I could make the default to ignore that account ID ...

runningman84 · 2019-01-03T17:49:10Z

Yes please ignore it by default this seams to be an aws account of the eks team.

errordeveloper · 2019-01-04T09:11:11Z

registry/aws.go

+	if len(config.Regions) == 0 {
+		// this forces the AWS SDK to load config, so we can get the default region
+		sess := session.Must(session.NewSessionWithOptions(session.Options{
+			SharedConfigState: session.SharedConfigEnable,


I think it's a good start, hopefully it will be sufficient long term and we won't need to add too many input variables, as it will always run in a container and won't need to support a host of configuration modes that eksctl has to support (environment variables, flags and ~/.aws/config file with profiles and MFA etc).

I've tried to leave space for mounting config files (e.g., by attempting to load config before using the ec2 metadata service). You are right that contexts etc. complicate this :-/ We'll see ..

registry/aws.go

errordeveloper · 2019-01-04T09:32:47Z

site/faq.md

+    - Google Container Registry works this way; Flux will
+      automatically attempt to use platform-provided credentials when
+      scanning images in GCR.
+    - (Amazon) Elastic Container Registry has its own authentication


I would change this to

Amazon Elastic Container Registry (ECR) has its own authentication; Flux will attempt to scan for images in ECR, if EC2 instance it is running on has access to ECR.

Which also suggest that we might want to consider to support for ECR outside of EC2, but I don't think kubelet supports that kind of mode yet (IIRC, it was mentioned in the comment in kubelet code linked from here).

As far as EC2 itself is concerned, there is a possibility that user may wish to avoid using instance roles and pass credentials to flux directly (but we should check if kubelet supports that also). The reason someone may wish to do that is security, i.e. for the lack of real IAM role enforcement for pods, someone may prefer to disallow access to ECR for any but the chose process in their cluster. One can use separate sets of nodes to isolate privileged/unprivileged pods, but that won't work in the case, as kubelets will have to fetch images on all nodes. However, it's worse noting that fine-grain IAM roles can be created to avoid the possibility of certain images being fetched on certain nodes.

Just a few thoughts (well, more of a brain dump on ECR and IAM), perhaps we could open an issue for now, if it's worse it.

I think it would be possible (though ungainly) to use credentials on the host to grant kubelet access to ECR when not running in EC2. And, I think it'll be possible (with this PR) to make credentials available to flux too, by providing a ~/.aws/credentials.

But I'd expect this to be well off the beaten path. I agree that it's important to set out the caveats early -- I'll try to rephrase.

errordeveloper · 2019-01-04T09:42:31Z

site/faq.md

+   service accounts, platform-provided credentials on GCP or AWS, and
+   a Docker config file if you mount one into the fluxd container (see
+   the [command-line usage](./daemon.md)).
+ - When using images in ECR, from AWS, the IAM account used to run the


I'd rephrase as:

- When using images in AWS ECR, the EC2 instance role of the node fluxd container runs on must have permissions to query the ECR registry or registries in question. In turns, this is a requirement for kubelet also, so if your cluster can run containers from ECR, Flux should work too.

Is it definitely the EC2 instance role that decides whether you have access? I confess I skipped lightly over the question of how the container gets any IAM permissions at all (and Just Made It Work TM). I'm happy to be more specific about the mechanism.

errordeveloper

Few more comments, hopefully helpful.

cmd/fluxd/main.go

This takes the essential workings of #1455 and creates a helper for using AWS authorization with ECR (the AWS container registry).

Instead of asking for the AWS region and set of registry IDs (really account IDs), take these from the registry URLs as they come up. This means we have to make a token request for each region; so, keep track of the expiry, per region. And so we don't keep asking when there's a failure, embargo requests for a region (for ten minutes) whenever we fail to get a token. The arguments `--aws-region` and `--aws-registry-id` are now ignored, since the values are taken from the registry URLs; but, the plumbing for those values is left in, because the arguments are useful with an altered meaning of "only these regions/accountIDs". This change will be made in a subsequent commit.

After the last commit, which takes the region and account ID from the registry URL, this commit repurposes the arguments --aws-region and (renamed) --aws-account-id to be "allow lists". They differ in the default policy: if no account IDs are given, all accounts IDs are allowed; whereas, if no region is given, the allowed regions is taken to be the "cluster region". What is the cluster region? It's the region as detected from configuration (so it can be overridden), or as given by the EC2 metadata service, as a last resort. The rationale for always restricting the region is that using images from a different region will incurs costs, so it's better to encourage people to be deliberate about it. - [ ] does it still work with minikube (or does it fail at creating wrapper)

This removes a duplicate label from the AWS-specific logger.

This introduces the flag `--registry-ecr-exclude-id`, and renames `--registry-ecr-account-id` to `--registry-ecr-include-id` for consistency. Oddly, `--registry-ecr-exclude-id` is most useful when you don't supply it, or its "include" sibling, since then fluxd will scan everything except the registry used for EKS system images (which will always fail, so far as I can determine).

squaremo · 2019-01-07T15:30:10Z

Image that works as described in the newly-edited description: quay.io/squaremo/flux:issue-539-native-ecr-auth-1b5a3f2

This build

changes the argument names
excludes the EKS system images from being scanned, by default

squaremo · 2019-01-07T15:35:02Z

@errordeveloper I've tweaked the wording in the FAQ (two places) -- is it any better, do you think?

squaremo · 2019-01-08T10:46:27Z

Many thanks @errordeveloper and @hiddeco !

roffe · 2019-01-08T11:13:55Z

awesome sauce!

runningman84 · 2019-01-08T12:26:39Z

just waiting for a new release...

…d): elaborate on how to enable autologin for ECR fluxcd/flux#1619 fluxcd/flux2#107 fluxcd#11 fluxcd#139 fluxcd#147 fluxcd#174 fluxcd#179 fluxcd#180 fluxcd/website#601 fluxcd#193 fluxcd/website#611 https://fluxcd.io/docs/guides/image-update/#aws-elastic-container-registry Signed-off-by: lloydchang <lloydchang@gmail.com>

…d): elaborate on how to enable autologin for ECR fluxcd/flux#1619 fluxcd/flux2#107 fluxcd#11 fluxcd#139 fluxcd#147 fluxcd#174 fluxcd#179 fluxcd#180 fluxcd/website#601 fluxcd#193 fluxcd/website#611 https://fluxcd.io/docs/guides/image-update/#aws-elastic-container-registry Signed-off-by: lloydchang <lloydchang@gmail.com> Signed-off-by: Kingdon Barrett <kingdon@weave.works>

squaremo added enhancement help wanted labels Dec 27, 2018

squaremo changed the title ~~[WIP] Native ECR auth~~ Native ECR auth Dec 31, 2018

errordeveloper reviewed Jan 2, 2019

View reviewed changes

site/daemon.md Outdated Show resolved Hide resolved

errordeveloper reviewed Jan 2, 2019

View reviewed changes

cmd/fluxd/main.go Outdated Show resolved Hide resolved

squaremo force-pushed the issue/539-native-ecr-auth branch from 39cd52f to dd7e7cd Compare January 3, 2019 13:46

errordeveloper reviewed Jan 4, 2019

View reviewed changes

registry/aws.go Show resolved Hide resolved

errordeveloper reviewed Jan 4, 2019

View reviewed changes

cmd/fluxd/main.go Show resolved Hide resolved

squaremo added 8 commits January 7, 2019 11:46

Add ECR auth helpers

e27d88b

This takes the essential workings of #1455 and creates a helper for using AWS authorization with ECR (the AWS container registry).

Wire AWS auth into fluxd main

2a0ae94

Update documentation re ECR authentication

7d7ecc4

Tidy logging during fluxd startup

0aef983

This removes a duplicate label from the AWS-specific logger.

Update FAQ etc. for ECR scanning

c3f2fde

Be more careful re ECR auth in FAQ

4c156aa

squaremo force-pushed the issue/539-native-ecr-auth branch from dd7e7cd to 4c156aa Compare January 7, 2019 13:30

Change names of ECR-specific flags

8ea4044

hiddeco approved these changes Jan 8, 2019

View reviewed changes

squaremo merged commit 3ca823d into master Jan 8, 2019

squaremo deleted the issue/539-native-ecr-auth branch January 8, 2019 10:46

squaremo mentioned this pull request Jan 14, 2019

Exclude domains from image metadata scanning #1654

Closed

hiddeco mentioned this pull request Feb 18, 2019

Document using sidecar/job pod to get ECR credentials into a secret #1324

Closed

stefanprodan mentioned this pull request Feb 25, 2019

Helm Chart allow mounting of docker credentials file #1762

Merged

lloydchang mentioned this pull request Oct 31, 2021

docs(image-reflector-controller/docs/spec/v1beta1/imagerepositories.md): elaborate on how to enable autologin for ECR kingdonb/image-reflector-controller#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Native ECR auth #1619

Native ECR auth #1619

squaremo commented Dec 27, 2018 •

edited

Loading

squaremo commented Dec 31, 2018

errordeveloper commented Jan 2, 2019

squaremo commented Jan 2, 2019

squaremo commented Jan 2, 2019

squaremo commented Jan 3, 2019

runningman84 commented Jan 3, 2019

squaremo commented Jan 3, 2019

runningman84 commented Jan 3, 2019

errordeveloper Jan 4, 2019 •

edited

Loading

squaremo Jan 7, 2019

errordeveloper Jan 4, 2019 •

edited

Loading

squaremo Jan 7, 2019 •

edited

Loading

errordeveloper Jan 4, 2019

squaremo Jan 7, 2019

errordeveloper left a comment

squaremo commented Jan 7, 2019

squaremo commented Jan 7, 2019

squaremo commented Jan 8, 2019

roffe commented Jan 8, 2019

runningman84 commented Jan 8, 2019

Native ECR auth #1619

Native ECR auth #1619

Conversation

squaremo commented Dec 27, 2018 • edited Loading

squaremo commented Dec 31, 2018

errordeveloper commented Jan 2, 2019

squaremo commented Jan 2, 2019

squaremo commented Jan 2, 2019

squaremo commented Jan 3, 2019

runningman84 commented Jan 3, 2019

squaremo commented Jan 3, 2019

runningman84 commented Jan 3, 2019

errordeveloper Jan 4, 2019 • edited Loading

Choose a reason for hiding this comment

squaremo Jan 7, 2019

Choose a reason for hiding this comment

errordeveloper Jan 4, 2019 • edited Loading

Choose a reason for hiding this comment

squaremo Jan 7, 2019 • edited Loading

Choose a reason for hiding this comment

errordeveloper Jan 4, 2019

Choose a reason for hiding this comment

squaremo Jan 7, 2019

Choose a reason for hiding this comment

errordeveloper left a comment

Choose a reason for hiding this comment

squaremo commented Jan 7, 2019

squaremo commented Jan 7, 2019

squaremo commented Jan 8, 2019

roffe commented Jan 8, 2019

runningman84 commented Jan 8, 2019

squaremo commented Dec 27, 2018 •

edited

Loading

errordeveloper Jan 4, 2019 •

edited

Loading

errordeveloper Jan 4, 2019 •

edited

Loading

squaremo Jan 7, 2019 •

edited

Loading