Repository: nats-io/nats-server
Pull request #4605 (2023-09-28 21:03:09)

[FIXED] Do not bypass authorization blocks when turning on $SYS account access
Only setup auto no-auth for $G account iff no authorization block was defined.

Signed-off-by: Derek Collison <derek@nats.io>

Resolves #4535 




Patch:
diff --git a/server/opts.go b/server/opts.go
index 483548ce..039f982c 100644
--- a/server/opts.go
+++ b/server/opts.go
@@ -395,6 +395,9 @@ type Options struct {
 
 	// OCSP Cache config enables next-gen cache for OCSP features
 	OCSPCacheConfig *OCSPResponseCacheConfig
+
+	// Used to mark that we had a top level authorization block.
+	authBlockDefined bool
 }
 
 // WebsocketOpts are options for websocket
@@ -885,7 +888,7 @@ func (o *Options) processConfigFileLine(k string, v interface{}, errors *[]error
 			*errors = append(*errors, err)
 			return
 		}
-
+		o.authBlockDefined = true
 		o.Username = auth.user
 		o.Password = auth.pass
 		o.Authorization = auth.token
diff --git a/server/server.go b/server/server.go
index 5db5ca49..64a5534d 100644
--- a/server/server.go
+++ b/server/server.go
@@ -1239,8 +1239,8 @@ func (s *Server) configureAccounts(reloading bool) (map[string]struct{}, error)
 		// If we have defined a system account here check to see if its just us and the $G account.
 		// We would do this to add user/pass to the system account. If this is the case add in
 		// no-auth-user for $G.
-		// Only do this if non-operator mode.
-		if len(opts.TrustedOperators) == 0 && numAccounts == 2 && opts.NoAuthUser == _EMPTY_ {
+		// Only do this if non-operator mode and we did not have an authorization block defined.
+		if len(opts.TrustedOperators) == 0 && numAccounts == 2 && opts.NoAuthUser == _EMPTY_ && !opts.authBlockDefined {
 			// If we come here from config reload, let's not recreate the fake user name otherwise
 			// it will cause currently clients to be disconnected.
 			uname := s.sysAccOnlyNoAuthUser


Test patch:
diff --git a/server/mqtt_test.go b/server/mqtt_test.go
index 5712be0e..f8a28fea 100644
--- a/server/mqtt_test.go
+++ b/server/mqtt_test.go
@@ -4654,8 +4654,7 @@ func TestMQTTLockedSession(t *testing.T) {
 	s := testMQTTRunServer(t, o)
 	defer testMQTTShutdownServer(s)
 
-	subClientID := "sub"
-	ci := &mqttConnInfo{clientID: subClientID, cleanSess: false}
+	ci := &mqttConnInfo{clientID: "sub", cleanSess: false}
 	c, r := testMQTTConnect(t, ci, o.MQTT.Host, o.MQTT.Port)
 	defer c.Close()
 	testMQTTCheckConnAck(t, r, mqttConnAckRCConnectionAccepted, false)
@@ -4668,20 +4667,8 @@ func TestMQTTLockedSession(t *testing.T) {
 		t.Fatalf("account session manager not found")
 	}
 
-	// It is possible, however unlikely, to have received CONNACK while
-	// mqttProcessConnect is still running, and the session remains locked. Wait
-	// for it to finish.
-	checkFor(t, 250*time.Millisecond, 10*time.Millisecond, func() error {
-		asm.mu.RLock()
-		defer asm.mu.RUnlock()
-		if _, stillLocked := asm.sessLocked[subClientID]; stillLocked {
-			return fmt.Errorf("session still locked")
-		}
-		return nil
-	})
-
 	// Get the session for "sub"
-	cli := testMQTTGetClient(t, s, subClientID)
+	cli := testMQTTGetClient(t, s, "sub")
 	sess := cli.mqtt.sess
 
 	// Pretend that the session above is locked.
diff --git a/server/opts_test.go b/server/opts_test.go
index 4af13784..78920de6 100644
--- a/server/opts_test.go
+++ b/server/opts_test.go
@@ -120,6 +120,7 @@ func TestConfigFile(t *testing.T) {
 		LameDuckDuration:      4 * time.Minute,
 		ConnectErrorReports:   86400,
 		ReconnectErrorReports: 5,
+		authBlockDefined:      true,
 	}
 
 	opts, err := ProcessConfigFile("./configs/test.conf")
@@ -132,13 +133,14 @@ func TestConfigFile(t *testing.T) {
 
 func TestTLSConfigFile(t *testing.T) {
 	golden := &Options{
-		ConfigFile:  "./configs/tls.conf",
-		Host:        "127.0.0.1",
-		Port:        4443,
-		Username:    "derek",
-		Password:    "foo",
-		AuthTimeout: 1.0,
-		TLSTimeout:  2.0,
+		ConfigFile:       "./configs/tls.conf",
+		Host:             "127.0.0.1",
+		Port:             4443,
+		Username:         "derek",
+		Password:         "foo",
+		AuthTimeout:      1.0,
+		TLSTimeout:       2.0,
+		authBlockDefined: true,
 	}
 	opts, err := ProcessConfigFile("./configs/tls.conf")
 	if err != nil {
@@ -283,6 +285,7 @@ func TestMergeOverrides(t *testing.T) {
 		LameDuckDuration:      4 * time.Minute,
 		ConnectErrorReports:   86400,
 		ReconnectErrorReports: 5,
+		authBlockDefined:      true,
 	}
 	fopts, err := ProcessConfigFile("./configs/test.conf")
 	if err != nil {
diff --git a/server/routes_test.go b/server/routes_test.go
index 00fc4cc6..2798b091 100644
--- a/server/routes_test.go
+++ b/server/routes_test.go
@@ -105,7 +105,8 @@ func TestRouteConfig(t *testing.T) {
 			NoAdvertise:    true,
 			ConnectRetries: 2,
 		},
-		PidFile: "/tmp/nats-server/nats_cluster_test.pid",
+		PidFile:          "/tmp/nats-server/nats_cluster_test.pid",
+		authBlockDefined: true,
 	}
 
 	// Setup URLs
diff --git a/server/server_test.go b/server/server_test.go
index 3defd412..42b650ac 100644
--- a/server/server_test.go
+++ b/server/server_test.go
@@ -19,6 +19,7 @@ import (
 	"context"
 	"crypto/tls"
 	"encoding/json"
+	"errors"
 	"flag"
 	"fmt"
 	"io"
@@ -2080,3 +2081,29 @@ func TestServerRateLimitLogging(t *testing.T) {
 
 	checkLog(c1, c2)
 }
+
+// https://github.com/nats-io/nats-server/discussions/4535
+func TestServerAuthBlockAndSysAccounts(t *testing.T) {
+	conf := createConfFile(t, []byte(`
+		listen: 127.0.0.1:-1
+		server_name: s-test
+		authorization {
+			users = [ { user: "u", password: "pass"} ]
+		}
+		accounts {
+			$SYS: { users: [ { user: admin, password: pwd } ] }
+		}
+	`))
+
+	s, _ := RunServerWithConfig(conf)
+	defer s.Shutdown()
+
+	// This should work of course.
+	nc, err := nats.Connect(s.ClientURL(), nats.UserInfo("u", "pass"))
+	require_NoError(t, err)
+	defer nc.Close()
+
+	// This should not.
+	_, err = nats.Connect(s.ClientURL())
+	require_Error(t, err, nats.ErrAuthorization, errors.New("nats: Authorization Violation"))
+}