[GitHub] [qpid-broker-j] alex-rufous commented on a change in pull request #81: QPID-8504:[Broker-J]Change encryption to GCM

[GitBox <gi...@apache.org>]

alex-rufous commented on a change in pull request #81:
URL: https://github.com/apache/qpid-broker-j/pull/81#discussion_r575227365



##########
File path: broker-core/src/main/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypter.java
##########
@@ -0,0 +1,118 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.security.InvalidAlgorithmParameterException;
+import java.security.InvalidKeyException;
+import java.security.NoSuchAlgorithmException;
+import java.security.SecureRandom;
+import java.util.Base64;
+
+import javax.crypto.Cipher;
+import javax.crypto.CipherInputStream;
+import javax.crypto.NoSuchPaddingException;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.GCMParameterSpec;
+import javax.crypto.spec.IvParameterSpec;
+import javax.crypto.spec.SecretKeySpec;
+
+import org.apache.qpid.server.plugin.PluggableService;
+import org.apache.qpid.server.util.Strings;
+
+class AESGCMKeyFileEncrypter implements ConfigurationSecretEncrypter
+{
+    private static final String CIPHER_NAME = "AES/GCM/NoPadding";
+    private static final int GCM_INITIALIZATION_VECTOR_LENGTH = 12;
+    private static final int GCM_TAG_LENGTH = 128;
+    private static final String AES_ALGORITHM = "AES";
+    private final SecureRandom _random = new SecureRandom();
+    private final SecretKey _secretKey;
+
+    AESGCMKeyFileEncrypter(SecretKey secretKey)
+    {
+        if(secretKey == null)
+        {
+            throw new NullPointerException("A non null secret key must be supplied");
+        }
+        if(!AES_ALGORITHM.equals(secretKey.getAlgorithm()))
+        {
+            throw new IllegalArgumentException("Provided secret key was for the algorithm: " + secretKey.getAlgorithm()

Review comment:
       an exception message can be build using String#format

##########
File path: broker-core/src/main/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypterFactory.java
##########
@@ -0,0 +1,383 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.AclEntry;
+import java.nio.file.attribute.AclEntryPermission;
+import java.nio.file.attribute.AclEntryType;
+import java.nio.file.attribute.AclFileAttributeView;
+import java.nio.file.attribute.FileAttribute;
+import java.nio.file.attribute.PosixFileAttributeView;
+import java.nio.file.attribute.PosixFilePermission;
+import java.nio.file.attribute.PosixFilePermissions;
+import java.nio.file.attribute.UserPrincipal;
+import java.security.NoSuchAlgorithmException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.EnumSet;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.ListIterator;
+import java.util.Map;
+import java.util.Set;
+
+import javax.crypto.Cipher;
+import javax.crypto.KeyGenerator;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.SecretKeySpec;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.qpid.server.configuration.IllegalConfigurationException;
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.SystemConfig;
+import org.apache.qpid.server.plugin.ConditionallyAvailable;
+import org.apache.qpid.server.plugin.ConfigurationSecretEncrypterFactory;
+import org.apache.qpid.server.plugin.PluggableService;
+
+@PluggableService
+public class AESGCMKeyFileEncrypterFactory implements ConfigurationSecretEncrypterFactory, ConditionallyAvailable
+{
+    private static final Logger LOGGER = LoggerFactory.getLogger(AESGCMKeyFileEncrypterFactory.class);
+
+    static final String ENCRYPTER_KEY_FILE = "encrypter.key.file";
+
+    private static final int AES_KEY_SIZE_BITS = 256;
+    private static final int AES_KEY_SIZE_BYTES = AES_KEY_SIZE_BITS / 8;
+    private static final String AES_ALGORITHM = "AES";
+
+    public static final String TYPE = "AESGCMKeyFile";
+
+    static final String DEFAULT_KEYS_SUBDIR_NAME = ".keys";
+
+    private static final boolean IS_AVAILABLE;
+
+    private static final String ILLEGAL_ARG_EXCEPTION = "Unable to determine a mechanism to protect access to the key file on this filesystem";
+
+    static
+    {
+        boolean isAvailable;
+        try
+        {
+            final int allowedKeyLength = Cipher.getMaxAllowedKeyLength(AES_ALGORITHM);
+            isAvailable = allowedKeyLength >=AES_KEY_SIZE_BITS;
+            if(!isAvailable)
+            {
+                LOGGER.warn("The {} configuration encryption encryption mechanism is not available. "
+                            + "Maximum available AES key length is {} but {} is required. "
+                            + "Ensure the full strength JCE policy has been installed into your JVM.", TYPE, allowedKeyLength, AES_KEY_SIZE_BITS);
+            }
+        }
+        catch (NoSuchAlgorithmException e)
+        {
+            isAvailable = false;
+
+            LOGGER.error("The " + TYPE + " configuration encryption encryption mechanism is not available. "
+                        + "The " + AES_ALGORITHM + " algorithm is not available within the JVM (despite it being a requirement).");
+        }
+
+        IS_AVAILABLE = isAvailable;
+    }
+
+    @Override
+    public ConfigurationSecretEncrypter createEncrypter(final ConfiguredObject<?> object)

Review comment:
       this is a common code as well. please move it into a method and call it in both encrypter factories

##########
File path: broker-core/src/test/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypterTest.java
##########
@@ -0,0 +1,364 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import static org.mockito.Mockito.mock;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.SecureRandom;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Random;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+
+import javax.crypto.SecretKeyFactory;
+import javax.crypto.spec.PBEKeySpec;
+import javax.crypto.spec.SecretKeySpec;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.util.concurrent.SettableFuture;
+import org.junit.Before;
+import org.junit.Test;
+
+import org.apache.qpid.server.SystemLauncher;
+import org.apache.qpid.server.SystemLauncherListener;
+import org.apache.qpid.server.model.AuthenticationProvider;
+import org.apache.qpid.server.model.Broker;
+import org.apache.qpid.server.model.BrokerModel;
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.JsonSystemConfigImpl;
+import org.apache.qpid.server.model.SystemConfig;
+import org.apache.qpid.server.model.User;
+import org.apache.qpid.server.util.FileUtils;
+import org.apache.qpid.test.utils.UnitTestBase;
+
+public class AESGCMKeyFileEncrypterTest extends UnitTestBase
+{
+    private static final String PLAIN_SECRET = "secret";
+    private final SecureRandom _random = new SecureRandom();
+    public static final String PLAINTEXT = "guestkey";
+    private Path _configurationLocation;
+    private Path _workDir;
+    private Broker<?> _broker;
+    private SystemLauncher _systemLauncher;
+    private Map<String, Object> attributes = new HashMap<>();

Review comment:
       why attributes are declared as a test field?

##########
File path: broker-core/src/test/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypterTest.java
##########
@@ -0,0 +1,364 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import static org.mockito.Mockito.mock;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.SecureRandom;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Random;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+
+import javax.crypto.SecretKeyFactory;
+import javax.crypto.spec.PBEKeySpec;
+import javax.crypto.spec.SecretKeySpec;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.util.concurrent.SettableFuture;
+import org.junit.Before;
+import org.junit.Test;
+
+import org.apache.qpid.server.SystemLauncher;
+import org.apache.qpid.server.SystemLauncherListener;
+import org.apache.qpid.server.model.AuthenticationProvider;
+import org.apache.qpid.server.model.Broker;
+import org.apache.qpid.server.model.BrokerModel;
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.JsonSystemConfigImpl;
+import org.apache.qpid.server.model.SystemConfig;
+import org.apache.qpid.server.model.User;
+import org.apache.qpid.server.util.FileUtils;
+import org.apache.qpid.test.utils.UnitTestBase;
+
+public class AESGCMKeyFileEncrypterTest extends UnitTestBase
+{
+    private static final String PLAIN_SECRET = "secret";
+    private final SecureRandom _random = new SecureRandom();
+    public static final String PLAINTEXT = "guestkey";

Review comment:
       this needs to be moved up to other constant

##########
File path: broker-core/src/test/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypterTest.java
##########
@@ -0,0 +1,364 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import static org.mockito.Mockito.mock;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.SecureRandom;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Random;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+
+import javax.crypto.SecretKeyFactory;
+import javax.crypto.spec.PBEKeySpec;
+import javax.crypto.spec.SecretKeySpec;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.util.concurrent.SettableFuture;
+import org.junit.Before;
+import org.junit.Test;
+
+import org.apache.qpid.server.SystemLauncher;
+import org.apache.qpid.server.SystemLauncherListener;
+import org.apache.qpid.server.model.AuthenticationProvider;
+import org.apache.qpid.server.model.Broker;
+import org.apache.qpid.server.model.BrokerModel;
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.JsonSystemConfigImpl;
+import org.apache.qpid.server.model.SystemConfig;
+import org.apache.qpid.server.model.User;
+import org.apache.qpid.server.util.FileUtils;
+import org.apache.qpid.test.utils.UnitTestBase;
+
+public class AESGCMKeyFileEncrypterTest extends UnitTestBase
+{
+    private static final String PLAIN_SECRET = "secret";
+    private final SecureRandom _random = new SecureRandom();
+    public static final String PLAINTEXT = "guestkey";
+    private Path _configurationLocation;
+    private Path _workDir;
+    private Broker<?> _broker;
+    private SystemLauncher _systemLauncher;
+    private Map<String, Object> attributes = new HashMap<>();
+
+    private void createBrokerWithEncrypter(final Object encrptionType) throws Exception
+    {
+        try
+        {
+            _workDir = Files.createTempDirectory("qpid_work_dir");
+            _configurationLocation = Files.createTempFile(_workDir, "config", ".json");
+            Map<String, Object> context = new HashMap<>();
+            context.put("qpid.work_dir", _workDir.toFile().getAbsolutePath());
+            Map<String, Object> config = new HashMap<>();
+            config.put(ConfiguredObject.NAME, getTestName());
+            config.put(Broker.MODEL_VERSION, BrokerModel.MODEL_VERSION);
+            config.put(Broker.CONFIDENTIAL_CONFIGURATION_ENCRYPTION_PROVIDER, encrptionType);
+            new ObjectMapper().writeValue(_configurationLocation.toFile(), config);
+
+            attributes.put("storePath", _configurationLocation.toFile().getAbsolutePath());
+            attributes.put("preferenceStoreAttributes", "{\"type\": \"Noop\"}");
+            attributes.put(SystemConfig.TYPE, JsonSystemConfigImpl.SYSTEM_CONFIG_TYPE);
+            attributes.put(SystemConfig.STARTUP_LOGGED_TO_SYSTEM_OUT, Boolean.FALSE);
+            attributes.put(SystemConfig.CONTEXT, context);
+            final SettableFuture<SystemConfig<?>> configFuture = SettableFuture.create();
+            _systemLauncher = new SystemLauncher(new SystemLauncherListener.DefaultSystemLauncherListener()
+            {
+                @Override
+                public void onContainerResolve(final SystemConfig<?> systemConfig)
+                {
+                    configFuture.set(systemConfig);
+                }
+            });
+            _systemLauncher.startup(attributes);
+            final SystemConfig<?> systemConfig = configFuture.get(10, TimeUnit.SECONDS);
+            _broker = (Broker<?>) systemConfig.getContainer();
+
+            final Map<String, Object> authProviderAttributes = new HashMap<>();
+            authProviderAttributes.put(ConfiguredObject.NAME, "testAuthProvider");
+            authProviderAttributes.put(ConfiguredObject.TYPE, "Plain");
+
+            final AuthenticationProvider
+                    authProvider = _broker.createChild(AuthenticationProvider.class, authProviderAttributes);
+
+            Map<String, Object> userAttrs = new HashMap<>();
+            userAttrs.put(User.TYPE, "managed");
+            userAttrs.put(User.NAME, "guest");
+            userAttrs.put(User.PASSWORD, PLAINTEXT);
+            User user = (User) authProvider.createChild(User.class, userAttrs);
+        }finally{
+            _systemLauncher.shutdown();
+            FileUtils.deleteDirectory(_workDir.toFile().getAbsolutePath());
+            FileUtils.deleteDirectory(_configurationLocation.toFile().getAbsolutePath());
+        }
+    }
+
+    @Test
+    public void testRepeatedEncryptionsReturnDifferentValues() throws Exception
+    {
+        SecretKeySpec secretKey = createSecretKey();
+        AESGCMKeyFileEncrypter encrypter = new AESGCMKeyFileEncrypter(secretKey);
+
+        Set<String> encryptions = new HashSet<>();
+
+        int iterations = 100;
+
+        for (int i = 0; i < iterations; i++)
+        {
+            encryptions.add(encrypter.encrypt(PLAINTEXT));
+        }
+
+        assertEquals("Not all encryptions were distinct", (long) iterations, (long) encryptions.size());
+
+        for (String encrypted : encryptions)
+        {
+            assertEquals("Not all encryptions decrypt correctly", PLAINTEXT, encrypter.decrypt(encrypted));
+        }
+    }
+
+    @Test
+    public void testCreationFailsOnInvalidSecret() throws Exception
+    {
+        try
+        {
+            new AESGCMKeyFileEncrypter(null);
+            fail("An encrypter should not be creatable from a null key");
+        }
+        catch (NullPointerException e)
+        {
+            assertTrue("Unexpected exception message:" + e.getMessage(),
+                       e.getMessage().contains("A non null secret key must be supplied"));
+        }
+
+        try
+        {
+            PBEKeySpec keySpec = new PBEKeySpec(PLAIN_SECRET.toCharArray());
+            SecretKeyFactory factory = SecretKeyFactory.getInstance("PBEWithMD5AndDES");
+            new AESGCMKeyFileEncrypter(factory.generateSecret(keySpec));
+            fail("An encrypter should not be creatable from the wrong type of secret key");
+        }
+        catch (IllegalArgumentException e)
+        {
+            assertTrue("Unexpected exception message:" + e.getMessage(),
+                       e.getMessage().contains("Provided secret key was for the algorithm: PBEWithMD5AndDES when AES was needed."));
+        }
+    }
+
+    @Test
+    public void testEncryptionOfEmptyString() throws Exception
+    {
+        doTestSimpleEncryptDecrypt("");
+    }
+
+    private void doTestSimpleEncryptDecrypt(final String text)
+    {
+        SecretKeySpec secretKey = createSecretKey();
+        AESGCMKeyFileEncrypter encrypter = new AESGCMKeyFileEncrypter(secretKey);
+
+        String encrypted = encrypter.encrypt(text);
+        assertNotNull("Encrypter did not return a result from encryption", encrypted);
+        assertNotEquals("Plain text and encrypted version are equal", text, encrypted);
+        String decrypted = encrypter.decrypt(encrypted);
+        assertNotNull("Encrypter did not return a result from decryption", decrypted);
+        assertEquals("Encryption was not reversible", text, decrypted);
+    }
+
+    @Test
+    public void testEncryptingNullFails() throws Exception
+    {
+        try
+        {
+            SecretKeySpec secretKey = createSecretKey();
+            AESGCMKeyFileEncrypter encrypter = new AESGCMKeyFileEncrypter(secretKey);
+
+            String encrypted = encrypter.encrypt(null);
+            fail("Attempting to encrypt null should fail");
+        }
+        catch (NullPointerException e)
+        {
+            //pass
+        }
+    }
+
+    @Test
+    public void testEncryptingVeryLargeSecret() throws Exception
+    {
+        Random random = new Random();
+        byte[] data = new byte[4096];
+        random.nextBytes(data);
+        for (int i = 0; i < data.length; i++)
+        {
+            data[i] = (byte) (data[i] & 0xEF);
+        }
+        doTestSimpleEncryptDecrypt(new String(data, StandardCharsets.US_ASCII));
+    }
+
+    @Test
+    public void testDecryptNonsense() throws Exception
+    {
+        SecretKeySpec secretKey = createSecretKey();
+        AESGCMKeyFileEncrypter encrypter = new AESGCMKeyFileEncrypter(secretKey);
+
+        try
+        {
+            encrypter.decrypt(null);
+            fail("Should not decrypt a null value");
+        }
+        catch (NullPointerException e)
+        {
+            // pass
+        }
+
+        try
+        {
+            encrypter.decrypt("");
+            fail("Should not decrypt the empty String");
+        }
+        catch (IllegalArgumentException e)
+        {
+            // pass
+        }
+
+        try
+        {
+            encrypter.decrypt("thisisnonsense");
+            fail("Should not decrypt a small amount of nonsense");
+        }
+        catch (IllegalArgumentException e)
+        {
+            // pass
+        }
+
+        try
+        {
+            String answer = encrypter.decrypt("thisisn'tvalidBase64!soitshouldfailwithanIllegalArgumentException");
+            fail("Should not decrypt a larger amount of nonsense");
+        }
+        catch (IllegalArgumentException e)
+        {
+            // pass
+        }
+    }
+
+    private SecretKeySpec createSecretKey()

Review comment:
       this looks like a common code

##########
File path: broker-core/src/test/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypterTest.java
##########
@@ -0,0 +1,364 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import static org.mockito.Mockito.mock;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.SecureRandom;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Random;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+
+import javax.crypto.SecretKeyFactory;
+import javax.crypto.spec.PBEKeySpec;
+import javax.crypto.spec.SecretKeySpec;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.util.concurrent.SettableFuture;
+import org.junit.Before;
+import org.junit.Test;
+
+import org.apache.qpid.server.SystemLauncher;
+import org.apache.qpid.server.SystemLauncherListener;
+import org.apache.qpid.server.model.AuthenticationProvider;
+import org.apache.qpid.server.model.Broker;
+import org.apache.qpid.server.model.BrokerModel;
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.JsonSystemConfigImpl;
+import org.apache.qpid.server.model.SystemConfig;
+import org.apache.qpid.server.model.User;
+import org.apache.qpid.server.util.FileUtils;
+import org.apache.qpid.test.utils.UnitTestBase;
+
+public class AESGCMKeyFileEncrypterTest extends UnitTestBase
+{
+    private static final String PLAIN_SECRET = "secret";
+    private final SecureRandom _random = new SecureRandom();
+    public static final String PLAINTEXT = "guestkey";
+    private Path _configurationLocation;
+    private Path _workDir;
+    private Broker<?> _broker;
+    private SystemLauncher _systemLauncher;
+    private Map<String, Object> attributes = new HashMap<>();
+
+    private void createBrokerWithEncrypter(final Object encrptionType) throws Exception
+    {
+        try
+        {
+            _workDir = Files.createTempDirectory("qpid_work_dir");
+            _configurationLocation = Files.createTempFile(_workDir, "config", ".json");
+            Map<String, Object> context = new HashMap<>();
+            context.put("qpid.work_dir", _workDir.toFile().getAbsolutePath());
+            Map<String, Object> config = new HashMap<>();
+            config.put(ConfiguredObject.NAME, getTestName());
+            config.put(Broker.MODEL_VERSION, BrokerModel.MODEL_VERSION);
+            config.put(Broker.CONFIDENTIAL_CONFIGURATION_ENCRYPTION_PROVIDER, encrptionType);
+            new ObjectMapper().writeValue(_configurationLocation.toFile(), config);
+
+            attributes.put("storePath", _configurationLocation.toFile().getAbsolutePath());
+            attributes.put("preferenceStoreAttributes", "{\"type\": \"Noop\"}");
+            attributes.put(SystemConfig.TYPE, JsonSystemConfigImpl.SYSTEM_CONFIG_TYPE);
+            attributes.put(SystemConfig.STARTUP_LOGGED_TO_SYSTEM_OUT, Boolean.FALSE);
+            attributes.put(SystemConfig.CONTEXT, context);
+            final SettableFuture<SystemConfig<?>> configFuture = SettableFuture.create();
+            _systemLauncher = new SystemLauncher(new SystemLauncherListener.DefaultSystemLauncherListener()
+            {
+                @Override
+                public void onContainerResolve(final SystemConfig<?> systemConfig)
+                {
+                    configFuture.set(systemConfig);
+                }
+            });
+            _systemLauncher.startup(attributes);
+            final SystemConfig<?> systemConfig = configFuture.get(10, TimeUnit.SECONDS);
+            _broker = (Broker<?>) systemConfig.getContainer();
+
+            final Map<String, Object> authProviderAttributes = new HashMap<>();
+            authProviderAttributes.put(ConfiguredObject.NAME, "testAuthProvider");
+            authProviderAttributes.put(ConfiguredObject.TYPE, "Plain");
+
+            final AuthenticationProvider
+                    authProvider = _broker.createChild(AuthenticationProvider.class, authProviderAttributes);
+
+            Map<String, Object> userAttrs = new HashMap<>();
+            userAttrs.put(User.TYPE, "managed");
+            userAttrs.put(User.NAME, "guest");
+            userAttrs.put(User.PASSWORD, PLAINTEXT);
+            User user = (User) authProvider.createChild(User.class, userAttrs);
+        }finally{
+            _systemLauncher.shutdown();
+            FileUtils.deleteDirectory(_workDir.toFile().getAbsolutePath());
+            FileUtils.deleteDirectory(_configurationLocation.toFile().getAbsolutePath());
+        }
+    }
+
+    @Test
+    public void testRepeatedEncryptionsReturnDifferentValues() throws Exception
+    {
+        SecretKeySpec secretKey = createSecretKey();
+        AESGCMKeyFileEncrypter encrypter = new AESGCMKeyFileEncrypter(secretKey);
+
+        Set<String> encryptions = new HashSet<>();
+
+        int iterations = 100;
+
+        for (int i = 0; i < iterations; i++)
+        {
+            encryptions.add(encrypter.encrypt(PLAINTEXT));
+        }
+
+        assertEquals("Not all encryptions were distinct", (long) iterations, (long) encryptions.size());
+
+        for (String encrypted : encryptions)
+        {
+            assertEquals("Not all encryptions decrypt correctly", PLAINTEXT, encrypter.decrypt(encrypted));
+        }
+    }
+
+    @Test
+    public void testCreationFailsOnInvalidSecret() throws Exception
+    {
+        try
+        {
+            new AESGCMKeyFileEncrypter(null);
+            fail("An encrypter should not be creatable from a null key");
+        }
+        catch (NullPointerException e)
+        {
+            assertTrue("Unexpected exception message:" + e.getMessage(),
+                       e.getMessage().contains("A non null secret key must be supplied"));
+        }
+
+        try
+        {
+            PBEKeySpec keySpec = new PBEKeySpec(PLAIN_SECRET.toCharArray());
+            SecretKeyFactory factory = SecretKeyFactory.getInstance("PBEWithMD5AndDES");
+            new AESGCMKeyFileEncrypter(factory.generateSecret(keySpec));
+            fail("An encrypter should not be creatable from the wrong type of secret key");
+        }
+        catch (IllegalArgumentException e)
+        {
+            assertTrue("Unexpected exception message:" + e.getMessage(),
+                       e.getMessage().contains("Provided secret key was for the algorithm: PBEWithMD5AndDES when AES was needed."));
+        }
+    }
+
+    @Test
+    public void testEncryptionOfEmptyString() throws Exception
+    {
+        doTestSimpleEncryptDecrypt("");
+    }
+
+    private void doTestSimpleEncryptDecrypt(final String text)
+    {
+        SecretKeySpec secretKey = createSecretKey();
+        AESGCMKeyFileEncrypter encrypter = new AESGCMKeyFileEncrypter(secretKey);
+
+        String encrypted = encrypter.encrypt(text);
+        assertNotNull("Encrypter did not return a result from encryption", encrypted);
+        assertNotEquals("Plain text and encrypted version are equal", text, encrypted);
+        String decrypted = encrypter.decrypt(encrypted);
+        assertNotNull("Encrypter did not return a result from decryption", decrypted);
+        assertEquals("Encryption was not reversible", text, decrypted);
+    }
+
+    @Test
+    public void testEncryptingNullFails() throws Exception
+    {
+        try
+        {
+            SecretKeySpec secretKey = createSecretKey();
+            AESGCMKeyFileEncrypter encrypter = new AESGCMKeyFileEncrypter(secretKey);
+
+            String encrypted = encrypter.encrypt(null);
+            fail("Attempting to encrypt null should fail");
+        }
+        catch (NullPointerException e)
+        {
+            //pass
+        }
+    }
+
+    @Test
+    public void testEncryptingVeryLargeSecret() throws Exception
+    {
+        Random random = new Random();
+        byte[] data = new byte[4096];
+        random.nextBytes(data);
+        for (int i = 0; i < data.length; i++)
+        {
+            data[i] = (byte) (data[i] & 0xEF);
+        }
+        doTestSimpleEncryptDecrypt(new String(data, StandardCharsets.US_ASCII));
+    }
+
+    @Test
+    public void testDecryptNonsense() throws Exception
+    {
+        SecretKeySpec secretKey = createSecretKey();
+        AESGCMKeyFileEncrypter encrypter = new AESGCMKeyFileEncrypter(secretKey);
+
+        try
+        {
+            encrypter.decrypt(null);
+            fail("Should not decrypt a null value");
+        }
+        catch (NullPointerException e)
+        {
+            // pass
+        }
+
+        try
+        {
+            encrypter.decrypt("");
+            fail("Should not decrypt the empty String");
+        }
+        catch (IllegalArgumentException e)
+        {
+            // pass
+        }
+
+        try
+        {
+            encrypter.decrypt("thisisnonsense");
+            fail("Should not decrypt a small amount of nonsense");
+        }
+        catch (IllegalArgumentException e)
+        {
+            // pass
+        }
+
+        try
+        {
+            String answer = encrypter.decrypt("thisisn'tvalidBase64!soitshouldfailwithanIllegalArgumentException");
+            fail("Should not decrypt a larger amount of nonsense");
+        }
+        catch (IllegalArgumentException e)
+        {
+            // pass
+        }
+    }
+
+    private SecretKeySpec createSecretKey()
+    {
+        final byte[] keyData = new byte[32];
+        _random.nextBytes(keyData);
+        return new SecretKeySpec(keyData, "AES");
+    }
+
+    @Test
+    public void testChangeOfEncryptiontoGCM() throws Exception
+    {
+        createBrokerWithEncrypter(AESKeyFileEncrypterFactory.TYPE);
+        String decryptedPassword = null;
+        String encryptedPassword = getEncryptedPasswordFromConfig();
+        byte[] keyBytes = getSecretKey(decryptedPassword);
+        AESKeyFileEncrypter encrypter = new AESKeyFileEncrypter(new SecretKeySpec(keyBytes, "AES"));
+        decryptedPassword = encrypter.decrypt(encryptedPassword);
+        assertEquals("Decrpted text doesnt match original", PLAINTEXT, decryptedPassword);
+        //_broker.setAttributes(Collections.singletonMap(Broker.CONFIDENTIAL_CONFIGURATION_ENCRYPTION_PROVIDER,"AesGcmKeyFile"));
+        //updateBroker();
+        /*String gcmencryptedPassword = getEncryptedPasswordFromConfig();
+        byte[] gcmKeyBytes = getSecretKey(decryptedPassword);
+        AESGCMKeyFileEncrypter gcmEncrypter = new AESGCMKeyFileEncrypter(new SecretKeySpec(gcmKeyBytes, "AES"));
+        String gcmDecryptedPassword = gcmEncrypter.decrypt(gcmencryptedPassword);
+        assertEquals("Decrpted text doesnt match original", PLAINTEXT, gcmDecryptedPassword);*/
+    }
+
+    @Test
+    public void testChangeOfEncryptiontoAES() throws Exception
+    {
+        createBrokerWithEncrypter(AESGCMKeyFileEncrypterFactory.TYPE);
+        String decryptedPassword = null;
+        String encryptedPassword = getEncryptedPasswordFromConfig();
+        byte[] keyBytes = getSecretKey(decryptedPassword);
+        AESKeyFileEncrypter encrypter = new AESKeyFileEncrypter(new SecretKeySpec(keyBytes, "AES"));
+        decryptedPassword = encrypter.decrypt(encryptedPassword);
+        assertEquals("Decrpted text doesnt match original", PLAINTEXT, decryptedPassword);
+        //_broker.setAttributes(Collections.singletonMap(Broker.CONFIDENTIAL_CONFIGURATION_ENCRYPTION_PROVIDER,"AesGcmKeyFile"));

Review comment:
       commented code
   incomplete test

##########
File path: broker-core/src/test/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypterTest.java
##########
@@ -0,0 +1,364 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import static org.mockito.Mockito.mock;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.SecureRandom;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Random;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+
+import javax.crypto.SecretKeyFactory;
+import javax.crypto.spec.PBEKeySpec;
+import javax.crypto.spec.SecretKeySpec;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.util.concurrent.SettableFuture;
+import org.junit.Before;
+import org.junit.Test;
+
+import org.apache.qpid.server.SystemLauncher;
+import org.apache.qpid.server.SystemLauncherListener;
+import org.apache.qpid.server.model.AuthenticationProvider;
+import org.apache.qpid.server.model.Broker;
+import org.apache.qpid.server.model.BrokerModel;
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.JsonSystemConfigImpl;
+import org.apache.qpid.server.model.SystemConfig;
+import org.apache.qpid.server.model.User;
+import org.apache.qpid.server.util.FileUtils;
+import org.apache.qpid.test.utils.UnitTestBase;
+
+public class AESGCMKeyFileEncrypterTest extends UnitTestBase
+{
+    private static final String PLAIN_SECRET = "secret";
+    private final SecureRandom _random = new SecureRandom();
+    public static final String PLAINTEXT = "guestkey";
+    private Path _configurationLocation;
+    private Path _workDir;
+    private Broker<?> _broker;
+    private SystemLauncher _systemLauncher;
+    private Map<String, Object> attributes = new HashMap<>();
+
+    private void createBrokerWithEncrypter(final Object encrptionType) throws Exception
+    {
+        try
+        {
+            _workDir = Files.createTempDirectory("qpid_work_dir");
+            _configurationLocation = Files.createTempFile(_workDir, "config", ".json");
+            Map<String, Object> context = new HashMap<>();
+            context.put("qpid.work_dir", _workDir.toFile().getAbsolutePath());
+            Map<String, Object> config = new HashMap<>();
+            config.put(ConfiguredObject.NAME, getTestName());
+            config.put(Broker.MODEL_VERSION, BrokerModel.MODEL_VERSION);
+            config.put(Broker.CONFIDENTIAL_CONFIGURATION_ENCRYPTION_PROVIDER, encrptionType);
+            new ObjectMapper().writeValue(_configurationLocation.toFile(), config);
+
+            attributes.put("storePath", _configurationLocation.toFile().getAbsolutePath());
+            attributes.put("preferenceStoreAttributes", "{\"type\": \"Noop\"}");
+            attributes.put(SystemConfig.TYPE, JsonSystemConfigImpl.SYSTEM_CONFIG_TYPE);
+            attributes.put(SystemConfig.STARTUP_LOGGED_TO_SYSTEM_OUT, Boolean.FALSE);
+            attributes.put(SystemConfig.CONTEXT, context);
+            final SettableFuture<SystemConfig<?>> configFuture = SettableFuture.create();
+            _systemLauncher = new SystemLauncher(new SystemLauncherListener.DefaultSystemLauncherListener()
+            {
+                @Override
+                public void onContainerResolve(final SystemConfig<?> systemConfig)
+                {
+                    configFuture.set(systemConfig);
+                }
+            });
+            _systemLauncher.startup(attributes);
+            final SystemConfig<?> systemConfig = configFuture.get(10, TimeUnit.SECONDS);
+            _broker = (Broker<?>) systemConfig.getContainer();
+
+            final Map<String, Object> authProviderAttributes = new HashMap<>();
+            authProviderAttributes.put(ConfiguredObject.NAME, "testAuthProvider");
+            authProviderAttributes.put(ConfiguredObject.TYPE, "Plain");
+
+            final AuthenticationProvider
+                    authProvider = _broker.createChild(AuthenticationProvider.class, authProviderAttributes);
+
+            Map<String, Object> userAttrs = new HashMap<>();
+            userAttrs.put(User.TYPE, "managed");
+            userAttrs.put(User.NAME, "guest");
+            userAttrs.put(User.PASSWORD, PLAINTEXT);
+            User user = (User) authProvider.createChild(User.class, userAttrs);
+        }finally{
+            _systemLauncher.shutdown();
+            FileUtils.deleteDirectory(_workDir.toFile().getAbsolutePath());
+            FileUtils.deleteDirectory(_configurationLocation.toFile().getAbsolutePath());
+        }
+    }
+
+    @Test
+    public void testRepeatedEncryptionsReturnDifferentValues() throws Exception
+    {
+        SecretKeySpec secretKey = createSecretKey();
+        AESGCMKeyFileEncrypter encrypter = new AESGCMKeyFileEncrypter(secretKey);
+
+        Set<String> encryptions = new HashSet<>();
+
+        int iterations = 100;
+
+        for (int i = 0; i < iterations; i++)
+        {
+            encryptions.add(encrypter.encrypt(PLAINTEXT));
+        }
+
+        assertEquals("Not all encryptions were distinct", (long) iterations, (long) encryptions.size());
+
+        for (String encrypted : encryptions)
+        {
+            assertEquals("Not all encryptions decrypt correctly", PLAINTEXT, encrypter.decrypt(encrypted));
+        }
+    }
+
+    @Test
+    public void testCreationFailsOnInvalidSecret() throws Exception
+    {
+        try
+        {
+            new AESGCMKeyFileEncrypter(null);
+            fail("An encrypter should not be creatable from a null key");
+        }
+        catch (NullPointerException e)
+        {
+            assertTrue("Unexpected exception message:" + e.getMessage(),
+                       e.getMessage().contains("A non null secret key must be supplied"));
+        }
+
+        try
+        {
+            PBEKeySpec keySpec = new PBEKeySpec(PLAIN_SECRET.toCharArray());
+            SecretKeyFactory factory = SecretKeyFactory.getInstance("PBEWithMD5AndDES");

Review comment:
       Does SecretKeyFactory "PBEWithMD5AndDES"exist in JVM

##########
File path: broker-core/src/test/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypterTest.java
##########
@@ -0,0 +1,364 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import static org.mockito.Mockito.mock;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.SecureRandom;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Random;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+
+import javax.crypto.SecretKeyFactory;
+import javax.crypto.spec.PBEKeySpec;
+import javax.crypto.spec.SecretKeySpec;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.util.concurrent.SettableFuture;
+import org.junit.Before;
+import org.junit.Test;
+
+import org.apache.qpid.server.SystemLauncher;
+import org.apache.qpid.server.SystemLauncherListener;
+import org.apache.qpid.server.model.AuthenticationProvider;
+import org.apache.qpid.server.model.Broker;
+import org.apache.qpid.server.model.BrokerModel;
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.JsonSystemConfigImpl;
+import org.apache.qpid.server.model.SystemConfig;
+import org.apache.qpid.server.model.User;
+import org.apache.qpid.server.util.FileUtils;
+import org.apache.qpid.test.utils.UnitTestBase;
+
+public class AESGCMKeyFileEncrypterTest extends UnitTestBase
+{
+    private static final String PLAIN_SECRET = "secret";
+    private final SecureRandom _random = new SecureRandom();
+    public static final String PLAINTEXT = "guestkey";
+    private Path _configurationLocation;
+    private Path _workDir;
+    private Broker<?> _broker;
+    private SystemLauncher _systemLauncher;
+    private Map<String, Object> attributes = new HashMap<>();
+
+    private void createBrokerWithEncrypter(final Object encrptionType) throws Exception
+    {
+        try
+        {
+            _workDir = Files.createTempDirectory("qpid_work_dir");
+            _configurationLocation = Files.createTempFile(_workDir, "config", ".json");
+            Map<String, Object> context = new HashMap<>();

Review comment:
       this cam be represented as a singleton map

##########
File path: broker-core/src/main/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypterFactory.java
##########
@@ -0,0 +1,383 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.AclEntry;
+import java.nio.file.attribute.AclEntryPermission;
+import java.nio.file.attribute.AclEntryType;
+import java.nio.file.attribute.AclFileAttributeView;
+import java.nio.file.attribute.FileAttribute;
+import java.nio.file.attribute.PosixFileAttributeView;
+import java.nio.file.attribute.PosixFilePermission;
+import java.nio.file.attribute.PosixFilePermissions;
+import java.nio.file.attribute.UserPrincipal;
+import java.security.NoSuchAlgorithmException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.EnumSet;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.ListIterator;
+import java.util.Map;
+import java.util.Set;
+
+import javax.crypto.Cipher;
+import javax.crypto.KeyGenerator;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.SecretKeySpec;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.qpid.server.configuration.IllegalConfigurationException;
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.SystemConfig;
+import org.apache.qpid.server.plugin.ConditionallyAvailable;
+import org.apache.qpid.server.plugin.ConfigurationSecretEncrypterFactory;
+import org.apache.qpid.server.plugin.PluggableService;
+
+@PluggableService
+public class AESGCMKeyFileEncrypterFactory implements ConfigurationSecretEncrypterFactory, ConditionallyAvailable
+{
+    private static final Logger LOGGER = LoggerFactory.getLogger(AESGCMKeyFileEncrypterFactory.class);
+
+    static final String ENCRYPTER_KEY_FILE = "encrypter.key.file";
+
+    private static final int AES_KEY_SIZE_BITS = 256;
+    private static final int AES_KEY_SIZE_BYTES = AES_KEY_SIZE_BITS / 8;
+    private static final String AES_ALGORITHM = "AES";
+
+    public static final String TYPE = "AESGCMKeyFile";
+
+    static final String DEFAULT_KEYS_SUBDIR_NAME = ".keys";
+
+    private static final boolean IS_AVAILABLE;
+
+    private static final String ILLEGAL_ARG_EXCEPTION = "Unable to determine a mechanism to protect access to the key file on this filesystem";
+
+    static
+    {
+        boolean isAvailable;
+        try
+        {
+            final int allowedKeyLength = Cipher.getMaxAllowedKeyLength(AES_ALGORITHM);
+            isAvailable = allowedKeyLength >=AES_KEY_SIZE_BITS;
+            if(!isAvailable)
+            {
+                LOGGER.warn("The {} configuration encryption encryption mechanism is not available. "
+                            + "Maximum available AES key length is {} but {} is required. "
+                            + "Ensure the full strength JCE policy has been installed into your JVM.", TYPE, allowedKeyLength, AES_KEY_SIZE_BITS);
+            }
+        }
+        catch (NoSuchAlgorithmException e)
+        {
+            isAvailable = false;
+
+            LOGGER.error("The " + TYPE + " configuration encryption encryption mechanism is not available. "
+                        + "The " + AES_ALGORITHM + " algorithm is not available within the JVM (despite it being a requirement).");
+        }
+
+        IS_AVAILABLE = isAvailable;
+    }
+
+    @Override
+    public ConfigurationSecretEncrypter createEncrypter(final ConfiguredObject<?> object)
+    {
+        String fileLocation;
+        if(object.getContextKeys(false).contains(ENCRYPTER_KEY_FILE))
+        {
+            fileLocation = object.getContextValue(String.class, ENCRYPTER_KEY_FILE);
+        }
+        else
+        {
+
+            fileLocation = object.getContextValue(String.class, SystemConfig.QPID_WORK_DIR)
+                           + File.separator + DEFAULT_KEYS_SUBDIR_NAME + File.separator
+                           + object.getCategoryClass().getSimpleName() + "_"
+                           + object.getName() + ".key";
+
+            Map<String, String> context = object.getContext();
+            Map<String, String> modifiedContext = new LinkedHashMap<>(context);
+            modifiedContext.put(ENCRYPTER_KEY_FILE, fileLocation);
+
+            object.setAttributes(Collections.<String, Object>singletonMap(ConfiguredObject.CONTEXT, modifiedContext));
+        }
+        File file = new File(fileLocation);
+        if(!file.exists())
+        {
+            LOGGER.warn("Configuration encryption is enabled, but no configuration secret was found. A new configuration secret will be created at '{}'.", fileLocation);
+            createAndPopulateKeyFile(file);
+        }
+        if(!file.isFile())
+        {
+            throw new IllegalArgumentException("File '"+fileLocation+"' is not a regular file.");
+        }
+        try
+        {
+            checkFilePermissions(fileLocation, file);
+            if(Files.size(file.toPath()) != AES_KEY_SIZE_BYTES)
+            {
+                throw new IllegalArgumentException("Key file '" + fileLocation + "' contains an incorrect about of data");
+            }
+
+            try(FileInputStream inputStream = new FileInputStream(file))
+            {
+                byte[] key = new byte[AES_KEY_SIZE_BYTES];
+                int pos = 0;
+                int read;
+                while(pos < key.length && -1 != ( read = inputStream.read(key, pos, key.length - pos)))
+                {
+                    pos += read;
+                }
+                if(pos != key.length)
+                {
+                    throw new IllegalConfigurationException("Key file '" + fileLocation + "' contained an incorrect about of data");
+                }
+                SecretKeySpec keySpec = new SecretKeySpec(key, AES_ALGORITHM);
+                return new AESKeyFileEncrypter(keySpec);
+            }
+        }
+        catch (IOException e)
+        {
+            throw new IllegalConfigurationException("Unable to get file permissions: " + e.getMessage(), e);

Review comment:
       please use String#format for exception message

##########
File path: broker-core/src/main/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypterFactory.java
##########
@@ -0,0 +1,383 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.AclEntry;
+import java.nio.file.attribute.AclEntryPermission;
+import java.nio.file.attribute.AclEntryType;
+import java.nio.file.attribute.AclFileAttributeView;
+import java.nio.file.attribute.FileAttribute;
+import java.nio.file.attribute.PosixFileAttributeView;
+import java.nio.file.attribute.PosixFilePermission;
+import java.nio.file.attribute.PosixFilePermissions;
+import java.nio.file.attribute.UserPrincipal;
+import java.security.NoSuchAlgorithmException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.EnumSet;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.ListIterator;
+import java.util.Map;
+import java.util.Set;
+
+import javax.crypto.Cipher;
+import javax.crypto.KeyGenerator;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.SecretKeySpec;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.qpid.server.configuration.IllegalConfigurationException;
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.SystemConfig;
+import org.apache.qpid.server.plugin.ConditionallyAvailable;
+import org.apache.qpid.server.plugin.ConfigurationSecretEncrypterFactory;
+import org.apache.qpid.server.plugin.PluggableService;
+
+@PluggableService
+public class AESGCMKeyFileEncrypterFactory implements ConfigurationSecretEncrypterFactory, ConditionallyAvailable
+{
+    private static final Logger LOGGER = LoggerFactory.getLogger(AESGCMKeyFileEncrypterFactory.class);
+
+    static final String ENCRYPTER_KEY_FILE = "encrypter.key.file";
+
+    private static final int AES_KEY_SIZE_BITS = 256;
+    private static final int AES_KEY_SIZE_BYTES = AES_KEY_SIZE_BITS / 8;
+    private static final String AES_ALGORITHM = "AES";
+
+    public static final String TYPE = "AESGCMKeyFile";
+
+    static final String DEFAULT_KEYS_SUBDIR_NAME = ".keys";
+
+    private static final boolean IS_AVAILABLE;
+
+    private static final String ILLEGAL_ARG_EXCEPTION = "Unable to determine a mechanism to protect access to the key file on this filesystem";
+
+    static

Review comment:
       This is a common code and it needs to be moved into a method

##########
File path: broker-core/src/main/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypter.java
##########
@@ -0,0 +1,118 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.security.InvalidAlgorithmParameterException;
+import java.security.InvalidKeyException;
+import java.security.NoSuchAlgorithmException;
+import java.security.SecureRandom;
+import java.util.Base64;
+
+import javax.crypto.Cipher;
+import javax.crypto.CipherInputStream;
+import javax.crypto.NoSuchPaddingException;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.GCMParameterSpec;
+import javax.crypto.spec.IvParameterSpec;
+import javax.crypto.spec.SecretKeySpec;
+
+import org.apache.qpid.server.plugin.PluggableService;
+import org.apache.qpid.server.util.Strings;
+
+class AESGCMKeyFileEncrypter implements ConfigurationSecretEncrypter
+{
+    private static final String CIPHER_NAME = "AES/GCM/NoPadding";
+    private static final int GCM_INITIALIZATION_VECTOR_LENGTH = 12;
+    private static final int GCM_TAG_LENGTH = 128;
+    private static final String AES_ALGORITHM = "AES";
+    private final SecureRandom _random = new SecureRandom();
+    private final SecretKey _secretKey;
+
+    AESGCMKeyFileEncrypter(SecretKey secretKey)
+    {
+        if(secretKey == null)
+        {
+            throw new NullPointerException("A non null secret key must be supplied");
+        }
+        if(!AES_ALGORITHM.equals(secretKey.getAlgorithm()))
+        {
+            throw new IllegalArgumentException("Provided secret key was for the algorithm: " + secretKey.getAlgorithm()
+                                               + " when " + AES_ALGORITHM + " was needed.");
+        }
+        _secretKey = secretKey;
+    }
+
+    @Override
+    public String encrypt(final String unencrypted)
+    {
+        byte[] unencryptedBytes = unencrypted.getBytes(StandardCharsets.UTF_8);
+        try
+        {
+            byte[] ivbytes = new byte[GCM_INITIALIZATION_VECTOR_LENGTH];
+            _random.nextBytes(ivbytes);
+            Cipher cipher = Cipher.getInstance(CIPHER_NAME);
+            GCMParameterSpec gcmParameterSpec = new GCMParameterSpec(GCM_TAG_LENGTH, ivbytes);
+            cipher.init(Cipher.ENCRYPT_MODE, _secretKey, gcmParameterSpec);
+            byte[] encryptedBytes = EncryptionHelper.readFromCipherStream(unencryptedBytes, cipher);
+            byte[] output = new byte[GCM_INITIALIZATION_VECTOR_LENGTH + encryptedBytes.length];
+            System.arraycopy(ivbytes, 0, output, 0, GCM_INITIALIZATION_VECTOR_LENGTH);
+            System.arraycopy(encryptedBytes, 0, output, GCM_INITIALIZATION_VECTOR_LENGTH, encryptedBytes.length);
+            return Base64.getEncoder().encodeToString(output);
+        }
+        catch (IOException | InvalidAlgorithmParameterException | InvalidKeyException | NoSuchAlgorithmException | NoSuchPaddingException e)
+        {
+            throw new IllegalArgumentException("Unable to encrypt secret", e);
+        }
+    }
+
+    @Override
+    public String decrypt(final String encrypted)
+    {
+        if (!EncryptionHelper.isValidBase64(encrypted))
+        {
+            throw new IllegalArgumentException("Encrypted value is not valid Base 64 data: '" + encrypted + "'");
+        }
+        byte[] encryptedBytes = Strings.decodeBase64(encrypted);
+        try
+        {
+            Cipher cipher = Cipher.getInstance(CIPHER_NAME);
+            byte[] ivbytes = new byte[GCM_INITIALIZATION_VECTOR_LENGTH];
+            if(encryptedBytes.length > 0)

Review comment:
       if encryptedBytes.length is zero, it looks like a defect to me. I think that exception should be thrown...
   I suppose that intention here was to check whether length is greater than GCM_INITIALIZATION_VECTOR_LENGTH
   
   Though, if it is less than GCM_INITIALIZATION_VECTOR_LENGTH, an exception should be thrown
   

##########
File path: broker-core/src/main/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypterFactory.java
##########
@@ -0,0 +1,383 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.AclEntry;
+import java.nio.file.attribute.AclEntryPermission;
+import java.nio.file.attribute.AclEntryType;
+import java.nio.file.attribute.AclFileAttributeView;
+import java.nio.file.attribute.FileAttribute;
+import java.nio.file.attribute.PosixFileAttributeView;
+import java.nio.file.attribute.PosixFilePermission;
+import java.nio.file.attribute.PosixFilePermissions;
+import java.nio.file.attribute.UserPrincipal;
+import java.security.NoSuchAlgorithmException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.EnumSet;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.ListIterator;
+import java.util.Map;
+import java.util.Set;
+
+import javax.crypto.Cipher;
+import javax.crypto.KeyGenerator;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.SecretKeySpec;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.qpid.server.configuration.IllegalConfigurationException;
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.SystemConfig;
+import org.apache.qpid.server.plugin.ConditionallyAvailable;
+import org.apache.qpid.server.plugin.ConfigurationSecretEncrypterFactory;
+import org.apache.qpid.server.plugin.PluggableService;
+
+@PluggableService
+public class AESGCMKeyFileEncrypterFactory implements ConfigurationSecretEncrypterFactory, ConditionallyAvailable
+{
+    private static final Logger LOGGER = LoggerFactory.getLogger(AESGCMKeyFileEncrypterFactory.class);
+
+    static final String ENCRYPTER_KEY_FILE = "encrypter.key.file";
+
+    private static final int AES_KEY_SIZE_BITS = 256;
+    private static final int AES_KEY_SIZE_BYTES = AES_KEY_SIZE_BITS / 8;
+    private static final String AES_ALGORITHM = "AES";
+
+    public static final String TYPE = "AESGCMKeyFile";
+
+    static final String DEFAULT_KEYS_SUBDIR_NAME = ".keys";
+
+    private static final boolean IS_AVAILABLE;
+
+    private static final String ILLEGAL_ARG_EXCEPTION = "Unable to determine a mechanism to protect access to the key file on this filesystem";
+
+    static
+    {
+        boolean isAvailable;
+        try
+        {
+            final int allowedKeyLength = Cipher.getMaxAllowedKeyLength(AES_ALGORITHM);
+            isAvailable = allowedKeyLength >=AES_KEY_SIZE_BITS;
+            if(!isAvailable)
+            {
+                LOGGER.warn("The {} configuration encryption encryption mechanism is not available. "
+                            + "Maximum available AES key length is {} but {} is required. "
+                            + "Ensure the full strength JCE policy has been installed into your JVM.", TYPE, allowedKeyLength, AES_KEY_SIZE_BITS);
+            }
+        }
+        catch (NoSuchAlgorithmException e)
+        {
+            isAvailable = false;
+
+            LOGGER.error("The " + TYPE + " configuration encryption encryption mechanism is not available. "
+                        + "The " + AES_ALGORITHM + " algorithm is not available within the JVM (despite it being a requirement).");
+        }
+
+        IS_AVAILABLE = isAvailable;
+    }
+
+    @Override
+    public ConfigurationSecretEncrypter createEncrypter(final ConfiguredObject<?> object)
+    {
+        String fileLocation;
+        if(object.getContextKeys(false).contains(ENCRYPTER_KEY_FILE))
+        {
+            fileLocation = object.getContextValue(String.class, ENCRYPTER_KEY_FILE);
+        }
+        else
+        {
+
+            fileLocation = object.getContextValue(String.class, SystemConfig.QPID_WORK_DIR)
+                           + File.separator + DEFAULT_KEYS_SUBDIR_NAME + File.separator
+                           + object.getCategoryClass().getSimpleName() + "_"
+                           + object.getName() + ".key";
+
+            Map<String, String> context = object.getContext();
+            Map<String, String> modifiedContext = new LinkedHashMap<>(context);
+            modifiedContext.put(ENCRYPTER_KEY_FILE, fileLocation);
+
+            object.setAttributes(Collections.<String, Object>singletonMap(ConfiguredObject.CONTEXT, modifiedContext));
+        }
+        File file = new File(fileLocation);
+        if(!file.exists())
+        {
+            LOGGER.warn("Configuration encryption is enabled, but no configuration secret was found. A new configuration secret will be created at '{}'.", fileLocation);
+            createAndPopulateKeyFile(file);
+        }
+        if(!file.isFile())
+        {
+            throw new IllegalArgumentException("File '"+fileLocation+"' is not a regular file.");
+        }
+        try
+        {
+            checkFilePermissions(fileLocation, file);
+            if(Files.size(file.toPath()) != AES_KEY_SIZE_BYTES)
+            {
+                throw new IllegalArgumentException("Key file '" + fileLocation + "' contains an incorrect about of data");
+            }
+
+            try(FileInputStream inputStream = new FileInputStream(file))
+            {
+                byte[] key = new byte[AES_KEY_SIZE_BYTES];
+                int pos = 0;
+                int read;
+                while(pos < key.length && -1 != ( read = inputStream.read(key, pos, key.length - pos)))
+                {
+                    pos += read;
+                }
+                if(pos != key.length)
+                {
+                    throw new IllegalConfigurationException("Key file '" + fileLocation + "' contained an incorrect about of data");
+                }
+                SecretKeySpec keySpec = new SecretKeySpec(key, AES_ALGORITHM);
+                return new AESKeyFileEncrypter(keySpec);
+            }
+        }
+        catch (IOException e)
+        {
+            throw new IllegalConfigurationException("Unable to get file permissions: " + e.getMessage(), e);
+        }
+    }
+
+    private void checkFilePermissions(String fileLocation, File file) throws IOException
+    {
+        if(isPosixFileSystem(file))
+        {
+            Set<PosixFilePermission> permissions = Files.getPosixFilePermissions(file.toPath());
+
+            if (permissions.contains(PosixFilePermission.GROUP_READ)
+                    || permissions.contains(PosixFilePermission.OTHERS_READ)
+                    || permissions.contains(PosixFilePermission.GROUP_WRITE)
+                    || permissions.contains(PosixFilePermission.OTHERS_WRITE)) {
+                throw new IllegalArgumentException("Key file '"
+                        + fileLocation
+                        + "' has incorrect permissions.  Only the owner "
+                        + "should be able to read or write this file.");
+            }
+        }
+        else if(isAclFileSystem(file))
+        {
+            AclFileAttributeView attributeView = Files.getFileAttributeView(file.toPath(), AclFileAttributeView.class);
+            ArrayList<AclEntry> acls = new ArrayList<>(attributeView.getAcl());
+            ListIterator<AclEntry> iter = acls.listIterator();
+            UserPrincipal owner = Files.getOwner(file.toPath());
+            while(iter.hasNext())
+            {
+                AclEntry acl = iter.next();
+                if(acl.type() == AclEntryType.ALLOW)
+                {
+                    Set<AclEntryPermission> originalPermissions = acl.permissions();
+                    Set<AclEntryPermission> updatedPermissions = EnumSet.copyOf(originalPermissions);
+
+
+                    if (updatedPermissions.removeAll(EnumSet.of(AclEntryPermission.APPEND_DATA,

Review comment:
       why we are updating existing file permissions?
   I think it is not broker concern

##########
File path: broker-core/src/main/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypterFactory.java
##########
@@ -0,0 +1,383 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.AclEntry;
+import java.nio.file.attribute.AclEntryPermission;
+import java.nio.file.attribute.AclEntryType;
+import java.nio.file.attribute.AclFileAttributeView;
+import java.nio.file.attribute.FileAttribute;
+import java.nio.file.attribute.PosixFileAttributeView;
+import java.nio.file.attribute.PosixFilePermission;
+import java.nio.file.attribute.PosixFilePermissions;
+import java.nio.file.attribute.UserPrincipal;
+import java.security.NoSuchAlgorithmException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.EnumSet;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.ListIterator;
+import java.util.Map;
+import java.util.Set;
+
+import javax.crypto.Cipher;
+import javax.crypto.KeyGenerator;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.SecretKeySpec;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.qpid.server.configuration.IllegalConfigurationException;
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.SystemConfig;
+import org.apache.qpid.server.plugin.ConditionallyAvailable;
+import org.apache.qpid.server.plugin.ConfigurationSecretEncrypterFactory;
+import org.apache.qpid.server.plugin.PluggableService;
+
+@PluggableService
+public class AESGCMKeyFileEncrypterFactory implements ConfigurationSecretEncrypterFactory, ConditionallyAvailable
+{
+    private static final Logger LOGGER = LoggerFactory.getLogger(AESGCMKeyFileEncrypterFactory.class);
+
+    static final String ENCRYPTER_KEY_FILE = "encrypter.key.file";
+
+    private static final int AES_KEY_SIZE_BITS = 256;
+    private static final int AES_KEY_SIZE_BYTES = AES_KEY_SIZE_BITS / 8;
+    private static final String AES_ALGORITHM = "AES";
+
+    public static final String TYPE = "AESGCMKeyFile";
+
+    static final String DEFAULT_KEYS_SUBDIR_NAME = ".keys";
+
+    private static final boolean IS_AVAILABLE;
+
+    private static final String ILLEGAL_ARG_EXCEPTION = "Unable to determine a mechanism to protect access to the key file on this filesystem";
+
+    static
+    {
+        boolean isAvailable;
+        try
+        {
+            final int allowedKeyLength = Cipher.getMaxAllowedKeyLength(AES_ALGORITHM);
+            isAvailable = allowedKeyLength >=AES_KEY_SIZE_BITS;
+            if(!isAvailable)
+            {
+                LOGGER.warn("The {} configuration encryption encryption mechanism is not available. "
+                            + "Maximum available AES key length is {} but {} is required. "
+                            + "Ensure the full strength JCE policy has been installed into your JVM.", TYPE, allowedKeyLength, AES_KEY_SIZE_BITS);
+            }
+        }
+        catch (NoSuchAlgorithmException e)
+        {
+            isAvailable = false;
+
+            LOGGER.error("The " + TYPE + " configuration encryption encryption mechanism is not available. "
+                        + "The " + AES_ALGORITHM + " algorithm is not available within the JVM (despite it being a requirement).");
+        }
+
+        IS_AVAILABLE = isAvailable;
+    }
+
+    @Override
+    public ConfigurationSecretEncrypter createEncrypter(final ConfiguredObject<?> object)
+    {
+        String fileLocation;
+        if(object.getContextKeys(false).contains(ENCRYPTER_KEY_FILE))
+        {
+            fileLocation = object.getContextValue(String.class, ENCRYPTER_KEY_FILE);
+        }
+        else
+        {
+
+            fileLocation = object.getContextValue(String.class, SystemConfig.QPID_WORK_DIR)
+                           + File.separator + DEFAULT_KEYS_SUBDIR_NAME + File.separator
+                           + object.getCategoryClass().getSimpleName() + "_"
+                           + object.getName() + ".key";
+
+            Map<String, String> context = object.getContext();
+            Map<String, String> modifiedContext = new LinkedHashMap<>(context);
+            modifiedContext.put(ENCRYPTER_KEY_FILE, fileLocation);
+
+            object.setAttributes(Collections.<String, Object>singletonMap(ConfiguredObject.CONTEXT, modifiedContext));
+        }
+        File file = new File(fileLocation);
+        if(!file.exists())
+        {
+            LOGGER.warn("Configuration encryption is enabled, but no configuration secret was found. A new configuration secret will be created at '{}'.", fileLocation);
+            createAndPopulateKeyFile(file);
+        }
+        if(!file.isFile())
+        {
+            throw new IllegalArgumentException("File '"+fileLocation+"' is not a regular file.");
+        }
+        try
+        {
+            checkFilePermissions(fileLocation, file);
+            if(Files.size(file.toPath()) != AES_KEY_SIZE_BYTES)
+            {
+                throw new IllegalArgumentException("Key file '" + fileLocation + "' contains an incorrect about of data");
+            }
+
+            try(FileInputStream inputStream = new FileInputStream(file))
+            {
+                byte[] key = new byte[AES_KEY_SIZE_BYTES];
+                int pos = 0;
+                int read;
+                while(pos < key.length && -1 != ( read = inputStream.read(key, pos, key.length - pos)))
+                {
+                    pos += read;
+                }
+                if(pos != key.length)
+                {
+                    throw new IllegalConfigurationException("Key file '" + fileLocation + "' contained an incorrect about of data");

Review comment:
       please use String#format for exception message

##########
File path: broker-core/src/main/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypter.java
##########
@@ -0,0 +1,118 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.security.InvalidAlgorithmParameterException;
+import java.security.InvalidKeyException;
+import java.security.NoSuchAlgorithmException;
+import java.security.SecureRandom;
+import java.util.Base64;
+
+import javax.crypto.Cipher;
+import javax.crypto.CipherInputStream;
+import javax.crypto.NoSuchPaddingException;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.GCMParameterSpec;
+import javax.crypto.spec.IvParameterSpec;
+import javax.crypto.spec.SecretKeySpec;
+
+import org.apache.qpid.server.plugin.PluggableService;
+import org.apache.qpid.server.util.Strings;
+
+class AESGCMKeyFileEncrypter implements ConfigurationSecretEncrypter
+{
+    private static final String CIPHER_NAME = "AES/GCM/NoPadding";
+    private static final int GCM_INITIALIZATION_VECTOR_LENGTH = 12;
+    private static final int GCM_TAG_LENGTH = 128;
+    private static final String AES_ALGORITHM = "AES";
+    private final SecureRandom _random = new SecureRandom();
+    private final SecretKey _secretKey;
+
+    AESGCMKeyFileEncrypter(SecretKey secretKey)
+    {
+        if(secretKey == null)
+        {
+            throw new NullPointerException("A non null secret key must be supplied");

Review comment:
       why NullPointerException is thrown here? Should it be IllegalArgumentException?

##########
File path: broker-core/src/main/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypterFactory.java
##########
@@ -0,0 +1,383 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.AclEntry;
+import java.nio.file.attribute.AclEntryPermission;
+import java.nio.file.attribute.AclEntryType;
+import java.nio.file.attribute.AclFileAttributeView;
+import java.nio.file.attribute.FileAttribute;
+import java.nio.file.attribute.PosixFileAttributeView;
+import java.nio.file.attribute.PosixFilePermission;
+import java.nio.file.attribute.PosixFilePermissions;
+import java.nio.file.attribute.UserPrincipal;
+import java.security.NoSuchAlgorithmException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.EnumSet;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.ListIterator;
+import java.util.Map;
+import java.util.Set;
+
+import javax.crypto.Cipher;
+import javax.crypto.KeyGenerator;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.SecretKeySpec;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.qpid.server.configuration.IllegalConfigurationException;
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.SystemConfig;
+import org.apache.qpid.server.plugin.ConditionallyAvailable;
+import org.apache.qpid.server.plugin.ConfigurationSecretEncrypterFactory;
+import org.apache.qpid.server.plugin.PluggableService;
+
+@PluggableService
+public class AESGCMKeyFileEncrypterFactory implements ConfigurationSecretEncrypterFactory, ConditionallyAvailable
+{
+    private static final Logger LOGGER = LoggerFactory.getLogger(AESGCMKeyFileEncrypterFactory.class);
+
+    static final String ENCRYPTER_KEY_FILE = "encrypter.key.file";
+
+    private static final int AES_KEY_SIZE_BITS = 256;
+    private static final int AES_KEY_SIZE_BYTES = AES_KEY_SIZE_BITS / 8;
+    private static final String AES_ALGORITHM = "AES";
+
+    public static final String TYPE = "AESGCMKeyFile";
+
+    static final String DEFAULT_KEYS_SUBDIR_NAME = ".keys";
+
+    private static final boolean IS_AVAILABLE;
+
+    private static final String ILLEGAL_ARG_EXCEPTION = "Unable to determine a mechanism to protect access to the key file on this filesystem";
+
+    static
+    {
+        boolean isAvailable;
+        try
+        {
+            final int allowedKeyLength = Cipher.getMaxAllowedKeyLength(AES_ALGORITHM);
+            isAvailable = allowedKeyLength >=AES_KEY_SIZE_BITS;
+            if(!isAvailable)
+            {
+                LOGGER.warn("The {} configuration encryption encryption mechanism is not available. "
+                            + "Maximum available AES key length is {} but {} is required. "
+                            + "Ensure the full strength JCE policy has been installed into your JVM.", TYPE, allowedKeyLength, AES_KEY_SIZE_BITS);
+            }
+        }
+        catch (NoSuchAlgorithmException e)
+        {
+            isAvailable = false;
+
+            LOGGER.error("The " + TYPE + " configuration encryption encryption mechanism is not available. "
+                        + "The " + AES_ALGORITHM + " algorithm is not available within the JVM (despite it being a requirement).");
+        }
+
+        IS_AVAILABLE = isAvailable;
+    }
+
+    @Override
+    public ConfigurationSecretEncrypter createEncrypter(final ConfiguredObject<?> object)
+    {
+        String fileLocation;
+        if(object.getContextKeys(false).contains(ENCRYPTER_KEY_FILE))
+        {
+            fileLocation = object.getContextValue(String.class, ENCRYPTER_KEY_FILE);
+        }
+        else
+        {
+
+            fileLocation = object.getContextValue(String.class, SystemConfig.QPID_WORK_DIR)
+                           + File.separator + DEFAULT_KEYS_SUBDIR_NAME + File.separator
+                           + object.getCategoryClass().getSimpleName() + "_"
+                           + object.getName() + ".key";
+
+            Map<String, String> context = object.getContext();
+            Map<String, String> modifiedContext = new LinkedHashMap<>(context);
+            modifiedContext.put(ENCRYPTER_KEY_FILE, fileLocation);
+
+            object.setAttributes(Collections.<String, Object>singletonMap(ConfiguredObject.CONTEXT, modifiedContext));
+        }
+        File file = new File(fileLocation);
+        if(!file.exists())
+        {
+            LOGGER.warn("Configuration encryption is enabled, but no configuration secret was found. A new configuration secret will be created at '{}'.", fileLocation);
+            createAndPopulateKeyFile(file);
+        }
+        if(!file.isFile())
+        {
+            throw new IllegalArgumentException("File '"+fileLocation+"' is not a regular file.");
+        }
+        try
+        {
+            checkFilePermissions(fileLocation, file);
+            if(Files.size(file.toPath()) != AES_KEY_SIZE_BYTES)
+            {
+                throw new IllegalArgumentException("Key file '" + fileLocation + "' contains an incorrect about of data");
+            }
+
+            try(FileInputStream inputStream = new FileInputStream(file))
+            {
+                byte[] key = new byte[AES_KEY_SIZE_BYTES];
+                int pos = 0;
+                int read;
+                while(pos < key.length && -1 != ( read = inputStream.read(key, pos, key.length - pos)))
+                {
+                    pos += read;
+                }
+                if(pos != key.length)
+                {
+                    throw new IllegalConfigurationException("Key file '" + fileLocation + "' contained an incorrect about of data");
+                }
+                SecretKeySpec keySpec = new SecretKeySpec(key, AES_ALGORITHM);
+                return new AESKeyFileEncrypter(keySpec);
+            }
+        }
+        catch (IOException e)
+        {
+            throw new IllegalConfigurationException("Unable to get file permissions: " + e.getMessage(), e);
+        }
+    }
+
+    private void checkFilePermissions(String fileLocation, File file) throws IOException
+    {
+        if(isPosixFileSystem(file))
+        {
+            Set<PosixFilePermission> permissions = Files.getPosixFilePermissions(file.toPath());
+
+            if (permissions.contains(PosixFilePermission.GROUP_READ)
+                    || permissions.contains(PosixFilePermission.OTHERS_READ)
+                    || permissions.contains(PosixFilePermission.GROUP_WRITE)
+                    || permissions.contains(PosixFilePermission.OTHERS_WRITE)) {
+                throw new IllegalArgumentException("Key file '"
+                        + fileLocation
+                        + "' has incorrect permissions.  Only the owner "
+                        + "should be able to read or write this file.");
+            }
+        }
+        else if(isAclFileSystem(file))
+        {
+            AclFileAttributeView attributeView = Files.getFileAttributeView(file.toPath(), AclFileAttributeView.class);
+            ArrayList<AclEntry> acls = new ArrayList<>(attributeView.getAcl());
+            ListIterator<AclEntry> iter = acls.listIterator();
+            UserPrincipal owner = Files.getOwner(file.toPath());
+            while(iter.hasNext())
+            {
+                AclEntry acl = iter.next();
+                if(acl.type() == AclEntryType.ALLOW)
+                {
+                    Set<AclEntryPermission> originalPermissions = acl.permissions();
+                    Set<AclEntryPermission> updatedPermissions = EnumSet.copyOf(originalPermissions);
+
+
+                    if (updatedPermissions.removeAll(EnumSet.of(AclEntryPermission.APPEND_DATA,
+                            AclEntryPermission.EXECUTE,
+                            AclEntryPermission.WRITE_ACL,
+                            AclEntryPermission.WRITE_DATA,
+                            AclEntryPermission.WRITE_OWNER))) {
+                        throw new IllegalArgumentException("Key file '"
+                                + fileLocation
+                                + "' has incorrect permissions.  The file should not be modifiable by any user.");
+                    }
+                    if (!owner.equals(acl.principal()) && updatedPermissions.removeAll(EnumSet.of(AclEntryPermission.READ_DATA))) {
+                        throw new IllegalArgumentException("Key file '"
+                                + fileLocation
+                                + "' has incorrect permissions.  Only the owner should be able to read from the file.");
+                    }
+                }
+            }
+        }
+        else
+        {
+            throw new IllegalArgumentException(ILLEGAL_ARG_EXCEPTION);
+        }
+    }
+
+    private boolean isPosixFileSystem(File file) throws IOException
+    {
+        return Files.getFileAttributeView(file.toPath(), PosixFileAttributeView.class) != null;
+    }
+
+    private boolean isAclFileSystem(File file) throws IOException
+    {
+        return Files.getFileAttributeView(file.toPath(), AclFileAttributeView.class) != null;
+    }
+
+
+    private void createAndPopulateKeyFile(final File file)
+    {
+        try
+        {
+            createEmptyKeyFile(file);
+
+            KeyGenerator keyGenerator = KeyGenerator.getInstance(AES_ALGORITHM);
+            keyGenerator.init(AES_KEY_SIZE_BITS);
+            SecretKey key = keyGenerator.generateKey();
+            try(FileOutputStream os = new FileOutputStream(file))
+            {
+                os.write(key.getEncoded());
+            }
+
+            makeKeyFileReadOnly(file);
+        }
+        catch (NoSuchAlgorithmException | IOException e)
+        {
+            throw new IllegalArgumentException("Cannot create key file: " + e.getMessage(), e);

Review comment:
       I am not sure that IllegalArgumentException is a right exception  type

##########
File path: broker-core/src/main/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypterFactory.java
##########
@@ -0,0 +1,383 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.AclEntry;
+import java.nio.file.attribute.AclEntryPermission;
+import java.nio.file.attribute.AclEntryType;
+import java.nio.file.attribute.AclFileAttributeView;
+import java.nio.file.attribute.FileAttribute;
+import java.nio.file.attribute.PosixFileAttributeView;
+import java.nio.file.attribute.PosixFilePermission;
+import java.nio.file.attribute.PosixFilePermissions;
+import java.nio.file.attribute.UserPrincipal;
+import java.security.NoSuchAlgorithmException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.EnumSet;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.ListIterator;
+import java.util.Map;
+import java.util.Set;
+
+import javax.crypto.Cipher;
+import javax.crypto.KeyGenerator;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.SecretKeySpec;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.qpid.server.configuration.IllegalConfigurationException;
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.SystemConfig;
+import org.apache.qpid.server.plugin.ConditionallyAvailable;
+import org.apache.qpid.server.plugin.ConfigurationSecretEncrypterFactory;
+import org.apache.qpid.server.plugin.PluggableService;
+
+@PluggableService
+public class AESGCMKeyFileEncrypterFactory implements ConfigurationSecretEncrypterFactory, ConditionallyAvailable
+{
+    private static final Logger LOGGER = LoggerFactory.getLogger(AESGCMKeyFileEncrypterFactory.class);
+
+    static final String ENCRYPTER_KEY_FILE = "encrypter.key.file";
+
+    private static final int AES_KEY_SIZE_BITS = 256;
+    private static final int AES_KEY_SIZE_BYTES = AES_KEY_SIZE_BITS / 8;
+    private static final String AES_ALGORITHM = "AES";
+
+    public static final String TYPE = "AESGCMKeyFile";
+
+    static final String DEFAULT_KEYS_SUBDIR_NAME = ".keys";
+
+    private static final boolean IS_AVAILABLE;
+
+    private static final String ILLEGAL_ARG_EXCEPTION = "Unable to determine a mechanism to protect access to the key file on this filesystem";
+
+    static
+    {
+        boolean isAvailable;
+        try
+        {
+            final int allowedKeyLength = Cipher.getMaxAllowedKeyLength(AES_ALGORITHM);
+            isAvailable = allowedKeyLength >=AES_KEY_SIZE_BITS;
+            if(!isAvailable)
+            {
+                LOGGER.warn("The {} configuration encryption encryption mechanism is not available. "
+                            + "Maximum available AES key length is {} but {} is required. "
+                            + "Ensure the full strength JCE policy has been installed into your JVM.", TYPE, allowedKeyLength, AES_KEY_SIZE_BITS);
+            }
+        }
+        catch (NoSuchAlgorithmException e)
+        {
+            isAvailable = false;
+
+            LOGGER.error("The " + TYPE + " configuration encryption encryption mechanism is not available. "
+                        + "The " + AES_ALGORITHM + " algorithm is not available within the JVM (despite it being a requirement).");
+        }
+
+        IS_AVAILABLE = isAvailable;
+    }
+
+    @Override
+    public ConfigurationSecretEncrypter createEncrypter(final ConfiguredObject<?> object)
+    {
+        String fileLocation;
+        if(object.getContextKeys(false).contains(ENCRYPTER_KEY_FILE))
+        {
+            fileLocation = object.getContextValue(String.class, ENCRYPTER_KEY_FILE);
+        }
+        else
+        {
+
+            fileLocation = object.getContextValue(String.class, SystemConfig.QPID_WORK_DIR)
+                           + File.separator + DEFAULT_KEYS_SUBDIR_NAME + File.separator
+                           + object.getCategoryClass().getSimpleName() + "_"
+                           + object.getName() + ".key";
+
+            Map<String, String> context = object.getContext();
+            Map<String, String> modifiedContext = new LinkedHashMap<>(context);
+            modifiedContext.put(ENCRYPTER_KEY_FILE, fileLocation);
+
+            object.setAttributes(Collections.<String, Object>singletonMap(ConfiguredObject.CONTEXT, modifiedContext));
+        }
+        File file = new File(fileLocation);
+        if(!file.exists())
+        {
+            LOGGER.warn("Configuration encryption is enabled, but no configuration secret was found. A new configuration secret will be created at '{}'.", fileLocation);
+            createAndPopulateKeyFile(file);
+        }
+        if(!file.isFile())
+        {
+            throw new IllegalArgumentException("File '"+fileLocation+"' is not a regular file.");
+        }
+        try
+        {
+            checkFilePermissions(fileLocation, file);
+            if(Files.size(file.toPath()) != AES_KEY_SIZE_BYTES)
+            {
+                throw new IllegalArgumentException("Key file '" + fileLocation + "' contains an incorrect about of data");
+            }
+
+            try(FileInputStream inputStream = new FileInputStream(file))
+            {
+                byte[] key = new byte[AES_KEY_SIZE_BYTES];
+                int pos = 0;
+                int read;
+                while(pos < key.length && -1 != ( read = inputStream.read(key, pos, key.length - pos)))
+                {
+                    pos += read;
+                }
+                if(pos != key.length)
+                {
+                    throw new IllegalConfigurationException("Key file '" + fileLocation + "' contained an incorrect about of data");
+                }
+                SecretKeySpec keySpec = new SecretKeySpec(key, AES_ALGORITHM);
+                return new AESKeyFileEncrypter(keySpec);
+            }
+        }
+        catch (IOException e)
+        {
+            throw new IllegalConfigurationException("Unable to get file permissions: " + e.getMessage(), e);
+        }
+    }
+
+    private void checkFilePermissions(String fileLocation, File file) throws IOException
+    {
+        if(isPosixFileSystem(file))
+        {
+            Set<PosixFilePermission> permissions = Files.getPosixFilePermissions(file.toPath());
+
+            if (permissions.contains(PosixFilePermission.GROUP_READ)
+                    || permissions.contains(PosixFilePermission.OTHERS_READ)
+                    || permissions.contains(PosixFilePermission.GROUP_WRITE)
+                    || permissions.contains(PosixFilePermission.OTHERS_WRITE)) {
+                throw new IllegalArgumentException("Key file '"
+                        + fileLocation
+                        + "' has incorrect permissions.  Only the owner "
+                        + "should be able to read or write this file.");
+            }
+        }
+        else if(isAclFileSystem(file))
+        {
+            AclFileAttributeView attributeView = Files.getFileAttributeView(file.toPath(), AclFileAttributeView.class);
+            ArrayList<AclEntry> acls = new ArrayList<>(attributeView.getAcl());
+            ListIterator<AclEntry> iter = acls.listIterator();
+            UserPrincipal owner = Files.getOwner(file.toPath());
+            while(iter.hasNext())
+            {
+                AclEntry acl = iter.next();
+                if(acl.type() == AclEntryType.ALLOW)
+                {
+                    Set<AclEntryPermission> originalPermissions = acl.permissions();
+                    Set<AclEntryPermission> updatedPermissions = EnumSet.copyOf(originalPermissions);
+
+
+                    if (updatedPermissions.removeAll(EnumSet.of(AclEntryPermission.APPEND_DATA,
+                            AclEntryPermission.EXECUTE,
+                            AclEntryPermission.WRITE_ACL,
+                            AclEntryPermission.WRITE_DATA,
+                            AclEntryPermission.WRITE_OWNER))) {
+                        throw new IllegalArgumentException("Key file '"
+                                + fileLocation
+                                + "' has incorrect permissions.  The file should not be modifiable by any user.");
+                    }
+                    if (!owner.equals(acl.principal()) && updatedPermissions.removeAll(EnumSet.of(AclEntryPermission.READ_DATA))) {
+                        throw new IllegalArgumentException("Key file '"
+                                + fileLocation
+                                + "' has incorrect permissions.  Only the owner should be able to read from the file.");
+                    }
+                }
+            }
+        }
+        else
+        {
+            throw new IllegalArgumentException(ILLEGAL_ARG_EXCEPTION);
+        }
+    }
+
+    private boolean isPosixFileSystem(File file) throws IOException
+    {
+        return Files.getFileAttributeView(file.toPath(), PosixFileAttributeView.class) != null;
+    }
+
+    private boolean isAclFileSystem(File file) throws IOException
+    {
+        return Files.getFileAttributeView(file.toPath(), AclFileAttributeView.class) != null;
+    }
+
+
+    private void createAndPopulateKeyFile(final File file)

Review comment:
       this is a common code

##########
File path: broker-core/src/test/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypterTest.java
##########
@@ -0,0 +1,364 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import static org.mockito.Mockito.mock;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.SecureRandom;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Random;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+
+import javax.crypto.SecretKeyFactory;
+import javax.crypto.spec.PBEKeySpec;
+import javax.crypto.spec.SecretKeySpec;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.util.concurrent.SettableFuture;
+import org.junit.Before;
+import org.junit.Test;
+
+import org.apache.qpid.server.SystemLauncher;
+import org.apache.qpid.server.SystemLauncherListener;
+import org.apache.qpid.server.model.AuthenticationProvider;
+import org.apache.qpid.server.model.Broker;
+import org.apache.qpid.server.model.BrokerModel;
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.JsonSystemConfigImpl;
+import org.apache.qpid.server.model.SystemConfig;
+import org.apache.qpid.server.model.User;
+import org.apache.qpid.server.util.FileUtils;
+import org.apache.qpid.test.utils.UnitTestBase;
+
+public class AESGCMKeyFileEncrypterTest extends UnitTestBase
+{
+    private static final String PLAIN_SECRET = "secret";
+    private final SecureRandom _random = new SecureRandom();
+    public static final String PLAINTEXT = "guestkey";
+    private Path _configurationLocation;
+    private Path _workDir;
+    private Broker<?> _broker;
+    private SystemLauncher _systemLauncher;
+    private Map<String, Object> attributes = new HashMap<>();
+
+    private void createBrokerWithEncrypter(final Object encrptionType) throws Exception
+    {
+        try
+        {
+            _workDir = Files.createTempDirectory("qpid_work_dir");
+            _configurationLocation = Files.createTempFile(_workDir, "config", ".json");
+            Map<String, Object> context = new HashMap<>();
+            context.put("qpid.work_dir", _workDir.toFile().getAbsolutePath());
+            Map<String, Object> config = new HashMap<>();
+            config.put(ConfiguredObject.NAME, getTestName());
+            config.put(Broker.MODEL_VERSION, BrokerModel.MODEL_VERSION);
+            config.put(Broker.CONFIDENTIAL_CONFIGURATION_ENCRYPTION_PROVIDER, encrptionType);
+            new ObjectMapper().writeValue(_configurationLocation.toFile(), config);
+
+            attributes.put("storePath", _configurationLocation.toFile().getAbsolutePath());
+            attributes.put("preferenceStoreAttributes", "{\"type\": \"Noop\"}");
+            attributes.put(SystemConfig.TYPE, JsonSystemConfigImpl.SYSTEM_CONFIG_TYPE);
+            attributes.put(SystemConfig.STARTUP_LOGGED_TO_SYSTEM_OUT, Boolean.FALSE);
+            attributes.put(SystemConfig.CONTEXT, context);
+            final SettableFuture<SystemConfig<?>> configFuture = SettableFuture.create();
+            _systemLauncher = new SystemLauncher(new SystemLauncherListener.DefaultSystemLauncherListener()
+            {
+                @Override
+                public void onContainerResolve(final SystemConfig<?> systemConfig)
+                {
+                    configFuture.set(systemConfig);
+                }
+            });
+            _systemLauncher.startup(attributes);
+            final SystemConfig<?> systemConfig = configFuture.get(10, TimeUnit.SECONDS);
+            _broker = (Broker<?>) systemConfig.getContainer();
+
+            final Map<String, Object> authProviderAttributes = new HashMap<>();
+            authProviderAttributes.put(ConfiguredObject.NAME, "testAuthProvider");
+            authProviderAttributes.put(ConfiguredObject.TYPE, "Plain");
+
+            final AuthenticationProvider
+                    authProvider = _broker.createChild(AuthenticationProvider.class, authProviderAttributes);
+
+            Map<String, Object> userAttrs = new HashMap<>();
+            userAttrs.put(User.TYPE, "managed");
+            userAttrs.put(User.NAME, "guest");
+            userAttrs.put(User.PASSWORD, PLAINTEXT);
+            User user = (User) authProvider.createChild(User.class, userAttrs);
+        }finally{

Review comment:
       why this code is here.
   It should be in tearDown

##########
File path: broker-core/src/test/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypterTest.java
##########
@@ -0,0 +1,364 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import static org.mockito.Mockito.mock;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.SecureRandom;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Random;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+
+import javax.crypto.SecretKeyFactory;
+import javax.crypto.spec.PBEKeySpec;
+import javax.crypto.spec.SecretKeySpec;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.util.concurrent.SettableFuture;
+import org.junit.Before;
+import org.junit.Test;
+
+import org.apache.qpid.server.SystemLauncher;
+import org.apache.qpid.server.SystemLauncherListener;
+import org.apache.qpid.server.model.AuthenticationProvider;
+import org.apache.qpid.server.model.Broker;
+import org.apache.qpid.server.model.BrokerModel;
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.JsonSystemConfigImpl;
+import org.apache.qpid.server.model.SystemConfig;
+import org.apache.qpid.server.model.User;
+import org.apache.qpid.server.util.FileUtils;
+import org.apache.qpid.test.utils.UnitTestBase;
+
+public class AESGCMKeyFileEncrypterTest extends UnitTestBase
+{
+    private static final String PLAIN_SECRET = "secret";
+    private final SecureRandom _random = new SecureRandom();
+    public static final String PLAINTEXT = "guestkey";
+    private Path _configurationLocation;
+    private Path _workDir;
+    private Broker<?> _broker;
+    private SystemLauncher _systemLauncher;
+    private Map<String, Object> attributes = new HashMap<>();
+
+    private void createBrokerWithEncrypter(final Object encrptionType) throws Exception
+    {
+        try
+        {
+            _workDir = Files.createTempDirectory("qpid_work_dir");
+            _configurationLocation = Files.createTempFile(_workDir, "config", ".json");
+            Map<String, Object> context = new HashMap<>();
+            context.put("qpid.work_dir", _workDir.toFile().getAbsolutePath());
+            Map<String, Object> config = new HashMap<>();
+            config.put(ConfiguredObject.NAME, getTestName());
+            config.put(Broker.MODEL_VERSION, BrokerModel.MODEL_VERSION);
+            config.put(Broker.CONFIDENTIAL_CONFIGURATION_ENCRYPTION_PROVIDER, encrptionType);
+            new ObjectMapper().writeValue(_configurationLocation.toFile(), config);
+
+            attributes.put("storePath", _configurationLocation.toFile().getAbsolutePath());
+            attributes.put("preferenceStoreAttributes", "{\"type\": \"Noop\"}");
+            attributes.put(SystemConfig.TYPE, JsonSystemConfigImpl.SYSTEM_CONFIG_TYPE);
+            attributes.put(SystemConfig.STARTUP_LOGGED_TO_SYSTEM_OUT, Boolean.FALSE);
+            attributes.put(SystemConfig.CONTEXT, context);
+            final SettableFuture<SystemConfig<?>> configFuture = SettableFuture.create();
+            _systemLauncher = new SystemLauncher(new SystemLauncherListener.DefaultSystemLauncherListener()
+            {
+                @Override
+                public void onContainerResolve(final SystemConfig<?> systemConfig)
+                {
+                    configFuture.set(systemConfig);
+                }
+            });
+            _systemLauncher.startup(attributes);
+            final SystemConfig<?> systemConfig = configFuture.get(10, TimeUnit.SECONDS);

Review comment:
       10 is magic number... can it be moved into a constant

##########
File path: broker-core/src/test/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypterTest.java
##########
@@ -0,0 +1,364 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import static org.mockito.Mockito.mock;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.SecureRandom;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Random;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+
+import javax.crypto.SecretKeyFactory;
+import javax.crypto.spec.PBEKeySpec;
+import javax.crypto.spec.SecretKeySpec;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.util.concurrent.SettableFuture;
+import org.junit.Before;
+import org.junit.Test;
+
+import org.apache.qpid.server.SystemLauncher;
+import org.apache.qpid.server.SystemLauncherListener;
+import org.apache.qpid.server.model.AuthenticationProvider;
+import org.apache.qpid.server.model.Broker;
+import org.apache.qpid.server.model.BrokerModel;
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.JsonSystemConfigImpl;
+import org.apache.qpid.server.model.SystemConfig;
+import org.apache.qpid.server.model.User;
+import org.apache.qpid.server.util.FileUtils;
+import org.apache.qpid.test.utils.UnitTestBase;
+
+public class AESGCMKeyFileEncrypterTest extends UnitTestBase
+{
+    private static final String PLAIN_SECRET = "secret";
+    private final SecureRandom _random = new SecureRandom();
+    public static final String PLAINTEXT = "guestkey";
+    private Path _configurationLocation;
+    private Path _workDir;
+    private Broker<?> _broker;
+    private SystemLauncher _systemLauncher;
+    private Map<String, Object> attributes = new HashMap<>();
+
+    private void createBrokerWithEncrypter(final Object encrptionType) throws Exception
+    {
+        try
+        {
+            _workDir = Files.createTempDirectory("qpid_work_dir");
+            _configurationLocation = Files.createTempFile(_workDir, "config", ".json");
+            Map<String, Object> context = new HashMap<>();
+            context.put("qpid.work_dir", _workDir.toFile().getAbsolutePath());
+            Map<String, Object> config = new HashMap<>();
+            config.put(ConfiguredObject.NAME, getTestName());
+            config.put(Broker.MODEL_VERSION, BrokerModel.MODEL_VERSION);
+            config.put(Broker.CONFIDENTIAL_CONFIGURATION_ENCRYPTION_PROVIDER, encrptionType);
+            new ObjectMapper().writeValue(_configurationLocation.toFile(), config);
+
+            attributes.put("storePath", _configurationLocation.toFile().getAbsolutePath());
+            attributes.put("preferenceStoreAttributes", "{\"type\": \"Noop\"}");
+            attributes.put(SystemConfig.TYPE, JsonSystemConfigImpl.SYSTEM_CONFIG_TYPE);
+            attributes.put(SystemConfig.STARTUP_LOGGED_TO_SYSTEM_OUT, Boolean.FALSE);
+            attributes.put(SystemConfig.CONTEXT, context);
+            final SettableFuture<SystemConfig<?>> configFuture = SettableFuture.create();
+            _systemLauncher = new SystemLauncher(new SystemLauncherListener.DefaultSystemLauncherListener()
+            {
+                @Override
+                public void onContainerResolve(final SystemConfig<?> systemConfig)
+                {
+                    configFuture.set(systemConfig);
+                }
+            });
+            _systemLauncher.startup(attributes);
+            final SystemConfig<?> systemConfig = configFuture.get(10, TimeUnit.SECONDS);
+            _broker = (Broker<?>) systemConfig.getContainer();
+
+            final Map<String, Object> authProviderAttributes = new HashMap<>();
+            authProviderAttributes.put(ConfiguredObject.NAME, "testAuthProvider");
+            authProviderAttributes.put(ConfiguredObject.TYPE, "Plain");
+
+            final AuthenticationProvider
+                    authProvider = _broker.createChild(AuthenticationProvider.class, authProviderAttributes);
+
+            Map<String, Object> userAttrs = new HashMap<>();
+            userAttrs.put(User.TYPE, "managed");
+            userAttrs.put(User.NAME, "guest");
+            userAttrs.put(User.PASSWORD, PLAINTEXT);
+            User user = (User) authProvider.createChild(User.class, userAttrs);
+        }finally{
+            _systemLauncher.shutdown();
+            FileUtils.deleteDirectory(_workDir.toFile().getAbsolutePath());
+            FileUtils.deleteDirectory(_configurationLocation.toFile().getAbsolutePath());
+        }
+    }
+
+    @Test
+    public void testRepeatedEncryptionsReturnDifferentValues() throws Exception
+    {
+        SecretKeySpec secretKey = createSecretKey();
+        AESGCMKeyFileEncrypter encrypter = new AESGCMKeyFileEncrypter(secretKey);
+
+        Set<String> encryptions = new HashSet<>();
+
+        int iterations = 100;

Review comment:
       why 100 iterations are actually needed here? what that solves?
   

##########
File path: broker-core/src/main/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypterFactory.java
##########
@@ -0,0 +1,383 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.AclEntry;
+import java.nio.file.attribute.AclEntryPermission;
+import java.nio.file.attribute.AclEntryType;
+import java.nio.file.attribute.AclFileAttributeView;
+import java.nio.file.attribute.FileAttribute;
+import java.nio.file.attribute.PosixFileAttributeView;
+import java.nio.file.attribute.PosixFilePermission;
+import java.nio.file.attribute.PosixFilePermissions;
+import java.nio.file.attribute.UserPrincipal;
+import java.security.NoSuchAlgorithmException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.EnumSet;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.ListIterator;
+import java.util.Map;
+import java.util.Set;
+
+import javax.crypto.Cipher;
+import javax.crypto.KeyGenerator;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.SecretKeySpec;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.qpid.server.configuration.IllegalConfigurationException;
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.SystemConfig;
+import org.apache.qpid.server.plugin.ConditionallyAvailable;
+import org.apache.qpid.server.plugin.ConfigurationSecretEncrypterFactory;
+import org.apache.qpid.server.plugin.PluggableService;
+
+@PluggableService
+public class AESGCMKeyFileEncrypterFactory implements ConfigurationSecretEncrypterFactory, ConditionallyAvailable
+{
+    private static final Logger LOGGER = LoggerFactory.getLogger(AESGCMKeyFileEncrypterFactory.class);
+
+    static final String ENCRYPTER_KEY_FILE = "encrypter.key.file";
+
+    private static final int AES_KEY_SIZE_BITS = 256;
+    private static final int AES_KEY_SIZE_BYTES = AES_KEY_SIZE_BITS / 8;
+    private static final String AES_ALGORITHM = "AES";
+
+    public static final String TYPE = "AESGCMKeyFile";
+
+    static final String DEFAULT_KEYS_SUBDIR_NAME = ".keys";
+
+    private static final boolean IS_AVAILABLE;
+
+    private static final String ILLEGAL_ARG_EXCEPTION = "Unable to determine a mechanism to protect access to the key file on this filesystem";

Review comment:
       the constant name does not match message name. I would inline this

##########
File path: broker-core/src/main/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypterFactory.java
##########
@@ -0,0 +1,383 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.AclEntry;
+import java.nio.file.attribute.AclEntryPermission;
+import java.nio.file.attribute.AclEntryType;
+import java.nio.file.attribute.AclFileAttributeView;
+import java.nio.file.attribute.FileAttribute;
+import java.nio.file.attribute.PosixFileAttributeView;
+import java.nio.file.attribute.PosixFilePermission;
+import java.nio.file.attribute.PosixFilePermissions;
+import java.nio.file.attribute.UserPrincipal;
+import java.security.NoSuchAlgorithmException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.EnumSet;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.ListIterator;
+import java.util.Map;
+import java.util.Set;
+
+import javax.crypto.Cipher;
+import javax.crypto.KeyGenerator;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.SecretKeySpec;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.qpid.server.configuration.IllegalConfigurationException;
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.SystemConfig;
+import org.apache.qpid.server.plugin.ConditionallyAvailable;
+import org.apache.qpid.server.plugin.ConfigurationSecretEncrypterFactory;
+import org.apache.qpid.server.plugin.PluggableService;
+
+@PluggableService
+public class AESGCMKeyFileEncrypterFactory implements ConfigurationSecretEncrypterFactory, ConditionallyAvailable
+{
+    private static final Logger LOGGER = LoggerFactory.getLogger(AESGCMKeyFileEncrypterFactory.class);
+
+    static final String ENCRYPTER_KEY_FILE = "encrypter.key.file";
+
+    private static final int AES_KEY_SIZE_BITS = 256;
+    private static final int AES_KEY_SIZE_BYTES = AES_KEY_SIZE_BITS / 8;
+    private static final String AES_ALGORITHM = "AES";
+
+    public static final String TYPE = "AESGCMKeyFile";
+
+    static final String DEFAULT_KEYS_SUBDIR_NAME = ".keys";
+
+    private static final boolean IS_AVAILABLE;
+
+    private static final String ILLEGAL_ARG_EXCEPTION = "Unable to determine a mechanism to protect access to the key file on this filesystem";
+
+    static
+    {
+        boolean isAvailable;
+        try
+        {
+            final int allowedKeyLength = Cipher.getMaxAllowedKeyLength(AES_ALGORITHM);
+            isAvailable = allowedKeyLength >=AES_KEY_SIZE_BITS;
+            if(!isAvailable)
+            {
+                LOGGER.warn("The {} configuration encryption encryption mechanism is not available. "
+                            + "Maximum available AES key length is {} but {} is required. "
+                            + "Ensure the full strength JCE policy has been installed into your JVM.", TYPE, allowedKeyLength, AES_KEY_SIZE_BITS);
+            }
+        }
+        catch (NoSuchAlgorithmException e)
+        {
+            isAvailable = false;
+
+            LOGGER.error("The " + TYPE + " configuration encryption encryption mechanism is not available. "
+                        + "The " + AES_ALGORITHM + " algorithm is not available within the JVM (despite it being a requirement).");
+        }
+
+        IS_AVAILABLE = isAvailable;
+    }
+
+    @Override
+    public ConfigurationSecretEncrypter createEncrypter(final ConfiguredObject<?> object)
+    {
+        String fileLocation;
+        if(object.getContextKeys(false).contains(ENCRYPTER_KEY_FILE))
+        {
+            fileLocation = object.getContextValue(String.class, ENCRYPTER_KEY_FILE);
+        }
+        else
+        {
+
+            fileLocation = object.getContextValue(String.class, SystemConfig.QPID_WORK_DIR)
+                           + File.separator + DEFAULT_KEYS_SUBDIR_NAME + File.separator
+                           + object.getCategoryClass().getSimpleName() + "_"
+                           + object.getName() + ".key";
+
+            Map<String, String> context = object.getContext();
+            Map<String, String> modifiedContext = new LinkedHashMap<>(context);
+            modifiedContext.put(ENCRYPTER_KEY_FILE, fileLocation);
+
+            object.setAttributes(Collections.<String, Object>singletonMap(ConfiguredObject.CONTEXT, modifiedContext));
+        }
+        File file = new File(fileLocation);
+        if(!file.exists())
+        {
+            LOGGER.warn("Configuration encryption is enabled, but no configuration secret was found. A new configuration secret will be created at '{}'.", fileLocation);
+            createAndPopulateKeyFile(file);
+        }
+        if(!file.isFile())
+        {
+            throw new IllegalArgumentException("File '"+fileLocation+"' is not a regular file.");
+        }
+        try
+        {
+            checkFilePermissions(fileLocation, file);
+            if(Files.size(file.toPath()) != AES_KEY_SIZE_BYTES)
+            {
+                throw new IllegalArgumentException("Key file '" + fileLocation + "' contains an incorrect about of data");
+            }
+
+            try(FileInputStream inputStream = new FileInputStream(file))
+            {
+                byte[] key = new byte[AES_KEY_SIZE_BYTES];
+                int pos = 0;
+                int read;
+                while(pos < key.length && -1 != ( read = inputStream.read(key, pos, key.length - pos)))
+                {
+                    pos += read;
+                }
+                if(pos != key.length)
+                {
+                    throw new IllegalConfigurationException("Key file '" + fileLocation + "' contained an incorrect about of data");
+                }
+                SecretKeySpec keySpec = new SecretKeySpec(key, AES_ALGORITHM);
+                return new AESKeyFileEncrypter(keySpec);
+            }
+        }
+        catch (IOException e)
+        {
+            throw new IllegalConfigurationException("Unable to get file permissions: " + e.getMessage(), e);
+        }
+    }
+
+    private void checkFilePermissions(String fileLocation, File file) throws IOException
+    {
+        if(isPosixFileSystem(file))
+        {
+            Set<PosixFilePermission> permissions = Files.getPosixFilePermissions(file.toPath());
+
+            if (permissions.contains(PosixFilePermission.GROUP_READ)
+                    || permissions.contains(PosixFilePermission.OTHERS_READ)
+                    || permissions.contains(PosixFilePermission.GROUP_WRITE)
+                    || permissions.contains(PosixFilePermission.OTHERS_WRITE)) {
+                throw new IllegalArgumentException("Key file '"
+                        + fileLocation
+                        + "' has incorrect permissions.  Only the owner "
+                        + "should be able to read or write this file.");
+            }
+        }
+        else if(isAclFileSystem(file))
+        {
+            AclFileAttributeView attributeView = Files.getFileAttributeView(file.toPath(), AclFileAttributeView.class);
+            ArrayList<AclEntry> acls = new ArrayList<>(attributeView.getAcl());
+            ListIterator<AclEntry> iter = acls.listIterator();
+            UserPrincipal owner = Files.getOwner(file.toPath());
+            while(iter.hasNext())
+            {
+                AclEntry acl = iter.next();
+                if(acl.type() == AclEntryType.ALLOW)
+                {
+                    Set<AclEntryPermission> originalPermissions = acl.permissions();
+                    Set<AclEntryPermission> updatedPermissions = EnumSet.copyOf(originalPermissions);
+
+
+                    if (updatedPermissions.removeAll(EnumSet.of(AclEntryPermission.APPEND_DATA,
+                            AclEntryPermission.EXECUTE,
+                            AclEntryPermission.WRITE_ACL,
+                            AclEntryPermission.WRITE_DATA,
+                            AclEntryPermission.WRITE_OWNER))) {
+                        throw new IllegalArgumentException("Key file '"
+                                + fileLocation
+                                + "' has incorrect permissions.  The file should not be modifiable by any user.");
+                    }
+                    if (!owner.equals(acl.principal()) && updatedPermissions.removeAll(EnumSet.of(AclEntryPermission.READ_DATA))) {
+                        throw new IllegalArgumentException("Key file '"
+                                + fileLocation
+                                + "' has incorrect permissions.  Only the owner should be able to read from the file.");
+                    }
+                }
+            }
+        }
+        else
+        {
+            throw new IllegalArgumentException(ILLEGAL_ARG_EXCEPTION);
+        }
+    }
+
+    private boolean isPosixFileSystem(File file) throws IOException
+    {
+        return Files.getFileAttributeView(file.toPath(), PosixFileAttributeView.class) != null;
+    }
+
+    private boolean isAclFileSystem(File file) throws IOException
+    {
+        return Files.getFileAttributeView(file.toPath(), AclFileAttributeView.class) != null;
+    }
+
+
+    private void createAndPopulateKeyFile(final File file)
+    {
+        try
+        {
+            createEmptyKeyFile(file);
+
+            KeyGenerator keyGenerator = KeyGenerator.getInstance(AES_ALGORITHM);
+            keyGenerator.init(AES_KEY_SIZE_BITS);
+            SecretKey key = keyGenerator.generateKey();
+            try(FileOutputStream os = new FileOutputStream(file))
+            {
+                os.write(key.getEncoded());
+            }
+
+            makeKeyFileReadOnly(file);
+        }
+        catch (NoSuchAlgorithmException | IOException e)
+        {
+            throw new IllegalArgumentException("Cannot create key file: " + e.getMessage(), e);
+        }
+
+    }
+
+    private void makeKeyFileReadOnly(File file) throws IOException
+    {
+        if(isPosixFileSystem(file))
+        {
+            Files.setPosixFilePermissions(file.toPath(), EnumSet.of(PosixFilePermission.OWNER_READ));
+        }
+        else if(isAclFileSystem(file))
+        {
+            AclFileAttributeView attributeView = Files.getFileAttributeView(file.toPath(), AclFileAttributeView.class);
+            ArrayList<AclEntry> acls = new ArrayList<>(attributeView.getAcl());
+            ListIterator<AclEntry> iter = acls.listIterator();
+            file.setReadOnly();
+            while(iter.hasNext())
+            {
+                AclEntry acl = iter.next();
+                Set<AclEntryPermission> originalPermissions = acl.permissions();
+                Set<AclEntryPermission> updatedPermissions = EnumSet.copyOf(originalPermissions);
+
+                if(updatedPermissions.removeAll(EnumSet.of(AclEntryPermission.APPEND_DATA,
+                                                           AclEntryPermission.DELETE,
+                                                           AclEntryPermission.EXECUTE,
+                                                           AclEntryPermission.WRITE_ACL,
+                                                           AclEntryPermission.WRITE_DATA,
+                                                           AclEntryPermission.WRITE_ATTRIBUTES,
+                                                           AclEntryPermission.WRITE_NAMED_ATTRS,
+                                                           AclEntryPermission.WRITE_OWNER)))
+                {
+                    AclEntry.Builder builder = AclEntry.newBuilder(acl);
+                    builder.setPermissions(updatedPermissions);
+                    iter.set(builder.build());
+                }
+            }
+            attributeView.setAcl(acls);
+        }
+        else
+        {
+            throw new IllegalArgumentException(ILLEGAL_ARG_EXCEPTION);

Review comment:
       I am not sure that IllegalArgumentException is a right exception  type

##########
File path: broker-core/src/test/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypterTest.java
##########
@@ -0,0 +1,364 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import static org.mockito.Mockito.mock;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.SecureRandom;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Random;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+
+import javax.crypto.SecretKeyFactory;
+import javax.crypto.spec.PBEKeySpec;
+import javax.crypto.spec.SecretKeySpec;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.util.concurrent.SettableFuture;
+import org.junit.Before;
+import org.junit.Test;
+
+import org.apache.qpid.server.SystemLauncher;
+import org.apache.qpid.server.SystemLauncherListener;
+import org.apache.qpid.server.model.AuthenticationProvider;
+import org.apache.qpid.server.model.Broker;
+import org.apache.qpid.server.model.BrokerModel;
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.JsonSystemConfigImpl;
+import org.apache.qpid.server.model.SystemConfig;
+import org.apache.qpid.server.model.User;
+import org.apache.qpid.server.util.FileUtils;
+import org.apache.qpid.test.utils.UnitTestBase;
+
+public class AESGCMKeyFileEncrypterTest extends UnitTestBase
+{
+    private static final String PLAIN_SECRET = "secret";
+    private final SecureRandom _random = new SecureRandom();
+    public static final String PLAINTEXT = "guestkey";
+    private Path _configurationLocation;
+    private Path _workDir;
+    private Broker<?> _broker;
+    private SystemLauncher _systemLauncher;
+    private Map<String, Object> attributes = new HashMap<>();
+
+    private void createBrokerWithEncrypter(final Object encrptionType) throws Exception
+    {
+        try
+        {
+            _workDir = Files.createTempDirectory("qpid_work_dir");
+            _configurationLocation = Files.createTempFile(_workDir, "config", ".json");
+            Map<String, Object> context = new HashMap<>();
+            context.put("qpid.work_dir", _workDir.toFile().getAbsolutePath());
+            Map<String, Object> config = new HashMap<>();
+            config.put(ConfiguredObject.NAME, getTestName());
+            config.put(Broker.MODEL_VERSION, BrokerModel.MODEL_VERSION);
+            config.put(Broker.CONFIDENTIAL_CONFIGURATION_ENCRYPTION_PROVIDER, encrptionType);
+            new ObjectMapper().writeValue(_configurationLocation.toFile(), config);
+
+            attributes.put("storePath", _configurationLocation.toFile().getAbsolutePath());
+            attributes.put("preferenceStoreAttributes", "{\"type\": \"Noop\"}");
+            attributes.put(SystemConfig.TYPE, JsonSystemConfigImpl.SYSTEM_CONFIG_TYPE);
+            attributes.put(SystemConfig.STARTUP_LOGGED_TO_SYSTEM_OUT, Boolean.FALSE);
+            attributes.put(SystemConfig.CONTEXT, context);
+            final SettableFuture<SystemConfig<?>> configFuture = SettableFuture.create();
+            _systemLauncher = new SystemLauncher(new SystemLauncherListener.DefaultSystemLauncherListener()
+            {
+                @Override
+                public void onContainerResolve(final SystemConfig<?> systemConfig)
+                {
+                    configFuture.set(systemConfig);
+                }
+            });
+            _systemLauncher.startup(attributes);
+            final SystemConfig<?> systemConfig = configFuture.get(10, TimeUnit.SECONDS);
+            _broker = (Broker<?>) systemConfig.getContainer();
+
+            final Map<String, Object> authProviderAttributes = new HashMap<>();
+            authProviderAttributes.put(ConfiguredObject.NAME, "testAuthProvider");
+            authProviderAttributes.put(ConfiguredObject.TYPE, "Plain");
+
+            final AuthenticationProvider
+                    authProvider = _broker.createChild(AuthenticationProvider.class, authProviderAttributes);
+
+            Map<String, Object> userAttrs = new HashMap<>();
+            userAttrs.put(User.TYPE, "managed");
+            userAttrs.put(User.NAME, "guest");
+            userAttrs.put(User.PASSWORD, PLAINTEXT);
+            User user = (User) authProvider.createChild(User.class, userAttrs);
+        }finally{
+            _systemLauncher.shutdown();
+            FileUtils.deleteDirectory(_workDir.toFile().getAbsolutePath());
+            FileUtils.deleteDirectory(_configurationLocation.toFile().getAbsolutePath());
+        }
+    }
+
+    @Test
+    public void testRepeatedEncryptionsReturnDifferentValues() throws Exception
+    {
+        SecretKeySpec secretKey = createSecretKey();
+        AESGCMKeyFileEncrypter encrypter = new AESGCMKeyFileEncrypter(secretKey);
+
+        Set<String> encryptions = new HashSet<>();
+
+        int iterations = 100;
+
+        for (int i = 0; i < iterations; i++)
+        {
+            encryptions.add(encrypter.encrypt(PLAINTEXT));
+        }
+
+        assertEquals("Not all encryptions were distinct", (long) iterations, (long) encryptions.size());
+
+        for (String encrypted : encryptions)
+        {
+            assertEquals("Not all encryptions decrypt correctly", PLAINTEXT, encrypter.decrypt(encrypted));
+        }
+    }
+
+    @Test
+    public void testCreationFailsOnInvalidSecret() throws Exception
+    {
+        try
+        {
+            new AESGCMKeyFileEncrypter(null);
+            fail("An encrypter should not be creatable from a null key");
+        }
+        catch (NullPointerException e)
+        {
+            assertTrue("Unexpected exception message:" + e.getMessage(),
+                       e.getMessage().contains("A non null secret key must be supplied"));
+        }
+
+        try
+        {
+            PBEKeySpec keySpec = new PBEKeySpec(PLAIN_SECRET.toCharArray());
+            SecretKeyFactory factory = SecretKeyFactory.getInstance("PBEWithMD5AndDES");
+            new AESGCMKeyFileEncrypter(factory.generateSecret(keySpec));
+            fail("An encrypter should not be creatable from the wrong type of secret key");
+        }
+        catch (IllegalArgumentException e)
+        {
+            assertTrue("Unexpected exception message:" + e.getMessage(),
+                       e.getMessage().contains("Provided secret key was for the algorithm: PBEWithMD5AndDES when AES was needed."));
+        }
+    }
+
+    @Test
+    public void testEncryptionOfEmptyString() throws Exception
+    {
+        doTestSimpleEncryptDecrypt("");
+    }
+
+    private void doTestSimpleEncryptDecrypt(final String text)
+    {
+        SecretKeySpec secretKey = createSecretKey();
+        AESGCMKeyFileEncrypter encrypter = new AESGCMKeyFileEncrypter(secretKey);
+
+        String encrypted = encrypter.encrypt(text);
+        assertNotNull("Encrypter did not return a result from encryption", encrypted);
+        assertNotEquals("Plain text and encrypted version are equal", text, encrypted);
+        String decrypted = encrypter.decrypt(encrypted);
+        assertNotNull("Encrypter did not return a result from decryption", decrypted);
+        assertEquals("Encryption was not reversible", text, decrypted);
+    }
+
+    @Test
+    public void testEncryptingNullFails() throws Exception
+    {
+        try
+        {
+            SecretKeySpec secretKey = createSecretKey();
+            AESGCMKeyFileEncrypter encrypter = new AESGCMKeyFileEncrypter(secretKey);
+
+            String encrypted = encrypter.encrypt(null);
+            fail("Attempting to encrypt null should fail");
+        }
+        catch (NullPointerException e)
+        {
+            //pass
+        }
+    }
+
+    @Test
+    public void testEncryptingVeryLargeSecret() throws Exception
+    {
+        Random random = new Random();
+        byte[] data = new byte[4096];
+        random.nextBytes(data);
+        for (int i = 0; i < data.length; i++)
+        {
+            data[i] = (byte) (data[i] & 0xEF);
+        }
+        doTestSimpleEncryptDecrypt(new String(data, StandardCharsets.US_ASCII));
+    }
+
+    @Test
+    public void testDecryptNonsense() throws Exception
+    {
+        SecretKeySpec secretKey = createSecretKey();
+        AESGCMKeyFileEncrypter encrypter = new AESGCMKeyFileEncrypter(secretKey);
+
+        try
+        {
+            encrypter.decrypt(null);
+            fail("Should not decrypt a null value");
+        }
+        catch (NullPointerException e)
+        {
+            // pass
+        }
+
+        try
+        {
+            encrypter.decrypt("");
+            fail("Should not decrypt the empty String");
+        }
+        catch (IllegalArgumentException e)
+        {
+            // pass
+        }
+
+        try
+        {
+            encrypter.decrypt("thisisnonsense");
+            fail("Should not decrypt a small amount of nonsense");
+        }
+        catch (IllegalArgumentException e)
+        {
+            // pass
+        }
+
+        try
+        {
+            String answer = encrypter.decrypt("thisisn'tvalidBase64!soitshouldfailwithanIllegalArgumentException");
+            fail("Should not decrypt a larger amount of nonsense");
+        }
+        catch (IllegalArgumentException e)
+        {
+            // pass
+        }
+    }
+
+    private SecretKeySpec createSecretKey()
+    {
+        final byte[] keyData = new byte[32];
+        _random.nextBytes(keyData);
+        return new SecretKeySpec(keyData, "AES");
+    }
+
+    @Test
+    public void testChangeOfEncryptiontoGCM() throws Exception
+    {
+        createBrokerWithEncrypter(AESKeyFileEncrypterFactory.TYPE);
+        String decryptedPassword = null;
+        String encryptedPassword = getEncryptedPasswordFromConfig();
+        byte[] keyBytes = getSecretKey(decryptedPassword);
+        AESKeyFileEncrypter encrypter = new AESKeyFileEncrypter(new SecretKeySpec(keyBytes, "AES"));
+        decryptedPassword = encrypter.decrypt(encryptedPassword);
+        assertEquals("Decrpted text doesnt match original", PLAINTEXT, decryptedPassword);
+        //_broker.setAttributes(Collections.singletonMap(Broker.CONFIDENTIAL_CONFIGURATION_ENCRYPTION_PROVIDER,"AesGcmKeyFile"));

Review comment:
       commented code
   incomplete test

##########
File path: broker-core/src/test/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypterTest.java
##########
@@ -0,0 +1,364 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import static org.mockito.Mockito.mock;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.security.SecureRandom;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Random;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+
+import javax.crypto.SecretKeyFactory;
+import javax.crypto.spec.PBEKeySpec;
+import javax.crypto.spec.SecretKeySpec;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.util.concurrent.SettableFuture;
+import org.junit.Before;
+import org.junit.Test;
+
+import org.apache.qpid.server.SystemLauncher;
+import org.apache.qpid.server.SystemLauncherListener;
+import org.apache.qpid.server.model.AuthenticationProvider;
+import org.apache.qpid.server.model.Broker;
+import org.apache.qpid.server.model.BrokerModel;
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.JsonSystemConfigImpl;
+import org.apache.qpid.server.model.SystemConfig;
+import org.apache.qpid.server.model.User;
+import org.apache.qpid.server.util.FileUtils;
+import org.apache.qpid.test.utils.UnitTestBase;
+
+public class AESGCMKeyFileEncrypterTest extends UnitTestBase
+{
+    private static final String PLAIN_SECRET = "secret";
+    private final SecureRandom _random = new SecureRandom();
+    public static final String PLAINTEXT = "guestkey";
+    private Path _configurationLocation;
+    private Path _workDir;
+    private Broker<?> _broker;
+    private SystemLauncher _systemLauncher;
+    private Map<String, Object> attributes = new HashMap<>();
+
+    private void createBrokerWithEncrypter(final Object encrptionType) throws Exception
+    {
+        try
+        {
+            _workDir = Files.createTempDirectory("qpid_work_dir");
+            _configurationLocation = Files.createTempFile(_workDir, "config", ".json");
+            Map<String, Object> context = new HashMap<>();
+            context.put("qpid.work_dir", _workDir.toFile().getAbsolutePath());
+            Map<String, Object> config = new HashMap<>();
+            config.put(ConfiguredObject.NAME, getTestName());
+            config.put(Broker.MODEL_VERSION, BrokerModel.MODEL_VERSION);
+            config.put(Broker.CONFIDENTIAL_CONFIGURATION_ENCRYPTION_PROVIDER, encrptionType);
+            new ObjectMapper().writeValue(_configurationLocation.toFile(), config);
+
+            attributes.put("storePath", _configurationLocation.toFile().getAbsolutePath());
+            attributes.put("preferenceStoreAttributes", "{\"type\": \"Noop\"}");
+            attributes.put(SystemConfig.TYPE, JsonSystemConfigImpl.SYSTEM_CONFIG_TYPE);
+            attributes.put(SystemConfig.STARTUP_LOGGED_TO_SYSTEM_OUT, Boolean.FALSE);
+            attributes.put(SystemConfig.CONTEXT, context);
+            final SettableFuture<SystemConfig<?>> configFuture = SettableFuture.create();
+            _systemLauncher = new SystemLauncher(new SystemLauncherListener.DefaultSystemLauncherListener()
+            {
+                @Override
+                public void onContainerResolve(final SystemConfig<?> systemConfig)
+                {
+                    configFuture.set(systemConfig);
+                }
+            });
+            _systemLauncher.startup(attributes);
+            final SystemConfig<?> systemConfig = configFuture.get(10, TimeUnit.SECONDS);
+            _broker = (Broker<?>) systemConfig.getContainer();
+
+            final Map<String, Object> authProviderAttributes = new HashMap<>();
+            authProviderAttributes.put(ConfiguredObject.NAME, "testAuthProvider");
+            authProviderAttributes.put(ConfiguredObject.TYPE, "Plain");
+
+            final AuthenticationProvider
+                    authProvider = _broker.createChild(AuthenticationProvider.class, authProviderAttributes);
+
+            Map<String, Object> userAttrs = new HashMap<>();
+            userAttrs.put(User.TYPE, "managed");
+            userAttrs.put(User.NAME, "guest");
+            userAttrs.put(User.PASSWORD, PLAINTEXT);
+            User user = (User) authProvider.createChild(User.class, userAttrs);
+        }finally{
+            _systemLauncher.shutdown();
+            FileUtils.deleteDirectory(_workDir.toFile().getAbsolutePath());
+            FileUtils.deleteDirectory(_configurationLocation.toFile().getAbsolutePath());
+        }
+    }
+
+    @Test
+    public void testRepeatedEncryptionsReturnDifferentValues() throws Exception
+    {
+        SecretKeySpec secretKey = createSecretKey();
+        AESGCMKeyFileEncrypter encrypter = new AESGCMKeyFileEncrypter(secretKey);
+
+        Set<String> encryptions = new HashSet<>();
+
+        int iterations = 100;
+
+        for (int i = 0; i < iterations; i++)
+        {
+            encryptions.add(encrypter.encrypt(PLAINTEXT));
+        }
+
+        assertEquals("Not all encryptions were distinct", (long) iterations, (long) encryptions.size());
+
+        for (String encrypted : encryptions)
+        {
+            assertEquals("Not all encryptions decrypt correctly", PLAINTEXT, encrypter.decrypt(encrypted));
+        }
+    }
+
+    @Test
+    public void testCreationFailsOnInvalidSecret() throws Exception
+    {
+        try
+        {
+            new AESGCMKeyFileEncrypter(null);
+            fail("An encrypter should not be creatable from a null key");
+        }
+        catch (NullPointerException e)
+        {
+            assertTrue("Unexpected exception message:" + e.getMessage(),
+                       e.getMessage().contains("A non null secret key must be supplied"));
+        }
+
+        try
+        {
+            PBEKeySpec keySpec = new PBEKeySpec(PLAIN_SECRET.toCharArray());
+            SecretKeyFactory factory = SecretKeyFactory.getInstance("PBEWithMD5AndDES");
+            new AESGCMKeyFileEncrypter(factory.generateSecret(keySpec));
+            fail("An encrypter should not be creatable from the wrong type of secret key");
+        }
+        catch (IllegalArgumentException e)
+        {
+            assertTrue("Unexpected exception message:" + e.getMessage(),
+                       e.getMessage().contains("Provided secret key was for the algorithm: PBEWithMD5AndDES when AES was needed."));
+        }
+    }
+
+    @Test
+    public void testEncryptionOfEmptyString() throws Exception
+    {
+        doTestSimpleEncryptDecrypt("");
+    }
+
+    private void doTestSimpleEncryptDecrypt(final String text)
+    {
+        SecretKeySpec secretKey = createSecretKey();
+        AESGCMKeyFileEncrypter encrypter = new AESGCMKeyFileEncrypter(secretKey);
+
+        String encrypted = encrypter.encrypt(text);
+        assertNotNull("Encrypter did not return a result from encryption", encrypted);
+        assertNotEquals("Plain text and encrypted version are equal", text, encrypted);
+        String decrypted = encrypter.decrypt(encrypted);
+        assertNotNull("Encrypter did not return a result from decryption", decrypted);
+        assertEquals("Encryption was not reversible", text, decrypted);
+    }
+
+    @Test
+    public void testEncryptingNullFails() throws Exception
+    {
+        try
+        {
+            SecretKeySpec secretKey = createSecretKey();

Review comment:
       the 2 lines below need moving out of try-catch block

##########
File path: broker-core/src/main/java/org/apache/qpid/server/security/encryption/AESGCMKeyFileEncrypterFactory.java
##########
@@ -0,0 +1,383 @@
+/*
+ * 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.qpid.server.security.encryption;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.AclEntry;
+import java.nio.file.attribute.AclEntryPermission;
+import java.nio.file.attribute.AclEntryType;
+import java.nio.file.attribute.AclFileAttributeView;
+import java.nio.file.attribute.FileAttribute;
+import java.nio.file.attribute.PosixFileAttributeView;
+import java.nio.file.attribute.PosixFilePermission;
+import java.nio.file.attribute.PosixFilePermissions;
+import java.nio.file.attribute.UserPrincipal;
+import java.security.NoSuchAlgorithmException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.EnumSet;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.ListIterator;
+import java.util.Map;
+import java.util.Set;
+
+import javax.crypto.Cipher;
+import javax.crypto.KeyGenerator;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.SecretKeySpec;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.qpid.server.configuration.IllegalConfigurationException;
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.model.SystemConfig;
+import org.apache.qpid.server.plugin.ConditionallyAvailable;
+import org.apache.qpid.server.plugin.ConfigurationSecretEncrypterFactory;
+import org.apache.qpid.server.plugin.PluggableService;
+
+@PluggableService
+public class AESGCMKeyFileEncrypterFactory implements ConfigurationSecretEncrypterFactory, ConditionallyAvailable
+{
+    private static final Logger LOGGER = LoggerFactory.getLogger(AESGCMKeyFileEncrypterFactory.class);
+
+    static final String ENCRYPTER_KEY_FILE = "encrypter.key.file";
+
+    private static final int AES_KEY_SIZE_BITS = 256;
+    private static final int AES_KEY_SIZE_BYTES = AES_KEY_SIZE_BITS / 8;
+    private static final String AES_ALGORITHM = "AES";
+
+    public static final String TYPE = "AESGCMKeyFile";
+
+    static final String DEFAULT_KEYS_SUBDIR_NAME = ".keys";
+
+    private static final boolean IS_AVAILABLE;
+
+    private static final String ILLEGAL_ARG_EXCEPTION = "Unable to determine a mechanism to protect access to the key file on this filesystem";
+
+    static
+    {
+        boolean isAvailable;
+        try
+        {
+            final int allowedKeyLength = Cipher.getMaxAllowedKeyLength(AES_ALGORITHM);
+            isAvailable = allowedKeyLength >=AES_KEY_SIZE_BITS;
+            if(!isAvailable)
+            {
+                LOGGER.warn("The {} configuration encryption encryption mechanism is not available. "
+                            + "Maximum available AES key length is {} but {} is required. "
+                            + "Ensure the full strength JCE policy has been installed into your JVM.", TYPE, allowedKeyLength, AES_KEY_SIZE_BITS);
+            }
+        }
+        catch (NoSuchAlgorithmException e)
+        {
+            isAvailable = false;
+
+            LOGGER.error("The " + TYPE + " configuration encryption encryption mechanism is not available. "
+                        + "The " + AES_ALGORITHM + " algorithm is not available within the JVM (despite it being a requirement).");
+        }
+
+        IS_AVAILABLE = isAvailable;
+    }
+
+    @Override
+    public ConfigurationSecretEncrypter createEncrypter(final ConfiguredObject<?> object)
+    {
+        String fileLocation;
+        if(object.getContextKeys(false).contains(ENCRYPTER_KEY_FILE))
+        {
+            fileLocation = object.getContextValue(String.class, ENCRYPTER_KEY_FILE);
+        }
+        else
+        {
+
+            fileLocation = object.getContextValue(String.class, SystemConfig.QPID_WORK_DIR)
+                           + File.separator + DEFAULT_KEYS_SUBDIR_NAME + File.separator
+                           + object.getCategoryClass().getSimpleName() + "_"
+                           + object.getName() + ".key";
+
+            Map<String, String> context = object.getContext();
+            Map<String, String> modifiedContext = new LinkedHashMap<>(context);
+            modifiedContext.put(ENCRYPTER_KEY_FILE, fileLocation);
+
+            object.setAttributes(Collections.<String, Object>singletonMap(ConfiguredObject.CONTEXT, modifiedContext));
+        }
+        File file = new File(fileLocation);
+        if(!file.exists())
+        {
+            LOGGER.warn("Configuration encryption is enabled, but no configuration secret was found. A new configuration secret will be created at '{}'.", fileLocation);
+            createAndPopulateKeyFile(file);
+        }
+        if(!file.isFile())
+        {
+            throw new IllegalArgumentException("File '"+fileLocation+"' is not a regular file.");
+        }
+        try
+        {
+            checkFilePermissions(fileLocation, file);
+            if(Files.size(file.toPath()) != AES_KEY_SIZE_BYTES)
+            {
+                throw new IllegalArgumentException("Key file '" + fileLocation + "' contains an incorrect about of data");
+            }
+
+            try(FileInputStream inputStream = new FileInputStream(file))
+            {
+                byte[] key = new byte[AES_KEY_SIZE_BYTES];
+                int pos = 0;
+                int read;
+                while(pos < key.length && -1 != ( read = inputStream.read(key, pos, key.length - pos)))
+                {
+                    pos += read;
+                }
+                if(pos != key.length)
+                {
+                    throw new IllegalConfigurationException("Key file '" + fileLocation + "' contained an incorrect about of data");
+                }
+                SecretKeySpec keySpec = new SecretKeySpec(key, AES_ALGORITHM);
+                return new AESKeyFileEncrypter(keySpec);
+            }
+        }
+        catch (IOException e)
+        {
+            throw new IllegalConfigurationException("Unable to get file permissions: " + e.getMessage(), e);
+        }
+    }
+
+    private void checkFilePermissions(String fileLocation, File file) throws IOException
+    {
+        if(isPosixFileSystem(file))

Review comment:
       Though, it is indeed insecure to have read permissions on the secret files for everyone, I am not sure that we have to enforce that. The group might actually have read/write permission. I do not see a problem with that




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org