-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[ADDED] Support for JWT BearerToken #1226
Conversation
bf6a4a4
to
a0693b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same than for the JWT PR... technically LGTM, let's just all decide if we want to add this or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should be a server directive to allow the user JWTs to act as bearer tokens. We would still do the validity checks up the chain, just not require the nonce. Alternatively we could have clients place it in a different named field in the connect json.
// Allow fallback to normal base64. | ||
sig, err = base64.StdEncoding.DecodeString(c.opts.Sig) | ||
// skip validation of nonce when presented with a bearer token | ||
if !juc.BearerToken { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't be up to the account to place in the user jwt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not secure. Account owner can create a user that circumvents the nonce/signature check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the JWT should disable the check, but the server directive validates whether those directives are valid, otherwise, you wouldn't be able to have the mix. You may want accounts that don't allow it but allow a different account to take the bearer tokens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok you are correct for the JWT, apologies. I am wondering more if client should look like it treats them differently, meaning to use token vs creds since you don't need the PK. This may be clearer. Account can still indicate whether it allows bearer tokens etc. So probably needs to be marked in the user jwt as such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am understanding that we want to do token@nats://
or put it in the token
option.
I think this is actually more complex for the server. The use case of a bearer token is to have a server assigned JWT be used. Client configuration will happen automatically, so there's no confusion there. Now if we stuff it into the token we'll have several code paths that we have to resolve - we would have to try token as a token/user auth, and then try to parse the token as a JWT, and if so stuff it back to where JWTs are handled.
This is currently what auth looks like on the ws client:
let conn = await nats.connect({
url: "{{WSURL}}",
payload: Payload.JSON,
userJWT: "eyJ0eXAiOiJqd3QiLCJhbGciOiJlZDI1NTE5In0.eyJqdGkiOiJSNko1QkdDUkhWM1FHUUxRTTRPVTM0UFk2Rk0yTlJXNFNZSURaQlpPVkpRU1E2TTRUQ1BBIiwiaWF0IjoxNTc2Njc5MTkwLCJpc3MiOiJBRDdZNFYyTkRXR05CRlhHTjZCV0RWVFJNSEY0QkVVTFZPUzRCNTZDSUpMWktNREVUTlBBWUdFUCIsIm5hbWUiOiJVIiwic3ViIjoiVUJBUlNKUFc3UVlYWFRNQVRXQkZMTUQ1WVhQWFBDVVA3QjdPVTVVQ1dJVkdUTExaQUNUSUpQVDYiLCJ0eXBlIjoidXNlciIsIm5hdHMiOnsicHViIjp7fSwic3ViIjp7fX0sImJlYXJlcl90b2tlbiI6dHJ1ZX0.jDrXwtfTY5uv6keYlvwmKNproQFLKtMrFhwYmJmilwtkeAp91Q4qDJUl2I3QWkcdLO1oHEpHEaOqQw96MQQnCw"
});
In go they would be doing:
nc, err := nats.Connect(url, nats.UserJWT(jwtCB, func(challenge string) string {
return ""
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's get on a GH call with you, me and Ivan possibly Phil to finalize.
a0693b4
to
cff9aeb
Compare
Updated the PR to include a server flag/option allow_bearer_tokens which allows the server to enforce whether the feature should be honored. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per previous meeting, the global configuration in the server has been removed. LGTM (we can update the deps after when JWT is actually released).
Disables nonce signature for JWT that has the BearerToken boolean set to true. Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
bearer_token
set to true
@derekcollison @aricart Rebased from master and added a test with JWT user claim with BearerToken set and no nonce signing, verifying that server accepts. |
This relaxes the nonce signing checks when a bearer token is provided. PR is a preview and depends on JWT changes.