Allow server accept calls to proceed concurrently

[andrewjstone]

The sprockets handshake includes both TLS handshake and bidirectional
remote attestation. On real Oxide hardware this results in talking over
IPCC to the SP and then over SPI to the RoT. This introduces delays in
the protocol. And while each message that must reach the RoT may have
to go over IPCC serially (pipelined?), there are other operations that
occur interleaved with these operations, such as the messages sent
between nodes over the bootstrap network. We therefore want to allow the
accept operations as a whole to proceed concurrently, which will speed
up cold boot and also match the behavior we allow on the client side.

As in most async networking code, we wait for each individual connection
by polling the TCP socket acceptor. Once that happens we return a future
that can be polled concurrently or in parallel as desired by the caller.
This matches the tokio-rustls pattern.

A test was added to ensure that the pattern works as we expect with
spawn, which we plan to use in Omicron.

Some minor test deduplicaton was also done.

[andrewjstone]

Everything below here is unchanged except for the removal of self.

[jgallagher]

This is kind of a strange API - if we're already async, why not just go ahead and do the remaining asynchronous work and let the caller spawn() the whole thing if they want to?

I assume this didn't work before because of &mut self, but if we're able to change it to &self - could we not return the nested future and still be spawnable?

[andrewjstone]

I agree this is a strange API, which is primarily why I tagged you on this review :)

The problem is that we don't know when to spawn that task. The TCP accept is what indicates there is a connection and lets us know that we can proceed with the spawn to perform the handshake. This is why tokio_rustls takes the TCP stream itself as a parameter and then performs the TLS handshake. It separates the two by having the caller have to manage the TCP socket separately, whereas this code wraps the TCP accept by returning it's own future.

We could have the caller spawn the whole things, but then we'd have to spawn some number of accept tasks and each time one went away, spawn another. This seems trickier and less friendly to the user.

[jgallagher]

Ahh I see. Okay, could we do one of these to make what's going on more obvious?

• Do the same split that tokio_rustls does and have the caller pass in an already-accepted socket that we do the handshake on?
• Instead of returning an impl Future, return a newtype (SprocketsAcceptor or something) that has an async handshake() method on it?

[andrewjstone]

Good idea. Either of those would be less weird.

I originally wrapped the TCP accept so that the tcp socket couldn't be used on it's own before the sprockets handshake was complete. This was mainly to prevent mistakes where somehow we ended up using plain sockets other than attested TLS channels. But thinking about that some more, I realize that's kind of silly. The connection side will always be sprockets only and so the handshake will bomb out. And plus, this is only used in 1 place. I say this, because I think the tokio_rustls version is a little clearer.

[andrewjstone]

Hmm, looking at the existing code, I know think the second option of returning a future newtype makes sense.

While it would be cool to take any AsyncWrite + AsyncRead + Unpin for IO like tokio_rustls, I'd have to make the same change on the client side, and the examples for this to work. And I find it hard to justify passing in an accepted TcpStream rather than the more generic type :) This would be a larger change than I'm looking to make right now.

[andrewjstone]

Fixed in 05f412b

This looks so much better. Thanks @jgallagher!

[labbott]

Do you have any numbers about how much this actually speeds things up? The theory does seem solid.

[jgallagher]

API changes LGTM; agreed the newtype is a lot clearer.

[andrewjstone]

Do you have any numbers about how much this actually speeds things up? The theory does seem solid.

Unfortunately not. That would require using these changes in omicron and then running on real hardware.

On the flip side, even if this doesn't improve performance at all it provides the API flexibility TLS has in terms of being able to structure your code concurrently with no downsides. The only reason I wouldn't merge it in personally was if running concurrent operations over IPCC was somehow unsafe. That seems like something that should be fixed independently though.

[andrewjstone]

Tested out using generated certs from oxidecomputer/pki-playground#126 on a branch in omicron. The connection handshake works until it tries to load the log files which don't exist because I haven't yet integrated the AttestMock stuff. AttestMock needs to be turned into a library first. I opened an issue for this.