QPID-4657, QPID-4683: review changes for new port configuration functionality

- Fix ability to select SSL for a port
- Add ability to set SSL Client Auth attributes for a port
- Enforce that you have SSL keystores/trustures in place when creating new ports that will fail to work without them
- Update names and placeholder text in UI to convey what happens when you dont fill out an optional attribute.
- Remove the default AMQP port value in form, makes the user specify port and avoid near definite clash.
- Removed requirement to specify Transport since it is actually optional.
- Ensure the port state is set accurately for newly added ports
- Fix the ability to override the management ports in ManagementMode
- Allow editing the management ports in Management Mode without having to override them first.


git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk@1463060 13f79535-47bb-0310-9956-ffa450edef68

5 files changed, 174 insertions, 15 deletions

diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/model/Broker.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/model/Broker.java
index 56f92ebb6a..8f64cab2ec 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/model/Broker.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/model/Broker.java
@@ -215,4 +215,6 @@ public interface Broker extends ConfiguredObject
     TrustStore getDefaultTrustStore();
 
     TaskExecutor getTaskExecutor();
+
+    boolean isManagementMode();
 }
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java
index 97acac668b..9759718ddd 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java
@@ -58,6 +58,7 @@ import org.apache.qpid.server.model.Statistics;
 import org.apache.qpid.server.model.TrustStore;
 import org.apache.qpid.server.model.UUIDGenerator;
 import org.apache.qpid.server.model.VirtualHost;
+import org.apache.qpid.server.configuration.store.ManagementModeStoreHandler;
 import org.apache.qpid.server.configuration.updater.TaskExecutor;
 import org.apache.qpid.server.security.access.Operation;
 import org.apache.qpid.server.security.group.FileGroupManager;
@@ -195,6 +196,8 @@ public class BrokerAdapter extends AbstractAdapter implements Broker, Configurat
     private final Collection<String> _supportedStoreTypes;
     private final ConfigurationEntryStore _brokerStore;
 
+    private boolean _managementMode;
+
     public BrokerAdapter(UUID id, Map<String, Object> attributes, StatisticsGatherer statisticsGatherer, VirtualHostRegistry virtualHostRegistry,
             LogRecorder logRecorder, RootMessageLogger rootMessageLogger, AuthenticationProviderFactory authenticationProviderFactory,
             PortFactory portFactory, TaskExecutor taskExecutor, ConfigurationEntryStore brokerStore)
@@ -214,6 +217,7 @@ public class BrokerAdapter extends AbstractAdapter implements Broker, Configurat
         createBrokerChildrenFromAttributes();
         _supportedStoreTypes = new MessageStoreCreator().getStoreTypes();
         _brokerStore = brokerStore;
+        _managementMode =  brokerStore instanceof ManagementModeStoreHandler;
     }
 
     /*
@@ -542,11 +546,20 @@ public class BrokerAdapter extends AbstractAdapter implements Broker, Configurat
         port.addChangeListener(this);
     }
 
+    /**
+     * Called when adding a new port via the management interface
+     */
     private Port createPort(Map<String, Object> attributes)
     {
         Port port = _portFactory.createPort(UUID.randomUUID(), this, attributes);
         addPort(port);
-        port.setDesiredState(State.INITIALISING, State.ACTIVE);
+
+        //AMQP ports are disable during ManagementMode, and the management
+        //plugins can currently only start ports at broker startup and
+        //not when they are newly created via the management interfaces.
+        boolean quiesce = isManagementMode() || !(port instanceof AmqpPortAdapter);
+        port.setDesiredState(State.INITIALISING, quiesce ? State.QUIESCED : State.ACTIVE);
+
         return port;
     }
 
@@ -1237,4 +1250,10 @@ public class BrokerAdapter extends AbstractAdapter implements Broker, Configurat
             throw new AccessControlException("Setting of broker attributes is denied");
         }
     }
