puzzle / cert-manager-webhook-dnsimple

A cert-manager ACME DNS01 solver webhook for DNSimple.
Apache License 2.0
19 stars 24 forks source link

Add the ability to use a DNSimple User API token #26

Closed IntegralProgrammer closed 3 months ago

IntegralProgrammer commented 5 months ago

This Pull Request adds the ability to use a User API token (as opposed to an Account API token) for communicating with the DNSimple API. Currently, if a User API token is attempted to be used, it will fail as discussed in https://github.com/neoskop/cert-manager-webhook-dnsimple/issues/12.

To test, set the token value to a valid User DNSimple API token and the accountID value to the account ID for the DNSimple account that contains the DNS zones which contain the domains for which Let's Encrypt certificates are to be obtained for.

IntegralProgrammer commented 4 months ago

@splattner I'm not sure if you're the right person to ask for this but looking at https://github.com/puzzle/cert-manager-webhook-dnsimple/pull/23#issuecomment-1705391752, given that @puzzle now maintains this repository, I'm wondering if this pull request could be reviewed and merged if there are no issues with it.

moll-re commented 4 months ago

@IntegralProgrammer Hello on behalf of Puzzle. Our maintenance of the project has just begun to take shape. You can expect some updates in the upcoming weeks and your request will be reviewed. Thanks for your patience!

moll-re commented 4 months ago

Wouldn't it make more sense to save this data as part of the secret dnsimple-token? That way the configuration for the authentication lies in single (and encrypted) source.

IntegralProgrammer commented 4 months ago

Wouldn't it make more sense to save this data as part of the secret dnsimple-token? That way the configuration for the authentication lies in single (and encrypted) source.

Okay, that is doable. The easiest way to do that would be to edit https://github.com/IntegralProgrammer/cert-manager-webhook-dnsimple/blob/feature-env-override-account-id/deploy/dnsimple/templates/deployment.yaml#L41-L45 so that if .Values.dnsimple.existingAccountIdSecret is set to true, the DNSIMPLE_ACCOUNT_ID environment variable value will come from the Kubernetes secret referenced by .Values.dnsimple.accountIdSecretName and .Values.dnsimple.accountIdSecretKey. So therefore, by setting .Values.dnsimple.existingAccountIdSecret to true, .Values.dnsimple.accountIdSecretName to dnsimple-token, and .Values.dnsimple.accountIdSecretKey to accountId we would have the option of storing the entire DNSimple API access configuration in the dnsimple-token Kubernetes secret with the API access token stored under the token key and the DNSimple account ID stored under the accountId key.

This would really only be for convenience and simplicity as the DNSimple account ID value need not be kept secret as, unlike the API access token, knowing it does not permit any access to the DNSimple API.

WDYT?

moll-re commented 4 months ago

I was thinking about modifying https://github.com/IntegralProgrammer/cert-manager-webhook-dnsimple/blob/feature-env-override-account-id/main.go#L92-L106 and load the data directly from the code. This would keep the template more clean. That makes the code more complex though so I'm honestly not sure if that is preferable.

IntegralProgrammer commented 4 months ago

I was thinking about modifying https://github.com/IntegralProgrammer/cert-manager-webhook-dnsimple/blob/feature-env-override-account-id/main.go#L92-L106 and load the data directly from the code. This would keep the template more clean. That makes the code more complex though so I'm honestly not sure if that is preferable.

I think the best option is to refactor the code so that Kubernetes Secrets are passed into containers via environment variables. To do so, we would need to modify https://github.com/puzzle/cert-manager-webhook-dnsimple/blob/04cc3cc9b6f4528a0eac5f09454db5389e2cab95/deploy/dnsimple/templates/deployment.yaml#L41-L43 and add

- name: DNSIMPLE_TOKEN
  valueFrom:
    secretKeyRef:
      name: {{ include "dnsimple-webhook.tokenSecretName" . }}
      key: token

