fix: workaround for undocumented caching in getpw/getgr funcs

bin/plugin/open/groupList

@@ -36,7 +36,7 @@ EOF
 
 my $fnret;
 
-$fnret = OVH::Bastion::get_group_list(groupType => "key");
+$fnret = OVH::Bastion::get_group_list();
 $fnret or osh_exit $fnret;
 
 my $includere = OVH::Bastion::build_re_from_wildcards(wildcards => \@includes, implicit_contains => 1)->value;

bin/plugin/open/info

@@ -118,9 +118,9 @@ osh_info sprintf("\nI am %s, aka %s", colored(Sys::Hostname::hostname(), 'green'
 $ret{'hostname'}     = Sys::Hostname::hostname();
 $ret{'bastion_name'} = $config->{'bastionName'};
 
-$fnret = OVH::Bastion::get_account_list();
+$fnret = OVH::Bastion::get_account_list(cache => 1);
 my $nbaccounts = $fnret ? keys %{$fnret->value} : '?';
-$fnret = OVH::Bastion::get_group_list(groupType => 'key');
+$fnret = OVH::Bastion::get_group_list(cache => 1);
 my $nbgroups = $fnret ? keys %{$fnret->value} : '?';
 osh_info "I have " . colored($nbaccounts, 'green') . " registered accounts and " . colored($nbgroups, 'green') . " groups";
 $ret{'registered_accounts'} = $nbaccounts;

bin/plugin/restricted/accountInfo

@@ -68,18 +68,18 @@ $ret{'allowed_commands'} = \@granted;
 my $result_hash = {};
 
 if ($listGroups) {
-    $fnret = OVH::Bastion::get_group_list(groupType => "key");
+    $fnret = OVH::Bastion::get_group_list(cache => 1);
     $fnret or osh_exit $fnret;
 
     osh_info "\nThis account is part of the following groups:";
 
     foreach my $name (sort keys %{$fnret->value}) {
         my @flags;
-        push @flags, 'owner' if OVH::Bastion::is_group_owner(group => $name, account => $account);
+        push @flags, 'owner' if OVH::Bastion::is_group_owner(group => $name, account => $account, cache => 1);
-        push @flags, 'gatekeeper' if OVH::Bastion::is_group_gatekeeper(group => $name, account => $account);
+        push @flags, 'gatekeeper' if OVH::Bastion::is_group_gatekeeper(group => $name, account => $account, cache => 1);
-        push @flags, 'aclkeeper' if OVH::Bastion::is_group_aclkeeper(group => $name, account => $account);
+        push @flags, 'aclkeeper' if OVH::Bastion::is_group_aclkeeper(group => $name, account => $account, cache => 1);
-        push @flags, 'member' if OVH::Bastion::is_group_member(group => $name, account => $account);
+        push @flags, 'member' if OVH::Bastion::is_group_member(group => $name, account => $account, cache => 1);
-        push @flags, 'guest' if OVH::Bastion::is_group_guest(group => $name, account => $account);
+        push @flags, 'guest' if OVH::Bastion::is_group_guest(group => $name, account => $account, cache => 1);
         if (@flags) {
             my $line = sprintf "%18s", $name;
             $line .= sprintf " %14s", colored(grep({ $_ eq 'owner' } @flags)      ? 'Owner'      : '-', 'red');

lib/perl/OVH/Bastion.pm

@@ -141,7 +141,7 @@ my %_autoload_files = (
         qw{ enable_mocking is_mocking set_mock_data mock_get_account_entry mock_get_account_accesses mock_get_account_personal_accesses mock_get_account_legacy_accesses mock_get_group_accesses mock_get_account_guest_accesses }
     ],
     os => [
-        qw{ sysinfo is_linux is_debian is_redhat is_bsd is_freebsd is_openbsd is_netbsd has_acls sys_useradd sys_groupadd sys_userdel sys_groupdel sys_addmembertogroup sys_delmemberfromgroup sys_changepassword sys_neutralizepassword sys_setpasswordpolicy sys_getpasswordinfo sys_getsudoersfolder sys_setfacl is_in_path }
+        qw{ sysinfo is_linux is_debian is_redhat is_bsd is_freebsd is_openbsd is_netbsd has_acls sys_useradd sys_groupadd sys_userdel sys_groupdel sys_addmembertogroup sys_delmemberfromgroup sys_changepassword sys_neutralizepassword sys_setpasswordpolicy sys_getpasswordinfo sys_getsudoersfolder sys_setfacl is_in_path sys_getent_pw sys_getent_gr }
     ],
     password => [qw{ get_hashes_from_password get_hashes_list is_valid_hash }],
     ssh      => [

lib/perl/OVH/Bastion/allowdeny.inc

@@ -340,9 +340,8 @@ sub ip2host {
     return R('OK', value => $host);
 }
 
-# Return an array containing the groups for which user is a member of
+# returns a list of the bastion groups (more precisely, the system group names
-my %_cache_get_user_groups;
+# mapped to these groups) a user is member of.
-
 sub get_user_groups {
     my %params = @_;
     my $user   = $params{'user'} || $params{'account'};
@@ -353,19 +352,16 @@ sub get_user_groups {
         return R('ERR_MISSING_PARAMETER', msg => "Missing parameter 'account'");
     }
 
-    if (not %_cache_get_user_groups) {
+    # try to use sys_getent_gr()'s cache if available and allowed by caller.
-
+    # we loop through all the system groups to find the ones having user
-        # build cache, it'll be faster than even one exec `id -nG` anyway
+    # as a meember
-        setgrent();
+    my @groups;
-        while (my ($name, $passwd, $gid, $members) = getgrent()) {
+    foreach my $gr (@{OVH::Bastion::sys_getent_gr(cache => $cache)->value}) {
-            foreach my $member (split / /, $members) {
+        if (grep { $user eq $_ } @{$gr->{'members'} || []}) {
-                push @{$_cache_get_user_groups{$member}}, $name;
+            push @groups, $gr->{'name'};
-            }
         }
-        setgrent();
     }
 
-    my @groups = @{$_cache_get_user_groups{$user} || []};
     my @availableGroups;
     foreach my $group (@groups) {
         if ($group =~ /^key.+-(gatekeeper|aclkeeper|owner)$/) {

lib/perl/OVH/Bastion/allowkeeper.inc

@@ -6,61 +6,64 @@ use common::sense;
 
 use Time::Piece;    # $t->strftime
 
-# used to speedup extensive calls to is_user_in_group() and is_group_existing(),
+# Check if a system user belongs to a specific system group
-# if our caller allows us:
-my %_cache_getgrnam;
-
-# Check if user belongs to a specific group
 sub is_user_in_group {
     my %params = @_;
     my $group  = $params{'group'};
     my $user   = $params{'user'} || OVH::Bastion::get_user_from_env()->value;
-    my $cache  = $params{'cache'};                                              # if true, allow cache use from potential previous calls
+    my $cache  = $params{'cache'};                                              # if true, allow cache use of sys_getent_gr()
 
     # mandatory keys
     if (!$user || !$group) {
         return R('ERR_MISSING_PARAMETER', msg => "Missing parameter 'user' or 'group'");
     }
 
-    # if we're not allowed to use cache, call getgrnam() in all cases.
+    # try to use sys_getent_gr()'s cache if available and allowed by caller.
-    # if we're allowed to use it; only call getgrnam() if we have no cache for this group
+    # we loop through all the system groups to find the proper one, then
-    if (!$cache || (!$_cache_getgrnam{$group} || ref $_cache_getgrnam{$group} ne 'ARRAY')) {
+    # check whether $user appears in the member list
-        $_cache_getgrnam{$group} = [(getgrnam($group))];
+    foreach my $gr (@{OVH::Bastion::sys_getent_gr(cache => $cache)->value}) {
+        next if $gr->{'name'} ne $group;
+        if (grep { $user eq $_ } @{$gr->{'members'} || []}) {
+            return R('OK', value => {account => $user});
+        }
+        else {
+            return R('KO_NOT_IN_GROUP', msg => "Account $user doesn't belong to the group $group");
+        }
     }
-    my ($groupname, $passwd, $gid, $members) = @{$_cache_getgrnam{$group}};
+    return R('KO_GROUP_NOT_FOUND', msg => "The group $group doesn't exist");
-    my @membersList = split / /, $members;
-
-    if (grep { $user eq $_ } @membersList) {
-        return R('OK', value => {account => $user});
-    }
-    return R('KO_NOT_IN_GROUP', msg => "Account $user doesn't belong to the group $group");
 }
 
-# does this group exist ?
+# does this system group exist ? if it happens to be mapped to a bastion group,
-# uses %_cache_getgrnam defined above
+# also return the corresponding "shortGroup" (with the "key" prefix removed)
 sub is_group_existing {
-    my %params = @_;
+    my %params              = @_;
-    my $group  = $params{'group'};
+    my $group               = $params{'group'};
-    my $cache  = $params{'cache'};    # if true, allow cache use from potential previous calls
+    my $cache               = $params{'cache'};                 # if true, allow cache use from potential previous calls
+    my $user_friendly_error = $params{'user_friendly_error'};
 
     if (!$group) {
         return R('ERR_MISSING_PARAMETER', msg => "Missing parameter 'group'");
     }
 
-    # if we're not allowed to use cache, call getgrnam() in all cases.
+    # try to use sys_getent_gr()'s cache if available and allowed by caller.
-    # if we're allowed to use it; only call getgrnam() if we have no cache for this group
+    # we loop through all the system groups to find the proper one
-    if (!$cache || (!$_cache_getgrnam{$group} || ref $_cache_getgrnam{$group} ne 'ARRAY')) {
+    foreach my $gr (@{OVH::Bastion::sys_getent_gr(cache => $cache)->value}) {
-        $_cache_getgrnam{$group} = [(getgrnam($group))];
+        next if $gr->{'name'} ne $group;
-    }
-    my ($groupname, $passwd, $gid, $members) = @{$_cache_getgrnam{$group}};
-    if ($groupname) {
         my (undef, $shortGroup) = $group =~ m{^(key)?(.+)};
-        return R('OK', value => {group => $group, shortGroup => $shortGroup, gid => $gid, keyhome => "/home/keykeeper/$group", members => [split(/ /, $members)]});
+        return R(
+            'OK',
+            value => {
+                group      => $group,
+                shortGroup => $shortGroup,
+                gid        => $gr->{'gid'},
+                keyhome    => "/home/keykeeper/$group",
+                members    => $gr->{'members'},
+            }
+        );
     }
 
-    if ($params{'user_friendly_error'}) {
+    # build a user-compatible error message if asked to, as it can make its way through osh_exit()
-
+    if ($user_friendly_error) {
-        # build a user-compatible error message, as it can make its way through osh_exit()
         $group =~ s/^key//;
         return R('KO_GROUP_NOT_FOUND', msg => "The bastion group '$group' doesn't exist.\nYou may use groupList --all to see all existing groups.");
     }
@@ -244,42 +247,52 @@ sub is_account_valid {
     return R('ERR_IMPOSSIBLE_CASE');
 }
 
-# check that this account is present on the bastion
-# it also returns untainted data, including splitting by realm where applicable
-my %_cache_getpwnam;
-
 sub is_account_existing {
     my %params            = @_;
     my $account           = $params{'account'};
-    my $checkBastionShell = $params{'checkBastionShell'};    # check if this account is a bastion user
+    my $checkBastionShell = $params{'checkBastionShell'};                                                             # check if this account is a bastion user
-    my $cache             = $params{'cache'};                # allow cache use to speedup commands that don't modify accounts but may call us extensively, i.e. groupList
+    my $cache             = $params{'cache'};                                                                         # allow cache use
 
     if (!$account) {
         return R('ERR_MISSING_PARAMETER', msg => "Missing parameter 'account'");
     }
 
-    # if we're not allowed to use cache, call getpwnam() in all cases.
+    my %entry;
-    # if we're allowed to use it; only call getpwnam() if we have no cache for this account
-    if (!$cache || (!$_cache_getpwnam{$account} || ref $_cache_getpwnam{$account} ne 'ARRAY')) {
-        $_cache_getpwnam{$account} = [(getpwnam($account))];
-    }
-    my ($name, $passwd, $uid, $gid, $quota, $comment, $gcos, $dir, $shell, $expire) = @{$_cache_getpwnam{$account}};
     if (OVH::Bastion::is_mocking()) {
-        ($name, $passwd, $uid, $gid, $gcos, $dir, $shell) = OVH::Bastion::mock_get_account_entry(account => $account);
+        my @fields = OVH::Bastion::mock_get_account_entry(account => $account);
+        %entry = (
+            name   => $fields[0],
+            passwd => $fields[1],
+            uid    => $fields[2],
+            gid    => $fields[3],
+            gcos   => $fields[4],
+            dir    => $fields[5],
+            shell  => $fields[6],
+        );
+    }
+    else {
+        # try to use sys_getent_pw()'s cache if available and allowed by caller.
+        # we loop through all the system accounts to find the proper one
+        foreach my $pw (@{OVH::Bastion::sys_getent_pw(cache => $cache)->value}) {
+            next if $pw->{'name'} ne $account;
+            %entry = %$pw;
+            last;
+        }
     }
-    if ($name) {
-        my ($newname) = $name =~ m{([a-zA-Z0-9._-]+)};
-        return R('ERR_SECURITY_VIOLATION', msg => "Forbidden characters in account name") if ($newname ne $name);
-        $name = $newname;    # untaint
 
-        if ($checkBastionShell && $shell ne $OVH::Bastion::BASEPATH . "/bin/shell/osh.pl") {
+    if (%entry) {
+        my ($newname) = $entry{'name'} =~ m{([a-zA-Z0-9._-]+)};
+        return R('ERR_SECURITY_VIOLATION', msg => "Forbidden characters in account name") if ($newname ne $entry{'name'});
+        $entry{'name'} = $newname;    # untaint
+
+        if ($checkBastionShell && $entry{'shell'} ne $OVH::Bastion::BASEPATH . "/bin/shell/osh.pl") {
             return R('KO_NOT_FOUND', msg => "Account '$account' doesn't exist");    # msg is the same as below, voluntarily
         }
 
-        my ($newdir) = $dir =~ m{([/a-zA-Z0-9._-]+)};                               # untaint
+        my ($newdir) = $entry{'dir'} =~ m{([/a-zA-Z0-9._-]+)};                      # untaint
-        return R('ERR_SECURITY_VIOLATION', msg => "Forbidden characters in account home directory") if ($newdir ne $dir);
+        return R('ERR_SECURITY_VIOLATION', msg => "Forbidden characters in account home directory") if ($newdir ne $entry{'dir'});
-        $dir = $newdir;                                                             # untaint
+        $entry{'dir'} = $newdir;                                                    # untaint
-        return R('OK', value => {uid => $uid, gid => $gid, dir => $dir, account => $name});
+        return R('OK', value => {uid => $entry{'uid'}, gid => $entry{'gid'}, dir => $entry{'dir'}, account => $entry{'name'}});
     }
     return R('KO_NOT_FOUND', msg => "Account '$account' doesn't exist");
 }
@@ -722,97 +735,114 @@ sub add_user_to_group {
     return R('OK');
 }
 
-my %_cache_get_group_list;
+# return the list of the bastion groups (i.e. not the system group list)
-
 sub get_group_list {
-    my %params    = @_;
+    my %params = @_;
-    my $groupType = $params{'groupType'};
+    my $cache  = $params{'cache'};    # if true, allow cache use
-    my $cache     = $params{'cache'};       # if true, allow cache use
+    state $cached_response;
 
-    if ($cache and $_cache_get_group_list{$groupType}) {
+    # if we've been called before and can use the cache, just return it
-        return $_cache_get_group_list{$groupType};
+    if ($cache and $cached_response) {
+        return $cached_response;
     }
 
     my %groups;
-    setgrent();
-    while (my @nextgroup = getgrent()) {
-        my ($name, $passwd, $gid, $members) = @nextgroup;
 
-        # also be nice and fill this cache for our sibling funcs
+    # sys_getent_gr() might have been called before, and has its own cache,
-        $_cache_getgrnam{$name} = \@nextgroup;
+    # so also try to use its cache if allowed and available.
-        if (    $groupType eq 'key'
+    # we loop through all the system groups and only retain those starting
-            and $name =~ /^key/
+    # with "key", and not finishing in -owner, -gatekeeper or -aclkeeper.
-            and $name !~ /-(?:owner|gatekeeper|aclkeeper)$/
+    # we also exclude special builtin groups (keykeeper and keyreader)
-            and not grep { $name eq $_ } qw{ keykeeper keyreader })
+    foreach my $gr (@{OVH::Bastion::sys_getent_gr(cache => $cache)->value}) {
+        if (   $gr->{'name'} =~ /^key/
+            && $gr->{'name'} !~ /-(?:owner|gatekeeper|aclkeeper)$/
+            && !grep { $gr->{'name'} eq $_ } qw{ keykeeper keyreader })
         {
-            $name =~ s/^key//;
+            $gr->{'name'} =~ s/^key//;
-            $groups{$name} = {gid => $gid, members => [split(/ /, $members)]} if ($name ne '');
+            $groups{$gr->{'name'}} = {gid => $gr->{'gid'}, members => $gr->{'members'}} if ($gr->{'name'} ne '');
         }
     }
-    $_cache_get_group_list{$groupType} = R('OK', value => \%groups);
+    $cached_response = R('OK', value => \%groups);
-    return $_cache_get_group_list{$groupType};
+    return $cached_response;
 }
 
+# return the list of bastion accounts (i.e. not the system user list)
 sub get_account_list {
     my %params   = @_;
     my $accounts = $params{'accounts'} || [];
     my $cache    = $params{'cache'};            # if true, allow cache use
     state $cached_response;
 
-    if ($cache and $cached_response) {
+    # if we've been called before and can use the cache, just return it
+    # don't do it if we're asked to only return a subset of all the accounts
+    if ($cache && $cached_response && !@$accounts) {
         return $cached_response;
     }
 
     my %users;
 
+    # sys_getent_pw() might have been called before, and has its own cache,
+    # so also try to use its cache if allowed and available.
+    # we loop through all the accounts known to the OS
+    foreach my $pw (@{OVH::Bastion::sys_getent_pw(cache => $cache)->value}) {
+
+        # if $accounts has been specified, only consider those
+        next if (@$accounts && !grep { $pw->{'name'} eq $_ } @$accounts);
+
+        # skip invalid accounts
+        next if not OVH::Bastion::is_bastion_account_valid_and_existing(account => $pw->{'name'});
+
+        # add proper accounts, only include a subset of the fields we got
+        $users{$pw->{'name'}} = {name => $pw->{'name'}, gid => $pw->{'gid'}, home => $pw->{'dir'}, shell => $pw->{'shell'}, uid => $pw->{'uid'}};
+    }
+
     if (@$accounts) {
-        foreach (@$accounts) {
+        return R('OK', value => \%users);
-            my ($name, $passwd, $uid, $gid, $quota, $comment, $gcos, $dir, $shell, $expire) = getpwnam($_);
-            next unless OVH::Bastion::is_bastion_account_valid_and_existing(account => $name);
-            $users{$name} = {name => $name, uid => $uid, gid => $gid, home => $dir, shell => $shell};
-        }
     }
     else {
-        setpwent();
+        $cached_response = R('OK', value => \%users);
-        while (my @nextaccount = getpwent()) {
+        return $cached_response;
-            my ($name, $passwd, $uid, $gid, $quota, $comment, $gcos, $dir, $shell, $expire) = @nextaccount;
-
-            # also be nice and fill this cache for our sibling funcs
-            $_cache_getpwnam{$name} = \@nextaccount;
-            next unless OVH::Bastion::is_bastion_account_valid_and_existing(account => $name);
-            $users{$name} = {name => $name, uid => $uid, gid => $gid, home => $dir, shell => $shell};
-        }
     }
-    $cached_response = R('OK', value => \%users);
-    return $cached_response;
 }
 
 sub get_realm_list {
     my %params = @_;
     my $realms = $params{'realms'} || [];
+    my $cache  = $params{'cache'};          # if true, allow cache use
+    state $cached_response;
+
+    # if we've been called before and can use the cache, just return it
+    # don't do it if we're asked to only return a subset of all the accounts
+    if ($cache && $cached_response && !@$realms) {
+        return $cached_response;
+    }
 
     my %users;
 
-    setpwent();
+    # sys_getent_pw() might have been called before, and has its own cache,
+    # so also try to use its cache if allowed and available.
+    # we loop through all the accounts known to the OS
+    foreach my $pw (@{OVH::Bastion::sys_getent_pw(cache => $cache)->value}) {
+
+        # if $realms has been specified, only consider those
+        next if (@$realms && !grep { $pw->{'name'} eq "realm_$_" } @$realms);
+
+        # skip invalid realms
+        next if not OVH::Bastion::is_bastion_account_valid_and_existing(account => $pw->{'name'}, accountType => "realm");
+
+        # add proper realms
+        my $name = $pw->{'name'};
+        $name =~ s{^realm_}{};
+        $users{$name} = {name => $name};
+    }
 
     if (@$realms) {
-        foreach (@$realms) {
+        return R('OK', value => \%users);
-            my ($name, $passwd, $uid, $gid, $quota, $comment, $gcos, $dir, $shell, $expire) = getpwnam("realm_" . $_);
-            next unless OVH::Bastion::is_bastion_account_valid_and_existing(account => $name, accountType => "realm");
-            $name =~ s{^realm_}{};
-            $users{$name} = {name => $name};
-        }
     }
     else {
-        setpwent();
+        $cached_response = R('OK', value => \%users);
-        while (my ($name, $passwd, $uid, $gid, $quota, $comment, $gcos, $dir, $shell, $expire) = getpwent()) {
+        return $cached_response;
-            next unless OVH::Bastion::is_bastion_account_valid_and_existing(account => $name, accountType => "realm");
-            $name =~ s{^realm_}{};
-            $users{$name} = {name => $name};
-        }
     }
-
-    return R('OK', value => \%users);
 }
 
 # check if account is a bastion admin (gives access to adminXyz commands)

lib/perl/OVH/Bastion/os.inc

@@ -595,4 +595,84 @@ sub is_in_path {
     return R('KO', msg => "$binary was not found in PATH");
 }
 
+# as setpwent/endpwent can have side effects to any getpwent used between
+# them, possibly in other parts of the program, encapsulate everything here
+# to avoid side effects
+sub sys_getent_pw {
+    my %params = @_;
+    my $cache  = $params{'cache'};
+    state $cached_response;
+
+    if ($cache && $cached_response) {
+        return $cached_response;
+    }
+
+    my @all;
+
+    # end/set: for some reason, if we don't end() before set(), there seem
+    # to be a cache somewhere, and we get entries that have been
+    # deleted (milli)seconds before. This behaviour is undocumented, as set()
+    # is supposed to reset the pointer, and a no mention of a cache is done
+    # anywhere. Luckily, Prepending the set() with an end() seems to
+    # reliably fix the issue on all tested OSes.
+    endpwent();
+    setpwent();
+    while (my @line = getpwent()) {
+        push @all,
+          {
+            name    => $line[0],
+            passwd  => $line[1],
+            uid     => $line[2],
+            gid     => $line[3],
+            quota   => $line[4],
+            comment => $line[5],
+            gcos    => $line[6],
+            dir     => $line[7],
+            shell   => $line[8],
+            expire  => $line[9],
+          };
+    }
+    endpwent();
+
+    $cached_response = R('OK', value => \@all);
+    return $cached_response;
+}
+
+# as setgrent/endgrent can have side effects to any getgrent used between
+# them, possibly in other parts of the program, encapsulate everything here
+# to avoid side effects
+sub sys_getent_gr {
+    my %params = @_;
+    my $cache  = $params{'cache'};
+    state $cached_response;
+
+    if ($cache && $cached_response) {
+        return $cached_response;
+    }
+
+    my @all;
+
+    # end/set: for some reason, if we don't end() before set(), there seem
+    # to be a cache somewhere, and we get entries that have been
+    # deleted (milli)seconds before. This behaviour is undocumented, as set()
+    # is supposed to reset the pointer, and a no mention of a cache is done
+    # anywhere. Luckily, pPrepending the set() with an end() seems to
+    # reliably fix the issue on all tested OSes.
+    endgrent();
+    setgrent();
+    while (my @line = getgrent()) {
+        push @all,
+          {
+            name    => $line[0],
+            passwd  => $line[1],
+            gid     => $line[2],
+            members => [split(/ /, $line[3])],
+          };
+    }
+    endgrent();
+
+    $cached_response = R('OK', value => \@all);
+    return $cached_response;
+}
+
 1;