+
+    @Override
+    public boolean isManagementMode()
+    {
+        return _managementMode;
+    }
 }
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/PortAdapter.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/PortAdapter.java
index a37c2dceb7..ba10816a35 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/PortAdapter.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/PortAdapter.java
@@ -312,6 +312,13 @@ public class PortAdapter extends AbstractAdapter implements Port
                 throw new IllegalStateException("Cannot activate port in " + state + " state");
             }
         }
+        else if (desiredState == State.QUIESCED)
+        {
+            if (state == State.INITIALISING && _state.compareAndSet(state, State.QUIESCED))
+            {
+                return true;
+            }
+        }
         else if (desiredState == State.STOPPED)
         {
             if (_state.compareAndSet(state, State.STOPPED))
@@ -351,9 +358,9 @@ public class PortAdapter extends AbstractAdapter implements Port
     @Override
     protected void changeAttributes(Map<String, Object> attributes)
     {
-        if (getActualState() == State.ACTIVE)
+        if (getActualState() == State.ACTIVE && !_broker.isManagementMode())
         {
-            throw new IllegalStateException("Cannot change attributes for an active port");
+            throw new IllegalStateException("Cannot change attributes for an active port outside of Management Mode");
         }
         super.changeAttributes(MapValueConverter.convert(attributes, ATTRIBUTE_TYPES));
     }
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/PortFactory.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/PortFactory.java
index 1872f3e2ee..50bb2e2fcb 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/PortFactory.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/PortFactory.java
@@ -104,6 +104,18 @@ public class PortFactory
             defaults.put(Port.RECEIVE_BUFFER_SIZE, DEFAULT_AMQP_RECEIVE_BUFFER_SIZE);
             defaults.put(Port.SEND_BUFFER_SIZE, DEFAULT_AMQP_SEND_BUFFER_SIZE);
             port = new AmqpPortAdapter(id, broker, attributes, defaults, broker.getTaskExecutor());
+
+            boolean useClientAuth = (Boolean) port.getAttribute(Port.NEED_CLIENT_AUTH) || (Boolean) port.getAttribute(Port.WANT_CLIENT_AUTH);
+            if(useClientAuth && broker.getTrustStores().isEmpty())
+            {
+                throw new IllegalConfigurationException("Cant create port which requests SSL client certificates as the broker has no trust/peer stores configured.");
+            }
+
+            boolean doesntUseSSL = port.getTransports().isEmpty() || !port.getTransports().contains(Transport.SSL);
+            if(useClientAuth && doesntUseSSL)
+            {
+                throw new IllegalConfigurationException("Cant create port which requests SSL client certificates but doesnt use SSL transport.");
+            }
         }
         else
         {
@@ -112,18 +124,35 @@ public class PortFactory
                 throw new IllegalConfigurationException("Only one protocol can be used on non AMQP port");
             }
             Protocol protocol = protocols.iterator().next();
-            Collection<Port> existingPorts = broker.getPorts();
-            for (Port existingPort : existingPorts)
+
+            if(!broker.isManagementMode())
             {
-                Collection<Protocol> portProtocols = existingPort.getProtocols();
-                if (portProtocols != null && portProtocols.contains(protocol))
+                //ManagementMode needs this relaxed to allow its overriding management ports to be inserted.
+
+                //Enforce only a single port of each management protocol, as the plugins will only use one.
+                Collection<Port> existingPorts = broker.getPorts();
+                for (Port existingPort : existingPorts)
                 {
-                    throw new IllegalConfigurationException("Port for protocol " + protocol + " already exist. Only one management port per protocol can be created");
+                    Collection<Protocol> portProtocols = existingPort.getProtocols();
+                    if (portProtocols != null && portProtocols.contains(protocol))
+                    {
+                        throw new IllegalConfigurationException("Port for protocol " + protocol + " already exist. Only one management port per protocol can be created");
+                    }
                 }
             }
+
             defaults.put(Port.NAME, portValue + "-" + protocol.name());
             port = new PortAdapter(id, broker, attributes, defaults, broker.getTaskExecutor());
         }
+
+        if(port.getTransports().contains(Transport.SSL) || port.getProtocols().contains(Protocol.HTTPS))
+        {
+            if(broker.getKeyStores().isEmpty())
+            {
+                throw new IllegalConfigurationException("Cant create port which requires SSL as the broker has no keystore configured.");
+            }
+        }
+
         return port;
     }
 