then modify https://github.com/puzzle/cert-manager-webhook-dnsimple/blob/04cc3cc9b6f4528a0eac5f09454db5389e2cab95/main.go#L89-L103 so that apiKey is read from an environment variable (apiKey := os.Getenv("DNSIMPLE_TOKEN")).

In addition to that simplification, we would be able to remove https://github.com/puzzle/cert-manager-webhook-dnsimple/blob/04cc3cc9b6f4528a0eac5f09454db5389e2cab95/deploy/dnsimple/templates/secret.yaml#L16-L48 as the webhook container would no longer need to read this secret via the Kubernetes API.

Furthermore, we could probably also remove https://github.com/puzzle/cert-manager-webhook-dnsimple/blob/04cc3cc9b6f4528a0eac5f09454db5389e2cab95/deploy/dnsimple/templates/staging.cluster-issuer.yaml#L20-L23 and https://github.com/puzzle/cert-manager-webhook-dnsimple/blob/04cc3cc9b6f4528a0eac5f09454db5389e2cab95/deploy/dnsimple/templates/production.cluster-issuer.yaml#L20-L23 since the DNSimple API access token would now always be coming from the DNSIMPLE_TOKEN environment variable.

So while this solution would make the deployment.yaml file slightly more complicated, it would have a net result of much less complicated code.

So if you agree that this is the preferable option, I can do that refactoring.

moll-re commented 4 months ago

Yes tthat looks like a very sensible simplification. It would be great if you could implement that!

dvob commented 4 months ago

Hi @IntegralProgrammer Thank you for your contribution. I think we should keep the ability to configure the token via a secret. To my understanding the idea behind that design is that you could have multiple issuers which could use different DNSimple credentials. Also if we keep it in the current way we don't break users which rely on this.

