AuthCallout request should include TLS data when client is NATS WS client #4544
Conversation
server/auth_callout_test.go
Outdated
| ` | ||
| handler := func(m *nats.Msg) { | ||
| user, si, ci, _, ctls := decodeAuthRequest(t, m.Data) | ||
| require_True(t, si.Name == "T") |
There was a problem hiding this comment.
Please use require_Equal instead of require_True in these cases, otherwise the test logging doesn't say anything useful (like what the values were) when the condition fails.
There was a problem hiding this comment.
Oh man, you're brutal. I just copied @derekcollison existing test verbatim (on purpose) with only change the nature of the client as NATS WS TLS instead of NATS TLS.
There was a problem hiding this comment.
Sorry :D It's on my list to go through at some point and tidy up these things in the tests, as we've had an awful lot of lacking logging output when debugging flakes recently, but I'd rather not make the problem worse in the meantime.
git grep "require_True" | grep "==" | wc -l
647
git grep "require_True" | grep "!=" | wc -l
61
There was a problem hiding this comment.
I like to tease. :-)
I am seeing that fixing the issue of this PR surfaced a WS compression test artifact that overrides the natural underlying type of the *net.Conn for compression testing purposes and thus causes server panic in monitor.go when the *tls.Conn is mysteriously changed to a *server.testWSWrappedConn. Will fix with better type check handling in monitor.go.
…he natural underlying type for test purposes which would otherwise cause a panic
… better fail logs
Make sure the client handshake flag is set when TLS handshake is made as part of WebSocket connection/upgrade (notionally HTTPS) rather than as part of the NATS protocol TLS initiation chain. AuthCallout tests the flag when building the data for the AuthCallout service request.
Added AuthCallout unit test for NATS WS client auth that requires the TLS data.