diff --git a/cmd/authd-oidc/daemon/daemon.go b/cmd/authd-oidc/daemon/daemon.go index 42850374..100e596e 100644 --- a/cmd/authd-oidc/daemon/daemon.go +++ b/cmd/authd-oidc/daemon/daemon.go @@ -125,6 +125,12 @@ func (a *App) serve(config daemonConfig) error { } } + brokerConfigDir := broker.GetDropInDir(config.Paths.BrokerConf) + if err := ensureDirWithPerms(brokerConfigDir, 0700, os.Geteuid()); err != nil { + close(a.ready) + return fmt.Errorf("error initializing broker configuration directory %q: %v", brokerConfigDir, err) + } + b, err := broker.New(broker.Config{ ConfigFile: config.Paths.BrokerConf, DataDir: config.Paths.DataDir, diff --git a/conf/broker.conf b/conf/broker.conf index 889c43e1..6118d1f3 100644 --- a/conf/broker.conf +++ b/conf/broker.conf @@ -15,3 +15,31 @@ client_id = ## If configured, only users with a suffix in this list are allowed to ## log in via SSH. The suffixes must be separated by comma. #ssh_allowed_suffixes = @example.com,@anotherexample.com + +## 'allowed_users' specifies the users who are permitted to log in after +## successfully authenticating with the Identity Provider. +## Values are separated by commas. Supported values: +## - 'OWNER': Grants access to the user specified in the 'owner' option +## (see below). This is the default. +## - 'ALL': Grants access to all users who successfully authenticate +## with the Identity Provider. +## - : Grants access to specific additional users +## (e.g. user1@example.com). +## Example: allowed_users = OWNER,user1@example.com,admin@example.com +#allowed_users = OWNER + +## 'owner' specifies the user assigned the owner role. This user is +## permitted to log in if 'OWNER' is included in the 'allowed_users' +## option. +## +## If this option is left unset, the first user to successfully log in +## via this broker will automatically be assigned the owner role. A +## drop-in configuration file will be created in broker.conf.d/ to set +## the 'owner' option. +## +## To disable automatic assignment, you can either: +## 1. Explicitly set this option to an empty value (e.g. owner = "") +## 2. Remove 'OWNER' from the 'allowed_users' option +## +## Example: owner = user2@example.com +#owner = diff --git a/internal/broker/broker.go b/internal/broker/broker.go index 548b64ca..d6041398 100644 --- a/internal/broker/broker.go +++ b/internal/broker/broker.go @@ -45,14 +45,6 @@ type Config struct { userConfig } -type userConfig struct { - clientID string - clientSecret string - issuerURL string - homeBaseDir string - allowedSSHSuffixes []string -} - // Broker is the real implementation of the broker to track sessions and process oidc calls. type Broker struct { cfg Config @@ -106,15 +98,17 @@ type Option func(*option) func New(cfg Config, args ...Option) (b *Broker, err error) { defer decorate.OnError(&err, "could not create broker") + p := providers.CurrentProvider() + if cfg.ConfigFile != "" { - cfg.userConfig, err = parseConfigFile(cfg.ConfigFile) + cfg.userConfig, err = parseConfigFile(cfg.ConfigFile, p) if err != nil { return nil, fmt.Errorf("could not parse config: %v", err) } } opts := option{ - provider: providers.CurrentProvider(), + provider: p, } for _, arg := range args { arg(&opts) @@ -364,7 +358,6 @@ func (b *Broker) generateUILayout(session *session, authModeID string) (map[stri // Some providers support both of these authentication methods, some implement only one and // some implement neither. // This was tested with the following providers: - // - Google: supports client_secret_post // - Ory Hydra: supports client_secret_post // TODO @shipperizer: client_authentication methods should be configurable if secret := session.oauth2Config.ClientSecret; secret != "" { @@ -620,6 +613,17 @@ func (b *Broker) handleIsAuthenticated(ctx context.Context, session *session, au } } + if err := b.cfg.registerOwner(b.cfg.ConfigFile, authInfo.UserInfo.Name); err != nil { + // The user is not allowed if we fail to create the owner-autoregistration file. + // Otherwise the owner might change if the broker is restarted. + slog.Error(fmt.Sprintf("Failed to assign the owner role: %v", err)) + return AuthDenied, errorMessage{Message: "could not register the owner"} + } + + if !b.userNameIsAllowed(authInfo.UserInfo.Name) { + return AuthDenied, errorMessage{Message: "permission denied"} + } + if session.isOffline { return AuthGranted, userInfoMessage{UserInfo: authInfo.UserInfo} } @@ -636,6 +640,23 @@ func (b *Broker) handleIsAuthenticated(ctx context.Context, session *session, au return AuthGranted, userInfoMessage{UserInfo: authInfo.UserInfo} } +// userNameIsAllowed checks whether the user's username is allowed to access the machine. +func (b *Broker) userNameIsAllowed(userName string) bool { + normalizedUsername := b.provider.NormalizeUsername(userName) + // The user is allowed to log in if: + // - ALL users are allowed + // - the user's name is in the list of allowed_users + // - the user is the owner of the machine and OWNER is in the allowed_users list + if b.cfg.userConfig.allUsersAllowed { + return true + } + if _, ok := b.cfg.userConfig.allowedUsers[normalizedUsername]; ok { + return true + } + + return b.cfg.isOwnerAllowed(normalizedUsername) +} + func (b *Broker) startAuthenticate(sessionID string) (context.Context, error) { session, err := b.getSession(sessionID) if err != nil { diff --git a/internal/broker/broker_test.go b/internal/broker/broker_test.go index 6648dd83..12e5ba8f 100644 --- a/internal/broker/broker_test.go +++ b/internal/broker/broker_test.go @@ -6,6 +6,8 @@ import ( "fmt" "os" "path/filepath" + "slices" + "strings" "testing" "time" @@ -536,8 +538,10 @@ func TestIsAuthenticated(t *testing.T) { require.NoError(t, err, "Setup: Mkdir should not have returned an error") cfg := &brokerForTestConfig{ - Config: broker.Config{DataDir: dataDir}, - getUserInfoFails: tc.getUserInfoFails, + Config: broker.Config{DataDir: dataDir}, + getUserInfoFails: tc.getUserInfoFails, + ownerAllowed: true, + firstUserBecomesOwner: true, } if tc.customHandlers == nil { // Use the default provider URL if no custom handlers are provided. @@ -707,14 +711,36 @@ func TestIsAuthenticated(t *testing.T) { // Due to ordering restrictions, this test can not be run in parallel, otherwise the routines would not be ordered as expected. func TestConcurrentIsAuthenticated(t *testing.T) { tests := map[string]struct { - firstCallDelay int - secondCallDelay int + firstCallDelay int + secondCallDelay int + ownerAllowed bool + allUsersAllowed bool + firstUserBecomesOwner bool timeBetween time.Duration }{ - "First_auth_starts_and_finishes_before_second": {secondCallDelay: 1, timeBetween: 2 * time.Second}, - "First_auth_starts_first_but_second_finishes_first": {firstCallDelay: 3, timeBetween: time.Second}, - "First_auth_starts_first_then_second_starts_and_first_finishes": {firstCallDelay: 2, secondCallDelay: 3, timeBetween: time.Second}, + "First_auth_starts_and_finishes_before_second": { + secondCallDelay: 1, + timeBetween: 2 * time.Second, + allUsersAllowed: true, + }, + "First_auth_starts_first_but_second_finishes_first": { + firstCallDelay: 3, + timeBetween: time.Second, + allUsersAllowed: true, + }, + "First_auth_starts_first_then_second_starts_and_first_finishes": { + firstCallDelay: 2, + secondCallDelay: 3, + timeBetween: time.Second, + allUsersAllowed: true, + }, + "First_auth_starts_first_but_second_finishes_first_and_is_registered_as_the_owner": { + firstCallDelay: 3, + timeBetween: time.Second, + ownerAllowed: true, + firstUserBecomesOwner: true, + }, } for name, tc := range tests { @@ -728,9 +754,12 @@ func TestConcurrentIsAuthenticated(t *testing.T) { username2 := "user2@example.com" b := newBrokerForTests(t, &brokerForTestConfig{ - Config: broker.Config{DataDir: dataDir}, - firstCallDelay: tc.firstCallDelay, - secondCallDelay: tc.secondCallDelay, + Config: broker.Config{DataDir: dataDir}, + allUsersAllowed: tc.allUsersAllowed, + ownerAllowed: tc.ownerAllowed, + firstUserBecomesOwner: tc.firstUserBecomesOwner, + firstCallDelay: tc.firstCallDelay, + secondCallDelay: tc.secondCallDelay, tokenHandlerOptions: &testutils.TokenHandlerOptions{ IDTokenClaims: []map[string]interface{}{ {"sub": "user1", "name": "user1", "email": username1}, @@ -827,6 +856,126 @@ func TestConcurrentIsAuthenticated(t *testing.T) { } } +func TestIsAuthenticatedAllowedUsersConfig(t *testing.T) { + t.Parallel() + + u1 := "u1" + u2 := "u2" + u3 := "U3" + allUsers := []string{u1, u2, u3} + + idTokenClaims := []map[string]interface{}{} + for _, uname := range allUsers { + idTokenClaims = append(idTokenClaims, map[string]interface{}{"sub": "user", "name": "user", "email": uname}) + } + + tests := map[string]struct { + allowedUsers map[string]struct{} + owner string + ownerAllowed bool + allUsersAllowed bool + firstUserBecomesOwner bool + + wantAllowedUsers []string + wantUnallowedUsers []string + }{ + "No_users_allowed": { + wantUnallowedUsers: allUsers, + }, + "No_users_allowed_when_owner_is_allowed_but_not_set": { + ownerAllowed: true, + wantUnallowedUsers: allUsers, + }, + "No_users_allowed_when_owner_is_set_but_not_allowed": { + owner: u1, + wantUnallowedUsers: allUsers, + }, + + "All_users_are_allowed": { + allUsersAllowed: true, + wantAllowedUsers: allUsers, + }, + "Only_owner_allowed": { + ownerAllowed: true, + owner: u1, + wantAllowedUsers: []string{u1}, + wantUnallowedUsers: []string{u2, u3}, + }, + "Only_first_user_allowed": { + ownerAllowed: true, + firstUserBecomesOwner: true, + wantAllowedUsers: []string{u1}, + wantUnallowedUsers: []string{u2, u3}, + }, + "Specific_users_allowed": { + allowedUsers: map[string]struct{}{u1: {}, u2: {}}, + wantAllowedUsers: []string{u1, u2}, + wantUnallowedUsers: []string{u3}, + }, + "Specific_users_and_owner": { + ownerAllowed: true, + allowedUsers: map[string]struct{}{u1: {}}, + owner: u2, + wantAllowedUsers: []string{u1, u2}, + wantUnallowedUsers: []string{u3}, + }, + "Usernames_are_normalized": { + ownerAllowed: true, + allowedUsers: map[string]struct{}{u3: {}}, + owner: strings.ToLower(u3), + wantAllowedUsers: []string{u3}, + wantUnallowedUsers: []string{u1, u2}, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + outDir := t.TempDir() + dataDir := filepath.Join(outDir, "data") + err := os.Mkdir(dataDir, 0700) + require.NoError(t, err, "Setup: Mkdir should not have returned an error") + + b := newBrokerForTests(t, &brokerForTestConfig{ + Config: broker.Config{DataDir: dataDir}, + allowedUsers: tc.allowedUsers, + owner: tc.owner, + ownerAllowed: tc.ownerAllowed, + allUsersAllowed: tc.allUsersAllowed, + firstUserBecomesOwner: tc.firstUserBecomesOwner, + tokenHandlerOptions: &testutils.TokenHandlerOptions{ + IDTokenClaims: idTokenClaims, + }, + }) + + for _, u := range allUsers { + sessionID, key := newSessionForTests(t, b, u, "") + token := tokenOptions{username: u} + generateAndStoreCachedInfo(t, token, b.TokenPathForSession(sessionID)) + err = password.HashAndStorePassword("password", b.PasswordFilepathForSession(sessionID)) + require.NoError(t, err, "Setup: HashAndStorePassword should not have returned an error") + + updateAuthModes(t, b, sessionID, authmodes.Password) + + authData := `{"challenge":"` + encryptChallenge(t, "password", key) + `"}` + + access, data, err := b.IsAuthenticated(sessionID, authData) + require.True(t, json.Valid([]byte(data)), "IsAuthenticated returned data must be a valid JSON") + require.NoError(t, err) + if slices.Contains(tc.wantAllowedUsers, u) { + require.Equal(t, access, broker.AuthGranted, "authentication failed") + continue + } + if slices.Contains(tc.wantUnallowedUsers, u) { + require.Equal(t, access, broker.AuthDenied, "authentication failed") + continue + } + t.Fatalf("user %s is not in the allowed or unallowed users list", u) + } + }) + } +} + func TestFetchUserInfo(t *testing.T) { t.Parallel() diff --git a/internal/broker/config.go b/internal/broker/config.go index 207c541a..c084069e 100644 --- a/internal/broker/config.go +++ b/internal/broker/config.go @@ -1,12 +1,15 @@ package broker import ( + "embed" "errors" "fmt" + "html/template" "io/fs" "os" "path/filepath" "strings" + "sync" "gopkg.in/ini.v1" ) @@ -24,15 +27,63 @@ const ( // usersSection is the section name in the config file for the users and broker specific configuration. usersSection = "users" + // allowedUsersKey is the key in the config file for the users that are allowed to access the machine. + allowedUsersKey = "allowed_users" + // ownerKey is the key in the config file for the owner of the machine. + ownerKey = "owner" // homeDirKey is the key in the config file for the home directory prefix. homeDirKey = "home_base_dir" // SSHSuffixKey is the key in the config file for the SSH allowed suffixes. sshSuffixesKey = "ssh_allowed_suffixes" + + // allUsersKeyword is the keyword for the `allowed_users` key that allows access to all users. + allUsersKeyword = "ALL" + // ownerUserKeyword is the keyword for the `allowed_users` key that allows access to the owner. + ownerUserKeyword = "OWNER" + + // ownerAutoRegistrationConfigPath is the name of the file that will be auto-generated to register the owner. + ownerAutoRegistrationConfigPath = "20-owner-autoregistration.conf" + ownerAutoRegistrationConfigTemplate = "templates/20-owner-autoregistration.conf.tmpl" +) + +var ( + //go:embed templates/20-owner-autoregistration.conf.tmpl + ownerAutoRegistrationConfig embed.FS ) +type provider interface { + NormalizeUsername(username string) string +} + +type templateEnv struct { + Owner string +} + +type userConfig struct { + clientID string + clientSecret string + issuerURL string + + allowedUsers map[string]struct{} + allUsersAllowed bool + ownerAllowed bool + firstUserBecomesOwner bool + owner string + ownerMutex *sync.RWMutex + homeBaseDir string + allowedSSHSuffixes []string + + provider provider +} + +// GetDropInDir takes the broker configuration path and returns the drop in dir path. +func GetDropInDir(cfgPath string) string { + return cfgPath + ".d" +} + func getDropInFiles(cfgPath string) ([]any, error) { // Check if a .d directory exists and return the paths to the files in it. - dropInDir := cfgPath + ".d" + dropInDir := GetDropInDir(cfgPath) files, err := os.ReadDir(dropInDir) if errors.Is(err, fs.ErrNotExist) { return nil, nil @@ -53,9 +104,53 @@ func getDropInFiles(cfgPath string) ([]any, error) { return dropInFiles, nil } +func populateUsersConfig(cfg *userConfig, users *ini.Section) { + if users == nil { + // The default behavior is to allow only the owner + cfg.ownerAllowed = true + cfg.firstUserBecomesOwner = true + return + } + + cfg.homeBaseDir = users.Key(homeDirKey).String() + cfg.allowedSSHSuffixes = strings.Split(users.Key(sshSuffixesKey).String(), ",") + + if cfg.allowedUsers == nil { + cfg.allowedUsers = make(map[string]struct{}) + } + + allowedUsers := users.Key(allowedUsersKey).Strings(",") + if len(allowedUsers) == 0 { + allowedUsers = append(allowedUsers, ownerUserKeyword) + } + + for _, user := range allowedUsers { + if user == allUsersKeyword { + cfg.allUsersAllowed = true + continue + } + if user == ownerUserKeyword { + cfg.ownerAllowed = true + if !users.HasKey(ownerKey) { + // If owner is unset, then the first user becomes owner + cfg.firstUserBecomesOwner = true + } + continue + } + + cfg.allowedUsers[cfg.provider.NormalizeUsername(user)] = struct{}{} + } + + // We need to read the owner key after we call HasKey, because the key is created + // when we call the "Key" function and we can't distinguish between empty and unset. + cfg.ownerMutex.Lock() + defer cfg.ownerMutex.Unlock() + cfg.owner = cfg.provider.NormalizeUsername(users.Key(ownerKey).String()) +} + // parseConfigFile parses the config file and returns a map with the configuration keys and values. -func parseConfigFile(cfgPath string) (userConfig, error) { - cfg := userConfig{} +func parseConfigFile(cfgPath string, p provider) (userConfig, error) { + cfg := userConfig{provider: p, ownerMutex: &sync.RWMutex{}} dropInFiles, err := getDropInFiles(cfgPath) if err != nil { @@ -86,11 +181,60 @@ func parseConfigFile(cfgPath string) (userConfig, error) { cfg.clientSecret = oidc.Key(clientSecret).String() } - users := iniCfg.Section(usersSection) - if users != nil { - cfg.homeBaseDir = users.Key(homeDirKey).String() - cfg.allowedSSHSuffixes = strings.Split(users.Key(sshSuffixesKey).String(), ",") - } + populateUsersConfig(&cfg, iniCfg.Section(usersSection)) return cfg, nil } + +func (uc *userConfig) shouldRegisterOwner() bool { + return uc.ownerAllowed && uc.firstUserBecomesOwner && uc.owner == "" +} + +func (uc *userConfig) isOwnerAllowed(userName string) bool { + uc.ownerMutex.RLock() + defer uc.ownerMutex.RUnlock() + + return uc.ownerAllowed && uc.owner == userName +} + +func (uc *userConfig) registerOwner(cfgPath, userName string) error { + // We need to lock here to avoid a race condition where two users log in at the same time, causing both to be + // considered the owner. + uc.ownerMutex.Lock() + defer uc.ownerMutex.Unlock() + + if !uc.shouldRegisterOwner() { + return nil + } + + if cfgPath == "" { + uc.owner = uc.provider.NormalizeUsername(userName) + uc.firstUserBecomesOwner = false + return nil + } + + p := filepath.Join(GetDropInDir(cfgPath), ownerAutoRegistrationConfigPath) + + templateName := filepath.Base(ownerAutoRegistrationConfigTemplate) + t, err := template.New(templateName).ParseFS(ownerAutoRegistrationConfig, ownerAutoRegistrationConfigTemplate) + if err != nil { + return fmt.Errorf("failed to open autoregistration template: %v", err) + } + + f, err := os.OpenFile(p, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0600) + if err != nil { + return fmt.Errorf("failed to create owner registration file: %v", err) + } + defer f.Close() + + if err := t.Execute(f, templateEnv{Owner: userName}); err != nil { + return fmt.Errorf("failed to write owner registration file: %v", err) + } + + // We set the owner after we create the autoregistration file, so that in case of an error + // the owner is not updated. + uc.owner = uc.provider.NormalizeUsername(userName) + uc.firstUserBecomesOwner = false + + return nil +} diff --git a/internal/broker/config_test.go b/internal/broker/config_test.go index 40d4dd5b..f20e9d50 100644 --- a/internal/broker/config_test.go +++ b/internal/broker/config_test.go @@ -6,10 +6,12 @@ import ( "path/filepath" "reflect" "strings" + "sync" "testing" "unsafe" "github.com/stretchr/testify/require" + "github.com/ubuntu/authd-oidc-brokers/internal/testutils" "github.com/ubuntu/authd-oidc-brokers/internal/testutils/golden" ) @@ -56,6 +58,8 @@ issuer = https://higher-precedence-issuer.url.com func TestParseConfig(t *testing.T) { t.Parallel() + p := &testutils.MockProvider{} + ignoredFields := map[string]struct{}{"provider": {}, "ownerMutex": {}} tests := map[string]struct { configType string @@ -96,7 +100,7 @@ func TestParseConfig(t *testing.T) { require.NoError(t, err, "Setup: Failed to make config file unreadable") } - dropInDir := confPath + ".d" + dropInDir := GetDropInDir(confPath) if tc.dropInType != "" { err = os.Mkdir(dropInDir, 0700) require.NoError(t, err, "Setup: Failed to create drop-in directory") @@ -121,12 +125,13 @@ func TestParseConfig(t *testing.T) { require.NoError(t, err, "Setup: Failed to make drop-in file unreadable") } - cfg, err := parseConfigFile(confPath) + cfg, err := parseConfigFile(confPath, p) if tc.wantErr { require.Error(t, err) return } require.NoError(t, err) + require.Equal(t, cfg.provider, p) outDir := t.TempDir() // Write the names and values of all fields in the config to a file. We can't use the json or yaml @@ -136,6 +141,9 @@ func TestParseConfig(t *testing.T) { typ := reflect.TypeOf(&cfg).Elem() for i := 0; i < typ.NumField(); i++ { field := typ.Field(i) + if _, ok := ignoredFields[field.Name]; ok { + continue + } fieldValue := val.Field(i) if field.PkgPath != "" { //nolint: gosec // We are using unsafe to access unexported fields for testing purposes @@ -150,3 +158,177 @@ func TestParseConfig(t *testing.T) { }) } } + +var testParseUserConfigTypes = map[string]string{ + "All_are_allowed": ` +[oidc] +issuer = https://issuer.url.com +client_id = client_id + +[users] +allowed_users = ALL +owner = machine_owner +home_base_dir = /home +allowed_ssh_suffixes = @issuer.url.com +`, + "Only_owner_is_allowed": ` +[oidc] +issuer = https://issuer.url.com +client_id = client_id + +[users] +allowed_users = OWNER +owner = machine_owner +home_base_dir = /home +allowed_ssh_suffixes = @issuer.url.com +`, + "By_default_only_owner_is_allowed": ` +[oidc] +issuer = https://issuer.url.com +client_id = client_id + +[users] +owner = machine_owner +home_base_dir = /home +allowed_ssh_suffixes = @issuer.url.com +`, + "Only_owner_is_allowed_but_is_unset": ` +[oidc] +issuer = https://issuer.url.com +client_id = client_id + +[users] +home_base_dir = /home +allowed_ssh_suffixes = @issuer.url.com +`, + "Only_owner_is_allowed_but_is_empty": ` +[oidc] +issuer = https://issuer.url.com +client_id = client_id + +[users] +owner = +home_base_dir = /home +allowed_ssh_suffixes = @issuer.url.com +`, + "Users_u1_and_u2_are_allowed": ` +[oidc] +issuer = https://issuer.url.com +client_id = client_id + +[users] +allowed_users = u1,u2 +home_base_dir = /home +allowed_ssh_suffixes = @issuer.url.com +`, + "Unset_owner_and_u1_is_allowed": ` +[oidc] +issuer = https://issuer.url.com +client_id = client_id + +[users] +allowed_users = OWNER,u1 +home_base_dir = /home +allowed_ssh_suffixes = @issuer.url.com +`, + "Set_owner_and_u1_is_allowed": ` +[oidc] +issuer = https://issuer.url.com +client_id = client_id + +[users] +allowed_users = OWNER,u1 +owner = machine_owner +home_base_dir = /home +allowed_ssh_suffixes = @issuer.url.com +`, +} + +func TestParseUserConfig(t *testing.T) { + t.Parallel() + p := &testutils.MockProvider{} + + tests := map[string]struct { + wantAllUsersAllowed bool + wantOwnerAllowed bool + wantFirstUserBecomesOwner bool + wantOwner string + wantAllowedUsers []string + }{ + "All_are_allowed": {wantAllUsersAllowed: true, wantOwner: "machine_owner"}, + "Only_owner_is_allowed": {wantOwnerAllowed: true, wantOwner: "machine_owner"}, + "By_default_only_owner_is_allowed": {wantOwnerAllowed: true, wantOwner: "machine_owner"}, + "Only_owner_is_allowed_but_is_unset": {wantOwnerAllowed: true, wantFirstUserBecomesOwner: true}, + "Only_owner_is_allowed_but_is_empty": {wantOwnerAllowed: true}, + "Users_u1_and_u2_are_allowed": {wantAllowedUsers: []string{"u1", "u2"}}, + "Unset_owner_and_u1_is_allowed": { + wantOwnerAllowed: true, + wantFirstUserBecomesOwner: true, + wantAllowedUsers: []string{"u1"}, + }, + "Set_owner_and_u1_is_allowed": { + wantOwnerAllowed: true, + wantOwner: "machine_owner", + wantAllowedUsers: []string{"u1"}, + }, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + outDir := t.TempDir() + confPath := filepath.Join(outDir, "broker.conf") + + err := os.WriteFile(confPath, []byte(testParseUserConfigTypes[name]), 0600) + require.NoError(t, err, "Setup: Failed to write config file") + + dropInDir := GetDropInDir(confPath) + err = os.Mkdir(dropInDir, 0700) + require.NoError(t, err, "Setup: Failed to create drop-in directory") + + cfg, err := parseConfigFile(confPath, p) + + // convert the allowed users array to a map + allowedUsersMap := map[string]struct{}{} + for _, k := range tc.wantAllowedUsers { + allowedUsersMap[k] = struct{}{} + } + + require.Equal(t, tc.wantAllUsersAllowed, cfg.allUsersAllowed) + require.Equal(t, tc.wantOwnerAllowed, cfg.ownerAllowed) + require.Equal(t, tc.wantOwner, cfg.owner) + require.Equal(t, tc.wantFirstUserBecomesOwner, cfg.firstUserBecomesOwner) + require.Equal(t, allowedUsersMap, cfg.allowedUsers) + + require.NoError(t, err) + golden.CheckOrUpdateFileTree(t, outDir) + }) + } +} + +func TestRegisterOwner(t *testing.T) { + p := &testutils.MockProvider{} + outDir := t.TempDir() + userName := "owner_name" + confPath := filepath.Join(outDir, "broker.conf") + + err := os.WriteFile(confPath, []byte(configTypes["valid"]), 0600) + require.NoError(t, err, "Setup: Failed to write config file") + + dropInDir := GetDropInDir(confPath) + err = os.Mkdir(dropInDir, 0700) + require.NoError(t, err, "Setup: Failed to create drop-in directory") + + cfg := userConfig{firstUserBecomesOwner: true, ownerAllowed: true, provider: p, ownerMutex: &sync.RWMutex{}} + err = cfg.registerOwner(confPath, userName) + require.NoError(t, err) + + require.Equal(t, cfg.owner, userName) + require.Equal(t, cfg.firstUserBecomesOwner, false) + + f, err := os.Open(filepath.Join(dropInDir, "20-owner-autoregistration.conf")) + require.NoError(t, err, "failed to open 20-owner-autoregistration.conf") + defer f.Close() + + golden.CheckOrUpdateFileTree(t, outDir) +} diff --git a/internal/broker/export_test.go b/internal/broker/export_test.go index 6bfc49a2..769c0937 100644 --- a/internal/broker/export_test.go +++ b/internal/broker/export_test.go @@ -2,11 +2,16 @@ package broker import ( "context" + "sync" "github.com/ubuntu/authd-oidc-brokers/internal/providers/info" tokenPkg "github.com/ubuntu/authd-oidc-brokers/internal/token" ) +func (cfg *Config) Init() { + cfg.ownerMutex = &sync.RWMutex{} +} + func (cfg *Config) SetClientID(clientID string) { cfg.clientID = clientID } @@ -19,10 +24,38 @@ func (cfg *Config) SetHomeBaseDir(homeBaseDir string) { cfg.homeBaseDir = homeBaseDir } +func (cfg *Config) SetAllowedUsers(allowedUsers map[string]struct{}) { + cfg.allowedUsers = allowedUsers +} + +func (cfg *Config) SetOwner(owner string) { + cfg.ownerMutex.Lock() + defer cfg.ownerMutex.Unlock() + cfg.owner = owner +} + +func (cfg *Config) SetFirstUserBecomesOwner(firstUserBecomesOwner bool) { + cfg.ownerMutex.Lock() + defer cfg.ownerMutex.Unlock() + cfg.firstUserBecomesOwner = firstUserBecomesOwner +} + +func (cfg *Config) SetAllUsersAllowed(allUsersAllowed bool) { + cfg.allUsersAllowed = allUsersAllowed +} + +func (cfg *Config) SetOwnerAllowed(ownerAllowed bool) { + cfg.ownerAllowed = ownerAllowed +} + func (cfg *Config) SetAllowedSSHSuffixes(allowedSSHSuffixes []string) { cfg.allowedSSHSuffixes = allowedSSHSuffixes } +func (cfg *Config) SetProvider(provider provider) { + cfg.provider = provider +} + func (cfg *Config) ClientID() string { return cfg.clientID } diff --git a/internal/broker/helper_test.go b/internal/broker/helper_test.go index 1810074a..f605814f 100644 --- a/internal/broker/helper_test.go +++ b/internal/broker/helper_test.go @@ -14,6 +14,7 @@ import ( "github.com/golang-jwt/jwt/v5" "github.com/stretchr/testify/require" "github.com/ubuntu/authd-oidc-brokers/internal/broker" + "github.com/ubuntu/authd-oidc-brokers/internal/providers" "github.com/ubuntu/authd-oidc-brokers/internal/providers/info" "github.com/ubuntu/authd-oidc-brokers/internal/testutils" "github.com/ubuntu/authd-oidc-brokers/internal/token" @@ -22,9 +23,15 @@ import ( type brokerForTestConfig struct { broker.Config - issuerURL string - homeBaseDir string - allowedSSHSuffixes []string + issuerURL string + allowedUsers map[string]struct{} + allUsersAllowed bool + ownerAllowed bool + firstUserBecomesOwner bool + owner string + homeBaseDir string + allowedSSHSuffixes []string + provider providers.Provider getUserInfoFails bool firstCallDelay int @@ -40,6 +47,7 @@ type brokerForTestConfig struct { func newBrokerForTests(t *testing.T, cfg *brokerForTestConfig) (b *broker.Broker) { t.Helper() + cfg.Init() if cfg.issuerURL != "" { cfg.SetIssuerURL(cfg.issuerURL) } @@ -49,6 +57,21 @@ func newBrokerForTests(t *testing.T, cfg *brokerForTestConfig) (b *broker.Broker if cfg.allowedSSHSuffixes != nil { cfg.SetAllowedSSHSuffixes(cfg.allowedSSHSuffixes) } + if cfg.allowedUsers != nil { + cfg.SetAllowedUsers(cfg.allowedUsers) + } + if cfg.owner != "" { + cfg.SetOwner(cfg.owner) + } + if cfg.firstUserBecomesOwner != false { + cfg.SetFirstUserBecomesOwner(cfg.firstUserBecomesOwner) + } + if cfg.allUsersAllowed != false { + cfg.SetAllUsersAllowed(cfg.allUsersAllowed) + } + if cfg.ownerAllowed != false { + cfg.SetOwnerAllowed(cfg.ownerAllowed) + } provider := &testutils.MockProvider{ GetUserInfoFails: cfg.getUserInfoFails, @@ -57,6 +80,9 @@ func newBrokerForTests(t *testing.T, cfg *brokerForTestConfig) (b *broker.Broker GetGroupsFunc: cfg.getGroupsFunc, } + if cfg.provider == nil { + cfg.SetProvider(provider) + } if cfg.DataDir == "" { cfg.DataDir = t.TempDir() } diff --git a/internal/broker/templates/20-owner-autoregistration.conf.tmpl b/internal/broker/templates/20-owner-autoregistration.conf.tmpl new file mode 100644 index 00000000..4673fe2c --- /dev/null +++ b/internal/broker/templates/20-owner-autoregistration.conf.tmpl @@ -0,0 +1,13 @@ +## This file was generated automatically by the broker. DO NOT EDIT. +## +## This file registers the first authenticated user as the owner of +## this device. +## +## The 'owner' option is only considered for authentication if +## 'allowed_users' contains the 'OWNER' keyword. +## +## To register a different owner for the machine on the next +## successful authentication, delete this file. + +[users] +owner = {{ .Owner }} diff --git a/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first_and_is_registered_as_the_owner/data/provider_url/user1@example.com/password b/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first_and_is_registered_as_the_owner/data/provider_url/user1@example.com/password new file mode 100644 index 00000000..11994724 --- /dev/null +++ b/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first_and_is_registered_as_the_owner/data/provider_url/user1@example.com/password @@ -0,0 +1 @@ +Definitely a hashed password \ No newline at end of file diff --git a/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first_and_is_registered_as_the_owner/data/provider_url/user1@example.com/token.json b/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first_and_is_registered_as_the_owner/data/provider_url/user1@example.com/token.json new file mode 100644 index 00000000..80ab7838 --- /dev/null +++ b/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first_and_is_registered_as_the_owner/data/provider_url/user1@example.com/token.json @@ -0,0 +1 @@ +Definitely an encrypted token \ No newline at end of file diff --git a/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first_and_is_registered_as_the_owner/data/provider_url/user2@example.com/password b/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first_and_is_registered_as_the_owner/data/provider_url/user2@example.com/password new file mode 100644 index 00000000..11994724 --- /dev/null +++ b/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first_and_is_registered_as_the_owner/data/provider_url/user2@example.com/password @@ -0,0 +1 @@ +Definitely a hashed password \ No newline at end of file diff --git a/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first_and_is_registered_as_the_owner/data/provider_url/user2@example.com/token.json b/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first_and_is_registered_as_the_owner/data/provider_url/user2@example.com/token.json new file mode 100644 index 00000000..80ab7838 --- /dev/null +++ b/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first_and_is_registered_as_the_owner/data/provider_url/user2@example.com/token.json @@ -0,0 +1 @@ +Definitely an encrypted token \ No newline at end of file diff --git a/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first_and_is_registered_as_the_owner/first_auth b/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first_and_is_registered_as_the_owner/first_auth new file mode 100644 index 00000000..ffd251e4 --- /dev/null +++ b/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first_and_is_registered_as_the_owner/first_auth @@ -0,0 +1,3 @@ +access: denied +data: '{"message":"authentication failure: permission denied"}' +err: diff --git a/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first_and_is_registered_as_the_owner/second_auth b/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first_and_is_registered_as_the_owner/second_auth new file mode 100644 index 00000000..f0643dd9 --- /dev/null +++ b/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first_and_is_registered_as_the_owner/second_auth @@ -0,0 +1,3 @@ +access: granted +data: '{"userinfo":{"name":"user2@example.com","uuid":"user2","dir":"/home/user2@example.com","shell":"/usr/bin/bash","gecos":"user2@example.com","groups":[{"name":"remote-test-group","ugid":"12345"},{"name":"local-test-group","ugid":""}]}}' +err: diff --git a/internal/broker/testdata/golden/TestParseConfig/Do_not_fail_if_values_contain_a_single_template_delimiter/config.txt b/internal/broker/testdata/golden/TestParseConfig/Do_not_fail_if_values_contain_a_single_template_delimiter/config.txt index ac876a00..8005e7b9 100644 --- a/internal/broker/testdata/golden/TestParseConfig/Do_not_fail_if_values_contain_a_single_template_delimiter/config.txt +++ b/internal/broker/testdata/golden/TestParseConfig/Do_not_fail_if_values_contain_a_single_template_delimiter/config.txt @@ -1,5 +1,10 @@ clientID= +allowedUsers=map[] +allUsersAllowed=false +ownerAllowed=true +firstUserBecomesOwner=true +owner= homeBaseDir= allowedSSHSuffixes=[] \ No newline at end of file diff --git a/internal/broker/testdata/golden/TestParseConfig/Successfully_parse_config_file/config.txt b/internal/broker/testdata/golden/TestParseConfig/Successfully_parse_config_file/config.txt index ba4609b6..78d109ab 100644 --- a/internal/broker/testdata/golden/TestParseConfig/Successfully_parse_config_file/config.txt +++ b/internal/broker/testdata/golden/TestParseConfig/Successfully_parse_config_file/config.txt @@ -1,5 +1,10 @@ clientID=client_id clientSecret= issuerURL=https://issuer.url.com +allowedUsers=map[] +allUsersAllowed=false +ownerAllowed=true +firstUserBecomesOwner=true +owner= homeBaseDir= allowedSSHSuffixes=[] \ No newline at end of file diff --git a/internal/broker/testdata/golden/TestParseConfig/Successfully_parse_config_file_with_optional_values/config.txt b/internal/broker/testdata/golden/TestParseConfig/Successfully_parse_config_file_with_optional_values/config.txt index 6ad8bf05..3066f32f 100644 --- a/internal/broker/testdata/golden/TestParseConfig/Successfully_parse_config_file_with_optional_values/config.txt +++ b/internal/broker/testdata/golden/TestParseConfig/Successfully_parse_config_file_with_optional_values/config.txt @@ -1,5 +1,10 @@ clientID=client_id clientSecret= issuerURL=https://issuer.url.com +allowedUsers=map[] +allUsersAllowed=false +ownerAllowed=true +firstUserBecomesOwner=true +owner= homeBaseDir=/home allowedSSHSuffixes=[] \ No newline at end of file diff --git a/internal/broker/testdata/golden/TestParseConfig/Successfully_parse_config_with_drop_in_files/config.txt b/internal/broker/testdata/golden/TestParseConfig/Successfully_parse_config_with_drop_in_files/config.txt index 7761f024..f651947f 100644 --- a/internal/broker/testdata/golden/TestParseConfig/Successfully_parse_config_with_drop_in_files/config.txt +++ b/internal/broker/testdata/golden/TestParseConfig/Successfully_parse_config_with_drop_in_files/config.txt @@ -1,5 +1,10 @@ clientID=lower_precedence_client_id clientSecret= issuerURL=https://higher-precedence-issuer.url.com +allowedUsers=map[] +allUsersAllowed=false +ownerAllowed=true +firstUserBecomesOwner=true +owner= homeBaseDir=/home allowedSSHSuffixes=[] \ No newline at end of file diff --git a/internal/broker/testdata/golden/TestParseUserConfig/All_are_allowed/broker.conf b/internal/broker/testdata/golden/TestParseUserConfig/All_are_allowed/broker.conf new file mode 100644 index 00000000..b777f7fc --- /dev/null +++ b/internal/broker/testdata/golden/TestParseUserConfig/All_are_allowed/broker.conf @@ -0,0 +1,10 @@ + +[oidc] +issuer = https://issuer.url.com +client_id = client_id + +[users] +allowed_users = ALL +owner = machine_owner +home_base_dir = /home +allowed_ssh_suffixes = @issuer.url.com diff --git a/internal/broker/testdata/golden/TestParseUserConfig/All_are_allowed/broker.conf.d/.empty b/internal/broker/testdata/golden/TestParseUserConfig/All_are_allowed/broker.conf.d/.empty new file mode 100644 index 00000000..e69de29b diff --git a/internal/broker/testdata/golden/TestParseUserConfig/By_default_only_owner_is_allowed/broker.conf b/internal/broker/testdata/golden/TestParseUserConfig/By_default_only_owner_is_allowed/broker.conf new file mode 100644 index 00000000..46abf5a3 --- /dev/null +++ b/internal/broker/testdata/golden/TestParseUserConfig/By_default_only_owner_is_allowed/broker.conf @@ -0,0 +1,9 @@ + +[oidc] +issuer = https://issuer.url.com +client_id = client_id + +[users] +owner = machine_owner +home_base_dir = /home +allowed_ssh_suffixes = @issuer.url.com diff --git a/internal/broker/testdata/golden/TestParseUserConfig/By_default_only_owner_is_allowed/broker.conf.d/.empty b/internal/broker/testdata/golden/TestParseUserConfig/By_default_only_owner_is_allowed/broker.conf.d/.empty new file mode 100644 index 00000000..e69de29b diff --git a/internal/broker/testdata/golden/TestParseUserConfig/Only_owner_is_allowed/broker.conf b/internal/broker/testdata/golden/TestParseUserConfig/Only_owner_is_allowed/broker.conf new file mode 100644 index 00000000..bcd7c04b --- /dev/null +++ b/internal/broker/testdata/golden/TestParseUserConfig/Only_owner_is_allowed/broker.conf @@ -0,0 +1,10 @@ + +[oidc] +issuer = https://issuer.url.com +client_id = client_id + +[users] +allowed_users = OWNER +owner = machine_owner +home_base_dir = /home +allowed_ssh_suffixes = @issuer.url.com diff --git a/internal/broker/testdata/golden/TestParseUserConfig/Only_owner_is_allowed/broker.conf.d/.empty b/internal/broker/testdata/golden/TestParseUserConfig/Only_owner_is_allowed/broker.conf.d/.empty new file mode 100644 index 00000000..e69de29b diff --git a/internal/broker/testdata/golden/TestParseUserConfig/Only_owner_is_allowed_but_is_empty/broker.conf b/internal/broker/testdata/golden/TestParseUserConfig/Only_owner_is_allowed_but_is_empty/broker.conf new file mode 100644 index 00000000..8ad17837 --- /dev/null +++ b/internal/broker/testdata/golden/TestParseUserConfig/Only_owner_is_allowed_but_is_empty/broker.conf @@ -0,0 +1,9 @@ + +[oidc] +issuer = https://issuer.url.com +client_id = client_id + +[users] +owner = +home_base_dir = /home +allowed_ssh_suffixes = @issuer.url.com diff --git a/internal/broker/testdata/golden/TestParseUserConfig/Only_owner_is_allowed_but_is_empty/broker.conf.d/.empty b/internal/broker/testdata/golden/TestParseUserConfig/Only_owner_is_allowed_but_is_empty/broker.conf.d/.empty new file mode 100644 index 00000000..e69de29b diff --git a/internal/broker/testdata/golden/TestParseUserConfig/Only_owner_is_allowed_but_is_unset/broker.conf b/internal/broker/testdata/golden/TestParseUserConfig/Only_owner_is_allowed_but_is_unset/broker.conf new file mode 100644 index 00000000..2f17914d --- /dev/null +++ b/internal/broker/testdata/golden/TestParseUserConfig/Only_owner_is_allowed_but_is_unset/broker.conf @@ -0,0 +1,8 @@ + +[oidc] +issuer = https://issuer.url.com +client_id = client_id + +[users] +home_base_dir = /home +allowed_ssh_suffixes = @issuer.url.com diff --git a/internal/broker/testdata/golden/TestParseUserConfig/Only_owner_is_allowed_but_is_unset/broker.conf.d/.empty b/internal/broker/testdata/golden/TestParseUserConfig/Only_owner_is_allowed_but_is_unset/broker.conf.d/.empty new file mode 100644 index 00000000..e69de29b diff --git a/internal/broker/testdata/golden/TestParseUserConfig/Set_owner_and_u1_is_allowed/broker.conf b/internal/broker/testdata/golden/TestParseUserConfig/Set_owner_and_u1_is_allowed/broker.conf new file mode 100644 index 00000000..050e0230 --- /dev/null +++ b/internal/broker/testdata/golden/TestParseUserConfig/Set_owner_and_u1_is_allowed/broker.conf @@ -0,0 +1,10 @@ + +[oidc] +issuer = https://issuer.url.com +client_id = client_id + +[users] +allowed_users = OWNER,u1 +owner = machine_owner +home_base_dir = /home +allowed_ssh_suffixes = @issuer.url.com diff --git a/internal/broker/testdata/golden/TestParseUserConfig/Set_owner_and_u1_is_allowed/broker.conf.d/.empty b/internal/broker/testdata/golden/TestParseUserConfig/Set_owner_and_u1_is_allowed/broker.conf.d/.empty new file mode 100644 index 00000000..e69de29b diff --git a/internal/broker/testdata/golden/TestParseUserConfig/Unset_owner_and_u1_is_allowed/broker.conf b/internal/broker/testdata/golden/TestParseUserConfig/Unset_owner_and_u1_is_allowed/broker.conf new file mode 100644 index 00000000..890dcb98 --- /dev/null +++ b/internal/broker/testdata/golden/TestParseUserConfig/Unset_owner_and_u1_is_allowed/broker.conf @@ -0,0 +1,9 @@ + +[oidc] +issuer = https://issuer.url.com +client_id = client_id + +[users] +allowed_users = OWNER,u1 +home_base_dir = /home +allowed_ssh_suffixes = @issuer.url.com diff --git a/internal/broker/testdata/golden/TestParseUserConfig/Unset_owner_and_u1_is_allowed/broker.conf.d/.empty b/internal/broker/testdata/golden/TestParseUserConfig/Unset_owner_and_u1_is_allowed/broker.conf.d/.empty new file mode 100644 index 00000000..e69de29b diff --git a/internal/broker/testdata/golden/TestParseUserConfig/Users_u1_and_u2_are_allowed/broker.conf b/internal/broker/testdata/golden/TestParseUserConfig/Users_u1_and_u2_are_allowed/broker.conf new file mode 100644 index 00000000..6a2c1bc1 --- /dev/null +++ b/internal/broker/testdata/golden/TestParseUserConfig/Users_u1_and_u2_are_allowed/broker.conf @@ -0,0 +1,9 @@ + +[oidc] +issuer = https://issuer.url.com +client_id = client_id + +[users] +allowed_users = u1,u2 +home_base_dir = /home +allowed_ssh_suffixes = @issuer.url.com diff --git a/internal/broker/testdata/golden/TestParseUserConfig/Users_u1_and_u2_are_allowed/broker.conf.d/.empty b/internal/broker/testdata/golden/TestParseUserConfig/Users_u1_and_u2_are_allowed/broker.conf.d/.empty new file mode 100644 index 00000000..e69de29b diff --git a/internal/broker/testdata/golden/TestRegisterOwner/broker.conf b/internal/broker/testdata/golden/TestRegisterOwner/broker.conf new file mode 100644 index 00000000..aa6cfd3e --- /dev/null +++ b/internal/broker/testdata/golden/TestRegisterOwner/broker.conf @@ -0,0 +1,4 @@ + +[oidc] +issuer = https://issuer.url.com +client_id = client_id diff --git a/internal/broker/testdata/golden/TestRegisterOwner/broker.conf.d/20-owner-autoregistration.conf b/internal/broker/testdata/golden/TestRegisterOwner/broker.conf.d/20-owner-autoregistration.conf new file mode 100644 index 00000000..f6c5a43b --- /dev/null +++ b/internal/broker/testdata/golden/TestRegisterOwner/broker.conf.d/20-owner-autoregistration.conf @@ -0,0 +1,13 @@ +## This file was generated automatically by the broker. DO NOT EDIT. +## +## This file registers the first authenticated user as the owner of +## this device. +## +## The 'owner' option is only considered for authentication if +## 'allowed_users' contains the 'OWNER' keyword. +## +## To register a different owner for the machine on the next +## successful authentication, delete this file. + +[users] +owner = owner_name diff --git a/internal/providers/msentraid/msentraid.go b/internal/providers/msentraid/msentraid.go index b4ce8396..a5ee0bef 100644 --- a/internal/providers/msentraid/msentraid.go +++ b/internal/providers/msentraid/msentraid.go @@ -273,12 +273,17 @@ func (p Provider) CurrentAuthenticationModesOffered( return offeredModes, nil } +// NormalizeUsername parses a username into a normalized version. +func (p Provider) NormalizeUsername(username string) string { + // Microsoft Entra usernames are case-insensitive. We can safely use strings.ToLower here without worrying about + // different Unicode characters that fold to the same lowercase letter, because the Microsoft Entra username policy + // (which we check in VerifyUsername) ensures that the username only contains ASCII characters. + return strings.ToLower(username) +} + // VerifyUsername checks if the authenticated username matches the requested username and that both are valid. func (p Provider) VerifyUsername(requestedUsername, authenticatedUsername string) error { - // Microsoft Entra usernames are case-insensitive. We can safely use strings.EqualFold here without worrying about - // different Unicode characters that fold to the same lowercase letter, because the Microsoft Entra username policy - // (which we checked above) ensures that the username only contains ASCII characters. - if !strings.EqualFold(requestedUsername, authenticatedUsername) { + if p.NormalizeUsername(requestedUsername) != p.NormalizeUsername(authenticatedUsername) { return fmt.Errorf("requested username %q does not match the authenticated user %q", requestedUsername, authenticatedUsername) } diff --git a/internal/providers/msentraid/msentraid_test.go b/internal/providers/msentraid/msentraid_test.go index ce3a12ee..34aee242 100644 --- a/internal/providers/msentraid/msentraid_test.go +++ b/internal/providers/msentraid/msentraid_test.go @@ -52,6 +52,33 @@ func TestCheckTokenScopes(t *testing.T) { } } +func TestNormalizeUsername(t *testing.T) { + t.Parallel() + tests := map[string]struct { + username string + + wantNormalized string + }{ + "Shouldnt_change_all_lower_case": { + username: "name@email.com", + wantNormalized: "name@email.com", + }, + "Should_convert_all_to_lower_case": { + username: "NAME@email.com", + wantNormalized: "name@email.com", + }, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + p := msentraid.New() + ret := p.NormalizeUsername(tc.username) + require.Equal(t, tc.wantNormalized, ret) + }) + } +} + func TestVerifyUsername(t *testing.T) { t.Parallel() diff --git a/internal/providers/noprovider/noprovider.go b/internal/providers/noprovider/noprovider.go index e0cbec70..7d261954 100644 --- a/internal/providers/noprovider/noprovider.go +++ b/internal/providers/noprovider/noprovider.go @@ -106,9 +106,14 @@ func (p NoProvider) GetUserInfo(ctx context.Context, accessToken *oauth2.Token, ), nil } +// NormalizeUsername parses a username into a normalized version. +func (p NoProvider) NormalizeUsername(username string) string { + return username +} + // VerifyUsername checks if the requested username matches the authenticated user. func (p NoProvider) VerifyUsername(requestedUsername, username string) error { - if requestedUsername != username { + if p.NormalizeUsername(requestedUsername) != p.NormalizeUsername(username) { return fmt.Errorf("requested username %q does not match the authenticated user %q", requestedUsername, username) } return nil diff --git a/internal/providers/providers.go b/internal/providers/providers.go index 6e6db807..c13e170b 100644 --- a/internal/providers/providers.go +++ b/internal/providers/providers.go @@ -24,5 +24,6 @@ type Provider interface { ) ([]string, error) GetExtraFields(token *oauth2.Token) map[string]interface{} GetUserInfo(ctx context.Context, accessToken *oauth2.Token, idToken *oidc.IDToken) (info.User, error) + NormalizeUsername(username string) string VerifyUsername(requestedUsername, authenticatedUsername string) error } diff --git a/internal/testutils/provider.go b/internal/testutils/provider.go index 4d53c7a5..4f55cfaf 100644 --- a/internal/testutils/provider.go +++ b/internal/testutils/provider.go @@ -373,6 +373,11 @@ func (p *MockProvider) AuthOptions() []oauth2.AuthCodeOption { return p.NoProvider.AuthOptions() } +// NormalizeUsername parses a username into a normalized version. +func (p *MockProvider) NormalizeUsername(username string) string { + return strings.ToLower(username) +} + // GetUserInfo is a no-op when no specific provider is in use. func (p *MockProvider) GetUserInfo(ctx context.Context, accessToken *oauth2.Token, idToken *oidc.IDToken) (info.User, error) { if p.GetUserInfoFails {