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

euank · 2017-11-13T07:36:00Z

What this PR does / why we need it:

It's not unusual to store the aws access and secret keys together in the
same secret under different keys. This allows conveniently using that
same secret for both.

I have not yet deployed and manually tested this change, but I plan to fairly soon. Feel free to wait on that.

The AWS Route53 dns01 provider's Access Key must now be configured as a secret reference. If you have any Issuers or CluserIssuers configured with AWS IAM credentials, they **must** be updated to use the new `accessKeyIDSecretRef` field.

Note, this incidentally improves error handling for the other dns providers by refactoring secret-key-data-has-that-key checking into its own function

jetstack-bot · 2017-11-13T07:36:09Z

Hi @euank. Thanks for your PR.

I'm waiting for a jetstack 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 to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

jetstack-ci-bot · 2017-11-14T23:10:45Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: munnerz

Assign the PR to them by writing /assign @munnerz in a comment when ready.

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

Needs approval from an approver in each of these OWNERS Files:

OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

euank · 2017-11-19T05:29:13Z

I've deployed this change to my cluster and insured that the route53 provider config, using the accessKeyIDSecretRef, works for me.

munnerz

Sorry, I've had a review of this sitting in my GitHub but not submitted for almost a week now!

Thanks for this change - as we do not yet provide a stable API, I think we're best to remove the old field for this and instead require the accessKeyIDSecretRef is specified on issuers. wdyt?

munnerz · 2017-11-14T11:50:33Z

docs/api-types/issuer/spec.md

@@ -126,12 +126,17 @@ clouddns:
 ```yaml
 route53:
  accessKeyID: AKIAIOSFODNN7EXAMPLE


I think we should remove the accessKeyID field altogether. I'd rather have a smaller API surface than provide countless options that aren't that useful 😄

munnerz · 2017-11-22T14:28:01Z

docs/api-types/issuer/spec.md

@@ -126,12 +126,17 @@ clouddns:
 ```yaml
 route53:
  accessKeyID: AKIAIOSFODNN7EXAMPLE
+  accessKeyIDRef:


This should be accessKeyIDSecretRef (from types.go below)

munnerz · 2017-11-22T14:28:15Z

docs/examples/acme-issuer.yaml

@@ -36,6 +36,7 @@ spec:
      - name: route53
        route53:
          # The Route53 access key ID
+          # optionally, 'accessKeyIDRef' may be used instead to reference a secret


accessKeyIDSecretRef

munnerz · 2017-11-22T14:28:23Z

