geode-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From upthewatersp...@apache.org
Subject [geode] branch develop updated: GEODE-4007: Authentication/Handshake errors should close the socket
Date Wed, 06 Dec 2017 22:48:12 GMT
This is an automated email from the ASF dual-hosted git repository.

upthewaterspout pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git


The following commit(s) were added to refs/heads/develop by this push:
     new 5063aca  GEODE-4007: Authentication/Handshake errors should close the socket
5063aca is described below

commit 5063aca4b2b1a1ff43d448cd7b24f17355373a50
Author: Galen O'Sullivan <gosullivan@pivotal.io>
AuthorDate: Tue Nov 21 17:38:06 2017 -0800

    GEODE-4007: Authentication/Handshake errors should close the socket
    
    This will cause the connection to be closed whenever a handshake or
     authentication message fails.
    The connection will also be broken if we ever receive an unexpected handshake or
     authenticantication message.
    
    Signed-off-by: Galen O'Sullivan <gosullivan@pivotal.io>
---
 .../protocol/operations/OperationHandler.java      |  7 ++-
 .../ConnectionShiroAuthorizingStateProcessor.java  |  3 +-
 .../protocol/state/ConnectionStateProcessor.java   |  9 ++++
 .../ConnectionTerminatingStateProcessor.java}      | 21 ++++++--
 .../OperationNotAuthorizedException.java}          | 10 ++--
 .../client/protocol/ClientProtocolProcessor.java   |  5 ++
 .../sockets/GenericProtocolServerConnection.java   |  4 ++
 .../protobuf/v1/ProtobufCachePipeline.java         |  5 ++
 .../protobuf/v1/ProtobufLocatorPipeline.java       |  6 +++
 .../protocol/protobuf/v1/ProtobufOpsProcessor.java | 11 +++-
 .../HandshakeRequestOperationHandler.java          | 14 ++---
 .../AuthenticationRequestOperationHandler.java     | 38 ++++---------
 .../protobuf/v1/AuthenticationIntegrationTest.java | 17 ++++++
 .../protobuf/v1/HandshakeIntegrationTest.java      | 62 ++++++++++++++++++++++
 .../CacheConnectionTimeoutJUnitTest.java           | 15 +++++-
 .../GetAllRequestOperationHandlerJUnitTest.java    |  7 +--
 ...tAvailableServersOperationHandlerJUnitTest.java |  7 ++-
 ...egionNamesRequestOperationHandlerJUnitTest.java |  2 +-
 .../HandshakeRequestOperationHandlerJUnitTest.java | 56 ++++++++++++-------
 19 files changed, 221 insertions(+), 78 deletions(-)

diff --git a/geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/operations/OperationHandler.java
b/geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/operations/OperationHandler.java
index 841976d..dc1fe2e 100644
--- a/geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/operations/OperationHandler.java
+++ b/geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/operations/OperationHandler.java
@@ -19,6 +19,7 @@ import org.apache.geode.internal.exception.InvalidExecutionContextException;
 import org.apache.geode.internal.protocol.MessageExecutionContext;
 import org.apache.geode.internal.protocol.Result;
 import org.apache.geode.internal.protocol.serialization.SerializationService;
+import org.apache.geode.internal.protocol.state.exception.ConnectionStateException;
 
 /**
  * This interface is implemented by a object capable of handling request types 'Req' and
returning
@@ -30,7 +31,11 @@ public interface OperationHandler<Req, Resp, ErrorResp> {
   /**
    * Decode the message, deserialize contained values using the serialization service, do
the work
    * indicated on the provided cache, and return a response.
+   *
+   * @throws ConnectionStateException if the connection is in an invalid state for the operation
in
+   *         question.
    */
   Result<Resp, ErrorResp> process(SerializationService serializationService, Req request,
-      MessageExecutionContext messageExecutionContext) throws InvalidExecutionContextException;
+      MessageExecutionContext messageExecutionContext)
+      throws InvalidExecutionContextException, ConnectionStateException;
 }
diff --git a/geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/state/ConnectionShiroAuthorizingStateProcessor.java
b/geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/state/ConnectionShiroAuthorizingStateProcessor.java
index cd2c6cc..c7c71aa 100644
--- a/geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/state/ConnectionShiroAuthorizingStateProcessor.java
+++ b/geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/state/ConnectionShiroAuthorizingStateProcessor.java
@@ -21,6 +21,7 @@ import org.apache.geode.internal.protocol.MessageExecutionContext;
 import org.apache.geode.internal.protocol.OperationContext;
 import org.apache.geode.internal.protocol.ProtocolErrorCode;
 import org.apache.geode.internal.protocol.state.exception.ConnectionStateException;
+import org.apache.geode.internal.protocol.state.exception.OperationNotAuthorizedException;
 import org.apache.geode.internal.security.SecurityService;
 import org.apache.geode.security.NotAuthorizedException;
 
