You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by bschuchardt <gi...@git.apache.org> on 2017/08/17 22:34:33 UTC

[GitHub] geode pull request #719: GEODE-3447 Implement client authorization for the n...

GitHub user bschuchardt opened a pull request:

    https://github.com/apache/geode/pull/719

    GEODE-3447 Implement client authorization for the new protocol

    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](http://www.apache.org/legal/resolved.html#category-a)?
    
    ### 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.


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/apache/geode feature/GEODE-3447

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/geode/pull/719.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 #719
    
----
commit 7d6a0a3e31a11c5bf584fe59efbfaebaf98e6c4d
Author: Bruce Schuchardt <bs...@pivotal.io>
Date:   2017-08-17T22:22:57Z

    GEODE-3447 Implement client authorization for the new protocol
    
    Implementation of authorization checks for the new protocol.

----


---
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] geode pull request #719: GEODE-3447 Implement client authorization for the n...

Posted by kohlmu-pivotal <gi...@git.apache.org>.
Github user kohlmu-pivotal commented on a diff in the pull request:

    https://github.com/apache/geode/pull/719#discussion_r134050811
  
    --- Diff: geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufSimpleAuthenticator.java ---
    @@ -40,20 +42,28 @@ public void receiveMessage(InputStream inputStream, OutputStream outputStream,
         properties.setProperty("username", authenticationRequest.getUsername());
         properties.setProperty("password", authenticationRequest.getPassword());
     
    +    authorizer = null; // authenticating a new user clears current authorizer
         try {
           Object principal = securityManager.authenticate(properties);
    -      authenticated = principal != null;
    +      if (principal != null) {
    +        authorizer = new ProtobufSimpleAuthorizer(principal, securityManager);
    +      }
         } catch (AuthenticationFailedException e) {
    -      authenticated = false;
    +      authorizer = null;
         }
     
    -    AuthenticationAPI.SimpleAuthenticationResponse.newBuilder().setAuthenticated(authenticated)
    +    AuthenticationAPI.SimpleAuthenticationResponse.newBuilder().setAuthenticated(isAuthenticated())
             .build().writeDelimitedTo(outputStream);
       }
     
       @Override
       public boolean isAuthenticated() {
    -    return authenticated;
    +    return authorizer != null;
    --- End diff --
    
    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`


---
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] geode pull request #719: GEODE-3447 Implement client authorization for the n...

Posted by bschuchardt <gi...@git.apache.org>.
Github user bschuchardt commented on a diff in the pull request:

    https://github.com/apache/geode/pull/719#discussion_r134599066
  
    --- Diff: geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufSimpleAuthorizer.java ---
    @@ -0,0 +1,42 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
    + * agreements. See the NOTICE file distributed with this work for additional information regarding
    + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance with the License. You may obtain a
    + * copy of the License at
    + *
    + * http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software distributed under the License
    + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
    + * or implied. See the License for the specific language governing permissions and limitations under
    + * the License.
    + */
    +package org.apache.geode.protocol.protobuf;
    +
    +import java.io.EOFException;
    +import java.io.IOException;
    +import java.io.InputStream;
    +import java.io.OutputStream;
    +import java.util.Properties;
    +
    +import org.apache.geode.security.AuthenticationFailedException;
    +import org.apache.geode.security.ResourcePermission;
    +import org.apache.geode.security.SecurityManager;
    +import org.apache.geode.security.StreamAuthenticator;
    +import org.apache.geode.security.StreamAuthorizer;
    +
    +public class ProtobufSimpleAuthorizer implements StreamAuthorizer {
    +  private final Object authenticatedPrincipal;
    +  private final SecurityManager securityManager;
    +
    +  public ProtobufSimpleAuthorizer(Object authenticatedPrincipal, SecurityManager securityManager) {
    +    this.authenticatedPrincipal = authenticatedPrincipal;
    +    this.securityManager = securityManager;
    +  }
    +
    +  @Override
    +  public boolean authorize(ResourcePermission permissionRequested) {
    --- End diff --
    
    I'd rather not have other code deal with the Principal.  Other implementations may not have a Principal.  I.e., a no-op 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] geode pull request #719: GEODE-3447 Implement client authorization for the n...

Posted by metatype <gi...@git.apache.org>.
Github user metatype commented on a diff in the pull request:

    https://github.com/apache/geode/pull/719#discussion_r135573704
  
    --- Diff: geode-core/src/main/java/org/apache/geode/security/StreamAuthenticator.java ---
    @@ -46,6 +44,14 @@ void receiveMessage(InputStream inputStream, OutputStream outputStream,
       boolean isAuthenticated();
     
       /**
    +   * 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.
    --- End diff --
    
    `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?


---
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] geode pull request #719: GEODE-3447 Implement client authorization for the n...

