Re: Recent architecture breakages to bmcweb

[Ed Tanous <ed@tanous.net>]

On Sun, Aug 2, 2020 at 8:53 AM Ed Tanous <ed@tanous.net> wrote:
>
> 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


It's been 3 weeks and I haven't gotten any replies to this pretty
major architecture break.  It also looks like it can also cause a
memory leak in HttpConnection here (found by code inspection here).
https://github.com/openbmc/bmcweb/blob/ebd459060ea4f42761402dd54acd0962c36136c2/http/http_connection.h#L351

I've pushed a revert to remove the features in question.  I would love
some comments from the developers that caused these breakages, so I
can make sure I'm doing the right thing here, and I'm not completely
off base (or that you intend to fix them, and this patch is
unnecessary).
https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/36038

Thanks,

-Ed