GEODE-3447 Implement client authorization for the new protocol #719

bschuchardt · 2017-08-17T22:34:30Z

Implementation of authorization checks for the new protocol. This will have to be merged with the Locator Protobuf communications work that is in progress on another branch before we check it in.

@kohlmu-pivotal @hiteshk25 @galen-pivotal @pivotal-amurmann @WireBaron

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

[ x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
[ x] Has your PR been rebased against the latest commit within the target branch (typically develop)?
[x ] Is your initial contribution a single, squashed commit?
Does gradlew build run cleanly?
rat failed on a new StreamAuthorizer interface file. This will be corrected.
[ x] 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?

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. If you need help, please send an
email to dev@geode.apache.org.

kohlmu-pivotal · 2017-08-18T18:26:23Z

geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufSimpleAuthorizer.java

+  }
+
+  @Override
+  public boolean authorize(ResourcePermission permissionRequested) {


why would I not pass in the Principal and requested permissions? Then I can have 1 authorizer and not 1 per user.

I'd rather not have other code deal with the Principal. Other implementations may not have a Principal. I.e., a no-op implementation.

kohlmu-pivotal · 2017-08-18T20:39:21Z

...e-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufSimpleAuthenticator.java

        .build().writeDelimitedTo(outputStream);
  }

  @Override
  public boolean isAuthenticated() {
-    return authenticated;
+    return authorizer != null;


I must disagree with this logic. Something is NOT authenticated just because there is an authorizer populated. A authorizer should be constructed BECAUSE something was authenticated

A authorizer IS only constructed BECAUSE something was authenticated. Therefore this is a valid implementation of isAuthenticated().

kohlmu-pivotal · 2017-08-18T20:41:01Z

...e-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufSimpleAuthenticator.java

    try {
      Object principal = securityManager.authenticate(properties);
-      authenticated = principal != null;
+      if (principal != null) {
+        authorizer = new ProtobufSimpleAuthorizer(principal, securityManager);


Why do we need a ProtobufSimpleAuthorizer per principal? Could we not have a single Authorizer that will authenticate all principals?

The Principal is part of the implementation of this authentication & authorization algorithm and is encapsulated by this class.

Implementation of authorization checks for the new protocol.

galen-pivotal

If we ever need to do permissions per region or other more fine-grained permissions we'll need to refactor this. that's fine, we don't need it now.

The one thing that does worry me is that clients (say, producers) may be authorized for write and not read, and GetAvailableServers might not work for them. Does GetAvailableServers need any permissions? I don't think the current client authorizes... perhaps we could simply not authorize? I'm a bit on the fence, what do you think?

galen-pivotal · 2017-08-23T00:38:24Z

geode-core/src/main/java/org/apache/geode/security/StreamAuthorizer.java

+package org.apache.geode.security;
+
+public interface StreamAuthorizer {
+  boolean authorize(ResourcePermission permissionRequested);


We may need to refactor this at some point when operations start needing more than one permission (which depends on whether write authorization implies read authorization), but I think that's more than a failing of the security API than ours, and it should be possible to do everything we need for the alpha anyways (no getAndSet yet).

galen-pivotal · 2017-08-23T00:45:03Z

...obuf/src/main/java/org/apache/geode/protocol/protobuf/registry/OperationContextRegistry.java

-        ClientProtocol.Request::getGetAvailableServersRequest,
-        new GetAvailableServersOperationHandler(),
-        opsResp -> ClientProtocol.Response.newBuilder().setGetAvailableServersResponse(opsResp)));
+    operationContexts.put(RequestAPICase.GETAVAILABLESERVERSREQUEST,


This is one that should be authorized for either read or write... is read a subset of write? I don't know.

galen-pivotal · 2017-08-23T00:54:01Z

geode-protobuf/src/test/java/org/apache/geode/protocol/AuthorizationIntegrationTest.java

+    verifyOperations(true, true);
+  }
+
+  private void verifyOperations(boolean readAllowed, boolean writeAllowed) throws Exception {


+1 thorough and clear tests.

bschuchardt · 2017-08-23T16:07:04Z

GetAvailableServers will be executed on a Locator which installs a no-op authorizer in the execution "context". That can change if we decide later on that we want to perform authentication and authorization on locator requests.

galen-pivotal

Okay, if we've got no-op on the locator then ship it!

pivotal-amurmann

A few opportunities for minor improvements but generally looks great.

pivotal-amurmann · 2017-08-24T23:12:07Z

geode-core/src/main/java/org/apache/geode/security/StreamAuthenticator.java

+   * Return an authorization object which can be used to determine which permissions this stream has
+   * according to the provided securityManager.
+   *
+   * Calling this before authentication has succeeded may result in a null return object.


Could we indicate this by using a Optional? That way we also force the consumer to deal with that case.

I kind of prefer a comment or even annotation saying nullable -- optionals themselves can be null, and have some cost.

How am I gonna see the comment as a user of this? I think it's unreasonable to assume that every consumer of the class is gonna look at its code.
I agree that it's exceedingly unfortunate that Java fails completely and allows null Optionals. However, if we expose a optional we are telling the consumer explicitly that they need to concern themselves with the empty case. We are also telling them that they shouldn't worry about the optional itself being null, because that's the concern of our method and not theirs. If we function as promised it's not gonna be null. I'd rather expose that contract explicitly than throw some text into the file that is only meaningful to humans who happen to look at the implementation.

Optional doesn't really fix the problem, just introduces more GC pressure :-)

Wouldn't it be better to throw IllegalStateException when the precondition is not met?

We'll change this to throw AuthenticationRequiredException if authentication has not yet been performed.

pivotal-amurmann · 2017-08-24T23:13:02Z

geode-core/src/main/java/org/apache/geode/security/StreamAuthorizer.java

+ */
+package org.apache.geode.security;
+
+public interface StreamAuthorizer {


Does it make sense to remove the Stream part of this name? There seems to be nothing about this interface that is related to streams at all.

pivotal-amurmann · 2017-08-24T23:15:06Z

geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessor.java

+      if (context.getAuthorizer().authorize(operationContext.getAccessPermissionRequired())) {
+        result = operationContext.getOperationHandler().process(serializationService,
+            operationContext.getFromRequest().apply(request), context);
+      } else {


We might want to log this. Logging all security related events can be a life saver during or after a security incident.

pivotal-amurmann · 2017-08-24T23:16:08Z

...e-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufSimpleAuthenticator.java

        .build().writeDelimitedTo(outputStream);
  }

  @Override
  public boolean isAuthenticated() {
-    return authenticated;
+    // note: an authorizer is only created if the user has been authenticated
+    return authorizer != null;


this would be nicer as well if Optional

pivotal-amurmann · 2017-08-24T23:24:35Z

geode-protobuf/src/test/java/org/apache/geode/protocol/AuthorizationIntegrationTest.java

+import org.apache.geode.test.junit.categories.IntegrationTest;
+
+@Category(IntegrationTest.class)
+public class AuthorizationIntegrationTest {


Hugs for not adding this to the existing Roundtrip test! 🤗

hiteshk25

Are we introducing new "Authorizer" interface which app needs to implement?

metatype

I'm confused why we need both SecurityManager and StreamAuthenticator. Are we bleeding communication/protocol ideas into the role-based access control domain?

galen-pivotal · 2017-08-25T20:49:47Z

@metatype We need the StreamAuthenticator to receive and send (Protobuf-encoded) messages containing the credentials that get passed to the SecurityManager. I would think that ideally it's nothing more than this.

I wonder if it would be better to send a hash that gets put into the Properties that SecurityManager uses, rather than having a message that explicitly contains username and password?

metatype · 2017-08-25T21:03:19Z

I'm not suggesting changing the protobuf message. That seems ok to me.

Since the StreamAuthenticator is part of protobuf messaging I think it should remain in the geode-protobuf module. Putting it in a public API package for RBAC seems like we're leaking abstractions into other parts of the codebase.

Am I missing something?

bschuchardt · 2017-08-28T16:48:45Z

@metatype StreamAuthenticator wasn't added in this change set. We added a StreamAuthorizer. Both of these are needed in geode-core because SecurityManager doesn't know about communication streams and we need these interfaces to be able to plug in checks in the geode-protobuf module.

to be done.

bschuchardt · 2017-08-28T17:57:22Z

@metatype After reviewing StreamAuthenticator I get your point. I think these new interfaces and classes need to be in a different package.

kohlmu-pivotal reviewed Aug 18, 2017

View reviewed changes

bschuchardt added 3 commits August 22, 2017 14:46

GEODE-3447 Implement client authorization for the new protocol

a2b678a

Implementation of authorization checks for the new protocol.

addressing review comments

495d788

rebase to origin/develop

ea1d8c1

asfgit force-pushed the feature/GEODE-3447 branch from 404eeca to ea1d8c1 Compare August 22, 2017 22:36

galen-pivotal reviewed Aug 23, 2017

View reviewed changes

galen-pivotal approved these changes Aug 23, 2017

View reviewed changes

pivotal-amurmann approved these changes Aug 24, 2017

View reviewed changes

kohlmu-pivotal approved these changes Aug 25, 2017

View reviewed changes

hiteshk25 reviewed Aug 25, 2017

View reviewed changes

metatype reviewed Aug 25, 2017

View reviewed changes

bschuchardt closed this Aug 28, 2017

bschuchardt reopened this Aug 28, 2017

throw an exception rather than return null if authentication has yet

ab8159d

to be done.

bschuchardt closed this Aug 28, 2017

PurelyApplied deleted the feature/GEODE-3447 branch November 16, 2018 23:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GEODE-3447 Implement client authorization for the new protocol #719

GEODE-3447 Implement client authorization for the new protocol #719

bschuchardt commented Aug 17, 2017 •

edited

kohlmu-pivotal Aug 18, 2017

bschuchardt Aug 22, 2017

kohlmu-pivotal Aug 18, 2017

bschuchardt Aug 22, 2017

kohlmu-pivotal Aug 18, 2017

bschuchardt Aug 22, 2017

galen-pivotal left a comment

galen-pivotal Aug 23, 2017

galen-pivotal Aug 23, 2017

galen-pivotal Aug 23, 2017

bschuchardt commented Aug 23, 2017

galen-pivotal left a comment

pivotal-amurmann left a comment

pivotal-amurmann Aug 24, 2017

galen-pivotal Aug 24, 2017

pivotal-amurmann Aug 28, 2017 •

edited

metatype Aug 28, 2017

bschuchardt Aug 28, 2017

pivotal-amurmann Aug 24, 2017

pivotal-amurmann Aug 24, 2017

pivotal-amurmann Aug 24, 2017

pivotal-amurmann Aug 24, 2017

hiteshk25 left a comment

metatype left a comment

galen-pivotal commented Aug 25, 2017 •

edited

metatype commented Aug 25, 2017

bschuchardt commented Aug 28, 2017

bschuchardt commented Aug 28, 2017

GEODE-3447 Implement client authorization for the new protocol #719

GEODE-3447 Implement client authorization for the new protocol #719

Conversation

bschuchardt commented Aug 17, 2017 • edited

For all changes:

Note:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galen-pivotal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bschuchardt commented Aug 23, 2017

galen-pivotal left a comment

Choose a reason for hiding this comment

pivotal-amurmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pivotal-amurmann Aug 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiteshk25 left a comment

Choose a reason for hiding this comment

metatype left a comment

Choose a reason for hiding this comment

galen-pivotal commented Aug 25, 2017 • edited

metatype commented Aug 25, 2017

bschuchardt commented Aug 28, 2017

bschuchardt commented Aug 28, 2017

bschuchardt commented Aug 17, 2017 •

edited

pivotal-amurmann Aug 28, 2017 •

edited

galen-pivotal commented Aug 25, 2017 •

edited