New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GEODE-9676: Limit array and string sizes for unauthenticated Radish connections #6994
GEODE-9676: Limit array and string sizes for unauthenticated Radish connections #6994
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a couple of small optional suggestions.
...st/java/org/apache/geode/redis/internal/executor/connection/AbstractAuthIntegrationTest.java
Outdated
Show resolved
Hide resolved
...st/java/org/apache/geode/redis/internal/executor/connection/AbstractAuthIntegrationTest.java
Outdated
Show resolved
Hide resolved
...for-redis/src/main/java/org/apache/geode/redis/internal/services/SecurityServiceWrapper.java
Outdated
Show resolved
Hide resolved
...for-redis/src/main/java/org/apache/geode/redis/internal/services/SecurityServiceWrapper.java
Outdated
Show resolved
Hide resolved
...for-redis/src/main/java/org/apache/geode/redis/internal/services/SecurityServiceWrapper.java
Outdated
Show resolved
Hide resolved
…onnections - This applies the same fix as introduced by CVE-2021-32675 for Redis. Unuathenticated requests limit the size of arrays and bulk strings to 10 and 16384 respectively. Once connections are authenticated, the size restriction is not applied. - Re-enable the relevant Redis TCL test.
e403c0f
to
36fac97
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
@@ -45,12 +50,27 @@ | |||
*/ | |||
public class ByteToCommandDecoder extends ByteToMessageDecoder { | |||
|
|||
public static final String UNAUTHENTICATED_MAX_ARRAY_SIZE_PARAM = | |||
"gemfire.geode-for-redis-unauthenticated-max-array-size"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if these properties should be prefixed geode.
and not gemfire.
?
...st/java/org/apache/geode/redis/internal/executor/connection/AbstractAuthIntegrationTest.java
Show resolved
Hide resolved
…onnections (apache#6994) - This applies the same fix as introduced by CVE-2021-32675 for Redis. When security is enabled, unuauthenticated requests limit the size of arrays and bulk strings to 10 and 16384 respectively. Once connections are authenticated, the size restriction is not applied. - When security is not enabled, this restriction does not apply. - Re-enable the relevant Redis TCL test.
When security is enabled, unauthenticated requests limit the size of
arrays and bulk strings to 10 and 16384 respectively. Once connections
are authenticated, the size restriction is not applied.
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?