From 59c45002682824fac94bd6b25ef66572e7595684 Mon Sep 17 00:00:00 2001 From: Brad House Date: Mon, 9 Nov 2026 19:10:27 +0000 Subject: [PATCH] Refuse an incomplete full update of a network ACL rule updateNetworkACLItem with partialupgrade=false replaces every field of the rule. A call that left out cidrlist or traffictype wrote the rule with those fields null, then failed with an NPE on the null CIDR list. The row was already committed, so listNetworkACLs and the next ACL apply failed on the null traffic type until the rule was fully updated again. A call without protocol failed with an NPE. Refuse a full update without protocol, cidrlist or traffictype before anything is changed. Write the rule and its CIDRs in one transaction, so that a failure on the CIDRs rolls back the rule too. Refuse an invalid traffictype, as createNetworkACL does, instead of ignoring it. A full update to a non-ICMP protocol also dropped an ICMP type or code given with it: protocol=tcp icmptype=8 was accepted and stored as tcp. Keep the given values so that the existing validation refuses them, as it does on create and on a partial update. Signed-off-by: Brad House --- .../user/network/UpdateNetworkACLItemCmd.java | 19 +-- .../vpc/dao/NetworkACLItemDaoImpl.java | 6 + .../network/vpc/NetworkACLServiceImpl.java | 11 +- .../vpc/NetworkACLServiceImplTest.java | 129 ++++++++++++++++-- 4 files changed, 142 insertions(+), 23 deletions(-) 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..bc39a57403c4 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,8 +26,10 @@ 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.InvalidParameterValueException; import com.cloud.exception.ResourceUnavailableException; import com.cloud.network.vpc.NetworkACLItem; import com.cloud.user.Account; @@ -62,7 +64,7 @@ public class UpdateNetworkACLItemCmd extends BaseAsyncCustomIdCmd { @Parameter(name = ApiConstants.ICMP_CODE, type = CommandType.INTEGER, description = "Error code for this ICMP message") private Integer icmpCode; - @Parameter(name = ApiConstants.TRAFFIC_TYPE, type = CommandType.STRING, description = "The traffic type for the ACL, can be Ingress or Egress, defaulted to Ingress if not specified") + @Parameter(name = ApiConstants.TRAFFIC_TYPE, type = CommandType.STRING, description = "The traffic type for the ACL, can be Ingress or Egress. Required when partialupgrade is false") private String trafficType; @Parameter(name = ApiConstants.NUMBER, type = CommandType.INTEGER, description = "The Network of the Instance the ACL will be created for") @@ -78,7 +80,7 @@ public class UpdateNetworkACLItemCmd extends BaseAsyncCustomIdCmd { @Parameter(name = ApiConstants.ACL_REASON, type = CommandType.STRING, description = "A description indicating why the ACL rule is required.") private String reason; - @Parameter(name = ApiConstants.ACL_RULE_PARTIAL_UPGRADE, type = CommandType.BOOLEAN, required = false, description = "Indicates if the ACL rule is to be updated partially (merging the parameters sent with current configuration) or completely (disconsidering all of the current configurations). The default value is 'true'.") + @Parameter(name = ApiConstants.ACL_RULE_PARTIAL_UPGRADE, type = CommandType.BOOLEAN, required = false, description = "Indicates if the ACL rule is to be updated partially (merging the parameters sent with current configuration) or completely (disconsidering all of the current configurations). The default value is 'true'. When it is 'false', protocol, cidrlist and traffictype are required.") private boolean partialUpgrade = true; // /////////////////////////////////////////////////// @@ -111,14 +113,15 @@ public List getSourceCidrList() { } public NetworkACLItem.TrafficType getTrafficType() { - if (trafficType != null) { - for (NetworkACLItem.TrafficType type : NetworkACLItem.TrafficType.values()) { - if (type.toString().equalsIgnoreCase(trafficType)) { - return type; - } + if (StringUtils.isBlank(trafficType)) { + return null; + } + for (NetworkACLItem.TrafficType type : NetworkACLItem.TrafficType.values()) { + if (type.toString().equalsIgnoreCase(trafficType.trim())) { + return type; } } - return null; + throw new InvalidParameterValueException("Invalid traffic type " + trafficType); } // /////////////////////////////////////////////////// diff --git a/engine/schema/src/main/java/com/cloud/network/vpc/dao/NetworkACLItemDaoImpl.java b/engine/schema/src/main/java/com/cloud/network/vpc/dao/NetworkACLItemDaoImpl.java index 925515f6f4ab..5f3a802d79a9 100644 --- a/engine/schema/src/main/java/com/cloud/network/vpc/dao/NetworkACLItemDaoImpl.java +++ b/engine/schema/src/main/java/com/cloud/network/vpc/dao/NetworkACLItemDaoImpl.java @@ -92,8 +92,14 @@ public NetworkACLItemVO findById(Long id) { @Override public boolean update(Long id, NetworkACLItemVO item) { + // the rule and its CIDRs are written together, so that a failure on the CIDRs does not leave the rule half updated + TransactionLegacy txn = TransactionLegacy.currentTxn(); + txn.start(); + boolean result = super.update(id, item); _networkACLItemCidrsDao.updateCidrs(item.getId(), item.getSourceCidrList()); + + txn.commit(); return result; } 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..d3fe5f535c57 100644 --- a/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java +++ b/server/src/main/java/com/cloud/network/vpc/NetworkACLServiceImpl.java @@ -885,6 +885,8 @@ public NetworkACLItem updateNetworkACLItem(UpdateNetworkACLItemCmd updateNetwork * All of the parameters in {@link UpdateNetworkACLItemCmd} that are not null will be set to their corresponding fields in {@link NetworkACLItemVO}. * If the parameter {@link UpdateNetworkACLItemCmd#isPartialUpgrade()} returns false, we will use null parameters, which will allow us to completely update the ACL rule. * However, the number and custom Uuid will never be set to null. Therefore, if it is not a partial upgrade, these values will remain the same. + * A complete update requires the protocol, the CIDR list and the traffic type: a rule cannot be stored without them, and defaulting them could widen the rule + * or change its direction without the caller noticing. * * We use {@link #validateAndCreateNetworkAclRuleAction(String)} when converting an action as {@link String} to its Enum corresponding value. */ @@ -898,6 +900,10 @@ protected void transferDataToNetworkAclRulePojo(UpdateNetworkACLItemCmd updateNe networkACLItemVo.setNumber(number); } boolean isPartialUpgrade = updateNetworkACLItemCmd.isPartialUpgrade(); + if (!isPartialUpgrade && (StringUtils.isBlank(updateNetworkACLItemCmd.getProtocol()) || updateNetworkACLItemCmd.getSourceCidrList() == null + || updateNetworkACLItemCmd.getTrafficType() == null)) { + throw new InvalidParameterValueException("The protocol, cidrlist and traffictype must be given when the ACL rule is updated with partialupgrade=false"); + } Integer sourcePortStart = updateNetworkACLItemCmd.getSourcePortStart(); if (!isPartialUpgrade || sourcePortStart != null) { @@ -960,8 +966,9 @@ private void updateIcmpCodeAndTypeFullUpgrade (Integer icmpCode, Integer icmpTyp networkACLItemVo.setIcmpCode(icmpCode != null ? icmpCode : -1); networkACLItemVo.setIcmpType(icmpType != null ? icmpType : -1); } else { - networkACLItemVo.setIcmpCode(null); - networkACLItemVo.setIcmpType(null); + // keep what was given, so that validateProtocol() refuses an ICMP type or code on another protocol, as on create + networkACLItemVo.setIcmpCode(icmpCode); + networkACLItemVo.setIcmpType(icmpType); } } 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..9c6539824ad7 100644 --- a/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java +++ b/server/src/test/java/com/cloud/network/vpc/NetworkACLServiceImplTest.java @@ -814,29 +814,73 @@ public void transferDataToNetworkAclRulePojoTestPartialUpgradeAllValuesNull() { public void transferDataToNetworkAclRulePojoTestNotPartialUpgradeAllValuesNull() { Mockito.when(updateNetworkACLItemCmdMock.isPartialUpgrade()).thenReturn(false); + Mockito.when(updateNetworkACLItemCmdMock.getNumber()).thenReturn(null); + Mockito.when(updateNetworkACLItemCmdMock.getProtocol()).thenReturn(null); + + InvalidParameterValueException e = Assert.assertThrows(InvalidParameterValueException.class, + () -> networkAclServiceImpl.transferDataToNetworkAclRulePojo(updateNetworkACLItemCmdMock, networkAclItemVoMock, networkAclMock)); + + Assert.assertEquals("The protocol, cidrlist and traffictype must be given when the ACL rule is updated with partialupgrade=false", e.getMessage()); + Mockito.verify(networkAclItemVoMock, Mockito.never()).setProtocol(nullable(String.class)); + Mockito.verify(networkAclItemVoMock, Mockito.never()).setSourceCidrList(nullable(List.class)); + Mockito.verify(networkAclItemVoMock, Mockito.never()).setTrafficType(nullable(TrafficType.class)); + } + + @Test + public void transferDataToNetworkAclRulePojoTestNotPartialUpgradeWithoutCidrListOrTrafficType() { + Mockito.when(updateNetworkACLItemCmdMock.isPartialUpgrade()).thenReturn(false); + Mockito.when(updateNetworkACLItemCmdMock.getNumber()).thenReturn(null); + Mockito.when(updateNetworkACLItemCmdMock.getProtocol()).thenReturn("tcp"); + + String expected = "The protocol, cidrlist and traffictype must be given when the ACL rule is updated with partialupgrade=false"; + + Mockito.when(updateNetworkACLItemCmdMock.getSourceCidrList()).thenReturn(null); + InvalidParameterValueException e = Assert.assertThrows(InvalidParameterValueException.class, + () -> networkAclServiceImpl.transferDataToNetworkAclRulePojo(updateNetworkACLItemCmdMock, networkAclItemVoMock, networkAclMock)); + Assert.assertEquals(expected, e.getMessage()); + + Mockito.when(updateNetworkACLItemCmdMock.getSourceCidrList()).thenReturn(List.of("10.1.1.0/24")); + Mockito.when(updateNetworkACLItemCmdMock.getTrafficType()).thenReturn(null); + e = Assert.assertThrows(InvalidParameterValueException.class, + () -> networkAclServiceImpl.transferDataToNetworkAclRulePojo(updateNetworkACLItemCmdMock, networkAclItemVoMock, networkAclMock)); + Assert.assertEquals(expected, e.getMessage()); + Mockito.verify(updateNetworkACLItemCmdMock).getTrafficType(); + + Mockito.verify(networkAclItemVoMock, Mockito.never()).setSourceCidrList(nullable(List.class)); + Mockito.verify(networkAclItemVoMock, Mockito.never()).setTrafficType(nullable(TrafficType.class)); + } + + @Test + public void transferDataToNetworkAclRulePojoTestNotPartialUpgradeRequiredValuesOnly() { + Mockito.when(updateNetworkACLItemCmdMock.isPartialUpgrade()).thenReturn(false); + + List cidrs = List.of("10.1.1.0/24"); Mockito.when(updateNetworkACLItemCmdMock.getNumber()).thenReturn(null); Mockito.when(updateNetworkACLItemCmdMock.getSourcePortStart()).thenReturn(null); Mockito.when(updateNetworkACLItemCmdMock.getSourcePortEnd()).thenReturn(null); - Mockito.when(updateNetworkACLItemCmdMock.getSourceCidrList()).thenReturn(null); - Mockito.when(updateNetworkACLItemCmdMock.getProtocol()).thenReturn(null); + Mockito.when(updateNetworkACLItemCmdMock.getSourceCidrList()).thenReturn(cidrs); + Mockito.when(updateNetworkACLItemCmdMock.getProtocol()).thenReturn("tcp"); + Mockito.when(updateNetworkACLItemCmdMock.getIcmpCode()).thenReturn(null); + Mockito.when(updateNetworkACLItemCmdMock.getIcmpType()).thenReturn(null); Mockito.when(updateNetworkACLItemCmdMock.getAction()).thenReturn(null); - Mockito.when(updateNetworkACLItemCmdMock.getTrafficType()).thenReturn(null); + Mockito.when(updateNetworkACLItemCmdMock.getTrafficType()).thenReturn(TrafficType.Egress); Mockito.when(updateNetworkACLItemCmdMock.getCustomId()).thenReturn(null); Mockito.when(updateNetworkACLItemCmdMock.getReason()).thenReturn(null); Mockito.when(updateNetworkACLItemCmdMock.isDisplay()).thenReturn(false); - Mockito.when(networkAclItemVoMock.getProtocol()).thenReturn(""); + Mockito.when(networkAclItemVoMock.getProtocol()).thenReturn("tcp"); networkAclServiceImpl.transferDataToNetworkAclRulePojo(updateNetworkACLItemCmdMock, networkAclItemVoMock, networkAclMock); Mockito.verify(networkAclItemVoMock, Mockito.times(0)).setNumber(nullable(Integer.class)); - Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setSourcePortStart(nullable(Integer.class)); - Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setSourcePortEnd(nullable(Integer.class)); - Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setSourceCidrList(nullable(List.class)); - Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setProtocol(nullable(String.class)); - Mockito.verify(networkAclServiceImpl).updateIcmpCodeAndType(Mockito.any(Boolean.class), Mockito.any(), Mockito.any()); - Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setAction(nullable(Action.class)); - Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setTrafficType(nullable(TrafficType.class)); + Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setSourcePortStart(null); + Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setSourcePortEnd(null); + Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setSourceCidrList(cidrs); + Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setProtocol("tcp"); + Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setIcmpCode(null); + Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setIcmpType(null); + Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setAction(Action.Allow); + Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setTrafficType(TrafficType.Egress); Mockito.verify(networkAclItemVoMock, Mockito.times(0)).setUuid(nullable(String.class)); Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setReason(nullable(String.class)); Mockito.verify(networkAclItemVoMock, Mockito.times(1)).setDisplay(nullable(Boolean.class)); @@ -944,8 +988,67 @@ public void updateICMPCodeAndTypeTestNotPartialUpgradeNotICMPNotNullCodeAndType( networkAclServiceImpl.updateIcmpCodeAndType(false, updateNetworkACLItemCmdMock, networkAclItemVoMock); - Mockito.verify(networkAclItemVoMock).setIcmpCode(null); - Mockito.verify(networkAclItemVoMock).setIcmpType(null); + Mockito.verify(networkAclItemVoMock).setIcmpCode(5); + Mockito.verify(networkAclItemVoMock).setIcmpType(5); + } + + private void setUpNotPartialUpgradeToTcp() { + Mockito.when(updateNetworkACLItemCmdMock.isPartialUpgrade()).thenReturn(false); + Mockito.when(updateNetworkACLItemCmdMock.getProtocol()).thenReturn("tcp"); + Mockito.when(updateNetworkACLItemCmdMock.getNumber()).thenReturn(null); + Mockito.when(updateNetworkACLItemCmdMock.getSourcePortStart()).thenReturn(null); + Mockito.when(updateNetworkACLItemCmdMock.getSourcePortEnd()).thenReturn(null); + Mockito.when(updateNetworkACLItemCmdMock.getIcmpCode()).thenReturn(null); + Mockito.when(updateNetworkACLItemCmdMock.getSourceCidrList()).thenReturn(List.of("10.1.1.0/24")); + Mockito.when(updateNetworkACLItemCmdMock.getTrafficType()).thenReturn(TrafficType.Ingress); + } + + private TrafficType trafficTypeOf(String trafficType) { + UpdateNetworkACLItemCmd cmd = new UpdateNetworkACLItemCmd(); + org.springframework.test.util.ReflectionTestUtils.setField(cmd, "trafficType", trafficType); + return cmd.getTrafficType(); + } + + @Test + public void updateNetworkACLItemCmdGetTrafficTypeTest() { + Assert.assertNull(trafficTypeOf(null)); + Assert.assertNull(trafficTypeOf("")); + Assert.assertEquals(TrafficType.Egress, trafficTypeOf("egress")); + Assert.assertEquals(TrafficType.Ingress, trafficTypeOf("Ingress")); + InvalidParameterValueException e = Assert.assertThrows(InvalidParameterValueException.class, () -> trafficTypeOf("egres")); + Assert.assertEquals("Invalid traffic type egres", e.getMessage()); + } + + @Test + public void transferDataToNetworkAclRulePojoTestNotPartialUpgradeTcpWithIcmpTypeIsRefused() { + NetworkACLItemVO rule = new NetworkACLItemVO(null, null, NetUtils.ICMP_PROTO, networkAclMockId, List.of("10.1.1.0/24"), 0, 8, + TrafficType.Ingress, Action.Allow, 10, null); + setUpNotPartialUpgradeToTcp(); + Mockito.when(updateNetworkACLItemCmdMock.getIcmpType()).thenReturn(8); + Mockito.when(updateNetworkACLItemCmdMock.isDisplay()).thenReturn(true); + + networkAclServiceImpl.transferDataToNetworkAclRulePojo(updateNetworkACLItemCmdMock, rule, networkAclMock); + + InvalidParameterValueException e = Assert.assertThrows(InvalidParameterValueException.class, () -> networkAclServiceImpl.validateNetworkACLItem(rule)); + Assert.assertEquals("Can specify icmpCode and icmpType for ICMP protocol only", e.getMessage()); + } + + @Test + public void transferDataToNetworkAclRulePojoTestNotPartialUpgradeIcmpToTcpClearsIcmpFields() { + NetworkACLItemVO rule = new NetworkACLItemVO(null, null, NetUtils.ICMP_PROTO, networkAclMockId, List.of("10.1.1.0/24"), 0, 8, + TrafficType.Ingress, Action.Allow, 10, null); + setUpNotPartialUpgradeToTcp(); + Mockito.when(updateNetworkACLItemCmdMock.getIcmpType()).thenReturn(null); + Mockito.when(updateNetworkACLItemCmdMock.isDisplay()).thenReturn(true); + + networkAclServiceImpl.transferDataToNetworkAclRulePojo(updateNetworkACLItemCmdMock, rule, networkAclMock); + networkAclServiceImpl.validateNetworkACLItem(rule); + + Assert.assertEquals("tcp", rule.getProtocol()); + Assert.assertNull(rule.getIcmpType()); + Assert.assertNull(rule.getIcmpCode()); + Assert.assertEquals(List.of("10.1.1.0/24"), rule.getSourceCidrList()); + Assert.assertEquals(TrafficType.Ingress, rule.getTrafficType()); } @Test