Recent architecture breakages to bmcweb

[Ed Tanous]

I'm looking at a couple recent changes to bmcweb, and I'm finding a
significant architecture problem has been injected.  Namely, it's
these innocuous looking 4 lines here, which injects the socket adaptor
into the request object for use later.
https://github.com/openbmc/bmcweb/blob/30c58d581606b4484757e6ee9133c248de1514a6/http/http_request.h#L18

The problem with this approach has a few roots:
1. The Request class is meant to model a single request, single
response model.  Adding the stream semantics breaks this in pretty
significant ways, and forces a hard dependency between the streaming
adapter and the Request, which was not the intent.  We have
abstractions for "streaming" requests, but that was seemingly not
used.

2. In the code that existed before this, Adaptor was a template on
purpose.  It is designed to implement the std::networking
AsyncReadStream and AsyncWriteStream concepts.  This is designed to
allow injection of Unit Tests at some point, as I've talked about
before.  Hardcoding it in request to 2 forced stream types, based on
the compiler flag is incorrect per asio standards, and removes the
ability to inject arbitrary adapters, like test adaptors.  Also, the
flag used is incorrect, as it's possible to inject a non-ssl socket
even if SSL is enabled.

3. There is already a precedent and pattern for streaming interfaces
in bmcweb that we adopted from Crow.  If you look at the Websocket
request response type, it implements a way to request a route that
streams dynamically.  Frustratingly, part of what this was used for
was SSE, which I had already written a patch for that didn't have any
of the above issues, and only hadn't merged it because we didn't have
any SSE routes yet, and didn't want to check in dead code.
https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/13948

4. It opens the possibility for lifetime and possible (at the very
least harder to audit) security issues, as now the "http server"
component is no longer the only thing that can own sockets.
Previously, the server owned the sockets until handed off, then there
was no shared ownership between the websocket class, and the
Connection class.  The Connection class could be completely destroyed
(and memory freed) while the websocket was still connected and
running.

Moving to another track, you may ask, how did I come across this and
why does it matter?  I'm trying to add 2 new features to bmcweb.  The
first allows opening multiple sockets, and dynamically detecting TLS
streams on them.  This allows bmcweb to handle both HTTPS redirects in
band, and handle the case where users type in something erroneous,
like "http://mybmc:443" and connect to an SSL socket with a non-ssl
protocol.  In those cases, we can simply do the right thing.  It also
allows bmcweb to host on multiple ports, which might be interesting
for aggregator types.  More importantly, it cleans up some of the
Adaptor abstraction to make way for unit testing, and being able to
inject a "test" socket, that we can control the semantics of.  I'm
hoping eventually to be able to mock dbus, and mock the TCP socket,
and run a full Redfish validator run in a unit test.  I think that
would save a lot of time overall for both committers and consumers.

The first of these patches is posted here, and simply comments out the
above problems for now.
https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/35265

If I look through the commit logs, it looks like Ravi and Appu built
the two small subsystems that rely on the above abstraction, one for
SSE, and one for some NBD streamer.
What do you two think about the above?  Was it something you
considered when you wrote your patches?  Would you consider fixing
them?

  My recommendation would be to move both of those two over to
something similar to the websocket abstraction we have, with, on
connect, on data, and on close handlers.  This means that handlers no
longer take a hard dependency on the transport, which will help for
both unit testing, and if we ever want to support redfish device
enablement (which relies on an i2c based transport). The SSE one can
probably be used more or less as-is from my old patch.  The NBD one
might need a "Dynamic body" type, which beast already has an
abstraction for that seems to have been discounted.

What do you guys think?

-Ed