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..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 @@ -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; @@ -30,6 +33,8 @@ 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 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 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..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 @@ -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)); + } } 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..8909b594c7bf 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,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); } 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..97fd416a7eb0 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,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<>()); 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 || '' }