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. -- 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: us...@infra.apache.org
