Allow non-static AWS credentials for Route 53, gated by "ambient credentials" flags #363

euank · 2018-02-28T08:05:08Z

This includes all the changes in #243, but also implements the flags discussed in #308 and fixes the failing test.

The flags are implemented such that ambient credentials are only used if the access id and secret key are blank, which I personally think is the correct behavior.

This should not be merged yet because I the following still needs to be done:

Manually test each behaviour (each flag on/off and the expected failure/success of empty issuer, but valid ambient credentials)
- default flags, regular issuer, regular cluster issuer, ambient cluster issuer work, ambient issuer errors
- --issuer-ambient-credentials results in all four issuers working
- --cluster-issuer-ambient-credentials=false results in the ambient cluster issuer no longer working
Evaluate adding e2e tests, hopefully add (edit: punted, this is difficult with the current e2e setup)
Add documentation about flag / clusterissuers

My current status on the above is that I'm having trouble getting the e2e tests to run locally; if I have trouble for longer I may file a separate issue about that.

Release note:

Issuers using the AWS Route53 solver may attempt to find credentials using the environment, EC2 IAM Role, and other sources available to the cert-manager controller. This behavior is on by default for cluster issuers and off by default for issuers. This behavior may be enabled or disabled for all issuers or cluster issuers using the `--issuer-ambient-credentials` and `--cluster-issuer-ambient-credentials` flags on the cert-manager controller.

munnerz · 2018-02-28T08:13:06Z

/ok-to-test

…

On Wed, 28 Feb 2018 at 08:05, jetstack-bot ***@***.***> wrote: Hi @euank <https://github.com/euank>. Thanks for your PR. I'm waiting for a jetstack <https://github.com/orgs/jetstack/people> member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org <https://github.com/orgs/jetstack/people> to skip this step. I understand the commands that are listed here <https://go.k8s.io/bot-commands>. Instructions for interacting with me using PR comments are available here <https://git.k8s.io/community/contributors/devel/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#363 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMbP06ckWG5GV0GO5IaWPlYY2nMgtRLks5tZQjBgaJpZM4SWOsX> .

munnerz

Thanks very much for this PR, and thanks to @mattmoyer for the original work on the AWS provider too.

I share your concerns around ensuring we test this properly - this is in effect a security option, and so we need to ensure it does properly secure what it says. I think explicit 'ambient' variants of the dns provider constructor functions (like you've done here) and then unit tests to ensure they are not called with ambient credentials are disabled works well (at least, for now).

I think we also need to update our documentation to explain how ClusterIssuer and Issuer now differ - up until this point, they have behaved identically out of the box. This could definitely be a surprise to users and without proper documentation, might lead them to think "cert-manager just works some of the time /shrug".

munnerz · 2018-03-01T10:14:34Z

pkg/issuer/acme/dns/dns.go

+				secretAccessKey,
+				providerConfig.Route53.HostedZoneID,
+				providerConfig.Route53.Region,
+			)


So I'm thinking in order to test this, we could probably just add a unit test that ensures the correct function is called (i.e. ambient or not). We will struggle otherwise to run these tests, as we currently don't mock the entire provider environment (and thus, we have no way to properly e2e test DNS providers at the moment either).

euank · 2018-03-06T01:36:42Z

I pushed some additional fixups. I'm hitting some hangups testing this still, so it's still not ready for merge, but a bit closer.

euank · 2018-03-14T08:13:17Z

I added some infrastructure to the dns package to make it easier to unit test this sort of change in #391; unfortunately this and that PR will merge conflict.
Since that PR is significantly simpler, hopefully that one can get in, and then I can rebase this and add unit testing for this on top of the work in that one.

euank · 2018-03-16T02:45:44Z

Rebased and added a unit test, I'm still not happy with the test coverage of this and still don't consider it quite ready for merge.

I do think review of what's here is reasonable though

munnerz

