-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Conversation
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.
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>
b4d7de5
to
7ccbaca
Compare
@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>
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.
LGTM
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.
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>
@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. |
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.
LGTM
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.
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>
@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) |
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.
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