Skip to content

Commit 8e98249

Browse files
Dogface2kDogface2k
committed
network: reject VPC firewall rules on reserved IPs
The VPC ingress firewall path bypassed the network provider reservation check. A custom VPC offering could add a competing firewall rule on an NSX VPN endpoint and prevent recovery of a retained endpoint after an ambiguous gateway creation. Check reservation on the freshly locked IP after existing VPC validation and before persistence. Preserve ordinary VPC public IPs without tier associations and system rules without an IP. Add public-caller regression coverage for reservation, lock freshness and release, access checks, and those compatibility paths.
1 parent a4954ef commit 8e98249

2 files changed

Lines changed: 224 additions & 0 deletions

File tree

‎server/src/main/java/com/cloud/network/firewall/FirewallManagerImpl.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,10 @@ protected FirewallRule createFirewallRuleForVpc(final Long ipAddrId, Account cal
352352

353353
validateFirewallRuleForVpc(caller, ipAddress, portStart, portEnd, protocol, Purpose.Firewall, type, resolvedVpcId, trafficType);
354354

355+
if (ipAddress != null && _networkModel.isIpAddressReserved(ipAddress)) {
356+
throw new InvalidParameterValueException("IP address is reserved by a network provider and cannot be used for " + Service.Firewall.getName());
357+
}
358+
355359
if (!protocol.equalsIgnoreCase(NetUtils.ICMP_PROTO) && (icmpCode != null || icmpType != null)) {
356360
throw new InvalidParameterValueException("Can specify icmpCode and icmpType for ICMP protocol only");
357361
}

‎server/src/test/java/com/cloud/network/firewall/FirewallManagerTest.java‎

Lines changed: 220 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,19 +19,23 @@
1919

2020
import com.cloud.exception.InvalidParameterValueException;
2121
import com.cloud.exception.NetworkRuleConflictException;
22+
import com.cloud.exception.PermissionDeniedException;
2223
import com.cloud.exception.ResourceUnavailableException;
24+
import com.cloud.network.IpAddress;
2325
import com.cloud.network.IpAddressManager;
2426
import com.cloud.network.Network;
2527
import com.cloud.network.Network.Capability;
2628
import com.cloud.network.Network.Service;
2729
import com.cloud.network.NetworkModel;
30+
import com.cloud.network.NetworkModelImpl;
2831
import com.cloud.network.NetworkRuleApplier;
2932
import com.cloud.network.dao.FirewallRulesDao;
3033
import com.cloud.network.dao.IPAddressDao;
3134
import com.cloud.network.dao.IPAddressVO;
3235
import com.cloud.network.dao.NetworkDao;
3336
import com.cloud.network.dao.NetworkVO;
3437
import com.cloud.network.element.FirewallServiceProvider;
38+
import com.cloud.network.element.NetworkElement;
3539
import com.cloud.network.element.VirtualRouterElement;
3640
import com.cloud.network.element.VpcVirtualRouterElement;
3741
import com.cloud.network.rules.FirewallRule;
@@ -43,8 +47,13 @@
4347
import com.cloud.user.Account;
4448
import com.cloud.user.AccountManager;
4549
import com.cloud.user.DomainManager;
50+
import com.cloud.user.User;
4651
import com.cloud.utils.component.ComponentContext;
52+
import com.cloud.utils.db.Transaction;
53+
import com.cloud.utils.db.TransactionCallbackWithException;
4754
import com.cloud.utils.exception.CloudRuntimeException;
55+
import com.cloud.utils.net.Ip;
56+
import org.apache.cloudstack.context.CallContext;
4857
import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService;
4958
import org.apache.cloudstack.network.RoutedIpv4Manager;
5059
import org.junit.After;
@@ -55,10 +64,12 @@
5564
import org.junit.runner.RunWith;
5665
import org.mockito.InjectMocks;
5766
import org.mockito.Mock;
67+
import org.mockito.MockedStatic;
5868
import org.mockito.Mockito;
5969
import org.mockito.MockitoAnnotations;
6070
import org.mockito.Spy;
6171
import org.mockito.junit.MockitoJUnitRunner;
72+
import org.springframework.test.util.ReflectionTestUtils;
6273

6374
import java.util.ArrayList;
6475
import java.util.Arrays;
@@ -605,6 +616,215 @@ public void testValidateFirewallRuleForVpcSucceedsWithSupportedProtocolAndTraffi
605616
verify(_accountMgr, times(1)).checkAccess(caller, null, true, ipAddress);
606617
}
607618