Looking good to me! Let me know when you're happy with this or need me to take another look 😄

munnerz · 2018-03-19T13:25:36Z

pkg/issuer/acme/dns/route53/route53.go

+		// https://docs.aws.amazon.com/sdk-for-go/v1/developer-guide/configuring-sdk.html#specifying-credentials
+	} else {
+		glog.V(5).Infof("not using ambient credentials")
+		config.WithCredentials(credentials.NewStaticCredentials(accessKeyID, secretAccessKey, ""))


If these credentials are invalid, will the default chain still be used? (i.e. does WithCredentials append to the chain, or replace?)

It replaces the default credential chain. Even if they're an empty string, it will error out that they're invalid without trying anything else.

munnerz · 2018-03-19T13:30:41Z

pkg/issuer/acme/dns/route53/route53_integration_test.go

-		}
-	}
-	return m, nil
-}


Not sure whether this test file was meant to be removed? Can it be reworked for the new constructors?

This was intentional.

My understanding was this isn't being run in CI, and even if it were, these tests are really just testing the aws sdk, not testing any of the code here as far as I could tell

munnerz · 2018-03-19T13:31:27Z

pkg/issuer/acme/dns/dns_test.go

+					args: []interface{}{"", "", "", "us-west-2", true},
+				},
+			},
+		},


Can we get a corresponding fixture where ambient is set to false?

