From eef194c43ba0e54b640cf6905dc4e0ab9af7f905 Mon Sep 17 00:00:00 2001 From: Ivan Kozlovic Date: Mon, 21 Mar 2022 19:50:16 -0600 Subject: [PATCH 1/2] [CHANGED] Duplicates in authorization{} and accounts{} now detected If accounts{} block is specified, authorization{} should not have any user/password/token or users array defined. The reason is that users parsed in accounts{} are associated with their respective account but users parsed in authorization{} are associated with the global account. If the same user name is in both, and since internally the parsing of those 2 blocks is completely random (even if layed out in the config in a specific order), the outcome may be that a user is either associated with an account or the default global account. To minimize breaking changes, but still avoid this unexpected outcome, the server will now detect if there are duplicate users (or nkeys) inside authorization{} block itself, but also between this block and accounts{}. The check will also detect if accounts{} has any user/nkey, then the authorization{} block should not have any user/password/token, making this test similar to the check we had in authorization{} block itself. Resolves #2926 Signed-off-by: Ivan Kozlovic --- server/config_check_test.go | 87 +++++++++++++++++++++++++++++-------- server/opts.go | 75 +++++++++++++++++++++++++++----- server/opts_test.go | 33 ++++++++++++++ 3 files changed, 165 insertions(+), 30 deletions(-) diff --git a/server/config_check_test.go b/server/config_check_test.go index 45fc871a..edb808d4 100644 --- a/server/config_check_test.go +++ b/server/config_check_test.go @@ -917,13 +917,13 @@ func TestConfigCheck(t *testing.T) { name: "when user authorization config has both token and users", config: ` authorization = { - token = "s3cr3t" - users = [ - { - user = "foo" - pass = "bar" - } - ] + token = "s3cr3t" + users = [ + { + user = "foo" + pass = "bar" + } + ] } `, err: errors.New(`Can not have a token and a users array`), @@ -934,20 +934,63 @@ func TestConfigCheck(t *testing.T) { name: "when user authorization config has both token and user", config: ` authorization = { - user = "foo" - pass = "bar" - users = [ - { - user = "foo" - pass = "bar" - } - ] + user = "foo" + pass = "bar" + token = "baz" + } + `, + err: errors.New(`Cannot have a user/pass and token`), + errorLine: 2, + errorPos: 3, + }, + { + name: "when user authorization config has both user and users array", + config: ` + authorization = { + user = "user1" + pass = "pwd1" + users = [ + { + user = "user2" + pass = "pwd2" + } + ] } `, err: errors.New(`Can not have a single user/pass and a users array`), errorLine: 2, errorPos: 3, }, + { + name: "when user authorization has duplicate users", + config: ` + authorization = { + users = [ + {user: "user1", pass: "pwd"} + {user: "user2", pass: "pwd"} + {user: "user1", pass: "pwd"} + ] + } + `, + err: fmt.Errorf(`Duplicate user %q detected`, "user1"), + errorLine: 2, + errorPos: 3, + }, + { + name: "when user authorization has duplicate nkeys", + config: ` + authorization = { + users = [ + {nkey: UC6NLCN7AS34YOJVCYD4PJ3QB7QGLYG5B5IMBT25VW5K4TNUJODM7BOX } + {nkey: UBAAQWTW6CG2G6ANGNKB5U2B7HRWHSGMZEZX3AQSAJOQDAUGJD46LD2E } + {nkey: UC6NLCN7AS34YOJVCYD4PJ3QB7QGLYG5B5IMBT25VW5K4TNUJODM7BOX } + ] + } + `, + err: fmt.Errorf(`Duplicate nkey %q detected`, "UC6NLCN7AS34YOJVCYD4PJ3QB7QGLYG5B5IMBT25VW5K4TNUJODM7BOX"), + errorLine: 2, + errorPos: 3, + }, { name: "when user authorization config has users not as a list", config: ` @@ -1600,9 +1643,9 @@ func TestConfigCheckMultipleErrors(t *testing.T) { t.Errorf("Expected a %d warning, got: %d", expected, got) } got = len(cerr.Errors()) - expected = 7 - if got != 7 { - t.Errorf("Expected a %d errors, got: %d", expected, got) + // Could be 7 or 8 errors depending on internal ordering of the parsing. + if got != 7 && got != 8 { + t.Errorf("Expected 7 or 8 errors, got: %d", got) } errMsg := err.Error() @@ -1620,7 +1663,13 @@ func TestConfigCheckMultipleErrors(t *testing.T) { for _, msg := range errs { found := strings.Contains(errMsg, msg) if !found { - t.Errorf("Expected to find error %q", msg) + t.Fatalf("Expected to find error %q", msg) + } + } + if got == 8 { + extra := "./configs/multiple_errors.conf:54:5: Can not have a single user/pass and accounts" + if !strings.Contains(errMsg, extra) { + t.Fatalf("Expected to find error %q (%s)", extra, errMsg) } } } diff --git a/server/opts.go b/server/opts.go index e5e4289d..f1b5fbce 100644 --- a/server/opts.go +++ b/server/opts.go @@ -755,30 +755,55 @@ func (o *Options) processConfigFileLine(k string, v interface{}, errors *[]error o.Username = auth.user o.Password = auth.pass o.Authorization = auth.token - if (auth.user != "" || auth.pass != "") && auth.token != "" { + o.AuthTimeout = auth.timeout + if (auth.user != _EMPTY_ || auth.pass != _EMPTY_) && auth.token != _EMPTY_ { err := &configErr{tk, "Cannot have a user/pass and token"} *errors = append(*errors, err) return } - o.AuthTimeout = auth.timeout - // Check for multiple users defined - if auth.users != nil { - if auth.user != "" { + // In case parseAccounts() was done first, we need to check for duplicates. + unames := setupUsersAndNKeysDuplicateCheckMap(o) + // Check for multiple users defined. + // Note: auth.users will be != nil as long as `users: []` is present + // in the authorization block, even if empty, and will also account for + // nkey users. We also check for users/nkeys that may have been already + // added in parseAccounts() (which means they will be in unames) + if auth.users != nil || len(unames) > 0 { + if auth.user != _EMPTY_ { err := &configErr{tk, "Can not have a single user/pass and a users array"} *errors = append(*errors, err) return } - if auth.token != "" { + if auth.token != _EMPTY_ { err := &configErr{tk, "Can not have a token and a users array"} *errors = append(*errors, err) return } - // Users may have been added from Accounts parsing, so do an append here - o.Users = append(o.Users, auth.users...) + // Now check that if we have users, there is no duplicate, including + // users that may have been configured in parseAccounts(). + if len(auth.users) > 0 { + for _, u := range auth.users { + if _, ok := unames[u.Username]; ok { + err := &configErr{tk, fmt.Sprintf("Duplicate user %q detected", u.Username)} + *errors = append(*errors, err) + return + } + unames[u.Username] = struct{}{} + } + // Users may have been added from Accounts parsing, so do an append here + o.Users = append(o.Users, auth.users...) + } } - // Check for nkeys - if auth.nkeys != nil { + if len(auth.nkeys) > 0 { + for _, u := range auth.nkeys { + if _, ok := unames[u.Nkey]; ok { + err := &configErr{tk, fmt.Sprintf("Duplicate nkey %q detected", u.Nkey)} + *errors = append(*errors, err) + return + } + unames[u.Nkey] = struct{}{} + } // NKeys may have been added from Accounts parsing, so do an append here o.Nkeys = append(o.Nkeys, auth.nkeys...) } @@ -1303,6 +1328,17 @@ func (o *Options) processConfigFileLine(k string, v interface{}, errors *[]error } } +func setupUsersAndNKeysDuplicateCheckMap(o *Options) map[string]struct{} { + unames := make(map[string]struct{}, len(o.Users)+len(o.Nkeys)) + for _, u := range o.Users { + unames[u.Username] = struct{}{} + } + for _, u := range o.Nkeys { + unames[u.Nkey] = struct{}{} + } + return unames +} + func parseDuration(field string, tk token, v interface{}, errors *[]error, warnings *[]error) time.Duration { if wd, ok := v.(string); ok { if dur, err := time.ParseDuration(wd); err != nil { @@ -2449,7 +2485,10 @@ func parseAccounts(v interface{}, opts *Options, errors *[]error, warnings *[]er case map[string]interface{}: // Track users across accounts, must be unique across // accounts and nkeys vs users. - uorn := make(map[string]struct{}) + // We also want to check for users that may have been added in + // parseAuthorization{} if that happened first. + uorn := setupUsersAndNKeysDuplicateCheckMap(opts) + for aname, mv := range vv { tk, amv := unwrapValue(mv, <) @@ -2550,6 +2589,20 @@ func parseAccounts(v interface{}, opts *Options, errors *[]error, warnings *[]er } } } + // Report error if there is an authorization{} block + // with u/p or token and any user defined in accounts{} + if len(nkeyUsr) > 0 || len(users) > 0 { + if opts.Username != _EMPTY_ { + err := &configErr{usersTk, "Can not have a single user/pass and accounts"} + *errors = append(*errors, err) + continue + } + if opts.Authorization != _EMPTY_ { + err := &configErr{usersTk, "Can not have a token and accounts"} + *errors = append(*errors, err) + continue + } + } applyDefaultPermissions(users, nkeyUsr, acc.defaultPerms) for _, u := range nkeyUsr { if _, ok := uorn[u.Nkey]; ok { diff --git a/server/opts_test.go b/server/opts_test.go index f5c912f9..2ae8e88b 100644 --- a/server/opts_test.go +++ b/server/opts_test.go @@ -3286,3 +3286,36 @@ func TestGetStorageSize(t *testing.T) { } } + +func TestAuthorizationAndAccountsDuplicateUsers(t *testing.T) { + // There is a test called TestConfigCheck but we can't use it + // because the place where the error will be reported will depend + // if the duplicate is found while parsing authorization{} or + // accounts{}, which order is random. + for _, test := range []struct { + name string + config string + err string + }{ + {"duplicate users", + ` + authorization = {users = [ {user: "user1", pass: "pwd"} ] } + accounts { ACC { users = [ {user: "user1"} ] } } + `, + fmt.Sprintf("Duplicate user %q detected", "user1")}, + {"duplicate nkey", + ` + authorization = {users = [ {nkey: UC6NLCN7AS34YOJVCYD4PJ3QB7QGLYG5B5IMBT25VW5K4TNUJODM7BOX} ] } + accounts { ACC { users = [ {nkey: UC6NLCN7AS34YOJVCYD4PJ3QB7QGLYG5B5IMBT25VW5K4TNUJODM7BOX} ] } } + `, + fmt.Sprintf("Duplicate nkey %q detected", "UC6NLCN7AS34YOJVCYD4PJ3QB7QGLYG5B5IMBT25VW5K4TNUJODM7BOX")}, + } { + t.Run(test.name, func(t *testing.T) { + conf := createConfFile(t, []byte(test.config)) + defer removeFile(t, conf) + if _, err := ProcessConfigFile(conf); err == nil || !strings.Contains(err.Error(), test.err) { + t.Fatalf("Expected error %q, got %q", test.err, err.Error()) + } + }) + } +} From 897c229fa9adfb17364d0cd947849e73b4afcd25 Mon Sep 17 00:00:00 2001 From: Ivan Kozlovic Date: Tue, 22 Mar 2022 10:29:11 -0600 Subject: [PATCH 2/2] Update test to capture accounts{} and single u/p or token Signed-off-by: Ivan Kozlovic --- server/opts_test.go | 57 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 48 insertions(+), 9 deletions(-) diff --git a/server/opts_test.go b/server/opts_test.go index 2ae8e88b..25f9b5b3 100644 --- a/server/opts_test.go +++ b/server/opts_test.go @@ -3287,28 +3287,67 @@ func TestGetStorageSize(t *testing.T) { } -func TestAuthorizationAndAccountsDuplicateUsers(t *testing.T) { +func TestAuthorizationAndAccountsMisconfigurations(t *testing.T) { // There is a test called TestConfigCheck but we can't use it // because the place where the error will be reported will depend - // if the duplicate is found while parsing authorization{} or - // accounts{}, which order is random. + // if the error is found while parsing authorization{} or + // accounts{}, but we can't control the internal parsing of those + // (due to lexer giving back a map and iteration over map is random) + // regardless of the ordering in the configuration file. + // The test is also repeated for _, test := range []struct { name string config string err string }{ - {"duplicate users", + { + "duplicate users", ` authorization = {users = [ {user: "user1", pass: "pwd"} ] } accounts { ACC { users = [ {user: "user1"} ] } } - `, - fmt.Sprintf("Duplicate user %q detected", "user1")}, - {"duplicate nkey", + `, + fmt.Sprintf("Duplicate user %q detected", "user1"), + }, + { + "duplicate nkey", ` authorization = {users = [ {nkey: UC6NLCN7AS34YOJVCYD4PJ3QB7QGLYG5B5IMBT25VW5K4TNUJODM7BOX} ] } accounts { ACC { users = [ {nkey: UC6NLCN7AS34YOJVCYD4PJ3QB7QGLYG5B5IMBT25VW5K4TNUJODM7BOX} ] } } - `, - fmt.Sprintf("Duplicate nkey %q detected", "UC6NLCN7AS34YOJVCYD4PJ3QB7QGLYG5B5IMBT25VW5K4TNUJODM7BOX")}, + `, + fmt.Sprintf("Duplicate nkey %q detected", "UC6NLCN7AS34YOJVCYD4PJ3QB7QGLYG5B5IMBT25VW5K4TNUJODM7BOX"), + }, + { + "auth single user and password and accounts users", + ` + authorization = {user: "user1", password: "pwd"} + accounts = { ACC { users = [ {user: "user2", pass: "pwd"} ] } } + `, + "Can not have a single user/pass", + }, + { + "auth single user and password and accounts nkeys", + ` + authorization = {user: "user1", password: "pwd"} + accounts = { ACC { users = [ {nkey: UC6NLCN7AS34YOJVCYD4PJ3QB7QGLYG5B5IMBT25VW5K4TNUJODM7BOX} ] } } + `, + "Can not have a single user/pass", + }, + { + "auth token and accounts users", + ` + authorization = {token: "my_token"} + accounts = { ACC { users = [ {user: "user2", pass: "pwd"} ] } } + `, + "Can not have a token", + }, + { + "auth token and accounts nkeys", + ` + authorization = {token: "my_token"} + accounts = { ACC { users = [ {nkey: UC6NLCN7AS34YOJVCYD4PJ3QB7QGLYG5B5IMBT25VW5K4TNUJODM7BOX} ] } } + `, + "Can not have a token", + }, } { t.Run(test.name, func(t *testing.T) { conf := createConfFile(t, []byte(test.config))