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

Added an allowed connection type filter for users #1594

Merged
merged 4 commits into from Sep 18, 2020
Merged

Conversation

kozlovic
Copy link
Member

@kozlovic kozlovic commented Sep 9, 2020

Users and NKey users will now have the option to specify a list
of allowed connection types.

This will allow for instance a certain user to be allowed to
connect as a standard NATS client, but not as Websocket, or
vice-versa.

This also fixes the websocket auth override. Indeed, with
the original behavior, the websocket users would have been bound
to $G, which would not work when there are accounts defined, since
when that is the case, no app can connect/bind to $G account.

Signed-off-by: Ivan Kozlovic ivan@synadia.com

Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM

Users and NKey users will now have the option to specify a list
of allowed connection types.

This will allow for instance a certain user to be allowed to
connect as a standard NATS client, but not as Websocket, or
vice-versa.

This also fixes the websocket auth override. Indeed, with
the original behavior, the websocket users would have been bound
to $G, which would not work when there are accounts defined, since
when that is the case, no app can connect/bind to $G account.

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@kozlovic kozlovic changed the title Fixed websocket auth override Added an allowed connection type filter for users Sep 17, 2020
@kozlovic
Copy link
Member Author

@derekcollison I have completely changed the approach with a list of connection types allowed for given user/nkey. We updated the JWT library to provide this new field in the JWT and some constants.

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@matthiashanel matthiashanel left a comment

Choose a reason for hiding this comment

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

The change looks good.
I wonder if we should have a test in jwt_test.go that tests using a new type in a jwt and make sure the server rejects it.

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@kozlovic
Copy link
Member Author

@matthiashanel Thanks for that because it made me realize that in the case of JWT coming with an unknown type, we should not automatically fail the connection. See the big comment that I added in auth.go and let me know if you agree or not.

Copy link
Contributor

@matthiashanel matthiashanel left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@matthiashanel matthiashanel left a comment

Choose a reason for hiding this comment

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

LGTM

We were doing option validation from options parsing, but added
it also for Users/NKeyUsers options.

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@kozlovic
Copy link
Member Author

@matthiashanel Sorry, I added yet another commit: do validation of options for users that embed NATS Server in their app and would configure Users/NKeys directly. (I will squash all those commits at time of merge)

Copy link
Contributor

@matthiashanel matthiashanel left a comment

Choose a reason for hiding this comment

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

LGTM

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