diff --git a/docs/Configuration.md b/docs/Configuration.md index 5baf66760..4ad94935b 100644 --- a/docs/Configuration.md +++ b/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. diff --git a/src/StackExchange.Redis/CommandMap.cs b/src/StackExchange.Redis/CommandMap.cs index 24a1aec97..4b1386432 100644 --- a/src/StackExchange.Redis/CommandMap.cs +++ b/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, diff --git a/src/StackExchange.Redis/ConfigurationOptions.cs b/src/StackExchange.Redis/ConfigurationOptions.cs index 4860c5b07..12db0bac1 100644 --- a/src/StackExchange.Redis/ConfigurationOptions.cs +++ b/src/StackExchange.Redis/ConfigurationOptions.cs @@ -1463,6 +1463,25 @@ internal bool TryResp3() return use3 && CommandMap.IsAvailable(RedisCommand.HELLO); } + /// + /// Determines whether to issue HELLO as part of the handshake, and at which protocol level. + /// + /// 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 INFO + /// or CONFIG, which are commonly restricted by ACLs (both are @dangerous). + 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 diff --git a/src/StackExchange.Redis/Enums/ServerType.cs b/src/StackExchange.Redis/Enums/ServerType.cs index f364765e9..e7a84ad0a 100644 --- a/src/StackExchange.Redis/Enums/ServerType.cs +++ b/src/StackExchange.Redis/Enums/ServerType.cs @@ -70,7 +70,7 @@ internal static class ServerTypeExtensions }; /// - /// Whether a server type supports . + /// Whether a server type supports . /// internal static bool SupportsAutoConfigure(this ServerType type) => type switch { diff --git a/src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt b/src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt index 821995702..f05af89aa 100644 --- a/src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt +++ b/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 diff --git a/src/StackExchange.Redis/RedisFeatures.cs b/src/StackExchange.Redis/RedisFeatures.cs index a2a0c0726..3e30a65dc 100644 --- a/src/StackExchange.Redis/RedisFeatures.cs +++ b/src/StackExchange.Redis/RedisFeatures.cs @@ -289,6 +289,13 @@ public RedisFeatures(Version version) /// public bool Resp3 => Version.IsAtLeast(v6_0_0); + /// + /// Is the HELLO handshake available? + /// + /// This is useful even when staying on RESP2; HELLO 2 reports the server + /// version, role, mode and connection identifier without needing INFO or CONFIG. + public bool Hello => Version.IsAtLeast(v6_0_0); + /// /// Are the IF* modifiers on SET available? /// diff --git a/src/StackExchange.Redis/ResultProcessor.cs b/src/StackExchange.Redis/ResultProcessor.cs index 45a09cfbc..36e3045c9 100644 --- a/src/StackExchange.Redis/ResultProcessor.cs +++ b/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"); diff --git a/src/StackExchange.Redis/ServerEndPoint.cs b/src/StackExchange.Redis/ServerEndPoint.cs index ebf76262b..ee5cd3281 100644 --- a/src/StackExchange.Redis/ServerEndPoint.cs +++ b/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) + /// + /// Whether HELLO has told us our replication role (and server mode) on the current connection. + /// + /// Reset at the start of each handshake, and set when the HELLO reply is processed. + internal bool RoleKnownFromHello { get; set; } + + /// + /// Issues the topology/configuration discovery commands for this server. + /// + /// The connection to write to; null for an already-established connection. + /// The log to write handshake details to. + /// Additional flags to apply to the messages issued. + /// + /// Whether a HELLO 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. + /// + 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? 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) diff --git a/tests/RedisConfigs/.docker/Envoy/Dockerfile b/tests/RedisConfigs/.docker/Envoy/Dockerfile index 5c20d350c..c3a117e9d 100644 --- a/tests/RedisConfigs/.docker/Envoy/Dockerfile +++ b/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 diff --git a/tests/StackExchange.Redis.Tests/CommandMapUnitTests.cs b/tests/StackExchange.Redis.Tests/CommandMapUnitTests.cs index b3c9a7cfd..175c8d395 100644 --- a/tests/StackExchange.Redis.Tests/CommandMapUnitTests.cs +++ b/tests/StackExchange.Redis.Tests/CommandMapUnitTests.cs @@ -63,4 +63,17 @@ public void TryParseCI_RejectsNonCommands(string name) ReadOnlySpan bytes = Encoding.ASCII.GetBytes(name); Assert.False(RedisCommandMetadata.TryParseCI(bytes, out _), $"byte parse unexpectedly succeeded for '{name}'"); } + + /// + /// We now issue HELLO 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. + /// + [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"); + } } diff --git a/tests/StackExchange.Redis.Tests/HelloHandshakeTests.cs b/tests/StackExchange.Redis.Tests/HelloHandshakeTests.cs new file mode 100644 index 000000000..1b929802c --- /dev/null +++ b/tests/StackExchange.Redis.Tests/HelloHandshakeTests.cs @@ -0,0 +1,133 @@ +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Linq; +using System.Threading.Tasks; +using StackExchange.Redis.Server; +using Xunit; + +namespace StackExchange.Redis.Tests; + +/// +/// We issue HELLO whenever we expect the server to understand it, even when staying on RESP2: +/// the reply tells us the version/role/mode without needing INFO or CONFIG, both of +/// which are @dangerous and commonly restricted by ACLs; see issue #2968. +/// +public class HelloHandshakeTests(ITestOutputHelper log) +{ + [Theory] + [InlineData(RedisProtocol.Resp2, "2")] + [InlineData(RedisProtocol.Resp3, "3")] + public async Task HelloIsIssuedForBothProtocols(RedisProtocol protocol, string expectedProtover) + { + using var server = new RecordingServer(log); + var config = server.GetClientConfig(); + config.Protocol = protocol; + + await using var conn = await ConnectionMultiplexer.ConnectAsync(config); + + var hellos = server.Recorded("HELLO"); + Assert.NotEmpty(hellos); + Assert.All(hellos, args => Assert.Equal(expectedProtover, args.FirstOrDefault())); + Assert.Equal(protocol, conn.GetServerSnapshot()[0].Protocol); + } + + [Fact] + public async Task NoHelloWhenDisabledViaCommandMap() + { + using var server = new RecordingServer(log); + var config = server.GetClientConfig(); + config.Protocol = RedisProtocol.Resp2; + config.CommandMap = server.CreateCommandMap(except: "HELLO"); + + await using var conn = await ConnectionMultiplexer.ConnectAsync(config); + + Assert.Empty(server.Recorded("HELLO")); + Assert.Equal(RedisProtocol.Resp2, conn.GetServerSnapshot()[0].Protocol); + } + + [Fact] + public async Task NoHelloWhenServerIsAssumedDownLevel() + { + using var server = new RecordingServer(log); + var config = server.GetClientConfig(); + config.Protocol = RedisProtocol.Resp2; + config.DefaultVersion = new Version(5, 0, 0); // HELLO arrived in 6.0 + + await using var conn = await ConnectionMultiplexer.ConnectAsync(config); + + Assert.Empty(server.Recorded("HELLO")); + Assert.Equal(RedisProtocol.Resp2, conn.GetServerSnapshot()[0].Protocol); + } + + /// + /// The point of issue #2968: with INFO and CONFIG unavailable we used to fall back to + /// SET {random-guid} replica-read-only PX 1 NX to detect a read-only replica - a key write that + /// cannot be allow-listed in an ACL. HELLO reports the role, so the probe isn't needed. + /// + [Theory] + [InlineData(RedisProtocol.Resp2)] + [InlineData(RedisProtocol.Resp3)] + public async Task NoReplicaProbeWhenHelloTellsUsTheRole(RedisProtocol protocol) + { + using var server = new RecordingServer(log); + var config = server.GetClientConfig(); + config.Protocol = protocol; + config.CommandMap = server.CreateCommandMap(except: ["INFO", "CONFIG"]); + + await using var conn = await ConnectionMultiplexer.ConnectAsync(config); + + Assert.NotEmpty(server.Recorded("HELLO")); + Assert.Empty(server.Recorded("SET")); + Assert.False(conn.GetServerSnapshot()[0].IsReplica); // from HELLO's "role" + } + + /// + /// The converse of : when HELLO isn't available + /// either, the key-based probe is still the only signal we have, so it must still be issued. + /// + [Fact] + public async Task ReplicaProbeStillUsedWhenHelloUnavailable() + { + using var server = new RecordingServer(log); + var config = server.GetClientConfig(); + config.Protocol = RedisProtocol.Resp2; + config.CommandMap = server.CreateCommandMap(except: ["INFO", "CONFIG", "HELLO"]); + + await using var conn = await ConnectionMultiplexer.ConnectAsync(config); + + Assert.Empty(server.Recorded("HELLO")); + var sets = server.Recorded("SET"); + Assert.NotEmpty(sets); + Assert.All(sets, args => Assert.Equal("replica-read-only", args.Skip(1).FirstOrDefault())); + } + + private sealed class RecordingServer(ITestOutputHelper log) : InProcessTestServer(log) + { + private readonly ConcurrentQueue<(string Command, string[] Args)> _commands = new(); + + public override TypedRedisValue Execute(RedisClient client, in RedisRequest request) + { + var args = new string[Math.Max(request.Count - 1, 0)]; + for (int i = 0; i < args.Length; i++) + { + args[i] = request.GetString(i + 1); + } + _commands.Enqueue((request.GetString(0).ToUpperInvariant(), args)); + return base.Execute(client, in request); + } + + public List Recorded(string command) + => _commands.Where(x => x.Command == command).Select(x => x.Args).ToList(); + + public CommandMap CreateCommandMap(params string[] except) + { + var commands = GetCommands(); + foreach (var command in except) + { + commands.Remove(command); + } + return CommandMap.Create(commands); + } + } +} diff --git a/tests/StackExchange.Redis.Tests/Resp3HandshakeTests.cs b/tests/StackExchange.Redis.Tests/Resp3HandshakeTests.cs index 167f8996d..5a8c72979 100644 --- a/tests/StackExchange.Redis.Tests/Resp3HandshakeTests.cs +++ b/tests/StackExchange.Redis.Tests/Resp3HandshakeTests.cs @@ -38,17 +38,12 @@ public static IEnumerable GetHandshakeParameters() { foreach (var server in servers) { - if (client is RedisProtocol.Resp2 & server is not ServerResponse.Resp2) + // note that every combination is meaningful: we issue HELLO even for RESP2 clients + // (as HELLO 2), so "server doesn't understand HELLO" etc still needs exercising + int count = 1 << HandshakeFlagsCount; + for (int i = 0; i < count; i++) { - // we don't issue HELLO for this, nothing to test - } - else - { - int count = 1 << HandshakeFlagsCount; - for (int i = 0; i < count; i++) - { - yield return [client, server, (HandshakeFlags)i]; - } + yield return [client, server, (HandshakeFlags)i]; } } } @@ -67,6 +62,11 @@ public async Task Handshake(RedisProtocol client, ServerResponse server, Handsha await using var clientObj = await ConnectionMultiplexer.ConnectAsync(config); + // RESP3 requires both sides to want it; anything else lands on RESP2 + var expectedProtocol = client is RedisProtocol.Resp3 && server is ServerResponse.Resp3 + ? RedisProtocol.Resp3 : RedisProtocol.Resp2; + Assert.Equal(expectedProtocol, clientObj.GetServerSnapshot()[0].Protocol); + var sub = clientObj.GetSubscriber(); var db = clientObj.GetDatabase(); ConcurrentBag received = [];