619+
@Test
620+
public void testCreateIngressFirewallRuleRejectsReservedVpcIpUnderLock() throws NetworkRuleConflictException {
621+
Account caller = mock(Account.class);
622+
IPAddressVO sourceIp = createVpcIpAddress();
623+
IPAddressVO lockedIp = createVpcIpAddress();
624+
when(_ipAddressDao.findById(1L)).thenReturn(sourceIp);
625+
when(_ipAddressDao.acquireInLockTable(1L)).thenReturn(lockedIp);
626+
configureVpcFirewallCapabilities();
627+
NetworkElement owningProvider = mock(NetworkElement.class);
628+
NetworkModelImpl networkModel = useProviderReservation(owningProvider);
629+
when(owningProvider.isIpAddressReserved(lockedIp)).thenReturn(true);
630+
631+
CallContext.register(mock(User.class), caller);
632+
try (MockedStatic<Transaction> transactions = Mockito.mockStatic(Transaction.class)) {
633+
InvalidParameterValueException exception = Assert.assertThrows(InvalidParameterValueException.class,
634+
() -> _firewallMgr.createIngressFirewallRule(createVpcIngressRule()));
635+
Assert.assertTrue(exception.getMessage().contains("reserved by a network provider"));
636+
transactions.verifyNoInteractions();
637+
} finally {
638+
CallContext.unregister();
639+
}
640+
641+
org.mockito.InOrder order = Mockito.inOrder(_ipAddressDao, _accountMgr, owningProvider);
642+
order.verify(_ipAddressDao).findById(1L);
643+
order.verify(_ipAddressDao).acquireInLockTable(1L);
644+
order.verify(_accountMgr).checkAccess(caller, null, true, lockedIp);
645+
order.verify(owningProvider).isIpAddressReserved(lockedIp);
646+
order.verify(_ipAddressDao).releaseFromLockTable(1L);
647+
verify(owningProvider, never()).isIpAddressReserved(sourceIp);
648+
verify(networkModel, never()).checkIpForService(any(), any(), any());
649+
Mockito.verifyNoInteractions(_firewallDao);
650+
}
651+
652+
@Test
653+
public void testCreateIngressFirewallRuleAllowsUnreservedVpcIpWithoutTierAssociation() throws NetworkRuleConflictException {
654+
Account caller = mock(Account.class);
655+
IPAddressVO ipAddress = createVpcIpAddress();
656+
Assert.assertNull(ipAddress.getAssociatedWithNetworkId());
657+
when(_ipAddressDao.findById(1L)).thenReturn(ipAddress);
658+
when(_ipAddressDao.acquireInLockTable(1L)).thenReturn(ipAddress);
659+
configureVpcFirewallCapabilities();
660+
NetworkElement owningProvider = mock(NetworkElement.class);
661+
NetworkModelImpl networkModel = useProviderReservation(owningProvider);
662+
List<FirewallRuleVO> persistedRules = mockFirewallRulePersistence();
663+
when(_firewallDao.listByIpAndPurposeAndNotRevoked(1L, null)).thenReturn(persistedRules);
664+
665+
FirewallRule result;
666+
CallContext.register(mock(User.class), caller);
667+
try (MockedStatic<Transaction> transactions = mockFirewallRuleTransactions()) {
668+
result = _firewallMgr.createIngressFirewallRule(createVpcIngressRule());
669+
Assert.assertEquals(result.getId(), CallContext.current().getContextParameter(FirewallRule.class));
670+
} finally {
671+
CallContext.unregister();
672+
}
673+
674+
Assert.assertSame(persistedRules.get(0), result);
675+
Assert.assertEquals(Long.valueOf(10L), result.getVpcId());
676+
Assert.assertNull(result.getNetworkId());
677+
Assert.assertEquals(Long.valueOf(1L), result.getSourceIpAddressId());
678+
Assert.assertEquals(3L, result.getAccountId());
679+
Assert.assertEquals(4L, result.getDomainId());
680+
Assert.assertEquals(FirewallRule.State.Add, result.getState());
681+
org.mockito.InOrder order = Mockito.inOrder(_ipAddressDao, _accountMgr, owningProvider, _firewallDao);
682+
order.verify(_ipAddressDao).acquireInLockTable(1L);
683+
order.verify(_accountMgr).checkAccess(caller, null, true, ipAddress);
684+
order.verify(owningProvider).isIpAddressReserved(ipAddress);
685+
order.verify(_firewallDao).persist((FirewallRuleVO) result);
686+
order.verify(_firewallDao).setStateToAdd((FirewallRuleVO) result);
687+
order.verify(_ipAddressDao).releaseFromLockTable(1L);
688+
verify(networkModel, never()).checkIpForService(any(), any(), any());
689+
}
690+
691+
@Test
692+
public void testCreateIngressFirewallRuleChecksVpcIpAccessBeforeReservation() throws NetworkRuleConflictException {
693+
Account caller = mock(Account.class);
694+
IPAddressVO ipAddress = createVpcIpAddress();
695+
when(_ipAddressDao.findById(1L)).thenReturn(ipAddress);
696+
when(_ipAddressDao.acquireInLockTable(1L)).thenReturn(ipAddress);
697+
PermissionDeniedException denied = new PermissionDeniedException("No access to this IP address");
698+
Mockito.doThrow(denied).when(_accountMgr).checkAccess(caller, null, true, ipAddress);
699+
700+
CallContext.register(mock(User.class), caller);
701+
try (MockedStatic<Transaction> transactions = Mockito.mockStatic(Transaction.class)) {
702+
Assert.assertSame(denied, Assert.assertThrows(PermissionDeniedException.class,
703+
() -> _firewallMgr.createIngressFirewallRule(createVpcIngressRule())));
704+
transactions.verifyNoInteractions();
705+
} finally {
706+
CallContext.unregister();
707+
}
708+
709+
org.mockito.InOrder order = Mockito.inOrder(_ipAddressDao, _accountMgr);
710+
order.verify(_ipAddressDao).acquireInLockTable(1L);
711+
order.verify(_accountMgr).checkAccess(caller, null, true, ipAddress);
712+
order.verify(_ipAddressDao).releaseFromLockTable(1L);
713+
Mockito.verifyNoInteractions(_networkModel, _vpcMgr, _firewallDao);
714+
}
715+
716+
@Test
717+
public void testCreateIngressFirewallRuleCannotUseVpcIpWithoutLock() throws NetworkRuleConflictException {
718+
Account caller = mock(Account.class);
719+
when(_ipAddressDao.findById(1L)).thenReturn(createVpcIpAddress());
720+
721+
CallContext.register(mock(User.class), caller);
722+
try (MockedStatic<Transaction> transactions = Mockito.mockStatic(Transaction.class)) {
723+
InvalidParameterValueException exception = Assert.assertThrows(InvalidParameterValueException.class,
724+
() -> _firewallMgr.createIngressFirewallRule(createVpcIngressRule()));
725+
Assert.assertTrue(exception.getMessage().contains("couldn't locate IP address"));
726+
transactions.verifyNoInteractions();
727+
} finally {
728+
CallContext.unregister();
729+
}
730+
731+
org.mockito.InOrder order = Mockito.inOrder(_ipAddressDao);
732+
order.verify(_ipAddressDao).acquireInLockTable(1L);
733+
order.verify(_ipAddressDao).releaseFromLockTable(1L);
734+
Mockito.verifyNoInteractions(_accountMgr, _networkModel, _vpcMgr, _firewallDao);
735+
}
736+
737+
@Test
738+
public void testCreateFirewallRuleAllowsVpcSystemRuleWithoutIp() throws NetworkRuleConflictException {
739+
Account caller = mock(Account.class);
740+
Vpc vpc = mock(Vpc.class);
741+
when(_vpcMgr.getActiveVpc(10L)).thenReturn(vpc);
742+
when(vpc.getAccountId()).thenReturn(3L);
743+
when(vpc.getDomainId()).thenReturn(4L);
744+
List<FirewallRuleVO> persistedRules = mockFirewallRulePersistence();
745+
746+
FirewallRule result;
747+
CallContext.register(mock(User.class), caller);
748+
try (MockedStatic<Transaction> transactions = mockFirewallRuleTransactions()) {
749+
result = _firewallMgr.createFirewallRule(null, caller, "xid", 80, 80, "tcp",
750+
Collections.singletonList("0.0.0.0/0"), null, null, null, null,
751+
FirewallRuleType.System, null, 10L, FirewallRule.TrafficType.Egress, true);
752+
} finally {
753+
CallContext.unregister();
754+
}
755+
756+
Assert.assertSame(persistedRules.get(0), result);
757+
Assert.assertEquals(Long.valueOf(10L), result.getVpcId());
758+
Assert.assertNull(result.getNetworkId());
759+
Assert.assertNull(result.getSourceIpAddressId());
760+
Assert.assertEquals(3L, result.getAccountId());
761+
Assert.assertEquals(4L, result.getDomainId());
762+
Assert.assertEquals(FirewallRuleType.System, result.getType());
763+
Assert.assertEquals(FirewallRule.State.Add, result.getState());
764+
verify(_firewallDao).setStateToAdd((FirewallRuleVO) result);
765+
Mockito.verifyNoInteractions(_ipAddressDao, _accountMgr, _networkModel);
766+
}
767+
768+
private IPAddressVO createVpcIpAddress() {
769+
IPAddressVO ipAddress = new IPAddressVO(new Ip("198.51.100.10"), 1L, 0L, 1L, false);
770+
ReflectionTestUtils.setField(ipAddress, "id", 1L);
771+
ipAddress.setState(IpAddress.State.Allocated);
772+
ipAddress.setVpcId(10L);
773+
ipAddress.setAllocatedToAccountId(3L);
774+
ipAddress.setAllocatedInDomainId(4L);
775+
return ipAddress;
776+
}
777+
778+
private FirewallRule createVpcIngressRule() {
779+
return new FirewallRuleVO("xid", 1L, 80, 80, "tcp", null, 3L, 4L, Purpose.Firewall,
780+
Collections.singletonList("0.0.0.0/0"), null, null, null, FirewallRule.TrafficType.Ingress);
781+
}
782+
783+
private void configureVpcFirewallCapabilities() {
784+
FirewallServiceProvider firewallProvider = mock(FirewallServiceProvider.class);
785+
Map<Capability, String> firewallCaps = new HashMap<>();
786+
firewallCaps.put(Capability.SupportedProtocols, "tcp,udp,icmp");
787+
firewallCaps.put(Capability.SupportedTrafficDirection, "ingress,egress");
788+
when(_vpcMgr.getActiveVpc(10L)).thenReturn(mock(Vpc.class));
789+
when(firewallProvider.getProvider()).thenReturn(Network.Provider.VPCVirtualRouter);
790+
when(firewallProvider.getCapabilities()).thenReturn(Collections.singletonMap(Service.Firewall, firewallCaps));
791+
when(_vpcMgr.isProviderSupportServiceInVpc(10L, Service.Firewall, Network.Provider.VPCVirtualRouter)).thenReturn(true);
792+
_firewallMgr._firewallElements = Collections.singletonList(firewallProvider);
793+
}
794+
795+
private NetworkModelImpl useProviderReservation(NetworkElement owningProvider) {
796+
NetworkModelImpl networkModel = spy(new NetworkModelImpl());
797+
networkModel.setNetworkElements(Collections.singletonList(owningProvider));
798+
_firewallMgr._networkModel = networkModel;
799+
return networkModel;
800+
}
801+
802+
private List<FirewallRuleVO> mockFirewallRulePersistence() {
803+
List<FirewallRuleVO> persistedRules = new ArrayList<>();
804+
when(_firewallDao.persist(any(FirewallRuleVO.class))).thenAnswer(invocation -> {
805+
FirewallRuleVO rule = invocation.getArgument(0);
806+
ReflectionTestUtils.setField(rule, "id", 100L);
807+
persistedRules.add(rule);
808+
return rule;
809+
});
810+
when(_firewallDao.setStateToAdd(any(FirewallRuleVO.class))).thenAnswer(invocation -> {
811+
FirewallRuleVO rule = invocation.getArgument(0);
812+
rule.setState(FirewallRule.State.Add);
813+
return true;
814+
});
815+
return persistedRules;
816+
}
817+
818+
private MockedStatic<Transaction> mockFirewallRuleTransactions() {
819+
MockedStatic<Transaction> transactions = Mockito.mockStatic(Transaction.class);
820+
transactions.when(() -> Transaction.execute(Mockito.<TransactionCallbackWithException<FirewallRuleVO, NetworkRuleConflictException>>any()))
821+
.thenAnswer(invocation -> {
822+
TransactionCallbackWithException<FirewallRuleVO, NetworkRuleConflictException> callback = invocation.getArgument(0);
823+
return callback.doInTransaction(null);
824+
});
825+
return transactions;
826+
}
827+
608828
@Test
609829
public void testCreateFirewallRuleRoutesToVpcWhenVpcIdProvided() throws NetworkRuleConflictException {
610830
Account caller = Mockito.mock(Account.class);

0 commit comments

Comments
 (0)