[GitHub] [geode] upthewaterspout commented on a change in pull request #6844: GEODE-9546: Integrate Security Manager into Radish AUTH flow

[GitBox <gi...@apache.org>]

upthewaterspout commented on a change in pull request #6844:
URL: https://github.com/apache/geode/pull/6844#discussion_r704654640



##########
File path: geode-apis-compatible-with-redis/README.md
##########
@@ -91,68 +94,139 @@ To confirm that everything shut down correctly, if you execute a Redis command i
 Could not connect to Redis at 127.0.0.1:6379: Connection refused 
 not connected>
 ```
-### <a name="redis-commands"></a>Redis Commands
+
+## <a name="security"></a>Security
+
+Security is implemented slightly differently to OSS Redis. Redis stores password information in plain text in the redis.conf file.     
+
+When using Apache Geode, to enable security, a Security Manager needs to be configured on the server(s). This Security Manager will authenticate `AUTH <password>` commands and `AUTH <username> <password>` commands. Users can set a custom `default` username using the `geode-compatible-with-redis-username` parameter. This username will be used when `AUTH <password>` commands are sent without a `<username>`. 
+
+The following gfsh command will configure a `SimpleSecurityManager`:

Review comment:
       Maybe we should demo the `ExampleSecurityManager` instead - which at least lets you set the password?
   
   We should also mention what permission the user needs to have given to them - eg DATA:WRITE:REDIS_DATA.

##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/connection/AuthExecutor.java
##########
@@ -15,35 +15,55 @@
  */
 package org.apache.geode.redis.internal.executor.connection;
 
-import java.util.Arrays;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_AUTH_CALLED_WITHOUT_PASSWORD_CONFIGURED;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_INVALID_USERNAME_OR_PASSWORD;
+import static org.apache.geode.redis.internal.netty.Coder.bytesToString;
+
 import java.util.List;
+import java.util.Properties;
 
-import org.apache.geode.redis.internal.RedisConstants;
 import org.apache.geode.redis.internal.executor.Executor;
 import org.apache.geode.redis.internal.executor.RedisResponse;
 import org.apache.geode.redis.internal.netty.Command;
 import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+import org.apache.geode.security.AuthenticationFailedException;
+import org.apache.geode.security.SecurityManager;
 
 public class AuthExecutor implements Executor {
 
   @Override
-  public RedisResponse executeCommand(Command command,
-      ExecutionHandlerContext context) {
+  public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) {
     List<byte[]> commandElems = command.getProcessedCommand();
 
-    byte[] password = context.getAuthPassword();
-    if (password == null) {
-      return RedisResponse.error(RedisConstants.ERROR_NO_PASS);
+    SecurityManager securityManager = context.getSecurityManager();
+    Properties props = new Properties();
+    if (commandElems.size() == 2) {
+      if (securityManager == null) {
+        return RedisResponse.error(ERROR_AUTH_CALLED_WITHOUT_PASSWORD_CONFIGURED);
+      }
+      props.setProperty("security-username", context.getRedisUsername());
+      props.setProperty("security-password", bytesToString(commandElems.get(1)));
+    } else {
+      if (securityManager == null) {
+        String receivedUsername = new String(commandElems.get(1));
+        if (receivedUsername.equalsIgnoreCase(context.getRedisUsername())) {
+          return RedisResponse.ok();

Review comment:
       Yesterday we discussed that we think we ought to deviate for the native redis behavior here. If there is no security manager, *any* attempt to call auth should fail. Hopefully that can make the logic in this method a bit simpler.

##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/AbstractDistributionConfig.java
##########
@@ -1317,7 +1317,7 @@ static Class _getAttributeType(String attName) {
         "Specifies the address on which the Redis API for Geode is listening. If set to the empty string or this property is not specified, localhost is requested from the operating system.");
     m.put(REDIS_ENABLED,
         "When the default value of false, the Redis API for Geode is not available.  Set to true to enable the Redis API for Geode.");
-    m.put(REDIS_PASSWORD,
+    m.put(REDIS_USERNAME,
         "Specifies the password that the server uses when a client attempts to authenticate. The default is none and no authentication will be required.");

Review comment:
       I think you need to change the description here as well - it still refers to a password.

##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/executor/connection/AuthExecutor.java
##########
@@ -15,35 +15,55 @@
  */
 package org.apache.geode.redis.internal.executor.connection;
 
-import java.util.Arrays;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_AUTH_CALLED_WITHOUT_PASSWORD_CONFIGURED;
+import static org.apache.geode.redis.internal.RedisConstants.ERROR_INVALID_USERNAME_OR_PASSWORD;
+import static org.apache.geode.redis.internal.netty.Coder.bytesToString;
+
 import java.util.List;
+import java.util.Properties;
 
-import org.apache.geode.redis.internal.RedisConstants;
 import org.apache.geode.redis.internal.executor.Executor;
 import org.apache.geode.redis.internal.executor.RedisResponse;
 import org.apache.geode.redis.internal.netty.Command;
 import org.apache.geode.redis.internal.netty.ExecutionHandlerContext;
+import org.apache.geode.security.AuthenticationFailedException;
+import org.apache.geode.security.SecurityManager;
 
 public class AuthExecutor implements Executor {
 
   @Override
-  public RedisResponse executeCommand(Command command,
-      ExecutionHandlerContext context) {
+  public RedisResponse executeCommand(Command command, ExecutionHandlerContext context) {
     List<byte[]> commandElems = command.getProcessedCommand();
 
-    byte[] password = context.getAuthPassword();
-    if (password == null) {
-      return RedisResponse.error(RedisConstants.ERROR_NO_PASS);
+    SecurityManager securityManager = context.getSecurityManager();
+    Properties props = new Properties();
+    if (commandElems.size() == 2) {
+      if (securityManager == null) {
+        return RedisResponse.error(ERROR_AUTH_CALLED_WITHOUT_PASSWORD_CONFIGURED);
+      }
+      props.setProperty("security-username", context.getRedisUsername());
+      props.setProperty("security-password", bytesToString(commandElems.get(1)));

Review comment:
       I think you can use the constants from `SecurityManager` for strings like "security-username"




-- 
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