[FEAT] additional WS authentication cookie configurations #5131
Conversation
There was a problem hiding this comment.
Some questions / comments.
Also want @kozlovic to take a peek.
Looks good though and agree would be good addition.
| // Name of the cookie, which if present in WebSocket upgrade headers, | ||
| // will be treated as Token during CONNECT phase as long as | ||
| // "auth_token" specified in the CONNECT options is missing or empty. | ||
| TokenCookie string |
There was a problem hiding this comment.
Should we encourage this? I feel it should not be used and in my mind is deprecated. WDYT?
There was a problem hiding this comment.
This is the perfect location for users to provide a non-nats JWT for callout to process.
There was a problem hiding this comment.
(this is why I thought this was a good thing to do)
There was a problem hiding this comment.
ok that make sense. So maybe we call that out in the comment, that this should not be a plain server wide token but something external that folks want passed to the auth callout service etc.
There was a problem hiding this comment.
well, it can be anything they want, including a server-wide token even if that is discouraged.
There was a problem hiding this comment.
(added a good comment line)
aricart
force-pushed
the
allow-other-ws-cookies
branch
from
4ab7b3b
to
81e8e71
Compare
server/client.go
Outdated
| @@ -2029,6 +2029,22 @@ func (c *client) processConnect(arg []byte) error { | |||
| if ws := c.ws; ws != nil && c.opts.JWT == "" { | |||
There was a problem hiding this comment.
Why don't we consolidate everything under that first part of the if statement:
if ws := c.ws; ws != nil {
if c.opts.JWT == _EMPTY_ {
c.opts.JWT = ws.cookieJwt
}
if c.opts.Username == _EMPTY_ {
c.opts.Username = ws.cookieUsername
}
...
}
81e8e71
to
e8f3e8e
Compare
server/client.go
Outdated
| @@ -2026,9 +2026,25 @@ func (c *client) processConnect(arg []byte) error { | |||
| } | |||
|
|
|||
| // If websocket client and JWT not in the CONNECT, use the cookie JWT (possibly empty). | |||
| if ws := c.ws; ws != nil && c.opts.JWT == "" { | |||
| if ws := c.ws; ws != nil && c.opts.JWT == _EMPTY_ { | |||
There was a problem hiding this comment.
I am not sure if my earlier comment was properly posted. Trying again:
Why don't we consolidate everything under that first part of the if statement:
if ws := c.ws; ws != nil {
if c.opts.JWT == _EMPTY_ {
c.opts.JWT = ws.cookieJwt
}
if c.opts.Username == _EMPTY_ {
c.opts.Username = ws.cookieUsername
}
...
}
Basically, no need to check for ws := c.ws; ws != nil every time.
There was a problem hiding this comment.
didn't complete it yet.. Now it is in
…ie` to the WS configuration block - these mirror the behavior of `jwt_cookie`. If the options are not specified by the client but the server is has this configured, a WS connection will lookup values for the corresponding options under the HTTP cookie. This simplies websocket applications by allowing the HTTP application to set cookies that provide authentication details.
aricart
force-pushed
the
allow-other-ws-cookies
branch
from
e8f3e8e
to
d604747
Compare
[FEAT] this change adds
user_cookie,pass_cookie, andtoken_cookieto the WS configuration block - these mirror the behavior ofjwt_cookie. If the options are not specified by the client but the server is has them configured, a WS connection will lookup values for the corresponding options under the HTTP cookie. This simplies websocket applications by allowing the HTTP application to set cookies that provide authentication details.Simplifies #5047
Signed-off-by: Alberto Ricart aricart@synadia.com