Skip to content

Commit

Permalink
Fix regression in getting sid_info for local users (#14488)
Browse files Browse the repository at this point in the history
While fixing an unrelated bug that affected LDAP failover, a restart of
the winbind service was removed from the directory services setup
method. Changes to directory services health checks in electric eel
improved error recovery and eliminated the need for this step.

Unfortunately, this removal exposed that user.get_user_obj and
group.get_group_obj were still relying on winbind to perform SID
resolution (which failed because winbind had not started yet on the
server).

As of electric eel, the SID values for local accounts are derived from
the `id` value of the user / group entry in their respective database
table. This commit robustizes SID lookups for local accounts through
get_obj methods by retrieving via user.query and group.query.
  • Loading branch information
anodos325 authored Sep 11, 2024
1 parent 3c00bcd commit 923ac36
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 30 deletions.
64 changes: 36 additions & 28 deletions src/middlewared/middlewared/plugins/account.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@
from middlewared.plugins.idmap_.idmap_constants import (
BASE_SYNTHETIC_DATASTORE_ID,
IDType,
SID_LOCAL_USER_PREFIX,
SID_LOCAL_GROUP_PREFIX
)
from middlewared.plugins.idmap_ import idmap_winbind
from middlewared.plugins.idmap_ import idmap_sss
Expand Down Expand Up @@ -1022,9 +1020,22 @@ def get_user_obj(self, data):
user_obj['grouplist'] = None

if data['sid_info']:
sid = None
match user_obj['source']:
case 'LOCAL' | 'ACTIVEDIRECTORY':
# winbind provides idmapping for local and AD users
case 'LOCAL':
idmap_ctx = None
db_entry = self.middleware.call_sync('user.query', [[
'username', '=', user_obj['pw_name']
]], {'select': ['sid']})
if not db_entry:
self.logger.error(
'%s: local user exists on server but does not exist in the '
'the user account table.', user_obj['pw_name']
)
else:
sid = db_entry[0]['sid']
case 'ACTIVEDIRECTORY':
# winbind provides idmapping for AD users
try:
idmap_ctx = idmap_winbind.WBClient()
except wbclient.WBCError as e:
Expand All @@ -1047,20 +1058,9 @@ def get_user_obj(self, data):
'id': user_obj['pw_uid']
})['sid']
except MatchNotFound:
if user_obj['source'] == 'LOCAL':
# Local user that doesn't have passdb entry
# we can simply apply default prefix
sid = SID_LOCAL_USER_PREFIX + str(user_obj['pw_uid'])
else:
# This is a more odd situation. The user accout exists
# in IPA but doesn't have a SID assigned to it.
sid = None
else:
# We were unable to establish an idmap client context even
# though we were able to retrieve the user account info. This
# most likely means that we're dealing with a local account and
# winbindd is not running.
sid = None
# This is a more odd situation. Most likely case is that the user account exists
# in IPA but doesn't have a SID assigned to it. All AD users have SIDs.
sid = None

user_obj['sid'] = sid
else:
Expand Down Expand Up @@ -1933,9 +1933,23 @@ def get_group_obj(self, data):
grp_obj['local'] = grp_obj['source'] == 'LOCAL'

if data['sid_info']:
sid = None

match grp_obj['source']:
case 'LOCAL' | 'ACTIVEDIRECTORY':
# winbind provides idmapping for local and AD users
case 'LOCAL':
idmap_ctx = None
db_entry = self.middleware.call_sync('group.query', [[
'group', '=', grp_obj['gr_name']
]], {'select': ['sid']})
if not db_entry:
self.logger.error(
'%s: local group exists on server but does not exist in the '
'the group account table.', grp_obj['gr_name']
)
else:
sid = db_entry[0]['sid']
case 'ACTIVEDIRECTORY':
# winbind provides idmapping for AD groups
try:
idmap_ctx = idmap_winbind.WBClient()
except wbclient.WBCError as e:
Expand All @@ -1962,14 +1976,8 @@ def get_group_obj(self, data):
'id': grp_obj['gr_gid']
})['sid']
except MatchNotFound:
if grp_obj['source'] == 'LOCAL':
# Local user that doesn't have groupmap entry
# we can simply apply default prefix
sid = SID_LOCAL_GROUP_PREFIX + str(grp_obj['gr_gid'])
else:
sid = None
else:
sid = None
# This can happen if IPA and group doesn't have SID assigned
sid = None

grp_obj['sid'] = sid
else:
Expand Down
7 changes: 6 additions & 1 deletion src/middlewared/middlewared/plugins/smb.py
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,12 @@ async def configure(self, job, create_paths=True):
job.set_progress(70, 'Checking SMB server status.')
if await self.middleware.call("service.started_or_enabled", "cifs"):
job.set_progress(80, 'Restarting SMB service.')
await self.middleware.call("service.restart", "cifs")
await self.middleware.call("service.restart", "cifs", {"ha_propagate": False})

# Ensure that winbind is running once we configure SMB service
if not await self.middleware.call('service.started', 'idmap'):
await self.middleware.call('service.start', 'idmap', {'ha_propagate': False})

job.set_progress(100, 'Finished configuring SMB.')

@private
Expand Down
2 changes: 1 addition & 1 deletion tests/api2/test_011_user.py
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ def test_002_verify_user_exists_in_pwd(request):
assert pw['pw_dir'] == VAR_EMPTY

# At this point, we're not an SMB user
assert pw['sid'] is not None
assert pw['sid'] is None
assert pw['source'] == 'LOCAL'
assert pw['local'] is True

Expand Down

0 comments on commit 923ac36

Please sign in to comment.