From 9f3d4f92b857e2817228b0477a4f5260f2c74aa2 Mon Sep 17 00:00:00 2001 From: Daniil Kirilyuk Date: Mon, 30 Sep 2024 10:01:10 +0200 Subject: [PATCH] QPID-8677 - [Broker-J] IllegalConfigurationException when deleting misconfigured port (#247) --- .../qpid/server/model/port/AbstractPort.java | 49 ++++---- .../server/model/port/HttpPortImplTest.java | 118 +++++++++++++++++- 2 files changed, 136 insertions(+), 31 deletions(-) diff --git a/broker-core/src/main/java/org/apache/qpid/server/model/port/AbstractPort.java b/broker-core/src/main/java/org/apache/qpid/server/model/port/AbstractPort.java index 1e23580a1d..2e1ec3f3b2 100644 --- a/broker-core/src/main/java/org/apache/qpid/server/model/port/AbstractPort.java +++ b/broker-core/src/main/java/org/apache/qpid/server/model/port/AbstractPort.java @@ -279,25 +279,22 @@ protected void validateChange(final ConfiguredObject proxyForValidation, fina super.validateChange(proxyForValidation, changedAttributes); Port updated = (Port)proxyForValidation; - - if(!getName().equals(updated.getName())) + if (!getName().equals(updated.getName())) { throw new IllegalConfigurationException("Changing the port name is not allowed"); } - if(changedAttributes.contains(PORT)) + if (changedAttributes.contains(PORT)) { int newPort = updated.getPort(); if (getPort() != newPort && newPort != 0) { - for (Port p : _container.getChildren(Port.class)) + for (Port p : _container.getChildren(Port.class)) { if (p.getBoundPort() == newPort || p.getPort() == newPort) { - throw new IllegalConfigurationException("Port number " - + newPort - + " is already in use by port " - + p.getName()); + throw new IllegalConfigurationException("Port number " + newPort + + " is already in use by port " + p.getName()); } } } @@ -310,40 +307,42 @@ protected void validateChange(final ConfiguredObject proxyForValidation, fina boolean usesSsl = isUsingTLSTransport(transports); - if (usesSsl) + if (usesSsl && changedAttributes.contains(KEY_STORE) && updated.getKeyStore() == null) { - if (updated.getKeyStore() == null) - { - throw new IllegalConfigurationException("Can't create port which requires SSL but has no key store configured."); - } + throw new IllegalConfigurationException("Can't create port which requires SSL but has no key store configured."); } - if(changedAttributes.contains(Port.AUTHENTICATION_PROVIDER) || changedAttributes.contains(Port.TRANSPORTS)) + if (changedAttributes.contains(Port.AUTHENTICATION_PROVIDER) || changedAttributes.contains(Port.TRANSPORTS)) { validateAuthenticationMechanisms(updated.getAuthenticationProvider(), updated.getTransports()); } boolean requiresCertificate = updated.getNeedClientAuth() || updated.getWantClientAuth(); - if (usesSsl) + if (changedAttributes.contains(TRANSPORTS) || changedAttributes.contains(TRUST_STORES) || + changedAttributes.contains(NEED_CLIENT_AUTH) || changedAttributes.contains(WANT_CLIENT_AUTH)) { - if ((updated.getTrustStores() == null || updated.getTrustStores().isEmpty() ) && requiresCertificate) + if (usesSsl) { - throw new IllegalConfigurationException("Can't create port which requests SSL client certificates but has no trust store configured."); + if ((updated.getTrustStores() == null || updated.getTrustStores().isEmpty()) && requiresCertificate) + { + throw new IllegalConfigurationException( + "Can't create port which requests SSL client certificates but has no trust store configured."); + } } - } - else - { - if (requiresCertificate) + else { - throw new IllegalConfigurationException("Can't create port which requests SSL client certificates but doesn't use SSL transport."); + if (requiresCertificate) + { + throw new IllegalConfigurationException( + "Can't create port which requests SSL client certificates but doesn't use SSL transport."); + } } } - - if(requiresCertificate && updated.getClientCertRecorder() != null) + if (requiresCertificate && updated.getClientCertRecorder() != null) { - if(!(updated.getClientCertRecorder() instanceof ManagedPeerCertificateTrustStore)) + if (!(updated.getClientCertRecorder() instanceof ManagedPeerCertificateTrustStore)) { throw new IllegalConfigurationException("Only trust stores of type " + ManagedPeerCertificateTrustStore.TYPE_NAME + " may be used as the client certificate recorder"); } diff --git a/broker-core/src/test/java/org/apache/qpid/server/model/port/HttpPortImplTest.java b/broker-core/src/test/java/org/apache/qpid/server/model/port/HttpPortImplTest.java index 4d630370de..2a51d062c3 100644 --- a/broker-core/src/test/java/org/apache/qpid/server/model/port/HttpPortImplTest.java +++ b/broker-core/src/test/java/org/apache/qpid/server/model/port/HttpPortImplTest.java @@ -20,11 +20,13 @@ package org.apache.qpid.server.model.port; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -38,24 +40,31 @@ import org.apache.qpid.server.logging.EventLogger; import org.apache.qpid.server.model.AuthenticationProvider; import org.apache.qpid.server.model.Broker; +import org.apache.qpid.server.model.BrokerImpl; import org.apache.qpid.server.model.BrokerModel; +import org.apache.qpid.server.model.JsonSystemConfigImpl; +import org.apache.qpid.server.model.KeyStore; import org.apache.qpid.server.model.Model; +import org.apache.qpid.server.model.State; import org.apache.qpid.server.model.SystemConfig; +import org.apache.qpid.server.model.TrustStore; import org.apache.qpid.test.utils.UnitTestBase; @SuppressWarnings({"rawtypes", "unchecked"}) -public class HttpPortImplTest extends UnitTestBase +class HttpPortImplTest extends UnitTestBase { private static final String AUTHENTICATION_PROVIDER_NAME = "test"; private Broker _broker; @BeforeEach - public void setUp() throws Exception + public void setUp() { final TaskExecutor taskExecutor = CurrentThreadTaskExecutor.newStartedInstance(); final Model model = BrokerModel.getInstance(); final SystemConfig systemConfig = mock(SystemConfig.class); + when(systemConfig.getCategoryClass()).thenReturn(SystemConfig.class); + when(systemConfig.getTypeClass()).thenReturn(JsonSystemConfigImpl.class); _broker = mock(Broker.class); when(_broker.getParent()).thenReturn(systemConfig); when(_broker.getTaskExecutor()).thenReturn(taskExecutor); @@ -63,6 +72,7 @@ public void setUp() throws Exception when(_broker.getModel()).thenReturn(model); when(_broker.getEventLogger()).thenReturn(new EventLogger()); when(_broker.getCategoryClass()).thenReturn(Broker.class); + when(_broker.getTypeClass()).thenReturn(BrokerImpl.class); final AuthenticationProvider provider = mock(AuthenticationProvider.class); when(provider.getName()).thenReturn(AUTHENTICATION_PROVIDER_NAME); @@ -74,7 +84,7 @@ public void setUp() throws Exception } @Test - public void testCreateWithIllegalThreadPoolValues() + void testCreateWithIllegalThreadPoolValues() { final int threadPoolMinimumSize = 37; final int invalidThreadPoolMaximumSize = threadPoolMinimumSize - 1; @@ -88,7 +98,7 @@ HttpPort.NAME, getTestName(), } @Test - public void testIllegalChangeWithMaxThreadPoolSizeSmallerThanMinThreadPoolSize() + void testIllegalChangeWithMaxThreadPoolSizeSmallerThanMinThreadPoolSize() { final int threadPoolMinimumSize = 37; final int invalidThreadPoolMaximumSize = threadPoolMinimumSize - 1; @@ -105,7 +115,7 @@ HttpPort.NAME, getTestName(), } @Test - public void testIllegalChangeWithNegativeThreadPoolSize() + void testIllegalChangeWithNegativeThreadPoolSize() { final int illegalThreadPoolMinimumSize = -1; final int threadPoolMaximumSize = 1; @@ -122,7 +132,7 @@ HttpPort.NAME, getTestName(), } @Test - public void testChangeWithLegalThreadPoolValues() + void testChangeWithLegalThreadPoolValues() { final int threadPoolMinimumSize = 37; final int threadPoolMaximumSize = threadPoolMinimumSize + 1; @@ -141,4 +151,100 @@ HttpPort.NAME, getTestName(), assertEquals(port.getThreadPoolMaximum(), (long) threadPoolMaximumSize, "Port did not pickup changes to maximum thread pool size"); } + + @Test + void deletePort() + { + final Map attributes = Map.of(HttpPort.PORT, 0, + HttpPort.NAME, getTestName(), + HttpPort.AUTHENTICATION_PROVIDER, AUTHENTICATION_PROVIDER_NAME, + HttpPort.WANT_CLIENT_AUTH, true, + HttpPort.NEED_CLIENT_AUTH, true, + HttpPort.PROTOCOLS, List.of("HTTP"), + HttpPort.TRANSPORTS, List.of("SSL"), + HttpPort.KEY_STORE, mock(KeyStore.class), + HttpPort.TRUST_STORES, List.of(mock(TrustStore.class))); + + final HttpPortImpl port = new HttpPortImpl(attributes, _broker); + + assertEquals(State.UNINITIALIZED, port.getState()); + + port.create(); + + assertEquals(State.QUIESCED, port.getState()); + + port.delete(); + + assertEquals(State.DELETED, port.getState()); + } + + /** Checks the deletion of an invalid port */ + @Test + void deleteUninitializedInvalidPort() + { + final Map attributes = Map.of(HttpPort.PORT, 0, + HttpPort.NAME, getTestName(), + HttpPort.AUTHENTICATION_PROVIDER, AUTHENTICATION_PROVIDER_NAME, + HttpPort.WANT_CLIENT_AUTH, true, + HttpPort.NEED_CLIENT_AUTH, true, + HttpPort.PROTOCOLS, List.of("HTTP"), + HttpPort.TRANSPORTS, List.of("SSL"), + HttpPort.KEY_STORE, mock(KeyStore.class), + HttpPort.TRUST_STORES, List.of()); + + final HttpPortImpl port = new HttpPortImpl(attributes, _broker); + + assertEquals(State.UNINITIALIZED, port.getState()); + + port.delete(); + + assertEquals(State.DELETED, port.getState()); + } + + /** Checks the update of an invalid port */ + @Test + void updateUninitializedInvalidPort() + { + final Map attributes = Map.of(HttpPort.PORT, 0, + HttpPort.NAME, getTestName(), + HttpPort.AUTHENTICATION_PROVIDER, AUTHENTICATION_PROVIDER_NAME, + HttpPort.WANT_CLIENT_AUTH, true, + HttpPort.NEED_CLIENT_AUTH, true, + HttpPort.PROTOCOLS, List.of("HTTP"), + HttpPort.TRANSPORTS, List.of("SSL"), + HttpPort.KEY_STORE, mock(KeyStore.class), + HttpPort.TRUST_STORES, List.of()); + + final HttpPortImpl port = new HttpPortImpl(attributes, _broker); + + assertEquals(State.UNINITIALIZED, port.getState()); + + final Map args1 = Map.of(HttpPort.NAME, "changed"); + IllegalConfigurationException exception = assertThrows(IllegalConfigurationException.class, () -> port.setAttributes(args1)); + assertEquals("Changing the port name is not allowed", exception.getMessage()); + + final Map args2 = Map.of(HttpPort.WANT_CLIENT_AUTH, false); + exception = assertThrows(IllegalConfigurationException.class, () -> port.setAttributes(args2)); + assertEquals("Can't create port which requests SSL client certificates but has no trust store configured.", + exception.getMessage()); + + final Map args3 = Map.of(HttpPort.NEED_CLIENT_AUTH, false); + exception = assertThrows(IllegalConfigurationException.class, () -> port.setAttributes(args3)); + assertEquals("Can't create port which requests SSL client certificates but has no trust store configured.", + exception.getMessage()); + + final Map args4 = Map.of(HttpPort.TRANSPORTS, List.of("TCP")); + exception = assertThrows(IllegalConfigurationException.class, () -> port.setAttributes(args4)); + assertEquals("Can't create port which requests SSL client certificates but doesn't use SSL transport.", + exception.getMessage()); + + final Map args5 = new HashMap<>(); + args5.put(HttpPort.KEY_STORE, null); + exception = assertThrows(IllegalConfigurationException.class, () -> port.setAttributes(args5)); + assertEquals("Can't create port which requires SSL but has no key store configured.", + exception.getMessage()); + + final Map args6 = Map.of(HttpPort.TRUST_STORES, List.of(mock(TrustStore.class))); + assertDoesNotThrow(() -> port.setAttributes(args6)); + } }