[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...

[asfgit]

Github user asfgit closed the pull request at:

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


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...

[pivotal-amurmann]

Github user pivotal-amurmann commented on a diff in the pull request:

https://github.com/apache/geode/pull/707#discussion_r133246611
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactory.java
 ---
@@ -22,59 +22,89 @@
 
 import java.io.IOException;
 import java.net.Socket;
+import java.util.HashMap;
 import java.util.Iterator;
+import java.util.Map;
 import java.util.ServiceLoader;
-import javax.management.ServiceNotFoundException;
 
 /**
  * Creates instances of ServerConnection based on the connection mode 
provided.
  */
 public class ServerConnectionFactory {
-  private static ClientProtocolMessageHandler protobufProtocolHandler;
-  private static final Object protocolLoadLock = new Object();
+  private ClientProtocolMessageHandler protobufProtocolHandler;
+  private Map authenticators 
= null;
 
-  private static ClientProtocolMessageHandler 
findClientProtocolMessageHandler() {
+  public ServerConnectionFactory() {}
+
+  private synchronized void initializeAuthenticatorsMap() {
+if (authenticators != null) {
+  return;
+}
+authenticators = new HashMap<>();
+ServiceLoader loader = 
ServiceLoader.load(StreamAuthenticator.class);
+for (StreamAuthenticator streamAuthenticator : loader) {
+  authenticators.put(streamAuthenticator.implementationID(), 
streamAuthenticator.getClass());
+}
+  }
+
+  private synchronized ClientProtocolMessageHandler 
initializeMessageHandler() {
 if (protobufProtocolHandler != null) {
   return protobufProtocolHandler;
 }
+ServiceLoader loader =
+ServiceLoader.load(ClientProtocolMessageHandler.class);
+Iterator iterator = loader.iterator();
 
-synchronized (protocolLoadLock) {
-  if (protobufProtocolHandler != null) {
-return protobufProtocolHandler;
-  }
-
-  ServiceLoader loader =
-  ServiceLoader.load(ClientProtocolMessageHandler.class);
-  Iterator iterator = loader.iterator();
-
-  if (!iterator.hasNext()) {
-throw new ServiceLoadingFailureException(
-"ClientProtocolMessageHandler implementation not found in 
JVM");
-  }
+if (!iterator.hasNext()) {
+  throw new ServiceLoadingFailureException(
+  "There is no ClientProtocolMessageHandler implementation found 
in JVM");
+}
 
-  ClientProtocolMessageHandler returnValue = iterator.next();
+protobufProtocolHandler = iterator.next();
+return protobufProtocolHandler;
+  }
 
-  if (iterator.hasNext()) {
+  private StreamAuthenticator findStreamAuthenticator(String 
implementationID) {
+if (authenticators == null) {
+  initializeAuthenticatorsMap();
+}
+Class streamAuthenticatorClass =
+authenticators.get(implementationID);
+if (streamAuthenticatorClass == null) {
+  throw new ServiceLoadingFailureException(
+  "Could not find implementation for StreamAuthenticator with 
implementation ID "
+  + implementationID);
+} else {
+  try {
+return streamAuthenticatorClass.newInstance();
+  } catch (InstantiationException | IllegalAccessException e) {
 throw new ServiceLoadingFailureException(
-"Multiple service implementations found for 
ClientProtocolMessageHandler");
--- End diff --

Where are we now handling the case of having multiple implementations?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...

[galen-pivotal]

Github user galen-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/707#discussion_r132878512
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactory.java
 ---
@@ -72,9 +99,15 @@ public static ServerConnection 
makeServerConnection(Socket s, InternalCache c,
 throw new IOException("Acceptor received unknown communication 
mode: " + communicationMode);
   } else {
 protobufProtocolHandler = findClientProtocolMessageHandler();
-return new GenericProtocolServerConnection(s, c, helper, stats, 
hsTimeout, socketBufferSize,
-communicationModeStr, communicationMode, acceptor, 
protobufProtocolHandler,
-securityService);
+authenticatorClass = findStreamAuthenticator(
+
c.getInternalDistributedSystem().getConfig().getProtobufProtocolAuthenticationMode());
+try {
+  return new GenericProtocolServerConnection(s, c, helper, stats, 
hsTimeout,
+  socketBufferSize, communicationModeStr, communicationMode, 
acceptor,
+  protobufProtocolHandler, securityService, 
authenticatorClass.newInstance());
--- End diff --

We find the class exactly once, and create a new instance per protocol 
instance. We then create an instance per `GenericProtocolServerConnection`. 
This is because the authenticator (unlike the protocol handler) is stateful, so 
we can't share it between connections.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...

[galen-pivotal]

Github user galen-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/707#discussion_r132878378
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactory.java
 ---
@@ -63,6 +65,31 @@ private static ClientProtocolMessageHandler 
findClientProtocolMessageHandler() {
 }
   }
 
+  private static Class 
findStreamAuthenticator(
+  String implementationID) {
+if (authenticatorClass != null) {
+  return authenticatorClass;
+}
+
+synchronized (streamAuthenticatorLoadLock) {
--- End diff --

Once `authenticatorClass` is initialized, there will no longer be any 
synchronization necessary in this method. This means that each thread will need 
to synchronize at most once, which means no locking / cache misses.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...

[galen-pivotal]

Github user galen-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/707#discussion_r132877993
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java
 ---
@@ -1378,6 +1379,18 @@
*/
   String NAME = "name";
   /**
+   * The authentication mode for the protobuf client-server protocol.
+   *
+   * 
+   * Description: Specifies the authentication mode used by the 
geode-protobuf module.
+   * 
+   * Default: "NOOP"
+   * 
+   * Allowed values: "NOOP" "SIMPLE"
+   */
+  @Experimental
+  String PROTOBUF_PROTOCOL_AUTHENTICATION_MODE = 
"protobuf-protocol-authentication-mode";
--- End diff --

The property is a String representing the authentication mode to be used by 
the protobuf protocol. It currently has two valid settings. The protobuf in the 
name refers to the new client protocol (which we're calling the protobuf 
protocol).

I don't think we're quite ready to take the feature flag out yet, but we 
should consider it as this feature nears alpha. Or moving the feature flag to a 
better config mechanism (DistributionConfig), so that this property and that 
one are in approximately the same place.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...

[kohlmu-pivotal]

Github user kohlmu-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/707#discussion_r132735527
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactory.java
 ---
@@ -63,6 +65,31 @@ private static ClientProtocolMessageHandler 
findClientProtocolMessageHandler() {
 }
   }
 
+  private static Class 
findStreamAuthenticator(
+  String implementationID) {
+if (authenticatorClass != null) {
+  return authenticatorClass;
+}
+
+synchronized (streamAuthenticatorLoadLock) {
+  if (authenticatorClass != null) {
+return authenticatorClass;
+  }
+
+  ServiceLoader loader = 
ServiceLoader.load(StreamAuthenticator.class);
+
+  for (StreamAuthenticator classInstance : loader) {
+if (implementationID.equals(classInstance.implementationID())) {
+  return classInstance.getClass();
--- End diff --

Why do we get an instance of the StreamAuthenticator, just to call 
`getClass` on it, and then later we create a new instance of class from this 
instance. 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...

[kohlmu-pivotal]

Github user kohlmu-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/707#discussion_r132735728
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactory.java
 ---
@@ -72,9 +99,15 @@ public static ServerConnection 
makeServerConnection(Socket s, InternalCache c,
 throw new IOException("Acceptor received unknown communication 
mode: " + communicationMode);
   } else {
 protobufProtocolHandler = findClientProtocolMessageHandler();
-return new GenericProtocolServerConnection(s, c, helper, stats, 
hsTimeout, socketBufferSize,
-communicationModeStr, communicationMode, acceptor, 
protobufProtocolHandler,
-securityService);
+authenticatorClass = findStreamAuthenticator(
+
c.getInternalDistributedSystem().getConfig().getProtobufProtocolAuthenticationMode());
+try {
+  return new GenericProtocolServerConnection(s, c, helper, stats, 
hsTimeout,
+  socketBufferSize, communicationModeStr, communicationMode, 
acceptor,
+  protobufProtocolHandler, securityService, 
authenticatorClass.newInstance());
--- End diff --

We should use the authenticator instance returned from the 
`findClientProtocolMessageHandler`.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...

[kohlmu-pivotal]

Github user kohlmu-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/707#discussion_r132737475
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfig.java
 ---
@@ -2433,6 +2433,25 @@
   String getRedundancyZone();
 
   /**
+   * @since Geode 1.??? TODO FIXME
+   */
+  @ConfigAttribute(type = String.class)
+  String PROTOBUF_PROTOCOL_AUTHENTICATION_MODE_NAME = 
PROTOBUF_PROTOCOL_AUTHENTICATION_MODE;
+  String DEFAULT_PROTOBUF_PROTOCOL_AUTHENTICATION_MODE = "NOOP";
+
+  /**
+   * @since Geode 1.??? TODO FIXME
+   */
+  @ConfigAttributeSetter(name = PROTOBUF_PROTOCOL_AUTHENTICATION_MODE)
+  void setProtobufProtocolAuthenticationMode(String authenticationMode);
+
+  /**
+   * @since Geode 1.??? TODO FIXME
+   */
+  @ConfigAttributeGetter(name = PROTOBUF_PROTOCOL_AUTHENTICATION_MODE)
+  String getProtobufProtocolAuthenticationMode();
+
+  /**
--- End diff --

Without an official release version I don't think we should have any of the 
properties/configurations public until release.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...

[kohlmu-pivotal]

Github user kohlmu-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/707#discussion_r132737182
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java
 ---
@@ -1378,6 +1379,18 @@
*/
   String NAME = "name";
   /**
+   * The authentication mode for the protobuf client-server protocol.
+   *
+   * 
+   * Description: Specifies the authentication mode used by the 
geode-protobuf module.
+   * 
+   * Default: "NOOP"
+   * 
+   * Allowed values: "NOOP" "SIMPLE"
+   */
+  @Experimental
+  String PROTOBUF_PROTOCOL_AUTHENTICATION_MODE = 
"protobuf-protocol-authentication-mode";
--- End diff --

This property is misleading. It is NOT a protobuf specific authentication 
mode. It is merely an authentication mechanism that uses protobuf underneath 
the covers.
1) A different property name is to be used
2) With this property, the feature toggle should also maybe be removed??!!? 
One cannot live without the other


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...

[kohlmu-pivotal]

Github user kohlmu-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/707#discussion_r132738771
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnectionFactory.java
 ---
@@ -63,6 +65,31 @@ private static ClientProtocolMessageHandler 
findClientProtocolMessageHandler() {
 }
   }
 
+  private static Class 
findStreamAuthenticator(
+  String implementationID) {
+if (authenticatorClass != null) {
+  return authenticatorClass;
+}
+
+synchronized (streamAuthenticatorLoadLock) {
--- End diff --

why do we prefer this approach over a synchronized method?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...

[kohlmu-pivotal]

Github user kohlmu-pivotal commented on a diff in the pull request:

https://github.com/apache/geode/pull/707#discussion_r132738100
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/NoOpStreamAuthenticator.java
 ---
@@ -0,0 +1,43 @@
+/*
+ * 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.internal.cache.tier.sockets;
--- End diff --

I'm not sure this class should live in this package. Is it not more related 
to security?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...

[WireBaron]

GitHub user WireBaron opened a pull request:

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

GEODE-3412: Add simple authentication flow to protobuf protocol.

@pivotal-amurmann @galen-pivotal @kohlmu-pivotal @bschuchardt @hiteshk25 

This change adds a simple username/password validation to the protobuf 
protocol.
It also adds a new configuration parameter to specify the type of 
authentication required.

Signed-off-by: Galen O'Sullivan 

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?

- [ ] 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?

- [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/WireBaron/geode feature/GEODE-3412

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

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


commit daa140875c4f65838dde4ae0f627a9551bea7516
Author: Brian Rowe 
Date:   2017-08-10T18:16:25Z

GEODE-3412: Add simple authentication flow to protobuf protocol.

This change adds a simple username/password validation to the protobuf 
protocol.
It also adds a new configuration parameter to specify the type of 
authentication required.

Signed-off-by: Galen O'Sullivan 




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---