enh: add lock for group ACL change to avoid race conditions

bin/helper/osh-accountCreate

@@ -69,7 +69,9 @@ if (not grep { $type eq $_ } qw{ normal realm }) {
 # take a lock here, do it before checking for account existence,
 # because another parallel creation of the same account might be
 # occuring, in which case we'd still hit a race condition
-$fnret = OVH::Bastion::Helper::get_lock_fh();
+# additionally, the lock type "passwd" is used by all helpers
+# that may modify /etc/passwd or /etc/group.
+$fnret = OVH::Bastion::Helper::get_lock_fh(category => "passwd");
 $fnret or HEXIT($fnret);
 my $lock_fh = $fnret->value;
 $fnret = OVH::Bastion::Helper::acquire_lock($lock_fh);

bin/helper/osh-accountDelete

@@ -238,6 +238,15 @@ if (-d $fulldir) {
 }
 osh_info("Backup done");
 
+# take a lock here, as we're going to remove system accounts and groups.
+# additionally, the lock type "passwd" is used by all helpers
+# that may modify /etc/passwd or /etc/group.
+$fnret = OVH::Bastion::Helper::get_lock_fh(category => "passwd");
+$fnret or HEXIT($fnret);
+my $lock_fh = $fnret->value;
+$fnret = OVH::Bastion::Helper::acquire_lock($lock_fh);
+$fnret or HEXIT($fnret);
+
 osh_info "Removing '$account' group membership from 'keyreader' user";
 $fnret = OVH::Bastion::sys_delmemberfromgroup(user => "keyreader", group => $account);
 if (!$fnret) {

bin/helper/osh-groupAddServer

@@ -78,6 +78,13 @@ else {
 
 #<RIGHTSCHECK
 
+# take a lock here, to block other group ACL modifying commands for this group until we're done.
+$fnret = OVH::Bastion::Helper::get_lock_fh(category => "groupacl", basepath => "/home/$group");
+$fnret or HEXIT($fnret);
+my $lock_fh = $fnret->value;
+$fnret = OVH::Bastion::Helper::acquire_lock($lock_fh);
+$fnret or HEXIT($fnret);
+
 #>CODE
 my $machine = $ip;
 $port and $machine .= ":$port";

bin/helper/osh-groupCreate

@@ -96,8 +96,10 @@ my $shortGroup = $fnret->value->{'shortGroup'};
 
 # take a lock here, do it before checking for group existence,
 # because another parallel creation of the same group might be
-# occuring, in which case we'd still hit a race condition
-$fnret = OVH::Bastion::Helper::get_lock_fh();
+# occuring, in which case we'd still hit a race condition.
+# additionally, the lock type "passwd" is used by all helpers
+# that may modify /etc/passwd or /etc/group.
+$fnret = OVH::Bastion::Helper::get_lock_fh(category => "passwd");
 $fnret or HEXIT($fnret);
 my $lock_fh = $fnret->value;
 $fnret = OVH::Bastion::Helper::acquire_lock($lock_fh);

bin/helper/osh-groupDelete

@@ -162,6 +162,15 @@ if (-d $fulldir) {
 }
 osh_info("Backup done");
 
+# take a lock here, as we're going to remove system accounts and groups.
+# additionally, the lock type "passwd" is used by all helpers
+# that may modify /etc/passwd or /etc/group.
+$fnret = OVH::Bastion::Helper::get_lock_fh(category => "passwd");
+$fnret or HEXIT($fnret);
+my $lock_fh = $fnret->value;
+$fnret = OVH::Bastion::Helper::acquire_lock($lock_fh);
+$fnret or HEXIT($fnret);
+
 # remove dead symlinks in users homes
 my $dh;
 if (opendir($dh, "/home/allowkeeper")) {

bin/helper/osh-groupSetServers

@@ -77,6 +77,13 @@ if (!$data || ref $data ne 'ARRAY') {
     HEXIT('ERR_INVALID_ARGUMENT', msg => "Invalid JSON import format sent by the plugin");
 }
 
+# take a lock here, to block other group ACL modifying commands for this group until we're done.
+$fnret = OVH::Bastion::Helper::get_lock_fh(category => "groupacl", basepath => "/home/$group");
+$fnret or HEXIT($fnret);
+my $lock_fh = $fnret->value;
+$fnret = OVH::Bastion::Helper::acquire_lock($lock_fh);
+$fnret or HEXIT($fnret);
+
 $fnret = OVH::Bastion::access_modify(
     way    => 'group',
     action => 'clear',

lib/perl/OVH/Bastion/Helper.pm

@@ -75,27 +75,52 @@ if (not defined $self) {
 }
 
 sub get_lock_fh {
-    my $fh;
-    my $lockdir  = "/tmp/bastion.lock";
-    my $lockfile = "$lockdir/lock";
-
-    # to avoid symlink attacks, we first create a subdir only accessible by root
-    unlink $lockdir;    # will silently fail if doesn't exist or is not a file
-    mkdir $lockdir;     # will silently fail if we lost the race
-    chown 0, 0, $lockdir;
-    chmod 0700, $lockdir;
-
-    # now, check if we do have a directory, or if we lost the race
-    if (!-d $lockdir) {
-        warn_syslog("Couldn't create $lockdir: are we being raced against?");
-        return R('ERR_CANNOT_LOCK', msg => "Couldn't create lock file, please retry");
+    my %params   = @_;
+    my $category = $params{'category'};
+
+    return R('ERR_MISSING_PARAMETER', msg => "Missing category in get_lock_fh") if !$category;
+
+    my ($lockdir, $lockfile, $lockdircreate);
+    if ($category eq 'passwd') {
+        $lockdir       = "/tmp/bastion.lock.passwd";
+        $lockfile      = "$lockdir/lock";
+        $lockdircreate = 1;
     }
-    # here, $lockdir is guaranteed to be a directory, check its perms
-    my @perms = stat($lockdir);
-    if ($perms[4] != 0 || $perms[5] != 0 || S_IMODE($perms[2]) != oct(700)) {
-        warn_syslog("The $lockdir directory has invalid perms: are we being raced against? mode="
-              . sprintf("%04o", S_IMODE($perms[2])));
-        return R('ERR_CANNOT_LOCK', msg => "Couldn't create lock file, please retry");
+    elsif ($category eq 'groupacl') {
+        $lockdir       = $params{'basepath'};
+        $lockdircreate = 0;                     # must already exist
+        if (!$lockdir || !-d $lockdir || !-w $lockdir) {
+            return R('ERR_INVALID_PARAMETER',
+                msg => "Missing or invalid basepath '" . ($lockdir // '<u>') . "' in get_lock_fh");
+        }
+        # we use the .db suffix because it's already excluded from the cluster sync:
+        $lockfile = "$lockdir/lock.db";
+    }
+    else {
+        return R('ERR_INVALID_PARAMETER', msg => "Unknown category '$category' in get_lock_fh");
+    }
+
+    my $fh;
+
+    if ($lockdircreate) {
+        # to avoid symlink attacks, we first create a subdir only accessible by root
+        unlink $lockdir;    # will silently fail if doesn't exist or is not a file
+        mkdir $lockdir;     # will silently fail if we lost the race
+        chown 0, 0, $lockdir;
+        chmod 0700, $lockdir;
+
+        # now, check if we do have a directory, or if we lost the race
+        if (!-d $lockdir) {
+            warn_syslog("Couldn't create $lockdir: are we being raced against?");
+            return R('ERR_CANNOT_LOCK', msg => "Couldn't create lock file, please retry");
+        }
+        # here, $lockdir is guaranteed to be a directory, check its perms
+        my @perms = stat($lockdir);
+        if ($perms[4] != $< || $perms[5] != $( || S_IMODE($perms[2]) != oct(700)) {
+            warn_syslog("The $lockdir directory has invalid perms: are we being raced against? mode="
+                  . sprintf("%04o", S_IMODE($perms[2])));
+            return R('ERR_CANNOT_LOCK', msg => "Couldn't create lock file, please retry");
+        }
     }
 
     # here, $lockdir is guaranteed to be owned only by us. but rogue files