You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by mcgilman <gi...@git.apache.org> on 2016/11/18 21:23:47 UTC
[GitHub] nifi pull request #1247: Introducing restricted components which require add...
GitHub user mcgilman opened a pull request:
https://github.com/apache/nifi/pull/1247
Introducing restricted components which require additional access
NIFI-3050:
- Introducing a Restricted annotation for components that require elevated privileges to use.
- Updating the new Processor, Controller Service, and Reporting Task dialogs to include these details and prevent unauthorized selection.
- Including the Restricted description in the generated component documentation.
- Updating processor access control integration test to verify restricted component creation.
- Updating the developer, user, and admin guide to include the restricted component policy.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mcgilman/nifi NIFI-3050
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/1247.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 #1247
----
commit 890c42bb79d9b57ce7ed3e4d547337d76e741bc1
Author: Matt Gilman <ma...@gmail.com>
Date: 2016-11-18T21:19:04Z
NIFI-3050:
- Introducing a Restricted annotation for components that require elevated privileges to use.
- Updating the new Processor, Controller Service, and Reporting Task dialogs to include these details and prevent unauthorized selection.
- Including the Restricted description in the generated component documentation.
- Updating processor access control integration test to verify restricted component creation.
- Updating the developer, user, and admin guide to include the restricted component policy.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1247: Introducing restricted components which require add...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1247#discussion_r88792883
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/service/StandardControllerServiceNode.java ---
@@ -121,6 +122,11 @@ public Resource getResource() {
}
@Override
+ public boolean isRestricted() {
--- End diff --
Could this method be moved to `AbstractConfiguredComponent`? It seems like each place where it is implemented independently also extends that abstract class.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1247: Introducing restricted components which require additional...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/1247
Checked out the new changes. I like it. Thanks Matt.
Verified that existing restricted processors show indicator icon on canvas:
<img width="503" alt="Restricted processor showing indicator icon on canvas" src="https://cloud.githubusercontent.com/assets/798465/20497671/eed74c8a-afde-11e6-9c8f-765f0305c3ca.png">
Verified improved searchability:
Verified reporting task:
Verified that the restricted components are as follows:
* Controller Services
* *None*
* Reporting Tasks
* `SiteToSiteProvenanceReportingTask`
* Processors
* `DeleteHDFS`
* `ExecuteFlumeSink`
* `ExecuteFlumeSource`
* `ExecuteProcess`
* `ExecuteScript`
* `ExecuteStreamCommand`
* `FetchFile`
* `FetchHDFS`
* `GetFile`
* `GetHDFS`
* `InvokeScriptedProcessor`
* `PutFile`
* `PutHDFS`
* `TailFile`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1247: Introducing restricted components which require additional...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/1247
I'm just running the `contrib-check` and tests now. Once those pass, I'll provide a +1 and merge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1247: Introducing restricted components which require additional...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/1247
The issues with cleaning the content_repository were local to my machine. I resolved those with @markap14 's help. Now I set up two client certificates and two user identities -- myself and "Matt". We have the same permissions except that Andy can add restricted components and Matt cannot.
I then began adding components while logged in as Andy and Matt in separate windows. Andy was able to add and modify *restricted* components, while Matt was not.
<img width="1920" alt="Side by side canvases as different users" src="https://cloud.githubusercontent.com/assets/798465/20460020/ac5564c8-ae8a-11e6-94c0-52e278122c15.png">
I really like the small shield icon in the Add Processor dialog which has hover text explaining the restriction.
I think we can do more to indicate the restrictions, however. In the Add Processor dialog, I think it makes sense to put a textual indicator in the description area at the bottom above the processor description -- perhaps red text saying "Restricted Processor". I also think we should update the restricted processors with a *Restricted* tag so they can be quickly enumerated from the tag cloud or search field. I should be able to type "restr..." and see a list of all the restricted processors.
I also think it makes sense to show the shield icon (I was originally going to suggest a lock icon but I do like the shield) on processors that are already on the canvas. I think this will also help users who have a processor which we have now decided is restricted already on their canvas and they suddenly do not have access to it because of the new policy enforcement.
I am willing to +1 and merge this (I made one small checkstyle correction in `ITProcessorAccessControl.java` in Javadoc to resolve an issue), but would like to hear from @mcgilman how much work would be involved in addressing the minor suggestions I made above. They could be separated into a new enhancement Jira if necessary, pending release vote timing, as I think the current work does meet what was discussed in the original Jira.
Thanks Matt.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1247: Introducing restricted components which require additional...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on the issue:
https://github.com/apache/nifi/pull/1247
I worked through the majority of the recommendations. However, I think it makes sense to hold off on adding anything to the description area of the New Component Dialogs. We likely need to reconsider the layout and what is shown. As is, there is not enough space to adequately display all necessary information. Leaving the restriction as a tooltip in the table is a good option for the initial implementation.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1247: Introducing restricted components which require additional...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/1247
Reviewing...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1247: Introducing restricted components which require add...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/1247
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1247: Introducing restricted components which require additional...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/1247
I'm getting some errors when running the standard test suite. Did configuration requirements change for this?
```
Results :
Failed tests:
TestPutTcpSSL>TestPutTCPCommon.testLoadTest:231->TestPutTCPCommon.sendTestData:254 Processor has 1 validation failures:
'SSL Context Service' validated against 'ssl-context' is invalid because Controller Service is not valid: ['Keystore Properties' is invalid because Invalid KeyStore Password or Type specified for file src/test/resources/localhost-ks.jks, 'Truststore Properties' is invalid because Invalid KeyStore Password or Type specified for file src/test/resources/localhost-ts.jks]
Tests in error:
TestListDatabaseTables.cleanUpAfterClass:71 � SQL Database 'target/db_ldt' not...
TestListDatabaseTables.testListTablesAfterRefresh:153 � Process getConnection ...
TestListDatabaseTables.testListTablesMultipleRefresh:190 � Process getConnecti...
TestListDatabaseTables.testListTablesNoCount:100 � Process getConnection faile...
TestListDatabaseTables.testListTablesWithCount:126 � Process getConnection fai...
TestPutTcpSSL>TestPutTCPCommon.testConnectionPerFlowFile:142->TestPutTCPCommon.checkReceivedAllData:278->TestPutTCPCommon.checkReceivedAllData:285 � TestTimedOut
TestPutUDP.testLoadTest:176->checkReceivedAllData:236 � TestTimedOut test time...
TestPutUDP.testValidFiles:98->checkReceivedAllData:229->checkReceivedAllData:236 � TestTimedOut
Tests run: 994, Failures: 1, Errors: 8, Skipped: 17
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1247: Introducing restricted components which require additional...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on the issue:
https://github.com/apache/nifi/pull/1247
Thanks for the review @alopresto! I should be able to update this PR with all the of the suggestions and code comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1247: Introducing restricted components which require add...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1247#discussion_r88902132
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/service/StandardControllerServiceNode.java ---
@@ -121,6 +122,11 @@ public Resource getResource() {
}
@Override
+ public boolean isRestricted() {
--- End diff --
This cannot be moved as the logic to get the underlying class is not consistent between Processor's, Controller Service's, and Reporting Task's. Alternatively, we could introduce another method that is responsible for returning the underlying instance type. This would allow us to move `isRestricted` up but I didn't think it was necessary.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1247: Introducing restricted components which require add...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1247#discussion_r88792639
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/test/java/org/apache/nifi/integration/accesscontrol/ITProcessorAccessControl.java ---
@@ -399,6 +400,43 @@ public void testNoneUserDeleteProcessor() throws Exception {
verifyDelete(helper.getNoneUser(), NONE_CLIENT_ID, 403);
}
+ /**
+ * Tests attempt to create a restricted processor.
+ *
+ * @throws Exception
--- End diff --
Checkstyle fails on Javadoc with empty message. I changed the line to `* @throws Exception if there is an error creating this processor` to resolve.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1247: Introducing restricted components which require additional...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/1247
There is an issue in `master` where the `nifi-app.log` is spammed with the following lines:
```
2016-11-18 17:39:14,053 ERROR [Cleanup Archive for default] o.a.n.c.repository.FileSystemRepository Failed to cleanup archive for container default due to java.nio.file.NoSuchFileException: /Users/alopresto/Workspace/nifi/nifi-assembly/target/nifi-1.1.0-SNAPSHOT-bin/nifi-1.1.0-SNAPSHOT/content_repository
2016-11-18 17:39:15,055 ERROR [Cleanup Archive for default] o.a.n.c.repository.FileSystemRepository Failed to cleanup archive for container default due to java.nio.file.NoSuchFileException: /Users/alopresto/Workspace/nifi/nifi-assembly/target/nifi-1.1.0-SNAPSHOT-bin/nifi-1.1.0-SNAPSHOT/content_repository
```
Running an unsecured instance has this message but still loads the app fine, but with keystores configured, the application fails to start.
```
hw12203:...space/nifi/nifi-assembly/target/nifi-1.1.0-SNAPSHOT-bin/nifi-1.1.0-SNAPSHOT (pr1247) alopresto
\U0001f513 404s @ 17:39:21 $ ./bin/nifi.sh status
Java home: /Users/alopresto/.jenv/versions/1.8
NiFi home: /Users/alopresto/Workspace/nifi/nifi-assembly/target/nifi-1.1.0-SNAPSHOT-bin/nifi-1.1.0-SNAPSHOT
Bootstrap Config File: /Users/alopresto/Workspace/nifi/nifi-assembly/target/nifi-1.1.0-SNAPSHOT-bin/nifi-1.1.0-SNAPSHOT/conf/bootstrap.conf
2016-11-18 17:40:05,146 INFO [main] org.apache.nifi.bootstrap.Command Apache NiFi is not running
hw12203:...space/nifi/nifi-assembly/target/nifi-1.1.0-SNAPSHOT-bin/nifi-1.1.0-SNAPSHOT (pr1247) alopresto
\U0001f513 47s @ 17:40:08 $
```
At this point I can't verify the restricted processor changes because it requires a secured instance with multiple user accounts. The bug needs to be fixed in `master` first. I have filed [NIFI-3069](https://issues.apache.org/jira/browse/NIFI-3069) to document and address this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1247: Introducing restricted components which require add...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1247#discussion_r88792735
--- Diff: nifi-docs/src/main/asciidoc/developer-guide.adoc ---
@@ -565,6 +565,23 @@ for instance, they should not be
relied upon for critical business logic.
+[[restricted]]
+=== Restricted
+
+A Restricted component is one that can be used to execute arbitrary unsanitized code provided by the operator
+through the NiFi REST API/UI or can be used to obtain or alter data on the NiFi host system using the NiFi OS
+credentials. These components could be used by an otherwise authorized NiFi user to go beyond the intended use of
+the application, escalate privilege, or could expose data about the internals of the NiFi process or the host
+system. All of these capabilities should be considered privileged, and admins should be aware of these
+capabilities and explicitly enable them for a subset of trusted users.
+
+A Processor, Controller Service, or Reporting Task can be marked with the @Restricted annotation. This
--- End diff --
Maybe indicate `@Restricted` in code-formatting for consistency throughout document?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---