docs/api-types/issuer/spec.md

  region: eu-west-1
  secretAccessKeySecretRef:
    name: prod-route53-credentials-secret
    key: secret-access-key
 ```

+*Note*: Only one of `accessKeyID` and `accessKeyIDRef` should be specified.


This can be removed

munnerz · 2017-11-22T14:28:40Z

docs/examples/cluster-issuer.yaml

@@ -36,6 +36,7 @@ spec:
      - name: route53
        route53:
          # The Route53 access key ID
+          # optionally, 'accessKeyIDRef' may be used instead to reference a secret
          accessKeyID: AKIADKOU3GLWAQM8WQKJ


Replace with accessKeyIDSecretRef specification

munnerz · 2017-11-22T14:29:05Z

pkg/apis/certmanager/v1alpha1/types.go

-	HostedZoneID    string            `json:"hostedZoneID"`
-	Region          string            `json:"region"`
+	AccessKeyID        string            `json:"accessKeyID,omitempty"`
+	AccessKeyIDRef     SecretKeySelector `json:"accessKeyIDSecretRef,omitempty"`


I think we want to remove omitempty here

euank · 2017-12-23T02:30:30Z

Apologies for being slow to get back to this; I've addressed your comments.

I agree it makes sense to require the secret ref if we're okay with the API breakage.

euank · 2018-01-11T19:09:55Z

/hold

This change will merge conflict with #243. Let's land that one first. I'll remove the hold and do the rebase/merging once that one lands.

euank · 2018-01-11T19:11:02Z

/hold

(last time I had trailing stuff after /hold, I don't know if that breaks it or not)

jonboulle · 2018-01-29T21:54:50Z

docs/examples/acme-issuer.yaml

-          # The Route53 access key ID
-          accessKeyID: AKIADKOU3GLWAQM8WQKJ
-          # A secretKeyRef to a the route53 secret access key
+          # A secretKeyRef to a the AWS IAM credentials


to a the -> to the

(driveby, hi euank!)

Capitrium · 2018-04-18T18:16:06Z

@euank Is this still on hold? Seems like #243 was superseded by #363, it'd be great to get this merged in as well.

euank · 2018-04-18T19:28:49Z

Thanks for the reminder @Capitrium; I'll pick this back up

euank · 2018-04-19T06:02:16Z

/hold cancel

jetstack-bot · 2018-04-19T06:12:34Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: munnerz

Assign the PR to them by writing /assign @munnerz in a comment when ready.

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

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

euank · 2018-04-19T06:16:41Z

I've updated the code, but haven't updated my deployment to gain as much confidence as before.
I also made the release-note wording stronger since it's a breaking change.

I'll be away for the next two weeks; feel free to make changes or take it over if that's preferable to waiting.

euank · 2018-05-17T23:05:30Z

/retest

prow appears to be wedged

jetstack-bot · 2018-05-17T23:05:49Z

@euank: you can't request testing unless you are a jetstack member.

In response to this:

/retest

prow appears to be wedged

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

It's not unusual to store the aws access and secret keys together in the same secret under different keys. This allows conveniently using that same secret for both.

This introduces better error handling for the case where the value was not present since the helper checks the `, ok` value for the map accesses, but the existing code didn't.

It is preferable to use the 'accessKeyIDSecretRef' instead of the 'accessKeyID' field. This is primarily because in real-world deployments those two items are typically stored as two keys in a secret. This change matches host most users store aws secrets in K8s now.

euank · 2018-05-20T02:07:40Z

Rebased again, seems to be working fine on my cluster... ping @munnerz for review since you did the last round several months ago :)

jetstack-bot · 2018-06-07T00:50:01Z

@euank: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

munnerz

Sorry for the massive delay on this - it's just been buried in my list of assigned PRs/issues 😬

I've added a couple of extra thoughts - I think we should probably introduce this in a backwards compatible way, as it won't be too difficult to do.

We also now have API validations defined in pkg/apis/certmanager/validations. Could you update these accordingly for the new field?

You may also need to make some changes to the new resolveSecretKeyRef function since there have been some recently changes to how DNS providers detect the 's.resourceNamespace` field.

Again, very sorry for the delay here! I think if we can make this backwards compatible, we can get this merged straight away :)

munnerz · 2018-08-08T12:13:34Z

pkg/apis/certmanager/v1alpha1/types.go

-	SecretAccessKey SecretKeySelector `json:"secretAccessKeySecretRef"`
-	HostedZoneID    string            `json:"hostedZoneID"`
-	Region          string            `json:"region"`
+	AccessKeyIDRef     SecretKeySelector `json:"accessKeyIDSecretRef"`


Can we update this PR to not be a breaking change? i.e. if a user specifies accessKeyID, it is still used. But if a user specifies accessKeyIDSecretRef, it takes precedence?

In a later release we can then add validation to prevent both being set, before removing the old field altogether for 1.0.

Sure, I can revert it back to what I originally had.

I think at this point, this is blocked on making sure this is a step in the right direction via discussion in #687 though

retest-bot · 2018-11-06T17:32:24Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to jetstack.
/lifecycle stale

retest-bot · 2018-12-06T18:10:00Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.
Send feedback to jetstack.
/lifecycle rotten
/remove-lifecycle stale

retest-bot · 2019-01-05T18:56:58Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.
Send feedback to jetstack.
/close

jetstack-bot · 2019-01-05T18:57:07Z

@retest-bot: Closing this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.
Send feedback to jetstack.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sinjo · 2021-08-05T23:43:11Z

Hey, sorry to revive the comment thread on such an old PR, but I recently ran into a situation where I wanted to do this (read the accessKeyID from a secret rather than having to put it in the Issuer resource itself).

I figure the ship has long since sailed on replacing accessKeyID altogether, but I was wondering if you'd be open to a PR that lets you provide one or the other (i.e. either accessKeyID or accessKeyIDSecretRef, exclusively).

I ended up using ambient credentials, but that does mean using one AWS user for the whole cert-manager installation. Not a problem for now, but could be a limiting factor in future as our setup evolves.

jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 13, 2017

jetstack-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 13, 2017

jetstack-bot assigned munnerz and simonswine Nov 13, 2017

euank force-pushed the akid-ref branch from 8b165fb to e90e28c Compare November 13, 2017 07:48

