dschneider-pivotal commented on a change in pull request #6835: URL: https://github.com/apache/geode/pull/6835#discussion_r706372806
########## File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientReAuthenticateMessage.java ########## @@ -0,0 +1,103 @@ +/* + * 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; + +import java.io.DataInput; +import java.io.DataOutput; +import java.io.IOException; + +import org.apache.geode.DataSerializer; +import org.apache.geode.internal.cache.EventID; +import org.apache.geode.internal.cache.tier.MessageType; +import org.apache.geode.internal.serialization.DeserializationContext; +import org.apache.geode.internal.serialization.KnownVersion; +import org.apache.geode.internal.serialization.SerializationContext; + +public class ClientReAuthenticateMessage implements ClientMessage { + + /** + * This <code>ClientMessage</code>'s <code>EventID</code> + */ + private EventID eventId; Review comment: A more efficient way of having this message serialize "eventId" also allows you to make this field final. You would make the following changes: 1. make the field "final". 2. add to the default constructor "eventId = new EventID();" 3. change toData to be "eventId.toData(out, context);" 4. change fromData to be "eventId.fromData(in, context);" ########## File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/MessageDispatcher.java ########## @@ -362,31 +385,63 @@ protected void runDispatcher() { } waitForResumption(); } - try { - clientMessage = (ClientMessage) _messageQueue.peek(); - } catch (RegionDestroyedException skipped) { - break; + + if (clientMessage == null) { + try { + clientMessage = (ClientMessage) _messageQueue.peek(); + } catch (RegionDestroyedException skipped) { + break; + } } + getStatistics().setQueueSize(_messageQueue.size()); if (isStopped()) { break; } - if (clientMessage != null) { - // Process the message - long start = getStatistics().startTime(); - //// BUGFIX for BUG#38206 and BUG#37791 - boolean isDispatched = dispatchMessage(clientMessage); - getStatistics().endMessage(start); - if (isDispatched) { + + if (clientMessage == null) { + _messageQueue.remove(); + break; + } + + // Process the message + long start = getStatistics().startTime(); + try { + if (dispatchMessage(clientMessage)) { + getStatistics().endMessage(start); _messageQueue.remove(); if (clientMessage instanceof ClientMarkerMessageImpl) { getProxy().setMarkerEnqueued(false); } } - } else { + clientMessage = null; + wait_for_re_auth_start_time = -1; + } catch (NotAuthorizedException notAuthorized) { + // behave as if the message is dispatched, remove from the queue + logger.info("skip delivering message: " + clientMessage, notAuthorized); Review comment: But if it does happen (i.e. the info log message) do we want the server to keep logging it for every message it attempts to send to that unauthenticated client? Will the server close this dispatcher in this case or retry reauth? If not it seems like we could log once (maybe it should be a warning) for that client. ########## File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/AuthenticateUserOp.java ########## @@ -123,39 +119,61 @@ private AuthenticateUserOp() { super(MessageType.USER_CREDENTIAL_MESSAGE, 1); securityProperties = securityProps; needsServerLocation = needsServer; - getMessage().setMessageHasSecurePartFlag(); } @Override protected void sendMessage(Connection cnx) throws Exception { - HeapDataOutputStream hdos = new HeapDataOutputStream(KnownVersion.CURRENT); - byte[] secureBytes; - hdos.writeLong(cnx.getConnectionID()); - if (securityProperties != null) { - DistributedMember server = new InternalDistributedMember(cnx.getSocket().getInetAddress(), - cnx.getSocket().getPort(), false); - DistributedSystem sys = InternalDistributedSystem.getConnectedInstance(); - String authInitMethod = sys.getProperties().getProperty(SECURITY_CLIENT_AUTH_INIT); - - Properties credentials = Handshake.getCredentials(authInitMethod, securityProperties, - server, false, sys.getLogWriter(), sys.getSecurityLogWriter()); - byte[] credentialBytes; - try (HeapDataOutputStream heapdos = new HeapDataOutputStream(KnownVersion.CURRENT)) { - DataSerializer.writeProperties(credentials, heapdos); - credentialBytes = ((ConnectionImpl) cnx).encryptBytes(heapdos.toByteArray()); - } - getMessage().addBytesPart(credentialBytes); + if (securityProperties == null) { + securityProperties = getConnectedSystem().getSecurityProperties(); } - try { - secureBytes = ((ConnectionImpl) cnx).encryptBytes(hdos.toByteArray()); - } finally { - hdos.close(); + byte[] credentialBytes = getCredentialBytes(cnx, securityProperties); + getMessage().addBytesPart(credentialBytes); + + long userId = getUserId(cnx); + try (HeapDataOutputStream hdos = new HeapDataOutputStream(KnownVersion.CURRENT)) { Review comment: Since the default size of HeapDataOutputStream is 1024 you might want to call this with an initial size of 16 since you only write two longs to it. ########## File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/MessageDispatcher.java ########## @@ -69,6 +78,17 @@ */ private static final String KEY_SLOW_START_TIME_FOR_TESTING = "slowStartTimeForTesting"; + + /** + * Default value for waitinig for re-authentication + */ + private static final long DEFAULT_RE_AUTHENTICATE_WAIT_TIME = 5000; + + /** + * Key in the system property from which the slow starting time value will be retrieved + */ + protected static final String RE_AUTHENTICATE_WAIT_TIME_KEY = "geode.reauthenticate.wait.time"; Review comment: Consider using SystemPropertyHelper for you new system properties. It has support for both prefixes (geode. or gemfire.). ########## File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/MessageDispatcher.java ########## @@ -69,6 +78,17 @@ */ private static final String KEY_SLOW_START_TIME_FOR_TESTING = "slowStartTimeForTesting"; + + /** + * Default value for waitinig for re-authentication Review comment: typo "waitining". Also describe the units (milliseconds) ########## File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/AuthenticateUserOp.java ########## @@ -123,39 +119,61 @@ private AuthenticateUserOp() { super(MessageType.USER_CREDENTIAL_MESSAGE, 1); securityProperties = securityProps; needsServerLocation = needsServer; - getMessage().setMessageHasSecurePartFlag(); } @Override protected void sendMessage(Connection cnx) throws Exception { - HeapDataOutputStream hdos = new HeapDataOutputStream(KnownVersion.CURRENT); - byte[] secureBytes; - hdos.writeLong(cnx.getConnectionID()); - if (securityProperties != null) { - DistributedMember server = new InternalDistributedMember(cnx.getSocket().getInetAddress(), - cnx.getSocket().getPort(), false); - DistributedSystem sys = InternalDistributedSystem.getConnectedInstance(); - String authInitMethod = sys.getProperties().getProperty(SECURITY_CLIENT_AUTH_INIT); - - Properties credentials = Handshake.getCredentials(authInitMethod, securityProperties, - server, false, sys.getLogWriter(), sys.getSecurityLogWriter()); - byte[] credentialBytes; - try (HeapDataOutputStream heapdos = new HeapDataOutputStream(KnownVersion.CURRENT)) { - DataSerializer.writeProperties(credentials, heapdos); - credentialBytes = ((ConnectionImpl) cnx).encryptBytes(heapdos.toByteArray()); - } - getMessage().addBytesPart(credentialBytes); + if (securityProperties == null) { + securityProperties = getConnectedSystem().getSecurityProperties(); } - try { - secureBytes = ((ConnectionImpl) cnx).encryptBytes(hdos.toByteArray()); - } finally { - hdos.close(); + byte[] credentialBytes = getCredentialBytes(cnx, securityProperties); + getMessage().addBytesPart(credentialBytes); + + long userId = getUserId(cnx); + try (HeapDataOutputStream hdos = new HeapDataOutputStream(KnownVersion.CURRENT)) { + hdos.writeLong(cnx.getConnectionID()); + hdos.writeLong(userId); Review comment: since "userId" is only used here consider inlining the call of "getUserId(cnx)" or move the declaration into this block of code. ########## File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/MessageDispatcher.java ########## @@ -362,31 +390,67 @@ protected void runDispatcher() { } waitForResumption(); } - try { - clientMessage = (ClientMessage) _messageQueue.peek(); - } catch (RegionDestroyedException skipped) { - break; + + // if message is not delivered due to authentication expiation, this clientMessage Review comment: typo "expiation" should be "expiration"? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
