SignalR Negotiate protocol versioning #13389

mikaelm12 · 2019-08-23T20:19:30Z

This includes the changes for the server and the .NET client. Still polishing but ready for some review
Spec: https://github.com/aspnet/specs/pull/325

BrennanConroy

Quick first pass

src/SignalR/common/Http.Connections/src/HttpConnectionDispatcherOptions.cs

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs

src/SignalR/clients/csharp/Client/test/FunctionalTests/Hubs.cs

halter73 · 2019-08-27T21:05:43Z

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs

@@ -428,8 +429,10 @@ private async Task<NegotiationResponse> NegotiateAsync(Uri url, HttpClient httpC
                    urlBuilder.Path += "/";
                }
                urlBuilder.Path += "negotiate";
+                //urlBuilder.Query = $"version={_protocolVersionNumber}";


nit:

Suggested change

//urlBuilder.Query = $"version={_protocolVersionNumber}";

Suggested change

//urlBuilder.Query = $"version={_protocolVersionNumber}";

It looks like a follow up commit overwrote this.

Yeah, idk what happened here

halter73 · 2019-08-27T21:08:05Z

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs

+                Version = options.MinimumProtocolVersion
+            };
+
+            if (context.Request.Query.TryGetValue("version", out var queryStringVersion))


If the client doesn't pass a version and a minimum protocol version is specified via options, shouldn't that connection also be rejected?

options, shouldn't that connection also be rejected?

No version from the client implies V1 so if the minimum specified through options on the server is greater than that (it should be greater than V1 it it's explicitly set) we should reject the connection.

It doesn't look like there is a check for that though

halter73 · 2019-08-27T21:08:41Z

src/SignalR/common/Http.Connections/src/HttpConnectionDispatcherOptions.cs

+        /// Gets or sets the minimum protocol verison supported by the server.
+        /// The default value is 0, the lowest possible protocol version.
+        /// </summary>
+        public int MinimumProtocolVersion { get; set; } = 0;


What if this is set to a non-default value and a client tries to connection without negotiating?

Yeah, this was an open question in the design doc. I think your suggestion there of having an option to reject clients that skip negotiation seems reasonable. Otherwise you're left in a weird position where the client and server are unaware of each other's capabilities.

👍 We can always revisit if the default MinimumProtocolVersion ever goes above 0.

src/SignalR/clients/csharp/Client/test/FunctionalTests/HubConnectionTests.cs

src/SignalR/common/Http.Connections.Common/src/NegotiateProtocol.cs

src/SignalR/clients/csharp/Client/test/FunctionalTests/Startup.cs

BrennanConroy · 2019-08-28T00:02:36Z

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs

+                Version = options.MinimumProtocolVersion
+            };
+
+            if (context.Request.Query.TryGetValue("version", out var queryStringVersion))


It doesn't look like there is a check for that though

src/SignalR/common/Http.Connections/test/NegotiateProtocolTests.cs

halter73 · 2019-08-28T22:55:26Z

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs

+                {
+                    response.Version = clientProtocolVersion;
+                }
+            } else if (options.MinimumProtocolVersion > 0)


Suggested change

} else if (options.MinimumProtocolVersion > 0)

}

else if (options.MinimumProtocolVersion > 0)

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs

halter73 · 2019-08-28T22:58:48Z

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs

