Repository: nats-io/nats-server (go)

Issue:
Nats server with resolver: {type: full} crashes if `system_account` is not set
## Defect

Make sure that these boxes are checked before submitting your issue -- thank you!

 - [x] Included `nats-server -DV` output
 - [x] Included a [Minimal, Complete, and Verifiable example] (https://stackoverflow.com/help/mcve)

#### Versions of `nats-server` and affected client libraries used:

```sh
$ nats-server -DV
[3662451] 2021/04/24 12:08:54.602759 [INF] Starting nats-server
[3662451] 2021/04/24 12:08:54.602821 [INF]   Version:  2.2.2
[3662451] 2021/04/24 12:08:54.602826 [INF]   Git:      [a5f3aab]
[3662451] 2021/04/24 12:08:54.602830 [DBG]   Go build: go1.16.3
[3662451] 2021/04/24 12:08:54.602834 [INF]   Name:     NCHSX3HTBSRJNWB2IJAM6R2FGBNN5FRZ3X33RGHG23OWHTILXXB7ORLX
[3662451] 2021/04/24 12:08:54.602838 [INF]   ID:       NCHSX3HTBSRJNWB2IJAM6R2FGBNN5FRZ3X33RGHG23OWHTILXXB7ORLX
[3662451] 2021/04/24 12:08:54.602868 [DBG] Created system account: "$SYS"
[3662451] 2021/04/24 12:08:54.603528 [INF] Listening for client connections on 0.0.0.0:4222
[3662451] 2021/04/24 12:08:54.603537 [DBG] Get non local IPs for "0.0.0.0"
[3662451] 2021/04/24 12:08:54.603921 [DBG]   ip=192.168.199.2
[3662451] 2021/04/24 12:08:54.603928 [DBG]   ip=fdfc:6c1c:2d83:10:c1dc:4824:353:6a4a
[3662451] 2021/04/24 12:08:54.603932 [DBG]   ip=fdfc:6c1c:2d83:10:81f2:d9e:85d:6b40
[3662451] 2021/04/24 12:08:54.603936 [DBG]   ip=fdfc:6c1c:2d83:10:c416:6500:883c:682c
[3662451] 2021/04/24 12:08:54.603965 [DBG]   ip=192.168.122.1
[3662451] 2021/04/24 12:08:54.604022 [DBG]   ip=192.168.39.1
[3662451] 2021/04/24 12:08:54.604076 [DBG]   ip=172.19.0.1
[3662451] 2021/04/24 12:08:54.604116 [DBG]   ip=172.17.0.1
[3662451] 2021/04/24 12:08:54.604155 [INF] Server is ready
^C[3662451] 2021/04/24 12:08:57.148228 [DBG] Trapped "interrupt" signal
[3662451] 2021/04/24 12:08:57.148324 [INF] Initiating Shutdown...
[3662451] 2021/04/24 12:08:57.148462 [DBG] SYSTEM - System connection closed: Client Closed
[3662451] 2021/04/24 12:08:57.148471 [DBG] Client accept loop exiting..
[3662451] 2021/04/24 12:08:57.148503 [INF] Server Exiting..
```

#### OS/Container environment:

Ubuntu 20.04.2 LTS

#### Steps or code to reproduce the issue:

Use this config

```
trace: true
operator: /home/sindre/.nsc/nats/O_operator/O_operator.jwt
resolver: {
    type: full
    dir: '/home/sindre/src/iterate/app.iterate.no/nats/testing_nsc/jwt'
}
```

Start nats-server

```
nats-server -c nas.config
```

#### Expected result:

Error message: `system_account required` or at least not crashing

#### Actual result:

Server crashes with a SIGSEGV

```
$ nats-server -c nas.config
[3663003] 2021/04/24 12:10:15.199546 [INF] Starting nats-server
[3663003] 2021/04/24 12:10:15.199613 [INF]   Version:  2.2.2
[3663003] 2021/04/24 12:10:15.199618 [INF]   Git:      [a5f3aab]
[3663003] 2021/04/24 12:10:15.199627 [INF]   Name:     NDREZQBDOGGJJFX24Z5JMY3J6PC2QKF5PCP64ZTSPQHXEX667D3WVSA3
[3663003] 2021/04/24 12:10:15.199631 [INF]   ID:       NDREZQBDOGGJJFX24Z5JMY3J6PC2QKF5PCP64ZTSPQHXEX667D3WVSA3
[3663003] 2021/04/24 12:10:15.199640 [INF] Using configuration file: nas.config
[3663003] 2021/04/24 12:10:15.199645 [INF] Trusted Operators
[3663003] 2021/04/24 12:10:15.199651 [INF]   System  : ""
[3663003] 2021/04/24 12:10:15.199656 [INF]   Operator: "O_operator"
[3663003] 2021/04/24 12:10:15.199677 [INF]   Issued  : 2021-04-24 11:41:08 +0200 CEST
[3663003] 2021/04/24 12:10:15.199683 [INF]   Expires : 1970-01-01 01:00:00 +0100 CET
[3663003] 2021/04/24 12:10:15.199690 [WRN] Trusted Operators should utilize a System Account
[3663003] 2021/04/24 12:10:15.199761 [INF] Server is ready
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0xa0 pc=0x7e467a]

goroutine 1 [running]:
github.com/nats-io/nats-server/server.(*Server).newRespInbox(0xc0001b2000, 0xc000144150, 0x38)
	/home/travis/gopath/src/github.com/nats-io/nats-server/server/events.go:1705 +0x3a
github.com/nats-io/nats-server/server.(*DirAccResolver).Start(0xc00007eb60, 0xc0001b2000, 0x0, 0x0)
	/home/travis/gopath/src/github.com/nats-io/nats-server/server/accounts.go:3634 +0x1d9
github.com/nats-io/nats-server/server.(*Server).Start(0xc0001b2000)
	/home/travis/gopath/src/github.com/nats-io/nats-server/server/server.go:1524 +0xf78
github.com/nats-io/nats-server/server.Run(...)
	/home/travis/gopath/src/github.com/nats-io/nats-server/server/service.go:21
main.main()
	/home/travis/gopath/src/github.com/nats-io/nats-server/main.go:116 +0x188
```


Pull request description:
[fixed] crash when using nats-resolver without system account
Fixes #2160 
Will raise an error instead

Signed-off-by: Matthias Hanel <mh@synadia.com>

Also noticed that s.Shutdown relied on s being not nil.

Patch:
diff --git a/server/jwt.go b/server/jwt.go
index dda182a9..a41f79d7 100644
--- a/server/jwt.go
+++ b/server/jwt.go
@@ -96,11 +96,11 @@ func validateTrustedOperators(o *Options) error {
 	if len(o.TrustedOperators) > 0 && len(o.TrustedKeys) > 0 {
 		return fmt.Errorf("conflicting options for 'TrustedKeys' and 'TrustedOperators'")
 	}
-	if o.SystemAccount != "" {
+	if o.SystemAccount != _EMPTY_ {
 		foundSys := false
 		foundNonEmpty := false
 		for _, op := range o.TrustedOperators {
-			if op.SystemAccount != "" {
+			if op.SystemAccount != _EMPTY_ {
 				foundNonEmpty = true
 			}
 			if op.SystemAccount == o.SystemAccount {
@@ -111,6 +111,13 @@ func validateTrustedOperators(o *Options) error {
 		if foundNonEmpty && !foundSys {
 			return fmt.Errorf("system_account in config and operator JWT must be identical")
 		}
+	} else if o.TrustedOperators[0].SystemAccount == _EMPTY_ {
+		// In case the system account is neither defined in config nor in the first operator.
+		// If it would be needed due to the nats account resolver, raise an error.
+		switch o.AccountResolver.(type) {
+		case *DirAccResolver, *CacheDirAccResolver:
+			return fmt.Errorf("using nats based account resolver - the system account needs to be specified in configuration or the operator jwt")
+		}
 	}
 	ver := strings.Split(strings.Split(VERSION, "-")[0], ".RC")[0]
 	srvMajor, srvMinor, srvUpdate, _ := jwt.ParseServerVersion(ver)
diff --git a/server/leafnode.go b/server/leafnode.go
index 2a697ee1..3f253372 100644
--- a/server/leafnode.go
+++ b/server/leafnode.go
@@ -21,6 +21,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
+	"math/rand"
 	"net"
 	"net/http"
 	"net/url"
@@ -133,6 +134,17 @@ func validateLeafNode(o *Options) error {
 	if err := validateLeafNodeAuthOptions(o); err != nil {
 		return err
 	}
+
+	for _, rem := range o.LeafNode.Remotes {
+		if rem.NoRandomize {
+			continue
+		}
+
+		rand.Shuffle(len(rem.URLs), func(i, j int) {
+			rem.URLs[i], rem.URLs[j] = rem.URLs[j], rem.URLs[i]
+		})
+	}
+
 	// In local config mode, check that leafnode configuration refers to accounts that exist.
 	if len(o.TrustedOperators) == 0 {
 		accNames := map[string]struct{}{}
diff --git a/server/monitor.go b/server/monitor.go
index a077dfee..936e2a66 100644
--- a/server/monitor.go
+++ b/server/monitor.go
@@ -1101,11 +1101,18 @@ type LeafNodeOptsVarz struct {
 	Remotes     []RemoteLeafOptsVarz `json:"remotes,omitempty"`
 }
 
+// Contains lists of subjects not allowed to be imported/exported
+type DenyRules struct {
+	Exports []string `json:"exports,omitempty"`
+	Imports []string `json:"imports,omitempty"`
+}
+
 // RemoteLeafOptsVarz contains monitoring remote leaf node information
 type RemoteLeafOptsVarz struct {
-	LocalAccount string   `json:"local_account,omitempty"`
-	TLSTimeout   float64  `json:"tls_timeout,omitempty"`
-	URLs         []string `json:"urls,omitempty"`
+	LocalAccount string     `json:"local_account,omitempty"`
+	TLSTimeout   float64    `json:"tls_timeout,omitempty"`
+	URLs         []string   `json:"urls,omitempty"`
+	Deny         *DenyRules `json:"deny,omitempty"`
 }
 
 // VarzOptions are the options passed to Varz().
@@ -1278,10 +1285,18 @@ func (s *Server) createVarz(pcpu float64, rss int64) *Varz {
 	if l := len(ln.Remotes); l > 0 {
 		rlna := make([]RemoteLeafOptsVarz, l)
 		for i, r := range ln.Remotes {
+			var deny *DenyRules
+			if len(r.DenyImports) > 0 || len(r.DenyExports) > 0 {
+				deny = &DenyRules{
+					Imports: r.DenyImports,
+					Exports: r.DenyExports,
+				}
+			}
 			rlna[i] = RemoteLeafOptsVarz{
 				LocalAccount: r.LocalAccount,
 				URLs:         urlsToStrings(r.URLs),
 				TLSTimeout:   r.TLSTimeout,
+				Deny:         deny,
 			}
 		}
 		varz.LeafNode.Remotes = rlna
diff --git a/server/opts.go b/server/opts.go
index d7ebfdc3..cc5c08ac 100644
--- a/server/opts.go
+++ b/server/opts.go
@@ -139,6 +139,7 @@ type LeafNodeOpts struct {
 // RemoteLeafOpts are options for connecting to a remote server as a leaf node.
 type RemoteLeafOpts struct {
 	LocalAccount string      `json:"local_account,omitempty"`
+	NoRandomize  bool        `json:"-"`
 	URLs         []*url.URL  `json:"urls,omitempty"`
 	Credentials  string      `json:"-"`
 	TLS          bool        `json:"-"`
@@ -1761,6 +1762,8 @@ func parseRemoteLeafNodes(v interface{}, errors *[]error, warnings *[]error) ([]
 		for k, v := range rm {
 			tk, v = unwrapValue(v, &lt)
 			switch strings.ToLower(k) {
+			case "no_randomize", "dont_randomize":
+				remote.NoRandomize = v.(bool)
 			case "url", "urls":
 				switch v := v.(type) {
 				case []interface{}, []string:
diff --git a/server/server.go b/server/server.go
index d6a4ec44..a9612e76 100644
--- a/server/server.go
+++ b/server/server.go
@@ -1664,6 +1664,9 @@ func (s *Server) Start() {
 // Shutdown will shutdown the server instance by kicking out the AcceptLoop
 // and closing all associated clients.
 func (s *Server) Shutdown() {
+	if s == nil {
+		return
+	}
 	// Transfer off any raft nodes that we are a leader by shutting them all down.
 	s.shutdownRaftNodes()
 


Test patch:
diff --git a/server/jwt_test.go b/server/jwt_test.go
index e1940886..27e7cb61 100644
--- a/server/jwt_test.go
+++ b/server/jwt_test.go
@@ -5739,3 +5739,25 @@ func TestJWTMappings(t *testing.T) {
 	require_Len(t, 1, updateJwt(t, srv.ClientURL(), sysCreds, aJwtNoM, 1))
 	test("foo2", "bar2", true)
 }
+
+func TestJWTNoSystemAccountButNatsResolver(t *testing.T) {
+	dirSrv := createDir(t, "srv")
+	defer removeDir(t, dirSrv)
+	for _, resType := range []string{"full", "cache"} {
+		t.Run(resType, func(t *testing.T) {
+			conf := createConfFile(t, []byte(fmt.Sprintf(`
+			listen: -1
+			operator: %s
+			resolver: {
+				type: %s
+				dir: %s
+			}`, ojwt, resType, dirSrv)))
+			defer removeFile(t, conf)
+			opts := LoadConfig(conf)
+			s, err := NewServer(opts)
+			s.Shutdown()
+			require_Error(t, err)
+			require_Contains(t, err.Error(), "the system account needs to be specified in configuration or the operator jwt")
+		})
+	}
+}
diff --git a/server/leafnode_test.go b/server/leafnode_test.go
index e4a32e6d..d4559226 100644
--- a/server/leafnode_test.go
+++ b/server/leafnode_test.go
@@ -28,6 +28,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/nats-io/nkeys"
+
 	jwt "github.com/nats-io/jwt/v2"
 	"github.com/nats-io/nats.go"
 )
@@ -85,6 +87,59 @@ func TestLeafNodeRandomIP(t *testing.T) {
 	}
 }
 
+func TestLeafNodeRandomRemotes(t *testing.T) {
+	// 16! possible permutations.
+	orderedURLs := make([]*url.URL, 0, 16)
+	for i := 0; i < cap(orderedURLs); i++ {
+		orderedURLs = append(orderedURLs, &url.URL{
+			Scheme: "nats-leaf",
+			Host:   fmt.Sprintf("host%d:7422", i),
+		})
+	}
+
+	o := DefaultOptions()
+	o.LeafNode.Remotes = []*RemoteLeafOpts{
+		{NoRandomize: true},
+		{NoRandomize: false},
+	}
+	o.LeafNode.Remotes[0].URLs = make([]*url.URL, cap(orderedURLs))
+	copy(o.LeafNode.Remotes[0].URLs, orderedURLs)
+	o.LeafNode.Remotes[1].URLs = make([]*url.URL, cap(orderedURLs))
+	copy(o.LeafNode.Remotes[1].URLs, orderedURLs)
+
+	s := RunServer(o)
+	s.Shutdown()
+
+	gotOrdered := o.LeafNode.Remotes[0].URLs
+	if got, want := len(gotOrdered), len(orderedURLs); got != want {
+		t.Fatalf("Unexpected rem0 len URLs, got %d, want %d", got, want)
+	}
+
+	// These should be IN order.
+	for i := range orderedURLs {
+		if got, want := gotOrdered[i].String(), orderedURLs[i].String(); got != want {
+			t.Fatalf("Unexpected ordered url, got %s, want %s", got, want)
+		}
+	}
+
+	gotRandom := o.LeafNode.Remotes[1].URLs
+	if got, want := len(gotRandom), len(orderedURLs); got != want {
+		t.Fatalf("Unexpected rem1 len URLs, got %d, want %d", got, want)
+	}
+
+	// These should be OUT of order.
+	var random bool
+	for i := range orderedURLs {
+		if gotRandom[i].String() != orderedURLs[i].String() {
+			random = true
+			break
+		}
+	}
+	if !random {
+		t.Fatal("Expected urls to be random")
+	}
+}
+
 type testLoopbackResolver struct{}
 
 func (r *testLoopbackResolver) LookupHost(ctx context.Context, host string) ([]string, error) {
@@ -2402,6 +2457,10 @@ func TestLeafNodeRouteParseLSUnsub(t *testing.T) {
 }
 
 func TestLeafNodeOperatorBadCfg(t *testing.T) {
+	sysAcc, err := nkeys.CreateAccount()
+	require_NoError(t, err)
+	sysAccPk, err := sysAcc.PublicKey()
+	require_NoError(t, err)
 	tmpDir := createDir(t, "_nats-server")
 	defer removeDir(t, tmpDir)
 	for errorText, cfg := range map[string]string{
@@ -2421,6 +2480,7 @@ func TestLeafNodeOperatorBadCfg(t *testing.T) {
 			conf := createConfFile(t, []byte(fmt.Sprintf(`
 		port: -1
 		operator: %s
+		system_account: %s
 		resolver: {
 			type: cache
 			dir: %s
@@ -2428,7 +2488,7 @@ func TestLeafNodeOperatorBadCfg(t *testing.T) {
 		leafnodes: {
 			%s
 		}
-	`, ojwt, tmpDir, cfg)))
+	`, ojwt, sysAccPk, tmpDir, cfg)))
 			defer removeFile(t, conf)
 			opts := LoadConfig(conf)
 			s, err := NewServer(opts)
@@ -3226,16 +3286,15 @@ func TestLeafNodeLoopDetectionWithMultipleClusters(t *testing.T) {
 
 	checkClusterFormed(t, l1, l2)
 
-	u1, _ := url.Parse(fmt.Sprintf("nats://127.0.0.1:%d", lo1.LeafNode.Port))
-	u2, _ := url.Parse(fmt.Sprintf("nats://127.0.0.1:%d", lo2.LeafNode.Port))
-	urls := []*url.URL{u1, u2}
-
 	ro1 := DefaultOptions()
 	ro1.Cluster.Name = "remote"
 	ro1.Cluster.Host = "127.0.0.1"
 	ro1.Cluster.Port = -1
 	ro1.LeafNode.ReconnectInterval = 50 * time.Millisecond
-	ro1.LeafNode.Remotes = []*RemoteLeafOpts{{URLs: urls}}
+	ro1.LeafNode.Remotes = []*RemoteLeafOpts{{URLs: []*url.URL{
+		{Scheme: "nats", Host: fmt.Sprintf("127.0.0.1:%d", lo1.LeafNode.Port)},
+		{Scheme: "nats", Host: fmt.Sprintf("127.0.0.1:%d", lo2.LeafNode.Port)},
+	}}}
 	r1 := RunServer(ro1)
 	defer r1.Shutdown()
 
@@ -3248,7 +3307,10 @@ func TestLeafNodeLoopDetectionWithMultipleClusters(t *testing.T) {
 	ro2.Cluster.Port = -1
 	ro2.Routes = RoutesFromStr(fmt.Sprintf("nats://127.0.0.1:%d", ro1.Cluster.Port))
 	ro2.LeafNode.ReconnectInterval = 50 * time.Millisecond
-	ro2.LeafNode.Remotes = []*RemoteLeafOpts{{URLs: urls}}
+	ro2.LeafNode.Remotes = []*RemoteLeafOpts{{URLs: []*url.URL{
+		{Scheme: "nats", Host: fmt.Sprintf("127.0.0.1:%d", lo1.LeafNode.Port)},
+		{Scheme: "nats", Host: fmt.Sprintf("127.0.0.1:%d", lo2.LeafNode.Port)},
+	}}}
 	r2 := RunServer(ro2)
 	defer r2.Shutdown()
 
diff --git a/server/monitor_test.go b/server/monitor_test.go
index 756c3242..6a321db5 100644
--- a/server/monitor_test.go
+++ b/server/monitor_test.go
@@ -2821,7 +2821,7 @@ func TestMonitorLeafNode(t *testing.T) {
 		opts.LeafNode.TLSConfig != nil,
 		[]RemoteLeafOptsVarz{
 			{
-				"acc", 1, []string{"localhost:1234"},
+				"acc", 1, []string{"localhost:1234"}, nil,
 			},
 		},
 	}
@@ -2840,7 +2840,7 @@ func TestMonitorLeafNode(t *testing.T) {
 
 		// Having this here to make sure that if fields are added in ClusterOptsVarz,
 		// we make sure to update this test (compiler will report an error if we don't)
-		_ = LeafNodeOptsVarz{"", 0, 0, 0, false, false, []RemoteLeafOptsVarz{{"", 0, nil}}}
+		_ = LeafNodeOptsVarz{"", 0, 0, 0, false, false, []RemoteLeafOptsVarz{{"", 0, nil, nil}}}
 
 		// Alter the fields to make sure that we have a proper deep copy
 		// of what may be stored in the server. Anything we change here
diff --git a/server/opts_test.go b/server/opts_test.go
index 8d27d65f..0c75b8e4 100644
--- a/server/opts_test.go
+++ b/server/opts_test.go
@@ -16,6 +16,7 @@ package server
 import (
 	"bytes"
 	"crypto/tls"
+	"encoding/json"
 	"flag"
 	"fmt"
 	"io/ioutil"
@@ -2383,6 +2384,66 @@ func TestParsingLeafNodeRemotes(t *testing.T) {
 			t.Fatalf("Expected %v, got %v", expected, opts.LeafNode.Remotes[0])
 		}
 	})
+
+	t.Run("url ordering", func(t *testing.T) {
+		// 16! possible permutations.
+		orderedURLs := make([]string, 0, 16)
+		for i := 0; i < cap(orderedURLs); i++ {
+			orderedURLs = append(orderedURLs, fmt.Sprintf("nats-leaf://host%d:7422", i))
+		}
+		confURLs, err := json.Marshal(orderedURLs)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		content := `
+		leafnodes {
+			remotes = [
+				{
+					dont_randomize: true
+					urls: %[1]s
+				}
+				{
+					urls: %[1]s
+				}
+			]
+		}
+		`
+		conf := createConfFile(t, []byte(fmt.Sprintf(content, confURLs)))
+		defer removeFile(t, conf)
+
+		s, o := RunServerWithConfig(conf)
+		s.Shutdown()
+
+		gotOrdered := o.LeafNode.Remotes[0].URLs
+		if got, want := len(gotOrdered), len(orderedURLs); got != want {
+			t.Fatalf("Unexpected rem0 len URLs, got %d, want %d", got, want)
+		}
+
+		// These should be IN order.
+		for i := range orderedURLs {
+			if got, want := gotOrdered[i].String(), orderedURLs[i]; got != want {
+				t.Fatalf("Unexpected ordered url, got %s, want %s", got, want)
+			}
+		}
+
+		gotRandom := o.LeafNode.Remotes[1].URLs
+		if got, want := len(gotRandom), len(orderedURLs); got != want {
+			t.Fatalf("Unexpected rem1 len URLs, got %d, want %d", got, want)
+		}
+
+		// These should be OUT of order.
+		var random bool
+		for i := range orderedURLs {
+			if gotRandom[i].String() != orderedURLs[i] {
+				random = true
+				break
+			}
+		}
+		if !random {
+			t.Fatal("Expected urls to be random")
+		}
+	})
 }
 
 func TestLargeMaxControlLine(t *testing.T) {