Repository: geode
Updated Branches:
  refs/heads/feature/GEODE-3249 59b14dbc5 -> 41e14ddb3


GEODE-3249: internal messages should have credentials

added a system property to allow old clients to not send credentials and
a unit test of this change


Project: http://git-wip-us.apache.org/repos/asf/geode/repo
Commit: http://git-wip-us.apache.org/repos/asf/geode/commit/41e14ddb
Tree: http://git-wip-us.apache.org/repos/asf/geode/tree/41e14ddb
Diff: http://git-wip-us.apache.org/repos/asf/geode/diff/41e14ddb

Branch: refs/heads/feature/GEODE-3249
Commit: 41e14ddb3c8269556f4ef13951b6708c8d23acc5
Parents: 59b14db
Author: Bruce Schuchardt <bschucha...@pivotal.io>
Authored: Wed Aug 9 15:40:23 2017 -0700
Committer: Bruce Schuchardt <bschucha...@pivotal.io>
Committed: Wed Aug 9 15:40:23 2017 -0700

----------------------------------------------------------------------
 .../cache/tier/sockets/ServerConnection.java    | 92 ++++++++++++--------
 .../ClientAuthenticationPart2DUnitTest.java     | 26 ++++++
 2 files changed, 83 insertions(+), 35 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/geode/blob/41e14ddb/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java
----------------------------------------------------------------------
diff --git 
a/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java
 
