[GitHub] [nifi] exceptionfactory commented on a change in pull request #5206: NIFI-8695: Adding context to sensitive property providers

[GitBox <gi...@apache.org>]

exceptionfactory commented on a change in pull request #5206:
URL: https://github.com/apache/nifi/pull/5206#discussion_r671537819



##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-resources/src/main/resources/conf/bootstrap.conf
##########
@@ -63,6 +63,20 @@ nifi.bootstrap.sensitive.key=
 # HashiCorp Vault Sensitive Property Providers
 nifi.bootstrap.protection.hashicorp.vault.conf=./conf/bootstrap-hashicorp-vault.conf
 
+# Note: the following mapping properties only apply if a Sensitive Property Provider that uses property contexts
+# is configured.  Otherwise, these values are ignored.
+#
+# If no nifi.bootstrap.protection.xml.context.location.mapping.* properties are provided, the context for protected
+# properties uses their filename as a location prefix, e.g. "authorizers.xml||Manager Password".

Review comment:
       It looks like `getContextKey()` changed the separator from `||` to `/`.

##########
File path: nifi-commons/nifi-property-utils/src/main/java/org/apache/nifi/properties/ProtectedPropertyContext.java
##########
@@ -0,0 +1,119 @@
+/*
+ * 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 context for protected properties, encapsulating the name and location of a property.
+ * @see PropertyLocation#contextFor(String)
+ */
+public class ProtectedPropertyContext {
+    /**
+     * A property location, representing a specific configuration file.
+     */
+    public enum PropertyLocation {
+        LOGIN_IDENTITY_PROVIDERS("login-identity-providers.xml"),
+        AUTHORIZERS("authorizers.xml"),
+        NIFI_PROPERTIES("nifi.properties"),
+        NIFI_REGISTRY_PROPERTIES("nifi-registry.properties"),
+        CUSTOM("custom");

Review comment:
       Although this looks like it should cover existing configuration files, it seems somewhat difficult to maintain. Without making this too abstract, what do you think about other options for configuring the context?  Perhaps separating the sensitive properties provider configuration into its own file where these mappings could be configured?

##########
File path: nifi-commons/nifi-property-utils/src/main/java/org/apache/nifi/properties/BootstrapProperties.java
##########
@@ -32,13 +33,22 @@
 
     public enum BootstrapPropertyKey {
         SENSITIVE_KEY("bootstrap.sensitive.key"),
-        HASHICORP_VAULT_SENSITIVE_PROPERTY_PROVIDER_CONF("bootstrap.protection.hashicorp.vault.conf");
+        HASHICORP_VAULT_SENSITIVE_PROPERTY_PROVIDER_CONF("bootstrap.protection.hashicorp.vault.conf"),
+        XML_CONTEXT_LOCATION_MAPPING("bootstrap.protection.xml.context.location.mapping.");

Review comment:
       Naming this property with `XML` seems to be more format-specific than necessary, what do you think about renaming this to `CONTEXT_LOCATION_MAPPING`?

##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-resources/src/main/resources/conf/bootstrap.conf
##########
@@ -63,6 +63,20 @@ nifi.bootstrap.sensitive.key=
 # HashiCorp Vault Sensitive Property Providers
 nifi.bootstrap.protection.hashicorp.vault.conf=./conf/bootstrap-hashicorp-vault.conf
 
+# Note: the following mapping properties only apply if a Sensitive Property Provider that uses property contexts
+# is configured.  Otherwise, these values are ignored.
+#
+# If no nifi.bootstrap.protection.xml.context.location.mapping.* properties are provided, the context for protected
+# properties uses their filename as a location prefix, e.g. "authorizers.xml||Manager Password".
+# This creates a separate context for each unique property name in each XML configuration file.
+#
+# However, to reuse the same context in a more logical fashion, context mappings may be provided, in the format:
+# nifi.bootstrap.protection.xml.context.location.mapping.<contextLocation>=<identifier matching regex>
+# With the following configuration, for example, any XML property named "Manager Password" located inside
+# an XML block whose <identifier> starts with "ldap-" will be mapped to the context named "ldap||Manager Password",
+# regardless of whether it resides in authorizers.xml or login-identity-providers.xml.
+nifi.bootstrap.protection.xml.context.location.mapping.ldap=ldap-.*

Review comment:
       This explanation is helpful. Perhaps instead of defaulting to include a context, what about defaulting to no context or an internal default context?  That would allow values to be shared, but also allow differentiating them if necessary. Is there too much overlap in property naming for that to work?  Alternatively, having the default set of context name to file mapping in `bootstrap.conf` could provide a standard configuration without having to hard-coded locations in a `PropertyLocation` enum.




-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org