From 69525f3083a714b40a82e0e5e693f860d47248e5 Mon Sep 17 00:00:00 2001 From: Ivan Kozlovic Date: Tue, 14 Dec 2021 09:56:24 -0700 Subject: [PATCH] [FIXED] Check for no_auth_user Check for a no_auth_user should be done only when no authentication at all is provided by the user. This was not the case. For instance, if the user provided a token, the server would still check for no_auth_user if users are defined. It was not really an issue since the admin cannot configure users AND token, but it is better for the application to fail if providing a token that is actually not being used. If the admin configures a no_auth_user, this should be used only when no authentication is provided. Signed-off-by: Ivan Kozlovic --- server/auth.go | 3 ++- server/auth_test.go | 61 +++++++++++++++++++++++++++++++++++++++++++ server/events_test.go | 60 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 123 insertions(+), 1 deletion(-) diff --git a/server/auth.go b/server/auth.go index fb2c797f..c322777d 100644 --- a/server/auth.go +++ b/server/auth.go @@ -538,7 +538,8 @@ func (s *Server) processClientOrLeafAuthentication(c *client, opts *Options) boo // but we set it here to be able to identify it in the logs. c.opts.Username = user.Username } else { - if (c.kind == CLIENT || c.kind == LEAF) && c.opts.Username == _EMPTY_ && noAuthUser != _EMPTY_ { + if (c.kind == CLIENT || c.kind == LEAF) && noAuthUser != _EMPTY_ && + c.opts.Username == _EMPTY_ && c.opts.Password == _EMPTY_ && c.opts.Token == _EMPTY_ { if u, exists := s.users[noAuthUser]; exists { c.mu.Lock() c.opts.Username = u.Username diff --git a/server/auth_test.go b/server/auth_test.go index 1395250a..65cc6fcb 100644 --- a/server/auth_test.go +++ b/server/auth_test.go @@ -14,12 +14,15 @@ package server import ( + "fmt" "net/url" + "os" "reflect" "strings" "testing" "github.com/nats-io/jwt/v2" + "github.com/nats-io/nats.go" ) func TestUserCloneNilPermissions(t *testing.T) { @@ -212,3 +215,61 @@ func TestDNSAltNameMatching(t *testing.T) { } } } + +func TestNoAuthUser(t *testing.T) { + conf := createConfFile(t, []byte(` + listen: "127.0.0.1:-1" + accounts { + FOO { users [{user: "foo", password: "pwd1"}] } + BAR { users [{user: "bar", password: "pwd2"}] } + } + no_auth_user: "foo" + `)) + defer os.Remove(conf) + s, o := RunServerWithConfig(conf) + defer s.Shutdown() + + for _, test := range []struct { + name string + usrInfo string + ok bool + account string + }{ + {"valid user/pwd", "bar:pwd2@", true, "BAR"}, + {"invalid pwd", "bar:wrong@", false, _EMPTY_}, + {"some token", "sometoken@", false, _EMPTY_}, + {"user used without pwd", "bar@", false, _EMPTY_}, // will be treated as a token + {"user with empty password", "bar:@", false, _EMPTY_}, + {"no user", _EMPTY_, true, "FOO"}, + } { + t.Run(test.name, func(t *testing.T) { + url := fmt.Sprintf("nats://%s127.0.0.1:%d", test.usrInfo, o.Port) + nc, err := nats.Connect(url) + if err != nil { + if test.ok { + t.Fatalf("Unexpected error: %v", err) + } + return + } else if !test.ok { + nc.Close() + t.Fatalf("Should have failed, did not") + } + var accName string + s.mu.Lock() + for _, c := range s.clients { + c.mu.Lock() + if c.acc != nil { + accName = c.acc.Name + } + c.mu.Unlock() + break + } + s.mu.Unlock() + nc.Close() + checkClientsCount(t, s, 0) + if accName != test.account { + t.Fatalf("The account should have been %q, got %q", test.account, accName) + } + }) + } +} diff --git a/server/events_test.go b/server/events_test.go index ff0855bd..e2f12b51 100644 --- a/server/events_test.go +++ b/server/events_test.go @@ -19,6 +19,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "os" "strings" "sync" "sync/atomic" @@ -1661,6 +1662,65 @@ func TestSystemAccountWithGateways(t *testing.T) { t.Fatal("Expected a message") } } + +func TestSystemAccountNoAuthUser(t *testing.T) { + conf := createConfFile(t, []byte(` + listen: "127.0.0.1:-1" + accounts { + $SYS { + users [{user: "admin", password: "pwd"}] + } + } + `)) + defer os.Remove(conf) + s, o := RunServerWithConfig(conf) + defer s.Shutdown() + + for _, test := range []struct { + name string + usrInfo string + ok bool + account string + }{ + {"valid user/pwd", "admin:pwd@", true, "$SYS"}, + {"invalid pwd", "admin:wrong@", false, _EMPTY_}, + {"some token", "sometoken@", false, _EMPTY_}, + {"user used without pwd", "admin@", false, _EMPTY_}, // will be treated as a token + {"user with empty password", "admin:@", false, _EMPTY_}, + {"no user means global account", _EMPTY_, true, globalAccountName}, + } { + t.Run(test.name, func(t *testing.T) { + url := fmt.Sprintf("nats://%s127.0.0.1:%d", test.usrInfo, o.Port) + nc, err := nats.Connect(url) + if err != nil { + if test.ok { + t.Fatalf("Unexpected error: %v", err) + } + return + } else if !test.ok { + nc.Close() + t.Fatalf("Should have failed, did not") + } + var accName string + s.mu.Lock() + for _, c := range s.clients { + c.mu.Lock() + if c.acc != nil { + accName = c.acc.Name + } + c.mu.Unlock() + break + } + s.mu.Unlock() + nc.Close() + checkClientsCount(t, s, 0) + if accName != test.account { + t.Fatalf("The account should have been %q, got %q", test.account, accName) + } + }) + } +} + func TestServerEventsStatsZ(t *testing.T) { serverStatsReqSubj := "$SYS.REQ.SERVER.%s.STATSZ" preStart := time.Now().UTC()