OpenAPI for bmcweb

[Ed Tanous]

On Mon, Jun 8, 2020 at 10:04 AM Richard Hanley <rhanley at google.com> wrote:
> I recently started looking into using OpenAPI for Redfish as part of an
internal project, and I want to try using some of that work in bmcweb.
>
> Right now the OpenAPI support for C++ doesn't mesh very well with
nlohmann and boost::beast. Assuming that we can handle the integration,
what concerns or objectections should I keep in mind?
>
> When I brought the topic up a number of months ago, the biggest issue was
increasing the flash size usage. If a solution is made that doesn't bloat
the image, how do people feel about OpenAPI?
>
> Any other thoughts or ideas on the topic?

Way late on the reply to this one;  Meant to send it a while back and lost
track of it.  I'm frankly surprised more people haven't replied, as this
was a pretty popular topic a year or so ago, with several implementations
of generated bindings being proposed.

You might want to be more specific about what you're hoping to get
working.  bmcweb exposes an openapi introspection endpoint today under
/redfish/v1/JsonSchemas
Source:
https://github.com/openbmc/bmcweb/tree/master/static/redfish/v1/JsonSchemas

I'm guessing you're wanting to generate c++ structure definitions for use
within the server to ease serialization and deserialization of endpoint
structures.  (If I'm incorrect in my assumption, disregard what follows)

This has been tried by many, many people over the short span of time that
bmcweb has existed;  Prior to the bmcweb-on-master times, there was also a
competing web stack, written in go, that revolved around generating the
server entirely from schema.   So far as I'm aware, nobody got past the
"Look at this example" phase.  With that said, if you think it's needed,
and would be better than what's there, by all means attempt it;  In the
efforts of saving your time, I'd recommend talking to one of the people
that tried in the past.  First off, one thing that would've helped the past
attempts would be to be clear about what present problem you're trying to
fix, and include some examples of code that you don't like, and what the
equivalent code (once you're done) would look like.  I think that would
help a lot of people provide input.  Full disclosure, several years ago I
spent a day trying to do exactly this, and determined it was a non-starter,
because it added a lot of complexity for some seemingly simple calls.

I'm going to try to summarize the roadblocks that the various efforts hit
(this is by no means a complete list)
1. The redfish OpenAPI is a complete definition of all properties and
endpoints that _could_ exist.   The majority of the openapi model
properties are specified as optional in the definition; Therefore, because
bmcweb/OpenBMC implements a subset of these, there needs to be some kind of
compile time binding spec that determines which of the optional properties
bmcweb _will_ implement.  This also needs to be extended into "which enums
will bmcweb support" and "what validation will we use on certain strings",
as redfish is fairly loose in what it allows for a lot of things, like
resource names, whereas dbus is more stringent.  Adding this auxiliary
definition defeated most of the perceived complexity that OpenAPI would've
improved upon.

2. The generated code was far too verbose to inspect for security problems
line by line.    In one case (I'm not picking on the person that did this
if you're listening) python code read in the spec, and used Jinja to
generate templated c++ structures that were then converted into
serialize/deserialize functions for every endpoint.  Tracing code from a
user given json parameter, to a type-safe c++ parameter that could be
operated on was a mess, and it was difficult to inspect what kind of safety
could be expected for a given variable/structure (example, could this
string contain a unicode character that's disallowed on DBus?).  This could
certainly be optimized and improved in newer attempts, but it's a lot of
effort.

3. So far as I'm aware, nobody even made it to being able to handle the
PATCH case sanely, where most properties are essentially optional, and
there's some binding changes between properties that might have an implied
order.
There have been cases in the past where we've misunderstood the intent
behind the CSDL (XML) definition (before the openapi stuff existed), and
implemented the wrong input parsing code.   The one that comes to mind was
a case where we were accepting an object as a property directly, when it
needed to be an array of properties (usually with size 1) to be
compatible.  In the efforts to not break the people that had already coded
against the "wrong" implementation, we now accept both, but warn on the
former.  Implementing these rare corner backward compatibility cases in
generic code tends to be very difficult.

4. None of the efforts properly implemented the correct error message
handling for missing/bad properties.  This is non-trivial, and about half
of the readJson code (that you would likely be replacing) is simply mapping
the schema violations to the correct Redfish error message.
Binary sizes weren't great (as you've already noted) because they required
redfish to have different parsing code than the rest of the system
(dbus-rest, ect).  This is probably the least of my problems with the
effort, and is something that's certainly solvable from a technical
standpoint.
Because OpenAPI would be run on endpoints that are not yet trusted (like
PATCH /Sessions/) it adds all of the openAPI code to the high risk security
envelope of unauthenticated requests, and increases the effort of those
that manually have to audit the authentication code to meet security
standards.

5. Most of the efforts failed the Redfish schema validator for the
endpoints they produced.  This could certainly be debugged and improved.

6. (opinion) As is, bmcweb already has a problem with submitters not
testing their code.  This is why it now requires running the Redfish
service validator on every change.  Having pre-generated bindings is likely
to make developers assume that their schemas are correct, and are less
likely to run a test beforehand.  With that said, most of the problems


With all of that said, if you're looking to generate something, I think
some low hanging fruit could be found in the permissions definitions.
As-is, there is an implicit expectation that the committer has checked the
Privilege Registry file, and updated their permission definitions
accordingly.  In practice, most commiters don't realize the privilege
registry even exists, and simply copy a parameter from another schema.
This seems like something that's ripe for generated code to generate the
privilege mappings that people could simply #include against.  While we're
in the realm of the beach boys (wouldn't it be nice).  A tool that used the
openapi definitions to "fuzz" redfish would be another great opportunity
for generated code.


-Ed
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20200714/5a070e69/attachment.htm>