You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/02/23 14:15:58 UTC
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4836: NIFI-7127 - Allow injection of SecureHasher into FingerprintFactory
exceptionfactory commented on a change in pull request #4836:
URL: https://github.com/apache/nifi/pull/4836#discussion_r581038675
##########
File path: nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/NiFiProperties.java
##########
@@ -332,6 +332,7 @@
public static final String DEFAULT_SECURITY_USER_SAML_HTTP_CLIENT_CONNECT_TIMEOUT = "30 secs";
public static final String DEFAULT_SECURITY_USER_SAML_HTTP_CLIENT_READ_TIMEOUT = "30 secs";
public static final String DEFAULT_WEB_SHOULD_SEND_SERVER_VERSION = "true";
+ public static final String DEFAULT_SENSITIVE_PROPS_ALGORITHM = "NIFI_ARGON2_AES_GCM_128";
Review comment:
Instead of making this a public property, leveraging the `SecureHasherFactory` would effectively encapsulate the default implementation. This appears to be used only in one of the `FingerprintFactory` constructors, so it looks like this public value would be eliminated.
##########
File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/crypto/SecureHasherFactory.java
##########
@@ -0,0 +1,80 @@
+/*
+ * 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 javax.naming.ConfigurationException;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
Review comment:
Adding a basic comment that describes the general purpose of this class would be helpful.
##########
File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/crypto/SecureHasherException.java
##########
@@ -0,0 +1,32 @@
+/*
+ * 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;
+
+/**
+ * Exception indicating an error occurred instantiating a SecureHasher.
+ */
+@SuppressWarnings("serial")
+public class SecureHasherException extends RuntimeException {
+
Review comment:
Instead of suppressing warnings for the serial version, recommend adding a serialVersionUID:
```suggestion
private static final long serialVersionUID = 1L;
```
##########
File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/crypto/SecureHasherFactory.java
##########
@@ -0,0 +1,80 @@
+/*
+ * 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 javax.naming.ConfigurationException;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+public class SecureHasherFactory {
+ private static final Logger LOGGER = LoggerFactory.getLogger(SecureHasherFactory.class);
+
+ private static Map<String, Class<? extends SecureHasher>> registeredSecureHashers;
+ private static final String DEFAULT_HASHER = KeyDerivationFunction.ARGON2.getKdfName().toUpperCase();
+
+ static {
+ registeredSecureHashers = new HashMap<>();
+ registeredSecureHashers.put(KeyDerivationFunction.PBKDF2.getKdfName().toUpperCase(), PBKDF2SecureHasher.class);
+ registeredSecureHashers.put(KeyDerivationFunction.BCRYPT.getKdfName().toUpperCase(), BcryptSecureHasher.class);
+ registeredSecureHashers.put(KeyDerivationFunction.SCRYPT.getKdfName().toUpperCase(), ScryptSecureHasher.class);
+ registeredSecureHashers.put(KeyDerivationFunction.ARGON2.getKdfName().toUpperCase(), Argon2SecureHasher.class);
+ }
+
+ public static SecureHasher getSecureHasher() {
+ return getSecureHasher(DEFAULT_HASHER);
+ }
+
+ public static SecureHasher getSecureHasher(final String algorithm) {
+ try {
+ String hasherName = getHasherAlgorithm(algorithm);
+ Class<? extends SecureHasher> clazz = registeredSecureHashers.get(hasherName);
+ LOGGER.debug("Initializing secure hasher {}", clazz.getCanonicalName());
+ return clazz.getDeclaredConstructor().newInstance();
+ } catch (Exception e) {
+ throw new SecureHasherException("Could not instantiate a valid SecureHasher implementation.");
+ }
+ }
+
+ private static String getHasherAlgorithm(final String algorithm) throws ConfigurationException {
+ final String hashingName = getHashingName(algorithm);
+
+ Optional<String> chosenKdf = registeredSecureHashers.keySet().stream().filter(keyDerivationFunction -> keyDerivationFunction.equals(hashingName)).findFirst();
+
+ if(chosenKdf.isPresent()) {
+ return chosenKdf.get();
+ } else {
+ // Default to Argon2
+ LOGGER.debug("Secure hasher implementation for {} not found, defaulting secure hasher to {}", algorithm, DEFAULT_HASHER);
+ return DEFAULT_HASHER;
+ }
+ }
+
+ private static String getHashingName(final String algorithm) {
+ List<String> algorithmParts = Arrays.asList(algorithm.split("_"));
+ Optional<String> algoName = algorithmParts.stream().filter(item -> registeredSecureHashers.containsKey(item.toUpperCase())).findFirst();
Review comment:
Instead of splitting the algorithm property value, what about iterating over the registered secure hasher KDF Names and calling `algorithm.contains(kdfName)`? That would simplify the implementation and avoid the need for splitting the algorithm string.
##########
File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/crypto/SecureHasherFactory.java
##########
@@ -0,0 +1,80 @@
+/*
+ * 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 javax.naming.ConfigurationException;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+public class SecureHasherFactory {
+ private static final Logger LOGGER = LoggerFactory.getLogger(SecureHasherFactory.class);
+
+ private static Map<String, Class<? extends SecureHasher>> registeredSecureHashers;
+ private static final String DEFAULT_HASHER = KeyDerivationFunction.ARGON2.getKdfName().toUpperCase();
+
+ static {
+ registeredSecureHashers = new HashMap<>();
+ registeredSecureHashers.put(KeyDerivationFunction.PBKDF2.getKdfName().toUpperCase(), PBKDF2SecureHasher.class);
+ registeredSecureHashers.put(KeyDerivationFunction.BCRYPT.getKdfName().toUpperCase(), BcryptSecureHasher.class);
+ registeredSecureHashers.put(KeyDerivationFunction.SCRYPT.getKdfName().toUpperCase(), ScryptSecureHasher.class);
+ registeredSecureHashers.put(KeyDerivationFunction.ARGON2.getKdfName().toUpperCase(), Argon2SecureHasher.class);
+ }
+
+ public static SecureHasher getSecureHasher() {
Review comment:
Is it necessary to have this method to return the default value as opposed to calling the `getSecureHasher` method with some algorithm value or perhaps `null`? That would require checking the provided `algorithm` string for null, but it would funnel all calls through the same method.
##########
File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/crypto/SecureHasherFactory.java
##########
@@ -0,0 +1,80 @@
+/*
+ * 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 javax.naming.ConfigurationException;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+public class SecureHasherFactory {
+ private static final Logger LOGGER = LoggerFactory.getLogger(SecureHasherFactory.class);
+
+ private static Map<String, Class<? extends SecureHasher>> registeredSecureHashers;
+ private static final String DEFAULT_HASHER = KeyDerivationFunction.ARGON2.getKdfName().toUpperCase();
+
+ static {
+ registeredSecureHashers = new HashMap<>();
+ registeredSecureHashers.put(KeyDerivationFunction.PBKDF2.getKdfName().toUpperCase(), PBKDF2SecureHasher.class);
+ registeredSecureHashers.put(KeyDerivationFunction.BCRYPT.getKdfName().toUpperCase(), BcryptSecureHasher.class);
+ registeredSecureHashers.put(KeyDerivationFunction.SCRYPT.getKdfName().toUpperCase(), ScryptSecureHasher.class);
+ registeredSecureHashers.put(KeyDerivationFunction.ARGON2.getKdfName().toUpperCase(), Argon2SecureHasher.class);
+ }
+
+ public static SecureHasher getSecureHasher() {
+ return getSecureHasher(DEFAULT_HASHER);
+ }
+
+ public static SecureHasher getSecureHasher(final String algorithm) {
+ try {
+ String hasherName = getHasherAlgorithm(algorithm);
+ Class<? extends SecureHasher> clazz = registeredSecureHashers.get(hasherName);
+ LOGGER.debug("Initializing secure hasher {}", clazz.getCanonicalName());
+ return clazz.getDeclaredConstructor().newInstance();
+ } catch (Exception e) {
+ throw new SecureHasherException("Could not instantiate a valid SecureHasher implementation.");
Review comment:
Recommend including the algorithm argument and the Exception to enable troubleshooting:
```suggestion
throw new SecureHasherException(String.format("SecureHasher Instantiation Failed for Algorithm [%s]", algorithm), e);
```
##########
File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/crypto/SecureHasherFactory.java
##########
@@ -0,0 +1,80 @@
+/*
+ * 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 javax.naming.ConfigurationException;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+public class SecureHasherFactory {
+ private static final Logger LOGGER = LoggerFactory.getLogger(SecureHasherFactory.class);
+
+ private static Map<String, Class<? extends SecureHasher>> registeredSecureHashers;
Review comment:
To provide additional type safety, the Map key could be `KeyDerivationFunction` instead of `String` and then references could be adjusted.
##########
File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/crypto/SecureHasherFactory.java
##########
@@ -0,0 +1,80 @@
+/*
+ * 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 javax.naming.ConfigurationException;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+public class SecureHasherFactory {
+ private static final Logger LOGGER = LoggerFactory.getLogger(SecureHasherFactory.class);
+
+ private static Map<String, Class<? extends SecureHasher>> registeredSecureHashers;
+ private static final String DEFAULT_HASHER = KeyDerivationFunction.ARGON2.getKdfName().toUpperCase();
Review comment:
By changing the Map key, this property could also be changed to a type of `KeyDeriviationFunction`.
##########
File path: nifi-commons/nifi-security-utils/src/test/java/org/apache/nifi/security/util/crypto/SecureHasherFactoryTest.java
##########
@@ -0,0 +1,84 @@
+/*
+ * 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.junit.Test;
+
+public class SecureHasherFactoryTest {
+
+ private static final Argon2SecureHasher DEFAULT_HASHER = new Argon2SecureHasher();
+
+ @Test
+ public void testSecureHasherFactoryArgon2() {
+ SecureHasher hasher = SecureHasherFactory.getSecureHasher("NIFI_ARGON2_AES_GCM_128");
+ assert(hasher instanceof Argon2SecureHasher);
Review comment:
This and other assertions could be changed to use `assertEquals` with classes in order to provide improved output in the event of failures:
```suggestion
assertEquals(Argon2SecureHasher.class, hasher.getClass());
```
##########
File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/crypto/SecureHasherFactory.java
##########
@@ -0,0 +1,80 @@
+/*
+ * 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 javax.naming.ConfigurationException;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+public class SecureHasherFactory {
+ private static final Logger LOGGER = LoggerFactory.getLogger(SecureHasherFactory.class);
+
+ private static Map<String, Class<? extends SecureHasher>> registeredSecureHashers;
+ private static final String DEFAULT_HASHER = KeyDerivationFunction.ARGON2.getKdfName().toUpperCase();
+
+ static {
+ registeredSecureHashers = new HashMap<>();
+ registeredSecureHashers.put(KeyDerivationFunction.PBKDF2.getKdfName().toUpperCase(), PBKDF2SecureHasher.class);
+ registeredSecureHashers.put(KeyDerivationFunction.BCRYPT.getKdfName().toUpperCase(), BcryptSecureHasher.class);
+ registeredSecureHashers.put(KeyDerivationFunction.SCRYPT.getKdfName().toUpperCase(), ScryptSecureHasher.class);
+ registeredSecureHashers.put(KeyDerivationFunction.ARGON2.getKdfName().toUpperCase(), Argon2SecureHasher.class);
+ }
+
+ public static SecureHasher getSecureHasher() {
+ return getSecureHasher(DEFAULT_HASHER);
+ }
+
+ public static SecureHasher getSecureHasher(final String algorithm) {
+ try {
+ String hasherName = getHasherAlgorithm(algorithm);
+ Class<? extends SecureHasher> clazz = registeredSecureHashers.get(hasherName);
+ LOGGER.debug("Initializing secure hasher {}", clazz.getCanonicalName());
Review comment:
Recommend including the `algorithm` for logging:
```suggestion
LOGGER.debug("Instantiating Secure Hasher [{}] for Algorithm [{}]", clazz.getCanonicalName(), algorithm);
```
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster/src/test/java/org/apache/nifi/cluster/coordination/flow/TestPopularVoteFlowElection.java
##########
@@ -157,7 +158,7 @@ public void testEmptyFlowIgnoredIfNonEmptyFlowExists() throws IOException {
@Test
public void testAutoGeneratedVsPopulatedFlowElection() throws IOException {
final ExtensionManager extensionManager = new StandardExtensionDiscoveringManager();
- final FingerprintFactory fingerprintFactory = new FingerprintFactory(createEncryptorFromProperties(getNiFiProperties()), extensionManager);
+ final FingerprintFactory fingerprintFactory = new FingerprintFactory(createEncryptorFromProperties(getNiFiProperties()), extensionManager, SecureHasherFactory.getSecureHasher());
Review comment:
For the purposes of this unit test, an implementation of `SecureHasher` could be instantiated directly, perhaps one that performs more quickly than Argon2 for testing performance purposes.
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/fingerprint/FingerprintFactory.java
##########
@@ -565,14 +570,13 @@ private String getLoggableRepresentationOfSensitiveValue(String encryptedPropert
private void initializeSensitivePropertyKeyBytes() {
if (sensitivePropertyKeyBytes == null || sensitivePropertyKeyBytes.length == 0) {
- Argon2SecureHasher a2sh = new Argon2SecureHasher();
-
// Derive the reusable HMAC key from the nifi.sensitive.props.key to ensure deterministic output across nodes
try {
+ NiFiPropertiesLoader.loadDefaultWithKeyFromBootstrap().getProperty(NiFiProperties.SENSITIVE_PROPS_ALGORITHM);
String npsk = NiFiPropertiesLoader.loadDefaultWithKeyFromBootstrap().getProperty(NiFiProperties.SENSITIVE_PROPS_KEY);
Review comment:
Although it will require more adjustments, what do you think about refactoring to remove this approach to loading the NiFiProperties directly? The SecureHasher reference could actually be eliminated entirely if the `sensitivePropertyKeyBytes` were computed based and passed as a constructor argument to `FingerprintFactory`. At minimum, recommend refactoring the instantiation in `FlowFingerprintCheck` and `PopularVoteFlowElectionFactoryBean` to provide the sensitive properties key, but on review, it seems like the best approach is to refactor out the `SecureHasher` reference entirely and instead seed `FingerprintFactory` with the results of `SecureHasher.hashRaw(niFiSensitivePropertiesKey)`.
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/groovy/org/apache/nifi/fingerprint/FingerprintFactoryGroovyTest.groovy
##########
@@ -84,7 +85,7 @@ class FingerprintFactoryGroovyTest extends GroovyTestCase {
logger.info("Read initial flow: ${initialFlowXML[0..<100]}...")
// Create the FingerprintFactory with collaborators
- FingerprintFactory fingerprintFactory = new FingerprintFactory(mockEncryptor, extensionManager)
+ FingerprintFactory fingerprintFactory = new FingerprintFactory(mockEncryptor, extensionManager, SecureHasherFactory.getSecureHasher())
Review comment:
This test also has access to `nifi.properties`, so recommend using the configured `algorithm` property.
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/fingerprint/FingerprintFactory.java
##########
@@ -565,14 +570,13 @@ private String getLoggableRepresentationOfSensitiveValue(String encryptedPropert
private void initializeSensitivePropertyKeyBytes() {
if (sensitivePropertyKeyBytes == null || sensitivePropertyKeyBytes.length == 0) {
- Argon2SecureHasher a2sh = new Argon2SecureHasher();
-
// Derive the reusable HMAC key from the nifi.sensitive.props.key to ensure deterministic output across nodes
try {
+ NiFiPropertiesLoader.loadDefaultWithKeyFromBootstrap().getProperty(NiFiProperties.SENSITIVE_PROPS_ALGORITHM);
Review comment:
This call does not assign the return to a variable, is it necessary?
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/fingerprint/FingerprintFactoryTest.java
##########
@@ -81,7 +82,7 @@
public void setup() {
encryptor = new StringEncryptor("PBEWITHMD5AND256BITAES-CBC-OPENSSL", "BC", "nififtw!");
extensionManager = new StandardExtensionDiscoveringManager();
- fingerprinter = new FingerprintFactory(encryptor, extensionManager);
+ fingerprinter = new FingerprintFactory(encryptor, extensionManager, SecureHasherFactory.getSecureHasher());
Review comment:
As in other places, recommend refactoring to use `nifi.properties`.
##########
File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/crypto/SecureHasherFactory.java
##########
@@ -0,0 +1,80 @@
+/*
+ * 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 javax.naming.ConfigurationException;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+public class SecureHasherFactory {
+ private static final Logger LOGGER = LoggerFactory.getLogger(SecureHasherFactory.class);
+
+ private static Map<String, Class<? extends SecureHasher>> registeredSecureHashers;
+ private static final String DEFAULT_HASHER = KeyDerivationFunction.ARGON2.getKdfName().toUpperCase();
+
+ static {
+ registeredSecureHashers = new HashMap<>();
+ registeredSecureHashers.put(KeyDerivationFunction.PBKDF2.getKdfName().toUpperCase(), PBKDF2SecureHasher.class);
+ registeredSecureHashers.put(KeyDerivationFunction.BCRYPT.getKdfName().toUpperCase(), BcryptSecureHasher.class);
+ registeredSecureHashers.put(KeyDerivationFunction.SCRYPT.getKdfName().toUpperCase(), ScryptSecureHasher.class);
+ registeredSecureHashers.put(KeyDerivationFunction.ARGON2.getKdfName().toUpperCase(), Argon2SecureHasher.class);
+ }
+
+ public static SecureHasher getSecureHasher() {
+ return getSecureHasher(DEFAULT_HASHER);
+ }
+
+ public static SecureHasher getSecureHasher(final String algorithm) {
+ try {
+ String hasherName = getHasherAlgorithm(algorithm);
+ Class<? extends SecureHasher> clazz = registeredSecureHashers.get(hasherName);
+ LOGGER.debug("Initializing secure hasher {}", clazz.getCanonicalName());
+ return clazz.getDeclaredConstructor().newInstance();
+ } catch (Exception e) {
+ throw new SecureHasherException("Could not instantiate a valid SecureHasher implementation.");
+ }
+ }
+
+ private static String getHasherAlgorithm(final String algorithm) throws ConfigurationException {
+ final String hashingName = getHashingName(algorithm);
+
+ Optional<String> chosenKdf = registeredSecureHashers.keySet().stream().filter(keyDerivationFunction -> keyDerivationFunction.equals(hashingName)).findFirst();
+
+ if(chosenKdf.isPresent()) {
Review comment:
Recommend running code formatting to adjust spacing:
```suggestion
if (chosenKdf.isPresent()) {
```
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/groovy/org/apache/nifi/fingerprint/FingerprintFactoryGroovyIT.groovy
##########
@@ -90,7 +91,7 @@ class FingerprintFactoryGroovyIT extends GroovyTestCase {
logger.info("Read initial flow: ${initialFlowXML[0..<100]}...")
// Create the FingerprintFactory with collaborators
- FingerprintFactory fingerprintFactory = new FingerprintFactory(mockEncryptor, extensionManager)
+ FingerprintFactory fingerprintFactory = new FingerprintFactory(mockEncryptor, extensionManager, SecureHasherFactory.getSecureHasher())
Review comment:
It looks like this unit test has access to `nifi.properties`, so recommend adjusting to pass the `algorithm` property to `SecureHasherFactory`.
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/inheritance/FlowFingerprintCheck.java
##########
@@ -39,7 +40,7 @@ public FlowInheritability checkInheritability(final DataFlow existingFlow, final
final StringEncryptor encryptor = flowController.getEncryptor();
final ExtensionManager extensionManager = flowController.getExtensionManager();
- final FingerprintFactory fingerprintFactory = new FingerprintFactory(encryptor, extensionManager);
+ final FingerprintFactory fingerprintFactory = new FingerprintFactory(encryptor, extensionManager, SecureHasherFactory.getSecureHasher());
Review comment:
The `FlowFingerprintCheck` is one of the places where the `FingerprintFactory` is used at runtime, so it needs to honor the configured sensitive properties algorithm value for SecureHasher. Since `FlowFingerprintCheck` does not have access to `NiFiProperties`, but `FlowController` does, recommend evaluating adding a `getSecureHasher()` method to `FlowController` and then calling it here.
```suggestion
final FingerprintFactory fingerprintFactory = new FingerprintFactory(encryptor, extensionManager, flowController.getSecureHasher());
```
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/fingerprint/FingerprintFactory.java
##########
@@ -120,6 +124,7 @@ public FingerprintFactory(final StringEncryptor encryptor, final DocumentBuilder
this.encryptor = encryptor;
this.flowConfigDocBuilder = docBuilder;
this.extensionManager = extensionManager;
+ this.secureHasher = SecureHasherFactory.getSecureHasher(NiFiProperties.DEFAULT_SENSITIVE_PROPS_ALGORITHM);
Review comment:
Instead of calling `SecureHasherFactory`, this constructor should also take `SecureHasher` as an argument for configuring the class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org