NIFIREG-9: Initial Auth Implementation by kevdoran · Pull Request #14 · apache/nifi-registry

kevdoran · 2017-10-03T02:55:54Z

This is a rather large PR, including:

Authentication and authorization enforcement for web API, largely based on NiFi.
This adds interfaces, framework, and file-based authorizer providers (file access policy provider, file user group provider).
Authentication of identities is currently based on certificates in two-way SSL (HTTPS). Alternative identity strategies (user&pass, JWT) will be added later building upon the foundation in this commit.

For reviewers, a good place to start is AuthorizationService.java. That is called by Resources (web api endpoints) for authorizing requests. It makes use of authorizer providers (access policy provider and user group provider), which are interfaces and implementations heavily based in NiFi.

To run this, see the instructions in included authorizers.xml file. Configure that with your initial admin identity (must be certificate based) and configure your properties file to use your configured authorizers.

This functionality has been verified through manual testing. Documentation on proper configuration (aside what is in the provided in comment ins the included authorizers.xml example file) is in progress. Automated tests cases (unit, integration, etc.) are in progress and will likely be added to the PR during the time it is reviewed, but opening it now to get initial feedback, especially where it defines/modifies key interfaces.

As part of this feature, some changes were made to the RegistryService interface and the providers it utilizes. @bbende please review these changes when you get a change to make sure they are consistent with your design and intentions for the service and provider layers. Thanks!

Authentication and authorization enforcement for web API, largely based on NiFi. This commit adds interfaces, framework, and file-based authorizer providers (file access policy provider, file user group provider). Authentication of identities is currently based on certificates in two-way SSL (HTTPS). Alternative identity strategies (user&pass, JWT) will be added later building upon the foundation in this commit. As part of this feature, some changes were made to the RegistryService interface and the providers it utilizes.

bbende · 2017-10-03T13:14:39Z

Reviewing...

bbende

Overall looks great! Was able to secure the app and verify the REST endpoints blocked access when my user wasn't added to a policy. Just a few minor comments/suggestions.

bbende · 2017-10-03T14:58:40Z

+        // call post construction lifecycle event
+        instance.initialize(new StandardAuthorizerInitializationContext(identifier, this, this, this));
+
+//        if (StringUtils.isNotEmpty(classpathResources)) {


Should be fine to leave this out for now, but we may need to revisit this if/when we add support for Ranger... this was done so someone can configure additional libraries to be on the classpath of the authorizer, specifically for Ranger to know where the Hadoop client JARs were if wanting to send audit logs to HDFS.

Yeah hadn't gotten that far :)

Its a good point. I'll capture the outstanding work in a JIRA ticket and some in-line TODOs.

bbende · 2017-10-03T16:32:05Z

+        verifyAccessPolicyProviderIsConfigurable();
+
+        // Don't allow changing action or resource of existing policy (should only be adding/removing users/groups)
+        org.apache.nifi.registry.authorization.AccessPolicy currentAccessPolicy =


Should this be within the write lock?

I'm thinking if two people tried to update at the same time, they could both retrieve the initial policy, then assuming user1 obtains the write lock and makes changes, now user2 gets the write lock but is making changes to the initial version and doesn't have user1's changes.

The idea here is that no user can change the accessPolicy identifier, resource, or action fields on update. They can only change groups or users that have an access policy. To change resource or action, they would have to delete and create a new access policy.

So in your example, user1 and user2 both get the initial version, but those should be guaranteed to match for the fields we want to read. Unless I'm making a mistake in my logic...

That said, I think I'll move it to inside the write lock anyway, as it won't hurt and would protect this code block from future changes that break the expectation I had.

bbende · 2017-10-03T16:38:58Z