Posted by bschuchardt <gi...@git.apache.org>.
Github user bschuchardt commented on a diff in the pull request:

    https://github.com/apache/geode/pull/719#discussion_r134600066
  
    --- Diff: geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufSimpleAuthenticator.java ---
    @@ -40,20 +42,28 @@ public void receiveMessage(InputStream inputStream, OutputStream outputStream,
         properties.setProperty("username", authenticationRequest.getUsername());
         properties.setProperty("password", authenticationRequest.getPassword());
     
    +    authorizer = null; // authenticating a new user clears current authorizer
         try {
           Object principal = securityManager.authenticate(properties);
    -      authenticated = principal != null;
    +      if (principal != null) {
    +        authorizer = new ProtobufSimpleAuthorizer(principal, securityManager);
    --- End diff --
    
    The Principal is part of the implementation of this authentication & authorization algorithm and is encapsulated by this 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] geode pull request #719: GEODE-3447 Implement client authorization for the n...

Posted by pivotal-amurmann <gi...@git.apache.org>.
Github user pivotal-amurmann commented on a diff in the pull request:

    https://github.com/apache/geode/pull/719#discussion_r135550580
  
    --- Diff: geode-core/src/main/java/org/apache/geode/security/StreamAuthenticator.java ---
    @@ -46,6 +44,14 @@ void receiveMessage(InputStream inputStream, OutputStream outputStream,
       boolean isAuthenticated();
     
       /**
    +   * 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.
    --- End diff --
    
    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.


---
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.
---

Re: [GitHub] geode issue #719: GEODE-3447 Implement client authorization for the new prot...

Posted by Michael Stolz <ms...@pivotal.io>.
A hash is not guaranteed unique so is not suitable as a security token.

--
Mike Stolz
Principal Engineer, GemFire Product Manager
Mobile: +1-631-835-4771

On Fri, Aug 25, 2017 at 4:49 PM, galen-pivotal <gi...@git.apache.org> wrote:

> Github user galen-pivotal commented on the issue:
>
>     https://github.com/apache/geode/pull/719
>
>     @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.
>
>
> ---
> 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] geode issue #719: GEODE-3447 Implement client authorization for the new prot...

Posted by galen-pivotal <gi...@git.apache.org>.
Github user galen-pivotal commented on the issue:

    https://github.com/apache/geode/pull/719
  
    @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.


---
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] geode pull request #719: GEODE-3447 Implement client authorization for the n...

Posted by bschuchardt <gi...@git.apache.org>.
Github user bschuchardt commented on a diff in the pull request:

    https://github.com/apache/geode/pull/719#discussion_r135575597
  
    --- Diff: geode-core/src/main/java/org/apache/geode/security/StreamAuthenticator.java ---
    @@ -46,6 +44,14 @@ void receiveMessage(InputStream inputStream, OutputStream outputStream,
       boolean isAuthenticated();
     
       /**
    +   * 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.
    --- End diff --
    
    We'll change this to throw AuthenticationRequiredException if authentication has not yet been performed.


---
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] geode pull request #719: GEODE-3447 Implement client authorization for the n...

Posted by bschuchardt <gi...@git.apache.org>.
Github user bschuchardt closed the pull request at:

    https://github.com/apache/geode/pull/719


---
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] geode pull request #719: GEODE-3447 Implement client authorization for the n...

Posted by pivotal-amurmann <gi...@git.apache.org>.
Github user pivotal-amurmann commented on a diff in the pull request:

    https://github.com/apache/geode/pull/719#discussion_r135153564
  
    --- Diff: geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufSimpleAuthenticator.java ---
    @@ -41,20 +43,29 @@ public void receiveMessage(InputStream inputStream, OutputStream outputStream,
         properties.setProperty(ResourceConstants.USER_NAME, authenticationRequest.getUsername());
         properties.setProperty(ResourceConstants.PASSWORD, authenticationRequest.getPassword());
     
    +    authorizer = null; // authenticating a new user clears current authorizer
         try {
           Object principal = securityManager.authenticate(properties);
    -      authenticated = principal != null;
    +      if (principal != null) {
    +        authorizer = new ProtobufSimpleAuthorizer(principal, securityManager);
    +      }
         } catch (AuthenticationFailedException e) {
    -      authenticated = false;
    +      authorizer = null;
         }
     
    -    AuthenticationAPI.SimpleAuthenticationResponse.newBuilder().setAuthenticated(authenticated)
    +    AuthenticationAPI.SimpleAuthenticationResponse.newBuilder().setAuthenticated(isAuthenticated())
             .build().writeDelimitedTo(outputStream);
       }
     
       @Override
       public boolean isAuthenticated() {
    -    return authenticated;
    +    // note: an authorizer is only created if the user has been authenticated
    +    return authorizer != null;
    --- End diff --
    
    this would be nicer as well if `Optional`


---
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] geode pull request #719: GEODE-3447 Implement client authorization for the n...

Posted by galen-pivotal <gi...@git.apache.org>.
Github user galen-pivotal commented on a diff in the pull request:

    https://github.com/apache/geode/pull/719#discussion_r134633950
  
    --- Diff: geode-core/src/main/java/org/apache/geode/security/StreamAuthorizer.java ---
    @@ -0,0 +1,19 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
    + * agreements. See the NOTICE file distributed with this work for additional information regarding
    + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance with the License. You may obtain a
    + * copy of the License at
    + *
    + * http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software distributed under the License
    + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
    + * or implied. See the License for the specific language governing permissions and limitations under
    + * the License.
    + */
    +package org.apache.geode.security;
    +
    +public interface StreamAuthorizer {
    +  boolean authorize(ResourcePermission permissionRequested);
    --- End diff --
    
    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).


---
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] geode issue #719: GEODE-3447 Implement client authorization for the new prot...

Posted by bschuchardt <gi...@git.apache.org>.
Github user bschuchardt commented on the issue:

    https://github.com/apache/geode/pull/719
  
    @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.


---
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] geode pull request #719: GEODE-3447 Implement client authorization for the n...

Posted by pivotal-amurmann <gi...@git.apache.org>.
Github user pivotal-amurmann commented on a diff in the pull request:

    https://github.com/apache/geode/pull/719#discussion_r135153058
  
    --- Diff: geode-core/src/main/java/org/apache/geode/security/StreamAuthenticator.java ---
    @@ -46,6 +44,14 @@ void receiveMessage(InputStream inputStream, OutputStream outputStream,
       boolean isAuthenticated();
     
       /**
    +   * 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.
    --- End diff --
    
    Could we indicate this by using a Optional? That way we also force the consumer to deal with that case.


---
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] geode pull request #719: GEODE-3447 Implement client authorization for the n...

Posted by bschuchardt <gi...@git.apache.org>.
Github user bschuchardt commented on a diff in the pull request:

    https://github.com/apache/geode/pull/719#discussion_r134600589
  
    --- Diff: geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufSimpleAuthenticator.java ---
    @@ -40,20 +42,28 @@ public void receiveMessage(InputStream inputStream, OutputStream outputStream,
         properties.setProperty("username", authenticationRequest.getUsername());
         properties.setProperty("password", authenticationRequest.getPassword());
     
    +    authorizer = null; // authenticating a new user clears current authorizer
         try {
           Object principal = securityManager.authenticate(properties);
    -      authenticated = principal != null;
    +      if (principal != null) {
    +        authorizer = new ProtobufSimpleAuthorizer(principal, securityManager);
    +      }
         } catch (AuthenticationFailedException e) {
    -      authenticated = false;
    +      authorizer = null;
         }
     
    -    AuthenticationAPI.SimpleAuthenticationResponse.newBuilder().setAuthenticated(authenticated)
    +    AuthenticationAPI.SimpleAuthenticationResponse.newBuilder().setAuthenticated(isAuthenticated())
             .build().writeDelimitedTo(outputStream);
       }
     
       @Override
       public boolean isAuthenticated() {
    -    return authenticated;
    +    return authorizer != null;
    --- End diff --
    
    A authorizer IS only constructed BECAUSE something was authenticated.  Therefore this is a valid implementation of isAuthenticated().


---
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] geode issue #719: GEODE-3447 Implement client authorization for the new prot...

Posted by bschuchardt <gi...@git.apache.org>.
Github user bschuchardt commented on the issue:

    https://github.com/apache/geode/pull/719
  
    @metatype After reviewing StreamAuthenticator I get your point.  I think these new interfaces and classes need to be in a different package.


---
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] geode pull request #719: GEODE-3447 Implement client authorization for the n...

Posted by galen-pivotal <gi...@git.apache.org>.
Github user galen-pivotal commented on a diff in the pull request:

    https://github.com/apache/geode/pull/719#discussion_r134635734
  
    --- Diff: geode-protobuf/src/test/java/org/apache/geode/protocol/AuthorizationIntegrationTest.java ---
    @@ -0,0 +1,206 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
    + * agreements. See the NOTICE file distributed with this work for additional information regarding
    + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance with the License. You may obtain a
    + * copy of the License at
    + *
    + * http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software distributed under the License
    + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
    + * or implied. See the License for the specific language governing permissions and limitations under
    + * the License.
    + */
    +package org.apache.geode.protocol;
    +
    +import static org.junit.Assert.assertEquals;
    +import static org.junit.Assert.assertTrue;
    +import static org.mockito.ArgumentMatchers.any;
    +import static org.mockito.ArgumentMatchers.same;
    +import static org.mockito.Mockito.mock;
    +import static org.mockito.Mockito.when;
    +
    +import java.io.IOException;
    +import java.io.InputStream;
    +import java.io.OutputStream;
    +import java.net.Socket;
    +import java.util.Properties;
    +import java.util.concurrent.TimeUnit;
    +
    +import org.awaitility.Awaitility;
    +import org.junit.After;
    +import org.junit.Before;
    +import org.junit.Rule;
    +import org.junit.Test;
    +import org.junit.contrib.java.lang.system.RestoreSystemProperties;
    +import org.junit.experimental.categories.Category;
    +
    +import org.apache.geode.cache.Cache;
    +import org.apache.geode.cache.CacheFactory;
    +import org.apache.geode.cache.server.CacheServer;
    +import org.apache.geode.internal.AvailablePortHelper;
    +import org.apache.geode.management.internal.security.ResourceConstants;
    +import org.apache.geode.protocol.protobuf.AuthenticationAPI;
    +import org.apache.geode.protocol.protobuf.ClientProtocol;
    +import org.apache.geode.protocol.protobuf.ProtobufSerializationService;
    +import org.apache.geode.protocol.protobuf.ProtocolErrorCode;
    +import org.apache.geode.protocol.protobuf.RegionAPI;
    +import org.apache.geode.protocol.protobuf.serializer.ProtobufProtocolSerializer;
    +import org.apache.geode.protocol.protobuf.utilities.ProtobufUtilities;
    +import org.apache.geode.security.ResourcePermission;
    +import org.apache.geode.security.SecurityManager;
    +import org.apache.geode.serialization.registry.exception.CodecAlreadyRegisteredForTypeException;
    +import org.apache.geode.test.junit.categories.IntegrationTest;
    +
    +@Category(IntegrationTest.class)
    +public class AuthorizationIntegrationTest {
    +
    +  private static final String TEST_USERNAME = "bob";
    +  private static final String TEST_PASSWORD = "bobspassword";
    +  public static final String TEST_REGION = "testRegion";
    +
    +  @Rule
    +  public final RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
    +
    +  private Cache cache;
    +  private int cacheServerPort;
    +  private CacheServer cacheServer;
    +  private Socket socket;
    +  private OutputStream outputStream;
    +  private ProtobufSerializationService serializationService;
    +  private InputStream inputStream;
    +  private ProtobufProtocolSerializer protobufProtocolSerializer;
    +  private Object securityPrincipal;
    +  private SecurityManager mockSecurityManager;
    +  private String testRegion;
    +  public static final ResourcePermission READ_PERMISSION =
    +      new ResourcePermission(ResourcePermission.Resource.DATA, ResourcePermission.Operation.READ);
    +  public static final ResourcePermission WRITE_PERMISSION =
    +      new ResourcePermission(ResourcePermission.Resource.DATA, ResourcePermission.Operation.WRITE);
    +
    +  @Before
    +  public void setUp() throws IOException, CodecAlreadyRegisteredForTypeException {
    +    Properties expectedAuthProperties = new Properties();
    +    expectedAuthProperties.setProperty(ResourceConstants.USER_NAME, TEST_USERNAME);
    +    expectedAuthProperties.setProperty(ResourceConstants.PASSWORD, TEST_PASSWORD);
    +
    +    securityPrincipal = new Object();
    +    mockSecurityManager = mock(SecurityManager.class);
    +    when(mockSecurityManager.authenticate(expectedAuthProperties)).thenReturn(securityPrincipal);
    +
    +    Properties properties = new Properties();
    +    CacheFactory cacheFactory = new CacheFactory(properties);
    +    cacheFactory.set("mcast-port", "0"); // sometimes it isn't due to other tests.
    +
    +    cacheFactory.setSecurityManager(mockSecurityManager);
    +    cache = cacheFactory.create();
    +
    +    cacheServer = cache.addCacheServer();
    +    cacheServerPort = AvailablePortHelper.getRandomAvailableTCPPort();
    +    cacheServer.setPort(cacheServerPort);
    +    cacheServer.start();
    +
    +    cache.createRegionFactory().create(TEST_REGION);
    +
    +    System.setProperty("geode.feature-protobuf-protocol", "true");
    +    System.setProperty("geode.protocol-authentication-mode", "SIMPLE");
    +    socket = new Socket("localhost", cacheServerPort);
    +
    +    Awaitility.await().atMost(5, TimeUnit.SECONDS).until(socket::isConnected);
    +    outputStream = socket.getOutputStream();
    +    inputStream = socket.getInputStream();
    +    outputStream.write(110);
    +
    +    serializationService = new ProtobufSerializationService();
    +    protobufProtocolSerializer = new ProtobufProtocolSerializer();
    +
    +    when(mockSecurityManager.authorize(same(securityPrincipal), any())).thenReturn(false);
    +    AuthenticationAPI.SimpleAuthenticationRequest authenticationRequest =
    +        AuthenticationAPI.SimpleAuthenticationRequest.newBuilder().setUsername(TEST_USERNAME)
    +            .setPassword(TEST_PASSWORD).build();
    +    authenticationRequest.writeDelimitedTo(outputStream);
    +
    +    AuthenticationAPI.SimpleAuthenticationResponse authenticationResponse =
    +        AuthenticationAPI.SimpleAuthenticationResponse.parseDelimitedFrom(inputStream);
    +    assertTrue(authenticationResponse.getAuthenticated());
    +  }
    +
    +  @After
    +  public void shutDown() throws IOException {
    +    cache.close();
    +    socket.close();
    +  }
    +
    +
    +  @Test
    +  public void validateNoPermissions() throws Exception {
    +    when(mockSecurityManager.authorize(securityPrincipal, READ_PERMISSION)).thenReturn(false);
    +    when(mockSecurityManager.authorize(securityPrincipal, WRITE_PERMISSION)).thenReturn(false);
    +
    +    verifyOperations(false, false);
    +  }
    +
    +  @Test
    +  public void validateWritePermission() throws Exception {
    +    when(mockSecurityManager.authorize(securityPrincipal, READ_PERMISSION)).thenReturn(false);
    +    when(mockSecurityManager.authorize(securityPrincipal, WRITE_PERMISSION)).thenReturn(true);
    +
    +    verifyOperations(false, true);
    +  }
    +
    +  @Test
    +  public void validateReadPermission() throws Exception {
    +    when(mockSecurityManager.authorize(securityPrincipal, READ_PERMISSION)).thenReturn(true);
    +    when(mockSecurityManager.authorize(securityPrincipal, WRITE_PERMISSION)).thenReturn(false);
    +
    +    verifyOperations(true, false);
    +  }
    +
    +  @Test
    +  public void validateReadAndWritePermission() throws Exception {
    +    when(mockSecurityManager.authorize(securityPrincipal, READ_PERMISSION)).thenReturn(true);
    +    when(mockSecurityManager.authorize(securityPrincipal, WRITE_PERMISSION)).thenReturn(true);
    +
    +    verifyOperations(true, true);
    +  }
    +
    +  private void verifyOperations(boolean readAllowed, boolean writeAllowed) throws Exception {
    --- End diff --
    
    +1 thorough and clear tests.


---
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] geode pull request #719: GEODE-3447 Implement client authorization for the n...

Posted by kohlmu-pivotal <gi...@git.apache.org>.
Github user kohlmu-pivotal commented on a diff in the pull request:

    https://github.com/apache/geode/pull/719#discussion_r134025383
  
    --- Diff: geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufSimpleAuthorizer.java ---
    @@ -0,0 +1,42 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
    + * agreements. See the NOTICE file distributed with this work for additional information regarding
    + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance with the License. You may obtain a
    + * copy of the License at
    + *
    + * http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software distributed under the License
    + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
    + * or implied. See the License for the specific language governing permissions and limitations under
    + * the License.
    + */
    +package org.apache.geode.protocol.protobuf;
    +
    +import java.io.EOFException;
    +import java.io.IOException;
    +import java.io.InputStream;
    +import java.io.OutputStream;
    +import java.util.Properties;
    +
    +import org.apache.geode.security.AuthenticationFailedException;
    +import org.apache.geode.security.ResourcePermission;
    +import org.apache.geode.security.SecurityManager;
    +import org.apache.geode.security.StreamAuthenticator;
    +import org.apache.geode.security.StreamAuthorizer;
    +
    +public class ProtobufSimpleAuthorizer implements StreamAuthorizer {
    +  private final Object authenticatedPrincipal;
    +  private final SecurityManager securityManager;
    +
    +  public ProtobufSimpleAuthorizer(Object authenticatedPrincipal, SecurityManager securityManager) {
    +    this.authenticatedPrincipal = authenticatedPrincipal;
    +    this.securityManager = securityManager;
    +  }
    +
    +  @Override
    +  public boolean authorize(ResourcePermission permissionRequested) {
    --- End diff --
    
    why would I not pass in the Principal and requested permissions? Then I can have 1 authorizer and not 1 per user.


---
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] geode pull request #719: GEODE-3447 Implement client authorization for the n...

Posted by bschuchardt <gi...@git.apache.org>.
GitHub user bschuchardt reopened a pull request:

    https://github.com/apache/geode/pull/719

    GEODE-3447 Implement client authorization for the new protocol

    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?
    
    - [x] 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](http://www.apache.org/legal/resolved.html#category-a)?
    
    ### 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.


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/apache/geode feature/GEODE-3447

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/geode/pull/719.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 #719
    
----
commit a2b678aa06af73d218da6e9ae8d95f2942d23a7b
Author: Bruce Schuchardt <bs...@pivotal.io>
Date:   2017-08-17T22:22:57Z

    GEODE-3447 Implement client authorization for the new protocol
    
    Implementation of authorization checks for the new protocol.

commit 495d788abd1935dea27b00ccc16e822eaf690b38
Author: Bruce Schuchardt <bs...@pivotal.io>
Date:   2017-08-22T21:34:36Z

    addressing review comments

commit ea1d8c1f9540d86751a8ce22732bd6b42c0ae4ae
Author: Bruce Schuchardt <bs...@pivotal.io>
Date:   2017-08-22T22:34:21Z

    rebase to origin/develop

----


---
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] geode pull request #719: GEODE-3447 Implement client authorization for the n...

Posted by pivotal-amurmann <gi...@git.apache.org>.
Github user pivotal-amurmann commented on a diff in the pull request:

    https://github.com/apache/geode/pull/719#discussion_r135154701
  
    --- Diff: geode-protobuf/src/test/java/org/apache/geode/protocol/AuthorizationIntegrationTest.java ---
    @@ -0,0 +1,206 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
    + * agreements. See the NOTICE file distributed with this work for additional information regarding
    + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance with the License. You may obtain a
    + * copy of the License at
    + *
    + * http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software distributed under the License
    + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
    + * or implied. See the License for the specific language governing permissions and limitations under
    + * the License.
    + */
    +package org.apache.geode.protocol;
    +
    +import static org.junit.Assert.assertEquals;
    +import static org.junit.Assert.assertTrue;
    +import static org.mockito.ArgumentMatchers.any;
    +import static org.mockito.ArgumentMatchers.same;
    +import static org.mockito.Mockito.mock;
    +import static org.mockito.Mockito.when;
    +
    +import java.io.IOException;
    +import java.io.InputStream;
    +import java.io.OutputStream;
    +import java.net.Socket;
    +import java.util.Properties;
    +import java.util.concurrent.TimeUnit;
    +
    +import org.awaitility.Awaitility;
    +import org.junit.After;
    +import org.junit.Before;
    +import org.junit.Rule;
    +import org.junit.Test;
    +import org.junit.contrib.java.lang.system.RestoreSystemProperties;
    +import org.junit.experimental.categories.Category;
    +
    +import org.apache.geode.cache.Cache;
    +import org.apache.geode.cache.CacheFactory;
    +import org.apache.geode.cache.server.CacheServer;
    +import org.apache.geode.internal.AvailablePortHelper;
    +import org.apache.geode.management.internal.security.ResourceConstants;
    +import org.apache.geode.protocol.protobuf.AuthenticationAPI;
    +import org.apache.geode.protocol.protobuf.ClientProtocol;
    +import org.apache.geode.protocol.protobuf.ProtobufSerializationService;
    +import org.apache.geode.protocol.protobuf.ProtocolErrorCode;
    +import org.apache.geode.protocol.protobuf.RegionAPI;
    +import org.apache.geode.protocol.protobuf.serializer.ProtobufProtocolSerializer;
    +import org.apache.geode.protocol.protobuf.utilities.ProtobufUtilities;
    +import org.apache.geode.security.ResourcePermission;
    +import org.apache.geode.security.SecurityManager;
    +import org.apache.geode.serialization.registry.exception.CodecAlreadyRegisteredForTypeException;
    +import org.apache.geode.test.junit.categories.IntegrationTest;
    +
    +@Category(IntegrationTest.class)
    +public class AuthorizationIntegrationTest {
    --- End diff --
    
    Hugs for not adding this to the existing Roundtrip test! 🤗


---
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] geode pull request #719: GEODE-3447 Implement client authorization for the n...

Posted by pivotal-amurmann <gi...@git.apache.org>.
Github user pivotal-amurmann commented on a diff in the pull request:

    https://github.com/apache/geode/pull/719#discussion_r135153161
  
    --- Diff: geode-core/src/main/java/org/apache/geode/security/StreamAuthorizer.java ---
    @@ -0,0 +1,19 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
    + * agreements. See the NOTICE file distributed with this work for additional information regarding
    + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance with the License. You may obtain a
    + * copy of the License at
    + *
    + * http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software distributed under the License
    + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
    + * or implied. See the License for the specific language governing permissions and limitations under
    + * the License.
    + */
    +package org.apache.geode.security;
    +
    +public interface StreamAuthorizer {
    --- End diff --
    
    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.


---
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] geode pull request #719: GEODE-3447 Implement client authorization for the n...

Posted by galen-pivotal <gi...@git.apache.org>.
Github user galen-pivotal commented on a diff in the pull request:

    https://github.com/apache/geode/pull/719#discussion_r134634705
  
    --- Diff: geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/registry/OperationContextRegistry.java ---
    @@ -47,41 +48,57 @@ private void addContexts() {
         operationContexts.put(RequestAPICase.GETREQUEST,
             new OperationContext<>(ClientProtocol.Request::getGetRequest,
                 new GetRequestOperationHandler(),
    -            opsResp -> ClientProtocol.Response.newBuilder().setGetResponse(opsResp)));
    +            opsResp -> ClientProtocol.Response.newBuilder().setGetResponse(opsResp),
    +            new ResourcePermission(ResourcePermission.Resource.DATA,
    +                ResourcePermission.Operation.READ)));
     
         operationContexts.put(RequestAPICase.GETALLREQUEST,
             new OperationContext<>(ClientProtocol.Request::getGetAllRequest,
                 new GetAllRequestOperationHandler(),
    -            opsResp -> ClientProtocol.Response.newBuilder().setGetAllResponse(opsResp)));
    +            opsResp -> ClientProtocol.Response.newBuilder().setGetAllResponse(opsResp),
    +            new ResourcePermission(ResourcePermission.Resource.DATA,
    +                ResourcePermission.Operation.READ)));
     
         operationContexts.put(RequestAPICase.PUTREQUEST,
             new OperationContext<>(ClientProtocol.Request::getPutRequest,
                 new PutRequestOperationHandler(),
    -            opsResp -> ClientProtocol.Response.newBuilder().setPutResponse(opsResp)));
    +            opsResp -> ClientProtocol.Response.newBuilder().setPutResponse(opsResp),
    +            new ResourcePermission(ResourcePermission.Resource.DATA,
    +                ResourcePermission.Operation.WRITE)));
     
         operationContexts.put(RequestAPICase.PUTALLREQUEST,
             new OperationContext<>(ClientProtocol.Request::getPutAllRequest,
                 new PutAllRequestOperationHandler(),
    -            opsResp -> ClientProtocol.Response.newBuilder().setPutAllResponse(opsResp)));
    +            opsResp -> ClientProtocol.Response.newBuilder().setPutAllResponse(opsResp),
    +            new ResourcePermission(ResourcePermission.Resource.DATA,
    +                ResourcePermission.Operation.WRITE)));
     
         operationContexts.put(RequestAPICase.REMOVEREQUEST,
             new OperationContext<>(ClientProtocol.Request::getRemoveRequest,
                 new RemoveRequestOperationHandler(),
    -            opsResp -> ClientProtocol.Response.newBuilder().setRemoveResponse(opsResp)));
    +            opsResp -> ClientProtocol.Response.newBuilder().setRemoveResponse(opsResp),
    +            new ResourcePermission(ResourcePermission.Resource.DATA,
    +                ResourcePermission.Operation.WRITE)));
     
         operationContexts.put(RequestAPICase.GETREGIONNAMESREQUEST,
             new OperationContext<>(ClientProtocol.Request::getGetRegionNamesRequest,
                 new GetRegionNamesRequestOperationHandler(),
    -            opsResp -> ClientProtocol.Response.newBuilder().setGetRegionNamesResponse(opsResp)));
    +            opsResp -> ClientProtocol.Response.newBuilder().setGetRegionNamesResponse(opsResp),
    +            new ResourcePermission(ResourcePermission.Resource.DATA,
    +                ResourcePermission.Operation.READ)));
     
         operationContexts.put(RequestAPICase.GETREGIONREQUEST,
             new OperationContext<>(ClientProtocol.Request::getGetRegionRequest,
                 new GetRegionRequestOperationHandler(),
    -            opsResp -> ClientProtocol.Response.newBuilder().setGetRegionResponse(opsResp)));
    +            opsResp -> ClientProtocol.Response.newBuilder().setGetRegionResponse(opsResp),
    +            new ResourcePermission(ResourcePermission.Resource.DATA,
    +                ResourcePermission.Operation.READ)));
     
    -    operationContexts.put(RequestAPICase.GETAVAILABLESERVERSREQUEST, new OperationContext<>(
    -        ClientProtocol.Request::getGetAvailableServersRequest,
    -        new GetAvailableServersOperationHandler(),
    -        opsResp -> ClientProtocol.Response.newBuilder().setGetAvailableServersResponse(opsResp)));
    +    operationContexts.put(RequestAPICase.GETAVAILABLESERVERSREQUEST,
    --- End diff --
    
    This is one that should be authorized for either read or write... is read a subset of write? I don't know.


---
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] geode pull request #719: GEODE-3447 Implement client authorization for the n...