b/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java
index 91712e0..d25722b 100644
--- 
a/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java
+++ 
b/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java
@@ -84,6 +84,17 @@ public abstract class ServerConnection implements Runnable {
    */
   private static final int TIMEOUT_BUFFER_FOR_CONNECTION_CLEANUP_MS = 5000;
 
+  public static final String ALLOW_INTERNAL_MESSAGES_WITHOUT_CREDENTIALS_NAME =
+      "geode.allow-internal-messages-without-credentials";
+
+  /**
+   * This property allows folks to perform a rolling upgrade from pre-1.2.1 to
+   * a post-1.2.1 cluster.  Normally internal messages that can affect server 
state
+   * require credentials but pre-1.2.1 this wasn't the case.  See GEODE-3249
+   */
+  private static final boolean ALLOW_INTERNAL_MESSAGES_WITHOUT_CREDENTIALS =
+      Boolean.getBoolean(ALLOW_INTERNAL_MESSAGES_WITHOUT_CREDENTIALS_NAME);
+
   private Map commands;
 
   private final SecurityService securityService;
@@ -764,7 +775,8 @@ public abstract class ServerConnection implements Runnable {
 
         // if a subject exists for this uniqueId, binds the subject to this 
thread so that we can do
         // authorization later
-        if (AcceptorImpl.isIntegratedSecurity() && !isInternalMessage()
+        if (AcceptorImpl.isIntegratedSecurity() && !isInternalMessage(
+            this.requestMsg, ALLOW_INTERNAL_MESSAGES_WITHOUT_CREDENTIALS)
             && this.communicationMode != Acceptor.GATEWAY_TO_GATEWAY) {
           long uniqueId = getUniqueId();
           Subject subject = this.clientUserAuths.getSubject(uniqueId);
@@ -1068,7 +1080,8 @@ public abstract class ServerConnection implements 
Runnable {
     if (AcceptorImpl.isAuthenticationRequired()
         && this.handshake.getVersion().compareTo(Version.GFE_65) >= 0
         && (this.communicationMode != Acceptor.GATEWAY_TO_GATEWAY)
-        && (!this.requestMsg.getAndResetIsMetaRegion()) && 
(!isInternalMessage())) {
+        && (!this.requestMsg.getAndResetIsMetaRegion()) && (!isInternalMessage(
+        this.requestMsg, ALLOW_INTERNAL_MESSAGES_WITHOUT_CREDENTIALS))) {
       setSecurityPart();
       return this.securePart;
     } else {
@@ -1081,37 +1094,47 @@ public abstract class ServerConnection implements 
Runnable {
     return null;
   }
 
-  private boolean isInternalMessage() {
-    return this.requestMsg.messageType == MessageType.PING
-        || this.requestMsg.messageType == MessageType.USER_CREDENTIAL_MESSAGE
-        || this.requestMsg.messageType == MessageType.REQUEST_EVENT_VALUE
-        || this.requestMsg.messageType == MessageType.MAKE_PRIMARY
-        || this.requestMsg.messageType == MessageType.REMOVE_USER_AUTH
-        || this.requestMsg.messageType == MessageType.CLIENT_READY
-        || this.requestMsg.messageType == MessageType.SIZE
-        || this.requestMsg.messageType == MessageType.TX_FAILOVER
-        || this.requestMsg.messageType == MessageType.TX_SYNCHRONIZATION
-        || this.requestMsg.messageType == MessageType.COMMIT
-        || this.requestMsg.messageType == MessageType.ROLLBACK
-        || this.requestMsg.messageType == MessageType.CLOSE_CONNECTION
-        || this.requestMsg.messageType == MessageType.INVALID
-        || this.requestMsg.messageType == MessageType.PERIODIC_ACK
-        || this.requestMsg.messageType == 
MessageType.GET_CLIENT_PARTITION_ATTRIBUTES;
-
-    // || this.requestMsg.messageType == MessageType.GETCQSTATS_MSG_TYPE
-    // || this.requestMsg.messageType == MessageType.GET_CLIENT_PR_METADATA
-    // || this.requestMsg.messageType == MessageType.MONITORCQ_MSG_TYPE
-    // || this.requestMsg.messageType == MessageType.REGISTER_DATASERIALIZERS
-    // || this.requestMsg.messageType == MessageType.REGISTER_INSTANTIATORS
-    // || this.requestMsg.messageType == MessageType.ADD_PDX_TYPE
-    // || this.requestMsg.messageType == MessageType.GET_PDX_ID_FOR_TYPE
-    // || this.requestMsg.messageType == MessageType.GET_PDX_TYPE_BY_ID
-    // || this.requestMsg.messageType == MessageType.GET_FUNCTION_ATTRIBUTES
-    // || this.requestMsg.messageType == MessageType.ADD_PDX_ENUM
-    // || this.requestMsg.messageType == MessageType.GET_PDX_ID_FOR_ENUM
-    // || this.requestMsg.messageType == MessageType.GET_PDX_ENUM_BY_ID
-    // || this.requestMsg.messageType == MessageType.GET_PDX_TYPES
-    // || this.requestMsg.messageType == MessageType.GET_PDX_ENUMS
+  public boolean isInternalMessage(Message message, boolean 
allowOldInternalMessages) {
+    if (message.isSecureMode()) {
+      return false;
+    }
+    int messageType = message.getMessageType();
+    boolean isInternalMessage = messageType == MessageType.PING
+        || messageType == MessageType.USER_CREDENTIAL_MESSAGE
+        || messageType == MessageType.REQUEST_EVENT_VALUE
+        || messageType == MessageType.MAKE_PRIMARY
+        || messageType == MessageType.REMOVE_USER_AUTH
+        || messageType == MessageType.CLIENT_READY
+        || messageType == MessageType.SIZE
+        || messageType == MessageType.TX_FAILOVER
+        || messageType == MessageType.TX_SYNCHRONIZATION
+        || messageType == MessageType.COMMIT
+        || messageType == MessageType.ROLLBACK
+        || messageType == MessageType.CLOSE_CONNECTION
+        || messageType == MessageType.INVALID
+        || messageType == MessageType.PERIODIC_ACK
+        || messageType == MessageType.GET_CLIENT_PARTITION_ATTRIBUTES;
+
+    // we allow older clients to not send credentials for a handful of messages
+    // if and only if a system property is set.  This allows a rolling upgrade
+    // to be performed.
+    if (!isInternalMessage && allowOldInternalMessages) {
+      isInternalMessage = messageType == MessageType.GETCQSTATS_MSG_TYPE
+          || messageType == MessageType.GET_CLIENT_PR_METADATA
+          || messageType == MessageType.MONITORCQ_MSG_TYPE
+          || messageType == MessageType.REGISTER_DATASERIALIZERS
+          || messageType == MessageType.REGISTER_INSTANTIATORS
+          || messageType == MessageType.ADD_PDX_TYPE
+          || messageType == MessageType.GET_PDX_ID_FOR_TYPE
+          || messageType == MessageType.GET_PDX_TYPE_BY_ID
+          || messageType == MessageType.GET_FUNCTION_ATTRIBUTES
+          || messageType == MessageType.ADD_PDX_ENUM
+          || messageType == MessageType.GET_PDX_ID_FOR_ENUM
+          || messageType == MessageType.GET_PDX_ENUM_BY_ID
+          || messageType == MessageType.GET_PDX_TYPES
+          || messageType == MessageType.GET_PDX_ENUMS;
+    }
+    return isInternalMessage;
   }
 
   public void run() {
@@ -1719,8 +1742,7 @@ public abstract class ServerConnection implements 
Runnable {
           (HandShake) this.handshake, this.connectionId);
     } else {
       throw new AuthenticationRequiredException(
-          
LocalizedStrings.HandShake_NO_SECURITY_CREDENTIALS_ARE_PROVIDED.toLocalizedString()
-              + "; for message " + this.requestMsg);
+          
LocalizedStrings.HandShake_NO_SECURITY_CREDENTIALS_ARE_PROVIDED.toLocalizedString());
     }
     return uniqueId;
   }

http://git-wip-us.apache.org/repos/asf/geode/blob/41e14ddb/geode-core/src/test/java/org/apache/geode/security/ClientAuthenticationPart2DUnitTest.java
----------------------------------------------------------------------
diff --git 
a/geode-core/src/test/java/org/apache/geode/security/ClientAuthenticationPart2DUnitTest.java
 
b/geode-core/src/test/java/org/apache/geode/security/ClientAuthenticationPart2DUnitTest.java
index b9f6223..f1d4f23 100644
--- 
a/geode-core/src/test/java/org/apache/geode/security/ClientAuthenticationPart2DUnitTest.java
+++ 
b/geode-core/src/test/java/org/apache/geode/security/ClientAuthenticationPart2DUnitTest.java
@@ -14,10 +14,16 @@
  */
 package org.apache.geode.security;
 
+import static org.mockito.Mockito.*;
+
+import org.junit.Assert;
 import org.junit.Ignore;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
 
+import org.apache.geode.internal.cache.tier.MessageType;
+import org.apache.geode.internal.cache.tier.sockets.Message;
+import org.apache.geode.internal.cache.tier.sockets.ServerConnection;
 import org.apache.geode.test.junit.categories.DistributedTest;
 import org.apache.geode.test.junit.categories.SecurityTest;
 
@@ -33,12 +39,32 @@ public class ClientAuthenticationPart2DUnitTest extends 
ClientAuthenticationTest
     doTestNoCredentials(true);
   }
 
+  // GEODE-3249
   @Test
   public void testNoCredentialsForMultipleUsersCantRegisterMetadata() throws 
Exception {
     doTestNoCredentialsCantRegisterMetadata(true);
   }
 
   @Test
+  public void testServerConnectionAcceptsOldInternalMessagesIfAllowed() throws 
Exception {
+
+    ServerConnection serverConnection = mock(ServerConnection.class);
+    when(serverConnection.isInternalMessage(any(Message.class), 
any(Boolean.class))).thenCallRealMethod();
+
+    int[] oldInternalMessages = new int[]{MessageType.ADD_PDX_TYPE, 
MessageType.ADD_PDX_ENUM,
+        MessageType.REGISTER_INSTANTIATORS, 
MessageType.REGISTER_DATASERIALIZERS};
+
+    for (int i=0; i<oldInternalMessages.length; i++) {
+      Message message = mock(Message.class);
+      when(message.getMessageType()).thenReturn(oldInternalMessages[i]);
+
+      serverConnection.setRequestMsg(message);
+      Assert.assertFalse(serverConnection.isInternalMessage(message, false));
+      Assert.assertTrue(serverConnection.isInternalMessage(message, true));
+    }
+  }
+
+  @Test
   public void testInvalidCredentialsForMultipleUsers() throws Exception {
     doTestInvalidCredentials(true);
   }

Reply via email to