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 2021/10/13 20:59:57 UTC
[GitHub] [geode] jdeppe-pivotal opened a new pull request #6994: GEODE-9676: Limit array and string sizes for unauthenticated Radish connections
jdeppe-pivotal opened a new pull request #6994:
URL: https://github.com/apache/geode/pull/6994
- 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.
<!-- 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] nonbinaryprogrammer commented on a change in pull request #6994: GEODE-9676: Limit array and string sizes for unauthenticated Radish connections
Posted by GitBox <gi...@apache.org>.
nonbinaryprogrammer commented on a change in pull request #6994:
URL: https://github.com/apache/geode/pull/6994#discussion_r731316669
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/connection/AbstractAuthIntegrationTest.java
##########
@@ -128,4 +148,128 @@ public void givenNoSecurity_accessWithoutAuth_passes() throws Exception {
assertThat(jedis.ping()).isEqualTo("PONG");
}
+ @Test
+ public void givenSecurity_largeMultiBulkRequestsFail_whenNotAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+
+ try (Socket clientSocket = new Socket(BIND_ADDRESS, getPort())) {
+ clientSocket.setSoTimeout(1000);
+ PrintWriter out = new PrintWriter(clientSocket.getOutputStream());
+ BufferedReader in = new BufferedReader(new InputStreamReader(clientSocket.getInputStream()));
+
+ out.write("*100\r\n");
+ out.flush();
+ String response = in.readLine();
+
+ assertThat(response).contains(ERROR_UNAUTHENTICATED_MULTIBULK);
+ }
+ }
+
+ @Test
+ public void givenSecurity_largeMultiBulkRequestsSucceed_whenAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+
+ List<String> msetArgs = new ArrayList<>();
+ for (int i = 0; i < ByteToCommandDecoder.UNAUTHENTICATED_MAX_ARRAY_SIZE; i++) {
+ msetArgs.add("{hash}key-" + i);
+ msetArgs.add("value-" + i);
+ }
+
+ assertThat(jedis.auth(getUsername(), getPassword())).isEqualTo("OK");
+ assertThat(jedis.mset(msetArgs.toArray(new String[] {}))).isEqualTo("OK");
+ }
+
+ @Test
+ public void givenNoSecurity_largeMultiBulkRequestsSucceed_whenNotAuthenticated()
+ throws Exception {
+ setupCacheWithoutSecurity();
+
+ List<String> msetArgs = new ArrayList<>();
+ for (int i = 0; i < ByteToCommandDecoder.UNAUTHENTICATED_MAX_ARRAY_SIZE; i++) {
+ msetArgs.add("{hash}key-" + i);
+ msetArgs.add("value-" + i);
+ }
+
+ assertThat(jedis.mset(msetArgs.toArray(new String[] {}))).isEqualTo("OK");
+ }
+
+ @Test
+ public void givenSecurity_largeBulkStringRequestsFail_whenNotAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+
+ try (Socket clientSocket = new Socket(BIND_ADDRESS, getPort())) {
+ clientSocket.setSoTimeout(1000);
+ PrintWriter out = new PrintWriter(clientSocket.getOutputStream());
+ BufferedReader in = new BufferedReader(new InputStreamReader(clientSocket.getInputStream()));
+
+ out.write("*1\r\n$100000000\r\n");
+ out.flush();
+ String response = in.readLine();
+
+ assertThat(response).contains(ERROR_UNAUTHENTICATED_BULK);
+ }
+ }
+
+ @Test
+ public void givenSecurity_largeBulkStringRequestsSucceed_whenAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+ int stringSize = ByteToCommandDecoder.UNAUTHENTICATED_MAX_BULK_STRING_LENGTH + 1;
+
+ String largeString = StringUtils.repeat('a', stringSize);
+
+ assertThat(jedis.auth(getUsername(), getPassword())).isEqualTo("OK");
+ assertThat(jedis.set("key", largeString)).isEqualTo("OK");
+ }
+
+ @Test
+ public void givenNoSecurity_largeBulkStringRequestsSucceed_whenNotAuthenticated()
+ throws Exception {
+ setupCacheWithoutSecurity();
+ int stringSize = ByteToCommandDecoder.UNAUTHENTICATED_MAX_BULK_STRING_LENGTH + 1;
+
+ String largeString = StringUtils.repeat('a', stringSize);
+
+ assertThat(jedis.set("key", largeString)).isEqualTo("OK");
+ }
Review comment:
might be nice to have a test like this for the case where there is security but we aren't authenticated, where the string size is over the UNAUTHENTICATED_MAX_BULK_STRING_LENGTH
--
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] jdeppe-pivotal merged pull request #6994: GEODE-9676: Limit array and string sizes for unauthenticated Radish connections
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal merged pull request #6994:
URL: https://github.com/apache/geode/pull/6994
--
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] jdeppe-pivotal commented on a change in pull request #6994: GEODE-9676: Limit array and string sizes for unauthenticated Radish connections
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6994:
URL: https://github.com/apache/geode/pull/6994#discussion_r731990324
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/connection/AbstractAuthIntegrationTest.java
##########
@@ -128,4 +148,128 @@ public void givenNoSecurity_accessWithoutAuth_passes() throws Exception {
assertThat(jedis.ping()).isEqualTo("PONG");
}
+ @Test
+ public void givenSecurity_largeMultiBulkRequestsFail_whenNotAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+
+ try (Socket clientSocket = new Socket(BIND_ADDRESS, getPort())) {
+ clientSocket.setSoTimeout(1000);
+ PrintWriter out = new PrintWriter(clientSocket.getOutputStream());
+ BufferedReader in = new BufferedReader(new InputStreamReader(clientSocket.getInputStream()));
+
+ out.write("*100\r\n");
+ out.flush();
+ String response = in.readLine();
+
+ assertThat(response).contains(ERROR_UNAUTHENTICATED_MULTIBULK);
+ }
+ }
+
+ @Test
+ public void givenSecurity_largeMultiBulkRequestsSucceed_whenAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+
+ List<String> msetArgs = new ArrayList<>();
+ for (int i = 0; i < ByteToCommandDecoder.UNAUTHENTICATED_MAX_ARRAY_SIZE; i++) {
+ msetArgs.add("{hash}key-" + i);
+ msetArgs.add("value-" + i);
+ }
+
+ assertThat(jedis.auth(getUsername(), getPassword())).isEqualTo("OK");
+ assertThat(jedis.mset(msetArgs.toArray(new String[] {}))).isEqualTo("OK");
+ }
+
+ @Test
+ public void givenNoSecurity_largeMultiBulkRequestsSucceed_whenNotAuthenticated()
+ throws Exception {
+ setupCacheWithoutSecurity();
+
+ List<String> msetArgs = new ArrayList<>();
+ for (int i = 0; i < ByteToCommandDecoder.UNAUTHENTICATED_MAX_ARRAY_SIZE; i++) {
+ msetArgs.add("{hash}key-" + i);
+ msetArgs.add("value-" + i);
+ }
+
+ assertThat(jedis.mset(msetArgs.toArray(new String[] {}))).isEqualTo("OK");
+ }
+
+ @Test
+ public void givenSecurity_largeBulkStringRequestsFail_whenNotAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+
+ try (Socket clientSocket = new Socket(BIND_ADDRESS, getPort())) {
+ clientSocket.setSoTimeout(1000);
+ PrintWriter out = new PrintWriter(clientSocket.getOutputStream());
+ BufferedReader in = new BufferedReader(new InputStreamReader(clientSocket.getInputStream()));
+
+ out.write("*1\r\n$100000000\r\n");
+ out.flush();
+ String response = in.readLine();
+
+ assertThat(response).contains(ERROR_UNAUTHENTICATED_BULK);
+ }
+ }
+
+ @Test
+ public void givenSecurity_largeBulkStringRequestsSucceed_whenAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+ int stringSize = ByteToCommandDecoder.UNAUTHENTICATED_MAX_BULK_STRING_LENGTH + 1;
+
+ String largeString = StringUtils.repeat('a', stringSize);
+
+ assertThat(jedis.auth(getUsername(), getPassword())).isEqualTo("OK");
+ assertThat(jedis.set("key", largeString)).isEqualTo("OK");
+ }
+
+ @Test
+ public void givenNoSecurity_largeBulkStringRequestsSucceed_whenNotAuthenticated()
+ throws Exception {
+ setupCacheWithoutSecurity();
+ int stringSize = ByteToCommandDecoder.UNAUTHENTICATED_MAX_BULK_STRING_LENGTH + 1;
+
+ String largeString = StringUtils.repeat('a', stringSize);
+
+ assertThat(jedis.set("key", largeString)).isEqualTo("OK");
+ }
Review comment:
I think that's captured in `givenSecurity_largeBulkStringRequestsFail_whenNotAuthenticated`
--
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] dschneider-pivotal commented on a change in pull request #6994: GEODE-9676: Limit array and string sizes for unauthenticated Radish connections
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6994:
URL: https://github.com/apache/geode/pull/6994#discussion_r731107998
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/services/SecurityServiceWrapper.java
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.redis.internal.services;
+
+import java.util.Map;
+import java.util.Properties;
+import java.util.concurrent.ConcurrentHashMap;
+
+import io.netty.channel.ChannelId;
+import org.apache.shiro.subject.Subject;
+
+import org.apache.geode.internal.security.SecurityService;
+import org.apache.geode.security.ResourcePermission;
+
+/**
+ * This class is a thin wrapper around Geode's {@link SecurityService} and delegates
+ * login (authenticate), logout and authorize calls. It exists as a central point to track whether
+ * a given channel (connection) has previously been authenticated.
+ */
+public class SecurityServiceWrapper {
+
+ private final Map<String, Subject> subjects = new ConcurrentHashMap<>();
+
+ private final SecurityService securityService;
+
+ public SecurityServiceWrapper(SecurityService securityService) {
+ this.securityService = securityService;
+ }
+
+ public boolean isEnabled() {
+ return securityService.isIntegratedSecurity();
+ }
+
+ public boolean isAuthenticated(ChannelId channelId) {
+ return subjects.containsKey(channelId.asShortText());
+ }
+
+ public Subject login(ChannelId channelId, Properties properties) {
+ Subject subject = securityService.login(properties);
+ subjects.put(channelId.asShortText(), subject);
+ return subject;
+ }
+
+ public void logout(ChannelId channelId) {
+ subjects.remove(channelId.asShortText());
Review comment:
should this also call securityService.logout?
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/services/SecurityServiceWrapper.java
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.redis.internal.services;
+
+import java.util.Map;
+import java.util.Properties;
+import java.util.concurrent.ConcurrentHashMap;
+
+import io.netty.channel.ChannelId;
+import org.apache.shiro.subject.Subject;
+
+import org.apache.geode.internal.security.SecurityService;
+import org.apache.geode.security.ResourcePermission;
+
+/**
+ * This class is a thin wrapper around Geode's {@link SecurityService} and delegates
+ * login (authenticate), logout and authorize calls. It exists as a central point to track whether
+ * a given channel (connection) has previously been authenticated.
+ */
+public class SecurityServiceWrapper {
Review comment:
Given that this class does not implement the SecurityService interface and has its own Map it seems like it is more than a Wrapper. It seems like it is the entity that manages security for geode-for-redis. I don't think you need to expose in its name that it delegates to geode's SecurityService. I like that it is in redis's services package (I wish that all the stripe stuff was in a subpackage of services and that some of our other "services" were in this package). Could it just be named RedisSecurityService?
--
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] jdeppe-pivotal commented on a change in pull request #6994: GEODE-9676: Limit array and string sizes for unauthenticated Radish connections
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6994:
URL: https://github.com/apache/geode/pull/6994#discussion_r731232871
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/services/SecurityServiceWrapper.java
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.redis.internal.services;
+
+import java.util.Map;
+import java.util.Properties;
+import java.util.concurrent.ConcurrentHashMap;
+
+import io.netty.channel.ChannelId;
+import org.apache.shiro.subject.Subject;
+
+import org.apache.geode.internal.security.SecurityService;
+import org.apache.geode.security.ResourcePermission;
+
+/**
+ * This class is a thin wrapper around Geode's {@link SecurityService} and delegates
+ * login (authenticate), logout and authorize calls. It exists as a central point to track whether
+ * a given channel (connection) has previously been authenticated.
+ */
+public class SecurityServiceWrapper {
+
+ private final Map<String, Subject> subjects = new ConcurrentHashMap<>();
+
+ private final SecurityService securityService;
+
+ public SecurityServiceWrapper(SecurityService securityService) {
+ this.securityService = securityService;
+ }
+
+ public boolean isEnabled() {
+ return securityService.isIntegratedSecurity();
+ }
+
+ public boolean isAuthenticated(ChannelId channelId) {
+ return subjects.containsKey(channelId.asShortText());
+ }
+
+ public Subject login(ChannelId channelId, Properties properties) {
+ Subject subject = securityService.login(properties);
+ subjects.put(channelId.asShortText(), subject);
+ return subject;
+ }
+
+ public void logout(ChannelId channelId) {
+ subjects.remove(channelId.asShortText());
Review comment:
Previously `ExecutionHandlerContext.logout` was the only caller of this method and was performing a logout of the `Subject`. I've moved `Subject.logout()` into this method now to consolidate the logic a bit.
--
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] DonalEvans commented on a change in pull request #6994: GEODE-9676: Limit array and string sizes for unauthenticated Radish connections
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6994:
URL: https://github.com/apache/geode/pull/6994#discussion_r728516776
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/connection/AbstractAuthIntegrationTest.java
##########
@@ -128,4 +147,137 @@ public void givenNoSecurity_accessWithoutAuth_passes() throws Exception {
assertThat(jedis.ping()).isEqualTo("PONG");
}
+ @Test
+ public void givenSecurity_largeMultiBulkRequestsFail_whenNotAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+
+ try (Socket clientSocket = new Socket(BIND_ADDRESS, getPort())) {
+ clientSocket.setSoTimeout(1000);
+ PrintWriter out = new PrintWriter(clientSocket.getOutputStream());
+ BufferedReader in = new BufferedReader(new InputStreamReader(clientSocket.getInputStream()));
+
+ out.write("*100\r\n");
+ out.flush();
+ String response = in.readLine();
+
+ assertThat(response).contains(ERROR_UNAUTHENTICATED_MULTIBULK);
+ }
+ }
+
+ @Test
+ public void givenSecurity_largeMultiBulkRequestsSucceed_whenAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+
+ List<String> msetArgs = new ArrayList<>();
+ for (int i = 0; i < ByteToCommandDecoder.UNAUTHENTICATED_MAX_ARRAY_SIZE; i++) {
+ msetArgs.add("{hash}key-" + i);
+ msetArgs.add("value-" + i);
+ }
+
+ assertThat(jedis.auth(getUsername(), getPassword())).isEqualTo("OK");
+ assertThat(jedis.mset(msetArgs.toArray(new String[] {}))).isEqualTo("OK");
+ }
+
+ @Test
+ public void givenNoSecurity_largeMultiBulkRequestsSucceed_whenNotAuthenticated()
+ throws Exception {
+ setupCacheWithoutSecurity();
+
+ List<String> msetArgs = new ArrayList<>();
+ for (int i = 0; i < ByteToCommandDecoder.UNAUTHENTICATED_MAX_ARRAY_SIZE; i++) {
+ msetArgs.add("{hash}key-" + i);
+ msetArgs.add("value-" + i);
+ }
+
+ assertThat(jedis.mset(msetArgs.toArray(new String[] {}))).isEqualTo("OK");
+ }
+
+ @Test
+ public void givenSecurity_largeBulkStringRequestsFail_whenNotAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+
+ try (Socket clientSocket = new Socket(BIND_ADDRESS, getPort())) {
+ clientSocket.setSoTimeout(1000);
+ PrintWriter out = new PrintWriter(clientSocket.getOutputStream());
+ BufferedReader in = new BufferedReader(new InputStreamReader(clientSocket.getInputStream()));
+
+ out.write("*1\r\n$100000000\r\n");
+ out.flush();
+ String response = in.readLine();
+
+ assertThat(response).contains(ERROR_UNAUTHENTICATED_BULK);
+ }
+ }
+
+ @Test
+ public void givenSecurity_largeBulkStringRequestsSucceed_whenAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+ int stringSize = ByteToCommandDecoder.UNAUTHENTICATED_MAX_BULK_STRING_LENGTH + 1;
+
+ StringBuilder largeString = new StringBuilder(stringSize);
+ for (int i = 0; i < stringSize; i++) {
+ largeString.append("a");
+ }
+
+ assertThat(jedis.auth(getUsername(), getPassword())).isEqualTo("OK");
+ assertThat(jedis.set("key", largeString.toString())).isEqualTo("OK");
+ }
+
+ @Test
+ public void givenNoSecurity_largeBulkStringRequestsSucceed_whenNotAuthenticated()
+ throws Exception {
+ setupCacheWithoutSecurity();
+ int stringSize = ByteToCommandDecoder.UNAUTHENTICATED_MAX_BULK_STRING_LENGTH + 1;
+
+ StringBuilder largeString = new StringBuilder(stringSize);
+ for (int i = 0; i < stringSize; i++) {
+ largeString.append("a");
+ }
+
+ assertThat(jedis.set("key", largeString.toString())).isEqualTo("OK");
+ }
+
+ @Test
+ public void givenSecurity_closingConnectionLogsClientOut() throws Exception {
+ setupCacheWithSecurity();
+
+ int localPort = AvailablePortHelper.getRandomAvailableTCPPort();
+
+ try (Socket clientSocket = new Socket(BIND_ADDRESS, getPort(), InetAddress.getLoopbackAddress(),
+ localPort)) {
+ clientSocket.setSoTimeout(1000);
+ PrintWriter out = new PrintWriter(clientSocket.getOutputStream());
+ BufferedReader in = new BufferedReader(new InputStreamReader(clientSocket.getInputStream()));
+
+ out.write("*3\r\n$4\r\nAUTH\r\n" +
+ "$" + getUsername().length() + "\r\n" + getUsername() + "\r\n" +
+ "$" + getPassword().length() + "\r\n" + getPassword() + "\r\n");
+ out.flush();
+ String response = in.readLine();
+
+ assertThat(response).contains("OK");
+ }
+
+ AtomicReference<Socket> socketRef = new AtomicReference<>(null);
+
+ await().pollInterval(Duration.ofSeconds(1))
+ .untilAsserted(() -> assertThatNoException().isThrownBy(() -> socketRef.set(
+ new Socket(BIND_ADDRESS, getPort(), InetAddress.getLoopbackAddress(), localPort))));
+
+ Socket clientSocket = socketRef.get();
+ try {
Review comment:
This can be:
```
try (Socket clientSocket = socketRef.get()) {
```
which removes the need for the `finally` block.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/connection/AbstractAuthIntegrationTest.java
##########
@@ -128,4 +147,137 @@ public void givenNoSecurity_accessWithoutAuth_passes() throws Exception {
assertThat(jedis.ping()).isEqualTo("PONG");
}
+ @Test
+ public void givenSecurity_largeMultiBulkRequestsFail_whenNotAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+
+ try (Socket clientSocket = new Socket(BIND_ADDRESS, getPort())) {
+ clientSocket.setSoTimeout(1000);
+ PrintWriter out = new PrintWriter(clientSocket.getOutputStream());
+ BufferedReader in = new BufferedReader(new InputStreamReader(clientSocket.getInputStream()));
+
+ out.write("*100\r\n");
+ out.flush();
+ String response = in.readLine();
+
+ assertThat(response).contains(ERROR_UNAUTHENTICATED_MULTIBULK);
+ }
+ }
+
+ @Test
+ public void givenSecurity_largeMultiBulkRequestsSucceed_whenAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+
+ List<String> msetArgs = new ArrayList<>();
+ for (int i = 0; i < ByteToCommandDecoder.UNAUTHENTICATED_MAX_ARRAY_SIZE; i++) {
+ msetArgs.add("{hash}key-" + i);
+ msetArgs.add("value-" + i);
+ }
+
+ assertThat(jedis.auth(getUsername(), getPassword())).isEqualTo("OK");
+ assertThat(jedis.mset(msetArgs.toArray(new String[] {}))).isEqualTo("OK");
+ }
+
+ @Test
+ public void givenNoSecurity_largeMultiBulkRequestsSucceed_whenNotAuthenticated()
+ throws Exception {
+ setupCacheWithoutSecurity();
+
+ List<String> msetArgs = new ArrayList<>();
+ for (int i = 0; i < ByteToCommandDecoder.UNAUTHENTICATED_MAX_ARRAY_SIZE; i++) {
+ msetArgs.add("{hash}key-" + i);
+ msetArgs.add("value-" + i);
+ }
+
+ assertThat(jedis.mset(msetArgs.toArray(new String[] {}))).isEqualTo("OK");
+ }
+
+ @Test
+ public void givenSecurity_largeBulkStringRequestsFail_whenNotAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+
+ try (Socket clientSocket = new Socket(BIND_ADDRESS, getPort())) {
+ clientSocket.setSoTimeout(1000);
+ PrintWriter out = new PrintWriter(clientSocket.getOutputStream());
+ BufferedReader in = new BufferedReader(new InputStreamReader(clientSocket.getInputStream()));
+
+ out.write("*1\r\n$100000000\r\n");
+ out.flush();
+ String response = in.readLine();
+
+ assertThat(response).contains(ERROR_UNAUTHENTICATED_BULK);
+ }
+ }
+
+ @Test
+ public void givenSecurity_largeBulkStringRequestsSucceed_whenAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+ int stringSize = ByteToCommandDecoder.UNAUTHENTICATED_MAX_BULK_STRING_LENGTH + 1;
+
+ StringBuilder largeString = new StringBuilder(stringSize);
+ for (int i = 0; i < stringSize; i++) {
+ largeString.append("a");
+ }
Review comment:
This can be simplified to:
```
String largeString = StringUtils.repeat('a', stringSize);
```
--
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] jdeppe-pivotal commented on a change in pull request #6994: GEODE-9676: Limit array and string sizes for unauthenticated Radish connections
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6994:
URL: https://github.com/apache/geode/pull/6994#discussion_r728551703
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/connection/AbstractAuthIntegrationTest.java
##########
@@ -128,4 +147,137 @@ public void givenNoSecurity_accessWithoutAuth_passes() throws Exception {
assertThat(jedis.ping()).isEqualTo("PONG");
}
+ @Test
+ public void givenSecurity_largeMultiBulkRequestsFail_whenNotAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+
+ try (Socket clientSocket = new Socket(BIND_ADDRESS, getPort())) {
+ clientSocket.setSoTimeout(1000);
+ PrintWriter out = new PrintWriter(clientSocket.getOutputStream());
+ BufferedReader in = new BufferedReader(new InputStreamReader(clientSocket.getInputStream()));
+
+ out.write("*100\r\n");
+ out.flush();
+ String response = in.readLine();
+
+ assertThat(response).contains(ERROR_UNAUTHENTICATED_MULTIBULK);
+ }
+ }
+
+ @Test
+ public void givenSecurity_largeMultiBulkRequestsSucceed_whenAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+
+ List<String> msetArgs = new ArrayList<>();
+ for (int i = 0; i < ByteToCommandDecoder.UNAUTHENTICATED_MAX_ARRAY_SIZE; i++) {
+ msetArgs.add("{hash}key-" + i);
+ msetArgs.add("value-" + i);
+ }
+
+ assertThat(jedis.auth(getUsername(), getPassword())).isEqualTo("OK");
+ assertThat(jedis.mset(msetArgs.toArray(new String[] {}))).isEqualTo("OK");
+ }
+
+ @Test
+ public void givenNoSecurity_largeMultiBulkRequestsSucceed_whenNotAuthenticated()
+ throws Exception {
+ setupCacheWithoutSecurity();
+
+ List<String> msetArgs = new ArrayList<>();
+ for (int i = 0; i < ByteToCommandDecoder.UNAUTHENTICATED_MAX_ARRAY_SIZE; i++) {
+ msetArgs.add("{hash}key-" + i);
+ msetArgs.add("value-" + i);
+ }
+
+ assertThat(jedis.mset(msetArgs.toArray(new String[] {}))).isEqualTo("OK");
+ }
+
+ @Test
+ public void givenSecurity_largeBulkStringRequestsFail_whenNotAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+
+ try (Socket clientSocket = new Socket(BIND_ADDRESS, getPort())) {
+ clientSocket.setSoTimeout(1000);
+ PrintWriter out = new PrintWriter(clientSocket.getOutputStream());
+ BufferedReader in = new BufferedReader(new InputStreamReader(clientSocket.getInputStream()));
+
+ out.write("*1\r\n$100000000\r\n");
+ out.flush();
+ String response = in.readLine();
+
+ assertThat(response).contains(ERROR_UNAUTHENTICATED_BULK);
+ }
+ }
+
+ @Test
+ public void givenSecurity_largeBulkStringRequestsSucceed_whenAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+ int stringSize = ByteToCommandDecoder.UNAUTHENTICATED_MAX_BULK_STRING_LENGTH + 1;
+
+ StringBuilder largeString = new StringBuilder(stringSize);
+ for (int i = 0; i < stringSize; i++) {
+ largeString.append("a");
+ }
Review comment:
Thanks - I knew there had to be a better way.
##########
File path: geode-for-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/connection/AbstractAuthIntegrationTest.java
##########
@@ -128,4 +147,137 @@ public void givenNoSecurity_accessWithoutAuth_passes() throws Exception {
assertThat(jedis.ping()).isEqualTo("PONG");
}
+ @Test
+ public void givenSecurity_largeMultiBulkRequestsFail_whenNotAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+
+ try (Socket clientSocket = new Socket(BIND_ADDRESS, getPort())) {
+ clientSocket.setSoTimeout(1000);
+ PrintWriter out = new PrintWriter(clientSocket.getOutputStream());
+ BufferedReader in = new BufferedReader(new InputStreamReader(clientSocket.getInputStream()));
+
+ out.write("*100\r\n");
+ out.flush();
+ String response = in.readLine();
+
+ assertThat(response).contains(ERROR_UNAUTHENTICATED_MULTIBULK);
+ }
+ }
+
+ @Test
+ public void givenSecurity_largeMultiBulkRequestsSucceed_whenAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+
+ List<String> msetArgs = new ArrayList<>();
+ for (int i = 0; i < ByteToCommandDecoder.UNAUTHENTICATED_MAX_ARRAY_SIZE; i++) {
+ msetArgs.add("{hash}key-" + i);
+ msetArgs.add("value-" + i);
+ }
+
+ assertThat(jedis.auth(getUsername(), getPassword())).isEqualTo("OK");
+ assertThat(jedis.mset(msetArgs.toArray(new String[] {}))).isEqualTo("OK");
+ }
+
+ @Test
+ public void givenNoSecurity_largeMultiBulkRequestsSucceed_whenNotAuthenticated()
+ throws Exception {
+ setupCacheWithoutSecurity();
+
+ List<String> msetArgs = new ArrayList<>();
+ for (int i = 0; i < ByteToCommandDecoder.UNAUTHENTICATED_MAX_ARRAY_SIZE; i++) {
+ msetArgs.add("{hash}key-" + i);
+ msetArgs.add("value-" + i);
+ }
+
+ assertThat(jedis.mset(msetArgs.toArray(new String[] {}))).isEqualTo("OK");
+ }
+
+ @Test
+ public void givenSecurity_largeBulkStringRequestsFail_whenNotAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+
+ try (Socket clientSocket = new Socket(BIND_ADDRESS, getPort())) {
+ clientSocket.setSoTimeout(1000);
+ PrintWriter out = new PrintWriter(clientSocket.getOutputStream());
+ BufferedReader in = new BufferedReader(new InputStreamReader(clientSocket.getInputStream()));
+
+ out.write("*1\r\n$100000000\r\n");
+ out.flush();
+ String response = in.readLine();
+
+ assertThat(response).contains(ERROR_UNAUTHENTICATED_BULK);
+ }
+ }
+
+ @Test
+ public void givenSecurity_largeBulkStringRequestsSucceed_whenAuthenticated() throws Exception {
+ setupCacheWithSecurity();
+ int stringSize = ByteToCommandDecoder.UNAUTHENTICATED_MAX_BULK_STRING_LENGTH + 1;
+
+ StringBuilder largeString = new StringBuilder(stringSize);
+ for (int i = 0; i < stringSize; i++) {
+ largeString.append("a");
+ }
+
+ assertThat(jedis.auth(getUsername(), getPassword())).isEqualTo("OK");
+ assertThat(jedis.set("key", largeString.toString())).isEqualTo("OK");
+ }
+
+ @Test
+ public void givenNoSecurity_largeBulkStringRequestsSucceed_whenNotAuthenticated()
+ throws Exception {
+ setupCacheWithoutSecurity();
+ int stringSize = ByteToCommandDecoder.UNAUTHENTICATED_MAX_BULK_STRING_LENGTH + 1;
+
+ StringBuilder largeString = new StringBuilder(stringSize);
+ for (int i = 0; i < stringSize; i++) {
+ largeString.append("a");
+ }
+
+ assertThat(jedis.set("key", largeString.toString())).isEqualTo("OK");
+ }
+
+ @Test
+ public void givenSecurity_closingConnectionLogsClientOut() throws Exception {
+ setupCacheWithSecurity();
+
+ int localPort = AvailablePortHelper.getRandomAvailableTCPPort();
+
+ try (Socket clientSocket = new Socket(BIND_ADDRESS, getPort(), InetAddress.getLoopbackAddress(),
+ localPort)) {
+ clientSocket.setSoTimeout(1000);
+ PrintWriter out = new PrintWriter(clientSocket.getOutputStream());
+ BufferedReader in = new BufferedReader(new InputStreamReader(clientSocket.getInputStream()));
+
+ out.write("*3\r\n$4\r\nAUTH\r\n" +
+ "$" + getUsername().length() + "\r\n" + getUsername() + "\r\n" +
+ "$" + getPassword().length() + "\r\n" + getPassword() + "\r\n");
+ out.flush();
+ String response = in.readLine();
+
+ assertThat(response).contains("OK");
+ }
+
+ AtomicReference<Socket> socketRef = new AtomicReference<>(null);
+
+ await().pollInterval(Duration.ofSeconds(1))
+ .untilAsserted(() -> assertThatNoException().isThrownBy(() -> socketRef.set(
+ new Socket(BIND_ADDRESS, getPort(), InetAddress.getLoopbackAddress(), localPort))));
+
+ Socket clientSocket = socketRef.get();
+ try {
Review comment:
Nice.
--
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 change in pull request #6994: GEODE-9676: Limit array and string sizes for unauthenticated Radish connections
Posted by GitBox <gi...@apache.org>.
upthewaterspout commented on a change in pull request #6994:
URL: https://github.com/apache/geode/pull/6994#discussion_r731289740
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/netty/ByteToCommandDecoder.java
##########
@@ -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";
Review comment:
I wonder if these properties should be prefixed `geode.` and not `gemfire.`?
--
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] dschneider-pivotal commented on a change in pull request #6994: GEODE-9676: Limit array and string sizes for unauthenticated Radish connections
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6994:
URL: https://github.com/apache/geode/pull/6994#discussion_r731125065
##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/services/SecurityServiceWrapper.java
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.redis.internal.services;
+
+import java.util.Map;
+import java.util.Properties;
+import java.util.concurrent.ConcurrentHashMap;
+
+import io.netty.channel.ChannelId;
+import org.apache.shiro.subject.Subject;
+
+import org.apache.geode.internal.security.SecurityService;
+import org.apache.geode.security.ResourcePermission;
+
+/**
+ * This class is a thin wrapper around Geode's {@link SecurityService} and delegates
+ * login (authenticate), logout and authorize calls. It exists as a central point to track whether
+ * a given channel (connection) has previously been authenticated.
+ */
+public class SecurityServiceWrapper {
Review comment:
Add a unit test for this new class
--
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