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..25f9b5b3 100644 --- a/server/opts_test.go +++ b/server/opts_test.go @@ -3286,3 +3286,75 @@ func TestGetStorageSize(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 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", + ` + 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"), + }, + { + "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)) + 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()) + } + }) + } +}