Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix bad sys request for different account #3382

Merged
merged 1 commit into from
Aug 20, 2022
Merged

Fix bad sys request for different account #3382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 17 additions & 1 deletion server/accounts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1440,6 +1440,12 @@ func (a *Account) lowestServiceExportResponseTime() time.Duration {

// AddServiceImportWithClaim will add in the service import via the jwt claim.
func (a *Account) AddServiceImportWithClaim(destination *Account, from, to string, imClaim *jwt.Import) error {
return a.addServiceImportWithClaim(destination, from, to, imClaim, false)
}

// addServiceImportWithClaim will add in the service import via the jwt claim.
// It will also skip the authorization check in cases where internal is true
func (a *Account) addServiceImportWithClaim(destination *Account, from, to string, imClaim *jwt.Import, internal bool) error {
if destination == nil {
return ErrMissingAccount
}
Expand All @@ -1452,7 +1458,7 @@ func (a *Account) AddServiceImportWithClaim(destination *Account, from, to strin
}

// First check to see if the account has authorized us to route to the "to" subject.
if !destination.checkServiceImportAuthorized(a, to, imClaim) {
if !internal && !destination.checkServiceImportAuthorized(a, to, imClaim) {
return ErrServiceImportAuthorization
}

Expand Down Expand Up @@ -1502,6 +1508,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 {
Expand Down
40 changes: 22 additions & 18 deletions server/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -893,15 +893,7 @@ func (s *Server) initEventTracking() {
if tk := strings.Split(subject, tsep); len(tk) != accReqTokens {
return _EMPTY_, fmt.Errorf("subject %q is malformed", subject)
} else {
acc := tk[accReqAccIndex]
if ci, _, _, _, err := c.srv.getRequestInfo(c, msg); err == nil && ci.Account != _EMPTY_ {
// Make sure the accounts match.
if ci.Account != acc {
// Do not leak too much here.
return _EMPTY_, fmt.Errorf("bad request")
}
}
return acc, nil
return tk[accReqAccIndex], nil
}
}
monAccSrvc := map[string]msgHandler{
Expand Down Expand Up @@ -1054,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()
Expand Down Expand Up @@ -1639,7 +1643,7 @@ func (s *Server) registerSystemImports(a *Account) {

importSrvc := func(subj, mappedSubj string) {
if !a.serviceImportExists(subj) {
if err := a.AddServiceImport(sacc, subj, mappedSubj); err != nil {
if err := a.addServiceImportWithClaim(sacc, subj, mappedSubj, nil, true); err != nil {
s.Errorf("Error setting up system service import %s -> %s for account: %v",
subj, mappedSubj, err)
}
Expand Down
213 changes: 213 additions & 0 deletions server/jwt_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4106,6 +4106,219 @@ func TestJWTTimeExpiration(t *testing.T) {
})
}

func NewJwtAccountClaim(name string) (nkeys.KeyPair, string, *jwt.AccountClaims) {
sysKp, _ := nkeys.CreateAccount()
sysPub, _ := sysKp.PublicKey()
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",
})
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: "$SYS.REQ.ACCOUNT.*.INFO",
LocalSubject: "COMMON.ADVISORY.SYS.REQ.ACCOUNT.*.INFO",
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))
matthiashanel marked this conversation as resolved.
Show resolved Hide resolved
defer nc.Close()
// 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)
require_NoError(t, err)
resp := &ServerAPIResponse{}
require_NoError(t, json.Unmarshal(m.Data, resp))
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))
defer nc.Close()
// user for account a requests for a different account, the system account
matthiashanel marked this conversation as resolved.
Show resolved Hide resolved
_, 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))
defer nc.Close()
// user for account a requests for a different account, the system account
matthiashanel marked this conversation as resolved.
Show resolved Hide resolved
_, 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 satisfy
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))
defer nc.Close()
// make sure the internal import still got added
_, err = nc.Request("$SYS.REQ.ACCOUNT.PING.CONNZ", nil, time.Second)
matthiashanel marked this conversation as resolved.
Show resolved Hide resolved
require_NoError(t, err)
}

func TestJWTLimits(t *testing.T) {
doNotExpire := time.Now().AddDate(1, 0, 0)
// create account
Expand Down