diff --git a/qpid/java/broker/src/test/java/org/apache/qpid/server/model/adapter/PortFactoryTest.java b/qpid/java/broker/src/test/java/org/apache/qpid/server/model/adapter/PortFactoryTest.java
index 0c496dfd9b..5d9cfea709 100644
--- a/qpid/java/broker/src/test/java/org/apache/qpid/server/model/adapter/PortFactoryTest.java
+++ b/qpid/java/broker/src/test/java/org/apache/qpid/server/model/adapter/PortFactoryTest.java
@@ -23,6 +23,7 @@ package org.apache.qpid.server.model.adapter;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
@@ -36,21 +37,30 @@ import java.util.UUID;
 import org.apache.qpid.server.configuration.BrokerProperties;
 import org.apache.qpid.server.configuration.IllegalConfigurationException;
 import org.apache.qpid.server.model.Broker;
+import org.apache.qpid.server.model.KeyStore;
 import org.apache.qpid.server.model.Port;
 import org.apache.qpid.server.model.Protocol;
 import org.apache.qpid.server.model.Transport;
+import org.apache.qpid.server.model.TrustStore;
 import org.apache.qpid.test.utils.QpidTestCase;
 
 public class PortFactoryTest extends QpidTestCase
 {
     private UUID _portId = UUID.randomUUID();
+    private UUID _keyStoreId = UUID.randomUUID();
+    private UUID _trustStoreId = UUID.randomUUID();
     private int _portNumber = 123;
-    private Set<String> _tcpStringSet = Collections.singleton(Transport.SSL.name());
-    private Set<Transport> _tcpTransportSet = Collections.singleton(Transport.SSL);
+    private Set<String> _tcpStringSet = Collections.singleton(Transport.TCP.name());
+    private Set<Transport> _tcpTransportSet = Collections.singleton(Transport.TCP);
+    private Set<String> _sslStringSet = Collections.singleton(Transport.SSL.name());
+    private Set<Transport> _sslTransportSet = Collections.singleton(Transport.SSL);
 
     private Map<String, Object> _attributes = new HashMap<String, Object>();
 
     private Broker _broker = mock(Broker.class);
+    private KeyStore _keyStore = mock(KeyStore.class);
+    private TrustStore _trustStore = mock(TrustStore.class);
+
     private PortFactory _portFactory;
 
     @Override
@@ -66,8 +76,6 @@ public class PortFactoryTest extends QpidTestCase
         _attributes.put(Port.TCP_NO_DELAY, "true");
         _attributes.put(Port.RECEIVE_BUFFER_SIZE, "1");
         _attributes.put(Port.SEND_BUFFER_SIZE, "2");
-        _attributes.put(Port.NEED_CLIENT_AUTH, "true");
-        _attributes.put(Port.WANT_CLIENT_AUTH, "true");
         _attributes.put(Port.BINDING_ADDRESS, "127.0.0.1");
     }
 
