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(kuma-cp) user token with RSA256 #2992

Merged
merged 8 commits into from
Oct 28, 2021
Merged

Conversation

jakubdyszkiewicz
Copy link
Contributor

Summary

After discussing offline with @jpeach #2923 (comment) we decided we want to switch to RSA256 to provide the future possibility of extending User Token to be able to generate private key offline, issue tokens offline, and provide the public key as plain config.

Full changelog

  • Change JWT alg to RSA256. We already used RSA256 key but for JWT alg we picked HMAC which results in symmetric cryptography. Switch to RSA256 means that we can use only a public part of the key to validate tokens.
  • UserTokenIssuer was split to UserTokenIssuer/UserTokenValidator and UserTokenValidator is now using SigningKeyAccessor. In the future, we can implement static version of UserTokenValidator that relies ONLY on public key provided as a config (for example KUMA_API_SERVER_AUTHN_USER_TOKEN_PUB_KEYS). Of course, that means that token issuer would not be available in the control plane, but tokens will be issued offline somewhere else (other system like Vault or kumactl).
  • I considered splitting the key to two GlobalSecret resources, one for private key and one for public key, but since public key can be retrieved from private key in PKCS1 format, it seems like a duplication of information. Additionally, it's an extra hassle (generate two resources, when rotate you need to set two of them etc.). We can add kumactl extract public-key --private-key-file=key.pem or something like this.

Issues resolved

Related #2955

Documentation

  • UX stays the same with the online issuer.

Testing

  • Unit tests
  • E2E tests
  • Manual testing on Universal
  • Manual testing on Kubernetes

Backwards compatibility

  • No backporting.

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
@jakubdyszkiewicz jakubdyszkiewicz requested a review from a team as a code owner October 22, 2021 15:37
@codecov-commenter
Copy link

codecov-commenter commented Oct 22, 2021

Codecov Report

Merging #2992 (039d5d0) into master (c98be17) will increase coverage by 0.03%.
The diff coverage is 69.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2992      +/-   ##
==========================================
+ Coverage   52.30%   52.33%   +0.03%     
==========================================
  Files         916      919       +3     
  Lines       52962    52999      +37     
==========================================
+ Hits        27701    27739      +38     
+ Misses      23056    23048       -8     
- Partials     2205     2212       +7     
Impacted Files Coverage Δ
...s/authn/api-server/tokens/admin_token_bootstrap.go 82.69% <ø> (ø)
...hn/api-server/tokens/issuer/default_signing_key.go 67.56% <0.00%> (ø)
...g/plugins/authn/api-server/tokens/authenticator.go 7.14% <25.00%> (-0.55%) ⬇️
...lugins/authn/api-server/tokens/issuer/validator.go 66.66% <66.66%> (ø)
pkg/util/rsa/pem.go 69.23% <69.23%> (ø)
...gins/authn/api-server/tokens/issuer/signing_key.go 75.43% <71.42%> (-1.35%) ⬇️
...n/api-server/tokens/issuer/signing_key_accessor.go 78.57% <78.57%> (ø)
pkg/plugins/authn/api-server/tokens/plugin.go 76.92% <87.50%> (+1.92%) ⬆️
...g/plugins/authn/api-server/tokens/issuer/issuer.go 90.90% <100.00%> (+12.64%) ⬆️
pkg/xds/cache/once/cache.go 87.17% <0.00%> (-7.70%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c98be17...039d5d0. Read the comment docs.

@jpeach
Copy link
Contributor

jpeach commented Oct 25, 2021

Oh, also I think the commentary from the PR is really useful context that should be included in the commit message :)

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
pkg/util/rsa/pem.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jpeach jpeach left a comment

Choose a reason for hiding this comment

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

I had some minor quibbles about error propagation, but overall this looks great 👍

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
@jakubdyszkiewicz jakubdyszkiewicz merged commit 4c0f561 into master Oct 28, 2021
@jakubdyszkiewicz jakubdyszkiewicz deleted the chore/issuer-changes branch October 28, 2021 11:19
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.

None yet

3 participants