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 9e7e1b5b8542..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 @@ -31,6 +31,7 @@ import com.cloud.exception.ResourceUnavailableException; import com.cloud.network.vpc.NetworkACLItem; import com.cloud.user.Account; +import com.cloud.utils.net.NetUtils; @APICommand(name = "updateNetworkACLItem", description = "Updates ACL item with specified ID", responseObject = NetworkACLItemResponse.class, requestHasSensitiveInfo = false, responseHasSensitiveInfo = false) public class UpdateNetworkACLItemCmd extends BaseAsyncCustomIdCmd { @@ -99,11 +100,7 @@ public Long getId() { } public String getProtocol() { - if (protocol != null) { - return protocol.trim(); - } else { - return null; - } + return NetUtils.icmpProtocolNumberAsName(protocol); } public List 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..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 @@ -23,6 +23,9 @@ import java.util.Collections; import java.util.Comparator; import java.util.List; +import java.util.Locale; + +import org.apache.commons.lang3.StringUtils; import com.cloud.agent.api.to.NetworkACLTO; import com.cloud.agent.api.to.NicTO; @@ -70,9 +73,10 @@ public String[][] generateFwRules() { List cidr; final StringBuilder sb = new StringBuilder(); - sb.append(aclTO.getTrafficType().toString()).append(RULE_DETAIL_SEPARATOR).append(aclTO.getProtocol()).append(RULE_DETAIL_SEPARATOR); - if ("icmp".compareTo(aclTO.getProtocol()) == 0) { - sb.append(aclTO.getIcmpType()).append(RULE_DETAIL_SEPARATOR).append(aclTO.getIcmpCode()).append(RULE_DETAIL_SEPARATOR); + final String protocol = normalizeProtocol(aclTO.getProtocol(), aclTO.getSrcPortRange() != null); + sb.append(aclTO.getTrafficType().toString()).append(RULE_DETAIL_SEPARATOR).append(protocol).append(RULE_DETAIL_SEPARATOR); + if (NetUtils.ICMP_PROTO.equals(protocol)) { + sb.append(icmpTypeOrCode(aclTO.getIcmpType())).append(RULE_DETAIL_SEPARATOR).append(icmpTypeOrCode(aclTO.getIcmpCode())).append(RULE_DETAIL_SEPARATOR); } else { sb.append(aclTO.getStringPortRange().replace(":", RULE_DETAIL_SEPARATOR)).append(RULE_DETAIL_SEPARATOR); } @@ -97,6 +101,45 @@ public String[][] generateFwRules() { return result; } + /** + * The VR applies ports only to a rule it is sent as tcp or udp, and an ICMP type and code only to one + * sent as icmp; any other protocol goes to it as a bare number, which matches the whole protocol. So + * send protocol number 1 as icmp, and 6 and 17 as tcp and udp when the rule has ports, whatever form + * or case they were stored in. Without ports 6 and 17 stay bare numbers, which match the whole + * protocol on every VR path; a tcp or udp rule without ports has matched port 0 only on the iptables + * path (see #14363). + */ + protected static String normalizeProtocol(String protocol, boolean hasPorts) { + if (protocol == null) { + return null; + } + String p = protocol.trim().toLowerCase(Locale.ROOT); + if (!StringUtils.isNumeric(p)) { + return p; + } + int number; + try { + number = Integer.parseInt(p); + } catch (NumberFormatException e) { + return p; + } + if (number == NetUtils.ICMP_PROTO_NUMBER) { + return NetUtils.ICMP_PROTO; + } + if (hasPorts && number == NetUtils.TCP_PROTO_NUMBER) { + return NetUtils.TCP_PROTO; + } + if (hasPorts && number == NetUtils.UDP_PROTO_NUMBER) { + return NetUtils.UDP_PROTO; + } + return String.valueOf(number); + } + + private static int icmpTypeOrCode(Integer value) { + // -1 is "any"; a rule stored as protocol 1 may carry no ICMP type or code at all + return value == null ? -1 : value; + } + protected void orderNetworkAclRulesByRuleNumber(List 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..f18d7cb83bae 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; @@ -365,6 +366,7 @@ public NetworkACLItem createNetworkACLItem(CreateNetworkACLCmd createNetworkACLC networkACLItemVO.setDisplay(createNetworkACLCmd.isDisplay()); validateNetworkACLItem(networkACLItemVO); + validatePortsAreUsableWithProtocol(protocol, sourcePortStart, sourcePortEnd); return _networkAclMgr.createNetworkACLItem(networkACLItemVO); } @@ -690,6 +692,33 @@ protected void validateSourceStartAndEndPorts(NetworkACLItemVO networkACLItemVO) } } + /** + * Ports are only applied to TCP and UDP, so they cannot be given with any other protocol number: + * the rule would be applied to the whole protocol, ignoring them. + */ + protected void validatePortsAreUsableWithProtocol(String protocol, Integer sourcePortStart, Integer sourcePortEnd) { + if ((sourcePortStart == null && sourcePortEnd == null) || !StringUtils.isNumeric(protocol)) { + return; + } + int protoNumber = NumberUtils.toInt(protocol, -1); + if (protoNumber != NetUtils.TCP_PROTO_NUMBER && protoNumber != NetUtils.UDP_PROTO_NUMBER) { + throw new InvalidParameterValueException(String.format("Start and end port can only be given for TCP or UDP (protocol number %d or %d), not for protocol number [%s]", + NetUtils.TCP_PROTO_NUMBER, NetUtils.UDP_PROTO_NUMBER, protocol)); + } + } + + /** + * On update the ports a rule already carries are only checked when the protocol is given too, so + * that a rule stored with ports on another protocol number can still be edited otherwise, while a + * protocol change cannot leave them on it. + */ + protected void validatePortsAreUsableWithProtocolOnUpdate(UpdateNetworkACLItemCmd updateNetworkACLItemCmd, NetworkACLItemVO networkACLItemVo) { + boolean protocolGiven = StringUtils.isNotBlank(updateNetworkACLItemCmd.getProtocol()); + validatePortsAreUsableWithProtocol(networkACLItemVo.getProtocol(), + protocolGiven ? networkACLItemVo.getSourcePortStart() : updateNetworkACLItemCmd.getSourcePortStart(), + protocolGiven ? networkACLItemVo.getSourcePortEnd() : updateNetworkACLItemCmd.getSourcePortEnd()); + } + @Override public NetworkACLItem getNetworkACLItem(final long ruleId) { return _networkAclMgr.getNetworkACLItem(ruleId); @@ -876,6 +905,7 @@ public NetworkACLItem updateNetworkACLItem(UpdateNetworkACLItemCmd updateNetwork transferDataToNetworkAclRulePojo(updateNetworkACLItemCmd, networkACLItemVo, acl); validateNetworkACLItem(networkACLItemVo); + validatePortsAreUsableWithProtocolOnUpdate(updateNetworkACLItemCmd, networkACLItemVo); return _networkAclMgr.updateNetworkACLItem(networkACLItemVo); } 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 || '' } 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)); + } }