NIFI-8695: Adding context to sensitive property providers#5206
NIFI-8695: Adding context to sensitive property providers#5206gresockj wants to merge 12 commits intoapache:mainfrom
Conversation
432c790 to
19381d2
Compare
exceptionfactory
left a comment
There was a problem hiding this comment.
Thanks for putting together these changes @gresockj! The concept makes sense, and should be very helpful when it comes to additional Sensitive Property Provider implementations. As noted in specific comments, I'm wondering about the possibility of alternative approaches that could avoid hard-coding property file locations. Perhaps those are static enough that an enum is an acceptable approach, but it seems worth considering potential alternatives before going forward.
...ommons/nifi-property-utils/src/main/java/org/apache/nifi/properties/BootstrapProperties.java
Outdated
Show resolved
Hide resolved
...s/nifi-property-utils/src/main/java/org/apache/nifi/properties/ProtectedPropertyContext.java
Outdated
Show resolved
Hide resolved
...s/nifi-framework-bundle/nifi-framework/nifi-resources/src/main/resources/conf/bootstrap.conf
Outdated
Show resolved
Hide resolved
...s/nifi-framework-bundle/nifi-framework/nifi-resources/src/main/resources/conf/bootstrap.conf
Outdated
Show resolved
Hide resolved
|
Hey @gresockj ! I was wondering about potentially adding a I was thinking this would eliminate the need to modify existing SPPs which don't require this information, and addition of SPPs which dynamically protect/unprotect data vs. encrypting/decrypting them at rest could simply be a matter of adding a new |
I like your thought, @emiliosetiadarma. I think it will have to work the opposite way, with |
...rty-provider/src/main/java/org/apache/nifi/properties/AbstractSensitivePropertyProvider.java
Outdated
Show resolved
Hide resolved
...mons/nifi-property-utils/src/main/java/org/apache/nifi/properties/ApplicationProperties.java
Outdated
Show resolved
Hide resolved
This reverts commit ad75b33.
exceptionfactory
left a comment
There was a problem hiding this comment.
Thanks for making the latest round of updates @gresockj. The current approach looks good, and provides the flexibility for SPP implementations to decide whether or not to use the PropertyContext parameter.
I noted a few minor documentation and test adjustments, but otherwise this looks good.
...ommons/nifi-property-utils/src/main/java/org/apache/nifi/properties/BootstrapProperties.java
Outdated
Show resolved
Hide resolved
...erty-provider/src/main/java/org/apache/nifi/properties/SensitivePropertyProviderFactory.java
Outdated
Show resolved
Hide resolved
...vider/src/main/java/org/apache/nifi/properties/StandardSensitivePropertyProviderFactory.java
Outdated
Show resolved
Hide resolved
...s/nifi-framework-bundle/nifi-framework/nifi-resources/src/main/resources/conf/bootstrap.conf
Outdated
Show resolved
Hide resolved
nifi-registry/nifi-registry-core/nifi-registry-resources/src/main/resources/conf/bootstrap.conf
Outdated
Show resolved
Hide resolved
...operty-provider/src/test/java/org/apache/nifi/properties/AWSSensitivePropertyProviderIT.java
Outdated
Show resolved
Hide resolved
| # With the following configuration, for example, any property named "Manager Password" located inside | ||
| # a 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.context.mapping.ldap=ldap-.* |
There was a problem hiding this comment.
Although this example is helpful, it seems better to leave it commented out in the default configuration.
On further consideration, since none of the current SPP implementations support handling PropertyContexts, it seems better to remove this section completely. When the first SPP introduces leverages PropertyContexts, that seems like a better opportunity to update the project documentation and include one or two lines of comments in bootstrap.conf
nifi-registry/nifi-registry-core/nifi-registry-resources/src/main/resources/conf/bootstrap.conf
Outdated
Show resolved
Hide resolved
exceptionfactory
left a comment
There was a problem hiding this comment.
Thanks for working through the feedback @gresockj, the latest revisions look good. +1 Merging.
This closes apache#5206 Signed-off-by: David Handermann <exceptionfactory@apache.org>
Description of PR
Adds a
ProtectedPropertyContextargument toSensitivePropertyProvider.protect(...)andSensitivePropertyProvider.unprotect(...)in order to pave the way for sensitive property providers that require a context (e.g., using HashiCorp Vault's key-value Secrets engine). TheProtectedPropertyContextcomprises a PropertyLocation enum, representing the name of the configuration file in which the property resides, and a propertyName.In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically
main)?Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not
squashor use--forcewhen pushing to allow for clean monitoring of changes.For code changes:
mvn -Pcontrib-check clean installat the rootnififolder?LICENSEfile, including the mainLICENSEfile undernifi-assembly?NOTICEfile, including the mainNOTICEfile found undernifi-assembly?.displayNamein addition to .name (programmatic access) for each of the new properties?For documentation related changes:
Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.