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/UpdateNetworkACLItemCmd.java

@@ -26,11 +26,13 @@
 import org.apache.cloudstack.api.Parameter;
 import org.apache.cloudstack.api.response.NetworkACLItemResponse;
 import org.apache.cloudstack.context.CallContext;
+import org.apache.commons.lang3.StringUtils;
 
 import com.cloud.event.EventTypes;
 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 +101,15 @@ public Long getId() {
     }
 
     public String getProtocol() {
-        if (protocol != null) {
-            return protocol.trim();
-        } else {
+        if (protocol == null) {
             return null;
         }
+        String p = protocol.trim();
+        // As on create, protocol number 1 is ICMP so that it keeps its icmp type and code
+        if (StringUtils.isNumeric(p) && Integer.parseInt(p) == NetUtils.ICMP_PROTO_NUMBER) {
+            p = NetUtils.ICMP_PROTO;
+        }
+        return p;
     }
 
     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,13 +23,18 @@
 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;
 import com.cloud.utils.net.NetUtils;
 
 public class SetNetworkACLCommand extends NetworkElementCommand {
     public static final String RULE_DETAIL_SEPARATOR = ";";
+    private static final int TCP_PROTO_NUMBER = 6;
+    private static final int UDP_PROTO_NUMBER = 17;
 
     NetworkACLTO[] rules;
     NicTO nic;
@@ -70,9 +75,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 +103,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 == TCP_PROTO_NUMBER) {
+            return NetUtils.TCP_PROTO;
+        }
+        if (hasPorts && number == 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;
@@ -116,6 +117,8 @@ public class NetworkACLServiceImpl extends ManagerBase implements NetworkACLServ
     private VpcManager vpcManager;
 
     private String supportedProtocolsForAclRules = "tcp,udp,icmp,all";
+    private static final int TCP_PROTO_NUMBER = 6;
+    private static final int UDP_PROTO_NUMBER = 17;
 
     @Override
     public NetworkACL createNetworkACL(final String name, final String description, final long vpcId, final Boolean forDisplay) {
@@ -365,6 +368,7 @@ public NetworkACLItem createNetworkACLItem(CreateNetworkACLCmd createNetworkACLC
         networkACLItemVO.setDisplay(createNetworkACLCmd.isDisplay());
 
         validateNetworkACLItem(networkACLItemVO);
+        validatePortsAreUsableWithProtocol(protocol, sourcePortStart, sourcePortEnd);
         return _networkAclMgr.createNetworkACLItem(networkACLItemVO);
     }
 
@@ -690,6 +694,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 != TCP_PROTO_NUMBER && protoNumber != 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]",
+                    TCP_PROTO_NUMBER, 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 +907,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 || ''
       }