Recent architecture breakages to bmcweb

[raviteja bailapudi]

Ed,Sorry for the late response, I forgot to reply here, Some how the first
mail got missed but once I got a  reminder mail, I started working on this
and the discussion is going on in the gerrit.
I had few more doubts which I asked in the gerrit, Can you please take a
look?



-RaviTeja

On Fri, 28 Aug, 2020, 12:42 am Ed Tanous, <ed at tanous.net> wrote:

> On Sun, Aug 2, 2020 at 8:53 AM Ed Tanous <ed at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20200901/74c30a51/attachment-0001.htm>