munnerz reviewed Nov 22, 2017

View reviewed changes

euank force-pushed the akid-ref branch 2 times, most recently from 1f1a09e to 84107a9 Compare December 23, 2017 02:27

euank changed the title ~~dns/route53: allow accessKey to be a secret ref~~ dns/route53: change accessKey to be a secret ref Dec 23, 2017

mattmoyer mentioned this pull request Jan 11, 2018

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

Closed

jetstack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 11, 2018

munnerz removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 11, 2018

munnerz mentioned this pull request Jan 15, 2018

Added Azure DNS support for DNS01 challange #246

Merged

jonboulle reviewed Jan 29, 2018

View reviewed changes

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

jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 19, 2018

euank force-pushed the akid-ref branch from 84107a9 to 6604d14 Compare April 19, 2018 06:12

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

munnerz added area/acme Indicates a PR directly modifies the ACME Issuer code and removed area/acme Indicates a PR directly modifies the ACME Issuer code labels Apr 25, 2018

euank added 4 commits May 19, 2018 18:40

dns/route53: allow accessKey to be a secret ref

4b08dc2

It's not unusual to store the aws access and secret keys together in the same secret under different keys. This allows conveniently using that same secret for both.

issuer/dns01: use helper method for secretkeyrefs

60d2d87

This introduces better error handling for the case where the value was not present since the helper checks the `, ok` value for the map accesses, but the existing code didn't.

docs: update accessKeyID -> accessKeyIDSecretRef

137b9ac

euank force-pushed the akid-ref branch from 6604d14 to 137b9ac Compare May 20, 2018 01:40

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

euank mentioned this pull request Jun 27, 2018

DNS provider configuration: alternatives to secretKeyRef #687

Closed

munnerz reviewed Aug 8, 2018

View reviewed changes

jetstack-bot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Nov 6, 2018

jetstack-bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 6, 2018

jetstack-bot closed this Jan 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

euank commented Nov 13, 2017 •

edited

Loading

jetstack-bot commented Nov 13, 2017

jetstack-ci-bot commented Nov 14, 2017

euank commented Nov 19, 2017 •

edited

Loading

munnerz left a comment

munnerz Nov 14, 2017

munnerz Nov 22, 2017

munnerz Nov 22, 2017

munnerz Nov 22, 2017

munnerz Nov 22, 2017

munnerz Nov 22, 2017

euank commented Dec 23, 2017

euank commented Jan 11, 2018 •

edited

Loading

euank commented Jan 11, 2018

jonboulle Jan 29, 2018

Capitrium commented Apr 18, 2018

euank commented Apr 18, 2018

euank commented Apr 19, 2018

jetstack-bot commented Apr 19, 2018

euank commented Apr 19, 2018

euank commented May 17, 2018

jetstack-bot commented May 17, 2018

euank commented May 20, 2018

jetstack-bot commented Jun 7, 2018

munnerz left a comment

munnerz Aug 8, 2018

munnerz Aug 8, 2018

euank Aug 8, 2018

retest-bot commented Nov 6, 2018

retest-bot commented Dec 6, 2018

retest-bot commented Jan 5, 2019

jetstack-bot commented Jan 5, 2019

Sinjo commented Aug 5, 2021

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

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

Conversation

euank commented Nov 13, 2017 • edited Loading

jetstack-bot commented Nov 13, 2017

jetstack-ci-bot commented Nov 14, 2017

euank commented Nov 19, 2017 • edited Loading

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 Dec 23, 2017

euank commented Jan 11, 2018 • edited Loading

euank commented Jan 11, 2018

Choose a reason for hiding this comment

Capitrium commented Apr 18, 2018

euank commented Apr 18, 2018

euank commented Apr 19, 2018

jetstack-bot commented Apr 19, 2018

euank commented Apr 19, 2018

euank commented May 17, 2018

jetstack-bot commented May 17, 2018

euank commented May 20, 2018

jetstack-bot commented Jun 7, 2018

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

retest-bot commented Nov 6, 2018

retest-bot commented Dec 6, 2018

retest-bot commented Jan 5, 2019

jetstack-bot commented Jan 5, 2019

Sinjo commented Aug 5, 2021

euank commented Nov 13, 2017 •

edited

Loading

euank commented Nov 19, 2017 •

edited

Loading

euank commented Jan 11, 2018 •

edited

Loading