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

@@ -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);

bin/helper/osh-accountModifyPersonalAccess

@@ -91,6 +91,10 @@ if (not grep { $action eq $_ } qw{ add del }) {
 #<PARAMS:ACTION
 
 #>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);

bin/plugin/group-gatekeeper/groupAddGuestAccess

@@ -20,22 +20,25 @@ 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
 
 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
   --port PORT             allow connecting to HOST only to remote port PORT
   --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,

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 || ''],
             ]
         );
     }

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');
 }

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,17 +406,13 @@ 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");
     }
     elsif ($action eq 'del' and not $fnret) {
         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 {

tests/functional/tests.d/340-selfaccesses.sh

@@ -116,15 +116,15 @@ 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"
     json .command   selfAddPersonalAccess .error_code   OK_NO_CHANGE .value null
 
     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

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