exceptionfactory commented on a change in pull request #5131: URL: https://github.com/apache/nifi/pull/5131#discussion_r648267458
########## File path: nifi-commons/nifi-property-utils/src/main/java/org/apache/nifi/properties/ApplicationProperties.java ########## @@ -0,0 +1,106 @@ +/* + * 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.properties; + +import java.util.Properties; +import java.util.Set; + +/** + * A base interface for configuration properties of an application (e.g. NiFi or NiFi Registry). + */ +public interface ApplicationProperties { + + /** + * Retrieves the property value for the given property key. + * + * @param key the key of property value to lookup + * @return value of property at given key or null if not found + */ + String getProperty(String key); + + /** + * Retrieves the property value for the given property key. + * + * @param key the key of property value to lookup + * @param defaultValue The default value to use if the property does not exist + * @return value of property at given key or null if not found + */ + String getProperty(String key, String defaultValue); + + /** + * Retrieves all known property keys. + * + * @return all known property keys + */ + Set<String> getPropertyKeys(); + + /** + * Returns the number of properties. + * @return The number of properties + */ + int size(); + + /** + * Returns the application properties in a basic Properties object. + * @return The basic Properties object + */ + Properties toBasicProperties(); + + // Following are specific properties expected for all applications + + /** + * Keystore path for TLS configuration + * @return The keystore path + */ + String getKeyStorePath(); + + /** + * Keystore type for TLS configuration + * @return The keystore type + */ + String getKeyStoreType(); + + /** + * Keystore password for TLS configuration + * @return The keystore password + */ + String getKeyStorePassword(); + + /** + * Key password for TLS configuration + * @return The key password + */ + String getKeyPassword(); + + /** + * Truststore path for TLS configuration + * @return The truststore path + */ + String getTrustStorePath(); + + /** + * Truststore type for TLS configuration + * @return The truststore type + */ + String getTrustStoreType(); + + /** + * Truststore password for TLS configuration + * @return The truststore password + */ + String getTrustStorePassword(); Review comment: Although these methods are certainly convenient, I'm not sure about including them at the top-level interface, particularly since they can return null depending on the configuration. If this is something we should consider, I think it would be better as a separate issue, and perhaps as an extension of this interface when the properties are not null. ########## File path: nifi-commons/nifi-property-utils/src/main/java/org/apache/nifi/properties/ApplicationProperties.java ########## @@ -0,0 +1,106 @@ +/* + * 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.properties; + +import java.util.Properties; +import java.util.Set; + +/** + * A base interface for configuration properties of an application (e.g. NiFi or NiFi Registry). + */ +public interface ApplicationProperties { + + /** + * Retrieves the property value for the given property key. + * + * @param key the key of property value to lookup + * @return value of property at given key or null if not found + */ + String getProperty(String key); + + /** + * Retrieves the property value for the given property key. + * + * @param key the key of property value to lookup + * @param defaultValue The default value to use if the property does not exist + * @return value of property at given key or null if not found + */ + String getProperty(String key, String defaultValue); + + /** + * Retrieves all known property keys. + * + * @return all known property keys + */ + Set<String> getPropertyKeys(); + + /** + * Returns the number of properties. + * @return The number of properties + */ + int size(); + + /** + * Returns the application properties in a basic Properties object. + * @return The basic Properties object + */ + Properties toBasicProperties(); Review comment: This is also a useful convenience, but in terms of interface definition, particularly when it comes to unit tests, it seems better to separate out somewhere else. ########## File path: nifi-commons/nifi-sensitive-property-provider/src/main/java/org/apache/nifi/properties/ApplicationPropertiesProtector.java ########## @@ -0,0 +1,352 @@ +/* + * 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.properties; + +import org.apache.commons.lang3.StringUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Properties; +import java.util.Set; +import java.util.stream.Collectors; + +import static java.util.Arrays.asList; + +/** + * Class performing unprotection activities before returning a clean + * implementation of {@link ApplicationProperties}. + * This encapsulates the sensitive property access logic from external consumers + * of {@code ApplicationProperties}. + * + * @param <T> The type of protected application properties + * @param <U> The type of standard application properties that backs the protected application properties + */ +public class ApplicationPropertiesProtector<T extends ProtectedApplicationProperties<U>, U extends ApplicationProperties> + implements SensitivePropertyProtector<T, U> { + private static final Logger logger = LoggerFactory.getLogger(ApplicationPropertiesProtector.class); + + private T protectedApplicationProperties; + + private Map<String, SensitivePropertyProvider> localProviderCache = new HashMap<>(); + + /** + * Creates an instance containing the provided {@link ProtectedApplicationProperties}. + * + * @param protectedApplicationProperties the ProtectedApplicationProperties to contain + */ + public ApplicationPropertiesProtector(final T protectedApplicationProperties) { + this.protectedApplicationProperties = protectedApplicationProperties; + logger.debug("Loaded {} properties (including {} protection schemes) into {}", getPropertyKeysIncludingProtectionSchemes().size(), + getProtectedPropertyKeys().size(), this.getClass().getName()); + } + + /** + * Returns the sibling property key which specifies the protection scheme for this key. + * <p> + * Example: + * <p> + * nifi.sensitive.key=ABCXYZ + * nifi.sensitive.key.protected=aes/gcm/256 + * <p> + * nifi.sensitive.key -> nifi.sensitive.key.protected + * + * @param key the key identifying the sensitive property + * @return the key identifying the protection scheme for the sensitive property + */ + public static String getProtectionKey(final String key) { + if (key == null || key.isEmpty()) { + throw new IllegalArgumentException("Cannot find protection key for null key"); + } + + return key + ".protected"; Review comment: Recommend creating a static variable named something like `PROTECTED_SUFFIX` for `.protected` and reusing across methods. ########## File path: nifi-commons/nifi-sensitive-property-provider/src/main/java/org/apache/nifi/properties/ApplicationPropertiesProtector.java ########## @@ -0,0 +1,352 @@ +/* + * 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.properties; + +import org.apache.commons.lang3.StringUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Properties; +import java.util.Set; +import java.util.stream.Collectors; + +import static java.util.Arrays.asList; + +/** + * Class performing unprotection activities before returning a clean + * implementation of {@link ApplicationProperties}. + * This encapsulates the sensitive property access logic from external consumers + * of {@code ApplicationProperties}. + * + * @param <T> The type of protected application properties + * @param <U> The type of standard application properties that backs the protected application properties + */ +public class ApplicationPropertiesProtector<T extends ProtectedApplicationProperties<U>, U extends ApplicationProperties> + implements SensitivePropertyProtector<T, U> { + private static final Logger logger = LoggerFactory.getLogger(ApplicationPropertiesProtector.class); + + private T protectedApplicationProperties; + + private Map<String, SensitivePropertyProvider> localProviderCache = new HashMap<>(); + + /** + * Creates an instance containing the provided {@link ProtectedApplicationProperties}. + * + * @param protectedApplicationProperties the ProtectedApplicationProperties to contain + */ + public ApplicationPropertiesProtector(final T protectedApplicationProperties) { + this.protectedApplicationProperties = protectedApplicationProperties; + logger.debug("Loaded {} properties (including {} protection schemes) into {}", getPropertyKeysIncludingProtectionSchemes().size(), + getProtectedPropertyKeys().size(), this.getClass().getName()); + } + + /** + * Returns the sibling property key which specifies the protection scheme for this key. + * <p> + * Example: + * <p> + * nifi.sensitive.key=ABCXYZ + * nifi.sensitive.key.protected=aes/gcm/256 + * <p> + * nifi.sensitive.key -> nifi.sensitive.key.protected + * + * @param key the key identifying the sensitive property + * @return the key identifying the protection scheme for the sensitive property + */ + public static String getProtectionKey(final String key) { + if (key == null || key.isEmpty()) { + throw new IllegalArgumentException("Cannot find protection key for null key"); + } + + return key + ".protected"; + } + + /** + * Retrieves all known property keys. + * + * @return all known property keys + */ + @Override + public Set<String> getPropertyKeys() { + Set<String> filteredKeys = getPropertyKeysIncludingProtectionSchemes(); + filteredKeys.removeIf(p -> p.endsWith(".protected")); + return filteredKeys; + } + + @Override + public int size() { + return getPropertyKeys().size(); + } + + @Override + public Set<String> getPropertyKeysIncludingProtectionSchemes() { + return protectedApplicationProperties.getUnderlyingProperties().getPropertyKeys(); + } + + /** + * Splits a single string containing multiple property keys into a List. Delimited by ',' or ';' and ignores leading and trailing whitespace around delimiter. + * + * @param multipleProperties a single String containing multiple properties, i.e. "nifi.property.1; nifi.property.2, nifi.property.3" + * @return a List containing the split and trimmed properties + */ + private static List<String> splitMultipleProperties(final String multipleProperties) { + if (multipleProperties == null || multipleProperties.trim().isEmpty()) { + return new ArrayList<>(0); + } else { + List<String> properties = new ArrayList<>(asList(multipleProperties.split("\\s*[,;]\\s*"))); + for (int i = 0; i < properties.size(); i++) { + properties.set(i, properties.get(i).trim()); + } + return properties; + } + } + + private String getProperty(final String key) { + return protectedApplicationProperties.getUnderlyingProperties().getProperty(key); + } + + private String getAdditionalSensitivePropertiesKeys() { + return getProperty(protectedApplicationProperties.getAdditionalSensitivePropertiesKeysName()); + } + + private String getAdditionalSensitivePropertiesKeysName() { + return protectedApplicationProperties.getAdditionalSensitivePropertiesKeysName(); + } + + @Override + public List<String> getSensitivePropertyKeys() { + final String additionalPropertiesString = getAdditionalSensitivePropertiesKeys(); + final String additionalPropertiesKeyName = protectedApplicationProperties.getAdditionalSensitivePropertiesKeysName(); + if (additionalPropertiesString == null || additionalPropertiesString.trim().isEmpty()) { + return protectedApplicationProperties.getDefaultSensitiveProperties(); + } else { + List<String> additionalProperties = splitMultipleProperties(additionalPropertiesString); + /* Remove this key if it was accidentally provided as a sensitive key + * because we cannot protect it and read from it + */ + if (additionalProperties.contains(additionalPropertiesKeyName)) { + logger.warn("The key '{}' contains itself. This is poor practice and should be removed", additionalPropertiesKeyName); + additionalProperties.remove(additionalPropertiesKeyName); + } + additionalProperties.addAll(protectedApplicationProperties.getDefaultSensitiveProperties()); + return additionalProperties; + } + } + + @Override + public List<String> getPopulatedSensitivePropertyKeys() { + List<String> allSensitiveKeys = getSensitivePropertyKeys(); + return allSensitiveKeys.stream().filter(k -> StringUtils.isNotBlank(getProperty(k))).collect(Collectors.toList()); + } + + @Override + public boolean hasProtectedKeys() { + final List<String> sensitiveKeys = getSensitivePropertyKeys(); + for (String k : sensitiveKeys) { + if (isPropertyProtected(k)) { + return true; + } + } + return false; + } + + @Override + public Map<String, String> getProtectedPropertyKeys() { + final List<String> sensitiveKeys = getSensitivePropertyKeys(); + + // This is the Java 8 way, but can likely be optimized (and not sure of correctness) + // Map<String, String> protectedProperties = sensitiveKeys.stream().filter(key -> + // getProperty(getProtectionKey(key)) != null).collect(Collectors.toMap(Function.identity(), key -> + // getProperty(getProtectionKey(key)))); + + // Groovy + // Map<String, String> groovyProtectedProperties = sensitiveKeys.collectEntries { key -> + // [(key): getProperty(getProtectionKey(key))] }.findAll { k, v -> v } + + // Traditional way + final Map<String, String> traditionalProtectedProperties = new HashMap<>(); + for (final String key : sensitiveKeys) { + final String protection = getProperty(getProtectionKey(key)); + if (StringUtils.isNotBlank(protection) && StringUtils.isNotBlank(getProperty(key))) { + traditionalProtectedProperties.put(key, protection); + } + } + + return traditionalProtectedProperties; + } + + @Override + public Set<String> getProtectionSchemes() { + return new HashSet<>(getProtectedPropertyKeys().values()); + } + + @Override + public int getPercentOfSensitivePropertiesProtected() { + return (int) Math.round(getProtectedPropertyKeys().size() / ((double) getPopulatedSensitivePropertyKeys().size()) * 100); + } + + @Override + public boolean isPropertySensitive(final String key) { + // If the explicit check for ADDITIONAL_SENSITIVE_PROPERTIES_KEY is not here, this will loop infinitely + return key != null && !key.equals(getAdditionalSensitivePropertiesKeysName()) && getSensitivePropertyKeys().contains(key.trim()); + } + + /** + * Returns true if the property identified by this key is considered protected in this instance of {@code NiFiProperties}. + * The property value is protected if the key is sensitive and the sibling key of key.protected is present. + * + * @param key the key + * @return true if it is currently marked as protected + * @see ApplicationPropertiesProtector#getSensitivePropertyKeys() + */ + @Override + public boolean isPropertyProtected(final String key) { + return key != null && isPropertySensitive(key) && !StringUtils.isBlank(getProperty(getProtectionKey(key))); + } + + @Override + public U getUnprotectedProperties() throws SensitivePropertyProtectionException { + if (hasProtectedKeys()) { + logger.info("There are {} protected properties of {} sensitive properties ({}%)", + getProtectedPropertyKeys().size(), + getSensitivePropertyKeys().size(), + getPercentOfSensitivePropertiesProtected()); + + final Properties rawProperties = new Properties(); + + final Set<String> failedKeys = new HashSet<>(); + + for (final String key : getPropertyKeys()) { + /* Three kinds of keys + * 1. protection schemes -- skip + * 2. protected keys -- unprotect and copy + * 3. normal keys -- copy over + */ + if (key.endsWith(".protected")) { + // Do nothing + } else if (isPropertyProtected(key)) { + try { + rawProperties.setProperty(key, unprotectValue(key, getProperty(key))); + } catch (final SensitivePropertyProtectionException e) { + logger.warn("Failed to unprotect '{}'", key, e); + failedKeys.add(key); + } + } else { + rawProperties.setProperty(key, getProperty(key)); + } + } + + if (!failedKeys.isEmpty()) { + if (failedKeys.size() > 1) { + logger.warn("Combining {} failed keys [{}] into single exception", failedKeys.size(), StringUtils.join(failedKeys, ", ")); + throw new MultipleSensitivePropertyProtectionException("Failed to unprotect keys", failedKeys); + } else { + throw new SensitivePropertyProtectionException("Failed to unprotect key " + failedKeys.iterator().next()); + } + } + + final U unprotected = protectedApplicationProperties.createApplicationProperties(rawProperties); + + return unprotected; + } else { + logger.debug("No protected properties"); + return protectedApplicationProperties.getUnderlyingProperties(); + } + } + + @Override + public void addSensitivePropertyProvider(final SensitivePropertyProvider sensitivePropertyProvider) { + if (sensitivePropertyProvider == null) { + throw new IllegalArgumentException("Cannot add null SensitivePropertyProvider"); + } + + if (getSensitivePropertyProviders().containsKey(sensitivePropertyProvider.getIdentifierKey())) { + throw new UnsupportedOperationException("Cannot overwrite existing sensitive property provider registered for " + sensitivePropertyProvider.getIdentifierKey()); + } + + getSensitivePropertyProviders().put(sensitivePropertyProvider.getIdentifierKey(), sensitivePropertyProvider); + } + + @Override + public String toString() { + final Set<String> providers = getSensitivePropertyProviders().keySet(); + return new StringBuilder("ApplicationPropertiesProtector instance with ") + .append(size()).append(" properties (") + .append(getProtectedPropertyKeys().size()) + .append(" protected) and ") + .append(providers.size()) + .append(" sensitive property providers: ") + .append(StringUtils.join(providers, ", ")) + .toString(); + } + + @Override + public Map<String, SensitivePropertyProvider> getSensitivePropertyProviders() { + if (localProviderCache == null) { + localProviderCache = new HashMap<>(); + } + + return localProviderCache; + } + + private SensitivePropertyProvider getSensitivePropertyProvider(final String protectionScheme) { + if (isProviderAvailable(protectionScheme)) { + return getSensitivePropertyProviders().get(protectionScheme); + } else { + throw new SensitivePropertyProtectionException("No provider available for " + protectionScheme); + } + } + + private boolean isProviderAvailable(final String protectionScheme) { + return getSensitivePropertyProviders().containsKey(protectionScheme); + } + + /** + * If the value is protected, unprotects it and returns it. If not, returns the original value. + * + * @param key the retrieved property key + * @param retrievedValue the retrieved property value + * @return the unprotected value + */ + private String unprotectValue(final String key, final String retrievedValue) { + // Checks if the key is sensitive and marked as protected + if (isPropertyProtected(key)) { + final String protectionScheme = getProperty(getProtectionKey(key)); + + // No provider registered for this scheme, so just return the value + if (!isProviderAvailable(protectionScheme)) { + logger.warn("No provider available for {} so passing the protected {} value back", protectionScheme, key); + return retrievedValue; + } + + try { + final SensitivePropertyProvider sensitivePropertyProvider = getSensitivePropertyProvider(protectionScheme); + return sensitivePropertyProvider.unprotect(retrievedValue); + } catch (SensitivePropertyProtectionException e) { + throw new SensitivePropertyProtectionException("Error unprotecting value for " + key, e.getCause()); Review comment: Is the reason for catching and rethrowing the Exception in order to include the property key? It seems confusing initially, perhaps a subclassed exception would clarify what is happening? ########## File path: nifi-commons/nifi-property-utils/src/main/java/org/apache/nifi/properties/ApplicationProperties.java ########## @@ -0,0 +1,106 @@ +/* + * 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.properties; + +import java.util.Properties; +import java.util.Set; + +/** + * A base interface for configuration properties of an application (e.g. NiFi or NiFi Registry). + */ +public interface ApplicationProperties { + + /** + * Retrieves the property value for the given property key. + * + * @param key the key of property value to lookup + * @return value of property at given key or null if not found + */ + String getProperty(String key); + + /** + * Retrieves the property value for the given property key. + * + * @param key the key of property value to lookup + * @param defaultValue The default value to use if the property does not exist + * @return value of property at given key or null if not found + */ + String getProperty(String key, String defaultValue); + + /** + * Retrieves all known property keys. + * + * @return all known property keys + */ + Set<String> getPropertyKeys(); + + /** + * Returns the number of properties. + * @return The number of properties + */ + int size(); Review comment: Is this method necessary at this interface level? ########## File path: nifi-commons/nifi-property-utils/src/main/java/org/apache/nifi/properties/ProtectedApplicationProperties.java ########## @@ -0,0 +1,59 @@ +/* + * 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.properties; + +import java.util.List; +import java.util.Properties; + +/** + * Represents a protected set of ApplicationProperties, with methods regarding which sensitive properties + * are protected. + * @param <T> The ApplicationProperties type + */ +public interface ProtectedApplicationProperties<T extends ApplicationProperties> { + + /** + * Additional sensitive properties keys + * @return Additional sensitive properties keys + */ + String getAdditionalSensitivePropertiesKeys(); + + /** + * Returns the name of the property that specifies the additional sensitive properties keys + * @return Name of additional sensitive properties keys + */ + String getAdditionalSensitivePropertiesKeysName(); + + /** + * Additional sensitive properties keys + * @return Additional sensitive properties keys + */ + List<String> getDefaultSensitiveProperties(); + + /** + * Returns the underlying application properties. + * @return The underlying properties + */ + T getUnderlyingProperties(); Review comment: Renaming the interface would provide the opportunity to rename this to `getApplicationProperties()`, ########## File path: nifi-commons/nifi-property-utils/src/main/java/org/apache/nifi/properties/ApplicationPropertiesBase.java ########## @@ -18,64 +18,52 @@ import java.util.Enumeration; import java.util.HashSet; +import java.util.Map; import java.util.Properties; import java.util.Set; -import org.apache.nifi.util.NiFiProperties; -public class StandardNiFiProperties extends NiFiProperties { +public abstract class ApplicationPropertiesBase implements ApplicationProperties { Review comment: Although there are different naming conventions used in various places, what do you think about naming this AbstractApplicationProperties? Alternatively, if the keystore and truststore methods are removed from the interface definition, perhaps this does not need to be `abstract` and could instead be named `StandardApplicationProperties`. ########## File path: nifi-commons/nifi-property-utils/src/main/java/org/apache/nifi/properties/BootstrapProperties.java ########## @@ -0,0 +1,90 @@ +/* + * 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.properties; + +import java.nio.file.Path; +import java.util.Enumeration; +import java.util.Objects; +import java.util.Optional; +import java.util.Properties; + +public class BootstrapProperties { + private static final String PROPERTY_KEY_FORMAT = "%s.%s"; + private static final String BOOTSTRAP_SENSITIVE_KEY = "bootstrap.sensitive.key"; + + private final String propertyPrefix; + private final Properties properties; + private final Path configFilePath; + + public BootstrapProperties(final String propertyPrefix, final Properties properties, final Path configFilePath) { + Objects.requireNonNull(propertyPrefix, "Property prefix is required"); + Objects.requireNonNull(properties, "Properties are required"); + this.propertyPrefix = propertyPrefix; + this.configFilePath = configFilePath; + this.properties = filterProperties(properties); + } + + /** + * Returns the path to the bootstrap config file. + * @return The path to the file + */ + public Path getConfigFilePath() { + return configFilePath; + } + + /** + * Includes only the properties starting with the propertyPrefix. + * @param properties Unfiltered properties + * @return The filtered properties + */ + private Properties filterProperties(final Properties properties) { + final Properties filteredProperties = new Properties(); + for(final Enumeration<Object> e = properties.keys() ; e.hasMoreElements(); ) { + final String key = e.nextElement().toString(); + if (key.startsWith(propertyPrefix)) { + filteredProperties.put(key, properties.getProperty(key)); + } + } + return filteredProperties; + } + + private String getPropertyKey(final String subKey) { + return String.format(PROPERTY_KEY_FORMAT, propertyPrefix, subKey); + } + + /** + * Retrieves the value of the property by the full property key. + * @param key The property key in the bootstrap configuration + * @return The value of the property + */ + public String getProperty(final String key) { + return properties.getProperty(key); + } + + /** + * Returns the bootstrap sensitive key. + * @return The bootstrap sensitive key + */ + public Optional<String> getBootstrapSensitiveKey() { Review comment: Should this method return a `SecretKey` instead of forcing the caller to perform the conversion, or is the goal to keep this more generic? ########## File path: nifi-commons/nifi-sensitive-property-provider/src/main/java/org/apache/nifi/properties/SensitivePropertyHolder.java ########## @@ -0,0 +1,46 @@ +/* + * 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.properties; + +import java.util.function.Supplier; + +/** + * A base class that encapsulates some level of sensitive properties. Review comment: A little more clarity would be helpful, perhaps renaming this class to something like `SensitivePropertyAware` might helpful. Perhaps updating the comment to say something like: "encapsulates access to common Sensitive Property methods". ########## File path: nifi-commons/nifi-property-utils/src/main/java/org/apache/nifi/properties/BootstrapProperties.java ########## @@ -0,0 +1,90 @@ +/* + * 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.properties; + +import java.nio.file.Path; +import java.util.Enumeration; +import java.util.Objects; +import java.util.Optional; +import java.util.Properties; + +public class BootstrapProperties { Review comment: Depending on the interface definition, `BootstrapProperties` could implement `ApplicationProperties`, although that might be confusing when it comes to semantics. From a functional standpoint however, it seems like there could be a shared interface. A class comment might be helpful. ########## File path: nifi-commons/nifi-property-utils/src/main/java/org/apache/nifi/properties/ProtectedApplicationProperties.java ########## @@ -0,0 +1,59 @@ +/* + * 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.properties; + +import java.util.List; +import java.util.Properties; + +/** + * Represents a protected set of ApplicationProperties, with methods regarding which sensitive properties + * are protected. + * @param <T> The ApplicationProperties type + */ +public interface ProtectedApplicationProperties<T extends ApplicationProperties> { Review comment: Understanding the challenge of naming each of these components, `ProtectedApplicationProperties` appears to suggest an extension of `ApplicationProperties`. What about naming this `ProtectedProperties`? ########## File path: nifi-commons/nifi-property-utils/src/main/java/org/apache/nifi/properties/BootstrapProperties.java ########## @@ -0,0 +1,90 @@ +/* + * 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.properties; + +import java.nio.file.Path; +import java.util.Enumeration; +import java.util.Objects; +import java.util.Optional; +import java.util.Properties; + +public class BootstrapProperties { + private static final String PROPERTY_KEY_FORMAT = "%s.%s"; + private static final String BOOTSTRAP_SENSITIVE_KEY = "bootstrap.sensitive.key"; + + private final String propertyPrefix; + private final Properties properties; + private final Path configFilePath; + + public BootstrapProperties(final String propertyPrefix, final Properties properties, final Path configFilePath) { + Objects.requireNonNull(propertyPrefix, "Property prefix is required"); + Objects.requireNonNull(properties, "Properties are required"); Review comment: The `Objects.requireNonNull()` method returns the value, so these lines could be combined with the variable assignment. ########## File path: nifi-commons/nifi-sensitive-property-provider/src/main/java/org/apache/nifi/properties/SensitivePropertyProtectionScheme.java ########## @@ -0,0 +1,79 @@ +/* + * 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.properties; + +import java.util.Arrays; +import java.util.Objects; + +/** + * A scheme for protecting sensitive properties. Each scheme is intended to be backed by an implementation of + * SensitivePropertyProvider. + */ +public enum SensitivePropertyProtectionScheme { Review comment: For brevity, perhaps just `PropertyProtectionScheme` is sufficient? ########## File path: nifi-commons/nifi-sensitive-property-provider/src/main/java/org/apache/nifi/properties/SensitivePropertyProtectionScheme.java ########## @@ -0,0 +1,79 @@ +/* + * 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.properties; + +import java.util.Arrays; +import java.util.Objects; + +/** + * A scheme for protecting sensitive properties. Each scheme is intended to be backed by an implementation of + * SensitivePropertyProvider. + */ +public enum SensitivePropertyProtectionScheme { + AES_GCM("aes/gcm/[0-9]+", "aes/gcm/%s", "AES Sensitive Property Provider", true); Review comment: For the identifier pattern, the number should be more narrowly defined, either `[0-9]{3}` to require three numbers, or perhaps more limited to allow only 128, 192, or 256. ########## File path: nifi-commons/nifi-sensitive-property-provider/src/main/java/org/apache/nifi/properties/SensitivePropertyProviderFactory.java ########## @@ -0,0 +1,43 @@ +/* + * 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.properties; + +import java.util.Collection; + +public interface SensitivePropertyProviderFactory { + + /** + * Gives the appropriate SensitivePropertyProvider, given a protection scheme. + * @param protectionScheme The protection scheme to use + * @return The appropriate SensitivePropertyProvider + */ + SensitivePropertyProvider getProvider(SensitivePropertyProtectionScheme protectionScheme); + + /** + * Gives the appropriate SensitivePropertyProvider, given a protection scheme identifier. + * @param protectionSchemeIdentifier The identifier of the protection scheme to use + * @return The appropriate SensitivePropertyProvider + */ + SensitivePropertyProvider getProvider(String protectionSchemeIdentifier); Review comment: Is it necessary to support this method as well as the one above which requires the more concrete protection scheme parameter? ########## File path: nifi-commons/nifi-sensitive-property-provider/src/main/java/org/apache/nifi/properties/StandardSensitivePropertyProviderFactory.java ########## @@ -0,0 +1,126 @@ +/* + * 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.properties; + +import org.apache.nifi.security.kms.CryptoUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.util.Arrays; +import java.util.Collection; +import java.util.HashMap; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.function.Supplier; +import java.util.stream.Collectors; + +public class StandardSensitivePropertyProviderFactory implements SensitivePropertyProviderFactory { + private static final Logger logger = LoggerFactory.getLogger(StandardSensitivePropertyProviderFactory.class); + + private final Optional<String> keyHex; + private final Supplier<BootstrapProperties> bootstrapPropertiesSupplier; + private final Map<SensitivePropertyProtectionScheme, SensitivePropertyProvider> providerMap; + + /** + * Creates a StandardSensitivePropertyProviderFactory using the default bootstrap.conf location and + * the keyHex extracted from this bootstrap.conf. + */ + public static SensitivePropertyProviderFactory withDefaults() { + return withKeyAndBootstrapSupplier(null, null); + } + + /** + * Creates a StandardSensitivePropertyProviderFactory using only the provided secret key hex. The default + * bootstrap.conf will be used for any providers that may require it, but the provided keyHex will be used instead + * of the one from the default bootstrap.conf. + * @param keyHex The secret key hex for encrypting properties + * @return A StandardSensitivePropertyProviderFactory + */ + public static SensitivePropertyProviderFactory withKey(final String keyHex) { + return new StandardSensitivePropertyProviderFactory(keyHex, null); + } + + /** + * Creates a new StandardSensitivePropertyProviderFactory using a separate keyHex and provided bootstrap.conf. + * The provided keyHex will be used instead of the one from the bootstrap.conf. + * @param keyHex The secret key hex for encrypting properties + * @param bootstrapPropertiesSupplier A supplier for the BootstrapProperties that represent bootstrap.conf. + * If the supplier returns null, the default bootstrap.conf will be used instead. + * @return A StandardSensitivePropertyProviderFactory + */ + public static SensitivePropertyProviderFactory withKeyAndBootstrapSupplier(final String keyHex, + final Supplier<BootstrapProperties> bootstrapPropertiesSupplier) { + return new StandardSensitivePropertyProviderFactory(keyHex, bootstrapPropertiesSupplier); + } + + private StandardSensitivePropertyProviderFactory(final String keyHex, final Supplier<BootstrapProperties> bootstrapPropertiesSupplier) { + this.keyHex = Optional.ofNullable(keyHex); + this.bootstrapPropertiesSupplier = bootstrapPropertiesSupplier == null ? () -> null : bootstrapPropertiesSupplier; + this.providerMap = new HashMap<>(); + } + + private String getKeyHex() { + return keyHex.orElseGet(() -> getBootstrapProperties().getBootstrapSensitiveKey() + .orElseThrow(() -> new SensitivePropertyProtectionException("Could not read root key from bootstrap.conf"))); + } + + /** + * Returns the configured bootstrap properties, or the default bootstrap.conf properties if + * not provided. + * @return The bootstrap.conf properties + */ + private BootstrapProperties getBootstrapProperties() { + return Optional.ofNullable(bootstrapPropertiesSupplier.get()).orElseGet(() -> { + try { + return CryptoUtils.loadBootstrapProperties(""); Review comment: Although it involves more changes, this seems like the time to refactor bootstrap properties loading out of the `CryptoUtils` class. Having an empty string as a parameter is also confusing. ########## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-cluster-protocol/src/main/java/org/apache/nifi/cluster/protocol/spring/ServerSocketConfigurationFactoryBean.java ########## @@ -16,13 +16,14 @@ */ package org.apache.nifi.cluster.protocol.spring; -import java.util.concurrent.TimeUnit; import org.apache.nifi.io.socket.ServerSocketConfiguration; import org.apache.nifi.security.util.StandardTlsConfiguration; import org.apache.nifi.util.FormatUtils; import org.apache.nifi.util.NiFiProperties; import org.springframework.beans.factory.FactoryBean; +import java.util.concurrent.TimeUnit; Review comment: It looks like this change could be reverted since it is just reordering imports. ########## File path: nifi-commons/nifi-sensitive-property-provider/src/main/java/org/apache/nifi/properties/SensitivePropertyProtector.java ########## @@ -0,0 +1,153 @@ +/* + * 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.properties; + +import java.util.List; +import java.util.Map; +import java.util.Set; + +/** + * Encapsulates methods needed to protect application properties. + * @param <T> The ProtectedApplicationProperties type + * @param <U> The ApplicationProperties type + */ +public interface SensitivePropertyProtector<T extends ProtectedApplicationProperties<U>, U extends ApplicationProperties> { + + /** + * Returns the number of properties, excluding protection scheme properties. + * <p> + * Example: + * <p> + * key: E(value, key) + * key.protected: aes/gcm/256 + * key2: value2 + * <p> + * would return size 2 + * + * @return the count of real properties + */ + int size(); + + /** + * Retrieves all known property keys. + * + * @return all known property keys + */ + Set<String> getPropertyKeys(); + + /** + * Returns the complete set of property keys, including any protection keys (i.e. 'x.y.z.protected'). + * + * @return the set of property keys + */ + Set<String> getPropertyKeysIncludingProtectionSchemes(); + + /** + * Returns a list of the keys identifying "sensitive" properties. There is a default list, + * and additional keys can be provided in the {@code nifi.sensitive.props.additional.keys} property in the ApplicationProperties. + * + * @return the list of sensitive property keys + */ + List<String> getSensitivePropertyKeys(); + + /** + * Returns a list of the keys identifying "sensitive" properties. There is a default list, + * and additional keys can be provided in the {@code nifi.sensitive.props.additional.keys} property in the ApplicationProperties. + * + * @return the list of sensitive property keys + */ + List<String> getPopulatedSensitivePropertyKeys(); + + /** + * Returns true if any sensitive keys are protected. + * + * @return true if any key is protected; false otherwise + */ + boolean hasProtectedKeys(); + + /** + * Returns the unique set of all protection schemes currently in use for this instance. + * + * @return the set of protection schemes + */ + Set<String> getProtectionSchemes(); + + /** + * Returns a Map of the keys identifying "sensitive" properties that are currently protected and the "protection" key for each. + * This may or may not include all properties marked as sensitive. + * + * @return the Map of protected property keys and the protection identifier for each + */ + Map<String, String> getProtectedPropertyKeys(); + + /** + * Returns the local provider cache (null-safe) as a Map of protection schemes -> implementations. + * + * @return the map + */ + Map<String, SensitivePropertyProvider> getSensitivePropertyProviders(); + + /** + * Returns a percentage of the total number of populated properties marked as sensitive that are currently protected. + * + * @return the percent of sensitive properties marked as protected + */ + int getPercentOfSensitivePropertiesProtected(); Review comment: Although this is used for informational purposes, is it necessary at the interface level? ########## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/spring/LoginIdentityProviderFactoryBean.java ########## @@ -87,11 +84,18 @@ private static JAXBContext initializeJaxbContext() { } } - private NiFiProperties properties; private ExtensionManager extensionManager; private LoginIdentityProvider loginIdentityProvider; private final Map<String, LoginIdentityProvider> loginIdentityProviders = new HashMap<>(); + public NiFiProperties getProperties() { Review comment: Is this get method necessary? ########## File path: nifi-commons/nifi-sensitive-property-provider/src/test/java/org/apache/nifi/properties/StandardSensitivePropertyProviderFactoryTest.java ########## @@ -0,0 +1,139 @@ +/* + * 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.properties; + +import org.apache.nifi.util.NiFiProperties; +import org.bouncycastle.jce.provider.BouncyCastleProvider; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.Test; +import org.mockito.internal.util.io.IOUtil; + +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.security.Security; +import java.util.Properties; +import java.util.function.Supplier; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +public class StandardSensitivePropertyProviderFactoryTest { + + private SensitivePropertyProviderFactory factory; + + private static final String BOOTSTRAP_KEY_HEX = "0123456789ABCDEFFEDCBA9876543210"; + private static final String AD_HOC_KEY_HEX = "123456789ABCDEFFEDCBA98765432101"; + + private static Path tempConfDir; + private static Path mockBootstrapConf; + private static Path mockNifiProperties; + + private static NiFiProperties niFiProperties; + + @BeforeClass + public static void initOnce() throws IOException { + Security.addProvider(new BouncyCastleProvider()); + tempConfDir = Files.createTempDirectory("conf"); + mockBootstrapConf = Files.createTempFile("bootstrap", ".conf").toAbsolutePath(); + + mockNifiProperties = Files.createTempFile("nifi", ".properties").toAbsolutePath(); + + mockBootstrapConf = Files.move(mockBootstrapConf, tempConfDir.resolve("bootstrap.conf")); + mockNifiProperties = Files.move(mockNifiProperties, tempConfDir.resolve("nifi.properties")); + + IOUtil.writeText("nifi.bootstrap.sensitive.key=" + BOOTSTRAP_KEY_HEX, mockBootstrapConf.toFile()); + System.setProperty(NiFiProperties.PROPERTIES_FILE_PATH, mockNifiProperties.toString()); + + niFiProperties = new NiFiProperties(); + } + + @AfterClass + public static void tearDownOnce() throws IOException { + Files.deleteIfExists(mockBootstrapConf); + Files.deleteIfExists(mockNifiProperties); + Files.deleteIfExists(tempConfDir); + System.clearProperty(NiFiProperties.PROPERTIES_FILE_PATH); + } + + /** + * Configures the factory using the default bootstrap location. + */ + private void configureDefaultFactory() { + factory = StandardSensitivePropertyProviderFactory.withDefaults(); + } + + /** + * Configures the factory using an ad hoc key hex. + */ + private void configureAdHocKeyFactory() { + factory = StandardSensitivePropertyProviderFactory.withKey(AD_HOC_KEY_HEX); + } + + /** + * Configures the factory using an ad hoc key hex and bootstrap.conf properties. The key should override + * the on in the bootstrap.conf. + */ + private void configureAdHocKeyAndPropertiesFactory() throws IOException { + factory = StandardSensitivePropertyProviderFactory.withKeyAndBootstrapSupplier(AD_HOC_KEY_HEX, mockBootstrapProperties()); + } + + private Supplier<BootstrapProperties> mockBootstrapProperties() throws IOException { + final Properties bootstrapProperties = new Properties(); + try (final InputStream inputStream = Files.newInputStream(mockBootstrapConf)) { + bootstrapProperties.load(inputStream); + return () -> new BootstrapProperties("nifi", bootstrapProperties, mockBootstrapConf); + } + } + + @Test + public void testAES_GCM() throws IOException { + configureDefaultFactory(); + + final SensitivePropertyProvider spp = factory.getProvider(SensitivePropertyProtectionScheme.AES_GCM); + assertNotNull(spp); + assertTrue(spp.isSupported()); + + final String cleartext = "test"; + assertEquals(cleartext, spp.unprotect(spp.protect(cleartext))); + assertNotEquals(cleartext, spp.protect(cleartext)); + assertEquals("aes/gcm/128", spp.getIdentifierKey()); Review comment: Recommend declaring a method variable or static variable for `aes/gcm/128` and reusing in this method. ########## File path: nifi-commons/nifi-sensitive-property-provider/src/main/java/org/apache/nifi/properties/ApplicationPropertiesProtector.java ########## @@ -0,0 +1,352 @@ +/* + * 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.properties; + +import org.apache.commons.lang3.StringUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Properties; +import java.util.Set; +import java.util.stream.Collectors; + +import static java.util.Arrays.asList; + +/** + * Class performing unprotection activities before returning a clean + * implementation of {@link ApplicationProperties}. + * This encapsulates the sensitive property access logic from external consumers + * of {@code ApplicationProperties}. + * + * @param <T> The type of protected application properties + * @param <U> The type of standard application properties that backs the protected application properties + */ +public class ApplicationPropertiesProtector<T extends ProtectedApplicationProperties<U>, U extends ApplicationProperties> + implements SensitivePropertyProtector<T, U> { + private static final Logger logger = LoggerFactory.getLogger(ApplicationPropertiesProtector.class); + + private T protectedApplicationProperties; + + private Map<String, SensitivePropertyProvider> localProviderCache = new HashMap<>(); + + /** + * Creates an instance containing the provided {@link ProtectedApplicationProperties}. + * + * @param protectedApplicationProperties the ProtectedApplicationProperties to contain + */ + public ApplicationPropertiesProtector(final T protectedApplicationProperties) { + this.protectedApplicationProperties = protectedApplicationProperties; + logger.debug("Loaded {} properties (including {} protection schemes) into {}", getPropertyKeysIncludingProtectionSchemes().size(), + getProtectedPropertyKeys().size(), this.getClass().getName()); + } + + /** + * Returns the sibling property key which specifies the protection scheme for this key. + * <p> + * Example: + * <p> + * nifi.sensitive.key=ABCXYZ + * nifi.sensitive.key.protected=aes/gcm/256 + * <p> + * nifi.sensitive.key -> nifi.sensitive.key.protected + * + * @param key the key identifying the sensitive property + * @return the key identifying the protection scheme for the sensitive property + */ + public static String getProtectionKey(final String key) { + if (key == null || key.isEmpty()) { + throw new IllegalArgumentException("Cannot find protection key for null key"); + } + + return key + ".protected"; + } + + /** + * Retrieves all known property keys. + * + * @return all known property keys + */ + @Override + public Set<String> getPropertyKeys() { + Set<String> filteredKeys = getPropertyKeysIncludingProtectionSchemes(); + filteredKeys.removeIf(p -> p.endsWith(".protected")); + return filteredKeys; + } + + @Override + public int size() { + return getPropertyKeys().size(); + } + + @Override + public Set<String> getPropertyKeysIncludingProtectionSchemes() { + return protectedApplicationProperties.getUnderlyingProperties().getPropertyKeys(); + } + + /** + * Splits a single string containing multiple property keys into a List. Delimited by ',' or ';' and ignores leading and trailing whitespace around delimiter. + * + * @param multipleProperties a single String containing multiple properties, i.e. "nifi.property.1; nifi.property.2, nifi.property.3" + * @return a List containing the split and trimmed properties + */ + private static List<String> splitMultipleProperties(final String multipleProperties) { + if (multipleProperties == null || multipleProperties.trim().isEmpty()) { + return new ArrayList<>(0); + } else { + List<String> properties = new ArrayList<>(asList(multipleProperties.split("\\s*[,;]\\s*"))); + for (int i = 0; i < properties.size(); i++) { + properties.set(i, properties.get(i).trim()); + } + return properties; + } + } + + private String getProperty(final String key) { + return protectedApplicationProperties.getUnderlyingProperties().getProperty(key); + } + + private String getAdditionalSensitivePropertiesKeys() { + return getProperty(protectedApplicationProperties.getAdditionalSensitivePropertiesKeysName()); + } + + private String getAdditionalSensitivePropertiesKeysName() { + return protectedApplicationProperties.getAdditionalSensitivePropertiesKeysName(); + } + + @Override + public List<String> getSensitivePropertyKeys() { + final String additionalPropertiesString = getAdditionalSensitivePropertiesKeys(); + final String additionalPropertiesKeyName = protectedApplicationProperties.getAdditionalSensitivePropertiesKeysName(); + if (additionalPropertiesString == null || additionalPropertiesString.trim().isEmpty()) { + return protectedApplicationProperties.getDefaultSensitiveProperties(); + } else { + List<String> additionalProperties = splitMultipleProperties(additionalPropertiesString); + /* Remove this key if it was accidentally provided as a sensitive key + * because we cannot protect it and read from it + */ + if (additionalProperties.contains(additionalPropertiesKeyName)) { + logger.warn("The key '{}' contains itself. This is poor practice and should be removed", additionalPropertiesKeyName); + additionalProperties.remove(additionalPropertiesKeyName); + } + additionalProperties.addAll(protectedApplicationProperties.getDefaultSensitiveProperties()); + return additionalProperties; + } + } + + @Override + public List<String> getPopulatedSensitivePropertyKeys() { + List<String> allSensitiveKeys = getSensitivePropertyKeys(); + return allSensitiveKeys.stream().filter(k -> StringUtils.isNotBlank(getProperty(k))).collect(Collectors.toList()); + } + + @Override + public boolean hasProtectedKeys() { + final List<String> sensitiveKeys = getSensitivePropertyKeys(); + for (String k : sensitiveKeys) { + if (isPropertyProtected(k)) { + return true; + } + } + return false; + } + + @Override + public Map<String, String> getProtectedPropertyKeys() { + final List<String> sensitiveKeys = getSensitivePropertyKeys(); + + // This is the Java 8 way, but can likely be optimized (and not sure of correctness) + // Map<String, String> protectedProperties = sensitiveKeys.stream().filter(key -> + // getProperty(getProtectionKey(key)) != null).collect(Collectors.toMap(Function.identity(), key -> + // getProperty(getProtectionKey(key)))); + + // Groovy + // Map<String, String> groovyProtectedProperties = sensitiveKeys.collectEntries { key -> + // [(key): getProperty(getProtectionKey(key))] }.findAll { k, v -> v } + + // Traditional way + final Map<String, String> traditionalProtectedProperties = new HashMap<>(); + for (final String key : sensitiveKeys) { + final String protection = getProperty(getProtectionKey(key)); + if (StringUtils.isNotBlank(protection) && StringUtils.isNotBlank(getProperty(key))) { + traditionalProtectedProperties.put(key, protection); + } + } + + return traditionalProtectedProperties; + } + + @Override + public Set<String> getProtectionSchemes() { + return new HashSet<>(getProtectedPropertyKeys().values()); + } + + @Override + public int getPercentOfSensitivePropertiesProtected() { + return (int) Math.round(getProtectedPropertyKeys().size() / ((double) getPopulatedSensitivePropertyKeys().size()) * 100); + } + + @Override + public boolean isPropertySensitive(final String key) { + // If the explicit check for ADDITIONAL_SENSITIVE_PROPERTIES_KEY is not here, this will loop infinitely + return key != null && !key.equals(getAdditionalSensitivePropertiesKeysName()) && getSensitivePropertyKeys().contains(key.trim()); + } + + /** + * Returns true if the property identified by this key is considered protected in this instance of {@code NiFiProperties}. + * The property value is protected if the key is sensitive and the sibling key of key.protected is present. + * + * @param key the key + * @return true if it is currently marked as protected + * @see ApplicationPropertiesProtector#getSensitivePropertyKeys() + */ + @Override + public boolean isPropertyProtected(final String key) { + return key != null && isPropertySensitive(key) && !StringUtils.isBlank(getProperty(getProtectionKey(key))); + } + + @Override + public U getUnprotectedProperties() throws SensitivePropertyProtectionException { + if (hasProtectedKeys()) { + logger.info("There are {} protected properties of {} sensitive properties ({}%)", + getProtectedPropertyKeys().size(), + getSensitivePropertyKeys().size(), + getPercentOfSensitivePropertiesProtected()); + + final Properties rawProperties = new Properties(); + + final Set<String> failedKeys = new HashSet<>(); + + for (final String key : getPropertyKeys()) { + /* Three kinds of keys + * 1. protection schemes -- skip + * 2. protected keys -- unprotect and copy + * 3. normal keys -- copy over + */ + if (key.endsWith(".protected")) { + // Do nothing + } else if (isPropertyProtected(key)) { + try { + rawProperties.setProperty(key, unprotectValue(key, getProperty(key))); + } catch (final SensitivePropertyProtectionException e) { + logger.warn("Failed to unprotect '{}'", key, e); + failedKeys.add(key); + } + } else { + rawProperties.setProperty(key, getProperty(key)); + } + } + + if (!failedKeys.isEmpty()) { + if (failedKeys.size() > 1) { + logger.warn("Combining {} failed keys [{}] into single exception", failedKeys.size(), StringUtils.join(failedKeys, ", ")); + throw new MultipleSensitivePropertyProtectionException("Failed to unprotect keys", failedKeys); + } else { + throw new SensitivePropertyProtectionException("Failed to unprotect key " + failedKeys.iterator().next()); + } + } + + final U unprotected = protectedApplicationProperties.createApplicationProperties(rawProperties); + + return unprotected; + } else { + logger.debug("No protected properties"); + return protectedApplicationProperties.getUnderlyingProperties(); + } + } + + @Override + public void addSensitivePropertyProvider(final SensitivePropertyProvider sensitivePropertyProvider) { + if (sensitivePropertyProvider == null) { + throw new IllegalArgumentException("Cannot add null SensitivePropertyProvider"); + } + + if (getSensitivePropertyProviders().containsKey(sensitivePropertyProvider.getIdentifierKey())) { + throw new UnsupportedOperationException("Cannot overwrite existing sensitive property provider registered for " + sensitivePropertyProvider.getIdentifierKey()); + } + + getSensitivePropertyProviders().put(sensitivePropertyProvider.getIdentifierKey(), sensitivePropertyProvider); + } + + @Override + public String toString() { + final Set<String> providers = getSensitivePropertyProviders().keySet(); + return new StringBuilder("ApplicationPropertiesProtector instance with ") + .append(size()).append(" properties (") + .append(getProtectedPropertyKeys().size()) + .append(" protected) and ") + .append(providers.size()) + .append(" sensitive property providers: ") + .append(StringUtils.join(providers, ", ")) + .toString(); + } + + @Override + public Map<String, SensitivePropertyProvider> getSensitivePropertyProviders() { + if (localProviderCache == null) { + localProviderCache = new HashMap<>(); + } + + return localProviderCache; + } + + private SensitivePropertyProvider getSensitivePropertyProvider(final String protectionScheme) { + if (isProviderAvailable(protectionScheme)) { + return getSensitivePropertyProviders().get(protectionScheme); + } else { + throw new SensitivePropertyProtectionException("No provider available for " + protectionScheme); + } + } + + private boolean isProviderAvailable(final String protectionScheme) { + return getSensitivePropertyProviders().containsKey(protectionScheme); + } + + /** + * If the value is protected, unprotects it and returns it. If not, returns the original value. + * + * @param key the retrieved property key + * @param retrievedValue the retrieved property value + * @return the unprotected value + */ + private String unprotectValue(final String key, final String retrievedValue) { + // Checks if the key is sensitive and marked as protected + if (isPropertyProtected(key)) { + final String protectionScheme = getProperty(getProtectionKey(key)); + + // No provider registered for this scheme, so just return the value + if (!isProviderAvailable(protectionScheme)) { + logger.warn("No provider available for {} so passing the protected {} value back", protectionScheme, key); Review comment: Should this be a warning as opposed to throwing some subclass of RuntimeException? Perhaps this validation could be performed earlier, but it seems like a configuration error that should cause an exception. ########## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-authorizer/src/main/java/org/apache/nifi/authorization/AuthorizerFactoryBean.java ########## @@ -87,12 +84,19 @@ private static JAXBContext initializeJaxbContext() { } private Authorizer authorizer; - private NiFiProperties properties; private ExtensionManager extensionManager; private final Map<String, UserGroupProvider> userGroupProviders = new HashMap<>(); private final Map<String, AccessPolicyProvider> accessPolicyProviders = new HashMap<>(); private final Map<String, Authorizer> authorizers = new HashMap<>(); + public NiFiProperties getProperties() { + return properties; + } Review comment: Is this get method necessary? ########## File path: nifi-toolkit/nifi-toolkit-encrypt-config/pom.xml ########## @@ -30,8 +30,8 @@ <version>1.14.0-SNAPSHOT</version> </dependency> <dependency> - <groupId>org.apache.nifi</groupId> - <artifactId>nifi-properties</artifactId> + <groupId>org.apache.nifi.registry</groupId> + <artifactId>nifi-registry-properties</artifactId> Review comment: Is it intentional to replace `nifi-properties` here with `nifi-registry-properties`? Should both dependencies be declared? -- 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]
