diff --git a/migrations/container/20260927120000_auth-command_drop_invalid_whitelist_entries_from_acl_files.php b/migrations/container/20260927120000_auth-command_drop_invalid_whitelist_entries_from_acl_files.php new file mode 100644 index 0000000..358d090 --- /dev/null +++ b/migrations/container/20260927120000_auth-command_drop_invalid_whitelist_entries_from_acl_files.php @@ -0,0 +1,116 @@ +is_first_execution ) { + $this->skip_this_migration = true; + + return; + } + + foreach ( glob( EE_ROOT_DIR . '/services/nginx-proxy/vhost.d/*_acl' ) ?: [] as $file ) { + $content = file_get_contents( $file ); + if ( false === $content ) { + // It may hold an invalid entry, so up() fails and the migration stays pending. + $this->unreadable[] = $file; + continue; + } + $removed = []; + $lines = []; + foreach ( explode( "\n", $content ) as $line ) { + // nginx also accepts `all` and `unix:`, which EE doesn't write but a hand edit may add. + if ( ! preg_match( '/^allow (.*);(\r?)$/', $line, $m ) || is_valid_whitelist_ip( $m[1] ) || in_array( $m[1], [ 'all', 'unix:' ], true ) ) { + $lines[] = $line; + continue; + } + // Entries nginx accepts, e.g. with a prefix leading zero, are kept in a valid form. + $entry = normalize_stored_whitelist_ip( $m[1] ); + if ( is_valid_whitelist_ip( $entry ) ) { + $lines[] = "allow $entry;$m[2]"; + } else { + $removed[] = $m[1]; + } + } + $new_content = implode( "\n", $lines ); + if ( $new_content !== $content ) { + $this->files[ $file ] = [ $new_content, $removed ]; + } + } + + $this->skip_this_migration = empty( $this->files ) && empty( $this->unreadable ); + } + + /** + * Removes the invalid entries from the `_acl` files: older versions wrote them unchecked, and nginx fails every reload and restart on most of them. + * + * The files are edited in place instead of regenerated, so no new file is written while an older nginx-proxy runs. + * + * @throws \Exception When a file can't be read or rewritten, so the upgrade stops before the image migration recreates the proxy on it. + */ + public function up() { + + if ( $this->skip_this_migration ) { + EE::debug( 'Skipping the whitelist entries check: every _acl file is valid.' ); + + return; + } + + $removed = []; + try { + foreach ( $this->files as $file => list( $content, $entries ) ) { + $this->fs->dumpFile( $file, $content ); + if ( $entries ) { + $removed[ implode( "', '", $entries ) ][] = basename( $file ); + } else { + EE::debug( "Normalized the whitelist entries of $file" ); + } + } + if ( $this->unreadable ) { + throw new \Exception( 'Could not read ' . implode( ', ', $this->unreadable ) ); + } + } finally { + // Also after a failed write: a retry no longer sees the files already fixed. + foreach ( $removed as $entries => $files ) { + EE::warning( sprintf( "Removed the invalid whitelist entries '%s' from %s in %s.", $entries, implode( ', ', $files ), EE_ROOT_DIR . '/services/nginx-proxy/vhost.d' ) ); + } + } + + // The rows stay stored; name the command that removes each one. + foreach ( Whitelist::all() as $row ) { + if ( ! is_valid_whitelist_ip( normalize_stored_whitelist_ip( (string) $row->ip ) ) ) { + warn_invalid_stored_whitelist_ip( $row->site_url, $row->ip ); + } + } + + \EE\Site\Utils\reload_global_nginx_proxy(); + } + + /** + * Not reverted: the removed entries made the file invalid. + */ + public function down() { + } +} diff --git a/src/Auth_Command.php b/src/Auth_Command.php index 55588ad..cd06c9b 100644 --- a/src/Auth_Command.php +++ b/src/Auth_Command.php @@ -19,6 +19,7 @@ use Symfony\Component\Filesystem\Filesystem; use function EE\Auth\Utils\generate_site_auth_files; use function EE\Auth\Utils\generate_site_whitelist; +use function EE\Auth\Utils\is_valid_whitelist_ip; use function EE\Auth\Utils\verify_htpasswd_is_present; use function EE\Auth\Utils\write_htpasswd_file; use function EE\Site\Utils\auto_site_name; @@ -97,26 +98,41 @@ public function create( $args, $assoc_args ) { /** * Cleans and Validate IP addresses - * Converts input separated by comma, spaces and new-lines in array + * Converts input separated by comma, whitespace and new-lines in an array of unique entries * - * @param string $ips IPs to clean and validate + * @param string $ips IPs to clean and validate + * @param string $site_url If set, entries already stored for it are accepted, so ones saved by older versions can be deleted. * * @return array $user_ips Cleaned IP addresses. */ - private function clean_and_validate_ips( string $ips ) { + private function clean_and_validate_ips( string $ips, string $site_url = '' ) { - $user_ips = preg_split( '/[\ \n\,]+/', $ips ); + // Unique, as a repeated entry would hit the UNIQUE(site_url, ip) constraint. + $user_ips = array_values( array_unique( preg_split( '/[\s,]+/', trim( $ips ) ) ) ); foreach ( $user_ips as $ip ) { - // Remove subnet from ip if present. - if ( preg_match( '~^(.+?)/([^/]+)$~', $ip, $m ) ) { - $ip = $m[1]; + if ( is_valid_whitelist_ip( $ip ) ) { + continue; } - if ( ! filter_var( $ip, FILTER_VALIDATE_IP ) ) { - EE::error( 'Please check your list do not have any empty or wrong IP addresses.' ); + $stored = '' !== $site_url && '' !== $ip && Whitelist::where( + [ + 'site_url' => $site_url, + 'ip' => $ip, + ] + ); + + if ( $stored ) { + continue; } + + EE::error( + sprintf( + 'Please check your list do not have any empty or wrong IP addresses. Invalid entry: %s. Use an IPv4 or IPv6 address, optionally with a CIDR prefix of 0-32 (IPv4) or 0-128 (IPv6), e.g. 192.0.2.0/24 or 2001:db8::/32.', + '' === $ip ? 'an empty one' : "'$ip'" + ) + ); } return $user_ips; @@ -548,7 +564,7 @@ public function delete( $args, $assoc_args ) { $whitelist->delete(); } } else { - $user_ips = $this->clean_and_validate_ips( $ip ); + $user_ips = $this->clean_and_validate_ips( $ip, $site_url ); foreach ( $user_ips as $ip ) { $existing_ips = Whitelist::where( diff --git a/src/auth-utils.php b/src/auth-utils.php index 4ed908f..8d22d0a 100644 --- a/src/auth-utils.php +++ b/src/auth-utils.php @@ -364,9 +364,86 @@ function htpasswd_command( string $flags, string $name, string $username, string ); } +/** + * Checks that a whitelist entry is an IPv4 or IPv6 address, optionally with a CIDR prefix, that nginx's `allow` accepts. + * + * @param string $entry Whitelist entry. + * + * @return bool + */ +function is_valid_whitelist_ip( string $entry ): bool { + + $parts = explode( '/', $entry ); + + if ( count( $parts ) > 2 ) { + return false; + } + + $is_ipv4 = false !== filter_var( $parts[0], FILTER_VALIDATE_IP, FILTER_FLAG_IPV4 ); + + if ( ! $is_ipv4 && false === filter_var( $parts[0], FILTER_VALIDATE_IP, FILTER_FLAG_IPV6 ) ) { + return false; + } + + // nginx reads 255.255.255.255 as its INADDR_NONE error value, also when embedded in an IPv6 address. + if ( preg_match( '/(^|:)255\.255\.255\.255\z/', $parts[0] ) ) { + return false; + } + + // nginx rejects a `::` that stands for no group, e.g. `1:2:3:4:5:6:7::`. + if ( preg_match( '/^(?:[0-9a-f]{1,4}:){7}:\z/i', $parts[0] ) ) { + return false; + } + + if ( ! isset( $parts[1] ) ) { + return true; + } + + // Digits only, so no sign, space or `;` reaches the nginx config; leading zeros are refused as ambiguous. + return 1 === preg_match( '/^(0|[1-9][0-9]{0,2})\z/', $parts[1] ) && (int) $parts[1] <= ( $is_ipv4 ? 32 : 128 ); +} + +/** + * Normalizes a whitelist entry stored by an older version that nginx accepts as is, e.g. `10.0.0.0/08` or a trailing CR. + * + * @param string $entry Stored whitelist entry. + * + * @return string The entry without surrounding whitespace and prefix leading zeros, or unchanged when it's still invalid. + */ +function normalize_stored_whitelist_ip( string $entry ): string { + + // Only the whitespace nginx itself separates on. + $normalized = preg_replace( '~/0+(?=[0-9])~', '/', trim( $entry, " \t\r\n" ) ); + + return is_valid_whitelist_ip( $normalized ) ? $normalized : $entry; +} + +/** + * Warns once per scope and entry about an invalid stored whitelist entry, with the command that removes it. + * + * @param string $site_url Site URL of the entry, `default` for global. + * @param string $ip Whitelist entry. + */ +function warn_invalid_stored_whitelist_ip( string $site_url, string $ip ) { + + static $warned = []; + + $scope = 'default' === $site_url ? 'global' : $site_url; + if ( ! empty( $warned[ $scope ][ $ip ] ) ) { + return; + } + $warned[ $scope ][ $ip ] = true; + + // `--ip` splits on whitespace and commas, so such an entry can only go with the whole list. + $hint = preg_match( '/[\s,]/', $ip ) ? "`ee auth delete $scope --ip` and add the valid ones again" : sprintf( '`ee auth delete %s --ip=%s`', $scope, escapeshellarg( $ip ) ); + EE::warning( sprintf( "Skipping the invalid whitelist entry '%s' of %s: nginx would reject it. Remove it with %s.", $ip, $scope, $hint ) ); +} + /** * Gets the IPs to whitelist on a site: global and site entries, or none when the site has no own entries. * + * Invalid entries stored by older versions are skipped with a warning, as nginx would reject the whole file. + * * @param string $site_url URL of site, `default` for global. * * @return array @@ -379,10 +456,18 @@ function get_site_whitelist_ips( string $site_url ): array { return []; } - return array_column( - 'default' === $site_url ? $site_ips : array_merge( Whitelist::get_global_ips(), $site_ips ), - 'ip' - ); + $ips = []; + foreach ( 'default' === $site_url ? $site_ips : array_merge( Whitelist::get_global_ips(), $site_ips ) as $row ) { + $ip = normalize_stored_whitelist_ip( (string) $row->ip ); + if ( is_valid_whitelist_ip( $ip ) ) { + $ips[] = $ip; + } else { + // Global entries are merged into every site's file, so this warns once. + warn_invalid_stored_whitelist_ip( $row->site_url, $row->ip ); + } + } + + return $ips; } /**