Skip to content

Commit

Permalink
Don't check if username matches the requested username
Browse files Browse the repository at this point in the history
It's the broker's responsibility to do that and authd doesn't know which
usernames the provider considers equal, for example if they are
case-sensitive or not.
  • Loading branch information
adombeck committed Sep 4, 2024
1 parent 5e33826 commit e91ab76
Show file tree
Hide file tree
Showing 12 changed files with 16 additions and 629 deletions.
15 changes: 5 additions & 10 deletions internal/brokers/broker.go
Original file line number Diff line number Diff line change
Expand Up @@ -194,11 +194,7 @@ func (b Broker) IsAuthenticated(ctx context.Context, sessionID, authenticationDa
return "", "", err
}

b.ongoingUserRequestsMu.Lock()
selectedUsername := b.ongoingUserRequests[sessionID]
b.ongoingUserRequestsMu.Unlock()

u, err := validateUserInfoAndGenerateIDs(selectedUsername, info)
u, err := validateUserInfoAndGenerateIDs(info)
if err != nil {
return "", "", err
}
Expand Down Expand Up @@ -362,16 +358,15 @@ func unmarshalUserInfo(rawMsg json.RawMessage) (userInfo, error) {
}

// validateUserInfoAndGenerateIDs checks if the specified userinfo is valid and generates the UID and GIDs.
func validateUserInfoAndGenerateIDs(selectedUsername string, uInfo userInfo) (user users.UserInfo, err error) {
func validateUserInfoAndGenerateIDs(uInfo userInfo) (user users.UserInfo, err error) {
defer decorate.OnError(&err, "provided userinfo is invalid")

// Validate username
// Validate username. We don't want to check here if it matches the username from the request, because it's the
// broker's responsibility to do that and we don't know which usernames the provider considers equal, for example if
// they are case-sensitive or not.
if uInfo.Name == "" {
return users.UserInfo{}, fmt.Errorf("empty username")
}
if uInfo.Name != selectedUsername {
return users.UserInfo{}, fmt.Errorf("username %q does not match the selected username %q", uInfo.Name, selectedUsername)
}

// Validate home and shell directories
if !filepath.IsAbs(filepath.Clean(uInfo.Dir)) {
Expand Down
2 changes: 1 addition & 1 deletion internal/brokers/broker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ func TestIsAuthenticated(t *testing.T) {
"No error when auth.Next and no data": {sessionID: "IA_next"},
"No error when broker returns userinfo with empty gecos": {sessionID: "IA_info_empty_gecos"},
"No error when broker returns userinfo with group with empty UGID": {sessionID: "IA_info_empty_ugid"},
"No error when broker returns userinfo with mismatching username": {sessionID: "IA_info_mismatching_user_name"},

// broker errors
"Error when authenticating": {sessionID: "IA_error"},
Expand All @@ -227,7 +228,6 @@ func TestIsAuthenticated(t *testing.T) {
"Error when broker returns invalid access": {sessionID: "IA_invalid_access"},
"Error when broker returns invalid userinfo": {sessionID: "IA_invalid_userinfo"},
"Error when broker returns userinfo with empty username": {sessionID: "IA_info_empty_user_name"},
"Error when broker returns userinfo with mismatching username": {sessionID: "IA_info_mismatching_user_name"},
"Error when broker returns userinfo with empty group name": {sessionID: "IA_info_empty_group_name"},
"Error when broker returns userinfo with empty UUID": {sessionID: "IA_info_empty_uuid"},
"Error when broker returns userinfo with invalid homedir": {sessionID: "IA_info_invalid_home"},
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
FIRST CALL:
access: granted
data: {"Name":"different_username","UID":83626,"Gecos":"gecos for IA_info_mismatching_user_name","Dir":"/home/IA_info_mismatching_user_name","Shell":"/bin/sh/IA_info_mismatching_user_name","Groups":[{"Name":"different_username","GID":83626},{"Name":"group-IA_info_mismatching_user_name","GID":92849}]}
err: <nil>
13 changes: 6 additions & 7 deletions internal/services/pam/pam_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -437,13 +437,12 @@ func TestIsAuthenticated(t *testing.T) {
"Error when there is no broker": {sessionID: "invalid-session"},

// broker errors
"Error when authenticating": {username: "IA_error"},
"Error on empty data even if granted": {username: "IA_empty_data"},
"Error when broker returns invalid access": {username: "IA_invalid_access"},
"Error when broker returns invalid data": {username: "IA_invalid_data"},
"Error when broker returns invalid userinfo": {username: "IA_invalid_userinfo"},
"Error when broker returns username different than the one selected": {username: "IA_info_mismatching_user_name"},
"Error when calling second time without cancelling": {username: "IA_second_call", secondCall: true},
"Error when authenticating": {username: "IA_error"},
"Error on empty data even if granted": {username: "IA_empty_data"},
"Error when broker returns invalid access": {username: "IA_invalid_access"},
"Error when broker returns invalid data": {username: "IA_invalid_data"},
"Error when broker returns invalid userinfo": {username: "IA_invalid_userinfo"},
"Error when calling second time without cancelling": {username: "IA_second_call", secondCall: true},

// local group error
"Error on updating local groups with unexisting file": {username: "success_with_local_groups", localGroupsFile: "does_not_exists.group"},
Expand Down

This file was deleted.

This file was deleted.

1 change: 0 additions & 1 deletion pam/integration-tests/cli_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ func TestCLIAuthenticate(t *testing.T) {

"Deny authentication if max attempts reached": {tape: "max_attempts"},
"Deny authentication if user does not exist": {tape: "unexistent_user"},
"Deny authentication if usernames dont match": {tape: "mismatch_username"},
"Deny authentication if newpassword does not match required criteria": {tape: "bad_password"},

"Exit authd if local broker is selected": {tape: "local_broker"},
Expand Down
1 change: 0 additions & 1 deletion pam/integration-tests/native_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ func TestNativeAuthenticate(t *testing.T) {

"Deny authentication if max attempts reached": {tape: "max_attempts"},
"Deny authentication if user does not exist": {tape: "unexistent_user"},
"Deny authentication if usernames dont match": {tape: "mismatch_username"},
"Deny authentication if newpassword does not match required criteria": {tape: "bad_password"},

"Exit authd if local broker is selected": {tape: "local_broker"},
Expand Down

This file was deleted.

Loading

0 comments on commit e91ab76

Please sign in to comment.