Always issue HELLO when the server should understand it, not just for RESP3

docs/Configuration.md

@@ -299,13 +299,17 @@ config.ReconnectRetryPolicy = new LinearRetry(5000);
 
 ## Redis protocol
 
-Without specific configuration, StackExchange.Redis will use the RESP2 protocol; this means that pub/sub requires a separate connection to the server. RESP3 is a newer protocol
-(usually, but not always, available on v6 servers and above) which allows (among other changes) pub/sub messages to be communicated on the *same* connection - which can be very
-desirable in servers with a large number of clients. The protocol handshake needs to happen very early in the connection, so *by default* the library does not attempt a RESP3 connection
-unless it has reason to expect it to work. 
+RESP3 is a newer protocol (available on v6 servers and above) which allows (among other changes) pub/sub messages to be communicated on the *same* connection - which can be very
+desirable in servers with a large number of clients; under RESP2, pub/sub requires a separate connection to the server. The protocol handshake needs to happen very early in the
+connection, so the library only attempts RESP3 when it has reason to expect it to work.
 
 The library determines whether to use RESP3 by:
 - The `HELLO` command has been disabled: RESP2 is used
 - A protocol *other than* `resp3` or `3` is specified: RESP2 is used
 - A protocol of `resp3` or `3` is specified: RESP3 is attempted (with fallback if it fails)
-- In all other scenarios: RESP2 is used
+- Otherwise: RESP3 is attempted if `defaultVersion` (6.0 unless overridden) is v6 or above
+
+Note that `HELLO` is issued either way: `HELLO 2` when staying on RESP2. The reply tells us the server version, replication role, mode (standalone/sentinel/cluster) and connection
+identifier, none of which we would otherwise know without `INFO` or `CONFIG GET` - both of which are in the `@dangerous` ACL category, and are commonly restricted. `HELLO` itself is
+in the `@connection` category. If you need to prevent it (for example when talking to a proxy that does not understand it), disable it at the command-map level with `$hello=` in the
+configuration string - or specify a `defaultVersion` below 6.0, since `HELLO` did not exist before then.

src/StackExchange.Redis/CommandMap.cs

@@ -46,6 +46,9 @@ private CommandMap(CommandBytes[] map, byte[] bytes)
 
             RedisCommand.ECHO, RedisCommand.SELECT,
 
+            // twemproxy *closes the connection* on an unsupported command, so HELLO is fatal, not just useless
+            RedisCommand.HELLO,
+
             RedisCommand.BGREWRITEAOF, RedisCommand.BGSAVE, RedisCommand.CLIENT, RedisCommand.CLUSTER, RedisCommand.CONFIG, RedisCommand.DBSIZE,
             RedisCommand.DEBUG, RedisCommand.FLUSHALL, RedisCommand.FLUSHDB, RedisCommand.INFO, RedisCommand.LASTSAVE, RedisCommand.MONITOR, RedisCommand.REPLICAOF,
             RedisCommand.SAVE, RedisCommand.SHUTDOWN, RedisCommand.SLAVEOF, RedisCommand.SLOWLOG, RedisCommand.SYNC, RedisCommand.TIME, RedisCommand.HOTKEYS,
@@ -70,6 +73,11 @@ private CommandMap(CommandBytes[] map, byte[] bytes)
 
             RedisCommand.SELECT,
 
+            // envoy either rejects HELLO ("unsupported command", up to ~1.31) or proxies it to an arbitrary
+            // backend node (1.39+), so the version/role/mode it reports describe *some* server, not the endpoint
+            // we're talking to; either way we don't want it
+            RedisCommand.HELLO,
+
             RedisCommand.BGREWRITEAOF, RedisCommand.BGSAVE, RedisCommand.CLIENT, RedisCommand.CLUSTER, RedisCommand.CONFIG, RedisCommand.DBSIZE,
             RedisCommand.DEBUG, RedisCommand.FLUSHALL, RedisCommand.FLUSHDB, RedisCommand.INFO, RedisCommand.LASTSAVE, RedisCommand.MONITOR, RedisCommand.REPLICAOF,
             RedisCommand.SAVE, RedisCommand.SHUTDOWN, RedisCommand.SLAVEOF, RedisCommand.SLOWLOG, RedisCommand.SYNC, RedisCommand.TIME, RedisCommand.HOTKEYS,

