You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2022/04/16 00:11:47 UTC
[GitHub] [geode] upthewaterspout opened a new pull request, #7598: GEODE-10243: Fail early if old client auth expires
upthewaterspout opened a new pull request, #7598:
URL: https://github.com/apache/geode/pull/7598
We don't support re-authentication of old clients with server->client queues.
1.15 and greater clients will receive a new message to trigger
reauthentication, but older clients have no reliable way to be notified that
they need to re-authenticate themselves when they have a server->client queue.
To make this clear to users, if an AuthenticationExpiredException is ever
triggered for a client that has a server->client queue and is running a version
less than 1.15, we will return an IllegalStateException to the client. If the
exception happens while processing the queue we will log a warning and
immediately disconnect their queue.
<!-- 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:
- [ ] 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`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] 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, check Concourse 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.
-->
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on a diff in pull request #7598: GEODE-10243: Fail early if old client auth expires
Posted by GitBox <gi...@apache.org>.
agingade commented on code in PR #7598:
URL: https://github.com/apache/geode/pull/7598#discussion_r851566972
##########
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientReAuthenticateMessage.java:
##########
@@ -28,6 +28,11 @@
public class ClientReAuthenticateMessage implements ClientMessage {
@Immutable
public static final KnownVersion RE_AUTHENTICATION_START_VERSION = KnownVersion.GEODE_1_15_0;
+
+ public static final String OLD_CLIENT_AUTHENTICATION_EXPIRED =
+ "Authorization expired for a client with a version less than Geode 1.15. Cannot re-authenticate an older client "
+ + " that has a server to client queue for CQs or interest registrations. "
+ + "Please upgrade your client to Geode 1.15 or greater to allow re-authentication.";
Review Comment:
Do we need please :)
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on a diff in pull request #7598: GEODE-10243: Fail early if old client auth expires
Posted by GitBox <gi...@apache.org>.
agingade commented on code in PR #7598:
URL: https://github.com/apache/geode/pull/7598#discussion_r852423995
##########
geode-old-client-support/src/main/java/com/gemstone/gemfire/OldClientSupportProvider.java:
##########
@@ -121,19 +124,23 @@ public String processOutgoingClassName(String name, DataOutput out) {
}
@Override
- public Throwable getThrowable(Throwable theThrowable, KnownVersion clientVersion) {
+ public Throwable getThrowable(Throwable theThrowable, KnownVersion clientVersion,
+ ClientProxyMembershipID clientId) {
if (theThrowable == null) {
return theThrowable;
}
// backward compatibility for authentication expiration
- if (clientVersion.isOlderThan(ClientReAuthenticateMessage.RE_AUTHENTICATION_START_VERSION)) {
- if (theThrowable instanceof AuthenticationExpiredException) {
- return new AuthenticationRequiredException(USER_NOT_FOUND);
- }
- Throwable cause = theThrowable.getCause();
- if (cause instanceof AuthenticationExpiredException) {
+ if (clientVersion.isOlderThan(RE_AUTHENTICATION_START_VERSION)) {
+ if (theThrowable instanceof AuthenticationExpiredException
+ || theThrowable.getCause() instanceof AuthenticationExpiredException) {
+ if (CacheClientNotifier.getInstance().getClientProxy(clientId) != null) {
+ // Re-authentication with Server->Client queues is not supported
+ return new IllegalStateException(OLD_CLIENT_AUTHENTICATION_EXPIRED);
Review Comment:
Can we do this:
CacheClientProxy proxy = CacheClientNotifier.getInstance().getClientProxy(clientId);
if (proxy != null && (proxy.hasNoCqs() || !proxy.hasInterestRegister()) {
}
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] upthewaterspout commented on a diff in pull request #7598: GEODE-10243: Fail early if old client auth expires
Posted by GitBox <gi...@apache.org>.
upthewaterspout commented on code in PR #7598:
URL: https://github.com/apache/geode/pull/7598#discussion_r852231187
##########
geode-old-client-support/src/main/java/com/gemstone/gemfire/OldClientSupportProvider.java:
##########
@@ -121,19 +124,23 @@ public String processOutgoingClassName(String name, DataOutput out) {
}
@Override
- public Throwable getThrowable(Throwable theThrowable, KnownVersion clientVersion) {
+ public Throwable getThrowable(Throwable theThrowable, KnownVersion clientVersion,
+ ClientProxyMembershipID clientId) {
if (theThrowable == null) {
return theThrowable;
}
// backward compatibility for authentication expiration
- if (clientVersion.isOlderThan(ClientReAuthenticateMessage.RE_AUTHENTICATION_START_VERSION)) {
- if (theThrowable instanceof AuthenticationExpiredException) {
- return new AuthenticationRequiredException(USER_NOT_FOUND);
- }
- Throwable cause = theThrowable.getCause();
- if (cause instanceof AuthenticationExpiredException) {
+ if (clientVersion.isOlderThan(RE_AUTHENTICATION_START_VERSION)) {
+ if (theThrowable instanceof AuthenticationExpiredException
+ || theThrowable.getCause() instanceof AuthenticationExpiredException) {
+ if (CacheClientNotifier.getInstance().getClientProxy(clientId) != null) {
+ // Re-authentication with Server->Client queues is not supported
+ return new IllegalStateException(OLD_CLIENT_AUTHENTICATION_EXPIRED);
Review Comment:
Good question. I think they should be able to, but I can test that out. In this case handling the exception would mean just deserializing it and reporting it to the user.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] upthewaterspout closed pull request #7598: GEODE-10243: Fail early if old client auth expires
Posted by GitBox <gi...@apache.org>.
upthewaterspout closed pull request #7598: GEODE-10243: Fail early if old client auth expires
URL: https://github.com/apache/geode/pull/7598
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] upthewaterspout commented on pull request #7598: GEODE-10243: Fail early if old client auth expires
Posted by GitBox <gi...@apache.org>.
upthewaterspout commented on PR #7598:
URL: https://github.com/apache/geode/pull/7598#issuecomment-1101855136
Closing in favor of #7603.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a diff in pull request #7598: GEODE-10243: Fail early if old client auth expires
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on code in PR #7598:
URL: https://github.com/apache/geode/pull/7598#discussion_r852236684
##########
geode-old-client-support/src/main/java/com/gemstone/gemfire/OldClientSupportProvider.java:
##########
@@ -121,19 +124,23 @@ public String processOutgoingClassName(String name, DataOutput out) {
}
@Override
- public Throwable getThrowable(Throwable theThrowable, KnownVersion clientVersion) {
+ public Throwable getThrowable(Throwable theThrowable, KnownVersion clientVersion,
+ ClientProxyMembershipID clientId) {
if (theThrowable == null) {
return theThrowable;
}
// backward compatibility for authentication expiration
- if (clientVersion.isOlderThan(ClientReAuthenticateMessage.RE_AUTHENTICATION_START_VERSION)) {
- if (theThrowable instanceof AuthenticationExpiredException) {
- return new AuthenticationRequiredException(USER_NOT_FOUND);
- }
- Throwable cause = theThrowable.getCause();
- if (cause instanceof AuthenticationExpiredException) {
+ if (clientVersion.isOlderThan(RE_AUTHENTICATION_START_VERSION)) {
+ if (theThrowable instanceof AuthenticationExpiredException
+ || theThrowable.getCause() instanceof AuthenticationExpiredException) {
+ if (CacheClientNotifier.getInstance().getClientProxy(clientId) != null) {
+ // Re-authentication with Server->Client queues is not supported
+ return new IllegalStateException(OLD_CLIENT_AUTHENTICATION_EXPIRED);
Review Comment:
Then this change would require the "user" to handle it. Users probably will have a catch call "exception" case, is this sufficient?
This has caused some client tests to fail that only sets the subscription enabled to be "true", but never uses a CQ/RI.
I still don't think this is necessary. As I have discussed in the slack channel, the queue would find out the exception soon enough to close all connections.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a diff in pull request #7598: GEODE-10243: Fail early if old client auth expires
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on code in PR #7598:
URL: https://github.com/apache/geode/pull/7598#discussion_r851562546
##########
geode-old-client-support/src/main/java/com/gemstone/gemfire/OldClientSupportProvider.java:
##########
@@ -121,19 +124,23 @@ public String processOutgoingClassName(String name, DataOutput out) {
}
@Override
- public Throwable getThrowable(Throwable theThrowable, KnownVersion clientVersion) {
+ public Throwable getThrowable(Throwable theThrowable, KnownVersion clientVersion,
+ ClientProxyMembershipID clientId) {
if (theThrowable == null) {
return theThrowable;
}
// backward compatibility for authentication expiration
- if (clientVersion.isOlderThan(ClientReAuthenticateMessage.RE_AUTHENTICATION_START_VERSION)) {
- if (theThrowable instanceof AuthenticationExpiredException) {
- return new AuthenticationRequiredException(USER_NOT_FOUND);
- }
- Throwable cause = theThrowable.getCause();
- if (cause instanceof AuthenticationExpiredException) {
+ if (clientVersion.isOlderThan(RE_AUTHENTICATION_START_VERSION)) {
+ if (theThrowable instanceof AuthenticationExpiredException
+ || theThrowable.getCause() instanceof AuthenticationExpiredException) {
+ if (CacheClientNotifier.getInstance().getClientProxy(clientId) != null) {
+ // Re-authentication with Server->Client queues is not supported
+ return new IllegalStateException(OLD_CLIENT_AUTHENTICATION_EXPIRED);
Review Comment:
Will the old client apps (without any code change) be able to handle this exception?
##########
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/MessageDispatcher.java:
##########
@@ -539,12 +540,17 @@ private boolean handleAuthenticationExpiredException(AuthenticationExpiredExcept
// flag for the notification to happen.
waitForReAuthenticationStartTime = System.currentTimeMillis();
Review Comment:
You want to move these two lines down to after the if (old version) block
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on a diff in pull request #7598: GEODE-10243: Fail early if old client auth expires
Posted by GitBox <gi...@apache.org>.
agingade commented on code in PR #7598:
URL: https://github.com/apache/geode/pull/7598#discussion_r851567137
##########
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ClientReAuthenticateMessage.java:
##########
@@ -28,6 +28,11 @@
public class ClientReAuthenticateMessage implements ClientMessage {
@Immutable
public static final KnownVersion RE_AUTHENTICATION_START_VERSION = KnownVersion.GEODE_1_15_0;
+
+ public static final String OLD_CLIENT_AUTHENTICATION_EXPIRED =
+ "Authorization expired for a client with a version less than Geode 1.15. Cannot re-authenticate an older client "
+ + " that has a server to client queue for CQs or interest registrations. "
Review Comment:
Instead of calling CQs or Interest; can we just say
"older clients with client event queue"
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a diff in pull request #7598: GEODE-10243: Fail early if old client auth expires
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on code in PR #7598:
URL: https://github.com/apache/geode/pull/7598#discussion_r851563056
##########
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/MessageDispatcher.java:
##########
@@ -539,12 +540,17 @@ private boolean handleAuthenticationExpiredException(AuthenticationExpiredExcept
// flag for the notification to happen.
waitForReAuthenticationStartTime = System.currentTimeMillis();
Review Comment:
You probably need to move these two lines down to after the if (old version) block
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org