Lehel44 commented on a change in pull request #5110: URL: https://github.com/apache/nifi/pull/5110#discussion_r644337507
########## File path: nifi-commons/nifi-security-kms/src/main/java/org/apache/nifi/security/kms/KeyProviderFactory.java ########## @@ -0,0 +1,85 @@ +/* + * 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.nifi.security.kms; + +import org.apache.commons.codec.DecoderException; +import org.apache.nifi.security.kms.configuration.FileBasedKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyStoreKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.StaticKeyProviderConfiguration; +import org.apache.commons.codec.binary.Hex; +import org.apache.nifi.security.kms.reader.KeyReaderException; + +import javax.crypto.SecretKey; +import javax.crypto.spec.SecretKeySpec; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.security.KeyStore; +import java.util.HashMap; +import java.util.Map; + +/** + * Key Provider Factory + */ +public class KeyProviderFactory { + private static final String SECRET_KEY_ALGORITHM = "AES"; + + /** + * Get Key Provider based on Configuration + * + * @param configuration Key Provider Configuration + * @return Key Provider + */ + public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { Review comment: I've got an idea to avoid downcasting and branching here. The _KeyProviderConfiguration_ interface could contain a new method: `KeyProvider getKeyProvider(final KeyProviderCreator keyProviderCreator);` A key provider creator class could handle creating the different providers based on different configurations: `public class KeyProviderCreator { private static final String SECRET_KEY_ALGORITHM = "AES"; public KeyProvider getKeyProvider(final StaticKeyProviderConfiguration configuration) { final Map<String, SecretKey> secretKeys; try { secretKeys = getSecretKeys(configuration.getKeys()); return new StaticKeyProvider(secretKeys); } catch (final DecoderException e) { throw new KeyReaderException("Decoding Hexadecimal Secret Keys failed", e); } } public KeyProvider getKeyProvider(final FileBasedKeyProviderConfiguration configuration) { final Path keyProviderPath = Paths.get(configuration.getLocation()); return new FileBasedKeyProvider(keyProviderPath, configuration.getRootKey()); } public KeyProvider getKeyProvider(final KeyStoreKeyProviderConfiguration configuration) { final KeyStore keyStore = configuration.getKeyStore(); return new KeyStoreKeyProvider(keyStore, configuration.getKeyPassword()); } private Map<String, SecretKey> getSecretKeys(final Map<String, String> keys) throws DecoderException { final Map<String, SecretKey> secretKeys = new HashMap<>(); for (final Map.Entry<String, String> keyEntry : keys.entrySet()) { final byte[] encodedSecretKey = Hex.decodeHex(keyEntry.getValue()); final SecretKey secretKey = new SecretKeySpec(encodedSecretKey, SECRET_KEY_ALGORITHM); secretKeys.put(keyEntry.getKey(), secretKey); } return secretKeys; } }` The configuration classes can utilize the creator class to make providers: `@Override public KeyProvider getKeyProvider(KeyProviderCreator keyProviderCreator) { return keyProviderCreator.getKeyProvider(this); }` And then in the factory, there's no branching remaining: `public class KeyProviderFactory { private static final KeyProviderCreator creator = new KeyProviderCreator(); public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { return configuration.getKeyProvider(creator); } }` What do you think of this approach? It adds a bit of extra complexity to other classes but withdraws some from the factory. Also this way when somebody adds a new configuration, they're obliged to implement the provider method. ########## File path: nifi-commons/nifi-security-kms/src/main/java/org/apache/nifi/security/kms/KeyProviderFactory.java ########## @@ -0,0 +1,85 @@ +/* + * 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.nifi.security.kms; + +import org.apache.commons.codec.DecoderException; +import org.apache.nifi.security.kms.configuration.FileBasedKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyStoreKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.StaticKeyProviderConfiguration; +import org.apache.commons.codec.binary.Hex; +import org.apache.nifi.security.kms.reader.KeyReaderException; + +import javax.crypto.SecretKey; +import javax.crypto.spec.SecretKeySpec; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.security.KeyStore; +import java.util.HashMap; +import java.util.Map; + +/** + * Key Provider Factory + */ +public class KeyProviderFactory { + private static final String SECRET_KEY_ALGORITHM = "AES"; + + /** + * Get Key Provider based on Configuration + * + * @param configuration Key Provider Configuration + * @return Key Provider + */ + public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { Review comment: I've got an idea to avoid downcasting and branching here. The _KeyProviderConfiguration_ interface could contain a new method: `KeyProvider getKeyProvider(final KeyProviderCreator keyProviderCreator);` A key provider creator class could handle creating the different providers based on different configurations: ` public class KeyProviderCreator { private static final String SECRET_KEY_ALGORITHM = "AES"; public KeyProvider getKeyProvider(final StaticKeyProviderConfiguration configuration) { final Map<String, SecretKey> secretKeys; try { secretKeys = getSecretKeys(configuration.getKeys()); return new StaticKeyProvider(secretKeys); } catch (final DecoderException e) { throw new KeyReaderException("Decoding Hexadecimal Secret Keys failed", e); } } public KeyProvider getKeyProvider(final FileBasedKeyProviderConfiguration configuration) { final Path keyProviderPath = Paths.get(configuration.getLocation()); return new FileBasedKeyProvider(keyProviderPath, configuration.getRootKey()); } public KeyProvider getKeyProvider(final KeyStoreKeyProviderConfiguration configuration) { final KeyStore keyStore = configuration.getKeyStore(); return new KeyStoreKeyProvider(keyStore, configuration.getKeyPassword()); } private Map<String, SecretKey> getSecretKeys(final Map<String, String> keys) throws DecoderException { final Map<String, SecretKey> secretKeys = new HashMap<>(); for (final Map.Entry<String, String> keyEntry : keys.entrySet()) { final byte[] encodedSecretKey = Hex.decodeHex(keyEntry.getValue()); final SecretKey secretKey = new SecretKeySpec(encodedSecretKey, SECRET_KEY_ALGORITHM); secretKeys.put(keyEntry.getKey(), secretKey); } return secretKeys; } }` The configuration classes can utilize the creator class to make providers: `@Override public KeyProvider getKeyProvider(KeyProviderCreator keyProviderCreator) { return keyProviderCreator.getKeyProvider(this); }` And then in the factory, there's no branching remaining: `public class KeyProviderFactory { private static final KeyProviderCreator creator = new KeyProviderCreator(); public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { return configuration.getKeyProvider(creator); } }` What do you think of this approach? It adds a bit of extra complexity to other classes but withdraws some from the factory. Also this way when somebody adds a new configuration, they're obliged to implement the provider method. ########## File path: nifi-commons/nifi-security-kms/src/main/java/org/apache/nifi/security/kms/KeyProviderFactory.java ########## @@ -0,0 +1,85 @@ +/* + * 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.nifi.security.kms; + +import org.apache.commons.codec.DecoderException; +import org.apache.nifi.security.kms.configuration.FileBasedKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyStoreKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.StaticKeyProviderConfiguration; +import org.apache.commons.codec.binary.Hex; +import org.apache.nifi.security.kms.reader.KeyReaderException; + +import javax.crypto.SecretKey; +import javax.crypto.spec.SecretKeySpec; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.security.KeyStore; +import java.util.HashMap; +import java.util.Map; + +/** + * Key Provider Factory + */ +public class KeyProviderFactory { + private static final String SECRET_KEY_ALGORITHM = "AES"; + + /** + * Get Key Provider based on Configuration + * + * @param configuration Key Provider Configuration + * @return Key Provider + */ + public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { Review comment: I've got an idea to avoid downcasting and branching here. The _KeyProviderConfiguration_ interface could contain a new method: `KeyProvider getKeyProvider(final KeyProviderCreator keyProviderCreator);` A key provider creator class could handle creating the different providers based on different configurations: ` public class KeyProviderCreator { private static final String SECRET_KEY_ALGORITHM = "AES"; public KeyProvider getKeyProvider(final StaticKeyProviderConfiguration configuration) { final Map<String, SecretKey> secretKeys; try { secretKeys = getSecretKeys(configuration.getKeys()); return new StaticKeyProvider(secretKeys); } catch (final DecoderException e) { throw new KeyReaderException("Decoding Hexadecimal Secret Keys failed", e); } } public KeyProvider getKeyProvider(final FileBasedKeyProviderConfiguration configuration) { final Path keyProviderPath = Paths.get(configuration.getLocation()); return new FileBasedKeyProvider(keyProviderPath, configuration.getRootKey()); } public KeyProvider getKeyProvider(final KeyStoreKeyProviderConfiguration configuration) { final KeyStore keyStore = configuration.getKeyStore(); return new KeyStoreKeyProvider(keyStore, configuration.getKeyPassword()); } private Map<String, SecretKey> getSecretKeys(final Map<String, String> keys) throws DecoderException { final Map<String, SecretKey> secretKeys = new HashMap<>(); for (final Map.Entry<String, String> keyEntry : keys.entrySet()) { final byte[] encodedSecretKey = Hex.decodeHex(keyEntry.getValue()); final SecretKey secretKey = new SecretKeySpec(encodedSecretKey, SECRET_KEY_ALGORITHM); secretKeys.put(keyEntry.getKey(), secretKey); } return secretKeys; } }` The configuration classes can utilize the creator class to make providers: `@Override public KeyProvider getKeyProvider(KeyProviderCreator keyProviderCreator) { return keyProviderCreator.getKeyProvider(this); }` And then in the factory, there's no branching remaining: `public class KeyProviderFactory { private static final KeyProviderCreator creator = new KeyProviderCreator(); public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { return configuration.getKeyProvider(creator); } }` What do you think of this approach? It adds a bit of extra complexity to other classes but withdraws some from the factory. Also this way when somebody adds a new configuration, they're obliged to implement the provider method. ########## File path: nifi-commons/nifi-security-kms/src/main/java/org/apache/nifi/security/kms/KeyProviderFactory.java ########## @@ -0,0 +1,85 @@ +/* + * 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.nifi.security.kms; + +import org.apache.commons.codec.DecoderException; +import org.apache.nifi.security.kms.configuration.FileBasedKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyStoreKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.StaticKeyProviderConfiguration; +import org.apache.commons.codec.binary.Hex; +import org.apache.nifi.security.kms.reader.KeyReaderException; + +import javax.crypto.SecretKey; +import javax.crypto.spec.SecretKeySpec; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.security.KeyStore; +import java.util.HashMap; +import java.util.Map; + +/** + * Key Provider Factory + */ +public class KeyProviderFactory { + private static final String SECRET_KEY_ALGORITHM = "AES"; + + /** + * Get Key Provider based on Configuration + * + * @param configuration Key Provider Configuration + * @return Key Provider + */ + public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { Review comment: I've got an idea to avoid downcasting and branching here. The _KeyProviderConfiguration_ interface could contain a new method: `KeyProvider getKeyProvider(final KeyProviderCreator keyProviderCreator);` A key provider creator class could handle creating the different providers based on different configurations: ` public class KeyProviderCreator { private static final String SECRET_KEY_ALGORITHM = "AES"; public KeyProvider getKeyProvider(final StaticKeyProviderConfiguration configuration) { final Map<String, SecretKey> secretKeys; try { secretKeys = getSecretKeys(configuration.getKeys()); return new StaticKeyProvider(secretKeys); } catch (final DecoderException e) { throw new KeyReaderException("Decoding Hexadecimal Secret Keys failed", e); } } public KeyProvider getKeyProvider(final FileBasedKeyProviderConfiguration configuration) { final Path keyProviderPath = Paths.get(configuration.getLocation()); return new FileBasedKeyProvider(keyProviderPath, configuration.getRootKey()); } public KeyProvider getKeyProvider(final KeyStoreKeyProviderConfiguration configuration) { final KeyStore keyStore = configuration.getKeyStore(); return new KeyStoreKeyProvider(keyStore, configuration.getKeyPassword()); } private Map<String, SecretKey> getSecretKeys(final Map<String, String> keys) throws DecoderException { final Map<String, SecretKey> secretKeys = new HashMap<>(); for (final Map.Entry<String, String> keyEntry : keys.entrySet()) { final byte[] encodedSecretKey = Hex.decodeHex(keyEntry.getValue()); final SecretKey secretKey = new SecretKeySpec(encodedSecretKey, SECRET_KEY_ALGORITHM); secretKeys.put(keyEntry.getKey(), secretKey); } return secretKeys; } }` The configuration classes can utilize the creator class to make providers: `@Override public KeyProvider getKeyProvider(KeyProviderCreator keyProviderCreator) { return keyProviderCreator.getKeyProvider(this); }` And then in the factory, there's no branching remaining: `public class KeyProviderFactory { private static final KeyProviderCreator creator = new KeyProviderCreator(); public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { return configuration.getKeyProvider(creator); } }` What do you think of this approach? It adds a bit of extra complexity to other classes but withdraws some from the factory. Also this way when somebody adds a new configuration, they're obliged to implement the provider method. ########## File path: nifi-commons/nifi-security-kms/src/main/java/org/apache/nifi/security/kms/KeyProviderFactory.java ########## @@ -0,0 +1,85 @@ +/* + * 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.nifi.security.kms; + +import org.apache.commons.codec.DecoderException; +import org.apache.nifi.security.kms.configuration.FileBasedKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyStoreKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.StaticKeyProviderConfiguration; +import org.apache.commons.codec.binary.Hex; +import org.apache.nifi.security.kms.reader.KeyReaderException; + +import javax.crypto.SecretKey; +import javax.crypto.spec.SecretKeySpec; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.security.KeyStore; +import java.util.HashMap; +import java.util.Map; + +/** + * Key Provider Factory + */ +public class KeyProviderFactory { + private static final String SECRET_KEY_ALGORITHM = "AES"; + + /** + * Get Key Provider based on Configuration + * + * @param configuration Key Provider Configuration + * @return Key Provider + */ + public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { Review comment: I've got an idea to avoid downcasting and branching here. The _KeyProviderConfiguration_ interface could contain a new method: `KeyProvider getKeyProvider(final KeyProviderCreator keyProviderCreator);` A key provider creator class could handle creating the different providers based on different configurations: ` public class KeyProviderCreator { private static final String SECRET_KEY_ALGORITHM = "AES"; public KeyProvider getKeyProvider(final StaticKeyProviderConfiguration configuration) { final Map<String, SecretKey> secretKeys; try { secretKeys = getSecretKeys(configuration.getKeys()); return new StaticKeyProvider(secretKeys); } catch (final DecoderException e) { throw new KeyReaderException("Decoding Hexadecimal Secret Keys failed", e); } } public KeyProvider getKeyProvider(final FileBasedKeyProviderConfiguration configuration) { final Path keyProviderPath = Paths.get(configuration.getLocation()); return new FileBasedKeyProvider(keyProviderPath, configuration.getRootKey()); } public KeyProvider getKeyProvider(final KeyStoreKeyProviderConfiguration configuration) { final KeyStore keyStore = configuration.getKeyStore(); return new KeyStoreKeyProvider(keyStore, configuration.getKeyPassword()); } private Map<String, SecretKey> getSecretKeys(final Map<String, String> keys) throws DecoderException { final Map<String, SecretKey> secretKeys = new HashMap<>(); for (final Map.Entry<String, String> keyEntry : keys.entrySet()) { final byte[] encodedSecretKey = Hex.decodeHex(keyEntry.getValue()); final SecretKey secretKey = new SecretKeySpec(encodedSecretKey, SECRET_KEY_ALGORITHM); secretKeys.put(keyEntry.getKey(), secretKey); } return secretKeys; } }` The configuration classes can utilize the creator class to make providers: `@Override public KeyProvider getKeyProvider(KeyProviderCreator keyProviderCreator) { return keyProviderCreator.getKeyProvider(this); }` And then in the factory, there's no branching remaining: `public class KeyProviderFactory { private static final KeyProviderCreator creator = new KeyProviderCreator(); public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { return configuration.getKeyProvider(creator); } }` What do you think of this approach? It adds a bit of extra complexity to other classes but withdraws some from the factory. Also this way when somebody adds a new configuration, they're obliged to implement the provider method. ########## File path: nifi-commons/nifi-security-kms/src/main/java/org/apache/nifi/security/kms/KeyProviderFactory.java ########## @@ -0,0 +1,85 @@ +/* + * 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.nifi.security.kms; + +import org.apache.commons.codec.DecoderException; +import org.apache.nifi.security.kms.configuration.FileBasedKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyStoreKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.StaticKeyProviderConfiguration; +import org.apache.commons.codec.binary.Hex; +import org.apache.nifi.security.kms.reader.KeyReaderException; + +import javax.crypto.SecretKey; +import javax.crypto.spec.SecretKeySpec; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.security.KeyStore; +import java.util.HashMap; +import java.util.Map; + +/** + * Key Provider Factory + */ +public class KeyProviderFactory { + private static final String SECRET_KEY_ALGORITHM = "AES"; + + /** + * Get Key Provider based on Configuration + * + * @param configuration Key Provider Configuration + * @return Key Provider + */ + public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { Review comment: I've got an idea to avoid downcasting and branching here. The _KeyProviderConfiguration_ interface could contain a new method: `KeyProvider getKeyProvider(final KeyProviderCreator keyProviderCreator);` A key provider creator class could handle creating the different providers based on different configurations: ` public class KeyProviderCreator { private static final String SECRET_KEY_ALGORITHM = "AES"; public KeyProvider getKeyProvider(final StaticKeyProviderConfiguration configuration) { final Map<String, SecretKey> secretKeys; try { secretKeys = getSecretKeys(configuration.getKeys()); return new StaticKeyProvider(secretKeys); } catch (final DecoderException e) { throw new KeyReaderException("Decoding Hexadecimal Secret Keys failed", e); } } public KeyProvider getKeyProvider(final FileBasedKeyProviderConfiguration configuration) { final Path keyProviderPath = Paths.get(configuration.getLocation()); return new FileBasedKeyProvider(keyProviderPath, configuration.getRootKey()); } public KeyProvider getKeyProvider(final KeyStoreKeyProviderConfiguration configuration) { final KeyStore keyStore = configuration.getKeyStore(); return new KeyStoreKeyProvider(keyStore, configuration.getKeyPassword()); } private Map<String, SecretKey> getSecretKeys(final Map<String, String> keys) throws DecoderException { final Map<String, SecretKey> secretKeys = new HashMap<>(); for (final Map.Entry<String, String> keyEntry : keys.entrySet()) { final byte[] encodedSecretKey = Hex.decodeHex(keyEntry.getValue()); final SecretKey secretKey = new SecretKeySpec(encodedSecretKey, SECRET_KEY_ALGORITHM); secretKeys.put(keyEntry.getKey(), secretKey); } return secretKeys; } }` The configuration classes can utilize the creator class to make providers: `@Override public KeyProvider getKeyProvider(KeyProviderCreator keyProviderCreator) { return keyProviderCreator.getKeyProvider(this); }` And then in the factory, there's no branching remaining: `public class KeyProviderFactory { private static final KeyProviderCreator creator = new KeyProviderCreator(); public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { return configuration.getKeyProvider(creator); } }` What do you think of this approach? It adds a bit of extra complexity to other classes but withdraws some from the factory. Also this way when somebody adds a new configuration, they're obliged to implement the provider method. ########## File path: nifi-commons/nifi-security-kms/src/main/java/org/apache/nifi/security/kms/KeyProviderFactory.java ########## @@ -0,0 +1,85 @@ +/* + * 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.nifi.security.kms; + +import org.apache.commons.codec.DecoderException; +import org.apache.nifi.security.kms.configuration.FileBasedKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyStoreKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.StaticKeyProviderConfiguration; +import org.apache.commons.codec.binary.Hex; +import org.apache.nifi.security.kms.reader.KeyReaderException; + +import javax.crypto.SecretKey; +import javax.crypto.spec.SecretKeySpec; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.security.KeyStore; +import java.util.HashMap; +import java.util.Map; + +/** + * Key Provider Factory + */ +public class KeyProviderFactory { + private static final String SECRET_KEY_ALGORITHM = "AES"; + + /** + * Get Key Provider based on Configuration + * + * @param configuration Key Provider Configuration + * @return Key Provider + */ + public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { Review comment: I've got an idea to avoid downcasting and branching here. The _KeyProviderConfiguration_ interface could contain a new method: `KeyProvider getKeyProvider(final KeyProviderCreator keyProviderCreator);` A key provider creator class could handle creating the different providers based on different configurations: ` public class KeyProviderCreator { private static final String SECRET_KEY_ALGORITHM = "AES"; public KeyProvider getKeyProvider(final StaticKeyProviderConfiguration configuration) { final Map<String, SecretKey> secretKeys; try { secretKeys = getSecretKeys(configuration.getKeys()); return new StaticKeyProvider(secretKeys); } catch (final DecoderException e) { throw new KeyReaderException("Decoding Hexadecimal Secret Keys failed", e); } } public KeyProvider getKeyProvider(final FileBasedKeyProviderConfiguration configuration) { final Path keyProviderPath = Paths.get(configuration.getLocation()); return new FileBasedKeyProvider(keyProviderPath, configuration.getRootKey()); } public KeyProvider getKeyProvider(final KeyStoreKeyProviderConfiguration configuration) { final KeyStore keyStore = configuration.getKeyStore(); return new KeyStoreKeyProvider(keyStore, configuration.getKeyPassword()); } private Map<String, SecretKey> getSecretKeys(final Map<String, String> keys) throws DecoderException { final Map<String, SecretKey> secretKeys = new HashMap<>(); for (final Map.Entry<String, String> keyEntry : keys.entrySet()) { final byte[] encodedSecretKey = Hex.decodeHex(keyEntry.getValue()); final SecretKey secretKey = new SecretKeySpec(encodedSecretKey, SECRET_KEY_ALGORITHM); secretKeys.put(keyEntry.getKey(), secretKey); } return secretKeys; } }` The configuration classes can utilize the creator class to make providers: `@Override public KeyProvider getKeyProvider(KeyProviderCreator keyProviderCreator) { return keyProviderCreator.getKeyProvider(this); }` And then in the factory, there's no branching remaining: `public class KeyProviderFactory { private static final KeyProviderCreator creator = new KeyProviderCreator(); public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { return configuration.getKeyProvider(creator); } }` What do you think of this approach? It adds a bit of extra complexity to other classes but withdraws some from the factory. Also this way when somebody adds a new configuration, they're obliged to implement the provider method. ########## File path: nifi-commons/nifi-security-kms/src/main/java/org/apache/nifi/security/kms/KeyProviderFactory.java ########## @@ -0,0 +1,85 @@ +/* + * 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.nifi.security.kms; + +import org.apache.commons.codec.DecoderException; +import org.apache.nifi.security.kms.configuration.FileBasedKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyStoreKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.StaticKeyProviderConfiguration; +import org.apache.commons.codec.binary.Hex; +import org.apache.nifi.security.kms.reader.KeyReaderException; + +import javax.crypto.SecretKey; +import javax.crypto.spec.SecretKeySpec; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.security.KeyStore; +import java.util.HashMap; +import java.util.Map; + +/** + * Key Provider Factory + */ +public class KeyProviderFactory { + private static final String SECRET_KEY_ALGORITHM = "AES"; + + /** + * Get Key Provider based on Configuration + * + * @param configuration Key Provider Configuration + * @return Key Provider + */ + public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { Review comment: I've got an idea to avoid downcasting and branching here. The _KeyProviderConfiguration_ interface could contain a new method: `KeyProvider getKeyProvider(final KeyProviderCreator keyProviderCreator);` A key provider creator class could handle creating the different providers based on different configurations: `java public class KeyProviderCreator { private static final String SECRET_KEY_ALGORITHM = "AES"; public KeyProvider getKeyProvider(final StaticKeyProviderConfiguration configuration) { final Map<String, SecretKey> secretKeys; try { secretKeys = getSecretKeys(configuration.getKeys()); return new StaticKeyProvider(secretKeys); } catch (final DecoderException e) { throw new KeyReaderException("Decoding Hexadecimal Secret Keys failed", e); } } public KeyProvider getKeyProvider(final FileBasedKeyProviderConfiguration configuration) { final Path keyProviderPath = Paths.get(configuration.getLocation()); return new FileBasedKeyProvider(keyProviderPath, configuration.getRootKey()); } public KeyProvider getKeyProvider(final KeyStoreKeyProviderConfiguration configuration) { final KeyStore keyStore = configuration.getKeyStore(); return new KeyStoreKeyProvider(keyStore, configuration.getKeyPassword()); } private Map<String, SecretKey> getSecretKeys(final Map<String, String> keys) throws DecoderException { final Map<String, SecretKey> secretKeys = new HashMap<>(); for (final Map.Entry<String, String> keyEntry : keys.entrySet()) { final byte[] encodedSecretKey = Hex.decodeHex(keyEntry.getValue()); final SecretKey secretKey = new SecretKeySpec(encodedSecretKey, SECRET_KEY_ALGORITHM); secretKeys.put(keyEntry.getKey(), secretKey); } return secretKeys; } }` The configuration classes can utilize the creator class to make providers: `@Override public KeyProvider getKeyProvider(KeyProviderCreator keyProviderCreator) { return keyProviderCreator.getKeyProvider(this); }` And then in the factory, there's no branching remaining: `public class KeyProviderFactory { private static final KeyProviderCreator creator = new KeyProviderCreator(); public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { return configuration.getKeyProvider(creator); } }` What do you think of this approach? It adds a bit of extra complexity to other classes but withdraws some from the factory. Also this way when somebody adds a new configuration, they're obliged to implement the provider method. ########## File path: nifi-commons/nifi-security-kms/src/main/java/org/apache/nifi/security/kms/KeyProviderFactory.java ########## @@ -0,0 +1,85 @@ +/* + * 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.nifi.security.kms; + +import org.apache.commons.codec.DecoderException; +import org.apache.nifi.security.kms.configuration.FileBasedKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyStoreKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.StaticKeyProviderConfiguration; +import org.apache.commons.codec.binary.Hex; +import org.apache.nifi.security.kms.reader.KeyReaderException; + +import javax.crypto.SecretKey; +import javax.crypto.spec.SecretKeySpec; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.security.KeyStore; +import java.util.HashMap; +import java.util.Map; + +/** + * Key Provider Factory + */ +public class KeyProviderFactory { + private static final String SECRET_KEY_ALGORITHM = "AES"; + + /** + * Get Key Provider based on Configuration + * + * @param configuration Key Provider Configuration + * @return Key Provider + */ + public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { Review comment: I've got an idea to avoid downcasting and branching here. The _KeyProviderConfiguration_ interface could contain a new method: `KeyProvider getKeyProvider(final KeyProviderCreator keyProviderCreator);` A key provider creator class could handle creating the different providers based on different configurations: ` public class KeyProviderCreator { private static final String SECRET_KEY_ALGORITHM = "AES"; public KeyProvider getKeyProvider(final StaticKeyProviderConfiguration configuration) { final Map<String, SecretKey> secretKeys; try { secretKeys = getSecretKeys(configuration.getKeys()); return new StaticKeyProvider(secretKeys); } catch (final DecoderException e) { throw new KeyReaderException("Decoding Hexadecimal Secret Keys failed", e); } } public KeyProvider getKeyProvider(final FileBasedKeyProviderConfiguration configuration) { final Path keyProviderPath = Paths.get(configuration.getLocation()); return new FileBasedKeyProvider(keyProviderPath, configuration.getRootKey()); } public KeyProvider getKeyProvider(final KeyStoreKeyProviderConfiguration configuration) { final KeyStore keyStore = configuration.getKeyStore(); return new KeyStoreKeyProvider(keyStore, configuration.getKeyPassword()); } private Map<String, SecretKey> getSecretKeys(final Map<String, String> keys) throws DecoderException { final Map<String, SecretKey> secretKeys = new HashMap<>(); for (final Map.Entry<String, String> keyEntry : keys.entrySet()) { final byte[] encodedSecretKey = Hex.decodeHex(keyEntry.getValue()); final SecretKey secretKey = new SecretKeySpec(encodedSecretKey, SECRET_KEY_ALGORITHM); secretKeys.put(keyEntry.getKey(), secretKey); } return secretKeys; } }` The configuration classes can utilize the creator class to make providers: `@Override public KeyProvider getKeyProvider(KeyProviderCreator keyProviderCreator) { return keyProviderCreator.getKeyProvider(this); }` And then in the factory, there's no branching remaining: `public class KeyProviderFactory { private static final KeyProviderCreator creator = new KeyProviderCreator(); public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { return configuration.getKeyProvider(creator); } }` What do you think of this approach? It adds a bit of extra complexity to other classes but withdraws some from the factory. Also this way when somebody adds a new configuration, they're obliged to implement the provider method. ########## File path: nifi-commons/nifi-security-kms/src/main/java/org/apache/nifi/security/kms/KeyProviderFactory.java ########## @@ -0,0 +1,85 @@ +/* + * 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.nifi.security.kms; + +import org.apache.commons.codec.DecoderException; +import org.apache.nifi.security.kms.configuration.FileBasedKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyStoreKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.StaticKeyProviderConfiguration; +import org.apache.commons.codec.binary.Hex; +import org.apache.nifi.security.kms.reader.KeyReaderException; + +import javax.crypto.SecretKey; +import javax.crypto.spec.SecretKeySpec; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.security.KeyStore; +import java.util.HashMap; +import java.util.Map; + +/** + * Key Provider Factory + */ +public class KeyProviderFactory { + private static final String SECRET_KEY_ALGORITHM = "AES"; + + /** + * Get Key Provider based on Configuration + * + * @param configuration Key Provider Configuration + * @return Key Provider + */ + public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { Review comment: I've got an idea to avoid downcasting and branching here. The _KeyProviderConfiguration_ interface could contain a new method: `KeyProvider getKeyProvider(final KeyProviderCreator keyProviderCreator);` A key provider creator class could handle creating the different providers based on different configurations: public class KeyProviderCreator { private static final String SECRET_KEY_ALGORITHM = "AES"; public KeyProvider getKeyProvider(final StaticKeyProviderConfiguration configuration) { final Map<String, SecretKey> secretKeys; try { secretKeys = getSecretKeys(configuration.getKeys()); return new StaticKeyProvider(secretKeys); } catch (final DecoderException e) { throw new KeyReaderException("Decoding Hexadecimal Secret Keys failed", e); } } public KeyProvider getKeyProvider(final FileBasedKeyProviderConfiguration configuration) { final Path keyProviderPath = Paths.get(configuration.getLocation()); return new FileBasedKeyProvider(keyProviderPath, configuration.getRootKey()); } public KeyProvider getKeyProvider(final KeyStoreKeyProviderConfiguration configuration) { final KeyStore keyStore = configuration.getKeyStore(); return new KeyStoreKeyProvider(keyStore, configuration.getKeyPassword()); } private Map<String, SecretKey> getSecretKeys(final Map<String, String> keys) throws DecoderException { final Map<String, SecretKey> secretKeys = new HashMap<>(); for (final Map.Entry<String, String> keyEntry : keys.entrySet()) { final byte[] encodedSecretKey = Hex.decodeHex(keyEntry.getValue()); final SecretKey secretKey = new SecretKeySpec(encodedSecretKey, SECRET_KEY_ALGORITHM); secretKeys.put(keyEntry.getKey(), secretKey); } return secretKeys; } }` The configuration classes can utilize the creator class to make providers: `@Override public KeyProvider getKeyProvider(KeyProviderCreator keyProviderCreator) { return keyProviderCreator.getKeyProvider(this); }` And then in the factory, there's no branching remaining: `public class KeyProviderFactory { private static final KeyProviderCreator creator = new KeyProviderCreator(); public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { return configuration.getKeyProvider(creator); } }` What do you think of this approach? It adds a bit of extra complexity to other classes but withdraws some from the factory. Also this way when somebody adds a new configuration, they're obliged to implement the provider method. ########## File path: nifi-commons/nifi-security-kms/src/main/java/org/apache/nifi/security/kms/KeyProviderFactory.java ########## @@ -0,0 +1,85 @@ +/* + * 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.nifi.security.kms; + +import org.apache.commons.codec.DecoderException; +import org.apache.nifi.security.kms.configuration.FileBasedKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyStoreKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.StaticKeyProviderConfiguration; +import org.apache.commons.codec.binary.Hex; +import org.apache.nifi.security.kms.reader.KeyReaderException; + +import javax.crypto.SecretKey; +import javax.crypto.spec.SecretKeySpec; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.security.KeyStore; +import java.util.HashMap; +import java.util.Map; + +/** + * Key Provider Factory + */ +public class KeyProviderFactory { + private static final String SECRET_KEY_ALGORITHM = "AES"; + + /** + * Get Key Provider based on Configuration + * + * @param configuration Key Provider Configuration + * @return Key Provider + */ + public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { Review comment: I've got an idea to avoid downcasting and branching here. The _KeyProviderConfiguration_ interface could contain a new method: `KeyProvider getKeyProvider(final KeyProviderCreator keyProviderCreator);` A key provider creator class could handle creating the different providers based on different configurations: public class KeyProviderCreator { private static final String SECRET_KEY_ALGORITHM = "AES"; public KeyProvider getKeyProvider(final StaticKeyProviderConfiguration configuration) { final Map<String, SecretKey> secretKeys; try { secretKeys = getSecretKeys(configuration.getKeys()); return new StaticKeyProvider(secretKeys); } catch (final DecoderException e) { throw new KeyReaderException("Decoding Hexadecimal Secret Keys failed", e); } } public KeyProvider getKeyProvider(final FileBasedKeyProviderConfiguration configuration) { final Path keyProviderPath = Paths.get(configuration.getLocation()); return new FileBasedKeyProvider(keyProviderPath, configuration.getRootKey()); } public KeyProvider getKeyProvider(final KeyStoreKeyProviderConfiguration configuration) { final KeyStore keyStore = configuration.getKeyStore(); return new KeyStoreKeyProvider(keyStore, configuration.getKeyPassword()); } private Map<String, SecretKey> getSecretKeys(final Map<String, String> keys) throws DecoderException { final Map<String, SecretKey> secretKeys = new HashMap<>(); for (final Map.Entry<String, String> keyEntry : keys.entrySet()) { final byte[] encodedSecretKey = Hex.decodeHex(keyEntry.getValue()); final SecretKey secretKey = new SecretKeySpec(encodedSecretKey, SECRET_KEY_ALGORITHM); secretKeys.put(keyEntry.getKey(), secretKey); } return secretKeys; } }` The configuration classes can utilize the creator class to make providers: ` @Override public KeyProvider getKeyProvider(KeyProviderCreator keyProviderCreator) { return keyProviderCreator.getKeyProvider(this); } ` And then in the factory, there's no branching remaining: `public class KeyProviderFactory { private static final KeyProviderCreator creator = new KeyProviderCreator(); public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { return configuration.getKeyProvider(creator); } }` What do you think of this approach? It adds a bit of extra complexity to other classes but withdraws some from the factory. Also this way when somebody adds a new configuration, they're obliged to implement the provider method. ########## File path: nifi-commons/nifi-security-kms/src/main/java/org/apache/nifi/security/kms/KeyProviderFactory.java ########## @@ -0,0 +1,85 @@ +/* + * 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.nifi.security.kms; + +import org.apache.commons.codec.DecoderException; +import org.apache.nifi.security.kms.configuration.FileBasedKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyStoreKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.StaticKeyProviderConfiguration; +import org.apache.commons.codec.binary.Hex; +import org.apache.nifi.security.kms.reader.KeyReaderException; + +import javax.crypto.SecretKey; +import javax.crypto.spec.SecretKeySpec; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.security.KeyStore; +import java.util.HashMap; +import java.util.Map; + +/** + * Key Provider Factory + */ +public class KeyProviderFactory { + private static final String SECRET_KEY_ALGORITHM = "AES"; + + /** + * Get Key Provider based on Configuration + * + * @param configuration Key Provider Configuration + * @return Key Provider + */ + public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { Review comment: I've got an idea to avoid downcasting and branching here. The _KeyProviderConfiguration_ interface could contain a new method: `KeyProvider getKeyProvider(final KeyProviderCreator keyProviderCreator);` A key provider creator class could handle creating the different providers based on different configurations: public class KeyProviderCreator { private static final String SECRET_KEY_ALGORITHM = "AES"; public KeyProvider getKeyProvider(final StaticKeyProviderConfiguration configuration) { final Map<String, SecretKey> secretKeys; try { secretKeys = getSecretKeys(configuration.getKeys()); return new StaticKeyProvider(secretKeys); } catch (final DecoderException e) { throw new KeyReaderException("Decoding Hexadecimal Secret Keys failed", e); } } public KeyProvider getKeyProvider(final FileBasedKeyProviderConfiguration configuration) { final Path keyProviderPath = Paths.get(configuration.getLocation()); return new FileBasedKeyProvider(keyProviderPath, configuration.getRootKey()); } public KeyProvider getKeyProvider(final KeyStoreKeyProviderConfiguration configuration) { final KeyStore keyStore = configuration.getKeyStore(); return new KeyStoreKeyProvider(keyStore, configuration.getKeyPassword()); } private Map<String, SecretKey> getSecretKeys(final Map<String, String> keys) throws DecoderException { final Map<String, SecretKey> secretKeys = new HashMap<>(); for (final Map.Entry<String, String> keyEntry : keys.entrySet()) { final byte[] encodedSecretKey = Hex.decodeHex(keyEntry.getValue()); final SecretKey secretKey = new SecretKeySpec(encodedSecretKey, SECRET_KEY_ALGORITHM); secretKeys.put(keyEntry.getKey(), secretKey); } return secretKeys; } }` The configuration classes can utilize the creator class to make providers: ` @Override public KeyProvider getKeyProvider(KeyProviderCreator keyProviderCreator) { return keyProviderCreator.getKeyProvider(this); }` And then in the factory, there's no branching remaining: `public class KeyProviderFactory { private static final KeyProviderCreator creator = new KeyProviderCreator(); public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { return configuration.getKeyProvider(creator); } }` What do you think of this approach? It adds a bit of extra complexity to other classes but withdraws some from the factory. Also this way when somebody adds a new configuration, they're obliged to implement the provider method. ########## File path: nifi-commons/nifi-security-kms/src/main/java/org/apache/nifi/security/kms/KeyProviderFactory.java ########## @@ -0,0 +1,85 @@ +/* + * 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.nifi.security.kms; + +import org.apache.commons.codec.DecoderException; +import org.apache.nifi.security.kms.configuration.FileBasedKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.KeyStoreKeyProviderConfiguration; +import org.apache.nifi.security.kms.configuration.StaticKeyProviderConfiguration; +import org.apache.commons.codec.binary.Hex; +import org.apache.nifi.security.kms.reader.KeyReaderException; + +import javax.crypto.SecretKey; +import javax.crypto.spec.SecretKeySpec; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.security.KeyStore; +import java.util.HashMap; +import java.util.Map; + +/** + * Key Provider Factory + */ +public class KeyProviderFactory { + private static final String SECRET_KEY_ALGORITHM = "AES"; + + /** + * Get Key Provider based on Configuration + * + * @param configuration Key Provider Configuration + * @return Key Provider + */ + public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { Review comment: I've got an idea to avoid downcasting and branching here. The _KeyProviderConfiguration_ interface could contain a new method: `KeyProvider getKeyProvider(final KeyProviderCreator keyProviderCreator);` A key provider creator class could handle creating the different providers based on different configurations: public class KeyProviderCreator { private static final String SECRET_KEY_ALGORITHM = "AES"; public KeyProvider getKeyProvider(final StaticKeyProviderConfiguration configuration) { final Map<String, SecretKey> secretKeys; try { secretKeys = getSecretKeys(configuration.getKeys()); return new StaticKeyProvider(secretKeys); } catch (final DecoderException e) { throw new KeyReaderException("Decoding Hexadecimal Secret Keys failed", e); } } public KeyProvider getKeyProvider(final FileBasedKeyProviderConfiguration configuration) { final Path keyProviderPath = Paths.get(configuration.getLocation()); return new FileBasedKeyProvider(keyProviderPath, configuration.getRootKey()); } public KeyProvider getKeyProvider(final KeyStoreKeyProviderConfiguration configuration) { final KeyStore keyStore = configuration.getKeyStore(); return new KeyStoreKeyProvider(keyStore, configuration.getKeyPassword()); } private Map<String, SecretKey> getSecretKeys(final Map<String, String> keys) throws DecoderException { final Map<String, SecretKey> secretKeys = new HashMap<>(); for (final Map.Entry<String, String> keyEntry : keys.entrySet()) { final byte[] encodedSecretKey = Hex.decodeHex(keyEntry.getValue()); final SecretKey secretKey = new SecretKeySpec(encodedSecretKey, SECRET_KEY_ALGORITHM); secretKeys.put(keyEntry.getKey(), secretKey); } return secretKeys; } }` The configuration classes can utilize the creator class to make providers: ` @Override public KeyProvider getKeyProvider(KeyProviderCreator keyProviderCreator) { return keyProviderCreator.getKeyProvider(this); }` And then in the factory, there's no branching remaining: `public class KeyProviderFactory { private static final KeyProviderCreator creator = new KeyProviderCreator(); public static KeyProvider getKeyProvider(final KeyProviderConfiguration<?> configuration) { return configuration.getKeyProvider(creator); } }` What do you think of this approach? It adds a bit of extra complexity to other classes but withdraws some from the factory. Also, this way if anyone adds a new configuration, they will be obliged to implement the provider method. ########## File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/kms/CryptoUtils.java ########## @@ -133,43 +124,28 @@ public static boolean isValidRepositoryEncryptionConfiguration(RepositoryEncrypt * @return true if the provided configuration is valid */ public static boolean isValidKeyProvider(String keyProviderImplementation, String keyProviderLocation, String keyId, Map<String, String> encryptionKeys) { - logger.debug("Attempting to validate the key provider: keyProviderImplementation = " - + keyProviderImplementation + ", keyProviderLocation = " - + keyProviderLocation + ", keyId = " - + keyId + ", encryptionKeys = " - + ((encryptionKeys == null) ? "0" : encryptionKeys.size())); - try { keyProviderImplementation = handleLegacyPackages(keyProviderImplementation); - } catch (KeyManagementException e) { - logger.warn("The attempt to validate the key provider failed keyProviderImplementation = " - + keyProviderImplementation + ", keyProviderLocation = " - + keyProviderLocation + ", keyId = " - + keyId + ", encryptionKeys = " - + ((encryptionKeys == null) ? "0" : encryptionKeys.size())); - + } catch (final KeyManagementException e) { + logger.warn("Key Provider [{}] Validation Failed: {}", keyProviderImplementation, e.getMessage()); return false; } if (STATIC_KEY_PROVIDER_CLASS_NAME.equals(keyProviderImplementation)) { Review comment: Minor: Would you consider using switch here? It's more readable. ########## File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/kms/CryptoUtils.java ########## @@ -133,43 +124,28 @@ public static boolean isValidRepositoryEncryptionConfiguration(RepositoryEncrypt * @return true if the provided configuration is valid */ public static boolean isValidKeyProvider(String keyProviderImplementation, String keyProviderLocation, String keyId, Map<String, String> encryptionKeys) { - logger.debug("Attempting to validate the key provider: keyProviderImplementation = " - + keyProviderImplementation + ", keyProviderLocation = " - + keyProviderLocation + ", keyId = " - + keyId + ", encryptionKeys = " - + ((encryptionKeys == null) ? "0" : encryptionKeys.size())); - try { keyProviderImplementation = handleLegacyPackages(keyProviderImplementation); - } catch (KeyManagementException e) { - logger.warn("The attempt to validate the key provider failed keyProviderImplementation = " - + keyProviderImplementation + ", keyProviderLocation = " - + keyProviderLocation + ", keyId = " - + keyId + ", encryptionKeys = " - + ((encryptionKeys == null) ? "0" : encryptionKeys.size())); - + } catch (final KeyManagementException e) { + logger.warn("Key Provider [{}] Validation Failed: {}", keyProviderImplementation, e.getMessage()); return false; } if (STATIC_KEY_PROVIDER_CLASS_NAME.equals(keyProviderImplementation)) { Review comment: Minor: Would you consider using switch here? I think it's more readable. ########## File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/KeyStoreUtils.java ########## @@ -27,23 +27,24 @@ import java.nio.file.Files; import java.nio.file.Path; import java.security.GeneralSecurityException; -import java.security.Key; import java.security.KeyPair; import java.security.KeyPairGenerator; import java.security.KeyStore; import java.security.KeyStoreException; import java.security.NoSuchAlgorithmException; +import java.security.NoSuchProviderException; Review comment: Would you please remove the unused method on the 543rd line? ########## File path: nifi-nar-bundles/nifi-provenance-repository-bundle/nifi-persistent-provenance-repository/src/main/java/org/apache/nifi/provenance/EncryptedWriteAheadProvenanceRepository.java ########## @@ -133,33 +124,16 @@ public synchronized void initialize(final EventReporter eventReporter, final Aut super.init(recordWriterFactory, recordReaderFactory, eventReporter, authorizer, resourceFactory); } - private KeyProvider buildKeyProvider() throws KeyManagementException { - return buildKeyProvider(null); - } - - private KeyProvider buildKeyProvider(SecretKey rootKey) throws KeyManagementException { - RepositoryConfiguration config = super.getConfig(); - if (config == null) { - throw new KeyManagementException("The repository configuration is missing"); - } - - final String implementationClassName = config.getKeyProviderImplementation(); - if (implementationClassName == null) { - throw new KeyManagementException("Cannot create Key Provider because the NiFi Properties is missing the following property: " - + NiFiProperties.PROVENANCE_REPO_ENCRYPTION_KEY_PROVIDER_IMPLEMENTATION_CLASS); - } - - return KeyProviderFactory.buildKeyProvider(implementationClassName, config.getKeyProviderLocation(), config.getKeyId(), config.getEncryptionKeys(), rootKey); - } - - private static SecretKey getRootKey() throws KeyManagementException { - try { - // Get the root encryption key from bootstrap.conf - String rootKeyHex = CryptoUtils.extractKeyFromBootstrapFile(); - return new SecretKeySpec(Hex.decodeHex(rootKeyHex.toCharArray()), "AES"); - } catch (IOException | DecoderException e) { - logger.error("Encountered an error: ", e); - throw new KeyManagementException(e); - } + private KeyProvider buildKeyProvider() throws KeyManagementException, IOException { Review comment: KeyManagementException is never thrown in the method. -- 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: [email protected]