@@ -42,7 +43,7 @@ public class ConnectionShiroAuthorizingStateProcessor implements ConnectionState
       securityService.authorize(operationContext.getAccessPermissionRequired());
     } catch (NotAuthorizedException e) {
       messageContext.getStatistics().incAuthorizationViolations();
-      throw new ConnectionStateException(ProtocolErrorCode.AUTHORIZATION_FAILED,
+      throw new OperationNotAuthorizedException(ProtocolErrorCode.AUTHORIZATION_FAILED,
           "The user is not authorized to complete this operation");
     } finally {
       threadState.restore();
diff --git a/geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/state/ConnectionStateProcessor.java
b/geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/state/ConnectionStateProcessor.java
index 321120d..e0d18b3 100644
--- a/geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/state/ConnectionStateProcessor.java
+++ b/geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/state/ConnectionStateProcessor.java
@@ -58,4 +58,13 @@ public interface ConnectionStateProcessor {
     throw new ConnectionStateException(ProtocolErrorCode.UNSUPPORTED_OPERATION,
         "Requested operation not allowed at this time");
   }
+
+  /**
+   * This indicates whether this state is capable of receiving any more messages
+   *
+   * @return True if the socket should be closed
+   */
+  default boolean socketProcessingIsFinished() {
+    return false;
+  }
 }
diff --git a/geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/security/exception/IncompatibleAuthenticationMechanismsException.java
b/geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/state/ConnectionTerminatingStateProcessor.java
similarity index 50%
copy from geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/security/exception/IncompatibleAuthenticationMechanismsException.java
copy to geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/state/ConnectionTerminatingStateProcessor.java
index e799522..d1b47ec 100644
--- a/geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/security/exception/IncompatibleAuthenticationMechanismsException.java
+++ b/geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/state/ConnectionTerminatingStateProcessor.java
@@ -12,12 +12,23 @@
  * or implied. See the License for the specific language governing permissions and limitations
under
  * the License.
  */
-package org.apache.geode.internal.protocol.security.exception;
+package org.apache.geode.internal.protocol.state;
 
-import org.apache.geode.security.AuthenticationFailedException;
+import org.apache.geode.internal.protocol.MessageExecutionContext;
+import org.apache.geode.internal.protocol.OperationContext;
+import org.apache.geode.internal.protocol.ProtocolErrorCode;
+import org.apache.geode.internal.protocol.state.exception.ConnectionStateException;
 
-public class IncompatibleAuthenticationMechanismsException extends AuthenticationFailedException
{
-  public IncompatibleAuthenticationMechanismsException(String message) {
-    super(message);
+public class ConnectionTerminatingStateProcessor implements ConnectionStateProcessor {
+  @Override
+  public void validateOperation(MessageExecutionContext messageContext,
+      OperationContext operationContext) throws ConnectionStateException {
+    throw new ConnectionStateException(ProtocolErrorCode.GENERIC_FAILURE,
+        "This connection has been marked as terminating.");
+  }
+
+  @Override
+  public boolean socketProcessingIsFinished() {
+    return true;
   }
 }
diff --git a/geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/security/exception/IncompatibleAuthenticationMechanismsException.java
b/geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/state/exception/OperationNotAuthorizedException.java
similarity index 70%
rename from geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/security/exception/IncompatibleAuthenticationMechanismsException.java
rename to geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/state/exception/OperationNotAuthorizedException.java
index e799522..a06002b 100644
--- a/geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/security/exception/IncompatibleAuthenticationMechanismsException.java
+++ b/geode-client-protocol/src/main/java/org/apache/geode/internal/protocol/state/exception/OperationNotAuthorizedException.java
@@ -12,12 +12,12 @@
  * or implied. See the License for the specific language governing permissions and limitations
under
  * the License.
  */
-package org.apache.geode.internal.protocol.security.exception;
+package org.apache.geode.internal.protocol.state.exception;
 
-import org.apache.geode.security.AuthenticationFailedException;
+import org.apache.geode.internal.protocol.ProtocolErrorCode;
 
-public class IncompatibleAuthenticationMechanismsException extends AuthenticationFailedException
{
-  public IncompatibleAuthenticationMechanismsException(String message) {
-    super(message);
+public class OperationNotAuthorizedException extends ConnectionStateException {
+  public OperationNotAuthorizedException(ProtocolErrorCode errorCode, String errorMessage)
{
+    super(errorCode, errorMessage);
   }
 }
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/client/protocol/ClientProtocolProcessor.java
b/geode-core/src/main/java/org/apache/geode/internal/cache/client/protocol/ClientProtocolProcessor.java
index e49f16f..2631ed5 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/client/protocol/ClientProtocolProcessor.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/client/protocol/ClientProtocolProcessor.java
@@ -42,4 +42,9 @@ public interface ClientProtocolProcessor extends AutoCloseable {
    */
   @Override
   void close();
+
+  /**
+   * Indicates that the associated connection should be closed
+   */
+  boolean socketProcessingIsFinished();
 }
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/GenericProtocolServerConnection.java
b/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/GenericProtocolServerConnection.java
index 8063bf0..736c7ad 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/GenericProtocolServerConnection.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/GenericProtocolServerConnection.java
@@ -68,6 +68,10 @@ public class GenericProtocolServerConnection extends ServerConnection {
       OutputStream outputStream = socket.getOutputStream();
 
       protocolProcessor.processMessage(inputStream, outputStream);
+
+      if (protocolProcessor.socketProcessingIsFinished()) {
+        this.setFlagProcessMessagesAsFalse();
+      }
     } catch (EOFException e) {
       this.setFlagProcessMessagesAsFalse();
       setClientDisconnectedException(e);
diff --git a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/ProtobufCachePipeline.java
b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/ProtobufCachePipeline.java
index 647e13e..4b88ec4 100644
--- a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/ProtobufCachePipeline.java
+++ b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/ProtobufCachePipeline.java
@@ -57,4 +57,9 @@ public final class ProtobufCachePipeline implements ClientProtocolProcessor
{
   public void close() {
     this.statistics.clientDisconnected();
   }
+
+  @Override
+  public boolean socketProcessingIsFinished() {
+    return messageExecutionContext.getConnectionStateProcessor().socketProcessingIsFinished();
+  }
 }
diff --git a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/ProtobufLocatorPipeline.java
b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/ProtobufLocatorPipeline.java
index 3129d59..d67897f 100644
--- a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/ProtobufLocatorPipeline.java
+++ b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/ProtobufLocatorPipeline.java
@@ -55,4 +55,10 @@ public final class ProtobufLocatorPipeline implements ClientProtocolProcessor
{
   public void close() {
     this.statistics.clientDisconnected();
   }
+
+  @Override
+  public boolean socketProcessingIsFinished() {
+    // All locator connections are closed after one message, so this is not used
+    return false;
+  }
 }
diff --git a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/ProtobufOpsProcessor.java
b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/ProtobufOpsProcessor.java
index ef64027..9437c3a 100644
--- a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/ProtobufOpsProcessor.java
+++ b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/ProtobufOpsProcessor.java
@@ -28,7 +28,9 @@ import org.apache.geode.internal.protocol.Result;
 import org.apache.geode.internal.protocol.protobuf.v1.registry.ProtobufOperationContextRegistry;
 import org.apache.geode.internal.protocol.protobuf.v1.utilities.ProtobufResponseUtilities;
 import org.apache.geode.internal.protocol.serialization.SerializationService;
+import org.apache.geode.internal.protocol.state.ConnectionTerminatingStateProcessor;
 import org.apache.geode.internal.protocol.state.exception.ConnectionStateException;
+import org.apache.geode.internal.protocol.state.exception.OperationNotAuthorizedException;
 
 /**
  * This handles protobuf requests by determining the operation type of the request and dispatching
@@ -59,8 +61,14 @@ public class ProtobufOpsProcessor {
       messageExecutionContext.getConnectionStateProcessor()
           .validateOperation(messageExecutionContext, operationContext);
       result = processOperation(request, messageExecutionContext, requestType, operationContext);
+    } catch (OperationNotAuthorizedException e) {
+      // Don't move to a terminating state for authorization state failures
+      logger.warn(e.getMessage());
+      result = Failure.of(ProtobufResponseUtilities.makeErrorResponse(e));
     } catch (ConnectionStateException e) {
       logger.warn(e.getMessage());
+      messageExecutionContext
+          .setConnectionStateProcessor(new ConnectionTerminatingStateProcessor());
       result = Failure.of(ProtobufResponseUtilities.makeErrorResponse(e));
     }
 
@@ -69,7 +77,8 @@ public class ProtobufOpsProcessor {
   }
 
   private Result processOperation(ClientProtocol.Request request, MessageExecutionContext
context,
-      ClientProtocol.Request.RequestAPICase requestType, OperationContext operationContext)
{
+      ClientProtocol.Request.RequestAPICase requestType, OperationContext operationContext)
+      throws ConnectionStateException {
     try {
       return operationContext.getOperationHandler().process(serializationService,
           operationContext.getFromRequest().apply(request), context);
diff --git a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/HandshakeRequestOperationHandler.java
b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/HandshakeRequestOperationHandler.java
index 1521fc0..97338e6 100644
--- a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/HandshakeRequestOperationHandler.java
+++ b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/HandshakeRequestOperationHandler.java
@@ -29,6 +29,7 @@ import org.apache.geode.internal.protocol.protobuf.v1.utilities.ProtobufResponse
 import org.apache.geode.internal.protocol.serialization.SerializationService;
 import org.apache.geode.internal.protocol.state.ConnectionHandshakingStateProcessor;
 import org.apache.geode.internal.protocol.state.ConnectionStateProcessor;
+import org.apache.geode.internal.protocol.state.ConnectionTerminatingStateProcessor;
 import org.apache.geode.internal.protocol.state.exception.ConnectionStateException;
 
 public class HandshakeRequestOperationHandler implements
@@ -39,20 +40,21 @@ public class HandshakeRequestOperationHandler implements
   @Override
   public Result<ConnectionAPI.HandshakeResponse, ClientProtocol.ErrorResponse> process(
       SerializationService serializationService, ConnectionAPI.HandshakeRequest request,
-      MessageExecutionContext messageExecutionContext) throws InvalidExecutionContextException
{
+      MessageExecutionContext messageExecutionContext)
+      throws InvalidExecutionContextException, ConnectionStateException {
     ConnectionHandshakingStateProcessor stateProcessor;
 
-    try {
-      stateProcessor = messageExecutionContext.getConnectionStateProcessor().allowHandshake();
-    } catch (ConnectionStateException e) {
-      return Failure.of(ProtobufResponseUtilities.makeErrorResponse(e));
-    }
+    // If handshake not allowed by this state this will throw a ConnectionStateException
+    stateProcessor = messageExecutionContext.getConnectionStateProcessor().allowHandshake();
 
     final boolean handshakeSucceeded =
         validator.isValid(request.getMajorVersion(), request.getMinorVersion());
     if (handshakeSucceeded) {
       ConnectionStateProcessor nextStateProcessor = stateProcessor.handshakeSucceeded();
       messageExecutionContext.setConnectionStateProcessor(nextStateProcessor);
+    } else {
+      messageExecutionContext
+          .setConnectionStateProcessor(new ConnectionTerminatingStateProcessor());
     }
 
     return Success.of(ConnectionAPI.HandshakeResponse.newBuilder()
diff --git a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/security/AuthenticationRequestOperationHandler.java
b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/security/AuthenticationRequestOperationHandler.java
index 3decb49..727a693 100644
--- a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/security/AuthenticationRequestOperationHandler.java
+++ b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/security/AuthenticationRequestOperationHandler.java
@@ -20,19 +20,16 @@ import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
 import org.apache.geode.internal.exception.InvalidExecutionContextException;
-import org.apache.geode.internal.protocol.Failure;
 import org.apache.geode.internal.protocol.MessageExecutionContext;
-import org.apache.geode.internal.protocol.ProtocolErrorCode;
 import org.apache.geode.internal.protocol.Result;
 import org.apache.geode.internal.protocol.Success;
 import org.apache.geode.internal.protocol.operations.OperationHandler;
-import org.apache.geode.internal.protocol.protobuf.v1.BasicTypes;
 import org.apache.geode.internal.protocol.protobuf.v1.ClientProtocol;
 import org.apache.geode.internal.protocol.protobuf.v1.ConnectionAPI;
-import org.apache.geode.internal.protocol.protobuf.v1.utilities.ProtobufResponseUtilities;
-import org.apache.geode.internal.protocol.security.exception.IncompatibleAuthenticationMechanismsException;
 import org.apache.geode.internal.protocol.serialization.SerializationService;
 import org.apache.geode.internal.protocol.state.ConnectionAuthenticatingStateProcessor;
+import org.apache.geode.internal.protocol.state.ConnectionStateProcessor;
+import org.apache.geode.internal.protocol.state.ConnectionTerminatingStateProcessor;
 import org.apache.geode.internal.protocol.state.exception.ConnectionStateException;
 import org.apache.geode.security.AuthenticationFailedException;
 
@@ -43,41 +40,26 @@ public class AuthenticationRequestOperationHandler implements
   @Override
   public Result<ConnectionAPI.AuthenticationResponse, ClientProtocol.ErrorResponse>
process(
       SerializationService serializationService, ConnectionAPI.AuthenticationRequest request,
-      MessageExecutionContext messageExecutionContext) throws InvalidExecutionContextException
{
+      MessageExecutionContext messageExecutionContext)
+      throws InvalidExecutionContextException, ConnectionStateException {
     ConnectionAuthenticatingStateProcessor stateProcessor;
 
-    try {
-      stateProcessor = messageExecutionContext.getConnectionStateProcessor().allowAuthentication();
-    } catch (ConnectionStateException e) {
-      return Failure.of(ProtobufResponseUtilities.makeErrorResponse(e));
-    }
+    // If authentication not allowed by this state this will throw a ConnectionStateException
+    stateProcessor = messageExecutionContext.getConnectionStateProcessor().allowAuthentication();
 
     Properties properties = new Properties();
     properties.putAll(request.getCredentialsMap());
 
     try {
-      messageExecutionContext.setConnectionStateProcessor(stateProcessor.authenticate(properties));
+      ConnectionStateProcessor nextState = stateProcessor.authenticate(properties);
+      messageExecutionContext.setConnectionStateProcessor(nextState);
       return Success
           .of(ConnectionAPI.AuthenticationResponse.newBuilder().setAuthenticated(true).build());
-    } catch (IncompatibleAuthenticationMechanismsException e) {
-      return Failure.of(ClientProtocol.ErrorResponse.newBuilder().setError(
-          buildAndLogError(ProtocolErrorCode.UNSUPPORTED_AUTHENTICATION_MODE, e.getMessage(),
e))
-          .build());
     } catch (AuthenticationFailedException e) {
+      messageExecutionContext
+          .setConnectionStateProcessor(new ConnectionTerminatingStateProcessor());
       return Success
           .of(ConnectionAPI.AuthenticationResponse.newBuilder().setAuthenticated(false).build());
     }
   }
-
-  private BasicTypes.Error buildAndLogError(ProtocolErrorCode errorCode, String message,
-      Exception ex) {
-    if (ex == null) {
-      logger.warn(message);
-    } else {
-      logger.warn(message, ex);
-    }
-
-    return BasicTypes.Error.newBuilder().setErrorCode(errorCode.codeValue).setMessage(message)
-        .build();
-  }
 }
diff --git a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/AuthenticationIntegrationTest.java
b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/AuthenticationIntegrationTest.java
index e10573a..c3b6c73 100644
--- a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/AuthenticationIntegrationTest.java
+++ b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/AuthenticationIntegrationTest.java
@@ -200,6 +200,7 @@ public class AuthenticationIntegrationTest {
         errorResponse.getResponse().getResponseAPICase());
     assertEquals(AUTHENTICATION_FAILED.codeValue,
         errorResponse.getResponse().getErrorResponse().getError().getErrorCode());
+    verifyConnectionClosed();
   }
 
   @Test
@@ -246,6 +247,7 @@ public class AuthenticationIntegrationTest {
     ConnectionAPI.AuthenticationResponse authenticationResponse =
         parseSimpleAuthenticationResponseFromInput();
     assertFalse(authenticationResponse.getAuthenticated());
+    verifyConnectionClosed();
   }
 
   @Test
@@ -265,6 +267,8 @@ public class AuthenticationIntegrationTest {
     ConnectionAPI.AuthenticationResponse authenticationResponse =
         parseSimpleAuthenticationResponseFromInput();
     assertFalse(authenticationResponse.getAuthenticated());
+
+    verifyConnectionClosed();
   }
 
   @Test
@@ -296,6 +300,7 @@ public class AuthenticationIntegrationTest {
         errorResponse.getResponse().getResponseAPICase());
     assertEquals(UNSUPPORTED_AUTHENTICATION_MODE.codeValue,
         errorResponse.getResponse().getErrorResponse().getError().getErrorCode());
+    verifyConnectionClosed();
   }
 
   @Test
@@ -316,6 +321,18 @@ public class AuthenticationIntegrationTest {
         errorResponse.getResponse().getResponseAPICase());
     assertEquals(UNSUPPORTED_AUTHENTICATION_MODE.codeValue,
         errorResponse.getResponse().getErrorResponse().getError().getErrorCode());
+    verifyConnectionClosed();
+  }
+
+  private void verifyConnectionClosed() {
+    Awaitility.await().atMost(10, TimeUnit.SECONDS).until(() -> {
+      try {
+        assertEquals(-1, socket.getInputStream().read()); // EOF implies disconnected.
+        return true;
+      } catch (IOException e) {
+        throw new RuntimeException(e);
+      }
+    });
   }
 
   private void createLegacyAuthCache(String authenticationProperty) {
diff --git a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/HandshakeIntegrationTest.java
b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/HandshakeIntegrationTest.java
index b52ed0c..de3038f 100644
--- a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/HandshakeIntegrationTest.java
+++ b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/HandshakeIntegrationTest.java
@@ -15,6 +15,7 @@
 package org.apache.geode.internal.protocol.protobuf.v1;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
 import java.io.IOException;
@@ -40,6 +41,7 @@ import org.apache.geode.cache.server.CacheServer;
 import org.apache.geode.distributed.ConfigurationProperties;
 import org.apache.geode.internal.AvailablePortHelper;
 import org.apache.geode.internal.cache.tier.CommunicationMode;
+import org.apache.geode.internal.protocol.ProtocolErrorCode;
 import org.apache.geode.internal.protocol.protobuf.v1.serializer.ProtobufProtocolSerializer;
 import org.apache.geode.test.junit.categories.IntegrationTest;
 
@@ -122,4 +124,64 @@ public class HandshakeIntegrationTest {
       }
     });
   }
+
+  @Test
+  public void testInvalidMinorVersionBreaksConnectionAfterResponse() throws Exception {
+    outputStream.write(CommunicationMode.ProtobufClientServerProtocol.getModeNumber());
+    outputStream.write(ConnectionAPI.MajorVersions.CURRENT_MAJOR_VERSION_VALUE);
+
+    ClientProtocol.Message.newBuilder()
+        .setRequest(ClientProtocol.Request.newBuilder()
+            .setHandshakeRequest(ConnectionAPI.HandshakeRequest.newBuilder()
+                .setMajorVersion(ConnectionAPI.MajorVersions.CURRENT_MAJOR_VERSION_VALUE)
+                .setMinorVersion(ConnectionAPI.MinorVersions.INVALID_MINOR_VERSION_VALUE)))
+        .build().writeDelimitedTo(outputStream);
+    ClientProtocol.Message handshakeResponse = protobufProtocolSerializer.deserialize(inputStream);
+    assertFalse(handshakeResponse.getResponse().getHandshakeResponse().getHandshakePassed());
+
+    // Verify that connection is closed
+    Awaitility.await().atMost(10, TimeUnit.SECONDS).until(() -> {
+      try {
+        assertEquals(-1, socket.getInputStream().read()); // EOF implies disconnected.
+        return true;
+      } catch (IOException e) {
+        throw new RuntimeException(e);
+      }
+    });
+  }
+
+  @Test
+  public void testUnexpectedHandshakeFailsAndClosesConnection() throws Exception {
+    outputStream.write(CommunicationMode.ProtobufClientServerProtocol.getModeNumber());
+    outputStream.write(ConnectionAPI.MajorVersions.CURRENT_MAJOR_VERSION_VALUE);
+
+    ClientProtocol.Message.newBuilder()
+        .setRequest(ClientProtocol.Request.newBuilder()
+            .setHandshakeRequest(ConnectionAPI.HandshakeRequest.newBuilder()
+                .setMajorVersion(ConnectionAPI.MajorVersions.CURRENT_MAJOR_VERSION_VALUE)
+                .setMinorVersion(ConnectionAPI.MinorVersions.CURRENT_MINOR_VERSION_VALUE)))
+        .build().writeDelimitedTo(outputStream);
+    ClientProtocol.Message handshakeResponse = protobufProtocolSerializer.deserialize(inputStream);
+    assertTrue(handshakeResponse.getResponse().getHandshakeResponse().getHandshakePassed());
+
+    ClientProtocol.Message.newBuilder()
+        .setRequest(ClientProtocol.Request.newBuilder()
+            .setHandshakeRequest(ConnectionAPI.HandshakeRequest.newBuilder()
+                .setMajorVersion(ConnectionAPI.MajorVersions.CURRENT_MAJOR_VERSION_VALUE)
+                .setMinorVersion(ConnectionAPI.MinorVersions.CURRENT_MINOR_VERSION_VALUE)))
+        .build().writeDelimitedTo(outputStream);
+    ClientProtocol.Message failingHandshake = protobufProtocolSerializer.deserialize(inputStream);
+    assertEquals(ProtocolErrorCode.UNSUPPORTED_OPERATION.codeValue,
+        failingHandshake.getResponse().getErrorResponse().getError().getErrorCode());
+
+    // Verify that connection is closed
+    Awaitility.await().atMost(10, TimeUnit.SECONDS).until(() -> {
+      try {
+        assertEquals(-1, socket.getInputStream().read()); // EOF implies disconnected.
+        return true;
+      } catch (IOException e) {
+        throw new RuntimeException(e);
+      }
+    });
+  }
 }
diff --git a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/acceptance/CacheConnectionTimeoutJUnitTest.java
b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/acceptance/CacheConnectionTimeoutJUnitTest.java
index b8e4417..b14ceb2 100644
--- a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/acceptance/CacheConnectionTimeoutJUnitTest.java
+++ b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/acceptance/CacheConnectionTimeoutJUnitTest.java
@@ -17,6 +17,7 @@ package org.apache.geode.internal.protocol.protobuf.v1.acceptance;
 
 import static junit.framework.TestCase.assertTrue;
 import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.mock;
 
 import java.io.IOException;
 import java.io.InputStream;
@@ -27,6 +28,7 @@ import java.util.concurrent.TimeUnit;
 
 import org.awaitility.Awaitility;
 import org.junit.After;
+import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -41,9 +43,12 @@ import org.apache.geode.cache.Scope;
 import org.apache.geode.cache.server.CacheServer;
 import org.apache.geode.distributed.ConfigurationProperties;
 import org.apache.geode.internal.AvailablePortHelper;
+import org.apache.geode.internal.cache.InternalCache;
 import org.apache.geode.internal.cache.tier.CommunicationMode;
 import org.apache.geode.internal.cache.tier.sockets.ClientHealthMonitor;
 import org.apache.geode.internal.net.SocketCreatorFactory;
+import org.apache.geode.internal.protocol.MessageExecutionContext;
+import org.apache.geode.internal.protocol.Result;
 import org.apache.geode.internal.protocol.protobuf.v1.ClientProtocol;
 import org.apache.geode.internal.protocol.protobuf.v1.ConnectionAPI;
 import org.apache.geode.internal.protocol.protobuf.v1.MessageUtil;
@@ -83,7 +88,6 @@ public class CacheConnectionTimeoutJUnitTest {
     cacheFactory.set(ConfigurationProperties.MCAST_PORT, "0");
     cacheFactory.set(ConfigurationProperties.ENABLE_CLUSTER_CONFIGURATION, "false");
     cacheFactory.set(ConfigurationProperties.USE_CLUSTER_CONFIGURATION, "false");
-    cacheFactory.setSecurityManager(null);
 
     cache = cacheFactory.create();
 
@@ -150,6 +154,15 @@ public class CacheConnectionTimeoutJUnitTest {
   @Test
   public void testResponsiveClientsStaysConnected() throws Exception {
     ProtobufProtocolSerializer protobufProtocolSerializer = new ProtobufProtocolSerializer();
+
+    ClientProtocol.Message.newBuilder()
+        .setRequest(ClientProtocol.Request.newBuilder()
+            .setHandshakeRequest(ConnectionAPI.HandshakeRequest.newBuilder()
+                .setMajorVersion(ConnectionAPI.MajorVersions.CURRENT_MAJOR_VERSION_VALUE)
+                .setMinorVersion(ConnectionAPI.MinorVersions.CURRENT_MINOR_VERSION_VALUE)))
+        .build().writeDelimitedTo(outputStream);
+    protobufProtocolSerializer.deserialize(socket.getInputStream());
+
     ClientProtocol.Message putMessage =
         MessageUtil.makePutRequestMessage(serializationService, TEST_KEY, TEST_VALUE, TEST_REGION);
 
diff --git a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetAllRequestOperationHandlerJUnitTest.java
b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetAllRequestOperationHandlerJUnitTest.java
index c758438..592e8df 100644
--- a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetAllRequestOperationHandlerJUnitTest.java
+++ b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetAllRequestOperationHandlerJUnitTest.java
@@ -33,7 +33,6 @@ import org.junit.experimental.categories.Category;
 
 import org.apache.geode.cache.CacheLoaderException;
 import org.apache.geode.cache.Region;
-import org.apache.geode.internal.exception.InvalidExecutionContextException;
 import org.apache.geode.internal.protocol.Result;
 import org.apache.geode.internal.protocol.Success;
 import org.apache.geode.internal.protocol.TestExecutionContext;
@@ -95,8 +94,7 @@ public class GetAllRequestOperationHandlerJUnitTest extends OperationHandlerJUni
   }
 
   @Test
-  public void processReturnsNoEntriesForNoKeysRequested() throws UnsupportedEncodingTypeException,
-      CodecNotRegisteredForTypeException, InvalidExecutionContextException {
+  public void processReturnsNoEntriesForNoKeysRequested() throws Exception {
     Result result =
         operationHandler.process(serializationServiceStub, generateTestRequest(false, false),
             TestExecutionContext.getNoAuthCacheExecutionContext(cacheStub));
@@ -129,8 +127,7 @@ public class GetAllRequestOperationHandlerJUnitTest extends OperationHandlerJUni
   }
 
   @Test
-  public void multipleKeysWhereOneThrows() throws UnsupportedEncodingTypeException,
-      CodecNotRegisteredForTypeException, InvalidExecutionContextException {
+  public void multipleKeysWhereOneThrows() throws Exception {
     Result result =
         operationHandler.process(serializationServiceStub, generateTestRequest(true, true),
             TestExecutionContext.getNoAuthCacheExecutionContext(cacheStub));
diff --git a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetAvailableServersOperationHandlerJUnitTest.java
b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetAvailableServersOperationHandlerJUnitTest.java
index a43c5fa..61f72d1 100644
--- a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetAvailableServersOperationHandlerJUnitTest.java
+++ b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetAvailableServersOperationHandlerJUnitTest.java
@@ -38,6 +38,7 @@ import org.apache.geode.internal.protocol.protobuf.v1.BasicTypes;
 import org.apache.geode.internal.protocol.protobuf.v1.LocatorAPI;
 import org.apache.geode.internal.protocol.protobuf.v1.LocatorAPI.GetAvailableServersResponse;
 import org.apache.geode.internal.protocol.protobuf.v1.utilities.ProtobufRequestUtilities;
+import org.apache.geode.internal.protocol.state.exception.ConnectionStateException;
 import org.apache.geode.test.junit.categories.UnitTest;
 
 @Category(UnitTest.class)
@@ -81,8 +82,7 @@ public class GetAvailableServersOperationHandlerJUnitTest extends OperationHandl
   }
 
   @Test
-  public void testWhenServersFromSnapshotAreNullReturnsEmtpy()
-      throws InvalidExecutionContextException {
+  public void testWhenServersFromSnapshotAreNullReturnsEmtpy() throws Exception {
     when(locatorLoadSnapshot.getServers(any())).thenReturn(null);
 
     LocatorAPI.GetAvailableServersRequest getAvailableServersRequest =
@@ -95,8 +95,7 @@ public class GetAvailableServersOperationHandlerJUnitTest extends OperationHandl
   }
 
   private Result getOperationHandlerResult(
-      LocatorAPI.GetAvailableServersRequest getAvailableServersRequest)
-      throws InvalidExecutionContextException {
+      LocatorAPI.GetAvailableServersRequest getAvailableServersRequest) throws Exception
{
     return operationHandler.process(serializationServiceStub, getAvailableServersRequest,
         TestExecutionContext.getLocatorExecutionContext(internalLocatorMock));
   }
diff --git a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetRegionNamesRequestOperationHandlerJUnitTest.java
b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetRegionNamesRequestOperationHandlerJUnitTest.java
index 0deb3f6..4913e4b 100644
--- a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetRegionNamesRequestOperationHandlerJUnitTest.java
+++ b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetRegionNamesRequestOperationHandlerJUnitTest.java
@@ -61,7 +61,7 @@ public class GetRegionNamesRequestOperationHandlerJUnitTest extends OperationHan
   }
 
   @Test
-  public void processReturnsCacheRegions() throws InvalidExecutionContextException {
+  public void processReturnsCacheRegions() throws Exception {
     Result result = operationHandler.process(serializationServiceStub,
         ProtobufRequestUtilities.createGetRegionNamesRequest(),
         getNoAuthCacheExecutionContext(cacheStub));
diff --git a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/HandshakeRequestOperationHandlerJUnitTest.java
b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/HandshakeRequestOperationHandlerJUnitTest.java
index 0641e5d..0baf9bb 100644
--- a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/HandshakeRequestOperationHandlerJUnitTest.java
+++ b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/HandshakeRequestOperationHandlerJUnitTest.java
@@ -4,6 +4,7 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 import static org.mockito.Mockito.mock;
 
 import org.apache.shiro.subject.Subject;
@@ -13,6 +14,7 @@ import org.junit.experimental.categories.Category;
 
 import org.apache.geode.internal.cache.InternalCache;
 import org.apache.geode.internal.exception.InvalidExecutionContextException;
+import org.apache.geode.internal.protocol.Failure;
 import org.apache.geode.internal.protocol.MessageExecutionContext;
 import org.apache.geode.internal.protocol.ProtocolErrorCode;
 import org.apache.geode.internal.protocol.Result;
@@ -21,9 +23,11 @@ import org.apache.geode.internal.protocol.protobuf.v1.ConnectionAPI;
 import org.apache.geode.internal.protocol.protobuf.v1.ProtobufSerializationService;
 import org.apache.geode.internal.protocol.protobuf.v1.state.ConnectionShiroAuthenticatingStateProcessor;
 import org.apache.geode.internal.protocol.protobuf.v1.state.ProtobufConnectionHandshakeStateProcessor;
+import org.apache.geode.internal.protocol.protobuf.v1.utilities.ProtobufResponseUtilities;
 import org.apache.geode.internal.protocol.serialization.SerializationService;
 import org.apache.geode.internal.protocol.state.ConnectionShiroAuthorizingStateProcessor;
 import org.apache.geode.internal.protocol.state.NoSecurityConnectionStateProcessor;
+import org.apache.geode.internal.protocol.state.exception.ConnectionStateException;
 import org.apache.geode.internal.security.SecurityService;
 import org.apache.geode.test.junit.categories.UnitTest;
 
@@ -85,16 +89,20 @@ public class HandshakeRequestOperationHandlerJUnitTest {
         new MessageExecutionContext(mock(InternalCache.class), null, handshakeStateProcessor);
 
     verifyHandshakeFails(handshakeRequest, messageExecutionContext);
+  }
 
-    // Also validate the protobuf INVALID_MAJOR_VERSION_VALUE constant fails
-    handshakeRequest =
+  @Test
+  public void testInvalidMajorVersionProtocolConstantFails() throws Exception {
+    ConnectionAPI.HandshakeRequest handshakeRequest =
         generateHandshakeRequest(ConnectionAPI.MajorVersions.INVALID_MAJOR_VERSION_VALUE,
             ConnectionAPI.MinorVersions.CURRENT_MINOR_VERSION_VALUE);
+    MessageExecutionContext messageExecutionContext =
+        new MessageExecutionContext(mock(InternalCache.class), null, handshakeStateProcessor);
     verifyHandshakeFails(handshakeRequest, messageExecutionContext);
   }
 
   private void verifyHandshakeFails(ConnectionAPI.HandshakeRequest handshakeRequest,
-      MessageExecutionContext messageExecutionContext) throws InvalidExecutionContextException
{
+      MessageExecutionContext messageExecutionContext) throws Exception {
     Result<ConnectionAPI.HandshakeResponse, ClientProtocol.ErrorResponse> result =
         handshakeHandler.process(serializationService, handshakeRequest, messageExecutionContext);
     ConnectionAPI.HandshakeResponse handshakeResponse = result.getMessage();
@@ -111,11 +119,16 @@ public class HandshakeRequestOperationHandlerJUnitTest {
         new MessageExecutionContext(mock(InternalCache.class), null, handshakeStateProcessor);
 
     verifyHandshakeFails(handshakeRequest, messageExecutionContext);
+  }
 
-    // Also validate the protobuf INVALID_MINOR_VERSION_VALUE constant fails
-    handshakeRequest =
+  @Test
+  public void testInvalidMinorVersionProtocolConstantFails() throws Exception {
+    ConnectionAPI.HandshakeRequest handshakeRequest =
         generateHandshakeRequest(ConnectionAPI.MajorVersions.CURRENT_MAJOR_VERSION_VALUE,
             ConnectionAPI.MinorVersions.INVALID_MINOR_VERSION_VALUE);
+    MessageExecutionContext messageExecutionContext =
+        new MessageExecutionContext(mock(InternalCache.class), null, handshakeStateProcessor);
+
     verifyHandshakeFails(handshakeRequest, messageExecutionContext);
   }
 
@@ -127,11 +140,12 @@ public class HandshakeRequestOperationHandlerJUnitTest {
     MessageExecutionContext messageExecutionContext = new MessageExecutionContext(
         mock(InternalCache.class), null, new NoSecurityConnectionStateProcessor());
 
-    Result<ConnectionAPI.HandshakeResponse, ClientProtocol.ErrorResponse> result =
-        handshakeHandler.process(serializationService, handshakeRequest, messageExecutionContext);
-    ClientProtocol.ErrorResponse errorMessage = result.getErrorMessage();
-    assertEquals(ProtocolErrorCode.UNSUPPORTED_OPERATION.codeValue,
-        errorMessage.getError().getErrorCode());
+    try {
+      handshakeHandler.process(serializationService, handshakeRequest, messageExecutionContext);
+      fail("Handshake in non-handshake state should throw exception");
+    } catch (ConnectionStateException ex) {
+      assertEquals(ProtocolErrorCode.UNSUPPORTED_OPERATION, ex.getErrorCode());
+    }
   }
 
   @Test
@@ -143,11 +157,12 @@ public class HandshakeRequestOperationHandlerJUnitTest {
         new MessageExecutionContext(mock(InternalCache.class), null,
             new ConnectionShiroAuthenticatingStateProcessor(mock(SecurityService.class)));
 
-    Result<ConnectionAPI.HandshakeResponse, ClientProtocol.ErrorResponse> result =
-        handshakeHandler.process(serializationService, handshakeRequest, messageExecutionContext);
-    ClientProtocol.ErrorResponse errorMessage = result.getErrorMessage();
-    assertEquals(ProtocolErrorCode.UNSUPPORTED_OPERATION.codeValue,
-        errorMessage.getError().getErrorCode());
+    try {
+      handshakeHandler.process(serializationService, handshakeRequest, messageExecutionContext);
+      fail("Handshake in non-handshake state should throw exception");
+    } catch (ConnectionStateException ex) {
+      assertEquals(ProtocolErrorCode.UNSUPPORTED_OPERATION, ex.getErrorCode());
+    }
   }
 
   @Test
@@ -160,11 +175,12 @@ public class HandshakeRequestOperationHandlerJUnitTest {
             new ConnectionShiroAuthorizingStateProcessor(mock(SecurityService.class),
                 mock(Subject.class)));
 
-    Result<ConnectionAPI.HandshakeResponse, ClientProtocol.ErrorResponse> result =
-        handshakeHandler.process(serializationService, handshakeRequest, messageExecutionContext);
-    ClientProtocol.ErrorResponse errorMessage = result.getErrorMessage();
-    assertEquals(ProtocolErrorCode.UNSUPPORTED_OPERATION.codeValue,
-        errorMessage.getError().getErrorCode());
+    try {
+      handshakeHandler.process(serializationService, handshakeRequest, messageExecutionContext);
+      fail("Handshake in non-handshake state should throw exception");
+    } catch (ConnectionStateException ex) {
+      assertEquals(ProtocolErrorCode.UNSUPPORTED_OPERATION, ex.getErrorCode());
+    }
   }
 
   private ConnectionAPI.HandshakeRequest generateHandshakeRequest(int majorVersion,

-- 
To stop receiving notification emails like this one, please contact
['"commits@geode.apache.org" <commits@geode.apache.org>'].

Mime
View raw message