src/StackExchange.Redis/ConfigurationOptions.cs

@@ -1463,6 +1463,25 @@ internal bool TryResp3()
             return use3 && CommandMap.IsAvailable(RedisCommand.HELLO);
         }
 
+        /// <summary>
+        /// Determines whether to issue <c>HELLO</c> as part of the handshake, and at which protocol level.
+        /// </summary>
+        /// <remarks>We want HELLO even when staying on RESP2, because the reply tells us the server version,
+        /// role, mode and connection identifier - none of which we would otherwise know without <c>INFO</c>
+        /// or <c>CONFIG</c>, which are commonly restricted by ACLs (both are <c>@dangerous</c>).</remarks>
+        internal bool TryHello(out int protocolVersion)
+        {
+            // HELLO arrived in 6.0, at the same time as RESP3; the command-map and the assumed server
+            // version are therefore both ways of opting out (`$hello=` and `defaultVersion=5.0`, say)
+            if (CommandMap.IsAvailable(RedisCommand.HELLO) && new RedisFeatures(DefaultVersion).Hello)
+            {
+                protocolVersion = TryResp3() ? 3 : 2;
+                return true;
+            }
+            protocolVersion = 0;
+            return false;
+        }
+
         internal static bool TryParseRedisProtocol(string? value, out RedisProtocol protocol)
         {
             // accept raw integers too, but only trust them if we recognize them

src/StackExchange.Redis/Enums/ServerType.cs

@@ -70,7 +70,7 @@ internal static class ServerTypeExtensions
         };
 
         /// <summary>
-        /// Whether a server type supports <see cref="ServerEndPoint.AutoConfigureAsync(PhysicalConnection?, Microsoft.Extensions.Logging.ILogger?, CommandFlags)"/>.
+        /// Whether a server type supports <see cref="ServerEndPoint.AutoConfigureAsync(PhysicalConnection?, Microsoft.Extensions.Logging.ILogger?, CommandFlags, bool)"/>.
         /// </summary>
         internal static bool SupportsAutoConfigure(this ServerType type) => type switch
         {

src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt

@@ -1,6 +1,7 @@
-#nullable enable
+#nullable enable
 StackExchange.Redis.ConfigurationOptions.SentinelPassword.get -> string?
 StackExchange.Redis.ConfigurationOptions.SentinelPassword.set -> void
 StackExchange.Redis.ConfigurationOptions.SentinelUser.get -> string?
 StackExchange.Redis.ConfigurationOptions.SentinelUser.set -> void
+StackExchange.Redis.RedisFeatures.Hello.get -> bool
 [SER009]virtual StackExchange.Redis.Configuration.Tunnel.ConnectTransportAsync(System.Net.EndPoint! endpoint, StackExchange.Redis.ConnectionType connectionType, System.Threading.CancellationToken cancellationToken) -> System.Threading.Tasks.ValueTask<RESPite.Transports.DuplexTransport?>

src/StackExchange.Redis/RedisFeatures.cs

@@ -289,6 +289,13 @@ public RedisFeatures(Version version)
         /// </summary>
         public bool Resp3 => Version.IsAtLeast(v6_0_0);
 
+        /// <summary>
+        /// Is the <see href="https://redis.io/commands/hello/">HELLO</see> handshake available?
+        /// </summary>
+        /// <remarks>This is useful even when staying on RESP2; <c>HELLO 2</c> reports the server
+        /// version, role, mode and connection identifier without needing <c>INFO</c> or <c>CONFIG</c>.</remarks>
+        public bool Hello => Version.IsAtLeast(v6_0_0);
+
         /// <summary>
         /// Are the <c>IF*</c> modifiers on <see href="https://redis.io/commands/set/">SET</see> available?
         /// </summary>

src/StackExchange.Redis/ResultProcessor.cs

@@ -1103,14 +1103,21 @@ protected override bool SetResultCore(PhysicalConnection connection, Message mes
                                     connection.ConnectionId = i64;
                                     Log?.LogInformationAutoConfiguredHelloConnectionId(new(server), i64);
                                     break;
+                                // note: "mode" and "role" describe a *server*, so we only trust them when we're
+                                // talking to one directly; a proxy may answer HELLO from an arbitrary backend node
+                                // (envoy 1.39 does), and taking that at face value would flip us into cluster mode
+                                // or mark the proxy as a replica
                                 case HelloField.Mode
-                                    when iter.Value.TryParseScalar(&ServerTypeMetadata.TryParse, out ServerType serverType):
+                                    when server.ServerType.SupportsAutoConfigure()
+                                        && iter.Value.TryParseScalar(&ServerTypeMetadata.TryParse, out ServerType serverType):
                                     server.ServerType = serverType;
                                     Log?.LogInformationAutoConfiguredHelloServerType(new(server), serverType);
                                     break;
                                 case HelloField.Role
-                                    when iter.Value.TryParseScalar(&KnownRoleMetadata.TryParse, out bool isReplica):
+                                    when server.ServerType.SupportsAutoConfigure()
+                                        && iter.Value.TryParseScalar(&KnownRoleMetadata.TryParse, out bool isReplica):
                                     server.IsReplica = isReplica;
+                                    server.RoleKnownFromHello = true; // so we don't need the key-based fallback probe
                                     Log?.LogInformationAutoConfiguredHelloRole(
                                         new(server),
                                         isReplica ? "replica" : "primary");

src/StackExchange.Redis/ServerEndPoint.cs

@@ -388,7 +388,23 @@ internal void AddScript(string script, byte[] hash)
             }
         }
 
