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

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

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



##########
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) {

Review comment:
       nitpick: probably do early returns to avoid nested if/elses.

##########
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();

Review comment:
       Is it possible to get the securtyService out of context? SecurityManager is supposed to be an internal component used by the SecurityService, the product should interact only with SecurityService. 

##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/netty/NettyRedisServer.java
##########
@@ -86,18 +85,20 @@
   private final Channel serverChannel;
   private final int serverPort;
   private final DistributedMember member;
+  private final SecurityManager securityManager;
 
   public NettyRedisServer(Supplier<DistributionConfig> configSupplier,
       RegionProvider regionProvider, PubSub pubsub, Supplier<Boolean> allowUnsupportedSupplier,
       Runnable shutdownInvoker, int port, String requestedAddress, RedisStats redisStats,
-      DistributedMember member) {
+      DistributedMember member, SecurityManager securityManager) {

Review comment:
       Maybe we can pass in the SecurityService here

##########
File path: geode-apis-compatible-with-redis/README.md
##########
@@ -56,14 +53,40 @@ Your Geode instance should now be up and running (1 locator and 1 server) and re
 To confirm the server is listening, in a separate terminal run:
 
 ```console
-redis-cli -h <compatibleWithRedisBindAddress> -p <compatibleWithRedisPort> -a <compatibleWithRedisPassword> ping
+redis-cli -h <compatibleWithRedisBindAddress> -p <compatibleWithRedisPort> ping
 ```
 
 - Replace `<compatibleWithRedisBindAddress>`, `<compatibleWithRedisPort>`, and `<compatibleWithRedisPassword>` with the same values as the server.
 
 If the server is functioning properly, you should see a response of `PONG`.
 
-### <a name="adding-a-server"></a>Optional - Adding an additional Geode server with compatible with Redis APIS
+## <a name="security"></a>Security
+
+Security is implemented slightly differently to Redis. To enable security, a Security Manager needs to be configured. This Security Manager will authenticate `AUTH username password` commands. Instead of a default, system-wide password, a default username can be set using the `geode-compatible-with-redis-username` parameter. This username is used when `AUTH` commands are sent with only a password.
+
+For example, the following gfsh command will configure a `SimpleSecurityManager`:
+
+```console
+start server \
+  --name=<serverName> \
+  --locators=<locatorPort> \
+  --compatible-with-redis-port=<compatibleWithRedisPort> \
+  --compatible-with-redis-bind-address=<compatibleWithRedisBindAddress> \
+  --compatible-with-redis-username=<compatibleWithRedisUsername> \
+  --J=-Dgemfire.security-manager=org.apache.geode.examples.SimpleSecurityManager
+```
+
+To confirm that the server is working, in a separate terminal run:
+
+```console
+redis-cli -h <compatibleWithRedisBindAddress> -p <compatibleWithRedisPort> --user <compatibleWithRedisUsername> -a <compatibleWithRedisUsername> ping

Review comment:
       --user and -a both takes "compatibleWithRedisUsername"?




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