Skip to content

Commit

Permalink
enh: 35% faster is_valid_ip() when fast=1
Browse files Browse the repository at this point in the history
  • Loading branch information
@speed47
speed47 committed Jan 3, 2025
1 parent d2d47ba commit 82572e8
Show file tree
Hide file tree
Showing 5 changed files with 93 additions and 45 deletions.
77 changes: 37 additions & 40 deletions lib/perl/OVH/Bastion.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{^(?<shortip>
(?<x1>[0-9]{1,3})
\.
(?<x2>[0-9]{1,3})
\.
(?<x3>[0-9]{1,3})
\.
(?<x4>[0-9]{1,3})
)
(
(?<slash>/)
(?<prefixlen>\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)");
}
Expand All @@ -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) {
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions lib/perl/OVH/Bastion/allowdeny.inc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'};

Expand Down
2 changes: 1 addition & 1 deletion lib/perl/OVH/Bastion/allowkeeper.inc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
4 changes: 2 additions & 2 deletions tests/functional/tests.d/340-selfaccesses.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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

Expand Down
51 changes: 51 additions & 0 deletions tests/unit/tests/is_valid_ip.t
View file
Original file line number Diff line number Diff line change
@@ -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();

0 comments on commit 82572e8

Please sign in to comment.