-        internal async Task AutoConfigureAsync(PhysicalConnection? connection, ILogger? log = null, CommandFlags extraFlags = CommandFlags.None)
+        /// <summary>
+        /// Whether <c>HELLO</c> has told us our replication role (and server mode) on the current connection.
+        /// </summary>
+        /// <remarks>Reset at the start of each handshake, and set when the <c>HELLO</c> reply is processed.</remarks>
+        internal bool RoleKnownFromHello { get; set; }
+
+        /// <summary>
+        /// Issues the topology/configuration discovery commands for this server.
+        /// </summary>
+        /// <param name="connection">The connection to write to; <c>null</c> for an already-established connection.</param>
+        /// <param name="log">The log to write handshake details to.</param>
+        /// <param name="extraFlags">Additional flags to apply to the messages issued.</param>
+        /// <param name="helloPending">
+        /// Whether a <c>HELLO</c> has been written to this same batch, but not yet answered; in that case
+        /// we expect to learn our role from it, so we can skip the key-based fallback probe.
+        /// </param>
+        internal async Task AutoConfigureAsync(PhysicalConnection? connection, ILogger? log = null, CommandFlags extraFlags = CommandFlags.None, bool helloPending = false)
         {
             if (!serverType.SupportsAutoConfigure())
             {
@@ -452,9 +468,11 @@ internal async Task AutoConfigureAsync(PhysicalConnection? connection, ILogger?
                     await WriteDirectOrQueueFireAndForgetAsync(connection, msg, autoConfigProcessor).ForAwait();
                 }
             }
-            else if (commandMap.IsAvailable(RedisCommand.SET))
+            else if (commandMap.IsAvailable(RedisCommand.SET) && !(helloPending || RoleKnownFromHello))
             {
                 // This is a nasty way to find if we are a replica, and it will only work on up-level servers, but...
+                // (note we only get here when HELLO isn't going to tell us: the HELLO reply carries "role", and
+                // unlike this probe it doesn't need a key - which matters when ACLs restrict key patterns; see #2968)
                 RedisKey key = Multiplexer.UniqueId;
                 // The actual value here doesn't matter (we detect the error code if it fails).
                 // The value here is to at least give some indication to anyone watching via "monitor",
@@ -1002,10 +1020,24 @@ private async Task HandshakeAsync(PhysicalConnection connection, ILogger? log)
             // the various tasks and just `return connection.FlushAsync();` - however, since handshake is low
             // volume, we can afford to optimize for a good stack-trace rather than avoiding state machines.
             ResultProcessor<bool>? autoConfig = null;
-            if (Multiplexer.RawConfig.TryResp3()) // note this includes an availability check on HELLO
+            bool isInteractive = connection.BridgeCouldBeNull?.ConnectionType == ConnectionType.Interactive;
+            if (isInteractive)
+            {
+                // forget what the previous connection's HELLO told us; re-established below, if this one repeats it
+                // (the subscription handshake is deliberately left out of this: it doesn't do the discovery step)
+                RoleKnownFromHello = false;
+            }
+
+            // HELLO comes in two flavours: as the RESP3 negotiation (which has to be the *first* command, and has to
+            // carry the credentials), or - when we're staying on RESP2 - purely for discovery, in which case it is
+            // issued *after* AUTH, below; see #2968 for why we want it even on RESP2
+            bool helloAvailable = Multiplexer.RawConfig.TryHello(out int helloProtocol); // includes an availability check on HELLO
+            bool negotiateResp3 = helloAvailable && helloProtocol >= 3;
+            bool discoveryHello = helloAvailable && !negotiateResp3 && isInteractive;
+            if (negotiateResp3)
             {
                 log?.LogInformationAuthenticatingViaHello(new(this));
-                var hello = Message.CreateHello(3, user, password, clientName, CommandFlags.FireAndForget | Message.NoFlushFlag);
+                var hello = Message.CreateHello(helloProtocol, user, password, clientName, CommandFlags.FireAndForget | Message.NoFlushFlag);
                 hello.SetInternalCall();
                 await WriteDirectOrQueueFireAndForgetAsync(connection, hello, autoConfig ??= ResultProcessor.AutoConfigureProcessor.Create(log)).ForAwait();
 
@@ -1014,7 +1046,7 @@ private async Task HandshakeAsync(PhysicalConnection connection, ILogger? log)
             }
             else
             {
-                // if we're not even issuing HELLO, we're RESP2
+                // whether or not we issue HELLO for discovery below, we're RESP2
                 connection.SetProtocol(RedisProtocol.Resp2);
             }
 
@@ -1082,7 +1114,18 @@ private async Task HandshakeAsync(PhysicalConnection connection, ILogger? log)
             var connType = bridge.ConnectionType;
             if (connType == ConnectionType.Interactive)
             {
-                await AutoConfigureAsync(connection, log, extraFlags: Message.NoFlushFlag).ForAwait();
+                if (discoveryHello)
+                {
+                    // a bare HELLO (no AUTH clause of its own): we're already authenticated by this point, and
+                    // folding the credentials in here would change how credential failures surface. We only want
+                    // the reply, which reports version/role/mode/connection-id without INFO or CONFIG (both
+                    // @dangerous, hence often ACL-restricted); see #2968
+                    var hello = Message.CreateHello(helloProtocol, null, null, null, CommandFlags.FireAndForget | Message.NoFlushFlag);
+                    hello.SetInternalCall();
+                    await WriteDirectOrQueueFireAndForgetAsync(connection, hello, autoConfig ??= ResultProcessor.AutoConfigureProcessor.Create(log)).ForAwait();
+                }
+
+                await AutoConfigureAsync(connection, log, extraFlags: Message.NoFlushFlag, helloPending: negotiateResp3 || discoveryHello).ForAwait();
             }
 
             // note that the final messages *are* flushed (no Message.NoFlushFlag)

tests/RedisConfigs/.docker/Envoy/Dockerfile

@@ -1,4 +1,4 @@
-FROM envoyproxy/envoy:v1.31-latest
+FROM envoyproxy/envoy:v1.39-latest
 
 COPY envoy.yaml /etc/envoy/envoy.yaml
 RUN chmod go+r /etc/envoy/envoy.yaml

tests/StackExchange.Redis.Tests/CommandMapUnitTests.cs

@@ -63,4 +63,17 @@ public void TryParseCI_RejectsNonCommands(string name)
         ReadOnlySpan<byte> bytes = Encoding.ASCII.GetBytes(name);
         Assert.False(RedisCommandMetadata.TryParseCI(bytes, out _), $"byte parse unexpectedly succeeded for '{name}'");
     }
+
+    /// <summary>
+    /// We now issue <c>HELLO</c> whenever it is available (RESP2 included), so the proxy maps must exclude it:
+    /// twemproxy 0.5.0 *closes the connection* on an unsupported command, and envoy (1.39, at least) forwards
+    /// HELLO to an arbitrary backend node, so its version/role/mode describe the wrong server.
+    /// </summary>
+    [Fact]
+    public void ProxyCommandMapsExcludeHello()
+    {
+        Assert.False(CommandMap.Twemproxy.IsAvailable(RedisCommand.HELLO), "twemproxy");
+        Assert.False(CommandMap.Envoyproxy.IsAvailable(RedisCommand.HELLO), "envoyproxy");
+        Assert.True(CommandMap.Default.IsAvailable(RedisCommand.HELLO), "default");
+    }
 }