Bug 48177

AccountsService sort of assumes users in /etc/passwd are "local users"  and that the 40000 user network login cases are covered by nis/ldap/etc

I guess we should do some sort of sanity check and treat /etc/passwd users as remote in the case the check fails.  This way we sidestep all the costly work entirely.

maybe we could just bail if there are greater than N found users.  The problem then is finding a suitable value of N.

Created attachment 65941 [details] [review]
proposed patch

limit the no of user listed by a-s to UID_MAX. Also, sets user-limit to true when this condition is meet, allowing apps like gdm/lightdm to enable manual login option.

Talked with Ritesh about this bug today on IRC.  He's going to look into using fgetgrent to create the hash table of local groups and use that.

Created attachment 66171 [details] [review]
proposed patch

ritz, any chance you could get this applying against master (preferably as a git-format-patch formatted patch) ?

any updates on this?

Created attachment 73763 [details] [review]
patch

Hi halfline

  I have rewritten the code to use hashtable instead of list, leading to lot faster startup time on my test system ( <1s for ~40k users). I am currently testing the code ( add/remove users option ). This is for an older version of a-s (0.6.15) , will update the code against head and post. 

  Anything which does need the user list ( gnome-control-center user manager/ indicator-session/..) still breaks . 

  Personally I believe, the best solution would be to allow user to select an upper limit ( and list only cached users from login history ), and broadcast "user-limit-reached" option. The broadcast could be used by applications to display warning or change their behaviour ( such as disable user list in gdm ).

Created attachment 74177 [details] [review]
patch

initial patch, against latest release.

still need to work quirk against entry_generator_wtmp. It calls getpwnam for every entry ( even if it is a duplicate entry ).

any chance you could update this again for git master?

Created attachment 124799 [details] [review]
user: check if user is in wheel more efficiently

We currently get all the groups a user belongs to in one pass,
then check each one to see if it's wheel.

It's much more efficient to just get the wheel group and check if
any of its members are the user.

Created attachment 124800 [details] [review]
daemon: get local users from /etc/shadow not /etc/passwd

For some sites, it's common practice to rsync around large
/etc/passwd files containing the password entries for remote
users. That means accountsservices' "assume /etc/passwd is local
users" heuristic falls over.

This commit changes it to only treat users in /etc/shadow as local.

Created attachment 124801 [details] [review]
daemon: don't call getspnam for local users

We're already iterating over the whole shadow file, so
just cache the entries instead of calling getspname a
few lines later.

Created attachment 124802 [details] [review]
daemon: constrain max local users to 50

Systems with tens of thousands of users don't want all those users
showing up in the user list.

Set a cap at an even 50, which should cover the lion's share of use
cases well. Of course, if a user not in the list explicitly
logs in (from Not Listed? or whatever) they get added to the list.

Created attachment 124803 [details] [review]
daemon: don't source user list from wtmp

wtmp can get rather large on some systems from ssh logins.
Furthermore it's pretty much completely redundant given the user
cache in /var/lib/AccountService

This commit changes the wtmp code to only get used for maintaining
login frequency and accounting, not for generating new users.

i'm going to push these patches to master.  They're loosely based on ritz work in this bug, but they diverge in some significant ways:

1) i set a limit on the number of local users returned (ala comment 2)
2) i redid the wheel check to avoid needing an admin user cache
3) i cache shadow information a little differently and for a shorter period of time
4) i get "local user list" from /etc/shadow now instead of /etc/passwd (since /etc/passwd gets rsynced around)

Attachment 124799 [details] pushed as c85b41d - user: check if user is in wheel more efficiently
Attachment 124800 [details] pushed as 98f4287 - daemon: get local users from /etc/shadow not /etc/passwd
Attachment 124801 [details] pushed as 14ca424 - daemon: don't call getspnam for local users
Attachment 124802 [details] pushed as 20c0315 - daemon: constrain max local users to 50
Attachment 124803 [details] pushed as a2f448b - daemon: don't source user list from wtmp

Unconditioned #include <shadow.h> breaks the build on FreeBSD.

FreeBSD doesn't have shadow.h and /etc/shadow. Hashed passwords are stored in /etc/master.passwd, which is a /etc/passwd with additional fields. There is no API specifically made to read /etc/master.passwd because getpwent on FreeBSD automatically switches to use /etc/spwd.db (the binary form of /etc/master.passwd) instead of /etc/pwd.db (the binary form of /etc/passwd) when the user has permission to read the file.

Same on OpenBSD. Since that change, accountsservice is broken here as well.

is it common practice on BSD to rsync /etc/passwd around? Maybe bsd could go back to using /etc/passwd?

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/accountsservice/accountsservice/issues/39.