Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(acme): make account_key configurable #9746

Merged
merged 4 commits into from
Feb 28, 2023

Conversation

szesch
Copy link
Contributor

@szesch szesch commented Nov 10, 2022

Summary

There is currently no way to configure an existing private key to be used for the acme plugin. If the account does not have a key in storage, then a new one is created. This is a problem when external account binding credentials have already been associated with a key elsewhere (e.g. cert-manager or cert-bot). Some issuers like ZeroSSL allow for EAB credentials to be reused and multiple accounts can be created with different private keys. Others like GCP Public CA only allow for EAB credentials to be associated with one key so when the plugin tries to create a new account with a new key it fails.

The current workaround for this has been to manually insert the key in storage so a new key is not created.

This PR exposes account_key and will use that if the account was not found in storage and the value of account_key is not nil. Otherwise, it will generate a key on the fly as it does currently.

Full changelog

  • Add account_key in acme plugin schema
  • Add tests for when account_key is present and when it is absent in the configuration

cc @fffonion

@CLAassistant
Copy link

CLAassistant commented Nov 10, 2022

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@fffonion
Copy link
Contributor

Thanks for the PR @szesch ! I agree it's a correct way to do. I leave a comment, some tests might need to be adjusted
accordingly.

@szesch szesch requested a review from fffonion November 14, 2022 21:41
@fffonion
Copy link
Contributor

@szesch I forgot to mention last time that we need to add this field to https://github.com/Kong/kong/blob/master/kong/clustering/compat/removed_fields.lua for hybrid mode compatibility.
And could you re-commit using company email to pass CLA?

@szesch szesch force-pushed the feat/acme-plugin-account-key branch from ece89bb to 8083e5a Compare November 15, 2022 14:55
@szesch
Copy link
Contributor Author

szesch commented Nov 15, 2022

@fffonion Done! Thanks.

@fffonion
Copy link
Contributor

cc @jschmid1 this PR adds the account private key field to acme plugin, with key mgmt feature coming in 3.1, do
you think it should and is feasible for it to store the account key in key mgmt?

@jschmid1
Copy link
Contributor

yeah, I think it would. the acme-plugin would need to add a config field that allows to specify a key (or a set of keys)

@fffonion
Copy link
Contributor

@szesch I assume this wasn't in the urgency of getting in 3.1 (correct me if i'm wrong). As we now introduce
keys entity in kong, the account_key field may need to reference that entity. it won't be huge refactor
of this PR, most likely just conf.account_key becomes kong.db.keys:get_private_key(conf.account_key_id).
But since feature freeze is tomorrow, it's not likely we will have enought time to get this in.

@szesch
Copy link
Contributor Author

szesch commented Nov 16, 2022

@fffonion it's not urgent to get in to 3.1. Is there another PR I can reference or some docs to better understand what I have to refactor?

@dndx
Copy link
Member

dndx commented Nov 17, 2022

This PR needs rebasing.

@gszr gszr force-pushed the feat/acme-plugin-account-key branch from 8083e5a to 357dca4 Compare November 18, 2022 15:08
Copy link
Member

@gszr gszr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the vein of #9367, IMO we should make this accept a base64-encoded key also.

@szesch
Copy link
Contributor Author

szesch commented Nov 23, 2022

@gszr @fffonion Do I need to add base64 support if the plan is to wait for the key entity work to be done and then update this PR to use that (as mentioned #9746 (comment))? In other words, is the vision to support the key entity AND base64 encoded values or just the key entity?

@jschmid1
Copy link
Contributor

@szesch the key/key-sets feature is in Kong now. There is an ee-plugin that already uses this entity (see https://github.com/Kong/kong-ee/pull/4013 for inspiration)
Note that a base64 encoded version of a key-pair is not supported currently, but may be useful

@RobSerafini RobSerafini added this to the 3.2 milestone Jan 19, 2023
kikito
kikito previously requested changes Feb 1, 2023
Copy link
Member

@kikito kikito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment would need to be actioned before this PR can be merged:

@szesch the key/key-sets feature is in Kong now. There is an ee-plugin that already uses this entity (see https://github.com/Kong/kong-ee/pull/4013 for inspiration)
Note that a base64 encoded version of a key-pair is not supported currently, but may be useful

@kikito
Copy link
Member

kikito commented Feb 1, 2023

Since this PR has not been updated in time, I am removing it from the 3.2 milestone.

@kikito kikito removed this from the 3.2 milestone Feb 1, 2023
@szesch szesch force-pushed the feat/acme-plugin-account-key branch from 357dca4 to 5186c0e Compare February 10, 2023 20:50
@szesch
Copy link
Contributor Author

szesch commented Feb 10, 2023

@fffonion @jschmid1 I've updated the code to use keys and key_sets.

Copy link
Contributor

@fffonion fffonion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jschmid1 could you take another look?

@jschmid1 jschmid1 self-requested a review February 13, 2023 10:28
Copy link
Contributor

@jschmid1 jschmid1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@szesch szesch requested a review from kikito February 14, 2023 15:18
@fffonion fffonion dismissed kikito’s stale review February 28, 2023 09:59

dismiss as it's addressed

@fffonion
Copy link
Contributor

fffonion commented Feb 28, 2023

I'm merging this in as it's been a long while. @szesch Could you also make seperate PR to add changelog in https://github.com/Kong/kong/blob/master/CHANGELOG.md and update docs at kong/docs.konghq.com repo?

@fffonion fffonion merged commit f14d68a into master Feb 28, 2023
@fffonion fffonion deleted the feat/acme-plugin-account-key branch February 28, 2023 10:02
oowl pushed a commit that referenced this pull request Aug 15, 2024
…s has been changed from `age` to `Age` (#9360)" (#9746)

This reverts commit d3fc46c.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants