Fixed a PrivateRange bug for classic firewall when trying to use /32 (#25027)

ajtms · web-flow · commit cfd5934279e0 · 2024-06-14T11:29:16.000+08:00
* Fixed a PrivateRange bug for classic firewall when trying to use /32 in a private range. Modified the firewall/policy validation on PrivateRange to give a recommended masked address if the supplied one is improperly masked.

* Updated the Network change log. Minor formatting fix for whitespace in Utils.

* Fixed tab width formatting.

* Fixed tab width and formatting in PSAzureFirewallPolicy.cs

* Fixed formatting in Utils.cs

* Changed the added utility code so the class and namespace structure is similar to the existing utility code.

* Removed unused function.

* Removed a few more unused methods.

* Cleaned up small redundant string usage.

* Added the static keyword to clarify the intent of the utility class.
diff --git a/src/Network/Network.Test/ScenarioTests/AzureFirewallPolicyTests.ps1 b/src/Network/Network.Test/ScenarioTests/AzureFirewallPolicyTests.ps1
@@ -1517,6 +1517,7 @@ function Test-AzureFirewallPolicyPrivateRangeCRUD {
     $privateRange2 = @("IANAPrivateRanges", "0.0.0.0/0", "66.92.0.0/16")
     $privateRange1 = @("3.3.0.0/24", "98.0.0.0/8","10.227.16.0/20")
     $privateRange2Translated = @("0.0.0.0/0", "66.92.0.0/16", "10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16", "100.64.0.0/10")
+    $privateRange3 = @("255.255.255.255/32")
 
     try {
 
@@ -1541,6 +1542,12 @@ function Test-AzureFirewallPolicyPrivateRangeCRUD {
         Set-AzFirewallPolicy -InputObject $azureFirewallPolicy
         $getAzureFirewallPolicy = Get-AzFirewallPolicy -Name $azureFirewallPolicyName -ResourceGroupName $rgname
         Assert-AreEqualArray $privateRange2Translated $getAzureFirewallPolicy.PrivateRange
+
+        # Test Always SNAT
+        $azureFirewallPolicy.PrivateRange = $privateRange3
+        Set-AzFirewallPolicy -InputObject $azureFirewallPolicy
+        $getAzureFirewallPolicy = Get-AzFirewallPolicy -Name $azureFirewallPolicyName -ResourceGroupName $rgname
+        Assert-AreEqualArray $privateRange3 $getAzureFirewallPolicy.PrivateRange
     }
     finally {
         # Cleanup
diff --git a/src/Network/Network.Test/ScenarioTests/AzureFirewallTests.ps1 b/src/Network/Network.Test/ScenarioTests/AzureFirewallTests.ps1
@@ -1351,6 +1351,7 @@ function Test-AzureFirewallPrivateRangeCRUD {
 
     $privateRange1 = @("IANAPrivateRanges", "0.0.0.0/0", "66.92.0.0/16")
     $privateRange2 = @("3.3.0.0/24", "98.0.0.0/8","10.227.16.0/20","10.226.0.0/16")
+    $privateRange3 = @("255.255.255.255/32")
     
     try {
         # Create the resource group
@@ -1375,6 +1376,12 @@ function Test-AzureFirewallPrivateRangeCRUD {
         Set-AzFirewall -AzureFirewall $azureFirewall
         $getAzureFirewall = Get-AzFirewall -Name $azureFirewallName -ResourceGroupName $rgname
         Assert-AreEqualArray $privateRange2 $getAzureFirewall.PrivateRange
+
+        # Test Always SNAT
+        $azureFirewall.PrivateRange = $privateRange3
+        Set-AzFirewall -AzureFirewall $azureFirewall
+        $getAzureFirewall = Get-AzFirewall -Name $azureFirewallName -ResourceGroupName $rgname
+        Assert-AreEqualArray $privateRange3 $getAzureFirewall.PrivateRange
     }
     finally {
         # Cleanup
diff --git a/src/Network/Network.Test/SessionRecords/Commands.Network.Test.ScenarioTests.AzureFirewallPolicyTests/TestAzureFirewallPolicyPrivateRangeCRUD.json b/src/Network/Network.Test/SessionRecords/Commands.Network.Test.ScenarioTests.AzureFirewallPolicyTests/TestAzureFirewallPolicyPrivateRangeCRUD.json
diff --git a/src/Network/Network.Test/SessionRecords/Commands.Network.Test.ScenarioTests.AzureFirewallTests/TestAzureFirewallPrivateRangeCRUD.json b/src/Network/Network.Test/SessionRecords/Commands.Network.Test.ScenarioTests.AzureFirewallTests/TestAzureFirewallPrivateRangeCRUD.json
diff --git a/src/Network/Network/ChangeLog.md b/src/Network/Network/ChangeLog.md
@@ -19,6 +19,9 @@
 --->
 
 ## Upcoming Release
+* Updated the Azure Firewall and Azure Firewall Policy setter for their respective Private Range properties
+  - Fixed a bug that prevented using /32 in private ranges on classic Azure Firewalls
+  - Updated the error message to provide a suggested private range when the supplied range is not correctly masked by the host identifier
 
 ## Version 7.6.0
 * Added cmdlet `New-AzVirtualApplianceNetworkProfile` to build network profile for network virtual appliance and pass as a parameter.
diff --git a/src/Network/Network/Common/Utils.cs b/src/Network/Network/Common/Utils.cs
@@ -15,6 +15,8 @@
 using Microsoft.Azure.Commands.Network.Models;
 using System;
 using System.Globalization;
+using System.Management.Automation;
+using System.Net;
 
 namespace Microsoft.Azure.Commands.Network
 {
@@ -72,9 +74,9 @@ public static bool IsRegionRestrictedForBasicFirewall(string region)
         {
             if (!string.IsNullOrEmpty(region))
             {
-                foreach(var reg in RestrictedBasicSkuFirewallRegions)
+                foreach (var reg in RestrictedBasicSkuFirewallRegions)
                 {
-                    if(String.Compare(reg, region, CultureInfo.CurrentCulture, CompareOptions.IgnoreCase | CompareOptions.IgnoreSymbols) == 0)
+                    if (String.Compare(reg, region, CultureInfo.CurrentCulture, CompareOptions.IgnoreCase | CompareOptions.IgnoreSymbols) == 0)
                     {
                         return true;
                     }
@@ -83,4 +85,134 @@ public static bool IsRegionRestrictedForBasicFirewall(string region)
             return false;
         }
     }
+
+    public static class NetworkValidationUtils
+    {
+        public static void ValidateIpAddress(string ipAddress)
+        {
+            IPAddress ipVal;
+
+            if (!IPAddress.TryParse(ipAddress, out ipVal) || ipVal.AddressFamily != System.Net.Sockets.AddressFamily.InterNetwork)
+            {
+                throw new PSArgumentException(String.Format("\'{0}\' is not a valid IPv4 address", ipAddress));
+            }
+        }
+
+        public static void ValidateSubnet(string ipAddress)
+        {
+            ValidateIPv4CidrNotation(ipAddress);
+        }
+
+        private static void ValidateIPv4CidrNotation(string ipv4Cidr)
+        {
+            var subnet = ipv4Cidr.Split('/');
+            if (subnet.Length != 2)
+                throw new PSArgumentException(String.Format("\'{0}\' is not a valid IPv4 subnet", ipv4Cidr)); // Recommend proper format? e.g. 192.168.1.0/24
+
+            uint address;
+            if (!TryParseIPv4Address(subnet[0], out address))
+            {
+                throw new PSArgumentException(String.Format("The network prefix for \'{0}\' is not a valid IPv4 address", ipv4Cidr));
+            }
+
+            int host;
+            if (!TryParseIPv4HostIdentifier(subnet[1], out host))
+            {
+                throw new PSArgumentException(String.Format("\'{0}\' is not a valid IPv4 subnet. The network mask should be an integer from 0 to 32", ipv4Cidr));
+            }
+
+
+            if (!isIpAddressCorrectlyMaskByNetworkPrefixLength(address, host))
+            {
+                uint recommendedAddressBits;
+                string explanation = $"The network prefix for the CIDR string '{ipv4Cidr}' should be masked according to the suffix '{host}'.";
+                if (TryApplyMask(address, host, out recommendedAddressBits))
+                {
+                    string recommendation = $"Try using '{uintToIPv4AddressString(recommendedAddressBits)}' instead.";
+                    throw new PSArgumentException($"{explanation} {recommendation}");
+                }
+                else
+                {
+                    throw new PSArgumentException($"{explanation}");
+                }
+            }
+        }
+
+        private static string uintToIPv4AddressString(uint address)
+        {
+            var bytes = new byte[4];
+            for (int i = 0; i < 4; i++)
+            {
+                bytes[i] = (byte)(address >> (8 * (3 - i)));
+            }
+
+            return String.Join(".", bytes);
+        }
+
+        private static bool isValidHostIdentifier(int host)
+        {
+            return host >= 0 && host <= 32;
+        }
+
+        /// <summary>
+        /// Checks to see if an IP address (192.168.10.0) is correctly masked by the host identifier in IPv4 CIDR notation.
+        /// ex. 192.168.1.0/24 is correctly masked, but 192.168.1.0/23 is not correctly masked
+        /// </summary>
+        /// <param name="ipAddress"></param>
+        /// <param name="networkPrefixLength"></param>
+        /// <returns></returns>
+        private static bool isIpAddressCorrectlyMaskByNetworkPrefixLength(uint ipAddress, int networkPrefixLength)
+        {
+            const uint fullMask = UInt32.MaxValue;
+            if (!isValidHostIdentifier(networkPrefixLength)) return false;
+            if (networkPrefixLength == 32 && ipAddress != fullMask) return false;
+            if (networkPrefixLength == 32 && ipAddress == fullMask) return true;
+
+            return (ipAddress << networkPrefixLength) == 0;
+        }
+
+        /// <summary>
+        /// Tries to mask the non-network bits with 0, according to the prefixLength (if the length is valid).
+        /// ex. 192.168.111.234/16 -> 192.168.0.0
+        /// </summary>
+        /// <param name="address"></param>
+        /// <param name="prefixLength"></param>
+        /// <param name="maskedAddress"></param>
+        /// <returns></returns>
+        private static bool TryApplyMask(uint address, int prefixLength, out uint maskedAddress)
+        {
+            maskedAddress = 0;
+            if (!isValidHostIdentifier(prefixLength)) return false;
+            int shift = (32 - prefixLength);
+            maskedAddress = (address >> shift) << shift;
+
+            return true;
+        }
+
+        /// <summary>
+        /// Try to parse an IPv4 address string into a UInt32
+        /// </summary>
+        /// <param name="address"></param>
+        /// <param name="parsed"></param>
+        /// <returns></returns>
+        private static bool TryParseIPv4Address(string address, out uint parsed)
+        {
+            parsed = 0;
+            if (!IPAddress.TryParse(address, out var addr)) return false;
+            byte[] bytes = addr.GetAddressBytes();
+
+            for (int i = 0; i < 4; i++)
+            {
+                int amountToShift = (8 * (3 - i)); // 24, 16, 8, 0
+                parsed += ((uint)bytes[i] << amountToShift);
+            }
+
+            return true;
+        }
+
+        private static bool TryParseIPv4HostIdentifier(string host, out int parsed)
+        {
+            return Int32.TryParse(host, out parsed) && isValidHostIdentifier(parsed);
+        }
+    }
 }
diff --git a/src/Network/Network/Models/AzureFirewall/PSAzureFirewall.cs b/src/Network/Network/Models/AzureFirewall/PSAzureFirewall.cs
@@ -358,7 +358,8 @@ public void RemovePublicIpAddress(PSPublicIpAddress publicIpAddress)
                 throw new ArgumentException($"Public IP Address {publicIpAddress.Id} is not attached to firewall {this.Name}");
             }
 
-            if (this.ManagementIpConfiguration != null) {
+            if (this.ManagementIpConfiguration != null)
+            {
                 if (ipConfigToRemove.Subnet != null)
                 {
                     ipConfigToRemove.PublicIpAddress = null;
@@ -508,43 +509,12 @@ private void ValidatePrivateRange(string[] privateRange)
                     continue;
 
                 if (ip.Contains("/"))
-                    ValidateMaskedIpAddress(ip);
+                    NetworkValidationUtils.ValidateSubnet(ip);
                 else
-                    ValidateSingleIpAddress(ip);
-            }
-        }
-
-        private void ValidateSingleIpAddress(string ipAddress)
-        {
-            IPAddress ipVal;
-            if (!IPAddress.TryParse(ipAddress, out ipVal) || ipVal.AddressFamily != System.Net.Sockets.AddressFamily.InterNetwork)
-            {
-                throw new PSArgumentException(String.Format("\'{0}\' is not a valid private range ip address", ipAddress));
+                    NetworkValidationUtils.ValidateIpAddress(ip);
             }
         }
 
-        private void ValidateMaskedIpAddress(string ipAddress)
-        {
-            var split = ipAddress.Split('/');
-            if (split.Length != 2)
-                throw new PSArgumentException(String.Format("\'{0}\' is not a valid private range ip address", ipAddress));
-
-            // validate the ip
-            ValidateSingleIpAddress(split[0]);
-
-            // validate mask
-            var bit = 0;
-            if (!Int32.TryParse(split[1], out bit) || bit < 0 || bit > 32)
-                throw new PSArgumentException(String.Format("\'{0}\' is not a valid private range ip address, subnet mask should between 0 and 32", ipAddress));
-
-            // validated that unmasked bits are 0
-            var splittedIp = split[0].Split('.');
-            var ip = Int32.Parse(splittedIp[0]) << 24;            
-            ip += (Int32.Parse(splittedIp[1]) << 16) + (Int32.Parse(splittedIp[2]) << 8) + Int32.Parse(splittedIp[3]);
-            if (ip << bit != 0)
-                throw new PSArgumentException(String.Format("\'{0}\' is not a valid private range ip address, bits not covered by subnet mask should be all 0", ipAddress));
-        }
-
         #endregion
 
         #region DNS Proxy Validation
diff --git a/src/Network/Network/Models/AzureFirewallPolicy/PSAzureFirewallPolicy.cs b/src/Network/Network/Models/AzureFirewallPolicy/PSAzureFirewallPolicy.cs
@@ -91,43 +91,11 @@ private void ValidatePrivateRange(string[] privateRange)
                     continue;
 
                 if (ip.Contains("/"))
-                    ValidateMaskedIpAddress(ip);
+                    NetworkValidationUtils.ValidateSubnet(ip);
                 else
-                    ValidateSingleIpAddress(ip);
+                    NetworkValidationUtils.ValidateIpAddress(ip);
             }
         }
-
-        private void ValidateSingleIpAddress(string ipAddress)
-        {
-            IPAddress ipVal;
-            if (!IPAddress.TryParse(ipAddress, out ipVal) || ipVal.AddressFamily != System.Net.Sockets.AddressFamily.InterNetwork)
-            {
-                throw new AzPSArgumentException(String.Format(Resources.InvalidPrivateIPRange, ipAddress), nameof(ipAddress), ErrorKind.UserError);
-            }
-        }
-
-        private void ValidateMaskedIpAddress(string ipAddress)
-        {
-            var split = ipAddress.Split('/');
-            if (split.Length != 2)
-                throw new AzPSArgumentException(String.Format(Resources.InvalidPrivateIPRange, ipAddress), nameof(ipAddress), ErrorKind.UserError);
-
-            // validate the ip
-            ValidateSingleIpAddress(split[0]);
-
-            // validate mask
-            var bit = 0;
-            if (!Int32.TryParse(split[1], out bit) || bit < 0 || bit > 32)
-                throw new AzPSArgumentException(String.Format(Resources.InvalidPrivateIPRangeMask, ipAddress), nameof(ipAddress), ErrorKind.UserError);
-
-            // validated that unmasked bits are 0
-            var splittedIp = split[0].Split('.');
-            var ip = Int32.Parse(splittedIp[0]) << 24;
-            ip += (Int32.Parse(splittedIp[1]) << 16) + (Int32.Parse(splittedIp[2]) << 8) + Int32.Parse(splittedIp[3]);
-            if ((ip << bit != 0) && (ip << bit != -1))
-                throw new AzPSArgumentException(String.Format(Resources.InvalidPrivateIPRangeUnmaskedBits, ipAddress), nameof(ipAddress), ErrorKind.UserError);
-        }
-
         #endregion
     }
 }
