diff --git a/bin/helper/osh-accountUnexpire b/bin/helper/osh-accountUnexpire index cf475e7..237b04c 100755 --- a/bin/helper/osh-accountUnexpire +++ b/bin/helper/osh-accountUnexpire @@ -49,11 +49,6 @@ else { } } -# for this special helper, $account must be equal to $ENV{'USER'} -if (OVH::Bastion::get_user_from_env()->value ne $account) { - HEXIT('ERR_SECURITY_VIOLATION', msg => "You're not allowed to run this on $account, dear $self"); -} - #PARAMS:ACCOUNT @@ -61,23 +56,41 @@ osh_debug("Checking account"); $fnret = OVH::Bastion::is_bastion_account_valid_and_existing(account => $account); $fnret or HEXIT($fnret); $account = $fnret->value->{'account'}; # untainted +my $sysaccount = $fnret->value->{'sysaccount'}; +my $remoteaccount = $fnret->value->{'remoteaccount'}; +my $accounthome = $fnret->value->{'dir'}; #value->{'dir'}; +# for this special helper, $sysaccount must be equal to $ENV{'USER'} +if (OVH::Bastion::get_user_from_env()->value ne $sysaccount) { + HEXIT('ERR_SECURITY_VIOLATION', msg => "You're not allowed to run this on $account, dear $self"); +} + if (!-d $accounthome) { HEXIT('ERR_INVALID_HOME', msg => "Invalid HOME directory for this account"); } -$fnret = OVH::Bastion::is_account_nonexpired(sysaccount => $account); -$fnret->is_err and HEXIT($fnret); # couldn't read file or other error -$fnret->is_ok and HEXIT($fnret); # wasn't expired - -# is_ko: is expired -my $days = $fnret->value->{'days'}; -my $filepath = $fnret->value->{'filepath'}; - -$fnret = OVH::Bastion::touch_file($filepath); -$fnret or HEXIT($fnret); +$fnret = OVH::Bastion::is_account_nonexpired( + sysaccount => $sysaccount, + remoteaccount => $remoteaccount, + update_lastlog => 1, + no_create_remote => 1, +); +$fnret->is_err and HEXIT($fnret); # couldn't read file or other error +$fnret->is_ok and HEXIT($fnret); # wasn't expired (lastlog already refreshed inside is_account_nonexpired) + +my $days = $fnret->value->{'days'}; + +# is_ko: is expired, so we reset expiration on both the realm support user and the remote user specific file +my @filesToTouch = ("/home/$sysaccount/lastlog"); +if ($remoteaccount) { + my $remoteFile = "/home/$sysaccount/lastlog_$remoteaccount"; + push @filesToTouch, $remoteFile if -e $remoteFile; +} +foreach my $file (@filesToTouch) { + $fnret = OVH::Bastion::touch_file($file); + osh_warn("Couldn't touch $file: " . $fnret->msg) if !$fnret; +} HEXIT('OK', value => {account => $account, days => $days}); diff --git a/bin/plugin/restricted/accountUnexpire b/bin/plugin/restricted/accountUnexpire index 87f43f2..274a6d8 100755 --- a/bin/plugin/restricted/accountUnexpire +++ b/bin/plugin/restricted/accountUnexpire @@ -41,9 +41,10 @@ if (not $account) { $fnret = OVH::Bastion::is_bastion_account_valid_and_existing(account => $account); $fnret or osh_exit $fnret; $account = $fnret->value->{'account'}; +my $sysaccount = $fnret->value->{'sysaccount'}; my @command = qw{ sudo -n -u }; -push @command, ($account, '--'); +push @command, ($sysaccount, '--'); push @command, qw{ /usr/bin/env perl -T }; push @command, $OVH::Bastion::BASEPATH . '/bin/helper/osh-accountUnexpire'; push @command, ('--account', $account); @@ -51,16 +52,37 @@ push @command, ('--account', $account); $fnret = OVH::Bastion::helper(cmd => \@command); $fnret or osh_exit $fnret; +# the helper only ever returns an error (already handled above) or one of the OK_* codes below. +# build a consistent value across all cases: whatever the helper returned, plus the account name. +my $value = {%{$fnret->value || {}}, account => $account}; + if ($fnret->err eq 'OK') { - my $days = $fnret->value->{'days'}; + # the account was expired (or its last activity couldn't be determined) and has been reactivated + my $msg = + defined $value->{'days'} + ? "Account $account was expired ($value->{'days'} days without connection), it is now active again." + : "Account $account had no determinable last activity date, which has now been reset: it is active again."; + osh_ok R('OK', value => $value, msg => $msg); +} +elsif ($fnret->err eq 'OK_NOT_EXPIRED') { + # an expiration policy is configured but the account wasn't expired; its activity date was refreshed anyway osh_ok R( - 'OK', - value => {account => $account, days => $days}, - msg => "Account $account was expired ($days days without connection), it is now active again." + 'OK_NOT_EXPIRED', + value => $value, + msg => "Account $account is not expired, but its last activity date has been refreshed nevertheless." ); } -elsif ($fnret->is_ok) { - osh_ok R('OK_NO_CHANGE', msg => "Account $account wasn't expiring, no change was needed or made."); +elsif ($fnret->err eq 'OK_EXPIRATION_NOT_CONFIGURED' or $fnret->err eq 'OK_FIRST_LOGIN') { + # no expiration policy is configured on this bastion; the activity date was refreshed anyway. + # OK_FIRST_LOGIN (account had no recorded activity yet) is reported the same way: from the + # caller's point of view, the account is simply not expirable and its activity has been recorded. + osh_ok R( + 'OK_EXPIRATION_NOT_CONFIGURED', + value => $value, + msg => "Account $account is not expired and expiration is not configured on this bastion, " + . "but its last activity date has been refreshed nevertheless." + ); } -osh_exit $fnret; +# defensive fallback: any other (unexpected) OK_* code returned by the helper +osh_ok R('OK_NO_CHANGE', value => $value, msg => "Account $account wasn't expiring, no change was needed or made."); diff --git a/bin/shell/osh.pl b/bin/shell/osh.pl index 9246a41..984dbae 100755 --- a/bin/shell/osh.pl +++ b/bin/shell/osh.pl @@ -199,18 +199,21 @@ if (!$fnret) { # # Second check : has account logged-in recently enough to be allowed ? # -$fnret = OVH::Bastion::is_account_nonexpired(sysaccount => $sysself, remoteaccount => $remoteself); +$fnret = OVH::Bastion::is_account_nonexpired( + sysaccount => $sysself, + remoteaccount => $remoteself, + update_lastlog => 1, + ipfrom => $ipfrom, + hostfrom => $hostfrom +); if ($fnret->is_err) { - # internal error, warn and pass osh_warn($fnret); } elsif ($fnret->is_ko) { - # expired main_exit OVH::Bastion::EXIT_ACCOUNT_EXPIRED, 'account_expired', $fnret->msg; } -my $lastlog_filepath = $fnret->value->{'filepath'}; my $lastlogmsg = sprintf("Welcome to $bastionName, $self, this is your first connection"); if ($fnret && $fnret->value && $fnret->value->{'seconds'}) { @@ -223,15 +226,6 @@ if ($fnret && $fnret->value && $fnret->value->{'seconds'}) { ); } -# ok not expired, so we update lastlog -if ($lastlog_filepath && open(my $lastlogfh, '>', $lastlog_filepath)) { - print $lastlogfh sprintf("%s(%s)", $ipfrom, $hostfrom); - close($lastlogfh); -} -else { - osh_warn "Couldn't update your lastlog ($lastlog_filepath: $!), contact a bastion admin"; -} - # # Fetch command options # diff --git a/lib/perl/OVH/Bastion.pm b/lib/perl/OVH/Bastion.pm index f34cef6..23d0e1d 100644 --- a/lib/perl/OVH/Bastion.pm +++ b/lib/perl/OVH/Bastion.pm @@ -211,9 +211,13 @@ sub is_account_nonfrozen { # checks whether an account is expired (inactivity) if that's configured on this bastion sub is_account_nonexpired { - my %params = @_; - my $sysaccount = $params{'sysaccount'}; - my $remoteaccount = $params{'remoteaccount'}; + my %params = @_; + my $sysaccount = $params{'sysaccount'}; + my $remoteaccount = $params{'remoteaccount'}; + my $updateLastlog = $params{'update_lastlog'}; # update lastlog and lastlog_$remote + my $ipfrom = $params{'ipfrom'}; # when update_lastlog == 1, ip to update the log with + my $hostfrom = $params{'hostfrom'}; # when update_lastlog == 1, host to update the log with + my $noCreateRemote = $params{'no_create_remote'}; # for a realm account, don't create `lastlog_$remote` if it doesn't exist already if (not $sysaccount) { return R('ERR_MISSING_PARAMETER', msg => "Missing 'sysaccount' argument"); @@ -228,18 +232,41 @@ sub is_account_nonexpired { # some accounts might have a specific configuration overriding the global one $fnret = OVH::Bastion::account_config(account => $sysaccount, %{OVH::Bastion::OPT_ACCOUNT_MAX_INACTIVE_DAYS()}); - if ($fnret) { - $accountMaxInactiveDays = $fnret->value; + $accountMaxInactiveDays = $fnret->value if $fnret; + + my @lastlogFilesToRead; + my @lastlogFilesToUpdate; + if ($remoteaccount) { + # if we have a remoteaccount, this file takes precedence, however if it doesn't + # exist, we'll fallback to the lastlog file of the shared realm account itself + my $lastlogRemote = "/home/$sysaccount/lastlog_$remoteaccount"; + push @lastlogFilesToRead, $lastlogRemote; + + # if noCreateRemote, don't add it to @lastlogFilesToUpdate if it doesn't exist. + # don't use test -f to check that, in case we have -r+x on the folder: + # directly try to open it instead + if ($noCreateRemote) { + if (open(my $fh, "<", $lastlogRemote)) { + push @lastlogFilesToUpdate, $lastlogRemote; + close($fh); + } + } + else { + push @lastlogFilesToUpdate, $lastlogRemote; + } } + # the main lastlogfile of the sysaccount must be read and updated at all times + push @lastlogFilesToRead, "/home/$sysaccount/lastlog"; + push @lastlogFilesToUpdate, "/home/$sysaccount/lastlog"; - my $isFirstLogin; my $lastlog; - my $filepath = "/home/$sysaccount/lastlog" . ($remoteaccount ? "_$remoteaccount" : ""); - my $value = {filepath => $filepath}; - if (-e $filepath) { - $isFirstLogin = 0; - $lastlog = (stat(_))[9]; - osh_debug("is_account_nonexpired: got lastlog date: $lastlog"); + my $value = {}; + # use the first lastlog file that exists in the list + foreach my $filepath (@lastlogFilesToRead) { + next if !-e $filepath; + + $lastlog = (stat(_))[9]; + osh_debug("is_account_nonexpired: got lastlog date: $lastlog from file $filepath"); # if lastlog file is available, fetch some info from it if (open(my $lastloginfh, "<", $filepath)) { @@ -248,67 +275,106 @@ sub is_account_nonexpired { close($lastloginfh); $value->{'info'} = $info; } + + # no need to continue once we have the proper info + last; } - else { + + my $isFirstLogin = 0; + if (not defined $lastlog) { + # the lastlog file doesn't exist, or maybe we just don't have access to it. + # by trying to chdir() to the folder, we're confirming we do have -x on it, + # otherwise that would explain why we can't open lastlog or even know whether it exists: my ($previousDir) = getcwd() =~ m{^(/[a-z0-9_./-]+)}i; if (!chdir("/home/$sysaccount")) { osh_debug("is_account_nonexpired: no exec access to the folder!"); return R('ERR_NO_ACCESS', msg => "No read access to this account folder to compute last login time"); } + # access rights to the home of the account confirmed, so lastlog doesn't exist, + # so this is the first login chdir($previousDir); $isFirstLogin = 1; - # get the account creation timestamp as the lastlog - $fnret = OVH::Bastion::account_config(account => $sysaccount, key => "creation_timestamp"); - if ($fnret && $fnret->value) { - $lastlog = $fnret->value; - osh_debug("is_account_nonexpired: got creation date from config.creation_timestamp: $lastlog"); - } - elsif (-e "/home/$sysaccount/accountCreate.comment") { - - # fall back to the stat of the accountCreate.comment file - $lastlog = (stat(_))[9]; - osh_debug("is_account_nonexpired: got creation date from accountCreate.comment stat: $lastlog"); + if ($remoteaccount) { + # For a realm user connecting for the first time, don't penalise them + # with the age of the realm support account: use the current time so + # that they start with 0 days of inactivity. + $lastlog = time(); + osh_debug("is_account_nonexpired: realm user's first login, using current time as lastlog"); } else { - # last fall back to the stat of the ttyrec/ folder - $lastlog = (stat("/home/$sysaccount/ttyrec"))[9]; - osh_debug("is_account_nonexpired: got creation date from ttyrec/ stat: $lastlog"); + # get the account creation timestamp as the lastlog + $fnret = OVH::Bastion::account_config(account => $sysaccount, key => "creation_timestamp"); + if ($fnret && $fnret->value) { + $lastlog = $fnret->value; + osh_debug("is_account_nonexpired: got creation date from config.creation_timestamp: $lastlog"); + } } } - my $seconds = time() - $lastlog; - my $days = int($seconds / 86400); - $value->{'days'} = $days; - $value->{'seconds'} = $seconds; $value->{'already_seen_before'} = !$isFirstLogin; - osh_debug("Last account activity: $days days ago"); + if (defined $lastlog) { + $value->{'seconds'} = time() - $lastlog; + $value->{'days'} = int($value->{'seconds'} / 86400); + osh_debug("Last account activity: " . $value->{'days'} . " days ago"); + } + else { + # can't tell the last log nor the creation date + warn_syslog("Couldn't tell the last login date of $sysaccount" . ($remoteaccount ? "/$remoteaccount" : "")); + } - if ($accountMaxInactiveDays == 0) { + if (!defined($lastlog) && $accountMaxInactiveDays != 0) { + # last login date is unknown and an expiration policy is configured: + # err to the side of caution and return an error to deny login. + # if no policy is configured, we fall through and allow login below. + return R('KO_UNKNOWN_LAST_LOGIN', msg => "Couldn't determine last login date"); + } + # called at several places below, only when we return OK + my $updateFunc = sub { + return if !$updateLastlog; + foreach my $file (@lastlogFilesToUpdate) { + if ($ipfrom && $hostfrom) { + if (open(my $lastlogfh, '>', $file)) { + print $lastlogfh sprintf("%s(%s)", $ipfrom, $hostfrom); + close($lastlogfh); + } + else { + warn_syslog("Couldn't update the lastlog file '$file' ($!)"); + } + } + else { + # just touch it (i.e. accountUnexpire case) + $fnret = OVH::Bastion::touch_file($file); + if (!$fnret) { + warn_syslog("Couldn't update the lastlog file '$file' ($fnret)"); + } + } + } + }; + + if ($accountMaxInactiveDays == 0) { # no expiration configured, allow login and return some info + $updateFunc->(); return R('OK_FIRST_LOGIN', value => $value) if $isFirstLogin; return R('OK_EXPIRATION_NOT_CONFIGURED', value => $value); } - else { - if ($days < $accountMaxInactiveDays) { - # expiration configured, but account not expired, allow login - return R('OK_NOT_EXPIRED', value => $value); - } - else { - # account expired, deny login - my $msg = OVH::Bastion::config("accountExpiredMessage")->value; - $msg = "Sorry, but your account has expired (#DAYS# days), access denied by policy." if !$msg; - $msg =~ s/#DAYS#/$days/g; - return R( - 'KO_EXPIRED', - value => $value, - msg => $msg, - ); - } + if ($value->{'days'} < $accountMaxInactiveDays) { + # expiration configured, but account not expired, allow login + $updateFunc->(); + return R('OK_NOT_EXPIRED', value => $value); } - return R('ERR_INTERNAL_ERROR'); + + # account expired, deny login + my $msg = OVH::Bastion::config("accountExpiredMessage")->value; + $msg = "Sorry, but your account has expired (#DAYS# days), access denied by policy." if !$msg; + $msg =~ s/#DAYS#/$value->{'days'}/g; + return R( + 'KO_EXPIRED', + value => $value, + msg => $msg, + ); } sub is_account_ttl_nonexpired { @@ -787,6 +853,7 @@ sub touch_file { else { $ret = sysopen($fh, $file, O_RDWR | O_CREAT); } + my $err = $!; if ($ret) { close($fh); @@ -797,8 +864,8 @@ sub touch_file { } # else - warn_syslog(sprintf("Couldn't touch file '%s' with perms %o: %s", $file, $perms, $!)); - return R('KO', msg => "Couldn't create file $file: $!"); + warn_syslog(sprintf("Couldn't touch file '%s' with perms %o: %s", $file, $perms, $err)); + return R('KO', msg => "Couldn't create file $file: $err"); } sub create_file_if_not_exists { @@ -969,8 +1036,13 @@ sub can_account_execute_plugin { # restricted plugins (osh-* system groups based) if (-f ($path_plugin . '/restricted/' . $plugin)) { if (OVH::Bastion::is_user_in_group(user => $account, group => "osh-$plugin", cache => $cache)) { - return R('OK', - value => {fullpath => $path_plugin . '/restricted/' . $plugin, type => 'restricted', plugin => $plugin} + return R( + 'OK', + value => { + fullpath => $path_plugin . '/restricted/' . $plugin, + type => 'restricted', + plugin => $plugin + } ); } else { diff --git a/tests/functional/tests.d/310-realm.sh b/tests/functional/tests.d/310-realm.sh index b0decd3..f530d75 100644 --- a/tests/functional/tests.d/310-realm.sh +++ b/tests/functional/tests.d/310-realm.sh @@ -55,6 +55,54 @@ testsuite_realm() success firstconnect2 $a2 realm_$realm_shared_account@127.0.0.1 --kbd-interactive -- $js --osh info json .value.account $account2 .value.realm $realm_shared_account + # --- accountUnexpire on a 'realm/user' account --- + # regression: this used to fail because the helper compared $ENV{USER} (the realm_* sysaccount) + # against the full 'realm/user' account name, and the plugin tried to sudo to the non-existent + # 'realm/user' system user instead of the realm_* sysaccount. + local realm_sys="realm_$realm_shared_account" + local realm_user="$realm_shared_account/$account1" + + # the per-remote-user lastlog file must have been created by firstconnect1 above + success realm_lastlog_user_present $r0 "test -f /home/$realm_sys/lastlog_$account1 && echo PRESENT" + contain PRESENT + + # with no expiration policy configured, unexpiring still refreshes the activity date and reports it + success realm_unexpire_noexpi $a0 --osh accountUnexpire --account $realm_user + json .command accountUnexpire .error_code OK_EXPIRATION_NOT_CONFIGURED .value.account $realm_user + + # unexpiring the realm support account (the bare realm_* sysaccount) directly is forbidden: + # only the 'realm/user' form is a valid account to operate on, never the realm_* account itself + plgfail realm_unexpire_sysaccount $a0 --osh accountUnexpire --account $realm_sys + json .error_code KO_FORBIDDEN_PREFIX + + # now configure an expiration policy and artificially expire *only* the realm user's per-user lastlog; + # the shared realm-account lastlog is left fresh, so a detected expiry can only come from the per-user file + configchg 's=^\\\\x22accountMaxInactiveDays\\\\x22.+=\\\\x22accountMaxInactiveDays\\\\x22:2,=' + success realm_expire_user $r0 "touch -t 201501010101 /home/$realm_sys/lastlog_$account1" + + # accountUnexpire detects the expiry (reading the per-user file, not the fresh shared one) and reactivates + success realm_unexpire_expired $a0 --osh accountUnexpire --account $realm_user + json .command accountUnexpire .error_code OK .value.account $realm_user + json '.value.days > 1000' true + + # running it again proves the per-user file was the one refreshed: it now reads as not-expired + success realm_unexpire_again $a0 --osh accountUnexpire --account $realm_user + json .command accountUnexpire .error_code OK_NOT_EXPIRED .value.account $realm_user + + # accountUnexpire must ALSO refresh the shared realm-account lastlog, not just the per-user file. + # age *only* the shared file (per-user is fresh now), drop a dated reference, then unexpire: even + # though the account isn't expired, the shared file's mtime must move past the reference, proving + # it was really touched (a plain existence check wouldn't catch a missing refresh). + success realm_age_shared $r0 "touch -t 201501010101 /home/$realm_sys/lastlog && touch -t 202001010101 /tmp/bastiontest_lastlog_ref" + success realm_unexpire_refresh_shared $a0 --osh accountUnexpire --account $realm_user + json .command accountUnexpire .error_code OK_NOT_EXPIRED .value.account $realm_user + success realm_lastlog_shared_touched $r0 "test /home/$realm_sys/lastlog -nt /tmp/bastiontest_lastlog_ref && echo TOUCHED" + contain TOUCHED + success realm_ref_cleanup $r0 "rm -f /tmp/bastiontest_lastlog_ref" + + # reset the expiration policy for the rest of this module + configchg 's=^\\\\x22accountMaxInactiveDays\\\\x22.+=\\\\x22accountMaxInactiveDays\\\\x22:0,=' + # try forbidden plugins for plugin in selfAddPersonalAccess selfAddIngressKey selfDelIngressKey selfGenerateEgressKey selfAddPersonalAccess selfDelPersonalAccess selfPlaySession selfListSessions selfResetIngressKeys do diff --git a/tests/functional/tests.d/340-selfaccesses.sh b/tests/functional/tests.d/340-selfaccesses.sh index 3df236a..f44c70b 100644 --- a/tests/functional/tests.d/340-selfaccesses.sh +++ b/tests/functional/tests.d/340-selfaccesses.sh @@ -512,8 +512,19 @@ testsuite_selfaccesses() success dupe $a0 --osh selfForgetHostKey --host 127.0.0.2 json .command selfForgetHostKey .error_code OK '.value."127.0.0.2".action' OK_NO_MATCH - success nochange $a0 --osh accountUnexpire --account $account1 - json .command accountUnexpire .error_code OK_NO_CHANGE + # --- accountUnexpire (non-realm account) --- + # with no expiration policy configured, the account isn't expirable, but its activity date is + # still refreshed and the command reports so (and the account name is echoed back in the value) + success no_expi_configured $a0 --osh accountUnexpire --account $account1 + json .command accountUnexpire .error_code OK_EXPIRATION_NOT_CONFIGURED .value.account $account1 + + # same on a never-logged-in account (no lastlog file): this is internally OK_FIRST_LOGIN, reported + # the same way to the caller, and the lastlog file gets (re)created in the process + success removelastlog_noexpi $r0 "rm -f /home/$account1/lastlog" + success no_expi_firstlogin $a0 --osh accountUnexpire --account $account1 + json .command accountUnexpire .error_code OK_EXPIRATION_NOT_CONFIGURED .value.account $account1 + success lastlog_recreated $r0 "test -f /home/$account1/lastlog && echo PRESENT" + contain PRESENT # artificially expire account1 configchg 's=^\\\\x22accountMaxInactiveDays\\\\x22.+=\\\\x22accountMaxInactiveDays\\\\x22:2,=' @@ -523,19 +534,23 @@ testsuite_selfaccesses() retvalshouldbe 113 success works $a0 --osh accountUnexpire --account $account1 - json .command accountUnexpire .error_code OK + json .command accountUnexpire .error_code OK .value.account $account1 + json '.value.days > 1000' true success unexpired $a1 --osh info json .error_code OK success worksnochange $a0 --osh accountUnexpire --account $account1 - json .command accountUnexpire .error_code OK_NO_CHANGE + json .command accountUnexpire .error_code OK_NOT_EXPIRED .value.account $account1 - # try on never logged-in account (different code path) + # try on never logged-in account (different code path): remove lastlog while the policy is set success manuallyRemoveLastlog $r0 "rm -f /home/$account1/lastlog" - success worksnochange $a0 --osh accountUnexpire --account $account1 - json .command accountUnexpire .error_code OK_NO_CHANGE + success worksnochange2 $a0 --osh accountUnexpire --account $account1 + json .command accountUnexpire .error_code OK_NOT_EXPIRED .value.account $account1 + + # reset the expiration policy for the rest of this module + configchg 's=^\\\\x22accountMaxInactiveDays\\\\x22.+=\\\\x22accountMaxInactiveDays\\\\x22:0,=' # delete account1 script cleanup $a0 --osh accountDelete --account $account1 "<<< \"Yes, do as I say and delete $account1, kthxbye\""