exceptionfactory commented on a change in pull request #4867: URL: https://github.com/apache/nifi/pull/4867#discussion_r585818632
########## File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/crypto/SecureHasherFactory.java ########## @@ -0,0 +1,69 @@ +/* + * 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.util.crypto; + +import org.apache.nifi.security.util.KeyDerivationFunction; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.lang.reflect.InvocationTargetException; +import java.util.HashMap; +import java.util.Map; + +/** + * <p> Provides a factory for SecureHasher implementations. Will return Argon2 by default if no algorithm parameter is given. + * Algorithm parameter should align with the below registered secure hasher names (PBKDF2, BCRYPT, SCRYPT, ARGON2). + */ +public class SecureHasherFactory { + private static final Logger LOGGER = LoggerFactory.getLogger(SecureHasherFactory.class); + + private static Map<KeyDerivationFunction, Class<? extends SecureHasher>> registeredSecureHashers; + private static final KeyDerivationFunction DEFAULT_HASHER = KeyDerivationFunction.ARGON2; + + static { + registeredSecureHashers = new HashMap<>(); + registeredSecureHashers.put(KeyDerivationFunction.PBKDF2, PBKDF2SecureHasher.class); + registeredSecureHashers.put(KeyDerivationFunction.BCRYPT, BcryptSecureHasher.class); + registeredSecureHashers.put(KeyDerivationFunction.SCRYPT, ScryptSecureHasher.class); + registeredSecureHashers.put(KeyDerivationFunction.ARGON2, Argon2SecureHasher.class); + } + + public static SecureHasher getSecureHasher(final String algorithm) { + String hasherAlgorithm = algorithm; + + try { + for(KeyDerivationFunction hashingFunction : registeredSecureHashers.keySet()) { + if (hasherAlgorithm.toUpperCase().contains(hashingFunction.getKdfName().toUpperCase())) { + return instantiateHasher(hashingFunction, hasherAlgorithm); + } + } + + hasherAlgorithm = DEFAULT_HASHER.getKdfName(); + LOGGER.error("Failed to instantiate SecureHasher for algorithm [{}]. Trying [{}] instead", algorithm, hasherAlgorithm); Review comment: Logging this as an error is probably not the best approach since any of the existing PBE algorithms will fall through to the default hasher. Perhaps log as a debug message? ########## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/encrypt/SensitiveValueEncoder.java ########## @@ -0,0 +1,22 @@ +/* + * 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.encrypt; + Review comment: Recommend adding a basic comment on the purpose of this interface. ########## File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/crypto/SecureHasherFactory.java ########## @@ -0,0 +1,69 @@ +/* + * 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.util.crypto; + +import org.apache.nifi.security.util.KeyDerivationFunction; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.lang.reflect.InvocationTargetException; +import java.util.HashMap; +import java.util.Map; + +/** + * <p> Provides a factory for SecureHasher implementations. Will return Argon2 by default if no algorithm parameter is given. + * Algorithm parameter should align with the below registered secure hasher names (PBKDF2, BCRYPT, SCRYPT, ARGON2). + */ +public class SecureHasherFactory { + private static final Logger LOGGER = LoggerFactory.getLogger(SecureHasherFactory.class); + + private static Map<KeyDerivationFunction, Class<? extends SecureHasher>> registeredSecureHashers; + private static final KeyDerivationFunction DEFAULT_HASHER = KeyDerivationFunction.ARGON2; + + static { + registeredSecureHashers = new HashMap<>(); + registeredSecureHashers.put(KeyDerivationFunction.PBKDF2, PBKDF2SecureHasher.class); + registeredSecureHashers.put(KeyDerivationFunction.BCRYPT, BcryptSecureHasher.class); + registeredSecureHashers.put(KeyDerivationFunction.SCRYPT, ScryptSecureHasher.class); + registeredSecureHashers.put(KeyDerivationFunction.ARGON2, Argon2SecureHasher.class); + } + + public static SecureHasher getSecureHasher(final String algorithm) { + String hasherAlgorithm = algorithm; + + try { + for(KeyDerivationFunction hashingFunction : registeredSecureHashers.keySet()) { + if (hasherAlgorithm.toUpperCase().contains(hashingFunction.getKdfName().toUpperCase())) { + return instantiateHasher(hashingFunction, hasherAlgorithm); + } + } + + hasherAlgorithm = DEFAULT_HASHER.getKdfName(); + LOGGER.error("Failed to instantiate SecureHasher for algorithm [{}]. Trying [{}] instead", algorithm, hasherAlgorithm); + return instantiateHasher(DEFAULT_HASHER, DEFAULT_HASHER.getKdfName()); + } catch (Exception e) { + throw new SecureHasherException(String.format("SecureHasher instantiation failed for algorithm [%s]", hasherAlgorithm), e); + } + } + + private static SecureHasher instantiateHasher(KeyDerivationFunction hashingFunction, String hasherAlgorithm) Review comment: With changing the calling method to log the algorithm, this method could be changed to accept a single SecureHasher Class: ```suggestion private static SecureHasher instantiateHasher(final Class<? extends SecureHasher> secureHasherClass) ``` ########## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/encrypt/StandardSensitiveValueEncoder.java ########## @@ -0,0 +1,77 @@ +/* + * 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.encrypt; + +import org.apache.nifi.security.util.crypto.SecureHasher; +import org.apache.nifi.security.util.crypto.SecureHasherFactory; +import org.apache.nifi.util.NiFiProperties; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import javax.crypto.Mac; +import javax.crypto.spec.SecretKeySpec; +import java.nio.charset.StandardCharsets; +import java.security.InvalidKeyException; +import java.security.NoSuchAlgorithmException; +import java.util.Base64; +import java.util.Objects; + +/** + * Encode a sensitive value using the NiFi sensitive properties key to derive the secret key used in the MAC operation. + */ +public class StandardSensitiveValueEncoder implements SensitiveValueEncoder { + + private static final Logger logger = LoggerFactory.getLogger(StandardSensitiveValueEncoder.class); + + private SecretKeySpec secretKeySpec; + private static final String HMAC_SHA256 = "HmacSHA256"; + + public StandardSensitiveValueEncoder(final NiFiProperties properties) { + this(properties.getProperty(NiFiProperties.SENSITIVE_PROPS_KEY), + SecureHasherFactory.getSecureHasher(properties.getProperty(NiFiProperties.SENSITIVE_PROPS_ALGORITHM))); + } + + // We use the sensitive properties key and a SecureHasher impl to derive a secret key for the getEncoded() method + public StandardSensitiveValueEncoder(final String sensitivePropertiesKey, final SecureHasher hasher) { Review comment: Should this constructor be marked `private` since it does not appear to be used elsewhere? On the other hand, the other constructor is more convenient, but given that only two properties are necessary, it is worth having to pass the entire `NiFiProperties` object? ########## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/encrypt/SensitiveValueEncoder.java ########## @@ -0,0 +1,22 @@ +/* + * 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.encrypt; + +public interface SensitiveValueEncoder { + + String getEncoded(String plaintextSensitivePropertyValue); Review comment: What do you think of shortening this variable to just `sensitivePropertyValue` or `sensitiveValue`? ########## File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/crypto/SecureHasherFactory.java ########## @@ -0,0 +1,69 @@ +/* + * 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.util.crypto; + +import org.apache.nifi.security.util.KeyDerivationFunction; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.lang.reflect.InvocationTargetException; +import java.util.HashMap; +import java.util.Map; + +/** + * <p> Provides a factory for SecureHasher implementations. Will return Argon2 by default if no algorithm parameter is given. + * Algorithm parameter should align with the below registered secure hasher names (PBKDF2, BCRYPT, SCRYPT, ARGON2). + */ +public class SecureHasherFactory { + private static final Logger LOGGER = LoggerFactory.getLogger(SecureHasherFactory.class); + + private static Map<KeyDerivationFunction, Class<? extends SecureHasher>> registeredSecureHashers; + private static final KeyDerivationFunction DEFAULT_HASHER = KeyDerivationFunction.ARGON2; + + static { + registeredSecureHashers = new HashMap<>(); + registeredSecureHashers.put(KeyDerivationFunction.PBKDF2, PBKDF2SecureHasher.class); + registeredSecureHashers.put(KeyDerivationFunction.BCRYPT, BcryptSecureHasher.class); + registeredSecureHashers.put(KeyDerivationFunction.SCRYPT, ScryptSecureHasher.class); + registeredSecureHashers.put(KeyDerivationFunction.ARGON2, Argon2SecureHasher.class); + } + + public static SecureHasher getSecureHasher(final String algorithm) { + String hasherAlgorithm = algorithm; + + try { + for(KeyDerivationFunction hashingFunction : registeredSecureHashers.keySet()) { + if (hasherAlgorithm.toUpperCase().contains(hashingFunction.getKdfName().toUpperCase())) { + return instantiateHasher(hashingFunction, hasherAlgorithm); + } + } + + hasherAlgorithm = DEFAULT_HASHER.getKdfName(); + LOGGER.error("Failed to instantiate SecureHasher for algorithm [{}]. Trying [{}] instead", algorithm, hasherAlgorithm); + return instantiateHasher(DEFAULT_HASHER, DEFAULT_HASHER.getKdfName()); Review comment: Given that the current default algorithm does not reference any of the Key Derivation Functions, what do you think of the following approach to use the default value and log at the debug level? ```suggestion Class<? extends SecureHasher> secureHasherClass = DEFAULT_SECURE_HASHER_CLASS; final String algorithmPattern = algorithm.toUpperCase(); try { for (KeyDerivationFunction keyDerivationFunction : registeredSecureHashers.keySet()) { final String functionName = keyDerivationFunction.getKdfName().toUpperCase(); if (algorithmPattern.contains(functionName)) { secureHasherClass = registeredSecureHashers.get(keyDerivationFunction); } } LOGGER.debug("Creating SecureHasher [{}] for Algorithm [{}]", secureHasherClass.getName(), algorithm); return instantiateHasher(secureHasherClass); ``` ########## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/test/groovy/org/apache/nifi/cluster/coordination/flow/PopularVoteFlowElectionFactoryBeanTest.groovy ########## @@ -59,29 +59,29 @@ class PopularVoteFlowElectionFactoryBeanTest extends GroovyTestCase { mockProps } - @Test - void testGetObjectShouldPopulateDefaultSensitivePropsKeyIfEmpty() { - // Arrange - PopularVoteFlowElectionFactoryBean electionFactoryBean = new PopularVoteFlowElectionFactoryBean() - electionFactoryBean.properties = mockProperties() - - final PropertyEncryptor DEFAULT_ENCRYPTOR = PropertyEncryptorFactory.getPropertyEncryptor(mockProperties()) - final String EXPECTED_PLAINTEXT = "my.test.value" - final String EXPECTED_CIPHERTEXT = DEFAULT_ENCRYPTOR.encrypt(EXPECTED_PLAINTEXT) - logger.info("Expected ciphertext: ${EXPECTED_CIPHERTEXT}") - - // Act - PopularVoteFlowElection election = electionFactoryBean.object - logger.info("Got object: ${election}") - - // Assert - - // Violates LoD but need to evaluate nested encryptor can decrypt - def encryptor = election.fingerprintFactory.encryptor - String decrypted = encryptor.decrypt(EXPECTED_CIPHERTEXT) - logger.info("Decrypted plain text: ${decrypted}") - assert decrypted == EXPECTED_PLAINTEXT - } +// @Test +// void testGetObjectShouldPopulateDefaultSensitivePropsKeyIfEmpty() { +// // Arrange +// PopularVoteFlowElectionFactoryBean electionFactoryBean = new PopularVoteFlowElectionFactoryBean() +// electionFactoryBean.properties = mockProperties() +// +// final PropertyEncryptor DEFAULT_ENCRYPTOR = PropertyEncryptorFactory.getPropertyEncryptor(mockProperties()) +// final String EXPECTED_PLAINTEXT = "my.test.value" +// final String EXPECTED_CIPHERTEXT = DEFAULT_ENCRYPTOR.encrypt(EXPECTED_PLAINTEXT) +// logger.info("Expected ciphertext: ${EXPECTED_CIPHERTEXT}") +// +// // Act +// PopularVoteFlowElection election = electionFactoryBean.object +// logger.info("Got object: ${election}") +// +// // Assert +// +// // Violates LoD but need to evaluate nested encryptor can decrypt +// def encryptor = election.fingerprintFactory.encryptor +// String decrypted = encryptor.decrypt(EXPECTED_CIPHERTEXT) +// logger.info("Decrypted plain text: ${decrypted}") +// assert decrypted == EXPECTED_PLAINTEXT +// } Review comment: Recommend removing this test method if it is no longer applicable or functional. ########## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/encrypt/StandardSensitiveValueEncoder.java ########## @@ -0,0 +1,77 @@ +/* + * 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.encrypt; + +import org.apache.nifi.security.util.crypto.SecureHasher; +import org.apache.nifi.security.util.crypto.SecureHasherFactory; +import org.apache.nifi.util.NiFiProperties; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import javax.crypto.Mac; +import javax.crypto.spec.SecretKeySpec; +import java.nio.charset.StandardCharsets; +import java.security.InvalidKeyException; +import java.security.NoSuchAlgorithmException; +import java.util.Base64; +import java.util.Objects; + +/** + * Encode a sensitive value using the NiFi sensitive properties key to derive the secret key used in the MAC operation. + */ +public class StandardSensitiveValueEncoder implements SensitiveValueEncoder { + + private static final Logger logger = LoggerFactory.getLogger(StandardSensitiveValueEncoder.class); + + private SecretKeySpec secretKeySpec; + private static final String HMAC_SHA256 = "HmacSHA256"; + + public StandardSensitiveValueEncoder(final NiFiProperties properties) { + this(properties.getProperty(NiFiProperties.SENSITIVE_PROPS_KEY), + SecureHasherFactory.getSecureHasher(properties.getProperty(NiFiProperties.SENSITIVE_PROPS_ALGORITHM))); + } + + // We use the sensitive properties key and a SecureHasher impl to derive a secret key for the getEncoded() method + public StandardSensitiveValueEncoder(final String sensitivePropertiesKey, final SecureHasher hasher) { + Objects.requireNonNull(sensitivePropertiesKey, "Sensitive Properties Key is required"); + Objects.requireNonNull(hasher, "SecureHasher is required"); + byte[] hashedSensitivePropertyKey = hasher.hashRaw(sensitivePropertiesKey.getBytes(StandardCharsets.UTF_8)); Review comment: Would it be worthwhile to create a static variable for StandardCharsets.UTF_8 to be used here and in the `getEncoded()` method to indicate the character set expected for properties? Something like PROPERTY_CHARSET? ########## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/encrypt/StandardSensitiveValueEncoder.java ########## @@ -0,0 +1,77 @@ +/* + * 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.encrypt; + +import org.apache.nifi.security.util.crypto.SecureHasher; +import org.apache.nifi.security.util.crypto.SecureHasherFactory; +import org.apache.nifi.util.NiFiProperties; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import javax.crypto.Mac; +import javax.crypto.spec.SecretKeySpec; +import java.nio.charset.StandardCharsets; +import java.security.InvalidKeyException; +import java.security.NoSuchAlgorithmException; +import java.util.Base64; +import java.util.Objects; + +/** + * Encode a sensitive value using the NiFi sensitive properties key to derive the secret key used in the MAC operation. + */ +public class StandardSensitiveValueEncoder implements SensitiveValueEncoder { + + private static final Logger logger = LoggerFactory.getLogger(StandardSensitiveValueEncoder.class); + + private SecretKeySpec secretKeySpec; + private static final String HMAC_SHA256 = "HmacSHA256"; + + public StandardSensitiveValueEncoder(final NiFiProperties properties) { + this(properties.getProperty(NiFiProperties.SENSITIVE_PROPS_KEY), + SecureHasherFactory.getSecureHasher(properties.getProperty(NiFiProperties.SENSITIVE_PROPS_ALGORITHM))); + } + + // We use the sensitive properties key and a SecureHasher impl to derive a secret key for the getEncoded() method + public StandardSensitiveValueEncoder(final String sensitivePropertiesKey, final SecureHasher hasher) { + Objects.requireNonNull(sensitivePropertiesKey, "Sensitive Properties Key is required"); + Objects.requireNonNull(hasher, "SecureHasher is required"); + byte[] hashedSensitivePropertyKey = hasher.hashRaw(sensitivePropertiesKey.getBytes(StandardCharsets.UTF_8)); + secretKeySpec = new SecretKeySpec(hashedSensitivePropertyKey, HMAC_SHA256); + } + + /** + * Creates a securely-derived, deterministic representation of the provided decrypted NiFi property value + * for logging/comparison purposes. A SecureHasher implementation is used to derive a secret key from the sensitive which is + * then used to generate an HMAC using HMAC+SHA256. + * + * @param plaintextSensitivePropertyValue A decrypted, sensitive property value + * + * @return a deterministic, securely hashed representation of the value which will be consistent across nodes. Safe to print in a log. + */ + @Override + public String getEncoded(final String plaintextSensitivePropertyValue) { + try { + Mac mac = Mac.getInstance(HMAC_SHA256); + mac.init(secretKeySpec); + byte[] hashedBytes = mac.doFinal(plaintextSensitivePropertyValue.getBytes(StandardCharsets.UTF_8)); + return "[MASKED] (" + Base64.getEncoder().encodeToString(hashedBytes) + ")"; Review comment: Instances of `Base64.Encoder` are thread-safe, so the value of `Base64.getEncoder()` could be declared as a member variable. ---------------------------------------------------------------- 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]