@@ -126,22 +134,116 @@ public class PortFactoryTest extends QpidTestCase
 
     public void testCreateAmqpPort()
     {
+        createAmqpPortTestImpl(false,false,false);
+    }
+
+    public void testCreateAmqpPortUsingSslFailsWithoutKeyStore()
+    {
+        when(_broker.getKeyStores()).thenReturn(new ArrayList<KeyStore>());
+        try
+        {
+            createAmqpPortTestImpl(true,false,false);
+            fail("expected exception due to lack of SSL keystore");
+        }
+        catch(IllegalConfigurationException e)
+        {
+            //expected
+        }
+    }
+
+    public void testCreateAmqpPortUsingSsslSucceedsWithKeyStore()
+    {
+        when(_broker.getKeyStores()).thenReturn(Collections.singleton(_keyStore));
+
+        createAmqpPortTestImpl(true,false,false);
+    }
+
+    public void testCreateAmqpPortNeedingClientAuthFailsWithoutTrustStore()
+    {
+        when(_broker.getKeyStores()).thenReturn(Collections.singleton(_keyStore));
+        when(_broker.getTrustStores()).thenReturn(new ArrayList<TrustStore>());
+        try
+        {
+            createAmqpPortTestImpl(true,true,false);
+            fail("expected exception due to lack of SSL truststore");
+        }
+        catch(IllegalConfigurationException e)
+        {
+            //expected
+        }
+    }
+
+    public void testCreateAmqpPortNeedingClientAuthSucceedsWithTrustStore()
+    {
+        when(_broker.getKeyStores()).thenReturn(Collections.singleton(_keyStore));
+        when(_broker.getTrustStores()).thenReturn(Collections.singleton(_trustStore));
+
+        createAmqpPortTestImpl(true,true,false);
+    }
+
+    public void testCreateAmqpPortWantingClientAuthFailsWithoutTrustStore()
+    {
+        when(_broker.getKeyStores()).thenReturn(Collections.singleton(_keyStore));
+        when(_broker.getTrustStores()).thenReturn(new ArrayList<TrustStore>());
+        try
+        {
+            createAmqpPortTestImpl(true,false,true);
+            fail("expected exception due to lack of SSL truststore");
+        }
+        catch(IllegalConfigurationException e)
+        {
+            //expected
+        }
+    }
+
+    public void testCreateAmqpPortWantingClientAuthSucceedsWithTrustStore()
+    {
+        when(_broker.getKeyStores()).thenReturn(Collections.singleton(_keyStore));
+        when(_broker.getTrustStores()).thenReturn(Collections.singleton(_trustStore));
+
+        createAmqpPortTestImpl(true,false,true);
+    }
+
+    public void createAmqpPortTestImpl(boolean useSslTransport, boolean needClientAuth, boolean wantClientAuth)
+    {
         Set<Protocol> amqp010ProtocolSet = Collections.singleton(Protocol.AMQP_0_10);
         Set<String> amqp010StringSet = Collections.singleton(Protocol.AMQP_0_10.name());
         _attributes.put(Port.PROTOCOLS, amqp010StringSet);
 
+        if(useSslTransport)
+        {
+            _attributes.put(Port.TRANSPORTS, _sslStringSet);
+        }
+
+        if(needClientAuth)
+        {
+            _attributes.put(Port.NEED_CLIENT_AUTH, "true");
+        }
+
+        if(wantClientAuth)
+        {
+            _attributes.put(Port.WANT_CLIENT_AUTH, "true");
+        }
+
         Port port = _portFactory.createPort(_portId, _broker, _attributes);
 
         assertNotNull(port);
         assertTrue(port instanceof AmqpPortAdapter);
         assertEquals(_portId, port.getId());
         assertEquals(_portNumber, port.getPort());
-        assertEquals(_tcpTransportSet, port.getTransports());
+        if(useSslTransport)
+        {
+            assertEquals(_sslTransportSet, port.getTransports());
+        }
+        else
+        {
+            assertEquals(_tcpTransportSet, port.getTransports());
+        }
         assertEquals(amqp010ProtocolSet, port.getProtocols());
         assertEquals("Unexpected send buffer size", 2, port.getAttribute(Port.SEND_BUFFER_SIZE));
         assertEquals("Unexpected receive buffer size", 1, port.getAttribute(Port.RECEIVE_BUFFER_SIZE));
-        assertEquals("Unexpected need client auth", true, port.getAttribute(Port.NEED_CLIENT_AUTH));
-        assertEquals("Unexpected want client auth", true, port.getAttribute(Port.WANT_CLIENT_AUTH));
+        assertEquals("Unexpected need client auth", needClientAuth, port.getAttribute(Port.NEED_CLIENT_AUTH));
+        assertEquals("Unexpected want client auth", wantClientAuth, port.getAttribute(Port.WANT_CLIENT_AUTH));
         assertEquals("Unexpected tcp no delay", true, port.getAttribute(Port.TCP_NO_DELAY));
         assertEquals("Unexpected binding", "127.0.0.1", port.getAttribute(Port.BINDING_ADDRESS));
     }