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

AuthCallout request should include TLS data when client is NATS WS client #4544

Merged
merged 4 commits into from Sep 15, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
103 changes: 103 additions & 0 deletions server/auth_callout_test.go
Expand Up @@ -152,6 +152,30 @@ func (at *authTest) Connect(clientOptions ...nats.Option) *nats.Conn {
return conn
}

func (at *authTest) WSNewClient(clientOptions ...nats.Option) (*nats.Conn, error) {
pi := at.srv.PortsInfo(10 * time.Millisecond)
require_False(at.t, pi == nil)

// test cert is SAN to DNS localhost, not local IPs returned by server in test environments
wssUrl := strings.Replace(pi.WebSocket[0], "127.0.0.1", "localhost", 1)

// Seeing 127.0.1.1 in some test environments...
wssUrl = strings.Replace(wssUrl, "127.0.1.1", "localhost", 1)

conn, err := nats.Connect(wssUrl, clientOptions...)
if err != nil {
return nil, err
}
at.clients = append(at.clients, conn)
return conn, nil
}

func (at *authTest) WSConnect(clientOptions ...nats.Option) *nats.Conn {
conn, err := at.WSNewClient(clientOptions...)
require_NoError(at.t, err)
return conn
}

func (at *authTest) RequireConnectError(clientOptions ...nats.Option) {
_, err := at.NewClient(clientOptions...)
require_Error(at.t, err)
Expand Down Expand Up @@ -1423,3 +1447,82 @@ func TestAuthCalloutOperator_AnyAccount(t *testing.T) {
userInfo = response.Data.(*UserInfo)
require_Equal(t, userInfo.Account, bpk)
}

func TestAuthCalloutWSClientTLSCerts(t *testing.T) {
conf := `
server_name: T
listen: "localhost:-1"

tls {
cert_file = "../test/configs/certs/tlsauth/server.pem"
key_file = "../test/configs/certs/tlsauth/server-key.pem"
ca_file = "../test/configs/certs/tlsauth/ca.pem"
verify = true
}

websocket: {
listen: "localhost:-1"
tls {
cert_file = "../test/configs/certs/tlsauth/server.pem"
key_file = "../test/configs/certs/tlsauth/server-key.pem"
ca_file = "../test/configs/certs/tlsauth/ca.pem"
verify = true
}
}

accounts {
AUTH { users [ {user: "auth", password: "pwd"} ] }
FOO {}
}
authorization {
timeout: 1s
auth_callout {
# Needs to be a public account nkey, will work for both server config and operator mode.
issuer: "ABJHLOVMPA4CI6R5KLNGOB4GSLNIY7IOUPAJC4YFNDLQVIOBYQGUWVLA"
account: AUTH
auth_users: [ auth ]
}
}
`
handler := func(m *nats.Msg) {
user, si, ci, _, ctls := decodeAuthRequest(t, m.Data)
require_True(t, si.Name == "T")
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

require_True(t, ci.Host == "127.0.0.1")
require_True(t, ctls != nil)
// Zero since we are verified and will be under verified chains.
require_True(t, len(ctls.Certs) == 0)
require_True(t, len(ctls.VerifiedChains) == 1)
// Since we have a CA.
require_True(t, len(ctls.VerifiedChains[0]) == 2)
blk, _ := pem.Decode([]byte(ctls.VerifiedChains[0][0]))
cert, err := x509.ParseCertificate(blk.Bytes)
require_NoError(t, err)
if strings.HasPrefix(cert.Subject.String(), "CN=example.com") {
// Override blank name here, server will substitute.
ujwt := createAuthUser(t, user, "dlc", "FOO", "", nil, 0, nil)
m.Respond(serviceResponse(t, user, si.ID, ujwt, "", 0))
}
}

ac := NewAuthTest(t, conf, handler,
nats.UserInfo("auth", "pwd"),
nats.ClientCert("../test/configs/certs/tlsauth/client2.pem", "../test/configs/certs/tlsauth/client2-key.pem"),
nats.RootCAs("../test/configs/certs/tlsauth/ca.pem"))
defer ac.Cleanup()

// Will use client cert to determine user.
nc := ac.WSConnect(
nats.ClientCert("../test/configs/certs/tlsauth/client2.pem", "../test/configs/certs/tlsauth/client2-key.pem"),
nats.RootCAs("../test/configs/certs/tlsauth/ca.pem"),
)

resp, err := nc.Request(userDirectInfoSubj, nil, time.Second)
require_NoError(t, err)
response := ServerAPIResponse{Data: &UserInfo{}}
err = json.Unmarshal(resp.Data, &response)
require_NoError(t, err)
userInfo := response.Data.(*UserInfo)

require_True(t, userInfo.UserID == "dlc")
require_True(t, userInfo.Account == "FOO")
}
8 changes: 5 additions & 3 deletions server/websocket.go
Expand Up @@ -1234,12 +1234,14 @@ func (s *Server) createWSClient(conn net.Conn, ws *websocket) *client {
return nil
}
s.clients[c.cid] = c

// Websocket clients do TLS in the websocket http server.
// So no TLS here...
s.mu.Unlock()

c.mu.Lock()
// Websocket clients do TLS in the websocket http server.
// So no TLS initiation here...
if _, ok := conn.(*tls.Conn); ok {
c.flags.set(handshakeComplete)
}

if c.isClosed() {
c.mu.Unlock()
Expand Down