diff --git a/lib/perl/OVH/Bastion.pm b/lib/perl/OVH/Bastion.pm index e106e6299..5c19b9ec7 100644 --- a/lib/perl/OVH/Bastion.pm +++ b/lib/perl/OVH/Bastion.pm @@ -649,48 +649,46 @@ sub _osh_log { sub is_valid_ip { my %params = @_; my $ip = $params{'ip'}; - my $allowSubnets = $params{'allowSubnets'}; # if not, a /24 or /32 notation is rejected - my $fast = $params{'fast'}; # fast mode: avoid instantiating Net::IP... except if ipv6 + my $allowSubnets = $params{'allowSubnets'}; # if not, a prefix/size (e.g. /24) notation is rejected + my $fast = $params{'fast'}; # fast mode: avoid instantiating Net::IP, except if IPv6 - if ($fast and $ip !~ m{:}) { - # fast asked and it's not an IPv6, regex ftw - ## no critic (ProhibitUnusedCapture) + if ($fast and index($ip, ':') == -1) { + # We're being asked to be fast, and it's not an IPv6, just use a regex + # and don't instanciate a Net::IP. Also don't use named captures, as they're slower if ( - $ip =~ m{^(? - (?[0-9]{1,3}) - \. - (?[0-9]{1,3}) - \. - (?[0-9]{1,3}) - \. - (?[0-9]{1,3}) - ) - ( - (?/) - (?\d+) - )? + $ip =~ m{^ + (?: + ([0-9]{1,3})\. + ([0-9]{1,3})\. + ([0-9]{1,3})\. + ([0-9]{1,3}) + ) + (?:/ + ([0-9]{1,2}) + )? $}x ) { - if (defined $+{'prefixlen'} and not $allowSubnets) { - return R('KO_INVALID_IP', msg => "Invalid IP address ($ip), as subnets are not allowed"); - } - foreach my $key (qw{ x1 x2 x3 x4 }) { - return R('KO_INVALID_IP', msg => "Invalid IP address ($ip)") - if (not defined $+{$key} or $+{$key} > 255); - } - if (defined $+{'prefixlen'} and $+{'prefixlen'} > 32) { - return R('KO_INVALID_IP', msg => "Invalid IP address ($ip)"); - } - if (defined $+{'slash'} and not defined $+{'prefixlen'}) { - # got a / in $ip but it's not followed by \d+ + if ($1 > 255 || $2 > 255 || $3 > 255 || $4 > 255) { return R('KO_INVALID_IP', msg => "Invalid IP address ($ip)"); } - - if (defined $+{'prefixlen'} && $+{'prefixlen'} != 32) { - return R('OK', value => {ip => $ip, prefixlen => $+{'prefixlen'}}); + # int() to remove useless zeroes such as 192.00.002.03 => 192.0.2.3 + my $dottedquad = int($1) . '.' . int($2) . '.' . int($3) . '.' . int($4); + if (defined $5) { + if (!$allowSubnets && $5 != 32) { + return R('KO_INVALID_IP', msg => "Invalid IP address ($ip), as subnets are not allowed"); + } + if ($5 > 32) { + return R('KO_INVALID_IP', msg => "Invalid IP address ($ip)"); + } + # valid prefixlen? + if (((2**24 * $1 + 2**16 * $2 + 2**8 * $3 + $4) & (2**(32 - $5) - 1)) != 0) { + return R('KO_INVALID_IP', msg => "Invalid prefix length ($5) for this prefix ($dottedquad)"); + } + return R('OK', value => {ip => "$dottedquad/$5", prefix => $5}) if ($5 != 32); + # if prefixlen == 32, fallthrough and return the $dottedquad as a single IP below } - return R('OK', value => {ip => $+{'shortip'}}); + return R('OK', value => {ip => $dottedquad}); } return R('KO_INVALID_IP', msg => "Invalid IP address ($ip)"); } @@ -710,8 +708,8 @@ sub is_valid_ip { $type = 'single'; } else { - $shortip = $IpObject->prefix; - $type = 'prefix'; + $shortip = $IpObject->prefix; # what Net::IP calls a prefix is in fact the subnet in CIDR notation + $type = 'subnet'; } } elsif ($IpObject->version == 6) { @@ -721,19 +719,18 @@ sub is_valid_ip { } else { $shortip = $IpObject->short . '/' . $IpObject->prefixlen; - $type = 'prefix'; + $type = 'subnet'; } } - if (not $allowSubnets and $type eq 'prefix') { - return R('KO_INVALID_IP', msg => "Invalid IP address ($ip), as prefixes are not allowed"); + if (!$allowSubnets && $type eq 'subnet') { + return R('KO_INVALID_IP', msg => "Invalid IP address ($ip), as subnets are not allowed"); } return R( 'OK', value => { ip => $shortip, - prefix => $IpObject->prefix, prefixlen => $IpObject->prefixlen, version => $IpObject->version, type => $type diff --git a/lib/perl/OVH/Bastion/allowdeny.inc b/lib/perl/OVH/Bastion/allowdeny.inc index 08933d117..4f4397fdc 100644 --- a/lib/perl/OVH/Bastion/allowdeny.inc +++ b/lib/perl/OVH/Bastion/allowdeny.inc @@ -949,8 +949,8 @@ sub ssh_test_access_way { $fnret = OVH::Bastion::is_valid_ip(ip => $ip, allowSubnets => 1); $fnret or return $fnret; - if ($fnret->value->{'type'} eq 'prefix') { - return R('OK_PREFIX', msg => "Can't test a connection to a prefix, assuming it's OK"); + if ($fnret->value->{'type'} eq 'subnet') { + return R('OK_SUBNET', msg => "Can't test a connection to a subnet, assuming it's OK"); } $ip = $fnret->value->{'ip'}; diff --git a/lib/perl/OVH/Bastion/allowkeeper.inc b/lib/perl/OVH/Bastion/allowkeeper.inc index b3efefb91..a4c52ac67 100644 --- a/lib/perl/OVH/Bastion/allowkeeper.inc +++ b/lib/perl/OVH/Bastion/allowkeeper.inc @@ -371,7 +371,7 @@ sub access_modify { return $fnret unless $fnret; $ip = $fnret->value->{'ip'}; - if ($fnret->value->{'type'} eq 'prefix') { + if ($fnret->value->{'type'} eq 'subnet') { my $ipVersion = $fnret->value->{'version'}; if (defined $widestVxPrefix{$ipVersion} && $fnret->value->{'prefixlen'} < $widestVxPrefix{$ipVersion}) { return R( diff --git a/tests/functional/tests.d/340-selfaccesses.sh b/tests/functional/tests.d/340-selfaccesses.sh index 264cf8ac9..f28ca7f15 100644 --- a/tests/functional/tests.d/340-selfaccesses.sh +++ b/tests/functional/tests.d/340-selfaccesses.sh @@ -115,7 +115,7 @@ testsuite_selfaccesses() plgfail selfAddPersonalAccess_too_wide $a0 --osh selfAddPersonalAccess --host 127.0.0.0/8 --user $account0 --port-any json .error_code ERR_INVALID_PARAMETER - contain "IPv4 is /30 by this" + contain "IPv4 is /30 by policy" success selfAddPersonalAccess_constraints_ok $a0 --osh selfAddPersonalAccess --host 127.0.0.9 --user $account0 --port '*' --ttl 1 --force @@ -130,7 +130,7 @@ testsuite_selfaccesses() plgfail accountAddPersonalAccess_too_wide $a0 --osh accountAddPersonalAccess --host 127.0.0.0/8 --user $account1 --port-any --account $account1 json .error_code ERR_INVALID_PARAMETER - contain "IPv4 is /30 by this" + contain "IPv4 is /30 by policy" success accountAddPersonalAccess_constaints_ok $a0 --osh accountAddPersonalAccess --host 127.0.0.9 --user $account1 --port '*' --ttl 1 --account $account1 diff --git a/tests/unit/tests/is_valid_ip.t b/tests/unit/tests/is_valid_ip.t new file mode 100644 index 000000000..4665a2784 --- /dev/null +++ b/tests/unit/tests/is_valid_ip.t @@ -0,0 +1,51 @@ +#! /usr/bin/env perl +# vim: set filetype=perl ts=4 sw=4 sts=4 et: +use common::sense; +use Test::More; +use Test::Deep; + +use File::Basename; +use lib dirname(__FILE__) . '/../../../lib/perl'; +use OVH::Bastion; +use OVH::Result; + +OVH::Bastion::enable_mocking(); +OVH::Bastion::set_mock_data({}); +OVH::Bastion::load_configuration( + mock_data => { + bastionName => "mock", + } +); + +foreach my $testip ( + qw{ + 0.0.0.0 + 239.0.0.0 + 225.225.0.0 + 192.0.2.0 + 192.0.2.1 + 192.0.2.128 + 192.0.2.147 + 255.255.255.255 + 255.256.255.255 + 192.00.0002.002 + } + ) +{ + + foreach my $allowPrefixes (0 .. 1) { + foreach my $prefixlen (0 .. 33) { + my $ip = $testip . ($prefixlen != 33 ? "/$prefixlen" : ""); + my $Fast = OVH::Bastion::is_valid_ip(ip => $ip, allowPrefixes => $allowPrefixes, fast => 1); + my $Slow = OVH::Bastion::is_valid_ip(ip => $ip, allowPrefixes => $allowPrefixes, fast => 0); + # both should have the same error code and returned IP if any: + cmp_deeply( + {err => $Fast->err, value_ip => ($Fast->value ? $Fast->value->{'ip'} : undef)}, + {err => $Slow->err, value_ip => ($Slow->value ? $Slow->value->{'ip'} : undef)}, + "is_valid_ip(ip=$ip, allowPrefixes=$allowPrefixes)" + ); + } + } +} + +done_testing();