bmcweb NBD proxy

[Ed Tanous]

I'm looking through the nbd proxy code in bmcweb, and I'm seeing some
issues, and I'd like to understand why they were done this way.

First off, nbd proxy has its own authorization code that's been
injected, separate from the one all other handlers (including other
websocket handlers) use.  Why was this done?  It feels like a hack,
but for the life of me I can't imagine what's being hacked around.
https://github.com/openbmc/bmcweb/blob/d139c2364bec98a5da1fe803414f3b02fdcd3092/include/nbd_proxy.hpp#L288

We have other examples that existed before nbd was merged of doing
this this recommended/right way.  Here's one from obmc_console:
https://github.com/openbmc/bmcweb/blob/d4d77e399526671076936e9d9dd879dad2d24a2f/include/obmc_console.hpp#L108

Having individual handlers own their own authorization is a huge
problem for maintenance, and significantly increases the likelihood
that we make a mistake in a handler, and inject a security problem.

This came up because I started to do a security audit (like I did
regularly when I was the bmcweb maintainer) and this route popped up
as not having authorization checks, yet controlling something
important.

The patch in question is here:
https://github.com/openbmc/bmcweb/commit/250b0ebb0e8d55882fa8e6b156f88828a7ba185d
Which makes me think it _was_ a security issue prior to that patch,
which further proves the point that doing this is a bad idea.  The
commit message contains the statement: "I have chosen this approach,
as generic privilege check for all websockets introduces significant
changes in connection upgrade flow which makes implementaion vague and
caused some memory issues difficult to track down."

It sounds like this was done just for lack of wanting to debug doing
this the right/existing way?  This seems like the wrong approach, and
something we don't really want to promote for the project in general.

Does anyone have more context on this?

-Ed