Posted by pivotal-amurmann <gi...@git.apache.org>.
Github user pivotal-amurmann commented on a diff in the pull request:

    https://github.com/apache/geode/pull/719#discussion_r135153452
  
    --- Diff: geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufOpsProcessor.java ---
    @@ -44,9 +46,15 @@ public ProtobufOpsProcessor(SerializationService serializationService,
         ClientProtocol.Response.Builder builder;
         Result result;
         try {
    -      result = operationContext.getOperationHandler().process(serializationService,
    -          operationContext.getFromRequest().apply(request), context);
    -    } catch (InvalidExecutionContextException e) {
    +      if (context.getAuthorizer().authorize(operationContext.getAccessPermissionRequired())) {
    +        result = operationContext.getOperationHandler().process(serializationService,
    +            operationContext.getFromRequest().apply(request), context);
    +      } else {
    --- End diff --
    
    We might want to log this. Logging all security related events can be a life saver during or after a security incident.


---
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] geode pull request #719: GEODE-3447 Implement client authorization for the n...

Posted by kohlmu-pivotal <gi...@git.apache.org>.
Github user kohlmu-pivotal commented on a diff in the pull request:

    https://github.com/apache/geode/pull/719#discussion_r134051130
  
    --- Diff: geode-protobuf/src/main/java/org/apache/geode/protocol/protobuf/ProtobufSimpleAuthenticator.java ---
    @@ -40,20 +42,28 @@ public void receiveMessage(InputStream inputStream, OutputStream outputStream,
         properties.setProperty("username", authenticationRequest.getUsername());
         properties.setProperty("password", authenticationRequest.getPassword());
     
    +    authorizer = null; // authenticating a new user clears current authorizer
         try {
           Object principal = securityManager.authenticate(properties);
    -      authenticated = principal != null;
    +      if (principal != null) {
    +        authorizer = new ProtobufSimpleAuthorizer(principal, securityManager);
    --- End diff --
    
    Why do we need a `ProtobufSimpleAuthorizer` per principal? Could we not have a single `Authorizer` that will authenticate all principals?


---
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] geode issue #719: GEODE-3447 Implement client authorization for the new prot...

Posted by metatype <gi...@git.apache.org>.
Github user metatype commented on the issue:

    https://github.com/apache/geode/pull/719
  
    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?



---
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] geode issue #719: GEODE-3447 Implement client authorization for the new prot...

Posted by bschuchardt <gi...@git.apache.org>.
Github user bschuchardt commented on the issue:

    https://github.com/apache/geode/pull/719
  
    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.


---
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] geode pull request #719: GEODE-3447 Implement client authorization for the n...

Posted by bschuchardt <gi...@git.apache.org>.
Github user bschuchardt closed the pull request at:

    https://github.com/apache/geode/pull/719


---
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] geode pull request #719: GEODE-3447 Implement client authorization for the n...

Posted by galen-pivotal <gi...@git.apache.org>.
Github user galen-pivotal commented on a diff in the pull request:

    https://github.com/apache/geode/pull/719#discussion_r135158491
  
    --- Diff: geode-core/src/main/java/org/apache/geode/security/StreamAuthenticator.java ---
    @@ -46,6 +44,14 @@ void receiveMessage(InputStream inputStream, OutputStream outputStream,
       boolean isAuthenticated();
     
       /**
    +   * 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.
    --- End diff --
    
    I kind of prefer a comment or even annotation saying nullable -- optionals themselves can be null, and have some cost.


---
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.
---