review comments

Signed-off-by: Matthias Hanel <mh@synadia.com>
nats-io · Aug 19, 2022 · 4862e47 · 4862e47
1 parent fe8f880
commit 4862e47
Show file tree

Hide file tree

Showing 3 changed files with 198 additions and 13 deletions.
diff --git a/server/accounts.go b/server/accounts.go
@@ -1502,6 +1502,16 @@ func (a *Account) streamImportFormsCycle(dest *Account, to string) error {
 	return dest.checkStreamImportsForCycles(to, map[string]bool{a.Name: true})
 }
 
+// Lock should be held.
+func (a *Account) hasServiceExportMatching(to string) bool {
+	for subj := range a.exports.services {
+		if subjectIsSubsetMatch(to, subj) {
+			return true
+		}
+	}
+	return false
+}
+
 // Lock should be held.
 func (a *Account) hasStreamExportMatching(to string) bool {
 	for subj := range a.exports.streams {

diff --git a/server/events.go b/server/events.go
@@ -1046,20 +1046,32 @@ func (s *Server) addSystemAccountExports(sacc *Account) {
 		return
 	}
 	accConnzSubj := fmt.Sprintf(accDirectReqSubj, "*", "CONNZ")
-	if err := sacc.AddServiceExportWithResponse(accConnzSubj, Streamed, nil); err != nil {
-		s.Errorf("Error adding system service export for %q: %v", accConnzSubj, err)
+	// prioritize not automatically added exports
+	if !sacc.hasServiceExportMatching(accConnzSubj) {
+		// pick export type that clamps importing account id into subject
+		if err := sacc.addServiceExportWithResponseAndAccountPos(accConnzSubj, Streamed, nil, 4); err != nil {
+			//if err := sacc.AddServiceExportWithResponse(accConnzSubj, Streamed, nil); err != nil {
+			s.Errorf("Error adding system service export for %q: %v", accConnzSubj, err)
+		}
 	}
+	// prioritize not automatically added exports
 	accStatzSubj := fmt.Sprintf(accDirectReqSubj, "*", "STATZ")
-	if err := sacc.AddServiceExportWithResponse(accStatzSubj, Streamed, nil); err != nil {
-		s.Errorf("Error adding system service export for %q: %v", accStatzSubj, err)
+	if !sacc.hasServiceExportMatching(accStatzSubj) {
+		// pick export type that clamps importing account id into subject
+		if err := sacc.addServiceExportWithResponseAndAccountPos(accStatzSubj, Streamed, nil, 4); err != nil {
+			s.Errorf("Error adding system service export for %q: %v", accStatzSubj, err)
+		}
+	}
+	// FIXME(dlc) - Old experiment, Remove?
+	if !sacc.hasServiceExportMatching(accSubsSubj) {
+		if err := sacc.AddServiceExport(accSubsSubj, nil); err != nil {
+			s.Errorf("Error adding system service export for %q: %v", accSubsSubj, err)
+		}
 	}
+
 	// Register any accounts that existed prior.
 	s.registerSystemImportsForExisting()
 
-	// FIXME(dlc) - Old experiment, Remove?
-	if err := sacc.AddServiceExport(accSubsSubj, nil); err != nil {
-		s.Errorf("Error adding system service export for %q: %v", accSubsSubj, err)
-	}
 	// in case of a mixed mode setup, enable js exports anyway
 	if s.JetStreamEnabled() || !s.standAloneMode() {
 		s.checkJetStreamExports()

diff --git a/server/jwt_test.go b/server/jwt_test.go
@@ -4106,10 +4106,16 @@ func TestJWTTimeExpiration(t *testing.T) {
 	})
 }
 
-func TestJWTSysImportForDifferentAccount(t *testing.T) {
+func NewJwtAccountClaim(name string) (nkeys.KeyPair, string, *jwt.AccountClaims) {
 	sysKp, _ := nkeys.CreateAccount()
 	sysPub, _ := sysKp.PublicKey()
-	sysClaim := jwt.NewAccountClaims(sysPub)
+	claim := jwt.NewAccountClaims(sysPub)
+	claim.Name = name
+	return sysKp, sysPub, claim
+}
+
+func TestJWTSysImportForDifferentAccount(t *testing.T) {
+	_, sysPub, sysClaim := NewJwtAccountClaim("SYS")
 	sysClaim.Exports.Add(&jwt.Export{
 		Type:    jwt.Service,
 		Subject: "$SYS.REQ.ACCOUNT.*.INFO",
@@ -4118,9 +4124,7 @@ func TestJWTSysImportForDifferentAccount(t *testing.T) {
 	require_NoError(t, err)
 
 	// create account
-	aKp, _ := nkeys.CreateAccount()
-	aPub, _ := aKp.PublicKey()
-	claim := jwt.NewAccountClaims(aPub)
+	aKp, aPub, claim := NewJwtAccountClaim("A")
 	claim.Imports.Add(&jwt.Import{
 		Type:         jwt.Service,
 		Subject:      "$SYS.REQ.ACCOUNT.*.INFO",
@@ -4153,6 +4157,165 @@ func TestJWTSysImportForDifferentAccount(t *testing.T) {
 	require_True(t, resp.Error == nil)
 }
 
+func TestJWTSysImportFromNothing(t *testing.T) {
+	_, sysPub, sysClaim := NewJwtAccountClaim("SYS")
+	sysJwt, err := sysClaim.Encode(oKp)
+	require_NoError(t, err)
+
+	// create account
+	aKp, aPub, claim := NewJwtAccountClaim("A")
+	claim.Imports.Add(&jwt.Import{
+		Type: jwt.Service,
+		// fails as it's not for own account, but system account
+		Subject:      jwt.Subject(fmt.Sprintf("$SYS.REQ.ACCOUNT.%s.CONNZ", sysPub)),
+		LocalSubject: "fail1",
+		Account:      sysPub,
+	})
+	claim.Imports.Add(&jwt.Import{
+		Type: jwt.Service,
+		// fails as it's not for own account but all accounts
+		Subject:      "$SYS.REQ.ACCOUNT.*.CONNZ",
+		LocalSubject: "fail2.*",
+		Account:      sysPub,
+	})
+	claim.Imports.Add(&jwt.Import{
+		Type:         jwt.Service,
+		Subject:      jwt.Subject(fmt.Sprintf("$SYS.REQ.ACCOUNT.%s.CONNZ", aPub)),
+		LocalSubject: "pass",
+		Account:      sysPub,
+	})
+	aJwt, err := claim.Encode(oKp)
+	require_NoError(t, err)
+
+	conf := createConfFile(t, []byte(fmt.Sprintf(`
+		listen: 127.0.0.1:-1
+		operator: %s
+		system_account: %s
+		resolver: MEM
+		resolver_preload: {
+			%s: %s
+			%s: %s
+		}
+    `, ojwt, sysPub, sysPub, sysJwt, aPub, aJwt)))
+	defer removeFile(t, conf)
+	sA, _ := RunServerWithConfig(conf)
+	defer sA.Shutdown()
+
+	nc := natsConnect(t, sA.ClientURL(), createUserCreds(t, nil, aKp))
+	// user for account a requests for a different account, the system account
+	_, err = nc.Request("pass", nil, time.Second)
+	require_NoError(t, err)
+	// default import
+	_, err = nc.Request("$SYS.REQ.ACCOUNT.PING.CONNZ", nil, time.Second)
+	require_NoError(t, err)
+	_, err = nc.Request("fail1", nil, time.Second)
+	require_Error(t, err)
+	require_Contains(t, err.Error(), "no responders")
+	// fails even for own account, as the import itself is bad
+	_, err = nc.Request("fail2."+aPub, nil, time.Second)
+	require_Error(t, err)
+	require_Contains(t, err.Error(), "no responders")
+}
+
+func TestJWTSysImportOverwritePublic(t *testing.T) {
+	_, sysPub, sysClaim := NewJwtAccountClaim("SYS")
+	// this changes the export permissions to allow for requests for every account
+	sysClaim.Exports.Add(&jwt.Export{
+		Type:    jwt.Service,
+		Subject: "$SYS.REQ.ACCOUNT.*.>",
+	})
+	sysJwt, err := sysClaim.Encode(oKp)
+	require_NoError(t, err)
+
+	// create account
+	aKp, aPub, claim := NewJwtAccountClaim("A")
+	claim.Imports.Add(&jwt.Import{
+		Type:         jwt.Service,
+		Subject:      jwt.Subject(fmt.Sprintf("$SYS.REQ.ACCOUNT.%s.CONNZ", sysPub)),
+		LocalSubject: "pass1",
+		Account:      sysPub,
+	})
+	claim.Imports.Add(&jwt.Import{
+		Type:         jwt.Service,
+		Subject:      jwt.Subject(fmt.Sprintf("$SYS.REQ.ACCOUNT.%s.CONNZ", aPub)),
+		LocalSubject: "pass2",
+		Account:      sysPub,
+	})
+	claim.Imports.Add(&jwt.Import{
+		Type:         jwt.Service,
+		Subject:      "$SYS.REQ.ACCOUNT.*.CONNZ",
+		LocalSubject: "pass3.*",
+		Account:      sysPub,
+	})
+	aJwt, err := claim.Encode(oKp)
+	require_NoError(t, err)
+
+	conf := createConfFile(t, []byte(fmt.Sprintf(`
+		listen: 127.0.0.1:-1
+		operator: %s
+		system_account: %s
+		resolver: MEM
+		resolver_preload: {
+			%s: %s
+			%s: %s
+		}
+    `, ojwt, sysPub, sysPub, sysJwt, aPub, aJwt)))
+	defer removeFile(t, conf)
+	sA, _ := RunServerWithConfig(conf)
+	defer sA.Shutdown()
+
+	nc := natsConnect(t, sA.ClientURL(), createUserCreds(t, nil, aKp))
+	// user for account a requests for a different account, the system account
+	_, err = nc.Request("pass1", nil, time.Second)
+	require_NoError(t, err)
+	_, err = nc.Request("pass2", nil, time.Second)
+	require_NoError(t, err)
+	_, err = nc.Request("pass3."+sysPub, nil, time.Second)
+	require_NoError(t, err)
+	_, err = nc.Request("pass3."+aPub, nil, time.Second)
+	require_NoError(t, err)
+	_, err = nc.Request("pass3.PING", nil, time.Second)
+	require_NoError(t, err)
+}
+
+func TestJWTSysImportOverwriteToken(t *testing.T) {
+	_, sysPub, sysClaim := NewJwtAccountClaim("SYS")
+	// this changes the export permissions in a way that the internal imports can't be added any longer
+	sysClaim.Exports.Add(&jwt.Export{
+		Type:     jwt.Service,
+		Subject:  "$SYS.REQ.>",
+		TokenReq: true,
+	})
+
+	sysJwt, err := sysClaim.Encode(oKp)
+	require_NoError(t, err)
+
+	// create account
+	aKp, aPub, claim := NewJwtAccountClaim("A")
+	aJwt, err := claim.Encode(oKp)
+	require_NoError(t, err)
+
+	conf := createConfFile(t, []byte(fmt.Sprintf(`
+		listen: 127.0.0.1:-1
+		operator: %s
+		system_account: %s
+		resolver: MEM
+		resolver_preload: {
+			%s: %s
+			%s: %s
+		}
+    `, ojwt, sysPub, sysPub, sysJwt, aPub, aJwt)))
+	defer removeFile(t, conf)
+	sA, _ := RunServerWithConfig(conf)
+	defer sA.Shutdown()
+
+	nc := natsConnect(t, sA.ClientURL(), createUserCreds(t, nil, aKp))
+	_, err = nc.Request("$SYS.REQ.ACCOUNT.PING.CONNZ", nil, time.Second)
+	// This is expected to fail as the internal import could not
+	require_Error(t, err)
+	require_Contains(t, err.Error(), "no responders")
+}
+
 func TestJWTLimits(t *testing.T) {
 	doNotExpire := time.Now().AddDate(1, 0, 0)
 	// create account