Create connectionIds using RNGCrypto #1606

BrennanConroy · 2018-03-15T05:21:16Z

davidfowl · 2018-03-15T05:46:20Z

src/Microsoft.AspNetCore.Sockets.Http/ConnectionManager.cs

@@ -71,6 +78,7 @@ public DefaultConnectionContext CreateConnection(PipeOptions transportPipeOption
            var connectionTimer = SocketEventSource.Log.ConnectionStart(id);
            var pair = DuplexPipe.CreateConnectionPair(transportPipeOptions, appPipeOptions);

+            // TODO: We can possibly delay creating connections until we get the first request and check the connectionId


Why is this a todo? If we're not doing this I'm not sure what the point is?

JamesNK · 2018-03-15T06:52:32Z

The issue talks about using an HMAC to validate the ID, but the PR is using HMAC as the ID, and no validation. Issue change in offline discussion?

davidfowl · 2018-03-15T07:03:41Z

src/Microsoft.AspNetCore.Sockets.Http/ConnectionManager.cs

-            return Guid.NewGuid().ToString();
+            var id = Interlocked.Increment(ref _id);
+            // HMAC the id because we want a cryptographically random id, which GUID is not
+            var hashed = _hmacHash.ComputeHash(Encoding.UTF8.GetBytes(id.ToString()));


Maybe this whole thing makes no sense and we should just use rngcryptoserviceprovider

Yeah, if the only outcome is to generate a more-different random number, why not just do that? If the goal is to validate something, we need a different approach.

BrennanConroy · 2018-03-15T15:30:39Z

Yeah, lets talk offline today

davidfowl · 2018-03-16T00:36:20Z

src/Microsoft.AspNetCore.Sockets.Abstractions/DefaultConnectionContext.cs


        // This tcs exists so that multiple calls to DisposeAsync all wait asynchronously
        // on the same task
        private TaskCompletionSource<object> _disposeTcs = new TaskCompletionSource<object>();

-        public DefaultConnectionContext(string id, IDuplexPipe transport, IDuplexPipe application)
+        public DefaultConnectionContext(string id)


I'm not a fan of these changes. I think you should just set the pipes on the outside. The options shouldn't be part of this class at all.

Do we care about people using this layer having to do that themselves?

I think it's OK to require that callers have to set the pipe themselves. User applications (Endpoints) will get a fully configured ConnectionContext, so we're the only consumers of this during initialization.

We can keep the other ctor to avoid changing all of the callers. We're just adding a new optional overload.

davidfowl · 2018-03-16T00:36:57Z

src/Microsoft.AspNetCore.Sockets.Http/ConnectionManager.cs

+            Span<byte> buffer = stackalloc byte[16];
+            _keyGen.GetBytes(buffer);
+            // Generate the id with RNGCrypto because we want a cryptographically random id, which GUID is not
+            return WebEncoders.Base64UrlEncode(buffer.ToArray());


Convert to hex, don't base64 encode.

Y u hate base64? It looks so much cleaner, and no ugly == because it's the URL encode variant.

Plus, the hex encoding code looks gross (BitConverter.ToString(buffer.ToArray()).Replace("-", ""))

Base64URL: MTIzNDU2NzgxMjM0NTY3OA Hex: 31323334353637383132333435363738

So much cleaner, looks more encrypty (it's "Base64 Encrypted" after all ;))

fite me irl

Then we need to update the Base64 encoder to support taking a span and we need to update it to use the new base64 decoder.

https://github.com/aspnet/Home/issues/2966

Until then, lets remove the stackalloc since that just forces us to allocate more.

analogrelay · 2018-03-16T03:11:30Z

To close the loop on the discussions, we decided that the impact of HMACing or encrypting the key was heavy and would only gain us the ability to remove the initial allocation of the entry in the connection list. Keeping track of the IDs we've issued (which we do now) is equally secure, but we can reduce allocations by avoiding allocating the full DefaultConnectionContext on the /negotiate endpoint, and only allocate it when the client returns for the poll/SSE/WebSocket/send endpoints.

analogrelay

Let's tidy up the pipe initialization.

Base64 4 lyfe.

analogrelay · 2018-03-16T03:14:22Z

src/Microsoft.AspNetCore.Sockets.Http/ConnectionManager.cs

+            return WebEncoders.Base64UrlEncode(buffer.ToArray());
+#else
+            // 128 bit buffer / 8 bits per byte = 16 bytes
+            var buffer = new byte[16];


Since this buffer is so short-lived and limited in scope, it it worth using ArrayPool<byte>.Shared to rent it? Legit not sure exactly what the use cases are for that. @davidfowl ?

No I think this is stackalloc worthy but only if we update WebEncoders.Base64UrlEncode(buffer) to take a Span<byte>

analogrelay · 2018-03-16T03:15:56Z

src/Microsoft.AspNetCore.Sockets.Abstractions/DefaultConnectionContext.cs


        // This tcs exists so that multiple calls to DisposeAsync all wait asynchronously
        // on the same task
        private TaskCompletionSource<object> _disposeTcs = new TaskCompletionSource<object>();

-        public DefaultConnectionContext(string id, IDuplexPipe transport, IDuplexPipe application)
+        public DefaultConnectionContext(string id)


I think it's OK to require that callers have to set the pipe themselves. User applications (Endpoints) will get a fully configured ConnectionContext, so we're the only consumers of this during initialization.

davidfowl · 2018-03-16T06:17:12Z

src/Microsoft.AspNetCore.Sockets.Http/ConnectionManager.cs

@@ -22,6 +24,8 @@ public class ConnectionManager
        // TODO: Consider making this configurable? At least for testing?
        private static readonly TimeSpan _heartbeatTickRate = TimeSpan.FromSeconds(1);

+        private static readonly RNGCryptoServiceProvider _keyGen = new RNGCryptoServiceProvider();


_keyGenerator

davidfowl · 2018-03-16T16:29:03Z

test/Microsoft.AspNetCore.Sockets.Tests/ConnectionManagerTests.cs

@@ -23,7 +24,6 @@ public void NewConnectionsHaveConnectionId()
            Assert.Null(connection.TransportTask);
            Assert.Null(connection.Cancellation);
            Assert.NotEqual(default(DateTime), connection.LastSeenUtc);
-            Assert.NotNull(connection.Transport);


Assert.Null

davidfowl · 2018-03-16T22:10:45Z

test/Microsoft.AspNetCore.Sockets.Tests/ConnectionManagerTests.cs


            Assert.NotNull(connection.ConnectionId);
-            Assert.NotNull(connection.Transport);


Add this back

davidfowl · 2018-03-16T22:11:10Z

src/Microsoft.AspNetCore.Sockets.Http/HttpConnectionDispatcher.cs

@@ -555,6 +558,15 @@ private DefaultConnectionContext CreateConnectionInternal(HttpSocketOptions opti
                return null;
            }

+            if (connection.Transport == null)


Add a comment here.

davidfowl · 2018-03-16T22:12:08Z

src/Microsoft.AspNetCore.Sockets.Http/HttpConnectionDispatcher.cs


-        private DefaultConnectionContext CreateConnectionInternal(HttpSocketOptions options)


Why not bring this back and just check the transport here internally?

davidfowl

LGTM

analogrelay

nits and comments, otherwise good to merge IMO

analogrelay · 2018-03-16T23:01:04Z

src/Microsoft.AspNetCore.Sockets.Abstractions/DefaultConnectionContext.cs

@@ -22,16 +22,20 @@ public class DefaultConnectionContext : ConnectionContext,
                                            IConnectionHeartbeatFeature,
                                            ITransferFormatFeature
    {
-        private List<(Action<object> handler, object state)> _heartbeatHandlers = new List<(Action<object> handler, object state)>();
+        private object _heartBeatLock = new object();


nit: _heartbeatLock (lower case b)

analogrelay · 2018-03-16T23:02:19Z

src/Microsoft.AspNetCore.Sockets.Abstractions/DefaultConnectionContext.cs

-        public DefaultConnectionContext(string id, IDuplexPipe transport, IDuplexPipe application)
+        /// <summary>
+        /// Creates the DefaultConnectionContext without Pipes to avoid upfront allocations.
+        /// You'll need to add Transport and Application pipes on your own.


~~You'll need to add Transport and Application pipes on your own~~

The caller is expected to set the <see cref="Transport"/> and <see cref="Application"/> pipes manually

analogrelay · 2018-03-16T23:02:49Z

src/Microsoft.AspNetCore.Sockets.Http/ConnectionManager.cs

@@ -94,8 +106,12 @@ public void RemoveConnection(string id)

        private static string MakeNewConnectionId()
        {
-            // TODO: We need to sign and encyrpt this
-            return Guid.NewGuid().ToString();
+            // TODO: Use Span when WebEncoders implements Span methods


analogrelay · 2018-03-16T23:03:28Z

src/Microsoft.AspNetCore.Sockets.Http/HttpConnectionDispatcher.cs

-            var transportPipeOptions = new PipeOptions(pauseWriterThreshold: options.TransportMaxBufferSize, resumeWriterThreshold: options.TransportMaxBufferSize / 2, readerScheduler: PipeScheduler.ThreadPool, useSynchronizationContext: false);
-            var appPipeOptions = new PipeOptions(pauseWriterThreshold: options.ApplicationMaxBufferSize, resumeWriterThreshold: options.ApplicationMaxBufferSize / 2, readerScheduler: PipeScheduler.ThreadPool, useSynchronizationContext: false);
-            return _manager.CreateConnection(transportPipeOptions, appPipeOptions);
+            // If the connection doesn't have a pipe yet then create one, we lazily create the pipe to save on allocations until the connection actually connects


until the client actually connects

davidfowl reviewed Mar 15, 2018

View reviewed changes

davidfowl reviewed Mar 16, 2018

View reviewed changes

analogrelay reviewed Mar 16, 2018

View reviewed changes

davidfowl reviewed Mar 16, 2018

View reviewed changes

BrennanConroy changed the title ~~Create connectionIds using HMAC~~ Create connectionIds using RNGCrypto Mar 16, 2018

BrennanConroy changed the base branch from dev to release/2.1 March 16, 2018 18:43

BrennanConroy added 8 commits March 16, 2018 11:54

Create connectionIds using HMAC

018876f

wip

7c6f502

WIP

937fe98

different approach

c132b57

Cleanup

a9e26d9

more cleanup

bc35670

more

69489f3

cleanup

caa12b8

BrennanConroy force-pushed the brecon/hmac branch from 4fdfea9 to caa12b8 Compare March 16, 2018 18:55

davidfowl reviewed Mar 16, 2018

View reviewed changes

BrennanConroy added 2 commits March 16, 2018 15:31

fb

5028124

missed

86e954b

davidfowl approved these changes Mar 16, 2018

View reviewed changes

analogrelay approved these changes Mar 16, 2018

View reviewed changes

fb

aa7c8cb

BrennanConroy merged commit 0e38ee3 into release/2.1 Mar 16, 2018

mikeharder pushed a commit that referenced this pull request Mar 16, 2018

Create connectionIds using RNGCrypto (#1606)

a071e15

BrennanConroy deleted the brecon/hmac branch March 17, 2018 00:01

gfoidl mentioned this pull request Apr 4, 2018

Added Span support to Webencoders dotnet/extensions#334

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create connectionIds using RNGCrypto #1606

Create connectionIds using RNGCrypto #1606

BrennanConroy commented Mar 15, 2018

davidfowl Mar 15, 2018

JamesNK commented Mar 15, 2018 •

edited

Loading

davidfowl Mar 15, 2018

analogrelay Mar 15, 2018 •

edited

Loading

BrennanConroy commented Mar 15, 2018

davidfowl Mar 16, 2018

BrennanConroy Mar 16, 2018

analogrelay Mar 16, 2018

davidfowl Mar 16, 2018

davidfowl Mar 16, 2018

analogrelay Mar 16, 2018

analogrelay Mar 16, 2018

analogrelay Mar 16, 2018

davidfowl Mar 16, 2018

BrennanConroy Mar 16, 2018

davidfowl Mar 16, 2018

analogrelay commented Mar 16, 2018

analogrelay left a comment

analogrelay Mar 16, 2018

davidfowl Mar 16, 2018

analogrelay Mar 16, 2018

davidfowl Mar 16, 2018

davidfowl Mar 16, 2018

davidfowl Mar 16, 2018

davidfowl Mar 16, 2018

davidfowl Mar 16, 2018

davidfowl left a comment

analogrelay left a comment

analogrelay Mar 16, 2018

analogrelay Mar 16, 2018

analogrelay Mar 16, 2018

analogrelay Mar 16, 2018


		Assert.NotNull(connection.ConnectionId);
		Assert.NotNull(connection.Transport);


		private DefaultConnectionContext CreateConnectionInternal(HttpSocketOptions options)

Create connectionIds using RNGCrypto #1606

Create connectionIds using RNGCrypto #1606

Conversation

BrennanConroy commented Mar 15, 2018

Choose a reason for hiding this comment

JamesNK commented Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

analogrelay Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

BrennanConroy commented Mar 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

analogrelay commented Mar 16, 2018

analogrelay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl left a comment

Choose a reason for hiding this comment

analogrelay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK commented Mar 15, 2018 •

edited

Loading

analogrelay Mar 15, 2018 •

edited

Loading