This change maintains backwards compatibility, but makes the `accessKeyID` and `secretAccessKeySecretRef` fields of the `route53` DNS provider optional. If not provided, AWS credentials will be loaded from `AWS_*` environment variables or the EC2 metadata service. This should also work for things that impersonate the EC2 metadata service, such as [kube2iam](https://github.com/jtblin/kube2iam) and [kail](https://github.com/uswitch/kiam). Signed-off-by: Matt Moyer <moyer@heptio.com>

Signed-off-by: Matt Moyer <moyer@heptio.com>

The zone id is never read from the environment; this test tests functionality which doesn't exist in the actual software, so there's no point in having it.

This implements ambient credential support for AWS, gated behind flags for issuers and cluster issuers. This adds the pair of flags discussed in cert-manager#308. It provides an implementation for those flag's effects for the route53 solver.

I'm convinced this test was never run and also did not provide any significant value in this project.

euank · 2018-03-24T00:29:27Z

@munnerz I'm happy with this now. You should definitely do a pass on the docs I added since I'm sure there's a better way to explain some of that, but I've manually tested the code and think this is an okay to merge from my end.

I do wish there was a better way to deal with e2e testing this, but that seems like quite a rabbit hole to go down right now. I may do so later if my time allows.

munnerz

Just one minor pending question around how the REGION environment variable is handled, other this looks good to me 😄 thanks for all the work!

munnerz · 2018-03-24T09:23:07Z

docs/examples/acme-issuer.yaml

@@ -35,6 +35,9 @@ spec:
            key: api-key.txt
      - name: route53
        route53:
+          # Credentials may optionally be omitted to use ambient credentials if
+          # --issuer-ambient-credentials is enabled and
+          # ambient credentials are available to cert-manager.


Just wondering - would this be specified with something like:

- name: route53 route53: {}

?

Ah - I should read the docs 😄

- name: route53 route53: region: us-east1-a

munnerz · 2018-03-24T09:25:15Z

docs/user-guides/ambient-credentials.md

+
+They will only be used if no credentials are supplied, even if the supplied credentials are incorrect.
+
+By default, they may be used by cluster-issuers, but not regular issuers. The


s/cluster-issuers/ClusterIssuers

munnerz · 2018-03-24T09:25:39Z

docs/user-guides/ambient-credentials.md

+`--cluster-issuer-ambient-credentials=false` flags on the cert-manager may be
+used to override this behavior.
+
+Note that ambient credentials are disabled for regular 'Issuer's by default to


'Issuer's? I think this should just be Issuers?

munnerz · 2018-03-24T09:26:52Z

docs/user-guides/cluster-issuers.md

+
+## Ambient Credentials
+
+By default, a Cluster Issuer will be able to use 'ambient credentials' of the 'cert-manager' deployment for supported challenges. Currently, only the 'route53' challenge makes use of ambient credentials. To learn more about this behavior, see the [ambient credentials][ambient-creds] document.


Currently, only the 'route53' ACME DNS challenge validation makes...

munnerz · 2018-03-24T09:36:36Z

pkg/issuer/acme/dns/route53/route53_test.go

-	const testZoneID = "testzoneid"
-
+func TestAmbientRegionFromEnv(t *testing.T) {
+	os.Setenv("AWS_REGION", "us-east-1")


So it seems like if the 'region' field is not set in the Issuer resource, we use the environment to determine which to use (this var).

I guess this happens as part of credential loading in the default chain? & additionally, if AWS_REGION is set, but ambient credentials are disabled, will the region still be used? It's not such a big security concern, but it could cause strange behaviour for users and mask the fact they've not set the region explicitly in the YAML.

Region handling is distinct from credential chains. The two sources that will be consulted for region are that environment variable, and the 'shared config' (the ini file at ~/.aws/config). Notably, ec2 metadata isn't consulted.

The current implementation of the ambient option does not impact those settings. I didn't really think about that, so I left it as it was before.

The region thing is a bit silly here since route53 is a global service, so the region only really matters if you want to reduce api latency by hitting a nearer server, or if you're using a different aws partition (like aws china or govcloud).

I'll go ahead and make it respect the ambient config for now, but I think the above may be an argument for defaulting regardless it in the future.

Added a commit to do the above, and updated those unit tests.

This notably results in the region being a required field if the 'ambient' option is not set for a given issuer.

euank · 2018-03-25T00:07:25Z

/retest

unrelated flake

munnerz · 2018-03-26T11:14:37Z

/lgtm
/approve

🎉 thanks for this @mattmoyer and @euank! Lots of work done here, but I think we've laid out a good framework and best practice for how we handle this in future, and can start rolling out similar changes for other providers too 😄.

Really appreciate it, it's not easy finding time to go back and forth on PRs like this!

jetstack-bot · 2018-03-26T11:14:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [munnerz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jetstack-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Feb 28, 2018

jetstack-bot requested a review from munnerz February 28, 2018 08:05

jetstack-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 28, 2018

jetstack-bot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 28, 2018

munnerz reviewed Mar 1, 2018

View reviewed changes

euank force-pushed the nonstatic-aws-creds branch from 6c978bf to ba47a49 Compare March 6, 2018 01:32

munnerz added this to the v0.3 milestone Mar 8, 2018

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 15, 2018

euank force-pushed the nonstatic-aws-creds branch from ba47a49 to 22d6088 Compare March 16, 2018 02:40

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 16, 2018

munnerz reviewed Mar 19, 2018

View reviewed changes

munnerz assigned euank Mar 23, 2018

mattmoyer and others added 7 commits March 23, 2018 14:30

Drop unused NewDNSProvider() method.

14c109a

Signed-off-by: Matt Moyer <moyer@heptio.com>

issuer/route53: remove zone-id env test

0e6ca80

The zone id is never read from the environment; this test tests functionality which doesn't exist in the actual software, so there's no point in having it.

issuer/route53: improve logging hosted zone errs

0d39da5

issuer/route53: remove unused integ test

971ef4f

I'm convinced this test was never run and also did not provide any significant value in this project.

issuer/acme/dns: add ambient=false unit test

dd48f4a

euank force-pushed the nonstatic-aws-creds branch 3 times, most recently from 48099de to e6884f1 Compare March 24, 2018 00:25

munnerz reviewed Mar 24, 2018

View reviewed changes

docs: add ambient credentials documentation

1c57feb

euank added 3 commits March 24, 2018 14:13

docs: xref ambient issuers in cluster-issuer docs

f88707d

docs/examples: mention ambient credentials

f848ecf

docs/api-types: mention ambient credentials

8857ea5

euank force-pushed the nonstatic-aws-creds branch from 9b98598 to 2d93161 Compare March 24, 2018 21:13

issuer/route53: respect 'ambient' flag for region

faac070

This notably results in the region being a required field if the 'ambient' option is not set for a given issuer.

euank force-pushed the nonstatic-aws-creds branch from 2d93161 to faac070 Compare March 24, 2018 21:16

jetstack-bot assigned munnerz Mar 26, 2018

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 26, 2018

jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 26, 2018

jetstack-bot merged commit 95883c4 into cert-manager:master Mar 26, 2018

This was referenced Apr 4, 2018

Allow non-static AWS credentials for Route 53. #243

Closed

Add flags to enable/disable using 'ambient' credentials for ACME authz #308

Closed

euank deleted the nonstatic-aws-creds branch April 8, 2018 22:44

euank mentioned this pull request Apr 8, 2018

Support obtaining Route53 credentials via EC2 IAM role #195

Closed

darron mentioned this pull request Apr 17, 2018

Implement control loop to implement kubernetes.io/tls-acme: "true" behaviour on Ingress resources #19

Closed

Capitrium mentioned this pull request Apr 18, 2018

dns/route53: change accessKey to be a secret ref #197

Closed

wallrj mentioned this pull request Jan 21, 2022

Documentation about ambient-credentials #4738

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow non-static AWS credentials for Route 53, gated by "ambient credentials" flags #363

Allow non-static AWS credentials for Route 53, gated by "ambient credentials" flags #363

euank commented Feb 28, 2018 •

edited

munnerz commented Feb 28, 2018 via email

munnerz left a comment

munnerz Mar 1, 2018

euank commented Mar 6, 2018

euank commented Mar 14, 2018

euank commented Mar 16, 2018 •

edited

munnerz left a comment

munnerz Mar 19, 2018

euank Mar 23, 2018

munnerz Mar 19, 2018

euank Mar 23, 2018

munnerz Mar 19, 2018

euank Mar 24, 2018

euank commented Mar 24, 2018 •

edited

munnerz left a comment

munnerz Mar 24, 2018

munnerz Mar 24, 2018

munnerz Mar 24, 2018

munnerz Mar 24, 2018

munnerz Mar 24, 2018

munnerz Mar 24, 2018

euank Mar 24, 2018

euank Mar 24, 2018

euank commented Mar 25, 2018

munnerz commented Mar 26, 2018

jetstack-bot commented Mar 26, 2018


		They will only be used if no credentials are supplied, even if the supplied credentials are incorrect.

		By default, they may be used by cluster-issuers, but not regular issuers. The


		## Ambient Credentials

		By default, a Cluster Issuer will be able to use 'ambient credentials' of the 'cert-manager' deployment for supported challenges. Currently, only the 'route53' challenge makes use of ambient credentials. To learn more about this behavior, see the [ambient credentials][ambient-creds] document.

Allow non-static AWS credentials for Route 53, gated by "ambient credentials" flags #363

Allow non-static AWS credentials for Route 53, gated by "ambient credentials" flags #363

Conversation

euank commented Feb 28, 2018 • edited

munnerz commented Feb 28, 2018 via email

munnerz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

euank commented Mar 6, 2018

euank commented Mar 14, 2018

euank commented Mar 16, 2018 • edited

munnerz 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

euank commented Mar 24, 2018 • edited

munnerz 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

euank commented Mar 25, 2018

munnerz commented Mar 26, 2018

jetstack-bot commented Mar 26, 2018

euank commented Feb 28, 2018 •

edited

euank commented Mar 16, 2018 •

edited

euank commented Mar 24, 2018 •

edited