From 59da02ea25ee6284d54a84e13f969fdf715075c9 Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 8 Oct 2026 10:13:23 +0000 Subject: [PATCH 1/3] 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 b25c728502b08831f4045c2558638f39908cbb25 Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 8 Oct 2026 10:57:46 +0000 Subject: [PATCH 2/3] 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 631f707525ebdae3111489065291ba62beb36c71 Mon Sep 17 00:00:00 2001 From: Brad House Date: Thu, 8 Oct 2026 12:53:19 +0000 Subject: [PATCH 3/3] 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)); }