You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by WireBaron <gi...@git.apache.org> on 2017/08/10 18:19:56 UTC
[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...
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 <go...@pivotal.io>
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 <br...@pivotal.io>
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 <go...@pivotal.io>
----
---
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 #707: GEODE-3412: Add simple authentication flow to proto...
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/707#discussion_r132878110
--- 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 --
That's a reasonable choice. Do you think this should be another undocumented system property? It might be fine for an alpha, but I don't think we should do it for long after that.
---
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 #707: GEODE-3412: Add simple authentication flow to proto...
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/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<? extends StreamAuthenticator> findStreamAuthenticator(
+ String implementationID) {
+ if (authenticatorClass != null) {
+ return authenticatorClass;
+ }
+
+ synchronized (streamAuthenticatorLoadLock) {
+ if (authenticatorClass != null) {
+ return authenticatorClass;
+ }
+
+ ServiceLoader<StreamAuthenticator> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...
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/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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...
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/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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...
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/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<? extends StreamAuthenticator> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...
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/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.
+ *
+ * </p>
+ * <U>Description</U>: Specifies the authentication mode used by the geode-protobuf module.
+ * </p>
+ * <U>Default</U>: "NOOP"
+ * </p>
+ * <U>Allowed values</U>: "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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...
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/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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...
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/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.
+ *
+ * </p>
+ * <U>Description</U>: Specifies the authentication mode used by the geode-protobuf module.
+ * </p>
+ * <U>Default</U>: "NOOP"
+ * </p>
+ * <U>Allowed values</U>: "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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...
Posted by asfgit <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...
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/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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...
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/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<String, Class<? extends StreamAuthenticator>> authenticators = null;
- private static ClientProtocolMessageHandler findClientProtocolMessageHandler() {
+ public ServerConnectionFactory() {}
+
+ private synchronized void initializeAuthenticatorsMap() {
+ if (authenticators != null) {
+ return;
+ }
+ authenticators = new HashMap<>();
+ ServiceLoader<StreamAuthenticator> 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<ClientProtocolMessageHandler> loader =
+ ServiceLoader.load(ClientProtocolMessageHandler.class);
+ Iterator<ClientProtocolMessageHandler> iterator = loader.iterator();
- synchronized (protocolLoadLock) {
- if (protobufProtocolHandler != null) {
- return protobufProtocolHandler;
- }
-
- ServiceLoader<ClientProtocolMessageHandler> loader =
- ServiceLoader.load(ClientProtocolMessageHandler.class);
- Iterator<ClientProtocolMessageHandler> 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<? extends StreamAuthenticator> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode pull request #707: GEODE-3412: Add simple authentication flow to proto...
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/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<? extends StreamAuthenticator> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---