+
+        readLock.lock();
+        try {
+            final Set<BucketEntity> filterBuckets = bucketIdentifiers.stream()


Should we create a method in metadataService to retrieve all the buckets for the set of ids in one call?

I'm just slightly concerned making an individual DB retrieval for each id if there are a lot of ids passed in.

Yeah I agree that would be a better approach. Will make that change, thanks.

bbende · 2017-10-03T16:45:55Z

            <version>4.12</version>
            <scope>test</scope>
        </dependency>
+        <dependency>


I think we can get rid of this, doing a quick test the build still passed without it

bbende · 2017-10-03T16:59:52Z

                        </goals>
                        <configuration>
-                            <packageName>org.apache.nifi.registry.user.generated</packageName>
+                            <sources>


I noticed this file showed errors in my IDE, even though technically it built fine... seems like bumping the jaxb2-maven-plugin to 2.3.1 in the root pom pluginManagement cleared this up

Good find, will update that plugin version.

bbende · 2017-10-03T17:56:41Z

+/**
+ * Defines a policy for a set of userIdentifiers to perform a set of actions on a given resource.
+ */
+public class AccessPolicy {


I know this would be a tedious change, but what do you think about splitting out all of the interfaces/domain objects from nifi-registry-security into nifi-registry-security-api?

My thinking is that nifi-registry-security-api would live in the lib directory and be something that other people would implement against, where as nifi-registry-security would be bundled under the WAR for nifi-registry-web-api.

It's a good suggestion. I wasn't thrilled with the current organization. Your approach both distinguishes interfaces from implementation and adds extensibility. Will make that change, thanks!

bbende · 2017-10-03T18:01:24Z

            <artifactId>spring-boot-starter-web</artifactId>
            <version>${spring.boot.version}</version>
        </dependency>
+        <dependency>


If we make the change in one of the previous comments where we use spring-boot-starter-security in the nifi-registry-security pom, then we probably don't need to directly reference it here because it will be brought in transitively

bbende · 2017-10-03T18:12:06Z

@@ -0,0 +1,47 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more


I think this was maybe left-over from testing and not meant to be checked in?

yeah this was a test file. will remove

bbende · 2017-10-03T18:17:15Z

        <nifi.registry.security.truststorePasswd />
        <nifi.registry.security.needClientAuth />
        <nifi.registry.security.authorized.users>./conf/authorized-users.xml</nifi.registry.security.authorized.users>
+        <nifi.security.user.authorizer>managed-authorizer</nifi.security.user.authorizer>


Is the nifi.registry.security.authorized.users still needed?

nope. neither is login.identity.provider. will remove unused properties and fix the prefixes to be consistent. good catch!

bbende · 2017-10-03T18:18:11Z

 nifi.registry.security.truststorePasswd=${nifi.registry.security.truststorePasswd}
 nifi.registry.security.needClientAuth=${nifi.registry.security.needClientAuth}
 nifi.registry.security.authorized.users=${nifi.registry.security.authorized.users}
+nifi.security.user.authorizer=${nifi.security.user.authorizer}


Is nifi.registry.security.authorized.users still needed?

same as above. removed in the next update

kevdoran · 2017-10-03T21:06:31Z

Thanks a lot for the review @bbende! Great suggestions. Will make those changes and update.

scottyaslan · 2017-10-05T14:53:49Z

@kevdoran With this PR when I try to GET http://localhost:8080/nifi-registry-api/items I am getting a "400 Bad Request: Bucket Identifiers cannot be null or empty"...

kevdoran · 2017-10-05T15:43:50Z

Good catch @scottyaslan - will update with a fix.

kevdoran · 2017-10-05T16:52:29Z

@bbende I restructured the modules based on your suggestion. I now have the following modules:

nifi-registry-security                 // just a container module, packaged as pom
  +--nifi-registry-security-api        // gets placed in lib dir for others to implement against
  +--nifi-registry-security-api-impl   // NiFi Registry's implementation of security-api
  +--nifi-registry-security-framework  // extends registry-framework to add security

Let me know if you agree with this or if it is overkill for what we need. Technically nifi-registry-security-api-impl and nifi-registry-security-framework can be combined (technically, they could even be part of nifi-registry-framework). I can consolidate some of these if you'd rather keep a flatter, more condensed module structure. Let me know.

…r review feedback

bbende · 2017-10-05T17:35:20Z

Thanks for the update! Everything is looking good.

I'm thinking maybe we can fold nifi-registry-security-framework into nifi-registry-framework, and then move nifi-registry-security-api-impl and nifi-registry-security-api to top-level modules, just to keep it a little flatter.

bbende · 2017-10-05T18:05:31Z

+1 Latest update looks good, going to merge shortly, thanks!

bbende reviewed Oct 3, 2017

View reviewed changes

NIFIREG-9 Restructure security modules and other changes based on pee…

3fd1e5a

…r review feedback

kevdoran force-pushed the NIFIREG-9 branch from 597bbe4 to 3fd1e5a Compare October 5, 2017 17:23

NIFIREG-9 Consolidating security modules

6de4e07

asfgit closed this in 785cb81 Oct 5, 2017

kevdoran deleted the NIFIREG-9 branch October 25, 2017 14:38

		@@ -0,0 +1,47 @@
		# Licensed to the Apache Software Foundation (ASF) under one or more

Uh oh!

Conversation

kevdoran commented Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bbende commented Oct 3, 2017

Uh oh!

bbende left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevdoran Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevdoran commented Oct 3, 2017

Uh oh!

scottyaslan commented Oct 5, 2017

Uh oh!

kevdoran commented Oct 5, 2017

Uh oh!

kevdoran commented Oct 5, 2017

Uh oh!

bbende commented Oct 5, 2017

Uh oh!

bbende commented Oct 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kevdoran commented Oct 3, 2017 •

edited

Loading

kevdoran Oct 3, 2017 •

edited

Loading