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 <[email protected]> Authored: Wed Aug 9 15:40:23 2017 -0700 Committer: Bruce Schuchardt <[email protected]> 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); }
