You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by pvillard31 <gi...@git.apache.org> on 2018/06/11 19:19:06 UTC
[GitHub] nifi pull request #2785: NIFI-5296 - Add EL Support with Variable Registry s...
GitHub user pvillard31 opened a pull request:
https://github.com/apache/nifi/pull/2785
NIFI-5296 - Add EL Support with Variable Registry scope on SSL contex…
…t service
Thank you for submitting a contribution to Apache NiFi.
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 master)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
- [ ] If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
- [ ] If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/pvillard31/nifi NIFI-5296
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2785.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2785
----
commit c38e53f295d04055351dcb76f539e1338b229c30
Author: Pierre Villard <pi...@...>
Date: 2018-06-11T19:18:40Z
NIFI-5296 - Add EL Support with Variable Registry scope on SSL context service
----
---
[GitHub] nifi issue #2785: NIFI-5296 - Add EL Support with Variable Registry scope on...
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on the issue:
https://github.com/apache/nifi/pull/2785
@alopresto - could you have a look? I want to be sure I'm not doing something that would not be allowed for some reasons (I removed unit tests specifically checking that EL was not allowed, I hope I'm not missing a point here).
---
[GitHub] nifi issue #2785: NIFI-5296 - Add EL Support with Variable Registry scope on...
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on the issue:
https://github.com/apache/nifi/pull/2785
Thanks for your feedback @alopresto - as discussed, I share your concerns and I'm not convinced this brings any value as I'd expect keystore and truststore to be located in the same place across environments.
That's something we could reconsider when we have a clear solution to handle sensitive properties in variables. I'll close this PR for now.
---
[GitHub] nifi pull request #2785: NIFI-5296 - Add EL Support with Variable Registry s...
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 closed the pull request at:
https://github.com/apache/nifi/pull/2785
---
[GitHub] nifi issue #2785: NIFI-5296 - Add EL Support with Variable Registry scope on...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/2785
I discussed with @pvillard31 and we are going to hold off on merging this for right now. Here is what I wrote on the Jira:
> Pierre Villard can you describe the use case where you feel VR EL support is necessary for these fields? I am guessing you want to be able to specify a keystore and truststore path in the VR and allow an SSLContextService to reference it. I have a couple concerns with this approach:
>
> It allows any user with variable permissions to see the path to the keystore and truststore. In the current setup, the controller service can be deployed with literal paths by an administrator/trusted user, and the service can be referenced by dependent components without exposing the actual file system paths to a less-trusted user (given the proper absence of permissions on the controller service). Currently, a user who does not have view permission to the StandardSSLContextService cannot see the explicit path at all, even if they have view permission to the referencing InvokeHTTP processor (for example). In this new scenario, they would be able to see the explicit path in the Variables window, and see the UUID of the referencing SSLCS (see https://nifi.apache.org/docs/nifi-docs/html/user-guide.html#unauthorized-referencing-components)
> It allows the keystore and truststore path to change without visibility on the configuration dialog. Given EL evaluation, the processor will evaluate not only VR-specific variables, but also system properties and OS-level environment variables. This means that a malicious or incidental occurrence could change the path used to locate the keystore and truststore without the NiFi user being aware
> The original work that introduced the unit tests you reference was done in NIFI-4274 because of a Stack Overflow question where the documentation (which said EL was not supported) and the behavior (incorrectly evaluating EL) did not match.
>
> I appreciate the work you put into this PR. If you have a specific use case that really requires this behavior, let's discuss it. If this is just to bring these properties inline with the baseline EL evaluation, I would ask that we do not implement this at this time.
>
> There is some ongoing work/discussion about how to handle the sensitive properties in conjunction with the VR, Registry, flow versioning, setting from external sources, etc. and I believe it requires a cohesive approach with substantial threat modeling to avoid introducing serious issues into NiFi. We have traditionally held to a "secure but severe" policy where some features are absent because of the strict principle that "sensitive properties are always protected/blocked". It may be time to re-evaluate that, but introducing these changes piecemeal is dangerous in my opinion.
---
[GitHub] nifi issue #2785: NIFI-5296 - Add EL Support with Variable Registry scope on...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/2785
Reviewing...
---