+                var clientProtocolVersion = int.Parse(queryStringVersion.ToString());
+                if (clientProtocolVersion < options.MinimumProtocolVersion)
+                {
+                    response.Error = $"The client requested version '{clientProtocolVersion}', but the server does not support this version.";


I think this message should include what the server-configure min protocol version is.

halter73 · 2019-08-28T23:05:28Z

It might be nice to rename the "version" query string parameter and negotiate property to "protocolVersion" or similar so customers don't think this is supposed to match the SiganlR package version.

mikaelm12 · 2019-08-28T23:15:14Z

It might be nice to rename the "version" query string parameter and negotiate property to "protocolVersion"

ProtocolVersion is taken
https://github.com/aspnet/AspNetCore/blob/bcdc9b08dfbb79ca6b8780e9ef5f9cabba67f71d/src/SignalR/common/Http.Connections.Common/src/NegotiateProtocol.cs#L160-L163

We can use NegotiateVersion if no one hates it too much

src/SignalR/clients/csharp/Client/test/FunctionalTests/HubConnectionTests.cs

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs

BrennanConroy · 2019-08-28T22:43:51Z

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs

+                var clientProtocolVersion = int.Parse(queryStringVersion.ToString());
+                if (clientProtocolVersion < options.MinimumProtocolVersion)
+                {
+                    response.Error = $"The client requested version '{clientProtocolVersion}', but the server does not support this version.";


Might want to log on the server here and the one below?

BrennanConroy · 2019-08-28T23:06:28Z

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs

-            response.AvailableTransports = new List<AvailableTransport>();
+            var response = new NegotiationResponse
+            {
+                ConnectionId = connectionId,


We should delay setting this until we know the client is compatible, and perhaps we should invalidate the ID too if there is an error. Or maybe we can delay even creating the connectionId and do the version checking earlier?

mikaelm12 · 2019-08-28T23:29:37Z

Note to self to add a test for client sending an invalid version in the query string

halter73 · 2019-08-28T23:36:07Z

I think NegotiateVersion is fine.

BrennanConroy

We need to update the transport spec document too!

BrennanConroy · 2019-08-28T23:47:54Z

src/SignalR/common/Http.Connections.Common/src/NegotiateProtocol.cs

+                    return;
+                }
+
+                if (response.Version > 0)


Per the spec, the server should send back the version even for clients that don't support versions yet.

This could be useful for bug reports where we see people with "version" in the negotiate but are using <=3.0 clients.

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.Log.cs

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs

…version

mikaelm12 · 2019-09-03T23:02:19Z

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=336108&view=logs

aspnet-hello · 2019-09-03T23:07:18Z

This comment was made automatically. If there is a problem contact [email protected].

I've triaged the above build. I've created/commented on the following issue(s)
https://github.com/aspnet/AspNetCore-Internal/issues/3057

mikaelm12 · 2019-09-04T02:53:21Z

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=336109&view=logs

BrennanConroy reviewed Aug 23, 2019

View reviewed changes

Pilchie added the area-signalr Includes: SignalR clients and servers label Aug 26, 2019

mikaelm12 marked this pull request as ready for review August 27, 2019 16:50

mikaelm12 requested review from analogrelay and halter73 as code owners August 27, 2019 16:50

halter73 reviewed Aug 27, 2019

View reviewed changes

BrennanConroy reviewed Aug 28, 2019

View reviewed changes

Negotiate protocl versioning

24417fa

mikaelm12 force-pushed the mikaelm12/NegotiateProtocolVersioning branch from 1716fae to 24417fa Compare August 28, 2019 21:48

+ test

dfe79c9

halter73 reviewed Aug 28, 2019

View reviewed changes

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs Outdated Show resolved Hide resolved

halter73 reviewed Aug 28, 2019

View reviewed changes

halter73 approved these changes Aug 28, 2019

View reviewed changes

BrennanConroy reviewed Aug 28, 2019

View reviewed changes

mikaelm12 added 2 commits August 28, 2019 16:20

update

c8394b6

Detect client sending non int version in the query string. Needs a test

cecf2a0

BrennanConroy reviewed Aug 28, 2019

View reviewed changes

mikaelm12 and others added 4 commits August 29, 2019 15:18

updates

de37b83

some logging

8dbf34b

fix tests and update queryString

c54a6b2

camelCase

7d63633

BrennanConroy reviewed Sep 3, 2019

View reviewed changes

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.Log.cs Outdated Show resolved Hide resolved

BrennanConroy approved these changes Sep 3, 2019

View reviewed changes

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs Show resolved Hide resolved

src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs Show resolved Hide resolved

mikaelm12 added 3 commits September 3, 2019 13:50

+ test

bc1506e

test for v1 client trying to connect to server with > 0 min protocol …

1388496

…version

update log event ids

b5a2197

BrennanConroy approved these changes Sep 3, 2019

View reviewed changes

code check

861abb6

mikaelm12 merged commit e0ee042 into master Sep 4, 2019

ghost deleted the mikaelm12/NegotiateProtocolVersioning branch September 4, 2019 02:53

mikaelm12 mentioned this pull request Sep 11, 2019

Add support for negotiateVersion query string parameter #13880

Merged

analogrelay added the 3.1-candidate label Sep 17, 2019

analogrelay added this to the 5.0.0-preview1 milestone Sep 17, 2019

BrennanConroy mentioned this pull request Sep 19, 2019

[TS] Add support for negotiateVersion and connectionToken #14157

Merged

BrennanConroy pushed a commit that referenced this pull request Sep 24, 2019

SignalR Negotiate protocol versioning (#13389)

ea8c2b9

BrennanConroy mentioned this pull request Sep 24, 2019

Negotiate protocol versioning and ConnectionID split #14402

Merged

18 tasks

BrennanConroy modified the milestones: 5.0.0-preview1, 3.1.0 Apr 10, 2020

	} else if (options.MinimumProtocolVersion > 0)
	}
	else if (options.MinimumProtocolVersion > 0)

SignalR Negotiate protocol versioning #13389

SignalR Negotiate protocol versioning #13389

Uh oh!

Conversation

mikaelm12 commented Aug 23, 2019

Uh oh!

BrennanConroy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halter73 commented Aug 28, 2019

Uh oh!

mikaelm12 commented Aug 28, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikaelm12 commented Aug 28, 2019

Uh oh!

halter73 commented Aug 28, 2019

Uh oh!

BrennanConroy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mikaelm12 commented Sep 3, 2019

Uh oh!

aspnet-hello commented Sep 3, 2019

Uh oh!

mikaelm12 commented Sep 4, 2019

Uh oh!

Uh oh!