VPC ACL: keep the ports and ICMP type of rules given by protocol number or as upper-case ICMP

api/src/main/java/org/apache/cloudstack/api/command/user/network/CreateNetworkACLCmd.java

@@ -31,7 +31,6 @@
 import org.apache.cloudstack.api.response.NetworkACLResponse;
 import org.apache.cloudstack.api.response.NetworkResponse;
 import org.apache.cloudstack.context.CallContext;
-import org.apache.commons.lang3.StringUtils;
 
 import com.cloud.event.EventTypes;
 import com.cloud.exception.InvalidParameterValueException;
@@ -102,15 +101,7 @@ public boolean isDisplay() {
     }
 
     public String getProtocol() {
-        String p = protocol.trim();
-        // Deal with ICMP(protocol number 1) specially because it need to be paired with icmp type and code
-        if (StringUtils.isNumeric(p)) {
-            int protoNumber = Integer.parseInt(p);
-            if (protoNumber == 1) {
-                p = "icmp";
-            }
-        }
-        return p;
+        return NetUtils.icmpProtocolNumberAsName(protocol);
     }
 
     public void setProtocol(String protocol) {

api/src/main/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkACLItemCmd.java

@@ -31,6 +31,7 @@
 import com.cloud.exception.ResourceUnavailableException;
 import com.cloud.network.vpc.NetworkACLItem;
 import com.cloud.user.Account;
+import com.cloud.utils.net.NetUtils;
 
 @APICommand(name = "updateNetworkACLItem", description = "Updates ACL item with specified ID", responseObject = NetworkACLItemResponse.class, requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
 public class UpdateNetworkACLItemCmd extends BaseAsyncCustomIdCmd {
@@ -99,11 +100,7 @@ public Long getId() {
     }
 
     public String getProtocol() {
-        if (protocol != null) {
-            return protocol.trim();
-        } else {
-            return null;
-        }
+        return NetUtils.icmpProtocolNumberAsName(protocol);
     }
 
     public List<String> getSourceCidrList() {

api/src/test/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkACLItemCmdTest.java

@@ -0,0 +1,44 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.api.command.user.network;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.springframework.test.util.ReflectionTestUtils;
+
+public class UpdateNetworkACLItemCmdTest {
+
+    private String protocolOf(String protocol) {
+        UpdateNetworkACLItemCmd cmd = new UpdateNetworkACLItemCmd();
+        ReflectionTestUtils.setField(cmd, "protocol", protocol);
+        return cmd.getProtocol();
+    }
+
+    @Test
+    public void getProtocolTreatsProtocolNumberOneAsIcmp() {
+        // as createNetworkACL does, so that the rule keeps its icmp type and code
+        Assert.assertEquals("icmp", protocolOf("1"));
+        Assert.assertEquals("icmp", protocolOf(" 1 "));
+    }
+
+    @Test
+    public void getProtocolLeavesOtherProtocolsAlone() {
+        Assert.assertEquals("tcp", protocolOf(" tcp "));
+        Assert.assertEquals("47", protocolOf("47"));
+        Assert.assertNull(protocolOf(null));
+    }
+}

core/src/main/java/com/cloud/agent/api/routing/SetNetworkACLCommand.java

@@ -23,6 +23,9 @@
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.List;
+import java.util.Locale;
+
+import org.apache.commons.lang3.StringUtils;
 
 import com.cloud.agent.api.to.NetworkACLTO;
 import com.cloud.agent.api.to.NicTO;
@@ -70,9 +73,10 @@ public String[][] generateFwRules() {
 
             List<String> cidr;
             final StringBuilder sb = new StringBuilder();
-            sb.append(aclTO.getTrafficType().toString()).append(RULE_DETAIL_SEPARATOR).append(aclTO.getProtocol()).append(RULE_DETAIL_SEPARATOR);
-            if ("icmp".compareTo(aclTO.getProtocol()) == 0) {
-                sb.append(aclTO.getIcmpType()).append(RULE_DETAIL_SEPARATOR).append(aclTO.getIcmpCode()).append(RULE_DETAIL_SEPARATOR);
+            final String protocol = normalizeProtocol(aclTO.getProtocol(), aclTO.getSrcPortRange() != null);
+            sb.append(aclTO.getTrafficType().toString()).append(RULE_DETAIL_SEPARATOR).append(protocol).append(RULE_DETAIL_SEPARATOR);
+            if (NetUtils.ICMP_PROTO.equals(protocol)) {
+                sb.append(icmpTypeOrCode(aclTO.getIcmpType())).append(RULE_DETAIL_SEPARATOR).append(icmpTypeOrCode(aclTO.getIcmpCode())).append(RULE_DETAIL_SEPARATOR);
             } else {
                 sb.append(aclTO.getStringPortRange().replace(":", RULE_DETAIL_SEPARATOR)).append(RULE_DETAIL_SEPARATOR);
             }
@@ -97,6 +101,45 @@ public String[][] generateFwRules() {
         return result;
     }
 
+    /**
+     * The VR applies ports only to a rule it is sent as tcp or udp, and an ICMP type and code only to one
+     * sent as icmp; any other protocol goes to it as a bare number, which matches the whole protocol. So
+     * send protocol number 1 as icmp, and 6 and 17 as tcp and udp when the rule has ports, whatever form
+     * or case they were stored in. Without ports 6 and 17 stay bare numbers, which match the whole
+     * protocol on every VR path; a tcp or udp rule without ports has matched port 0 only on the iptables
+     * path (see #14363).
+     */
+    protected static String normalizeProtocol(String protocol, boolean hasPorts) {
+        if (protocol == null) {
+            return null;
+        }
+        String p = protocol.trim().toLowerCase(Locale.ROOT);
+        if (!StringUtils.isNumeric(p)) {
+            return p;
+        }
+        int number;
+        try {
+            number = Integer.parseInt(p);
+        } catch (NumberFormatException e) {
+            return p;
+        }
+        if (number == NetUtils.ICMP_PROTO_NUMBER) {
+            return NetUtils.ICMP_PROTO;
+        }
+        if (hasPorts && number == NetUtils.TCP_PROTO_NUMBER) {
+            return NetUtils.TCP_PROTO;
+        }
+        if (hasPorts && number == NetUtils.UDP_PROTO_NUMBER) {
+            return NetUtils.UDP_PROTO;
+        }
+        return String.valueOf(number);
+    }
+
+    private static int icmpTypeOrCode(Integer value) {
+        // -1 is "any"; a rule stored as protocol 1 may carry no ICMP type or code at all
+        return value == null ? -1 : value;
+    }
+
     protected void orderNetworkAclRulesByRuleNumber(List<NetworkACLTO> aclList) {
         Collections.sort(aclList, new Comparator<NetworkACLTO>() {
             @Override

core/src/test/java/com/cloud/agent/api/routing/SetNetworkACLCommandTest.java

@@ -26,6 +26,7 @@
 import org.junit.Test;
 
 import com.cloud.agent.api.to.NetworkACLTO;
+import com.cloud.network.vpc.NetworkACLItem.TrafficType;
 import com.google.common.collect.Lists;
 
 public class SetNetworkACLCommandTest {
@@ -50,4 +51,48 @@ public void testNetworkAclRuleOrdering(){
             assertEquals(aclList.get(i).getNumber(), i+1);
         }
     }
+
+    private String generateRule(String protocol, Integer portStart, Integer portEnd, Integer icmpType, Integer icmpCode) {
+        NetworkACLTO rule = new NetworkACLTO(1, null, protocol, portStart, portEnd, false, false, Lists.newArrayList("10.0.0.0/24"),
+                icmpType, icmpCode, TrafficType.Ingress, true, 1);
+        return new SetNetworkACLCommand(Lists.newArrayList(rule), null).generateFwRules()[0][0];
+    }
+
+    @Test
+    public void testTcpAndUdpByProtocolNumberKeepTheirPorts() {
+        assertEquals("Ingress;tcp;22;23;10.0.0.0/24;ACCEPT;", generateRule("6", 22, 23, null, null));
+        assertEquals("Ingress;udp;53;53;10.0.0.0/24;ACCEPT;", generateRule("17", 53, 53, null, null));
+    }
+
+    @Test
+    public void testTcpAndUdpByProtocolNumberWithoutPortsStayWholeProtocol() {
+        // a bare number matches the whole protocol on every VR path
+        assertEquals("Ingress;6;0;0;10.0.0.0/24;ACCEPT;", generateRule("6", null, null, null, null));
+        assertEquals("Ingress;17;0;0;10.0.0.0/24;ACCEPT;", generateRule("17", null, null, null, null));
+    }
+
+    @Test
+    public void testProtocolNumberInAnotherFormIsReadAsANumber() {
+        assertEquals("Ingress;tcp;22;22;10.0.0.0/24;ACCEPT;", generateRule("006", 22, 22, null, null));
+        assertEquals("Ingress;icmp;8;0;10.0.0.0/24;ACCEPT;", generateRule(" 01 ", null, null, 8, 0));
+        assertEquals("Ingress;47;0;0;10.0.0.0/24;ACCEPT;", generateRule("047", null, null, null, null));
+    }
+
+    @Test
+    public void testIcmpByProtocolNumberKeepsItsTypeAndCode() {
+        assertEquals("Ingress;icmp;8;0;10.0.0.0/24;ACCEPT;", generateRule("1", null, null, 8, 0));
+        // a rule updated to protocol 1 may have no ICMP type or code: any
+        assertEquals("Ingress;icmp;-1;-1;10.0.0.0/24;ACCEPT;", generateRule("1", null, null, null, null));
+    }
+
+    @Test
+    public void testIcmpIsMatchedWhateverItsCase() {
+        assertEquals("Ingress;icmp;8;0;10.0.0.0/24;ACCEPT;", generateRule("ICMP", null, null, 8, 0));
+    }
+
+    @Test
+    public void testOtherProtocolNumberIsPassedThrough() {
+        assertEquals("Ingress;47;0;0;10.0.0.0/24;ACCEPT;", generateRule("47", null, null, null, null));
+        assertEquals("Ingress;all;0;0;10.0.0.0/24;ACCEPT;", generateRule("all", null, null, null, null));
+    }
 }

server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java

@@ -42,6 +42,7 @@
 import org.apache.commons.lang3.BooleanUtils;
 import org.apache.commons.lang3.ObjectUtils;
 import org.apache.commons.lang3.StringUtils;
+import org.apache.commons.lang3.math.NumberUtils;
 import org.springframework.stereotype.Component;
 
 import com.cloud.dc.DataCenter;
@@ -365,6 +366,7 @@ public NetworkACLItem createNetworkACLItem(CreateNetworkACLCmd createNetworkACLC
         networkACLItemVO.setDisplay(createNetworkACLCmd.isDisplay());
 
         validateNetworkACLItem(networkACLItemVO);
+        validatePortsAreUsableWithProtocol(protocol, sourcePortStart, sourcePortEnd);
         return _networkAclMgr.createNetworkACLItem(networkACLItemVO);
     }
 
@@ -690,6 +692,33 @@ protected void validateSourceStartAndEndPorts(NetworkACLItemVO networkACLItemVO)
         }
     }
 
+    /**
+     * Ports are only applied to TCP and UDP, so they cannot be given with any other protocol number:
+     * the rule would be applied to the whole protocol, ignoring them.
+     */
+    protected void validatePortsAreUsableWithProtocol(String protocol, Integer sourcePortStart, Integer sourcePortEnd) {
+        if ((sourcePortStart == null && sourcePortEnd == null) || !StringUtils.isNumeric(protocol)) {
+            return;
+        }
+        int protoNumber = NumberUtils.toInt(protocol, -1);
+        if (protoNumber != NetUtils.TCP_PROTO_NUMBER && protoNumber != NetUtils.UDP_PROTO_NUMBER) {
+            throw new InvalidParameterValueException(String.format("Start and end port can only be given for TCP or UDP (protocol number %d or %d), not for protocol number [%s]",
+                    NetUtils.TCP_PROTO_NUMBER, NetUtils.UDP_PROTO_NUMBER, protocol));
+        }
+    }
+
+    /**
+     * On update the ports a rule already carries are only checked when the protocol is given too, so
+     * that a rule stored with ports on another protocol number can still be edited otherwise, while a
+     * protocol change cannot leave them on it.
+     */
+    protected void validatePortsAreUsableWithProtocolOnUpdate(UpdateNetworkACLItemCmd updateNetworkACLItemCmd, NetworkACLItemVO networkACLItemVo) {
+        boolean protocolGiven = StringUtils.isNotBlank(updateNetworkACLItemCmd.getProtocol());
+        validatePortsAreUsableWithProtocol(networkACLItemVo.getProtocol(),
+                protocolGiven ? networkACLItemVo.getSourcePortStart() : updateNetworkACLItemCmd.getSourcePortStart(),
+                protocolGiven ? networkACLItemVo.getSourcePortEnd() : updateNetworkACLItemCmd.getSourcePortEnd());
+    }
+
     @Override
     public NetworkACLItem getNetworkACLItem(final long ruleId) {
         return _networkAclMgr.getNetworkACLItem(ruleId);
@@ -876,6 +905,7 @@ public NetworkACLItem updateNetworkACLItem(UpdateNetworkACLItemCmd updateNetwork
 
         transferDataToNetworkAclRulePojo(updateNetworkACLItemCmd, networkACLItemVo, acl);
         validateNetworkACLItem(networkACLItemVo);
+        validatePortsAreUsableWithProtocolOnUpdate(updateNetworkACLItemCmd, networkACLItemVo);
         return _networkAclMgr.updateNetworkACLItem(networkACLItemVo);
     }
 

server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java

@@ -542,6 +542,48 @@ public void validateSourceStartAndEndPortsTestPortsWithTcpProtocol() {
         networkAclServiceImpl.validateSourceStartAndEndPorts(networkAclItemVoMock);
     }
 
+    @Test
+    public void validatePortsAreUsableWithProtocolTestTcpAndUdpByNameOrNumber() {
+        for (String protocol : new String[] {"tcp", "udp", "6", "17"}) {
+            networkAclServiceImpl.validatePortsAreUsableWithProtocol(protocol, 22, 23);
+        }
+    }
+
+    @Test
+    public void validatePortsAreUsableWithProtocolTestOtherProtocolNumberWithoutPorts() {
+        networkAclServiceImpl.validatePortsAreUsableWithProtocol("47", null, null);
+    }
+
+    @Test(expected = InvalidParameterValueException.class)
+    public void validatePortsAreUsableWithProtocolTestOtherProtocolNumberWithPorts() {
+        networkAclServiceImpl.validatePortsAreUsableWithProtocol("132", 22, 22);
+    }
+
+    @Test(expected = InvalidParameterValueException.class)
+    public void validatePortsAreUsableWithProtocolTestOtherProtocolNumberWithStartPortOnly() {
+        networkAclServiceImpl.validatePortsAreUsableWithProtocol("47", 22, null);
+    }
+
+    @Test
+    public void validatePortsAreUsableWithProtocolOnUpdateTestStoredPortsKeptWhenProtocolNotGiven() {
+        Mockito.when(updateNetworkACLItemCmdMock.getProtocol()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.getSourcePortStart()).thenReturn(null);
+        Mockito.when(updateNetworkACLItemCmdMock.getSourcePortEnd()).thenReturn(null);
+        Mockito.when(networkAclItemVoMock.getProtocol()).thenReturn("47");
+
+        networkAclServiceImpl.validatePortsAreUsableWithProtocolOnUpdate(updateNetworkACLItemCmdMock, networkAclItemVoMock);
+    }
+
+    @Test(expected = InvalidParameterValueException.class)
+    public void validatePortsAreUsableWithProtocolOnUpdateTestProtocolChangedOntoStoredPorts() {
+        Mockito.when(updateNetworkACLItemCmdMock.getProtocol()).thenReturn("47");
+        Mockito.when(networkAclItemVoMock.getProtocol()).thenReturn("47");
+        Mockito.when(networkAclItemVoMock.getSourcePortStart()).thenReturn(22);
+        Mockito.when(networkAclItemVoMock.getSourcePortEnd()).thenReturn(22);
+
+        networkAclServiceImpl.validatePortsAreUsableWithProtocolOnUpdate(updateNetworkACLItemCmdMock, networkAclItemVoMock);
+    }
+
     @Test
     public void validateSourceCidrListTestEmptySourceCirdList() {
         Mockito.when(networkAclItemVoMock.getSourceCidrList()).thenReturn(new ArrayList<>());

ui/src/views/network/AclRulesTab.vue

@@ -276,7 +276,7 @@
           </a-form-item>
         </div>
 
-        <div v-show="['tcp', 'udp', 'protocolnumber'].includes(form.protocol) && !(form.protocol === 'protocolnumber' && form.protocolnumber === 1)">
+        <div v-show="hasPorts(form.protocol, form.protocolnumber)">
           <a-form-item :label="$t('label.startport')" ref="startport" name="startport">
             <a-input-number style="width: 100%" v-model:value="form.startport" />
           </a-form-item>
@@ -607,6 +607,10 @@ export default {
         self.form.reason = acl.reason
       }, 200)
     },
+    hasPorts (protocol, protocolNumber) {
+      // ports only apply to TCP and UDP, also when given by protocol number
+      return ['tcp', 'udp'].includes(protocol) || (protocol === 'protocolnumber' && [6, 17].includes(protocolNumber))
+    },
     getDataFromForm (values) {
       const data = {
         cidrlist: values.cidrlist || '',
@@ -617,7 +621,7 @@ export default {
         reason: values.reason || ''
       }
 
-      if (values.protocol === 'tcp' || values.protocol === 'udp' || values.protocol === 'protocolnumber') {
+      if (this.hasPorts(values.protocol, values.protocolnumber)) {
         data.startport = values.startport || ''
         data.endport = values.endport || ''
       }

utils/src/main/java/com/cloud/utils/net/NetUtils.java

@@ -48,6 +48,7 @@
 import org.apache.commons.collections.CollectionUtils;
 import org.apache.commons.lang.SystemUtils;
 import org.apache.commons.lang3.StringUtils;
+import org.apache.commons.lang3.math.NumberUtils;
 import org.apache.commons.net.util.SubnetUtils;
 import org.apache.commons.validator.routines.InetAddressValidator;
 import org.apache.commons.validator.routines.RegexValidator;
@@ -82,6 +83,8 @@ public class NetUtils {
     public final static String ICMP_PROTO = "icmp";
     public static final String ICMP6_PROTO = "icmp6";
     public final static int ICMP_PROTO_NUMBER = 1;
+    public final static int TCP_PROTO_NUMBER = 6;
+    public final static int UDP_PROTO_NUMBER = 17;
     public final static String ALL_PROTO = "all";
     public final static String HTTP_PROTO = "http";
     public final static String HTTPS_PROTO = "https";
@@ -1013,6 +1016,21 @@ public static boolean isValidSecurityGroupProto(final String p) {
         return proto.equals(TCP_PROTO) || proto.equals(UDP_PROTO) || proto.equals(ICMP_PROTO) || proto.equals(ALL_PROTO);
     }
 
+    /**
+     * Gives a rule's protocol trimmed, with protocol number 1 as icmp, so that the rule keeps its ICMP
+     * type and code.
+     */
+    public static String icmpProtocolNumberAsName(final String protocol) {
+        if (protocol == null) {
+            return null;
+        }
+        final String p = protocol.trim();
+        if (StringUtils.isNumeric(p) && NumberUtils.toInt(p, -1) == ICMP_PROTO_NUMBER) {
+            return ICMP_PROTO;
+        }
+        return p;
+    }
+
     public static boolean isValidAlgorithm(final String p) {
         final String algo = p.toLowerCase();
         return algo.equals("roundrobin") || algo.equals("leastconn") || algo.equals("source");