From 3495705dc59e8c5f9e1013872e4f610f4221a20b Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 8 Oct 2026 10:13:23 +0000 Subject: [PATCH 1/4] Keep the ports and ICMP type of ACL rules given by protocol number The VR applies ports only to an ACL rule it is sent as tcp or udp, and an ICMP type and code only to one sent as icmp; anything else becomes a bare protocol-number rule. generateFwRules() sent the protocol as stored, so: - protocol 6 or 17 with ports, which the UI and API accept, was applied to the whole protocol, e.g. all TCP instead of port 22; - protocol 1 set by updateNetworkACLItem (create maps it to icmp, update did not) lost its ICMP type and allowed all ICMP; - an upper-case ICMP failed the case-sensitive "icmp" check and went out with the port range, 0:0, as its type and code, i.e. echo-reply only. Send 1, 6 and 17, and any case, as icmp, tcp and udp, with -1 (any) for a missing ICMP type or code; map protocol 1 to icmp on update as create does. Ports with any other protocol number can't be applied, so reject them when given, and stop the UI offering them. They are only checked when given, so existing rules that carry them stay editable. Fixes: #14356 Signed-off-by: Brad House --- .../user/network/UpdateNetworkACLItemCmd.java | 12 +++-- .../network/UpdateNetworkACLItemCmdTest.java | 44 +++++++++++++++++++ .../api/routing/SetNetworkACLCommand.java | 36 +++++++++++++-- .../api/routing/SetNetworkACLCommandTest.java | 32 ++++++++++++++ .../network/vpc/NetworkACLServiceImpl.java | 21 +++++++++ .../vpc/NetworkACLServiceImplTest.java | 22 ++++++++++ ui/src/views/network/AclRulesTab.vue | 8 +++- 7 files changed, 167 insertions(+), 8 deletions(-) create mode 100644 api/src/test/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkACLItemCmdTest.java diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkACLItemCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkACLItemCmd.java index 9e7e1b5b8542..10898e819f7d 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkACLItemCmd.java +++ b/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 getSourceCidrList() { diff --git a/api/src/test/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkACLItemCmdTest.java b/api/src/test/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkACLItemCmdTest.java new file mode 100644 index 000000000000..6bab9798deb0 --- /dev/null +++ b/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)); + } +} diff --git a/core/src/main/java/com/cloud/agent/api/routing/SetNetworkACLCommand.java b/core/src/main/java/com/cloud/agent/api/routing/SetNetworkACLCommand.java index c7cb1e61e577..9f2026ee2256 100644 --- a/core/src/main/java/com/cloud/agent/api/routing/SetNetworkACLCommand.java +++ b/core/src/main/java/com/cloud/agent/api/routing/SetNetworkACLCommand.java @@ -23,6 +23,7 @@ import java.util.Collections; import java.util.Comparator; import java.util.List; +import java.util.Locale; import com.cloud.agent.api.to.NetworkACLTO; import com.cloud.agent.api.to.NicTO; @@ -70,9 +71,10 @@ public String[][] generateFwRules() { List 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()); + 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 +99,34 @@ public String[][] generateFwRules() { return result; } + /** + * The VR applies ports only to a rule it is told is tcp or udp, and an ICMP type and code only to + * one it is told is icmp; any other protocol goes to it as a bare number. Name those three by their + * protocol number too, and whatever the case they were stored in, so their ports or ICMP type are + * not dropped. + */ + protected static String normalizeProtocol(String protocol) { + if (protocol == null) { + return null; + } + String p = protocol.trim().toLowerCase(Locale.ROOT); + switch (p) { + case "1": + return NetUtils.ICMP_PROTO; + case "6": + return NetUtils.TCP_PROTO; + case "17": + return NetUtils.UDP_PROTO; + default: + return p; + } + } + + 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 aclList) { Collections.sort(aclList, new Comparator() { @Override diff --git a/core/src/test/java/com/cloud/agent/api/routing/SetNetworkACLCommandTest.java b/core/src/test/java/com/cloud/agent/api/routing/SetNetworkACLCommandTest.java index af30652d8ca1..07b343b99f7c 100644 --- a/core/src/test/java/com/cloud/agent/api/routing/SetNetworkACLCommandTest.java +++ b/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,35 @@ 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 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 testProtocolNameIsMatchedWhateverItsCase() { + assertEquals("Ingress;icmp;8;0;10.0.0.0/24;ACCEPT;", generateRule("ICMP", null, null, 8, 0)); + assertEquals("Ingress;tcp;22;22;10.0.0.0/24;ACCEPT;", generateRule("TCP", 22, 22, null, null)); + } + + @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)); + } } diff --git a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java index 94e79ec6ea75..52c496a5b11c 100644 --- a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java +++ b/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,21 @@ 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)); + } + } + @Override public NetworkACLItem getNetworkACLItem(final long ruleId) { return _networkAclMgr.getNetworkACLItem(ruleId); @@ -876,6 +895,8 @@ public NetworkACLItem updateNetworkACLItem(UpdateNetworkACLItemCmd updateNetwork transferDataToNetworkAclRulePojo(updateNetworkACLItemCmd, networkACLItemVo, acl); validateNetworkACLItem(networkACLItemVo); + // only the ports given now: older rules may already carry ports, and must stay editable + validatePortsAreUsableWithProtocol(networkACLItemVo.getProtocol(), updateNetworkACLItemCmd.getSourcePortStart(), updateNetworkACLItemCmd.getSourcePortEnd()); return _networkAclMgr.updateNetworkACLItem(networkACLItemVo); } diff --git a/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java b/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java index b24136972ad8..9912e62d004c 100644 --- a/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java +++ b/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java @@ -542,6 +542,28 @@ 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 validateSourceCidrListTestEmptySourceCirdList() { Mockito.when(networkAclItemVoMock.getSourceCidrList()).thenReturn(new ArrayList<>()); diff --git a/ui/src/views/network/AclRulesTab.vue b/ui/src/views/network/AclRulesTab.vue index e452e495a224..ef34c5fa99ba 100644 --- a/ui/src/views/network/AclRulesTab.vue +++ b/ui/src/views/network/AclRulesTab.vue @@ -276,7 +276,7 @@ -
+
@@ -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 || '' } From a54b84a3cae5c72d6b0a821d7857b30dbd230e77 Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 8 Oct 2026 10:57:46 +0000 Subject: [PATCH 2/4] Keep protocol 6 and 17 without ports as the whole protocol Sending 6 or 17 to the VR as tcp or udp also when the rule has no ports made it a tcp or udp rule with the port range 0:0, which the VR's iptables path applies as --dport 0: a rule meant for all of TCP or UDP matched port 0 only. Map them only when the rule has ports; without, the bare number is what matches the whole protocol, as before. Also read the protocol as a number, so that "006" is treated as 6, as the API's own validation does, and check the ports a rule already carries when an update changes its protocol, so that switching a tcp rule to another protocol number cannot leave its ports on it. Signed-off-by: Brad House --- .../api/routing/SetNetworkACLCommand.java | 44 ++++++++++++------- .../api/routing/SetNetworkACLCommandTest.java | 17 ++++++- .../network/vpc/NetworkACLServiceImpl.java | 15 ++++++- .../vpc/NetworkACLServiceImplTest.java | 20 +++++++++ 4 files changed, 77 insertions(+), 19 deletions(-) diff --git a/core/src/main/java/com/cloud/agent/api/routing/SetNetworkACLCommand.java b/core/src/main/java/com/cloud/agent/api/routing/SetNetworkACLCommand.java index 9f2026ee2256..2c6d0f14cf48 100644 --- a/core/src/main/java/com/cloud/agent/api/routing/SetNetworkACLCommand.java +++ b/core/src/main/java/com/cloud/agent/api/routing/SetNetworkACLCommand.java @@ -25,12 +25,16 @@ 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; @@ -71,7 +75,7 @@ public String[][] generateFwRules() { List cidr; final StringBuilder sb = new StringBuilder(); - final String protocol = normalizeProtocol(aclTO.getProtocol()); + 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); @@ -100,26 +104,36 @@ public String[][] generateFwRules() { } /** - * The VR applies ports only to a rule it is told is tcp or udp, and an ICMP type and code only to - * one it is told is icmp; any other protocol goes to it as a bare number. Name those three by their - * protocol number too, and whatever the case they were stored in, so their ports or ICMP type are - * not dropped. + * 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: that is what matches all of + * TCP or UDP, where a tcp or udp rule without ports would match port 0 only. */ - protected static String normalizeProtocol(String protocol) { + protected static String normalizeProtocol(String protocol, boolean hasPorts) { if (protocol == null) { return null; } String p = protocol.trim().toLowerCase(Locale.ROOT); - switch (p) { - case "1": - return NetUtils.ICMP_PROTO; - case "6": - return NetUtils.TCP_PROTO; - case "17": - return NetUtils.UDP_PROTO; - default: - return p; + 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) { diff --git a/core/src/test/java/com/cloud/agent/api/routing/SetNetworkACLCommandTest.java b/core/src/test/java/com/cloud/agent/api/routing/SetNetworkACLCommandTest.java index 07b343b99f7c..e20d4f51c246 100644 --- a/core/src/test/java/com/cloud/agent/api/routing/SetNetworkACLCommandTest.java +++ b/core/src/test/java/com/cloud/agent/api/routing/SetNetworkACLCommandTest.java @@ -64,6 +64,20 @@ public void testTcpAndUdpByProtocolNumberKeepTheirPorts() { assertEquals("Ingress;udp;53;53;10.0.0.0/24;ACCEPT;", generateRule("17", 53, 53, null, null)); } + @Test + public void testTcpAndUdpByProtocolNumberWithoutPortsStayWholeProtocol() { + // as tcp or udp with no ports the VR would match port 0 only + 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)); @@ -72,9 +86,8 @@ public void testIcmpByProtocolNumberKeepsItsTypeAndCode() { } @Test - public void testProtocolNameIsMatchedWhateverItsCase() { + public void testIcmpIsMatchedWhateverItsCase() { assertEquals("Ingress;icmp;8;0;10.0.0.0/24;ACCEPT;", generateRule("ICMP", null, null, 8, 0)); - assertEquals("Ingress;tcp;22;22;10.0.0.0/24;ACCEPT;", generateRule("TCP", 22, 22, null, null)); } @Test diff --git a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java index 52c496a5b11c..8909b594c7bf 100644 --- a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java +++ b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java @@ -709,6 +709,18 @@ protected void validatePortsAreUsableWithProtocol(String protocol, Integer sourc } } + /** + * 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); @@ -895,8 +907,7 @@ public NetworkACLItem updateNetworkACLItem(UpdateNetworkACLItemCmd updateNetwork transferDataToNetworkAclRulePojo(updateNetworkACLItemCmd, networkACLItemVo, acl); validateNetworkACLItem(networkACLItemVo); - // only the ports given now: older rules may already carry ports, and must stay editable - validatePortsAreUsableWithProtocol(networkACLItemVo.getProtocol(), updateNetworkACLItemCmd.getSourcePortStart(), updateNetworkACLItemCmd.getSourcePortEnd()); + validatePortsAreUsableWithProtocolOnUpdate(updateNetworkACLItemCmd, networkACLItemVo); return _networkAclMgr.updateNetworkACLItem(networkACLItemVo); } diff --git a/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java b/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java index 9912e62d004c..97fd416a7eb0 100644 --- a/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java +++ b/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java @@ -564,6 +564,26 @@ public void validatePortsAreUsableWithProtocolTestOtherProtocolNumberWithStartPo 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<>()); From e32fbb802a0e9bcde25aba88d506434802bfef1e Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 8 Oct 2026 12:53:19 +0000 Subject: [PATCH 3/4] Describe why protocol 6 and 17 without ports stay numbers without relying on the port 0 bug Signed-off-by: Brad House --- .../com/cloud/agent/api/routing/SetNetworkACLCommand.java | 5 +++-- .../cloud/agent/api/routing/SetNetworkACLCommandTest.java | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/com/cloud/agent/api/routing/SetNetworkACLCommand.java b/core/src/main/java/com/cloud/agent/api/routing/SetNetworkACLCommand.java index 2c6d0f14cf48..c90e96f4b952 100644 --- a/core/src/main/java/com/cloud/agent/api/routing/SetNetworkACLCommand.java +++ b/core/src/main/java/com/cloud/agent/api/routing/SetNetworkACLCommand.java @@ -107,8 +107,9 @@ public String[][] generateFwRules() { * 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: that is what matches all of - * TCP or UDP, where a tcp or udp rule without ports would match port 0 only. + * 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) { diff --git a/core/src/test/java/com/cloud/agent/api/routing/SetNetworkACLCommandTest.java b/core/src/test/java/com/cloud/agent/api/routing/SetNetworkACLCommandTest.java index e20d4f51c246..cbe44e3a1021 100644 --- a/core/src/test/java/com/cloud/agent/api/routing/SetNetworkACLCommandTest.java +++ b/core/src/test/java/com/cloud/agent/api/routing/SetNetworkACLCommandTest.java @@ -66,7 +66,7 @@ public void testTcpAndUdpByProtocolNumberKeepTheirPorts() { @Test public void testTcpAndUdpByProtocolNumberWithoutPortsStayWholeProtocol() { - // as tcp or udp with no ports the VR would match port 0 only + // 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)); } From 252038f886172029ef42da36004d7f66ff0f4054 Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 22 Oct 2026 13:10:00 +0000 Subject: [PATCH 4/4] Move the protocol number helpers to NetUtils createNetworkACL and updateNetworkACLItem each turned protocol number 1 into icmp with their own copy of the same code; they now share NetUtils.icmpProtocolNumberAsName. The TCP and UDP protocol numbers move there too, next to ICMP_PROTO_NUMBER, in place of the private copies in NetworkACLServiceImpl and SetNetworkACLCommand. Signed-off-by: Brad House --- .../user/network/CreateNetworkACLCmd.java | 11 +---------- .../user/network/UpdateNetworkACLItemCmd.java | 11 +---------- .../api/routing/SetNetworkACLCommand.java | 6 ++---- .../network/vpc/NetworkACLServiceImpl.java | 6 ++---- .../java/com/cloud/utils/net/NetUtils.java | 18 ++++++++++++++++++ .../java/com/cloud/utils/net/NetUtilsTest.java | 15 +++++++++++++++ 6 files changed, 39 insertions(+), 28 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/network/CreateNetworkACLCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/network/CreateNetworkACLCmd.java index 087e31c08829..a2b86a4c4893 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/network/CreateNetworkACLCmd.java +++ b/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) { diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkACLItemCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkACLItemCmd.java index 10898e819f7d..19c13f5375c0 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkACLItemCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/network/UpdateNetworkACLItemCmd.java @@ -26,7 +26,6 @@ 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; @@ -101,15 +100,7 @@ public Long getId() { } public String getProtocol() { - 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; + return NetUtils.icmpProtocolNumberAsName(protocol); } public List getSourceCidrList() { diff --git a/core/src/main/java/com/cloud/agent/api/routing/SetNetworkACLCommand.java b/core/src/main/java/com/cloud/agent/api/routing/SetNetworkACLCommand.java index c90e96f4b952..06b58ade34fc 100644 --- a/core/src/main/java/com/cloud/agent/api/routing/SetNetworkACLCommand.java +++ b/core/src/main/java/com/cloud/agent/api/routing/SetNetworkACLCommand.java @@ -33,8 +33,6 @@ 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; @@ -128,10 +126,10 @@ protected static String normalizeProtocol(String protocol, boolean hasPorts) { if (number == NetUtils.ICMP_PROTO_NUMBER) { return NetUtils.ICMP_PROTO; } - if (hasPorts && number == TCP_PROTO_NUMBER) { + if (hasPorts && number == NetUtils.TCP_PROTO_NUMBER) { return NetUtils.TCP_PROTO; } - if (hasPorts && number == UDP_PROTO_NUMBER) { + if (hasPorts && number == NetUtils.UDP_PROTO_NUMBER) { return NetUtils.UDP_PROTO; } return String.valueOf(number); diff --git a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java index 8909b594c7bf..f18d7cb83bae 100644 --- a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java +++ b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java @@ -117,8 +117,6 @@ 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) { @@ -703,9 +701,9 @@ protected void validatePortsAreUsableWithProtocol(String protocol, Integer sourc return; } int protoNumber = NumberUtils.toInt(protocol, -1); - if (protoNumber != TCP_PROTO_NUMBER && protoNumber != UDP_PROTO_NUMBER) { + 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]", - TCP_PROTO_NUMBER, UDP_PROTO_NUMBER, protocol)); + NetUtils.TCP_PROTO_NUMBER, NetUtils.UDP_PROTO_NUMBER, protocol)); } } diff --git a/utils/src/main/java/com/cloud/utils/net/NetUtils.java b/utils/src/main/java/com/cloud/utils/net/NetUtils.java index d89d9fa2d93c..13e23abf3d19 100644 --- a/utils/src/main/java/com/cloud/utils/net/NetUtils.java +++ b/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"); diff --git a/utils/src/test/java/com/cloud/utils/net/NetUtilsTest.java b/utils/src/test/java/com/cloud/utils/net/NetUtilsTest.java index 5c9d41f90a25..5c7fe83e82cf 100644 --- a/utils/src/test/java/com/cloud/utils/net/NetUtilsTest.java +++ b/utils/src/test/java/com/cloud/utils/net/NetUtilsTest.java @@ -955,4 +955,19 @@ public void testVpnIpRange() { String cidr = NetUtils.transformCidr(startIp + "/" + cidrSize); Assert.assertEquals("10.1.2.0/28", cidr); } + + @Test + public void icmpProtocolNumberAsNameTurnsProtocolNumberOneIntoIcmp() { + assertEquals(NetUtils.ICMP_PROTO, NetUtils.icmpProtocolNumberAsName("1")); + assertEquals(NetUtils.ICMP_PROTO, NetUtils.icmpProtocolNumberAsName(" 1 ")); + assertEquals(NetUtils.ICMP_PROTO, NetUtils.icmpProtocolNumberAsName("01")); + } + + @Test + public void icmpProtocolNumberAsNameLeavesOtherProtocolsTrimmed() { + assertEquals("tcp", NetUtils.icmpProtocolNumberAsName(" tcp ")); + assertEquals("47", NetUtils.icmpProtocolNumberAsName("47")); + assertEquals("99999999999", NetUtils.icmpProtocolNumberAsName("99999999999")); + Assert.assertNull(NetUtils.icmpProtocolNumberAsName(null)); + } }