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

Allow CA to configure different key type for JWTs and X509 #1991

Merged
merged 5 commits into from
Mar 4, 2021

Conversation

mbyczkowski
Copy link
Contributor

Pull Request check list

  • Commit conforms to CONTRIBUTING.md?
  • Proper tests/regressions included?
  • Documentation updated?

Affected functionality
CA Manager

Description of change
This change introduces jwt_key_type config option and allows to
directly control which key type will be used by CA Manager to sign JWTs.

Existing ca_key_type config will continue to be used for defining X509
CA key type, but will no longer affect JWT key type.

Which issue this PR fixes
fixes #1928

@mbyczkowski mbyczkowski marked this pull request as ready for review December 7, 2020 19:26
@@ -71,6 +71,9 @@ type Config struct {
// CAKeyType is the key type used for the X509 and JWT signing keys
CAKeyType keymanager.KeyType
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if this should be renamed to X509CAKeyType? If so, maybe there should be some backwards compatibility with the ca_key_type config field?

@azdagron
Copy link
Member

azdagron commented Dec 10, 2020

Thanks for putting this together, @mbyczkowski!

As-is, this will break backwards compatibility for those who are currently using the ca_key_type configurable to change the JWT key type (which is pretty common for those using SPIRE to provide JWKS for cloud OIDC providers). I think whatever solution we land on here needs to maintain that compatibility.

A couple of options:
A. ca_key_type continues to set both the X.509 and JWT key types but can be overridden by either an explicit (and new) x509_key_type or jwt_key_type configurable.
B. same as #1 but we eventually deprecate ca_key_type so that you have to set two configurables if you want to change both away from the default.

The tradeoff between the two options is between flexibility and minimizing configurables. I personally am leaning towards A. Curious what @evan2645 thinks here, based on the previous slack discussion that started the issue.

@evan2645 evan2645 self-assigned this Dec 15, 2020
This change introduces `jwt_key_type` config option and allows to
directly control which key type will be used by CA Manager to sign JWTs.

Existing `ca_key_type` config will continue to be used for defining X509
CA key type, but will no longer affect JWT key type.

Signed-off-by: Mat Byczkowski <mbyczkowski@squareup.com>
@mbyczkowski
Copy link
Contributor Author

@azdagron I agree that, if possible, we should try not to break compatibility (and by extension try not to surprise SPIRE users!). Both options, that you have laid out, seem reasonable to me. If we were to go with A. then we're not forcing any changes on the users (just need to make sure this is well documented and tested!). With time this can turn into B. if SPIRE maintainers decide it's time to simplify ca_key_type is standing in the way.

As a side note if you or @evan2645 can point at any additional tests I could add here, I'd be happy to do that as well to the PR.

@evan2645
Copy link
Member

evan2645 commented Jan 4, 2021

Hey @mbyczkowski, sorry for the delay, thanks for bearing with us as we get through the holiday season :)

I do think there's quite a bit of work to do w.r.t. configurable key types. For example, if a user configures ca_key_type to be RSA, wouldn't they likely want their leaf certificates to use the same key type? And in that case, x509_key_type probably makes more sense and is aligned with jwt_key_type name-wise... all that said, there will be many steps to get there.

The first step of that journey I think will be the same, regardless of the direction we choose. Introducing x509_key_type now feels premature since it is likely to gain more meaning in the future, and its indefinite coexistence with ca_key_type will probably be confusing... So IMO, this PR should introduce jwt_key_type configurable which defaults to the value of ca_key_type. This will allow us to maintain compatibility for folks using ca_key_type to configure JWT signing keys, as well as light a deprecation path for ca_key_type when we're ready to do so.

What do you all think?

@azdagron azdagron self-assigned this Feb 2, 2021
@amartinezfayo
Copy link
Member

@mbyczkowski Thank you for this contribution!
I will be pushing updates to address the comments so we can get this merged. Thanks again!

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
@mbyczkowski
Copy link
Contributor Author

@amartinezfayo Thanks for taking over! I told @evan2645 I'd address the PR comments, but I haven't really had the time to do it recently, so appreciate your help here! :)

Copy link
Contributor

@APTy APTy left a comment

Choose a reason for hiding this comment

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

LGTM!

@amartinezfayo amartinezfayo merged commit c5d5304 into spiffe:master Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make JWT key type independently configurable
5 participants