To support the user API tokens we could add an additional setting accoundID in the issuer config. I created a PR(#29) which should implement that change. I couldn't test it so far as I don't have access to DNSimple at the moment. @IntegralProgrammer @moll-re can you test these changes? would this solve your requirement? Also my PR lacks the required changes in the helm chart and documentation. So feel free to merge the changes of #29 into this PR or we can add the missing changes to #29. whatever you prefer.

IntegralProgrammer commented 4 months ago

@dvob Thank you for your feedback and PR. Here are my thoughts on how to proceed...

I think we should keep the ability to configure the token via a secret. ... Also if we keep it in the current way we don't break users which rely on this.

This was my intent all along to keep the DNSimple token as a Kubernetes secret. In my comment on https://github.com/puzzle/cert-manager-webhook-dnsimple/pull/26#issuecomment-2015796855 I propose that we keep the DNSimple credentials as a Kubernetes secret but instead of that token being read from the webhook container using the Kubernetes API directly... https://github.com/puzzle/cert-manager-webhook-dnsimple/blob/04cc3cc9b6f4528a0eac5f09454db5389e2cab95/main.go#L89-L103 ...it would be read from a environment variable whose value would come from the DNSimple token environment variable - this environment variable from Kubernetes secret would be configured in the /deploy/dnsimple/templates/deployment.yaml file. So from the perspective of a user deploying puzzle/cert-manager-webhook-dnsimple nothing changes - the webhook container still obtains its DNSimple API credentials from the same Kubernetes secret as before therefore nothing should break.

To my understanding the idea behind that design is that you could have multiple issuers which could use different DNSimple credentials

Yes, that is correct - the DNSimple API credentials are associated with a certificate issuer and therefore it is possible to deploy multiple certificate issuers each with their own unique DNSimple API token. For example, certificate issuer (A) could be configured to use DNSimple API token (X), certificate issuer (B) could be configured to use DNSimple API token (Y), and certificate issuer (C) could be configured to use DNSimple API token (Z). From a user perspective, that would be done with multiple helm installs, one for each certificate issuer to be created each creating a new certificate issuer named as {{ helm_install_name }}-staging or {{ helm_install_name }}-production. Again, given that the DNSimple API token Kubernetes Secret is created on a per-install basis, it wouldn't matter if that secret is read by Kubernetes populating an environment variable with it or if the webhook container reads it directly via the Kubernetes API. It is however technically possible, although I don't know about how likely it would be, for a user to be using custom YAML manifests (as the Helm templates in this repository don't allow for this) that reference different Kubernetes secrets for different DNSimple API tokens on different certificate issuers but using the same webhook container. For that reason, I see merit in using the Kubernetes API to read secrets as opposed to having them passed by environment variable.

To support the user API tokens we could add an additional setting accoundID in the issuer config. I created a PR(#29) which should implement that change. I couldn't test it so far as I don't have access to DNSimple at the moment. @IntegralProgrammer @moll-re can you test these changes? would this solve your requirement? Also my PR lacks the required changes in the helm chart and documentation. So feel free to merge the changes of #29 into this PR or we can add the missing changes to #29. whatever you prefer.

I looked at PR(#29) and, although I didn't test it yet it, I anticipate that it would fail at https://github.com/dvob/cert-manager-webhook-dnsimple/blob/95329b721c2c353d4d947070cf34b8f2e0fef0f2/main.go#L141 given that I had to bypass that call in my branch https://github.com/IntegralProgrammer/cert-manager-webhook-dnsimple/blob/ca1b10acea4654bc21385e93dd0a0e240c3eb880/main.go#L122-L138 that being said, that change could easily be added to PR(#29) as well. With that change added and the required changes to the Helm template YAMLs added as well, this does look like it would solve my requirement.

So how should we proceed? @moll-re is it important that the DNSimple Account ID be stored as part of the dnsimple-token secret (https://github.com/puzzle/cert-manager-webhook-dnsimple/pull/26#issuecomment-2003594017) or should it be moved to the issuer config as @dvob suggests? I'm fine to stay with using the Kubernetes API from inside the webhook container to read secrets given the small chance that someone might be using custom YAML manifests and updating the image to use environment variables from secrets could break their custom deployment.

So here's how I propose that we proceed:

dvob commented 3 months ago

@IntegralProgrammer thank you for your response. I spoke with @moll-re and we decided it would be best to go ahead with the solution proposed in #29 where the account ID is stored in the issuer config. As you mentioned it would be transparent for users which deploy the webhook using the provided helm chart, but if someone uses the webhook with their own setup it would break them. Also we think this is the most flexible approach.

I plan to work on this branch PR(https://github.com/puzzle/cert-manager-webhook-dnsimple/pull/26) moving the changes to the getClient, Present, and CleanUp functions in PR(https://github.com/puzzle/cert-manager-webhook-dnsimple/pull/29) to here.

In that case I close #29 then you can merge/cherry-pick the changes from there and go ahead with this PR.

Thank you in advance and sorry for the back and forth.

IntegralProgrammer commented 3 months ago

@IntegralProgrammer thank you for your response. I spoke with @moll-re and we decided it would be best to go ahead with the solution proposed in #29 where the account ID is stored in the issuer config. As you mentioned it would be transparent for users which deploy the webhook using the provided helm chart, but if someone uses the webhook with their own setup it would break them. Also we think this is the most flexible approach.

I plan to work on this branch PR(#26) moving the changes to the getClient, Present, and CleanUp functions in PR(#29) to here.

In that case I close #29 then you can merge/cherry-pick the changes from there and go ahead with this PR.

Thank you in advance and sorry for the back and forth.

I have changed the implementation to follow this design pattern and can confirm that, when using a DNSimple User API token:

This fulfills the requirements within the scope of this PR for my intended use-case and therefore this PR can be merged upon satisfactory review.

moll-re commented 3 months ago

Tested on an independent build. Running on minikube and generating a dummy certificate yields the described results: