Skip to content

Fix HTTP tunnel #2448

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
May 1, 2023
Merged

Fix HTTP tunnel #2448

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ef12ad1
Fix `HttpProxyTunnel` expected response
flobernd Apr 26, 2023
59ae26c
Allow `http://` prefix besides `http:` to specify the tunnel format
flobernd Apr 26, 2023
b8b8622
Add roundtrip test for URI tunnel format
flobernd Apr 26, 2023
6d78217
Merge branch 'main' into pr/2448
NickCraver May 1, 2023
39f1369
Add release notes
NickCraver May 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions src/StackExchange.Redis/Configuration/Tunnel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,10 @@ private sealed class HttpProxyTunnel : Tunnel
{
var encoding = Encoding.ASCII;
var ep = Format.ToString(endpoint);
const string Prefix = "CONNECT ", Suffix = " HTTP/1.1\r\n\r\n", ExpectedResponse = "HTTP/1.1 200 OK\r\n\r\n";
const string Prefix = "CONNECT ", Suffix = " HTTP/1.1\r\n\r\n", ExpectedResponse1 = "HTTP/1.1 200 OK\r\n\r\n", ExpectedResponse2 = "HTTP/1.1 200 Connection established\r\n\r\n";
byte[] chunk = ArrayPool<byte>.Shared.Rent(Math.Max(
encoding.GetByteCount(Prefix) + encoding.GetByteCount(ep) + encoding.GetByteCount(Suffix),
encoding.GetByteCount(ExpectedResponse)
Math.Max(encoding.GetByteCount(ExpectedResponse1), encoding.GetByteCount(ExpectedResponse2))
));
var offset = 0;
offset += encoding.GetBytes(Prefix, 0, Prefix.Length, chunk, offset);
Expand All @@ -76,10 +76,11 @@ static void SafeAbort(object? obj)
await args;

// we expect to see: "HTTP/1.1 200 OK\n"; note our buffer is definitely big enough already
int toRead = encoding.GetByteCount(ExpectedResponse), read;
int toRead = Math.Max(encoding.GetByteCount(ExpectedResponse1), encoding.GetByteCount(ExpectedResponse2)), read;
offset = 0;

while (toRead > 0)
var actualResponse = "";
while (toRead > 0 && !actualResponse.EndsWith("\r\n\r\n"))
{
args.SetBuffer(chunk, offset, toRead);
if (!socket.ReceiveAsync(args)) args.Complete();
Expand All @@ -88,11 +89,12 @@ static void SafeAbort(object? obj)
if (read <= 0) break; // EOF (since we're never doing zero-length reads)
toRead -= read;
offset += read;

actualResponse = encoding.GetString(chunk, 0, offset);
}
if (toRead != 0) throw new EndOfStreamException("EOF negotiating HTTP tunnel");
if (toRead != 0 && !actualResponse.EndsWith("\r\n\r\n")) throw new EndOfStreamException("EOF negotiating HTTP tunnel");
// lazy
var actualResponse = encoding.GetString(chunk, 0, offset);
if (ExpectedResponse != actualResponse)
if (ExpectedResponse1 != actualResponse && ExpectedResponse2 != actualResponse)
{
throw new InvalidOperationException("Unexpected response negotiating HTTP tunnel");
}
Expand Down
20 changes: 13 additions & 7 deletions src/StackExchange.Redis/ConfigurationOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -904,19 +904,25 @@ private ConfigurationOptions DoParse(string configuration, bool ignoreUnknown)
{
Tunnel = null;
}
else if (value.StartsWith("http:"))
else
{
value = value.Substring(5);
if (!Format.TryParseEndPoint(value, out var ep))
// For backwards compatibility with `http:address_with_port`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should add tests for this change, i.e. that we get the expected values back out - even if that means poking at internal state after the parse

Copy link
Contributor Author

@flobernd flobernd Apr 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the new format to the roundtrip test. This should be sufficient to confirm that the resulting Tunnel is the same for both input formats (HttpProxyTunnel.ToString() internally uses this.EndPoint.ToString() which means address and port must be the same to end up with the same string).

IMO it would make sense to consider deprecating the old format for the following reasons:

  • Roundtrip is inconsistent now, if you use the URI format
  • URI format is standard
  • URI format supports authentication (username + password) if needed at some point of time

Edit: If you prefer, we can apply the parsing change in a different PR (or remove it at all - it's up to you).
Edit 2: Not sure why the unrelated test fails after my latest commit. Seems a random failure, maybe you can re-run the CI.

if (value.StartsWith("http:") && !value.StartsWith("http://"))
{
value = value.Insert(5, "//");
}

var uri = new Uri(value, UriKind.Absolute);
if (uri.Scheme != "http")
{
throw new ArgumentException("Tunnel cannot be parsed: " + value);
}
if (!Format.TryParseEndPoint($"{uri.Host}:{uri.Port}", out var ep))
{
throw new ArgumentException("HTTP tunnel cannot be parsed: " + value);
}
Tunnel = Tunnel.HttpProxy(ep);
}
else
{
throw new ArgumentException("Tunnel cannot be parsed: " + value);
}
break;
// Deprecated options we ignore...
case OptionKeys.HighPrioritySocketThreads:
Expand Down