Fix bad sys request for different account

[matthiashanel]

When a request for a system service like $SYS.REQ.ACCOUNT.*.CONNZ
is imported/exported we ensured that the requesting account is identical
to the account referenced in the subject.

In #3250 this check was extended from CONNZ to all $SYS.REQ.ACCOUNT..
requests.

In general this check interferes with monitoring accounts that need
to query all other accounts, not just itself.
There the use case is that account A sends a request with account B
in the subject. The check for equal accounts prevents this.

This change removes the check to support these use cases.

Instead of the check, the default export now uses exportAuth
tokenPos to ensure that the 4th token is the importer account id.
This guarantees that an explicit export (done by user) can only import
for the own account.

This change also ensures that an explicit export is not overwritten
by the system.
This is not a problem when the export is public.
Automatic imports set the account id correctly and do not use wildcards.

To cover cases where the export is private, automatically added imports
are not subject a token check.

[derekcollison]

Does this violate account isolation? Can any account simply import this from a system account and be able to monitor other accounts?

I am ok if the system account itself can inquire about other accounts, and that user credentials for the system account could be minted to just have access to this subject.

[matthiashanel]

@derekcollison I don't think this violates account isolation as the mandatory export/import is still needed.
There an admin can decide if the export/import is for a particular account (id in subject) or all (wildcard).
No account can unilaterally be imported from the system account.

In the example, a dedicated account to monitor was trying to access information of another account.
To do so $SYS.REQ.ACCOUNT.*.CONNZ was exported/imported.

I also tried this out by removing the export from the unit test.
Both test below only have the import. In both cases, the request has no responder:

> go test -v  --count 1 --vet=off --timeout 400m ./server   --run TestJWTSysImportForDifferentAccount  --failfast --race
=== RUN   TestJWTSysImportForDifferentAccount
    jwt_test.go:4150: require no error, but got: nats: no responders available for request
--- FAIL: TestJWTSysImportForDifferentAccount (0.06s)
FAIL
FAIL	github.com/nats-io/nats-server/v2/server	0.542s
FAIL
> git diff
diff --git a/server/jwt_test.go b/server/jwt_test.go
index f46a8d8b..08dad8ed 100644
--- a/server/jwt_test.go
+++ b/server/jwt_test.go
@@ -4110,10 +4110,10 @@ func TestJWTSysImportForDifferentAccount(t *testing.T) {
        sysKp, _ := nkeys.CreateAccount()
        sysPub, _ := sysKp.PublicKey()
        sysClaim := jwt.NewAccountClaims(sysPub)
-       sysClaim.Exports.Add(&jwt.Export{
+       /*sysClaim.Exports.Add(&jwt.Export{
                Type:    jwt.Service,
                Subject: "$SYS.REQ.ACCOUNT.*.INFO",
-       })
+       })*/
        sysJwt, err := sysClaim.Encode(oKp)
        require_NoError(t, err)

@@ -4124,7 +4124,7 @@ func TestJWTSysImportForDifferentAccount(t *testing.T) {
        claim.Imports.Add(&jwt.Import{
                Type:         jwt.Service,
                Subject:      "$SYS.REQ.ACCOUNT.*.INFO",
-               LocalSubject: "COMMON.ADVISORY.SYS.REQ.ACCOUNT.*.INFO",
+               LocalSubject: "$SYS.REQ.ACCOUNT.*.INFO",
                Account:      sysPub,
        })
        aJwt, err := claim.Encode(oKp)
@@ -4146,7 +4146,7 @@ func TestJWTSysImportForDifferentAccount(t *testing.T) {

        nc := natsConnect(t, sA.ClientURL(), createUserCreds(t, nil, aKp))
        // user for account a requests for a different account, the system account
-       m, err := nc.Request(fmt.Sprintf("COMMON.ADVISORY.SYS.REQ.ACCOUNT.%s.INFO", sysPub), nil, time.Second)
+       m, err := nc.Request(fmt.Sprintf("$SYS.REQ.ACCOUNT.%s.INFO", sysPub), nil, time.Second)
        require_NoError(t, err)
        resp := &ServerAPIResponse{}
        require_NoError(t, json.Unmarshal(m.Data, resp))
> git diff
diff --git a/server/jwt_test.go b/server/jwt_test.go
index f46a8d8b..879ff332 100644
--- a/server/jwt_test.go
+++ b/server/jwt_test.go
@@ -4110,10 +4110,10 @@ func TestJWTSysImportForDifferentAccount(t *testing.T) {
        sysKp, _ := nkeys.CreateAccount()
        sysPub, _ := sysKp.PublicKey()
        sysClaim := jwt.NewAccountClaims(sysPub)
-       sysClaim.Exports.Add(&jwt.Export{
+       /*sysClaim.Exports.Add(&jwt.Export{
                Type:    jwt.Service,
                Subject: "$SYS.REQ.ACCOUNT.*.INFO",
-       })
+       })*/
        sysJwt, err := sysClaim.Encode(oKp)
        require_NoError(t, err)

> go test -v  --count 1 --vet=off --timeout 400m ./server   --run TestJWTSysImportForDifferentAccount  --failfast --race
=== RUN   TestJWTSysImportForDifferentAccount
    jwt_test.go:4150: require no error, but got: nats: no responders available for request
--- FAIL: TestJWTSysImportForDifferentAccount (0.06s)
FAIL
FAIL	github.com/nats-io/nats-server/v2/server	0.537s
FAIL
>

[derekcollison]

ok so the system has to allow broader access to any given account, so our reasoning is this would be done on purpose to allow an account to monitor other accounts anyway.

[matthiashanel]

@derekcollison I have to rescind my earlier comment. With some more playing I was able to import without explicit export.
This happens because we add automatic exports for CONNZ and STATZ.

[derekcollison]

LGTM

[derekcollison]

Oh ok.. Then maybe we should discuss. My LGTM was before that last comment.

[derekcollison]

Let's look a bit more.

[kozlovic]

Remember to call defer nc.Close() after creating a client connection.

[derekcollison]

LGTM

[kozlovic]

LGTM (at least the tests missing defers). Defer (pun intended) to Derek for the actual code changes.