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

Support for User JWTs #408

Merged
merged 2 commits into from
Nov 24, 2018
Merged

Support for User JWTs #408

merged 2 commits into from
Nov 24, 2018

Conversation

derekcollison
Copy link
Member

This adds support for a user JWT. This adds to bare nkey support.

We support this as a callback to support JWT upgrades while an application is running.

We support decorated formats as provided by nsc, and we support chained files that have the user JWT and the user seed in the same file.

Signed-off-by: Derek Collison [email protected]

@coveralls
Copy link

coveralls commented Nov 24, 2018

Coverage Status

Coverage decreased (-1.05%) to 93.049% when pulling 4e4a9ca on userjwt into 9089d12 on master.

Signed-off-by: Derek Collison <[email protected]>
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Small nitpicks, otherwise LGTM

nats.go Outdated
@@ -1350,8 +1425,8 @@ func (nc *Conn) connectProto() (string, error) {
token = nc.Opts.TokenHandler()
}

cinfo := connectInfo{o.Verbose, o.Pedantic, nkey, sig,
user, pass, token, o.Secure, o.Name, LangString,
cinfo := connectInfo{o.Verbose, o.Pedantic, string(ujwt),
Copy link
Member

Choose a reason for hiding this comment

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

ujwt seem to be a string already, so no need to cast.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, had it as []byte at one point.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will fix.

} else {
ujwt = jwt
}
if nkey != _EMPTY_ {
Copy link
Member

Choose a reason for hiding this comment

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

I see that we already reject mix of NKey and UserJWT at beginning of Connect(). So why test here? Even if we want to allow options to be modified after the first Connect() (which we should not encourage), then this test should be probably done before calling o.UserJWT(), no? or at least check that returned ujwt is not empty. Meaning that if o.UserJWT() returns non empty, then we should fail if nkey is non empty too, but can be set as long as o.UserJWT() returned empty.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think to capture nats.Connect() and opts.Connect() scenarios.

Copy link

@sasbury sasbury left a comment

Choose a reason for hiding this comment

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

Defer to Ivan, but looks ok to me from the JWT side

Signed-off-by: Derek Collison <[email protected]>
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