From 99921725a91ba24086af62dd97c256f52d755717 Mon Sep 17 00:00:00 2001 From: Matthias Hanel Date: Fri, 24 Jul 2020 13:01:49 -0400 Subject: [PATCH] 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 --- server/jwt_test.go | 69 +++++++++++++++++++++++++++++++++++++++++-- server/server.go | 53 +++++++++++++++++---------------- test/operator_test.go | 43 +++++++++++++++++---------- 3 files changed, 122 insertions(+), 43 deletions(-) diff --git a/server/jwt_test.go b/server/jwt_test.go index 0c7bdb64..69d0cb07 100644 --- 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() diff --git a/server/server.go b/server/server.go index 30212bc9..3acc46bc 100644 --- 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. diff --git a/test/operator_test.go b/test/operator_test.go index 2ee585c3..814bad62 100644 --- 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) }