Ensuring that an untrusted account is not held in memory

The check that an account has to be signed by a configured operator is done after fetch as well. As a consequence an account claim will never become an Account in memory. The original check during client or leaf authentication is left in place. Adding unit tests. Modifying existing tests to not rely on an account but it's name instead. Signed-off-by: Matthias Hanel <mh@synadia.com>
2026-04-02 03:38:42 -07:00 · 2020-07-24 13:01:49 -04:00
parent 00eb20b293
commit 99921725a9
3 changed files with 122 additions and 43 deletions
--- a/server/jwt_test.go
+++ b/server/jwt_test.go
@@ -37,6 +37,8 @@ import (
 var (
 	// This matches ./configs/nkeys_jwts/test.seed
 	oSeed = []byte("SOAFYNORQLQFJYBYNUGC5D7SH2MXMUX5BFEWWGHN3EK4VGG5TPT5DZP7QU")
+	// This matches ./configs/nkeys/op.jwt
+	ojwt = "eyJ0eXAiOiJqd3QiLCJhbGciOiJlZDI1NTE5In0.eyJhdWQiOiJURVNUUyIsImV4cCI6MTg1OTEyMTI3NSwianRpIjoiWE5MWjZYWVBIVE1ESlFSTlFPSFVPSlFHV0NVN01JNVc1SlhDWk5YQllVS0VRVzY3STI1USIsImlhdCI6MTU0Mzc2MTI3NSwiaXNzIjoiT0NBVDMzTVRWVTJWVU9JTUdOR1VOWEo2NkFIMlJMU0RBRjNNVUJDWUFZNVFNSUw2NU5RTTZYUUciLCJuYW1lIjoiU3luYWRpYSBDb21tdW5pY2F0aW9ucyBJbmMuIiwibmJmIjoxNTQzNzYxMjc1LCJzdWIiOiJPQ0FUMzNNVFZVMlZVT0lNR05HVU5YSjY2QUgyUkxTREFGM01VQkNZQVk1UU1JTDY1TlFNNlhRRyIsInR5cGUiOiJvcGVyYXRvciIsIm5hdHMiOnsic2lnbmluZ19rZXlzIjpbIk9EU0tSN01ZRlFaNU1NQUo2RlBNRUVUQ1RFM1JJSE9GTFRZUEpSTUFWVk40T0xWMllZQU1IQ0FDIiwiT0RTS0FDU1JCV1A1MzdEWkRSVko2NTdKT0lHT1BPUTZLRzdUNEhONk9LNEY2SUVDR1hEQUhOUDIiLCJPRFNLSTM2TFpCNDRPWTVJVkNSNlA1MkZaSlpZTVlXWlZXTlVEVExFWjVUSzJQTjNPRU1SVEFCUiJdfX0.hyfz6E39BMUh0GLzovFfk3wT4OfualftjdJ_eYkLfPvu5tZubYQ_Pn9oFYGCV_6yKy3KMGhWGUCyCdHaPhalBw"
 )

 func opTrustBasicSetup() *Server {
@@ -1558,13 +1560,14 @@ func TestAccountURLResolver(t *testing.T) {
 			defer ts.Close()

 			confTemplate := `
+				operator: %s
 				listen: -1
 				resolver: URL("%s/ngs/v1/accounts/jwt/")
 				resolver_tls {
 					insecure: true
 				}
 			`
-			conf := createConfFile(t, []byte(fmt.Sprintf(confTemplate, ts.URL)))
+			conf := createConfFile(t, []byte(fmt.Sprintf(confTemplate, ojwt, ts.URL)))
 			defer os.Remove(conf)

 			s, opts := RunServerWithConfig(conf)
@@ -1643,10 +1646,11 @@ func TestAccountURLResolverNoFetchOnReload(t *testing.T) {
 	defer ts.Close()

 	confTemplate := `
+		operator: %s
 		listen: -1
 		resolver: URL("%s/ngs/v1/accounts/jwt/")
    `
-	conf := createConfFile(t, []byte(fmt.Sprintf(confTemplate, ts.URL)))
+	conf := createConfFile(t, []byte(fmt.Sprintf(confTemplate, ojwt, ts.URL)))
 	defer os.Remove(conf)

 	s, _ := RunServerWithConfig(conf)
@@ -1668,7 +1672,7 @@ func TestAccountURLResolverNoFetchOnReload(t *testing.T) {
 	}))
 	defer ts.Close()

-	changeCurrentConfigContentWithNewContent(t, conf, []byte(fmt.Sprintf(confTemplate, ts.URL)))
+	changeCurrentConfigContentWithNewContent(t, conf, []byte(fmt.Sprintf(confTemplate, ojwt, ts.URL)))

 	if err := s.Reload(); err != nil {
 		t.Fatalf("Error on reload: %v", err)
@@ -1848,6 +1852,65 @@ func TestAccountURLResolverFetchFailureInCluster(t *testing.T) {
 	}
 }

+func TestAccountURLResolverReturnDifferentOperator(t *testing.T) {
+	// Create a valid chain of op/acc/usr using a different operator
+	// This is so we can test if the server rejects this chain.
+	// Create Operator
+	op, _ := nkeys.CreateOperator()
+	// Create Account, this account is the one returned by the resolver
+	akp, _ := nkeys.CreateAccount()
+	apub, _ := akp.PublicKey()
+	nac := jwt.NewAccountClaims(apub)
+	ajwt, err := nac.Encode(op)
+	if err != nil {
+		t.Fatalf("Error generating account JWT: %v", err)
+	}
+	// Create User
+	nkp, _ := nkeys.CreateUser()
+	uSeed, _ := nkp.Seed()
+	upub, _ := nkp.PublicKey()
+	nuc := newJWTTestUserClaims()
+	nuc.Subject = upub
+	uJwt, err := nuc.Encode(akp)
+	if err != nil {
+		t.Fatalf("Error generating user JWT: %v", err)
+	}
+	creds := genCredsFile(t, uJwt, uSeed)
+	defer os.Remove(creds)
+	// Simulate an account server that was hijacked/mis configured
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Write([]byte(ajwt))
+	}))
+	defer ts.Close()
+	// Create Server
+	confA := createConfFile(t, []byte(fmt.Sprintf(`
+		listen: -1
+		operator: %s
+		resolver: URL("%s/A/")
+    `, ojwt, ts.URL)))
+	defer os.Remove(confA)
+	sA, _ := RunServerWithConfig(confA)
+	defer sA.Shutdown()
+	// Create first client, directly connects to A
+	urlA := fmt.Sprintf("nats://%s:%d", sA.opts.Host, sA.opts.Port)
+	if _, err := nats.Connect(urlA, nats.UserCredentials(creds),
+		nats.DisconnectErrHandler(func(_ *nats.Conn, err error) {
+			if err != nil {
+				t.Fatal("error not expected in this test", err)
+			}
+		}),
+		nats.ErrorHandler(func(_ *nats.Conn, _ *nats.Subscription, err error) {
+			t.Fatal("error not expected in this test", err)
+		}),
+	); err == nil {
+		t.Fatal("Expected connect to fail")
+	}
+	// Test if the server has the account in memory. (shouldn't)
+	if v, ok := sA.accounts.Load(apub); ok {
+		t.Fatalf("Expected account to NOT be in memory: %v", v.(*Account))
+	}
+}
+
 func TestJWTUserSigningKey(t *testing.T) {
 	s := opTrustBasicSetup()
 	defer s.Shutdown()
--- a/server/server.go
+++ b/server/server.go
@@ -1235,6 +1235,9 @@ func (s *Server) verifyAccountClaims(claimJWT string) (*jwt.AccountClaims, strin
 	if vr.IsBlocking(true) {
 		return nil, _EMPTY_, ErrAccountValidation
 	}
+	if !s.isTrustedIssuer(accClaims.Issuer) {
+		return nil, _EMPTY_, ErrAccountValidation
+	}
 	return accClaims, claimJWT, nil
 }

@@ -1242,32 +1245,32 @@ func (s *Server) verifyAccountClaims(claimJWT string) (*jwt.AccountClaims, strin
 // Lock is NOT held upon entry.
 func (s *Server) fetchAccount(name string) (*Account, error) {
 	accClaims, claimJWT, err := s.fetchAccountClaims(name)
-	if accClaims != nil {
-		acc := s.buildInternalAccount(accClaims)
-		acc.claimJWT = claimJWT
-		// Due to possible race, if registerAccount() returns a non
-		// nil account, it means the same account was already
-		// registered and we should use this one.
-		if racc := s.registerAccount(acc); racc != nil {
-			// Update with the new claims in case they are new.
-			// Following call will ignore ErrAccountResolverSameClaims
-			// if claims are the same.
-			err = s.updateAccountWithClaimJWT(racc, claimJWT)
-			if err != nil && err != ErrAccountResolverSameClaims {
-				return nil, err
-			}
-			return racc, nil
-		}
-		// The sub imports may have been setup but will not have had their
-		// subscriptions properly setup. Do that here.
-		if len(acc.imports.services) > 0 && acc.ic == nil {
-			acc.ic = s.createInternalAccountClient()
-			acc.ic.acc = acc
-			acc.addAllServiceImportSubs()
-		}
-		return acc, nil
+	if accClaims == nil {
+		return nil, err
 	}
-	return nil, err
+	acc := s.buildInternalAccount(accClaims)
+	acc.claimJWT = claimJWT
+	// Due to possible race, if registerAccount() returns a non
+	// nil account, it means the same account was already
+	// registered and we should use this one.
+	if racc := s.registerAccount(acc); racc != nil {
+		// Update with the new claims in case they are new.
+		// Following call will ignore ErrAccountResolverSameClaims
+		// if claims are the same.
+		err = s.updateAccountWithClaimJWT(racc, claimJWT)
+		if err != nil && err != ErrAccountResolverSameClaims {
+			return nil, err
+		}
+		return racc, nil
+	}
+	// The sub imports may have been setup but will not have had their
+	// subscriptions properly setup. Do that here.
+	if len(acc.imports.services) > 0 && acc.ic == nil {
+		acc.ic = s.createInternalAccountClient()
+		acc.ic.acc = acc
+		acc.addAllServiceImportSubs()
+	}
+	return acc, nil
 }

 // Start up the server, this will block.
--- a/test/operator_test.go
+++ b/test/operator_test.go
@@ -155,7 +155,15 @@ func runOperatorServer(t *testing.T) (*server.Server, *server.Options) {
 	return RunServerWithConfig(testOpConfig)
 }

-func createAccountForOperatorKey(t *testing.T, s *server.Server, seed []byte) (*server.Account, nkeys.KeyPair) {
+func publicKeyFromKeyPair(t *testing.T, pair nkeys.KeyPair) (pkey string) {
+	var err error
+	if pkey, err = pair.PublicKey(); err != nil {
+		t.Fatalf("Expected no error %v", err)
+	}
+	return
+}
+
+func createAccountForOperatorKey(t *testing.T, s *server.Server, seed []byte) nkeys.KeyPair {
 	t.Helper()
 	okp, _ := nkeys.FromSeed(seed)
 	akp, _ := nkeys.CreateAccount()
@@ -165,18 +173,20 @@ func createAccountForOperatorKey(t *testing.T, s *server.Server, seed []byte) (*
 	if err := s.AccountResolver().Store(pub, jwt); err != nil {
 		t.Fatalf("Account Resolver returned an error: %v", err)
 	}
-	acc, err := s.LookupAccount(pub)
-	if err != nil {
+	return akp
+}
+
+func createAccount(t *testing.T, s *server.Server) (acc *server.Account, akp nkeys.KeyPair) {
+	t.Helper()
+	akp = createAccountForOperatorKey(t, s, oSeed)
+	if pub, err := akp.PublicKey(); err != nil {
+		t.Fatalf("Expected this to pass, got %v", err)
+	} else if acc, err = s.LookupAccount(pub); err != nil {
 		t.Fatalf("Error looking up account: %v", err)
 	}
 	return acc, akp
 }

-func createAccount(t *testing.T, s *server.Server) (*server.Account, nkeys.KeyPair) {
-	t.Helper()
-	return createAccountForOperatorKey(t, s, oSeed)
-}
-
 func createUserCreds(t *testing.T, s *server.Server, akp nkeys.KeyPair) nats.Option {
 	t.Helper()
 	opt, _ := createUserCredsOption(t, s, akp)
@@ -221,11 +231,15 @@ func TestOperatorServer(t *testing.T) {
 	// Now create an account from another operator, this should fail.
 	okp, _ := nkeys.CreateOperator()
 	seed, _ := okp.Seed()
-	_, akp = createAccountForOperatorKey(t, s, seed)
+	akp = createAccountForOperatorKey(t, s, seed)
 	_, err = nats.Connect(url, createUserCreds(t, s, akp))
 	if err == nil {
 		t.Fatalf("Expected error on connect")
 	}
+	// The account should not be in memory either
+	if v, err := s.LookupAccount(publicKeyFromKeyPair(t, akp)); err == nil {
+		t.Fatalf("Expected account to NOT be in memory: %v", v)
+	}
 }

 func TestOperatorSystemAccount(t *testing.T) {
@@ -235,15 +249,15 @@ func TestOperatorSystemAccount(t *testing.T) {
 	// Create an account from another operator, this should fail if used as a system account.
 	okp, _ := nkeys.CreateOperator()
 	seed, _ := okp.Seed()
-	acc, _ := createAccountForOperatorKey(t, s, seed)
-	if err := s.SetSystemAccount(acc.Name); err == nil {
+	akp := createAccountForOperatorKey(t, s, seed)
+	if err := s.SetSystemAccount(publicKeyFromKeyPair(t, akp)); err == nil {
 		t.Fatalf("Expected this to fail")
 	}
 	if acc := s.SystemAccount(); acc != nil {
 		t.Fatalf("Expected no account to be set for system account")
 	}

-	acc, _ = createAccount(t, s)
+	acc, _ := createAccount(t, s)
 	if err := s.SetSystemAccount(acc.Name); err != nil {
 		t.Fatalf("Expected this succeed, got %v", err)
 	}
@@ -257,10 +271,9 @@ func TestOperatorSigningKeys(t *testing.T) {
 	defer s.Shutdown()

 	// Create an account with a signing key, not the master key.
-	acc, akp := createAccountForOperatorKey(t, s, skSeed)
-
+	akp := createAccountForOperatorKey(t, s, skSeed)
 	// Make sure we can set system account.
-	if err := s.SetSystemAccount(acc.Name); err != nil {
+	if err := s.SetSystemAccount(publicKeyFromKeyPair(t, akp)); err != nil {
 		t.Fatalf("Expected this succeed, got %v", err)
 	}