From d2d47ba6cecb2697ebf4dd0f467ab1472cd43ea8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Lesimple?= Date: Thu, 19 Dec 2024 13:51:01 +0000 Subject: [PATCH 1/2] chore: use proper naming of 'subnet' instead of 'prefix' or 'slash' To avoid confusion, we now use 'subnet' to talk about a subnet represented with the CIDR notation, such as 10.0.0.0/8. In in that case: - 10.0.0.0/8 is a 'subnet' - 10.0.0.0 is the 'prefix' - 8 is the 'prefix length', or by extension the 'subnet length' Use these words everywhere in the code and documentation for clarity. --- bin/plugin/group-aclkeeper/groupAddServer | 6 ++-- bin/plugin/group-aclkeeper/groupDelServer | 4 +-- bin/plugin/group-aclkeeper/groupSetServers | 4 +-- .../group-gatekeeper/groupAddGuestAccess | 4 +-- .../group-gatekeeper/groupDelGuestAccess | 4 +-- bin/plugin/open/alive | 2 +- bin/plugin/open/mtr | 2 +- bin/plugin/open/nc | 2 +- bin/plugin/open/ping | 2 +- .../restricted/accountAddPersonalAccess | 8 ++--- .../restricted/accountDelPersonalAccess | 4 +-- bin/plugin/restricted/assetForgetHostKey | 4 +-- bin/plugin/restricted/selfAddPersonalAccess | 8 ++--- bin/plugin/restricted/selfDelPersonalAccess | 4 +-- bin/shell/osh.pl | 12 +++---- .../configuration/bastion_conf.rst | 14 ++++---- .../group-aclkeeper/groupAddServer.rst | 4 +-- .../group-aclkeeper/groupDelServer.rst | 4 +-- .../group-gatekeeper/groupAddGuestAccess.rst | 4 +-- .../group-gatekeeper/groupDelGuestAccess.rst | 4 +-- .../restricted/accountAddPersonalAccess.rst | 10 +++--- .../restricted/accountDelPersonalAccess.rst | 4 +-- .../restricted/selfAddPersonalAccess.rst | 10 +++--- .../restricted/selfDelPersonalAccess.rst | 4 +-- etc/bastion/bastion.conf.dist | 14 ++++---- lib/perl/OVH/Bastion.pm | 24 ++++++------- lib/perl/OVH/Bastion/Plugin.pm | 4 +-- lib/perl/OVH/Bastion/allowdeny.inc | 36 +++++++++---------- lib/perl/OVH/Bastion/allowkeeper.inc | 4 +-- lib/perl/OVH/Bastion/ssh.inc | 4 +-- tests/functional/tests.d/350-groups.sh | 6 ++-- 31 files changed, 110 insertions(+), 110 deletions(-) diff --git a/bin/plugin/group-aclkeeper/groupAddServer b/bin/plugin/group-aclkeeper/groupAddServer index 79b487a33..2698cfe21 100755 --- a/bin/plugin/group-aclkeeper/groupAddServer +++ b/bin/plugin/group-aclkeeper/groupAddServer @@ -16,7 +16,7 @@ my $remainingOptions = OVH::Bastion::Plugin::begin( options => { "group=s" => \my $group, "protocol=s" => \my $protocol, - "force" => \my $force, # for slashes, and/or for servers that are down (no connection test) + "force" => \my $force, # for subnets, and/or for servers that are down (no connection test) "force-key=s" => \my $forceKey, "force-password=s" => \my $forcePassword, "ttl=s" => \my $ttl, @@ -34,8 +34,8 @@ Add an IP or IP block to a group's servers list Usage: --osh SCRIPT_NAME --group GROUP --host HOST --user USER|* --port PORT|* [OPTIONS] --group GROUP Specify which group this machine should be added to - --host HOST|IP|NET/CIDR Host(s) to add access to, either a HOST which will be resolved to an IP immediately, - or an IP, or a whole network using the NET/CIDR notation + --host HOST|IP|SUBNET Host(s) to add access to, either a HOST which will be resolved to an IP immediately, + or an IP, or a whole subnet using the PREFIX/SIZE notation --user USER|PATTERN|* Specify which remote user should be allowed to connect as. Globbing characters '*' and '?' are supported, so you can specify a pattern that will be matched against the actual remote user name. diff --git a/bin/plugin/group-aclkeeper/groupDelServer b/bin/plugin/group-aclkeeper/groupDelServer index f1be07ef7..3f58d8f79 100755 --- a/bin/plugin/group-aclkeeper/groupDelServer +++ b/bin/plugin/group-aclkeeper/groupDelServer @@ -30,8 +30,8 @@ Remove an IP or IP block from a group's server list Usage: --osh SCRIPT_NAME --group GROUP --host HOST --user USER --port PORT [OPTIONS] --group GROUP Specify which group this machine should be removed from - --host HOST|IP|NET/CIDR Host(s) to remove access from, either a HOST which will be resolved to an IP immediately, - or an IP, or a whole network using the NET/CIDR notation + --host HOST|IP|SUBNET Host(s) to remove access from, either a HOST which will be resolved to an IP immediately, + or an IP, or a whole subnet using the PREFIX/SIZE notation --user USER|PATTERN|* Specify which remote user was allowed to connect as. Globbing characters '*' and '?' are supported, so you can specify a pattern that will be matched against the actual remote user name. diff --git a/bin/plugin/group-aclkeeper/groupSetServers b/bin/plugin/group-aclkeeper/groupSetServers index 09d8ac421..1a5323292 100755 --- a/bin/plugin/group-aclkeeper/groupSetServers +++ b/bin/plugin/group-aclkeeper/groupSetServers @@ -115,9 +115,9 @@ while (my $line = ) { $acl_user = $fnret->value; } - # resolve host, unless it looks like a prefix + # resolve host, unless it looks like a subnet if ($acl_host =~ m{/}) { - $fnret = OVH::Bastion::is_valid_ip(ip => $acl_host, allowPrefixes => 1); + $fnret = OVH::Bastion::is_valid_ip(ip => $acl_host, allowSubnets => 1); } else { $fnret = OVH::Bastion::get_ip(host => $acl_host); diff --git a/bin/plugin/group-gatekeeper/groupAddGuestAccess b/bin/plugin/group-gatekeeper/groupAddGuestAccess index 934134d52..800d7259f 100755 --- a/bin/plugin/group-gatekeeper/groupAddGuestAccess +++ b/bin/plugin/group-gatekeeper/groupAddGuestAccess @@ -35,8 +35,8 @@ Usage: --osh SCRIPT_NAME --group GROUP --account ACCOUNT [OPTIONS] --account ACCOUNT Name of the other bastion account to add access to, they'll be given access to the GROUP key --group GROUP Group to add the guest access to, note that this group should already have access to the USER/HOST/PORT tuple you'll specify with the options below. - --host HOST|IP|NET/CIDR Host(s) to add access to, either a HOST which will be resolved to an IP immediately, - or an IP, or a whole network using the NET/CIDR notation + --host HOST|IP|SUBNET Host(s) to add access to, either a HOST which will be resolved to an IP immediately, + or an IP, or a whole subnet using the PREFIX/SIZE notation --user USER|PATTERN|* Specify which remote user should be allowed to connect as. Globbing characters '*' and '?' are supported, so you can specify a pattern that will be matched against the actual remote user name. diff --git a/bin/plugin/group-gatekeeper/groupDelGuestAccess b/bin/plugin/group-gatekeeper/groupDelGuestAccess index 52011d721..2966faa19 100755 --- a/bin/plugin/group-gatekeeper/groupDelGuestAccess +++ b/bin/plugin/group-gatekeeper/groupDelGuestAccess @@ -32,8 +32,8 @@ Usage: --osh SCRIPT_NAME --group GROUP --account ACCOUNT [OPTIONS] --account ACCOUNT Bastion account remove the guest access from --group GROUP Specify which group to remove the guest access to ACCOUNT from - --host HOST|IP|NET/CIDR Host(s) to remove access from, either a HOST which will be resolved to an IP immediately, - or an IP, or a whole network using the NET/CIDR notation + --host HOST|IP|SUBNET Host(s) to remove access from, either a HOST which will be resolved to an IP immediately, + or an IP, or a whole subnet using the PREFIX/SIZE notation --user USER|PATTERN|* Specify which remote user was allowed to connect as. Globbing characters '*' and '?' are supported, so you can specify a pattern that will be matched against the actual remote user name. diff --git a/bin/plugin/open/alive b/bin/plugin/open/alive index a4c3a3f47..2fa49c1f5 100755 --- a/bin/plugin/open/alive +++ b/bin/plugin/open/alive @@ -53,7 +53,7 @@ if (not $host) { if ($host =~ m{/}) { help(); - osh_exit 'ERR_INVALID_PARAMETER', "Please use a single IP, not a netblock"; + osh_exit 'ERR_INVALID_PARAMETER', "Please use a single IP, not a subnet"; } osh_info "Waiting for $host to be alive..."; diff --git a/bin/plugin/open/mtr b/bin/plugin/open/mtr index b7f0c62fd..dfab14271 100755 --- a/bin/plugin/open/mtr +++ b/bin/plugin/open/mtr @@ -44,7 +44,7 @@ if (not $host) { if ($host =~ m{/}) { help(); - osh_exit 'ERR_INVALID_PARAMETER', "Please use a single IP, not a netblock"; + osh_exit 'ERR_INVALID_PARAMETER', "Please use a single IP, not a subnet"; } my @command = qw{ mtr --show-ips --aslookup -n }; diff --git a/bin/plugin/open/nc b/bin/plugin/open/nc index faa0cbfe5..7df84899b 100755 --- a/bin/plugin/open/nc +++ b/bin/plugin/open/nc @@ -61,7 +61,7 @@ if (!$fnret) { if ($host =~ m{/}) { help(); - osh_exit 'ERR_INVALID_PARAMETER', "Please use a single IP, not a netblock"; + osh_exit 'ERR_INVALID_PARAMETER', "Please use a single IP, not a subnet"; } osh_info "Checking whether TCP port $port of $host is reachable..."; diff --git a/bin/plugin/open/ping b/bin/plugin/open/ping index 00e07281c..c7d338e42 100755 --- a/bin/plugin/open/ping +++ b/bin/plugin/open/ping @@ -55,7 +55,7 @@ if (not $host) { if ($host =~ m{/}) { help(); - osh_exit 'ERR_INVALID_PARAMETER', "Please use a single IP, not a netblock"; + osh_exit 'ERR_INVALID_PARAMETER', "Please use a single IP, not a subnet"; } my @command = qw{ ping }; diff --git a/bin/plugin/restricted/accountAddPersonalAccess b/bin/plugin/restricted/accountAddPersonalAccess index 0cef0c9b8..06abe614f 100755 --- a/bin/plugin/restricted/accountAddPersonalAccess +++ b/bin/plugin/restricted/accountAddPersonalAccess @@ -34,8 +34,8 @@ Add a personal server access to an account Usage: --osh SCRIPT_NAME --account ACCOUNT --host HOST --user USER --port PORT [OPTIONS] --account Bastion account to add the access to - --host HOST|IP|NET/CIDR Host(s) to add access to, either a HOST which will be resolved to an IP immediately, - or an IP, or a whole network using the NET/CIDR notation + --host HOST|IP|SUBNET Host(s) to add access to, either a HOST which will be resolved to an IP immediately, + or an IP, or a whole subnet using the PREFIX/SIZE notation --user USER|PATTERN|* Specify which remote user should be allowed to connect as. Globbing characters '*' and '?' are supported, so you can specify a pattern that will be matched against the actual remote user name. @@ -66,8 +66,8 @@ EOF =head2 widest_v4_prefix (optional, integer, between 0 and 32) -When specified, this limits the size of prefixes that can be added to an ACL, e.g. 24 would not allow -prefixes wider than /24 (such as /20 or /16). +When specified, this limits the size of subnets that can be added to an ACL, e.g. 24 would not allow +prefix lengths wider than /24 (such as /20 or /16). Note that this doesn't prevent users from adding thousands of ACLs to cover a wide range of networks, but this helps ensuring ACLs such as 0.0.0.0/0 can't be added in a single command. diff --git a/bin/plugin/restricted/accountDelPersonalAccess b/bin/plugin/restricted/accountDelPersonalAccess index 5d9525282..f57438d21 100755 --- a/bin/plugin/restricted/accountDelPersonalAccess +++ b/bin/plugin/restricted/accountDelPersonalAccess @@ -29,8 +29,8 @@ Remove a personal server access from an account Usage: --osh SCRIPT_NAME --account ACCOUNT --host HOST --user USER --port PORT [OPTIONS] --account Bastion account to remove access from - --host HOST|IP|NET/CIDR Host(s) to remove access from, either a HOST which will be resolved to an IP immediately, - or an IP, or a whole network using the NET/CIDR notation + --host HOST|IP|SUBNET Host(s) to remove access from, either a HOST which will be resolved to an IP immediately, + or an IP, or a whole subnet using the PREFIX/SIZE notation --user USER|PATTERN|* Specify which remote user was allowed to connect as. Globbing characters '*' and '?' are supported, so you can specify a pattern that will be matched against the actual remote user name. diff --git a/bin/plugin/restricted/assetForgetHostKey b/bin/plugin/restricted/assetForgetHostKey index 75eab0e72..dbd3f0418 100755 --- a/bin/plugin/restricted/assetForgetHostKey +++ b/bin/plugin/restricted/assetForgetHostKey @@ -27,10 +27,10 @@ if (!$ip) { osh_exit 'ERR_MISSING_PARAMETER', "Missing mandatory parameter --host (or host didn't resolve correctly)"; } -# IP can't be a prefix +# IP can't be a subnet if ($ip =~ m{/}) { help(); - osh_exit 'ERR_INVALID_PARAMETER', "Specified IP must not be a prefix ($ip)"; + osh_exit 'ERR_INVALID_PARAMETER', "Specified IP must not be a subnet ($ip)"; } osh_info "Removing $ip host key from accounts..."; diff --git a/bin/plugin/restricted/selfAddPersonalAccess b/bin/plugin/restricted/selfAddPersonalAccess index 506cbd060..a91ea7c06 100755 --- a/bin/plugin/restricted/selfAddPersonalAccess +++ b/bin/plugin/restricted/selfAddPersonalAccess @@ -33,8 +33,8 @@ Add a personal server access to your account Usage: --osh SCRIPT_NAME --host HOST --user USER --port PORT [OPTIONS] - --host HOST|IP|NET/CIDR Host(s) to add access to, either a HOST which will be resolved to an IP immediately, - or an IP, or a whole network using the NET/CIDR notation + --host HOST|IP|SUBNET Host(s) to add access to, either a HOST which will be resolved to an IP immediately, + or an IP, or a whole subnet using the PREFIX/SIZE notation --user USER|PATTERN|* Specify which remote user should be allowed to connect as. Globbing characters '*' and '?' are supported, so you can specify a pattern that will be matched against the actual remote user name. @@ -63,8 +63,8 @@ EOF =head2 widest_v4_prefix (optional, integer, between 0 and 32) -When specified, this limits the size of prefixes that can be added to an ACL, e.g. 24 would not allow -prefixes wider than /24 (such as /20 or /16). +When specified, this limits the size of subnets that can be added to an ACL, e.g. 24 would not allow +prefix lengths wider than /24 (such as /20 or /16). Note that this doesn't prevent users from adding thousands of ACLs to cover a wide range of networks, but this helps ensuring ACLs such as 0.0.0.0/0 can't be added in a single command. diff --git a/bin/plugin/restricted/selfDelPersonalAccess b/bin/plugin/restricted/selfDelPersonalAccess index 356678a33..2da715750 100755 --- a/bin/plugin/restricted/selfDelPersonalAccess +++ b/bin/plugin/restricted/selfDelPersonalAccess @@ -27,8 +27,8 @@ Remove a personal server access from your account Usage: --osh SCRIPT_NAME --host HOST --user USER --port PORT [OPTIONS] - --host HOST|IP|NET/CIDR Host(s) to remove access from, either a HOST which will be resolved to an IP immediately, - or an IP, or a whole network using the NET/CIDR notation + --host HOST|IP|SUBNET Host(s) to remove access from, either a HOST which will be resolved to an IP immediately, + or an IP, or a whole subnet using the PREFIX/SIZE notation --user USER|PATTERN|* Specify which remote user was allowed to connect as. Globbing characters '*' and '?' are supported, so you can specify a pattern that will be matched against the actual remote user name. diff --git a/bin/shell/osh.pl b/bin/shell/osh.pl index 401107778..73dd07f66 100755 --- a/bin/shell/osh.pl +++ b/bin/shell/osh.pl @@ -610,16 +610,16 @@ sub main_exit { # if: avoid loading Net::IP and BigInt if there's no host specified if ($host) { - # can be an IP (v4 or v6), hostname, or netblock (with a /) + # can be an IP (v4 or v6), hostname, or subnet (with a /) if ($host !~ m{^\[?[a-zA-Z0-9._/:-]+\]?$}) { main_exit OVH::Bastion::EXIT_INVALID_REMOTE_HOST, 'invalid_remote_host', "Remote host name '$host' seems invalid"; } - # netblocks are only allowed for plugins + # subnets are only allowed for plugins if (index($host, '/') != -1 && !$osh_command) { main_exit OVH::Bastion::EXIT_INVALID_REMOTE_HOST, 'invalid_remote_host', - "Remote host '$host' looks like a netblock, can't connect to that"; + "Remote host '$host' looks like a subnet, can't connect to that"; } # probably this "host" is in fact an option, but we didn't parse it because it's an unknown one, @@ -630,12 +630,12 @@ sub main_exit { } # otherwise, resolve the host - $fnret = OVH::Bastion::get_ip(host => $host, allowPrefixes => ($osh_command ? 1 : 0)); + $fnret = OVH::Bastion::get_ip(host => $host, allowSubnets => ($osh_command ? 1 : 0)); - # if it's a netblock but get_ip() sends an error, it's an invalid netblock + # if it's a subnet but get_ip() sends an error, it's an invalid subnet if (!$fnret && index($host, '/') != -1) { main_exit OVH::Bastion::EXIT_INVALID_REMOTE_HOST, 'invalid_remote_host', - "Remote host '$host' looks like a netblock, but with an invalid prefix"; + "Remote host '$host' looks like a subnet, but with an invalid prefix"; } } diff --git a/doc/sphinx/administration/configuration/bastion_conf.rst b/doc/sphinx/administration/configuration/bastion_conf.rst index bc341c2af..876afc811 100644 --- a/doc/sphinx/administration/configuration/bastion_conf.rst +++ b/doc/sphinx/administration/configuration/bastion_conf.rst @@ -366,7 +366,7 @@ If set to 0, The Bastion will never attempt to do DNS or reverse-DNS resolutions allowedNetworks *************** -:Type: ``array of strings (IPs and/or prefixes)`` +:Type: ``array of strings (IPs and/or subnets)`` :Default: ``[]`` @@ -379,13 +379,13 @@ Restricts egress connection attempts to those listed networks only. This is enfo forbiddenNetworks ***************** -:Type: ``array of strings (IPs and/or prefixes)`` +:Type: ``array of strings (IPs and/or subnets)`` :Default: ``[]`` :Example: ``["10.42.42.0/24"]`` -Prevents egress connection to the listed networks, this takes precedence over ``allowedNetworks``. This can be used to prevent connection to some hosts or subnets in a broadly allowed prefix. This is enforced at all times and can NOT be overridden by users. +Prevents egress connection to the listed networks, this takes precedence over ``allowedNetworks``. This can be used to prevent connection to some hosts or subnets in a broadly allowed subnet. This is enforced at all times and can NOT be overridden by users. .. _ingressToEgressRules: @@ -575,11 +575,11 @@ Other ingress policies ingressKeysFrom *************** -:Type: ``array of strings (list of IPs and/or prefixes)`` +:Type: ``array of strings (list of IPs and/or subnets)`` :Default: ``[]`` -This array of IPs (or prefixes, such as ``10.20.30.0/24``) will be used to build the ``from="..."`` in front of the ingress account public keys used to connect to the bastion (in ``accountCreate`` or ``selfAddIngressKey``). If the array is empty, then **NO** ``from="..."`` is added (this lowers the security). +This array of IPs (or subnets, such as ``10.20.30.0/24``) will be used to build the ``from="..."`` in front of the ingress account public keys used to connect to the bastion (in ``accountCreate`` or ``selfAddIngressKey``). If the array is empty, then **NO** ``from="..."`` is added (this lowers the security). .. _ingressKeysFromAllowOverride: @@ -613,7 +613,7 @@ The default remote user to use for egress ssh connections where no user has been egressKeysFrom ************** -:Type: ``array of strings (IPs and/or prefixes)`` +:Type: ``array of strings (IPs and/or subnets)`` :Default: ``[]`` @@ -864,7 +864,7 @@ List of system groups to add a new account to when its created (see ``accountCre accountCreateDefaultPersonalAccesses ************************************ -:Type: ``array of strings (list of IPs and/or prefixes)`` +:Type: ``array of strings (list of IPs and/or subnets)`` :Default: ``[]`` diff --git a/doc/sphinx/plugins/group-aclkeeper/groupAddServer.rst b/doc/sphinx/plugins/group-aclkeeper/groupAddServer.rst index ccd600b13..a8d3070af 100644 --- a/doc/sphinx/plugins/group-aclkeeper/groupAddServer.rst +++ b/doc/sphinx/plugins/group-aclkeeper/groupAddServer.rst @@ -18,11 +18,11 @@ Add an IP or IP block to a group's servers list Specify which group this machine should be added to -.. option:: --host HOST|IP|NET/CIDR +.. option:: --host HOST|IP|SUBNET Host(s) to add access to, either a HOST which will be resolved to an IP immediately, - or an IP, or a whole network using the NET/CIDR notation + or an IP, or a whole subnet using the PREFIX/SIZE notation --user USER|PATTERN|* Specify which remote user should be allowed to connect as. Globbing characters '*' and '?' are supported, so you can specify a pattern that will be matched against the actual remote user name. diff --git a/doc/sphinx/plugins/group-aclkeeper/groupDelServer.rst b/doc/sphinx/plugins/group-aclkeeper/groupDelServer.rst index 10ca738fc..4c90666cb 100644 --- a/doc/sphinx/plugins/group-aclkeeper/groupDelServer.rst +++ b/doc/sphinx/plugins/group-aclkeeper/groupDelServer.rst @@ -18,11 +18,11 @@ Remove an IP or IP block from a group's server list Specify which group this machine should be removed from -.. option:: --host HOST|IP|NET/CIDR +.. option:: --host HOST|IP|SUBNET Host(s) to remove access from, either a HOST which will be resolved to an IP immediately, - or an IP, or a whole network using the NET/CIDR notation + or an IP, or a whole subnet using the PREFIX/SIZE notation --user USER|PATTERN|* Specify which remote user was allowed to connect as. Globbing characters '*' and '?' are supported, so you can specify a pattern that will be matched against the actual remote user name. diff --git a/doc/sphinx/plugins/group-gatekeeper/groupAddGuestAccess.rst b/doc/sphinx/plugins/group-gatekeeper/groupAddGuestAccess.rst index 2f65cc208..000b606bc 100644 --- a/doc/sphinx/plugins/group-gatekeeper/groupAddGuestAccess.rst +++ b/doc/sphinx/plugins/group-gatekeeper/groupAddGuestAccess.rst @@ -23,11 +23,11 @@ Add a specific group server access to an account Group to add the guest access to, note that this group should already have access to the USER/HOST/PORT tuple you'll specify with the options below. -.. option:: --host HOST|IP|NET/CIDR +.. option:: --host HOST|IP|SUBNET Host(s) to add access to, either a HOST which will be resolved to an IP immediately, - or an IP, or a whole network using the NET/CIDR notation + or an IP, or a whole subnet using the PREFIX/SIZE notation --user USER|PATTERN|* Specify which remote user should be allowed to connect as. Globbing characters '*' and '?' are supported, so you can specify a pattern that will be matched against the actual remote user name. diff --git a/doc/sphinx/plugins/group-gatekeeper/groupDelGuestAccess.rst b/doc/sphinx/plugins/group-gatekeeper/groupDelGuestAccess.rst index 6bb0f87bc..6717a2b01 100644 --- a/doc/sphinx/plugins/group-gatekeeper/groupDelGuestAccess.rst +++ b/doc/sphinx/plugins/group-gatekeeper/groupDelGuestAccess.rst @@ -22,11 +22,11 @@ Remove a specific group server access from an account Specify which group to remove the guest access to ACCOUNT from -.. option:: --host HOST|IP|NET/CIDR +.. option:: --host HOST|IP|SUBNET Host(s) to remove access from, either a HOST which will be resolved to an IP immediately, - or an IP, or a whole network using the NET/CIDR notation + or an IP, or a whole subnet using the PREFIX/SIZE notation --user USER|PATTERN|* Specify which remote user was allowed to connect as. Globbing characters '*' and '?' are supported, so you can specify a pattern that will be matched against the actual remote user name. diff --git a/doc/sphinx/plugins/restricted/accountAddPersonalAccess.rst b/doc/sphinx/plugins/restricted/accountAddPersonalAccess.rst index d3940ea1b..843ecd756 100644 --- a/doc/sphinx/plugins/restricted/accountAddPersonalAccess.rst +++ b/doc/sphinx/plugins/restricted/accountAddPersonalAccess.rst @@ -18,11 +18,11 @@ Add a personal server access to an account Bastion account to add the access to -.. option:: --host HOST|IP|NET/CIDR +.. option:: --host HOST|IP|SUBNET Host(s) to add access to, either a HOST which will be resolved to an IP immediately, - or an IP, or a whole network using the NET/CIDR notation + or an IP, or a whole subnet using the PREFIX/SIZE notation --user USER|PATTERN|* Specify which remote user should be allowed to connect as. Globbing characters '*' and '?' are supported, so you can specify a pattern that will be matched against the actual remote user name. @@ -68,9 +68,9 @@ Options .. option:: widest_v4_prefix (optional, integer, between 0 and 32) - When specified, this limits the size of prefixes that can be added to an - ACL, e.g. 24 would not allow prefixes wider than /24 (such as /20 or - /16). + When specified, this limits the size of subnets that can be added to an + ACL, e.g. 24 would not allow prefix lengths wider than /24 (such as /20 + or /16). Note that this doesn't prevent users from adding thousands of ACLs to cover a wide range of networks, but this helps ensuring ACLs such as 0.0.0.0/0 can't be added in a single command. diff --git a/doc/sphinx/plugins/restricted/accountDelPersonalAccess.rst b/doc/sphinx/plugins/restricted/accountDelPersonalAccess.rst index 600f24755..a393432a7 100644 --- a/doc/sphinx/plugins/restricted/accountDelPersonalAccess.rst +++ b/doc/sphinx/plugins/restricted/accountDelPersonalAccess.rst @@ -18,11 +18,11 @@ Remove a personal server access from an account Bastion account to remove access from -.. option:: --host HOST|IP|NET/CIDR +.. option:: --host HOST|IP|SUBNET Host(s) to remove access from, either a HOST which will be resolved to an IP immediately, - or an IP, or a whole network using the NET/CIDR notation + or an IP, or a whole subnet using the PREFIX/SIZE notation --user USER|PATTERN|* Specify which remote user was allowed to connect as. Globbing characters '*' and '?' are supported, so you can specify a pattern that will be matched against the actual remote user name. diff --git a/doc/sphinx/plugins/restricted/selfAddPersonalAccess.rst b/doc/sphinx/plugins/restricted/selfAddPersonalAccess.rst index f814b90b3..c37ea5078 100644 --- a/doc/sphinx/plugins/restricted/selfAddPersonalAccess.rst +++ b/doc/sphinx/plugins/restricted/selfAddPersonalAccess.rst @@ -14,11 +14,11 @@ Add a personal server access to your account .. program:: selfAddPersonalAccess -.. option:: --host HOST|IP|NET/CIDR +.. option:: --host HOST|IP|SUBNET Host(s) to add access to, either a HOST which will be resolved to an IP immediately, - or an IP, or a whole network using the NET/CIDR notation + or an IP, or a whole subnet using the PREFIX/SIZE notation --user USER|PATTERN|* Specify which remote user should be allowed to connect as. Globbing characters '*' and '?' are supported, so you can specify a pattern that will be matched against the actual remote user name. @@ -65,9 +65,9 @@ Options .. option:: widest_v4_prefix (optional, integer, between 0 and 32) - When specified, this limits the size of prefixes that can be added to an - ACL, e.g. 24 would not allow prefixes wider than /24 (such as /20 or - /16). + When specified, this limits the size of subnets that can be added to an + ACL, e.g. 24 would not allow prefix lengths wider than /24 (such as /20 + or /16). Note that this doesn't prevent users from adding thousands of ACLs to cover a wide range of networks, but this helps ensuring ACLs such as 0.0.0.0/0 can't be added in a single command. diff --git a/doc/sphinx/plugins/restricted/selfDelPersonalAccess.rst b/doc/sphinx/plugins/restricted/selfDelPersonalAccess.rst index 10f858802..2528f9c86 100644 --- a/doc/sphinx/plugins/restricted/selfDelPersonalAccess.rst +++ b/doc/sphinx/plugins/restricted/selfDelPersonalAccess.rst @@ -14,11 +14,11 @@ Remove a personal server access from your account .. program:: selfDelPersonalAccess -.. option:: --host HOST|IP|NET/CIDR +.. option:: --host HOST|IP|SUBNET Host(s) to remove access from, either a HOST which will be resolved to an IP immediately, - or an IP, or a whole network using the NET/CIDR notation + or an IP, or a whole subnet using the PREFIX/SIZE notation --user USER|PATTERN|* Specify which remote user was allowed to connect as. Globbing characters '*' and '?' are supported, so you can specify a pattern that will be matched against the actual remote user name. diff --git a/etc/bastion/bastion.conf.dist b/etc/bastion/bastion.conf.dist index ac74a7618..a99f191ba 100644 --- a/etc/bastion/bastion.conf.dist +++ b/etc/bastion/bastion.conf.dist @@ -126,14 +126,14 @@ # DEFAULT: 2 "dnsSupportLevel": 2, # -# allowedNetworks (array of strings (IPs and/or prefixes)) +# allowedNetworks (array of strings (IPs and/or subnets)) # DESC: Restricts egress connection attempts to those listed networks only. This is enforced at all times and can NOT be overridden by users. If you are lucky enough to have you own IP blocks, it's probably a good idea to list them here. An empty array means no restriction is applied. # DEFAULT: [] # EXAMPLE: ["10.42.0.0/16","192.168.111.0/24","203.0.113.42"] "allowedNetworks": [], # -# forbiddenNetworks (array of strings (IPs and/or prefixes)) -# DESC: Prevents egress connection to the listed networks, this takes precedence over ``allowedNetworks``. This can be used to prevent connection to some hosts or subnets in a broadly allowed prefix. This is enforced at all times and can NOT be overridden by users. +# forbiddenNetworks (array of strings (IPs and/or subnets)) +# DESC: Prevents egress connection to the listed networks, this takes precedence over ``allowedNetworks``. This can be used to prevent connection to some hosts or subnets in a broadly allowed subnet. This is enforced at all times and can NOT be overridden by users. # DEFAULT: [] # EXAMPLE: ["10.42.42.0/24"] "forbiddenNetworks": [], @@ -244,8 +244,8 @@ # > Other ingress policies # >> Policies applying to the ingress connections # -# ingressKeysFrom (array of strings (list of IPs and/or prefixes)) -# DESC: This array of IPs (or prefixes, such as ``10.20.30.0/24``) will be used to build the ``from="..."`` in front of the ingress account public keys used to connect to the bastion (in ``accountCreate`` or ``selfAddIngressKey``). If the array is empty, then **NO** ``from="..."`` is added (this lowers the security). +# ingressKeysFrom (array of strings (list of IPs and/or subnets)) +# DESC: This array of IPs (or subnets, such as ``10.20.30.0/24``) will be used to build the ``from="..."`` in front of the ingress account public keys used to connect to the bastion (in ``accountCreate`` or ``selfAddIngressKey``). If the array is empty, then **NO** ``from="..."`` is added (this lowers the security). # DEFAULT: [] "ingressKeysFrom": [], # @@ -265,7 +265,7 @@ # DEFAULT: "" "defaultLogin": "", # -# egressKeysFrom (array of strings (IPs and/or prefixes)) +# egressKeysFrom (array of strings (IPs and/or subnets)) # DESC: These IPs will be added to the ``from="..."`` of the personal account keys and the group keys. Typically you want to specify only the bastions IP here (including all the slaves). Note that if this option is NOT set at all or set to the empty array, it will default to autodetection at runtime (using ``hostname --all-ip-addresses`` under the hood). This is dependent from your system configuration and is therefore discouraged. # DEFAULT: [] "egressKeysFrom": [], @@ -391,7 +391,7 @@ # DEFAULT: [] "accountCreateSupplementaryGroups": [], # -# accountCreateDefaultPersonalAccesses (array of strings (list of IPs and/or prefixes)) +# accountCreateDefaultPersonalAccesses (array of strings (list of IPs and/or subnets)) # DESC: List of strings of the form USER@IP or USER@IP:PORT or IP or IP:PORT, with IP being IP or prefix (such as 1.2.3.0/24). This is the list of accesses to add to the personal access list of newly created accounts. The special value ACCOUNT is replaced by the name of the account being created. This can be useful to grant some accesses by default to new accounts (for example ACCOUNT@0.0.0.0/0) # DEFAULT: [] "accountCreateDefaultPersonalAccesses": [], diff --git a/lib/perl/OVH/Bastion.pm b/lib/perl/OVH/Bastion.pm index 90efc26c6..e106e6299 100644 --- a/lib/perl/OVH/Bastion.pm +++ b/lib/perl/OVH/Bastion.pm @@ -647,10 +647,10 @@ sub _osh_log { } sub is_valid_ip { - my %params = @_; - my $ip = $params{'ip'}; - my $allowPrefixes = $params{'allowPrefixes'}; # if not, a /24 or /32 notation is rejected - my $fast = $params{'fast'}; # fast mode: avoid instantiating Net::IP... except if ipv6 + 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 if ($fast and $ip !~ m{:}) { # fast asked and it's not an IPv6, regex ftw @@ -667,28 +667,28 @@ sub is_valid_ip { ) ( (?/) - (?\d+) + (?\d+) )? $}x ) { - if (defined $+{'prefix'} and not $allowPrefixes) { - return R('KO_INVALID_IP', msg => "Invalid IP address ($ip), as prefixes are not allowed"); + 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 $+{'prefix'} and $+{'prefix'} > 32) { + if (defined $+{'prefixlen'} and $+{'prefixlen'} > 32) { return R('KO_INVALID_IP', msg => "Invalid IP address ($ip)"); } - if (defined $+{'slash'} and not defined $+{'prefix'}) { + if (defined $+{'slash'} and not defined $+{'prefixlen'}) { # got a / in $ip but it's not followed by \d+ return R('KO_INVALID_IP', msg => "Invalid IP address ($ip)"); } - if (defined $+{'prefix'} && $+{'prefix'} != 32) { - return R('OK', value => {ip => $ip, prefix => $+{'prefix'}}); + if (defined $+{'prefixlen'} && $+{'prefixlen'} != 32) { + return R('OK', value => {ip => $ip, prefixlen => $+{'prefixlen'}}); } return R('OK', value => {ip => $+{'shortip'}}); } @@ -725,7 +725,7 @@ sub is_valid_ip { } } - if (not $allowPrefixes and $type eq 'prefix') { + if (not $allowSubnets and $type eq 'prefix') { return R('KO_INVALID_IP', msg => "Invalid IP address ($ip), as prefixes are not allowed"); } diff --git a/lib/perl/OVH/Bastion/Plugin.pm b/lib/perl/OVH/Bastion/Plugin.pm index 9cedbc025..b5819b4e3 100644 --- a/lib/perl/OVH/Bastion/Plugin.pm +++ b/lib/perl/OVH/Bastion/Plugin.pm @@ -60,7 +60,7 @@ sub validate_tuple { if ($init) { if (defined $ip && $ip ne '') { - $fnret = OVH::Bastion::is_valid_ip(ip => $ip, allowPrefixes => 1); + $fnret = OVH::Bastion::is_valid_ip(ip => $ip, allowSubnets => 1); $fnret or osh_exit $fnret; $ip = $fnret->value->{'ip'}; } @@ -68,7 +68,7 @@ sub validate_tuple { # special case due to osh.pl: when host=1.2.3.0/24 then ip='' # in that case, validate host and set ip to the same if ($host =~ m{/}) { - $fnret = OVH::Bastion::is_valid_ip(ip => $host, allowPrefixes => 1); + $fnret = OVH::Bastion::is_valid_ip(ip => $host, allowSubnets => 1); $fnret or osh_exit $fnret; $ip = $host = $fnret->value->{'ip'}; } diff --git a/lib/perl/OVH/Bastion/allowdeny.inc b/lib/perl/OVH/Bastion/allowdeny.inc index 4e593bfd3..08933d117 100644 --- a/lib/perl/OVH/Bastion/allowdeny.inc +++ b/lib/perl/OVH/Bastion/allowdeny.inc @@ -72,7 +72,7 @@ sub get_group_keys { sub is_access_way_granted { my %params = @_; - my $exactIpMatch = $params{'exactIpMatch'}; # $ip must be explicitly allowed (not given through a wider slash or a 0.0.0.0/0 in grantfile) + my $exactIpMatch = $params{'exactIpMatch'}; # $ip must be explicitly allowed (not given through a wider subnet or a 0.0.0.0/0 in grantfile) my $exactPortMatch = $params{'exactPortMatch'}; # $port must be explicitly allowed (port wildcards in grantfile will be ignored) my $exactUserMatch = $params{'exactUserMatch'}; # $user must be explicitly allowed (user wildcards in grantfile will be ignored) my $exactMatch = $params{'exactMatch'}; # sets exactIpMatch exactPortMatch and exactUserMatch @@ -81,7 +81,7 @@ sub is_access_way_granted { my $ignorePort = $params{'ignorePort'}; # ignore port COMPLETELY (port 22, 2345, or port-wildcard will all match) my $wantedUser = $params{'user'}; # if undef, means we look for a user-any allow - my $wantedIp = $params{'ip'}; # can be a single IP or a prefix + my $wantedIp = $params{'ip'}; # can be a single IP or a subnet my $wantedPort = $params{'port'}; # if undef, means we look for a port-any allow my $way = $params{'way'}; # personal|group|groupguest|legacy @@ -228,14 +228,14 @@ sub is_access_way_granted { last; # perfect match, don't search further } - # check IP in not-exactIpMatch case. if it's a netblock (v4/v6) OR an IPv6, use Netmask matching + # check IP in not-exactIpMatch case. if it's a subnet (v4/v6) OR an IPv6, use Netmask matching if ($isNetblock || $isIPv6) { - # build slash and test + # build subnet and test require Net::Netmask; my $ipCheck = Net::Netmask->new2($entry->{'ip'}); if ($ipCheck && ($isNetblock ? $ipCheck->contains($wantedIp) : $ipCheck->match($wantedIp))) { - osh_debug("... we got a netblock match!"); + osh_debug("... we got a subnet match!"); if (not defined $match{'size'} or $ipCheck->size() < $match{'size'}) { %match = ( forceKey => $entry->{'forceKey'}, @@ -281,11 +281,11 @@ sub is_access_way_granted { # from a given hostname, check if we have an ip or a range of ip or try to resolve sub get_ip { - my %params = @_; - my $host = $params{'host'}; - my $v4 = $params{'v4'} // OVH::Bastion::config('IPv4Allowed')->value; - my $v6 = $params{'v6'} // OVH::Bastion::config('IPv6Allowed')->value; - my $allowPrefixes = $params{'allowPrefixes'} ||= 0; + my %params = @_; + my $host = $params{'host'}; + my $v4 = $params{'v4'} // OVH::Bastion::config('IPv4Allowed')->value; + my $v6 = $params{'v6'} // OVH::Bastion::config('IPv6Allowed')->value; + my $allowSubnets = $params{'allowSubnets'} ||= 0; if (!$host) { return R('ERR_MISSING_PARAMETER', msg => "Missing parameter 'host'"); @@ -297,7 +297,7 @@ sub get_ip { # try to see if it's already an IP osh_debug("checking if '$host' is already an IP (v4=$v4 v6=$v6)"); - my $fnret = OVH::Bastion::is_valid_ip(ip => $host, allowPrefixes => $allowPrefixes); + my $fnret = OVH::Bastion::is_valid_ip(ip => $host, allowSubnets => $allowSubnets); if ($fnret) { osh_debug("Host $host is already an IP"); if ( ($fnret->value->{'version'} == 4 && $v4) @@ -683,7 +683,7 @@ sub print_acls { return R('OK', value => \@jsonRows); } -# checks if ip matches any given array of prefixes/networks +# checks if ip matches any given array of subnets sub _is_in_any_net { my %params = @_; my $ip = $params{'ip'}; @@ -696,12 +696,12 @@ sub _is_in_any_net { return R('ERR_INVALID_PARAMETER', msg => "Parameter 'networks' must be an array"); } - my $isNetblock = (index($ip, '/') != -1); + my $isSubnet = (index($ip, '/') != -1); require Net::Netmask; foreach my $net (@$networks) { my $Netmask = Net::Netmask->new2($net); - if ($Netmask && ($isNetblock ? $Netmask->contains($ip) : $Netmask->match($ip))) { + if ($Netmask && ($isSubnet ? $Netmask->contains($ip) : $Netmask->match($ip))) { return R('OK', value => {matched => $net}); } } @@ -722,7 +722,7 @@ sub is_access_granted { # can also be of the format "realm/remoteself" my $ipfrom = $params{'ipfrom'}; # must be an IP (client IP) - my $ip = $params{'ip'}; # can be a single IP or a slash + my $ip = $params{'ip'}; # can be a single IP or a subnet my $port = $params{'port'}; # if undef, means we look for a port wildcard allow my $user = $params{'user'}; # if undef, means we look for a user wildcard allow @@ -947,7 +947,7 @@ sub ssh_test_access_way { return R('ERR_INCOMPATIBLE_PARAMETERS'); } - $fnret = OVH::Bastion::is_valid_ip(ip => $ip, allowPrefixes => 1); + $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"); @@ -1308,7 +1308,7 @@ sub _get_acl_from_file { [0-9.]+(/[0-9]+)? # IPv4 | # or \[[0-9a-f:.]+(/[0-9]+)?\] # IPv6 - ) # ip or netblock part, mandatory ($3) + ) # ip or subnet part, mandatory ($3) ( :([0-9]+) # $7 == port )? @@ -1338,7 +1338,7 @@ sub _get_acl_from_file { # extract ip (v4 or v6) if (defined $ip) { - $fnret = OVH::Bastion::is_valid_ip(ip => $ip, allowPrefixes => 1, fast => 1); + $fnret = OVH::Bastion::is_valid_ip(ip => $ip, allowSubnets => 1, fast => 1); if (!$fnret) { osh_debug("skipping line <$line> because IP ($ip) is invalid"); next; diff --git a/lib/perl/OVH/Bastion/allowkeeper.inc b/lib/perl/OVH/Bastion/allowkeeper.inc index 755e31403..b3efefb91 100644 --- a/lib/perl/OVH/Bastion/allowkeeper.inc +++ b/lib/perl/OVH/Bastion/allowkeeper.inc @@ -367,7 +367,7 @@ sub access_modify { # check ip if ($action ne 'clear') { - $fnret = OVH::Bastion::is_valid_ip(ip => $ip, allowPrefixes => 1); + $fnret = OVH::Bastion::is_valid_ip(ip => $ip, allowSubnets => 1); return $fnret unless $fnret; $ip = $fnret->value->{'ip'}; @@ -377,7 +377,7 @@ sub access_modify { return R( 'ERR_INVALID_PARAMETER', msg => sprintf( - "Specified prefix (/%d) is too wide, maximum allowed for IPv%d is /%d by this bastion policy", + "Specified prefix length (/%d) is too wide, maximum allowed for IPv%d is /%d by policy", $fnret->value->{'prefixlen'}, $ipVersion, $widestVxPrefix{$ipVersion} ), diff --git a/lib/perl/OVH/Bastion/ssh.inc b/lib/perl/OVH/Bastion/ssh.inc index 3af27e656..1fd2e9151 100644 --- a/lib/perl/OVH/Bastion/ssh.inc +++ b/lib/perl/OVH/Bastion/ssh.inc @@ -477,7 +477,7 @@ sub get_from_for_user_key { @ipList = @$forcedList; } - my @ipListVerified = grep { OVH::Bastion::is_valid_ip(ip => $_, allowPrefixes => 1) } @ipList; + my @ipListVerified = grep { OVH::Bastion::is_valid_ip(ip => $_, allowSubnets => 1) } @ipList; my $from = ''; if (@ipListVerified) { @@ -596,7 +596,7 @@ sub get_bastion_ips { @ips = @$egressKeysFrom; } - my @checkedIps = grep { OVH::Bastion::is_valid_ip(ip => $_, allowPrefixes => 1) } @ips; + my @checkedIps = grep { OVH::Bastion::is_valid_ip(ip => $_, allowSubnets => 1) } @ips; return R('OK', value => \@checkedIps); } diff --git a/tests/functional/tests.d/350-groups.sh b/tests/functional/tests.d/350-groups.sh index 21510f2c8..acaa02bbe 100644 --- a/tests/functional/tests.d/350-groups.sh +++ b/tests/functional/tests.d/350-groups.sh @@ -729,13 +729,13 @@ EOS success a3_add_server_to_g3 $a3 --osh groupAddServer --group $group3 --host 10.20.0.0/17 --port-any --user-any - run a0_add_personal_access_to_a3_works_slash_1 $a0 --osh accountAddPersonalAccess --account $account3 --host 77.66.55.0/24 --user-any --port-any + run a0_add_personal_access_to_a3_works_subnet_1 $a0 --osh accountAddPersonalAccess --account $account3 --host 77.66.55.0/24 --user-any --port-any json .command accountAddPersonalAccess .error_code OK .value.ip 77.66.55.0/24 .value.port null .value.user null - run a0_add_personal_access_to_a3_fail_badslash $a0 --osh accountAddPersonalAccess --account $account3 --host 77.66.55.0/23 --user-any --port-any + run a0_add_personal_access_to_a3_fail_badsubnet $a0 --osh accountAddPersonalAccess --account $account3 --host 77.66.55.0/23 --user-any --port-any json .command null .error_code KO_INVALID_REMOTE_HOST .value null - run a0_add_personal_access_to_a3_works_slash_2 $a0 --osh accountAddPersonalAccess --account $account3 --host 1.2.3.4/32 --user-any --port-any + run a0_add_personal_access_to_a3_works_subnet_2 $a0 --osh accountAddPersonalAccess --account $account3 --host 1.2.3.4/32 --user-any --port-any json .command accountAddPersonalAccess .error_code OK .value.ip 1.2.3.4 .value.port null .value.user null success a0_add_personal_access_to_a3_works $a0 --osh accountAddPersonalAccess --account $account3 --host 77.66.55.4 --user-any --port-any From 82572e887594ffc1b7c322bcbc6f6f9ff53a6f64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Lesimple?= Date: Wed, 18 Dec 2024 15:22:19 +0000 Subject: [PATCH 2/2] enh: 35% faster is_valid_ip() when fast=1 --- lib/perl/OVH/Bastion.pm | 77 ++++++++++---------- lib/perl/OVH/Bastion/allowdeny.inc | 4 +- lib/perl/OVH/Bastion/allowkeeper.inc | 2 +- tests/functional/tests.d/340-selfaccesses.sh | 4 +- tests/unit/tests/is_valid_ip.t | 51 +++++++++++++ 5 files changed, 93 insertions(+), 45 deletions(-) create mode 100644 tests/unit/tests/is_valid_ip.t 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();