Skip to content

Commit

Permalink
enh: guests: groupAddGuestAccess now supports setting a comment
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@speed47
speed47 committed Feb 19, 2021
1 parent 4b45887 commit d886b48
Show file tree
Hide file tree
Showing 8 changed files with 74 additions and 50 deletions.
4 changes: 3 additions & 1 deletion bin/helper/osh-accountAddGroupServer
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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] },
);
};
Expand Down Expand Up @@ -78,6 +79,7 @@ $fnret = OVH::Bastion::access_modify(
ip => $ip,
port => $port,
ttl => $ttl,
comment => $comment,
forceKey => $forceKey
);
HEXIT($fnret);
18 changes: 18 additions & 0 deletions bin/helper/osh-accountModifyPersonalAccess
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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);
8 changes: 6 additions & 2 deletions bin/plugin/group-gatekeeper/groupAddGuestAccess
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down
13 changes: 9 additions & 4 deletions lib/perl/OVH/Bastion/Plugin/groupSetRole.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand Down Expand Up @@ -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'};
Expand All @@ -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:");
Expand Down Expand Up @@ -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
Expand All @@ -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;
Expand Down Expand Up @@ -350,6 +354,7 @@ sub act {
['host', $host],
['port', $port],
['ttl', $ttl],
['comment', $comment || ''],
]
);
}
Expand Down
27 changes: 15 additions & 12 deletions lib/perl/OVH/Bastion/allowdeny.inc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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
}
}
Expand All @@ -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');
}
Expand Down
10 changes: 3 additions & 7 deletions lib/perl/OVH/Bastion/allowkeeper.inc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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 {
Expand Down
24 changes: 10 additions & 14 deletions tests/functional/tests.d/340-selfaccesses.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,16 +115,14 @@ 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
Expand Down Expand Up @@ -360,8 +358,8 @@ testsuite_selfaccesses()

#sudo usermod -a -G osh-selfDelPersonalAccess $account1
success selfDelPersonalAccess mustwork $a0 -osh selfDelPersonalAccess -h 127.0.0.2 -u $shellaccount -p 22
contain "Access to $shellaccount@127.0.0.2:22 successfully removed"
json .command selfDelPersonalAccess .error_code OK .value null
contain "Access to $shellaccount@127.0.0.2:22"
json .command selfDelPersonalAccess .error_code OK .value.ip 127.0.0.2 .value.user $shellaccount .value.port 22

run ssh shellaccountatlo2_mustfail $a1 $shellaccount@127.0.0.2 -- echo $randomstr
retvalshouldbe 107
Expand All @@ -371,8 +369,7 @@ testsuite_selfaccesses()

success selfAddPersonalAccess mustwork $a0 -osh selfAddPersonalAccess -h 127.0.0.2 -u $shellaccount -p 226
nocontain "already"
contain "Access to $shellaccount@127.0.0.2:226 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 226

# shouldn't work

Expand All @@ -391,8 +388,8 @@ testsuite_selfaccesses()
contain "$randomstr"

success selfDelPersonalAccess mustwork $a0 -osh selfDelPersonalAccess -h 127.0.0.2 -u $shellaccount -p 226
contain "Access to $shellaccount@127.0.0.2:226 successfully removed"
json .command selfDelPersonalAccess .error_code OK .value null
contain "Access to $shellaccount@127.0.0.2:226"
json .command selfDelPersonalAccess .error_code OK .value.ip 127.0.0.2 .value.user $shellaccount .value.port 226

run ssh shellaccountatlo2_mustfailnow $a0 $shellaccount@127.0.0.2 -p 226 -- echo $randomstr
retvalshouldbe 107
Expand All @@ -408,8 +405,7 @@ testsuite_selfaccesses()
success selfAddPersonalAccess nousernoport $a0 -osh selfAddPersonalAccess -h 127.0.0.4 --force
nocontain "already"
contain "Forcing add as asked"
contain "Access to 127.0.0.4 successfully added"
json .command selfAddPersonalAccess .error_code OK .value null
json .command selfAddPersonalAccess .error_code OK .value.ip 127.0.0.4 .value.port null .value.user null

run ssh rootport22 $a0 [email protected] -- echo $randomstr
retvalshouldbe 255
Expand Down Expand Up @@ -441,8 +437,8 @@ testsuite_selfaccesses()
nocontain "$randomstr"

success selfDelPersonalAccess nousernoport $a0 -osh selfDelPersonalAccess -h 127.0.0.4
contain "Access to 127.0.0.4 successfully removed"
json .command selfDelPersonalAccess .error_code OK .value null
contain "Access to 127.0.0.4 "
json .command selfDelPersonalAccess .error_code OK .value.ip 127.0.0.4 .value.port null .value.user null

success selfDelPersonalAccess nousernoport_dupe $a0 -osh selfDelPersonalAccess -h 127.0.0.4
nocontain "no longer has a personal access"
Expand Down
Loading

0 comments on commit d886b48

Please sign in to comment.