diff --git a/bin/helper/osh-accountAddGroupServer b/bin/helper/osh-accountAddGroupServer index a244090df..9d9f24b25 100755 --- a/bin/helper/osh-accountAddGroupServer +++ b/bin/helper/osh-accountAddGroupServer @@ -31,7 +31,7 @@ if (not defined $self) { # Fetch command options my $fnret; my ($result, @optwarns); -my ($account, $group, $ip, $user, $port, $action, $ttl, $forceKey); +my ($account, $group, $ip, $user, $port, $action, $ttl, $comment, $forceKey); eval { local $SIG{__WARN__} = sub { push @optwarns, shift }; $result = GetOptions( @@ -42,6 +42,7 @@ eval { "port=i" => sub { $port //= $_[1] }, "action=s" => sub { $action //= $_[1] }, "ttl=i" => sub { $ttl //= $_[1] }, + "comment=s" => sub { $comment //= $_[1] }, "force-key=s" => sub { $forceKey //= $_[1] }, ); }; @@ -78,6 +79,7 @@ $fnret = OVH::Bastion::access_modify( ip => $ip, port => $port, ttl => $ttl, + comment => $comment, forceKey => $forceKey ); HEXIT($fnret); diff --git a/bin/helper/osh-accountModifyPersonalAccess b/bin/helper/osh-accountModifyPersonalAccess index 8492c6e93..da6faa7ee 100755 --- a/bin/helper/osh-accountModifyPersonalAccess +++ b/bin/helper/osh-accountModifyPersonalAccess @@ -91,6 +91,10 @@ if (not grep { $action eq $_ } qw{ add del }) { #CODE +my $machine = $ip; +$port and $machine .= ":$port"; +$user and $machine = $user . '@' . $machine; + # access_modify validates all its parameters, don't do it ourselves here for clarity $fnret = OVH::Bastion::access_modify( way => 'personal', @@ -103,4 +107,18 @@ $fnret = OVH::Bastion::access_modify( forceKey => $forceKey, comment => $comment, ); +if ($fnret->err eq 'OK') { + my $ttlmsg = $ttl ? ' (expires in ' . OVH::Bastion::duration2human(seconds => $ttl)->value->{'human'} . ')' : ''; + HEXIT( + 'OK', + value => {action => $action, account => $account, ip => $ip, user => $user, port => $port, ttl => $ttl, comment => $comment}, + msg => $action eq 'add' ? "Access to $machine was added to account $account$ttlmsg" : "Access to $machine was removed from account $account$ttlmsg" + ); +} +elsif ($fnret->err eq 'OK_NO_CHANGE') { + HEXIT('OK_NO_CHANGE', + msg => $action eq 'add' + ? "Access to $machine was already granted to account $account, nothing done" + : "Access to $machine was not granted to account $account, nothing done"); +} HEXIT($fnret); diff --git a/bin/plugin/group-gatekeeper/groupAddGuestAccess b/bin/plugin/group-gatekeeper/groupAddGuestAccess index 4a10340f4..483f6d7db 100755 --- a/bin/plugin/group-gatekeeper/groupAddGuestAccess +++ b/bin/plugin/group-gatekeeper/groupAddGuestAccess @@ -20,6 +20,7 @@ my $remainingOptions = OVH::Bastion::Plugin::begin( "scpup" => \my $scpUp, "scpdown" => \my $scpDown, "ttl=s" => \my $ttl, + "comment=s" => \my $comment, }, helptext => <<'EOF', Add a specific group server access to an account @@ -27,7 +28,7 @@ Add a specific group server access to an account Usage: --osh SCRIPT_NAME --group GROUP --account ACCOUNT [OPTIONS] --group GROUP group to add guest access to - --account ACCOUNT name of the other bastion account to add access to, he'll be given access to the GROUP key + --account ACCOUNT name of the other bastion account to add access to, they'll be given access to the GROUP key --host HOST|IP add access to this HOST (which must belong to the GROUP) --user USER allow connecting to HOST only with remote login USER --user-any allow connecting to HOST with any remote login @@ -35,7 +36,9 @@ Usage: --osh SCRIPT_NAME --group GROUP --account ACCOUNT [OPTIONS] --port-any allow connecting to HOST with any remote port --scpup allow SCP upload, you--bastion-->server (omit --user in this case) --scpdown allow SCP download, you<--bastion--server (omit --user in this case) - --ttl SECONDS|DURATION Specify a number of seconds after which the access will automatically expire + --ttl SECONDS|DURATION specify a number of seconds after which the access will automatically expire + --comment '"ANY TEXT'" add a comment alongside this access. + If omitted, we'll use the closest preexisting group access' comment as seen in groupListServers This command adds, to an existing bastion account, access to the egress keys of a group, but only to accessing one or several given servers, instead of all the servers of this group. @@ -83,6 +86,7 @@ my $fnret = OVH::Bastion::Plugin::groupSetRole::act( portAny => $portAny, host => ($ip || $host), ttl => $ttl, + comment => $comment, sudo => 0, silentoverride => 0, self => $self, diff --git a/lib/perl/OVH/Bastion/Plugin/groupSetRole.pm b/lib/perl/OVH/Bastion/Plugin/groupSetRole.pm index 318118621..6d2b38472 100644 --- a/lib/perl/OVH/Bastion/Plugin/groupSetRole.pm +++ b/lib/perl/OVH/Bastion/Plugin/groupSetRole.pm @@ -33,7 +33,7 @@ sub preconditions { if ($type eq 'guest' && !$sudo) { # guest access need (user||user-any), host and (port||port-any) - # in sudo mode, these are not used, because the helper doesn't handle the guest access add by itself, the do() func of this package does + # in sudo mode, these are not used, because the helper doesn't handle the guest access add by itself, the act() func of this package does if (!($user xor $userAny)) { return R('ERR_MISSING_PARAMETER', msg => "Require exactly one argument of user or user-any"); } @@ -144,8 +144,8 @@ sub act { # get returned untainted value my %values = %{$fnret->value()}; - my ($group, $shortGroup, $account, $type, $realm, $remoteaccount, $sysaccount) = @values{qw{ group shortGroup account type realm remoteaccount sysaccount }}; - my ($action, $self, $user, $host, $port, $ttl) = @params{qw{ action self user host port ttl }}; + my ($group, $shortGroup, $account, $type, $realm, $remoteaccount, $sysaccount) = @values{qw{ group shortGroup account type realm remoteaccount sysaccount }}; + my ($action, $self, $user, $host, $port, $ttl, $comment) = @params{qw{ action self user host port ttl comment }}; undef $user if $params{'userAny'}; undef $port if $params{'portAny'}; @@ -168,7 +168,7 @@ sub act { if ($action eq 'add' && OVH::Bastion::is_group_guest(group => $shortGroup, account => $account, sudo => $params{'sudo'})) { - # if the user is a guest, must remove all his guest accesses first + # if the user is a guest, must remove all their guest accesses first $fnret = OVH::Bastion::get_acl_way(way => 'groupguest', group => $shortGroup, account => $account); if ($fnret && $fnret->value && @{$fnret->value}) { osh_warn("This account was previously a guest of this group, with the following accesses:"); @@ -242,6 +242,9 @@ sub act { msg => "The group $shortGroup doesn't have access to $machine, so you can't add a guest group access " . "to it (first add it to the group if applicable, with groupAddServer)"); } + + # if no comment was specified for this guest access, reuse the one from the matching group ACL entry + $comment ||= $fnret->value->{'comment'}; } # If the account is already a member, can't add/del them as guest @@ -259,6 +262,7 @@ sub act { push @command, '--user', $user if $user; push @command, '--port', $port if $port; push @command, '--ttl', $ttl if $ttl; + push @command, '--comment', $comment if $comment; $fnret = OVH::Bastion::helper(cmd => \@command); $fnret or return $fnret; @@ -350,6 +354,7 @@ sub act { ['host', $host], ['port', $port], ['ttl', $ttl], + ['comment', $comment || ''], ] ); } diff --git a/lib/perl/OVH/Bastion/allowdeny.inc b/lib/perl/OVH/Bastion/allowdeny.inc index b57a9fed7..5f13143e9 100644 --- a/lib/perl/OVH/Bastion/allowdeny.inc +++ b/lib/perl/OVH/Bastion/allowdeny.inc @@ -98,7 +98,7 @@ sub is_access_way_granted { "checking way $way/$account/$group with ignorePort=$ignorePort ignoreUser=$ignoreUser exactIpMatch=$exactIpMatch exactPortMatch=$exactPortMatch exactUserMatch=$exactUserMatch" ); - my ($bestMatch, $bestMatchSize, $forceKey); + my ($bestMatch, $bestMatchSize, $bestMatchComment, $forceKey); foreach my $entry (@acl) { my $allowedIp = $entry->{'ip'}; # can be a prefix my $allowedUser = $entry->{'user'}; # can be undef (if any-user) @@ -200,10 +200,11 @@ sub is_access_way_granted { next if ($allowedIp ne $wantedIp); # here, we got a perfect match - $forceKey = $localForceKey; - $bestMatch = $allowedIp; - $bestMatchSize = undef; # not needed - last; # perfect match, don't search further + $forceKey = $localForceKey; + $bestMatch = $allowedIp; + $bestMatchComment = $entry->{'userComment'}; + $bestMatchSize = undef; # not needed + last; # perfect match, don't search further } # check IP in not-exactIpMatch case. if it contains / then it's a prefix @@ -215,9 +216,10 @@ sub is_access_way_granted { if ($ipCheck && $ipCheck->match($wantedIp)) { osh_debug("... we got a slash match !"); if (not defined $bestMatchSize or $ipCheck->size() < $bestMatchSize) { - $forceKey = $localForceKey; - $bestMatch = $allowedIp; - $bestMatchSize = $ipCheck->size(); + $forceKey = $localForceKey; + $bestMatch = $allowedIp; + $bestMatchComment = $entry->{'userComment'}; + $bestMatchSize = $ipCheck->size(); $bestMatchSize == 1 and last; # we won't get better than this } } @@ -226,16 +228,17 @@ sub is_access_way_granted { # it's a single ip, so a stupid strcmp() does the trick if ($allowedIp eq $wantedIp) { osh_debug("... we got a singleip match !"); - $forceKey = $localForceKey; - $bestMatch = $allowedIp; - $bestMatchSize = 1; + $forceKey = $localForceKey; + $bestMatch = $allowedIp; + $bestMatchComment = $entry->{'userComment'}; + $bestMatchSize = 1; last; } } } if (defined $bestMatch) { - return R('OK', value => {match => $bestMatch, size => $bestMatchSize, forceKey => $forceKey}); + return R('OK', value => {match => $bestMatch, size => $bestMatchSize, forceKey => $forceKey, comment => $bestMatchComment}); } return R('KO_ACCESS_DENIED'); } diff --git a/lib/perl/OVH/Bastion/allowkeeper.inc b/lib/perl/OVH/Bastion/allowkeeper.inc index 58205bf29..d98fade01 100644 --- a/lib/perl/OVH/Bastion/allowkeeper.inc +++ b/lib/perl/OVH/Bastion/allowkeeper.inc @@ -255,14 +255,14 @@ sub access_modify { my $ip = $params{'ip'}; # can be a single ip or prefix my $port = $params{'port'}; # if undef, means a port-wildcard access - my $ttl = $params{'ttl'}; + my $ttl = $params{'ttl'}; + my $comment = $params{'comment'}; my $way = $params{'way'}; # group, groupguest, personal my $group = $params{'group'}; # only for way=group or way=groupguest my $account = $params{'account'}; # only for way=personal my $forceKey = $params{'forceKey'}; - my $comment = $params{'comment'}; my $fnret; @@ -354,7 +354,7 @@ sub access_modify { } } - # check if the caller has the right to make the change he's asking + # check if the caller has the right to make the change they're asking # ... 1. either $> is allowkeeper and $ENV{'SUDO_USER'} is the requesting account # ... 2. or $> is $grouptomodify and $ENV{'SUDO_USER'} is the requesting account @@ -406,8 +406,6 @@ sub access_modify { ); osh_debug("... result is $fnret"); - #return $fnret if $fnret->is_err; - if ($action eq 'add' and $fnret) { return R('OK_NO_CHANGE', msg => "The requested access to add was already granted"); } @@ -415,8 +413,6 @@ sub access_modify { return R('OK_NO_CHANGE', msg => "The requested access to delete was not found, no change made"); } - # TODO for groupguest case, also check that the group has the right - # ok, now do the change, first define this sub my $_access_modify_file = sub { diff --git a/tests/functional/tests.d/340-selfaccesses.sh b/tests/functional/tests.d/340-selfaccesses.sh index df820a0b0..e98e5797f 100644 --- a/tests/functional/tests.d/340-selfaccesses.sh +++ b/tests/functional/tests.d/340-selfaccesses.sh @@ -116,7 +116,7 @@ testsuite_selfaccesses() success selfAddPersonalAccess mustwork $a0 -osh selfAddPersonalAccess -h 127.0.0.2 -u $shellaccount -p 22 --kbd-interactive nocontain "already" contain "Access to $shellaccount@127.0.0.2:22 successfully added" - json .command selfAddPersonalAccess .error_code OK .value null + json .command selfAddPersonalAccess .error_code OK .value.ip 127.0.0.2 .value.user $shellaccount .value.port 22 success selfAddPersonalAccess dupe $a0 -osh selfAddPersonalAccess -h 127.0.0.2 -u $shellaccount -p 22 --kbd-interactive contain "already" @@ -124,7 +124,7 @@ testsuite_selfaccesses() success selfAddPersonalAccess withttl $a0 -osh selfAddPersonalAccess -h 127.0.0.4 -u $shellaccount -p 22 --force --ttl 0d0h0m3s contain "Access to $shellaccount@127.0.0.4:22 successfully added (expires in 00:00:0" - json .command selfAddPersonalAccess .error_code OK .value null + json .command selfAddPersonalAccess .error_code OK .value.ip 127.0.0.4 .value.user $shellaccount .value.port 22 .value.ttl 3 run ssh a1atlo2_login8 $a0 127.0.0.2 -- id retvalshouldbe 107 diff --git a/tests/functional/tests.d/350-groups.sh b/tests/functional/tests.d/350-groups.sh index 803516653..a634ca040 100644 --- a/tests/functional/tests.d/350-groups.sh +++ b/tests/functional/tests.d/350-groups.sh @@ -889,7 +889,7 @@ EOS # group1: a1(owner,aclkeeper,gatekeeper,member) a2() servers() success groupAddServer firstadd_ok $a1 --osh groupAddServer --group $group1 --host 127.0.0.10 --port-any --user-any --force contain "was added to group" - json .command groupAddServer .error_code OK .value.group $group1 .value.host 127.0.0.10 .value.port null .value.user null + json .command groupAddServer .error_code OK .value.group $group1 .value.ip 127.0.0.10 .value.port null .value.user null # group1: a1(owner,aclkeeper,gatekeeper,member) a2() servers(127.0.0.10) success groupAddServer firstadd_dup $a1 --osh groupAddServer --group $group1 --host 127.0.0.10 --port-any --user-any --force @@ -898,13 +898,13 @@ EOS # group1: a1(owner,aclkeeper,gatekeeper,member) a2() servers(127.0.0.10) success groupAddServer secondadd $a1 --osh groupAddServer --group $group1 --host 127.0.0.11 --port-any --user-any --force contain "was added to group" - json .command groupAddServer .error_code OK .value.group $group1 .value.host 127.0.0.11 .value.port null .value.user null + json .command groupAddServer .error_code OK .value.group $group1 .value.ip 127.0.0.11 .value.port null .value.user null # group1: a1(owner,aclkeeper,gatekeeper,member) a2() servers(127.0.0.10,127.0.0.11) success groupAddServer thirdaddttl $a1 --osh groupAddServer --group $group1 --host 127.0.0.12 --port-any --user-any --force --ttl 0w19s0d contain "was added to group" contain "expires in 00:00:" - json .command groupAddServer .error_code OK .value.group $group1 .value.host 127.0.0.12 .value.port null .value.user null .value.ttl 19 + json .command groupAddServer .error_code OK .value.group $group1 .value.ip 127.0.0.12 .value.port null .value.user null .value.ttl 19 # group1: a1(owner,aclkeeper,gatekeeper,member) a2() servers(127.0.0.10,127.0.0.11,127.0.0.12-TTL) success groupListServers list $a1 --osh groupListServers --group $group1