Skip to content

Commit

Permalink
fix(PAM&NSS): Generate U(G)ID based on username and update UserPreChe…
Browse files Browse the repository at this point in the history
…ck to return complete user info from broker (#430)

We used to generate the IDs based on the UU(G)IDs and the broker name.
This meant we could only get the right value after the user
authenticated at least once. For cases like SSH, where the user needs to
be pre-checked if it doesn't exist locally, the user info would be
cached and the dummy ID would be used throughout the whole process
instead of the ID generated after authentication. Changing the
generation to consider only the username and moving it to the
internal/users package means we can return the right UID when
pre-checking the user through NSS, avoiding mismatching issues later in
the stack.

UDENG-3528
  • Loading branch information
denisonbarbosa authored Jul 17, 2024
2 parents 27b26db + 9a043b2 commit c148a79
Show file tree
Hide file tree
Showing 29 changed files with 245 additions and 99 deletions.
6 changes: 3 additions & 3 deletions examplebroker/broker.go
Original file line number Diff line number Diff line change
Expand Up @@ -725,11 +725,11 @@ func (b *Broker) CancelIsAuthenticated(ctx context.Context, sessionID string) {
}

// UserPreCheck checks if the user is known to the broker.
func (b *Broker) UserPreCheck(ctx context.Context, username string) error {
func (b *Broker) UserPreCheck(ctx context.Context, username string) (string, error) {
if _, exists := exampleUsers[username]; !exists {
return fmt.Errorf("user %q does not exist", username)
return "", fmt.Errorf("user %q does not exist", username)
}
return nil
return userInfoFromName(username), nil
}

func mapToJSON(input map[string]string) string {
Expand Down
8 changes: 4 additions & 4 deletions examplebroker/dbus.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,10 +126,10 @@ func (b *Bus) CancelIsAuthenticated(sessionID string) (dbusErr *dbus.Error) {
}

// UserPreCheck is the method through which the broker and the daemon will communicate once dbusInterface.UserPreCheck is called.
func (b *Bus) UserPreCheck(username string) (dbusErr *dbus.Error) {
err := b.broker.UserPreCheck(context.Background(), username)
func (b *Bus) UserPreCheck(username string) (userinfo string, dbusErr *dbus.Error) {
userinfo, err := b.broker.UserPreCheck(context.Background(), username)
if err != nil {
return dbus.MakeFailedError(err)
return "", dbus.MakeFailedError(err)
}
return nil
return userinfo, nil
}
22 changes: 6 additions & 16 deletions internal/brokers/broker.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ type brokerer interface {
EndSession(ctx context.Context, sessionID string) (err error)
CancelIsAuthenticated(ctx context.Context, sessionID string)

UserPreCheck(ctx context.Context, username string) (err error)
UserPreCheck(ctx context.Context, username string) (userinfo string, err error)
}

// Broker represents a broker object that can be used for authentication.
Expand Down Expand Up @@ -200,7 +200,7 @@ func (b Broker) IsAuthenticated(ctx context.Context, sessionID, authenticationDa
selectedUsername := b.ongoingUserRequests[sessionID]
b.ongoingUserRequestsMu.Unlock()

u, err := validateUserInfoAndGenerateIDs(b.Name, selectedUsername, info)
u, err := validateUserInfoAndGenerateIDs(selectedUsername, info)
if err != nil {
return "", "", err
}
Expand Down Expand Up @@ -245,7 +245,7 @@ func (b Broker) cancelIsAuthenticated(ctx context.Context, sessionID string) {
}

// UserPreCheck calls the broker corresponding method.
func (b Broker) UserPreCheck(ctx context.Context, username string) (err error) {
func (b Broker) UserPreCheck(ctx context.Context, username string) (userinfo string, err error) {
return b.brokerer.UserPreCheck(ctx, username)
}

Expand Down Expand Up @@ -364,7 +364,7 @@ func unmarshalUserInfo(rawMsg json.RawMessage) (userInfo, error) {
}

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

// Validate username
Expand All @@ -387,7 +387,7 @@ func validateUserInfoAndGenerateIDs(brokerName, selectedUsername string, uInfo u
if uInfo.UUID == "" {
return users.UserInfo{}, fmt.Errorf("empty UUID")
}
uInfo.UID = generateID(brokerName + uInfo.UUID)
uInfo.UID = users.GenerateID(uInfo.Name)

// Validate UGIDs and generate GIDs
for _, g := range uInfo.Groups {
Expand All @@ -396,7 +396,7 @@ func validateUserInfoAndGenerateIDs(brokerName, selectedUsername string, uInfo u
}
var gid *int
if g.UGID != "" {
gidv := generateID(brokerName + g.UGID)
gidv := users.GenerateID(g.UGID)
gid = &gidv
}
uInfo.UserInfo.Groups = append(uInfo.UserInfo.Groups, users.GroupInfo{Name: g.Name, GID: gid})
Expand All @@ -409,16 +409,6 @@ func validateUserInfoAndGenerateIDs(brokerName, selectedUsername string, uInfo u
return uInfo.UserInfo, nil
}

// generatedID generates an integer number based on the provided string.
func generateID(str string) int {
var sum int
for i, c := range str {
// Multiplies the uint value of the rune by its index+1. Subtracts the index to add another layer of conflict prevention.
sum += int(uint(c)*uint(i+1)) - i
}
return (sum % (100000 - 65537)) + 65536 // Ensures that ID is between 65536 and 100000
}

// unmarshalAndGetKey tries to unmarshal the content in data and returns the value of the requested key.
func unmarshalAndGetKey(data, key string) (json.RawMessage, error) {
var returnedData map[string]json.RawMessage
Expand Down
5 changes: 4 additions & 1 deletion internal/brokers/broker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -333,12 +333,15 @@ func TestUserPreCheck(t *testing.T) {
t.Run(name, func(t *testing.T) {
t.Parallel()

err := b.UserPreCheck(context.Background(), tc.username)
got, err := b.UserPreCheck(context.Background(), tc.username)
if tc.wantErr {
require.Error(t, err, "UserPreCheck should return an error, but did not")
return
}
require.NoError(t, err, "UserPreCheck should not return an error, but did")

want := testutils.LoadWithUpdateFromGolden(t, got)
require.Equal(t, want, got, "UserPreCheck should return the expected data, but did not")
})
}
}
Expand Down
10 changes: 7 additions & 3 deletions internal/brokers/dbusbroker.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,13 +136,17 @@ func (b dbusBroker) CancelIsAuthenticated(ctx context.Context, sessionID string)
}

// UserPreCheck calls the corresponding method on the broker bus.
func (b dbusBroker) UserPreCheck(ctx context.Context, username string) (err error) {
func (b dbusBroker) UserPreCheck(ctx context.Context, username string) (userinfo string, err error) {
dbusMethod := DbusInterface + ".UserPreCheck"

call := b.dbusObject.Call(dbusMethod, 0, username)
if err = call.Err; err != nil {
return err
return "", err
}

return nil
if err = call.Store(&userinfo); err != nil {
return "", err
}

return userinfo, nil
}
4 changes: 2 additions & 2 deletions internal/brokers/localbroker.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,6 @@ func (b localBroker) CancelIsAuthenticated(ctx context.Context, sessionID string
}

//nolint:unused // We still need localBroker to implement the brokerer interface, even though this method should never be called on it.
func (b localBroker) UserPreCheck(ctx context.Context, username string) error {
return errors.New("UserPreCheck should never be called on local broker")
func (b localBroker) UserPreCheck(ctx context.Context, username string) (string, error) {
return "", errors.New("UserPreCheck should never be called on local broker")
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
FIRST CALL:
access: granted
data: {"Name":"TestIsAuthenticated/Adds_default_groups_even_if_broker_did_not_set_them_separator_IA_info_empty_groups","UID":98803,"Gecos":"gecos for IA_info_empty_groups","Dir":"/home/IA_info_empty_groups","Shell":"/bin/sh/IA_info_empty_groups","Groups":[{"Name":"TestIsAuthenticated/Adds_default_groups_even_if_broker_did_not_set_them_separator_IA_info_empty_groups","GID":98803}]}
data: {"Name":"TestIsAuthenticated/Adds_default_groups_even_if_broker_did_not_set_them_separator_IA_info_empty_groups","UID":66266,"Gecos":"gecos for IA_info_empty_groups","Dir":"/home/IA_info_empty_groups","Shell":"/bin/sh/IA_info_empty_groups","Groups":[{"Name":"TestIsAuthenticated/Adds_default_groups_even_if_broker_did_not_set_them_separator_IA_info_empty_groups","GID":66266}]}
err: <nil>
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
FIRST CALL:
access: granted
data: {"Name":"TestIsAuthenticated/Error_when_calling_IsAuthenticated_a_second_time_without_cancelling_separator_IA_second_call","UID":69902,"Gecos":"gecos for IA_second_call","Dir":"/home/IA_second_call","Shell":"/bin/sh/IA_second_call","Groups":[{"Name":"TestIsAuthenticated/Error_when_calling_IsAuthenticated_a_second_time_without_cancelling_separator_IA_second_call","GID":69902},{"Name":"group-IA_second_call","GID":69608}]}
data: {"Name":"TestIsAuthenticated/Error_when_calling_IsAuthenticated_a_second_time_without_cancelling_separator_IA_second_call","UID":67508,"Gecos":"gecos for IA_second_call","Dir":"/home/IA_second_call","Shell":"/bin/sh/IA_second_call","Groups":[{"Name":"TestIsAuthenticated/Error_when_calling_IsAuthenticated_a_second_time_without_cancelling_separator_IA_second_call","GID":67508},{"Name":"group-IA_second_call","GID":84647}]}
err: <nil>
SECOND CALL:
access:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
FIRST CALL:
access: granted
data: {"Name":"TestIsAuthenticated/No_error_when_broker_returns_userinfo_with_empty_gecos_separator_IA_info_empty_gecos","UID":92651,"Gecos":"","Dir":"/home/IA_info_empty_gecos","Shell":"/bin/sh/IA_info_empty_gecos","Groups":[{"Name":"TestIsAuthenticated/No_error_when_broker_returns_userinfo_with_empty_gecos_separator_IA_info_empty_gecos","GID":92651},{"Name":"group-IA_info_empty_gecos","GID":92357}]}
data: {"Name":"TestIsAuthenticated/No_error_when_broker_returns_userinfo_with_empty_gecos_separator_IA_info_empty_gecos","UID":89715,"Gecos":"","Dir":"/home/IA_info_empty_gecos","Shell":"/bin/sh/IA_info_empty_gecos","Groups":[{"Name":"TestIsAuthenticated/No_error_when_broker_returns_userinfo_with_empty_gecos_separator_IA_info_empty_gecos","GID":89715},{"Name":"group-IA_info_empty_gecos","GID":96794}]}
err: <nil>
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
FIRST CALL:
access: granted
data: {"Name":"TestIsAuthenticated/No_error_when_broker_returns_userinfo_with_group_with_empty_UGID_separator_IA_info_empty_ugid","UID":88158,"Gecos":"gecos for IA_info_empty_ugid","Dir":"/home/IA_info_empty_ugid","Shell":"/bin/sh/IA_info_empty_ugid","Groups":[{"Name":"TestIsAuthenticated/No_error_when_broker_returns_userinfo_with_group_with_empty_UGID_separator_IA_info_empty_ugid","GID":88158},{"Name":"group-IA_info_empty_ugid","GID":null}]}
data: {"Name":"TestIsAuthenticated/No_error_when_broker_returns_userinfo_with_group_with_empty_UGID_separator_IA_info_empty_ugid","UID":91342,"Gecos":"gecos for IA_info_empty_ugid","Dir":"/home/IA_info_empty_ugid","Shell":"/bin/sh/IA_info_empty_ugid","Groups":[{"Name":"TestIsAuthenticated/No_error_when_broker_returns_userinfo_with_group_with_empty_UGID_separator_IA_info_empty_ugid","GID":91342},{"Name":"group-IA_info_empty_ugid","GID":null}]}
err: <nil>
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
FIRST CALL:
access: granted
data: {"Name":"TestIsAuthenticated/Successfully_authenticate_separator_success","UID":82162,"Gecos":"gecos for success","Dir":"/home/success","Shell":"/bin/sh/success","Groups":[{"Name":"TestIsAuthenticated/Successfully_authenticate_separator_success","GID":82162},{"Name":"group-success","GID":81868}]}
data: {"Name":"TestIsAuthenticated/Successfully_authenticate_separator_success","UID":71705,"Gecos":"gecos for success","Dir":"/home/success","Shell":"/bin/sh/success","Groups":[{"Name":"TestIsAuthenticated/Successfully_authenticate_separator_success","GID":71705},{"Name":"group-success","GID":73580}]}
err: <nil>
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@ FIRST CALL:
err: <nil>
SECOND CALL:
access: granted
data: {"Name":"TestIsAuthenticated/Successfully_authenticate_after_cancelling_first_call_separator_IA_second_call","UID":69902,"Gecos":"gecos for IA_second_call","Dir":"/home/IA_second_call","Shell":"/bin/sh/IA_second_call","Groups":[{"Name":"TestIsAuthenticated/Successfully_authenticate_after_cancelling_first_call_separator_IA_second_call","GID":69902},{"Name":"group-IA_second_call","GID":69608}]}
data: {"Name":"TestIsAuthenticated/Successfully_authenticate_after_cancelling_first_call_separator_IA_second_call","UID":85666,"Gecos":"gecos for IA_second_call","Dir":"/home/IA_second_call","Shell":"/bin/sh/IA_second_call","Groups":[{"Name":"TestIsAuthenticated/Successfully_authenticate_after_cancelling_first_call_separator_IA_second_call","GID":85666},{"Name":"group-IA_second_call","GID":84647}]}
err: <nil>
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"name": "user-pre-check",
"uuid": "uuid-user-pre-check",
"gecos": "gecos for user-pre-check",
"dir": "/home/user-pre-check",
"shell": "/bin/sh/user-pre-check",
"avatar": "avatar for user-pre-check",
"groups": [ {"name": "group-user-pre-check", "ugid": "ugid-user-pre-check"} ]
}
32 changes: 25 additions & 7 deletions internal/services/nss/nss.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package nss

import (
"context"
"encoding/json"
"errors"
"fmt"

Expand Down Expand Up @@ -40,6 +41,7 @@ func (s Service) GetPasswdByName(ctx context.Context, req *authd.GetPasswdByName
if req.GetName() == "" {
return nil, status.Error(codes.InvalidArgument, "no user name provided")
}

u, err := s.userManager.UserByName(req.GetName())
if err == nil {
return nssPasswdFromUsersPasswd(u), nil
Expand All @@ -50,11 +52,12 @@ func (s Service) GetPasswdByName(ctx context.Context, req *authd.GetPasswdByName
}

// If the user is not found in the local cache, we check if it exists in at least one broker.
if err := s.userPreCheck(ctx, req.GetName()); err != nil {
pwent, err := s.userPreCheck(ctx, req.GetName())
if err != nil {
return nil, status.Error(codes.NotFound, err.Error())
}

return nssPasswdFromUsersPasswd(users.UserEntry{Name: req.GetName(), UID: -1, GID: -1}), nil
return pwent, nil
}

// GetPasswdByUID returns the passwd entry for the given UID.
Expand Down Expand Up @@ -157,19 +160,34 @@ func (s Service) GetShadowEntries(ctx context.Context, req *authd.Empty) (*authd
}

// userPreCheck checks if the user exists in at least one broker.
func (s Service) userPreCheck(ctx context.Context, username string) error {
func (s Service) userPreCheck(ctx context.Context, username string) (pwent *authd.PasswdEntry, err error) {
// Check if the user exists in at least one broker.
var userinfo string
for _, b := range s.brokerManager.AvailableBrokers() {
// The local broker is not a real broker, so we skip it.
if b.ID == brokers.LocalBrokerName {
continue
}
if err := b.UserPreCheck(ctx, username); err != nil {
continue

userinfo, err = b.UserPreCheck(ctx, username)
if err == nil && userinfo != "" {
break
}
return nil
}
return fmt.Errorf("user %q is not known by any broker", username)

if err != nil || userinfo == "" {
return nil, fmt.Errorf("user %q is not known by any broker", username)
}

var u users.UserEntry
if err := json.Unmarshal([]byte(userinfo), &u); err != nil {
return nil, fmt.Errorf("user data from broker invalid: %v", err)
}
// We need to generate the ID for the user, as its business logic is authd responsibility, not the broker's.
u.UID = users.GenerateID(u.Name)
u.GID = u.UID

return nssPasswdFromUsersPasswd(u), nil
}

// nssPasswdFromUsersPasswd returns a PasswdEntry from users.UserEntry.
Expand Down
6 changes: 4 additions & 2 deletions internal/services/nss/nss_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,10 @@ func TestGetPasswdByName(t *testing.T) {
wantErr bool
wantErrNotExists bool
}{
"Return existing user": {username: "user1"},
"Precheck user if not in cache": {username: "user-pre-check", shouldPreCheck: true},
"Return existing user": {username: "user1"},

"Precheck user if not in cache": {username: "user-pre-check", shouldPreCheck: true},
"Prechecked user with upper cases in username has same id as lower case": {username: "User-Pre-Check", shouldPreCheck: true},

"Error in database fetched content": {username: "user1", sourceDB: "invalid.db.yaml", wantErr: true},
"Error with typed GRPC notfound code on unexisting user": {username: "does-not-exists", wantErr: true, wantErrNotExists: true},
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name: user-pre-check
passwd: x
uid: 4294967295
gid: 4294967295
gecos: ""
homedir: ""
shell: ""
uid: 75590
gid: 75590
gecos: gecos for user-pre-check
homedir: /home/user-pre-check
shell: /bin/sh/user-pre-check
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
name: User-Pre-Check
passwd: x
uid: 75590
gid: 75590
gecos: gecos for User-Pre-Check
homedir: /home/User-Pre-Check
shell: /bin/sh/User-Pre-Check
39 changes: 39 additions & 0 deletions internal/services/pam/pam_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -532,6 +532,45 @@ func TestIsAuthenticated(t *testing.T) {
}
}

func TestIDGeneration(t *testing.T) {
t.Parallel()
usernamePrefix := t.Name()

tests := map[string]struct {
username string
}{
"Generate ID": {username: "success"},
"Generates same ID if user has upper cases in username": {username: "SuCcEsS"},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
t.Parallel()

m, err := users.NewManager(t.TempDir())
require.NoError(t, err, "Setup: could not create user manager")
t.Cleanup(func() { _ = m.Stop() })
pm := newPermissionManager(t, false) // Allow starting the session (current user considered root)
client := newPamClient(t, m, &pm)

sbResp, err := client.SelectBroker(context.Background(), &authd.SBRequest{
BrokerId: mockBrokerGeneratedID,
Username: usernamePrefix + testutils.IDSeparator + tc.username,
Mode: authd.SessionMode_AUTH,
})
require.NoError(t, err, "Setup: failed to create session for tests")

resp, err := client.IsAuthenticated(context.Background(), &authd.IARequest{SessionId: sbResp.GetSessionId()})
require.NoError(t, err, "Setup: could not authenticate user")
require.Equal(t, "granted", resp.GetAccess(), "Setup: authentication should be granted")

gotDB, err := cachetestutils.DumpToYaml(userstestutils.GetManagerCache(m))
require.NoError(t, err, "Setup: failed to dump database for comparing")
wantDB := testutils.LoadWithUpdateFromGolden(t, gotDB, testutils.WithGoldenPath(filepath.Join(testutils.GoldenPath(t), "cache.db")))
require.Equal(t, wantDB, gotDB, "IsAuthenticated should update the cache database as expected")
})
}
}

func TestSetDefaultBrokerForUser(t *testing.T) {
t.Parallel()

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
GroupByID:
"73580": '{"Name":"group-success","GID":73580}'
"94411": '{"Name":"TestIDGeneration_separator_success","GID":94411}'
GroupByName:
TestIDGeneration_separator_success: '{"Name":"TestIDGeneration_separator_success","GID":94411}'
group-success: '{"Name":"group-success","GID":73580}'
GroupToUsers:
"73580": '{"GID":73580,"UIDs":[94411]}'
"94411": '{"GID":94411,"UIDs":[94411]}'
UserByID:
"94411": '{"Name":"TestIDGeneration_separator_success","UID":94411,"GID":94411,"Gecos":"gecos for success","Dir":"/home/success","Shell":"/bin/sh/success","LastPwdChange":-1,"MaxPwdAge":-1,"PwdWarnPeriod":-1,"PwdInactivity":-1,"MinPwdAge":-1,"ExpirationDate":-1,"LastLogin":"ABCDETIME"}'
UserByName:
TestIDGeneration_separator_success: '{"Name":"TestIDGeneration_separator_success","UID":94411,"GID":94411,"Gecos":"gecos for success","Dir":"/home/success","Shell":"/bin/sh/success","LastPwdChange":-1,"MaxPwdAge":-1,"PwdWarnPeriod":-1,"PwdInactivity":-1,"MinPwdAge":-1,"ExpirationDate":-1,"LastLogin":"ABCDETIME"}'
UserToBroker: {}
UserToGroups:
"94411": '{"UID":94411,"GIDs":[94411,73580]}'
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
GroupByID:
"73580": '{"Name":"group-SuCcEsS","GID":73580}'
"94411": '{"Name":"TestIDGeneration_separator_SuCcEsS","GID":94411}'
GroupByName:
TestIDGeneration_separator_SuCcEsS: '{"Name":"TestIDGeneration_separator_SuCcEsS","GID":94411}'
group-SuCcEsS: '{"Name":"group-SuCcEsS","GID":73580}'
GroupToUsers:
"73580": '{"GID":73580,"UIDs":[94411]}'
"94411": '{"GID":94411,"UIDs":[94411]}'
UserByID:
"94411": '{"Name":"TestIDGeneration_separator_SuCcEsS","UID":94411,"GID":94411,"Gecos":"gecos for SuCcEsS","Dir":"/home/SuCcEsS","Shell":"/bin/sh/SuCcEsS","LastPwdChange":-1,"MaxPwdAge":-1,"PwdWarnPeriod":-1,"PwdInactivity":-1,"MinPwdAge":-1,"ExpirationDate":-1,"LastLogin":"ABCDETIME"}'
UserByName:
TestIDGeneration_separator_SuCcEsS: '{"Name":"TestIDGeneration_separator_SuCcEsS","UID":94411,"GID":94411,"Gecos":"gecos for SuCcEsS","Dir":"/home/SuCcEsS","Shell":"/bin/sh/SuCcEsS","LastPwdChange":-1,"MaxPwdAge":-1,"PwdWarnPeriod":-1,"PwdInactivity":-1,"MinPwdAge":-1,"ExpirationDate":-1,"LastLogin":"ABCDETIME"}'
UserToBroker: {}
UserToGroups:
"94411": '{"UID":94411,"GIDs":[94411,73580]}'
Loading

0 comments on commit c148a79

Please sign in to comment.