From 80cc157d74a1483c7e43d93a0101c266119857db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Lesimple?= Date: Wed, 17 Feb 2021 14:38:59 +0000 Subject: [PATCH] enh: guests: groupAddGuestAccess now supports setting a comment If no comment is set, the comment is inherited from the group ACL, as seen in groupListServers. selfAddPersonalAccess now also return details about the added server in the returned JSON. Closes #18 Closes #17 --- bin/helper/osh-accountAddGroupServer | 4 ++- bin/helper/osh-accountModifyPersonalAccess | 18 +++++++++++++ .../group-gatekeeper/groupAddGuestAccess | 8 ++++-- lib/perl/OVH/Bastion/Plugin/groupSetRole.pm | 13 ++++++--- lib/perl/OVH/Bastion/allowdeny.inc | 27 ++++++++++--------- lib/perl/OVH/Bastion/allowkeeper.inc | 10 +++---- tests/functional/tests.d/340-selfaccesses.sh | 4 +-- tests/functional/tests.d/350-groups.sh | 6 ++--- 8 files changed, 59 insertions(+), 31 deletions(-) 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