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

JSON Web Key Support #410

Merged
merged 10 commits into from
Jan 19, 2021
Merged

JSON Web Key Support #410

merged 10 commits into from
Jan 19, 2021

Conversation

danikarik
Copy link
Contributor

Proposed changes

In case of asymmetric JWT encryption, it's hard to update or verify incoming connection tokens due to rotation nature of keys. Current scheme of interaction produces a lot of request to refresh proxy endpoints.

According to RFC7517 there is mechanism where we can put KeyID (kid) into token claims and find appropriate public rsa key for verifying using key identifier. All those public keys are available by specific url. This scheme of interaction makes possible to use same jwt token as system's internal token and as Centrifugo's connection token. But of course it works only if system's jwt token fully compatible with Centrifugo's connection token and JWKS public url and token's kid are specified.

What I suggesting

  • Added new option for authentication (current token_hmac_secret_key and token_rsa_public_key) - jwks_public_url
  • Added new jwks verifier which requests set of JWK by fetching from url above. Then using kid in claims it finds public key and verifies signature of token.

@FZambia
Copy link
Member

FZambia commented Jan 12, 2021

@danikarik hello, thanks - seems nice to have. I'll come with review comments very soon - need time to get some understanding on topic.

For now I have one question - have you already tried that inside your token infrastructure?

@danikarik
Copy link
Contributor Author

Tried it in PoC. Planning to deploy it on staging within couple of days.

Copy link
Member

@FZambia FZambia left a comment

Choose a reason for hiding this comment

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

Added comments, plz take a look.

internal/jwtverify/token_verifier_jwt.go Outdated Show resolved Hide resolved
internal/jwtverify/token_verifier.go Outdated Show resolved Hide resolved
internal/jwtverify/token_verifier_jwt.go Outdated Show resolved Hide resolved
internal/jwtverify/token_verifier_jwt.go Outdated Show resolved Hide resolved
internal/jwtverify/token_verifier_jwt.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
internal/jwtverify/token_verifier_jwt.go Outdated Show resolved Hide resolved
go.sum Outdated Show resolved Hide resolved
@danikarik
Copy link
Contributor Author

Last thing is to add own JWKS wrapper.

@danikarik
Copy link
Contributor Author

Added own jwks manager to deal with cache and retrieving set from specified endpoint.

internal/jwks/cache.go Outdated Show resolved Hide resolved
select {
case <-ticker:
tc.cleanup()
case <-tc.stop:
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to stop cache at the moment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

internal/jwks/manager.go Outdated Show resolved Hide resolved
internal/jwks/manager.go Outdated Show resolved Hide resolved
@FZambia
Copy link
Member

FZambia commented Jan 19, 2021

Thanks @danikarik, great job 👍 Ready to be merged I suppose. Going to make some minor tweaks here before release. Also JWKS support should be documented – will ask you to review documentation as soon as I send update to it.

@cristaloleg, @C-Pro thanks for your help!

@FZambia FZambia merged commit 7ba39ab into centrifugal:master Jan 19, 2021
@FZambia FZambia mentioned this pull request Jan 19, 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.

4 participants