These are all great comments. Let me see if I can walk you through my thought process for some of them.
Thank you for your thoughts; Keep them coming if you have more.
-Ed
From: Michael.E.Brown at dell.com [mailto:Michael.E.Brown at dell.com]
Sent: Wednesday, December 20, 2017 11:50 AM
To: Tanous, Ed <ed.tanous at intel.com>; openbmc at lists.ozlabs.org
Subject: RE: Request for comments: C++ embedded webserver
I started reviewing some of the code, and I have quite a few thoughts, let's see if I can convey them without writing a book. The main aspect I'm looking at is the redfish interface, but many of these apply equally well to the REST and redfish parts. Here are my initial thoughts when looking at this proposal.
1) extensibility: I see openbmc as a base that many people will want to build upon. To enable that, we need to be able to have easy ways to extend or override the things in the base.
My thoughts here mirror your own, and it's a problem that I don't think has been solved. In my mind, these are huge opens for me that I would like to see how to resolve without making a lot of mess.
- Compile time extensions vs runtime loadable - open question
My preference would be for a compile time superset of functionality. I dislike the idea of managing a runtime dynamic library registration scheme, due to the lack of traceability in code, and problems I've had in the past, but if you have ideas here, I'm all ears.
- Need to add OEM extensions to individual URLs
- Need to override specific properties in an individual URL
- Need to be able to add entire new URI trees
- Need to extend existing trees with links/references to new trees
At one point, the different sub trees were detectable and selectable at build time as header only libraries. If you look at redfish_v1.hpp, specifically the request_routes function, essentially all endpoints are "registered" with the url handler at runtime, but compiled into the main application. The way this was implemented uses the registration scheme for generating the individual redfish sub routes, so in theory a newly registered URL would be somewhat self-contained, and the higher level redfish routes would automatically add the routes to the tree. The way this was built allows overriding essentially any endpoint with your own code at compile time using another registration. There is a lot more work to do here, but I think the design supports what you want, at least in principal if not in examples.
You are absolutely right, there is a lot of stuff hardcoded to values that will need to be overridden. My preference would be to push the things that need to be configurable to dbus parameters so it can be modified at will, and by any client. With that said, I suspect that won't work in all cases, and we likely need to start with a couple examples of endpoints that need to be configured differently.
- Need to dynamically generate the metadata URIs
Completely agree.
My commentary while reviewing this code is that I don't immediately see obvious ways of doing any of these extensions that don't effectively fork the entire codebase and make future maintenance much more difficult.
Without devolving into a small novel (I'd rather just show the ideas as code) my thought was that there would be a Cmake step to detect applicable headers and generate a "register" file that would be compiled in. This assumes that the modifiable parts of the handler infrastructure aren't sourced from DBus. In general, a dbus workflow would look like this:
Call mapper to determine dbus objects expose a "Chassis" interfaces
For Chassis in chassis objects:
Register endpoint
I hope that dbus could cover most of the configurability that's needed, but I haven't trodden too far down this path yet.
2) (redfish) Easier ways of generating JSON and mapping JSON properties to backend data: The initial code to generate JSON I see here is quite simple, but one aspect worries me. It appears that you have a nice and simple way to create the output JSON data structure, but right now most everything is hard coded. Once you start filling in the data "for real", we will need to write an ever-growing pile of code to translate from DBUS calls to get the data and poke the data into the correct spot in the JSON output. In my mind, DBUS is introspectable and JSON is introspectable. It seems like it would be a big win to create a small, generic JSON engine that holds the data and then reads metadata about which calls to make to update the data.
This was the original direction I went, and very quickly backtracked as the complexity grew. This assumes the mapping of dbus property to a redfish property is 1:1 and can be covered generically. I found this to not be the case for a few reasons:
Some values needed conversions: ie, openbmc sensors are scaled integers that require a float conversion using two values on the bus.
Some properties don't exist on dbus at all, and need to be sourced from within the daemon (like Manager UUID)
Some properties will likely need to be generated from files on disk (like ComputerSystem UUID)
Some properties will need to generate files onto disk, and take special actions (like writing an SSL certificate)
Some properties and structures are aggregated into a single call instead of exposed separately, despite coming from the same "property" name (like in the case of the thermal schema) they are aggregated as dbus objects that contain some hardcoded fields, as well as some generated fields.
Some properties will need to be cached from dbus for performance on larger systems (sensor readings come to mind here).
Some properties will need to be read-only instead of read write. While this is captured in the property descriptions of DBus, I wasn't comfortable making the security assumption that anything that's writeable on dbus should be writeable externally.
Some properties require a call to the mapper to map an available interface name to the object on the bus that supports that interface.
Some properties will likely require a system call to return the result (Privileges schema getting user group membership for example).
There are probably more avenues of flexibility here and in the end, I ended up with a rats nest of special cases that usually only applied to a single property. I suspect I swung the hammer back too far here toward hardcoding, and there is likely a middle ground that would result in less code, but for the number of schemas this patchset currently supports, I didn't find any abstractions that were useful.
3) performance: It looks like you currently make backend calls to satisfy requests for data. This means that more frontend calls will result in higher load on the backend. This means that it would be possible for frontend load to impact things like power/thermal reaction times without some limits on the frontend webserver.
This is very true, and on my list of things that need tackling. Imposing limits correctly (so they're not exposable as DOS attack vectors) is a non-trivial problem. With that said, the crow framework supports middleware's that I think can be enabled to help with this. For certain things, I don't think this can be avoided, as DBus is currently our source of "truth" for system state.
However, the DBUS architecture has a signal based system for generating events that we could leverage.
I had a prototypes of exactly this for sensors at one time. This is something I think can be reasonably abstracted as a "dbus cache" that can be reused reliably. Ideally I'd like to work this cache into an ETag header handler so most unchanged requests can be sourced without even regenerating or storing the return json.
The webserver could keep an internal copy of all of its JSON output and subscribe to DBUS signals/events to update that internal cache. This way, external web requests do not automatically translate every time into internal DBUS calls. The webserver could easily satisfy most web requests using data it already has on hand. This would result in much higher performance
It should be noted, this caching mechanism becomes a tradeoff of total system speed for response time speed. As all the values need to be cached in the webserver, this means that there are process context switches to the web server on every property change. While great for the web response times and performance, it's pretty detrimental to the rest of the system, as most BMC targets are single core ARM devices without a lot of extra CPU power. We should run some performance measurements of the impact in both cases and talk about the tradeoffs. There might be some compromise here by using process priority, but I don't have great data here.
4) security: the redfish standard has extension points to enable uploading new permission mappings or dynamically create roles and permissions. It's not clear to me how we would satisfy these dynamic security requirements
This is intentionally omitted in this push because a lot of the user management architecture is in flux and under discussion in the OpenBmc community. Today, the webserver simply logs in as a given user using PAM, then does no role or privilege checks apart from if the user is enabled. My assumption is that the authentication middleware would be extended with a module for this dynamic security mapping. With that said, I haven't looked at that part of redfish in depth, and all our previous experience used fixed roles so I'm very keen if you guys have some experience here.
Token infrastructure: I haven't looked at this code yet. In general, though, I really like having token-based security middleware so that we can provide extensions to support OAuth 2.0 and OpenID Connect support. The main idea here is that we need a way to delegate security decisions to a trusted third party. If we are using standard JWT tokens compatible with OIDC for identity, this makes the transition much easier.
All of the token infrastructure is flexible, and what's there was mostly built as a "get it to work" type thing. I intentionally avoided JWT initially because I had no experience with using them outside of a few toy apps, and there are concerns about the session timeout capabilities, given the fact that systems can have a huge variant in the system clock during cmos reset. The ability to get a shared secret onto the BMC and store it securely also concern me, but I suspect they are solvable problems with enough thinking about it.
Thanks again for all your thoughts.
--
Michael
From: openbmc [mailto:openbmc-bounces+michael.e.brown=dell.com at lists.ozlabs.org] On Behalf Of Tanous, Ed
Sent: Monday, December 18, 2017 10:08 PM
To: OpenBMC Maillist <openbmc at lists.ozlabs.org<mailto:openbmc at lists.ozlabs.org>>
Subject: Request for comments: C++ embedded webserver
I'm looking for comments on a code review that's been outstanding. One of the large pushes we've made is to attempt to make the web server more efficient, and add capabilities that comprehend long term needs of OpenBmc. One key that wasn't made clear in the commit message is that it includes the basic redfish implementation that (we hope) should be extensible to the full redfish specification in the short term future.
https://gerrit.openbmc-project.xyz/#/c/7786/
We would very much appreciate comments to see if we can move this forward.
The following description is pulled from the readme.
# OpenBMC webserver #
This component attempts to be a "do everything" embedded webserver for openbmc.
## Capabilities ##
At this time, the webserver implements a few interfaces:
+ Authentication middleware that supports cookie and token based authentication, as well as CSRF prevention backed by linux PAM authentication credentials.
+ An (incomplete) attempt at replicating phosphor-dbus-rest interfaces in C++. Right now, a few of the endpoint definitions work as expected, but there is still a lot of work to be done. The portions of the interface that are functional are designed to work correctly for phosphor-webui, but may not yet be complete.
+ Replication of the rest-dbus backend interfaces to allow bmc debug to logged in users.
+ An initial attempt at a read-only redfish interface. Currently the redfish interface targets ServiceRoot, SessionService, AccountService, Roles, and ManagersService. Some functionality here has been shimmed to make development possible. For example, there exists only a single user role.
+ SSL key generation at runtime. If an RSA key and cert pair are not available to the server at runtime, keys are generated using the openssl routines, and written to disk.
+ Static file hosting. Currently, static files are hosted from the fixed location at /usr/share/www. This is intended to allow loose coupling with yocto projects, and allow overriding static files at build time.
+ Dbus-monitor over websocket. A generic endpoint that allows UIs to open a websocket and register for notification of events to avoid polling in single page applications. (this interface may be modified in the future due to security concerns.)
Thanks,
-Ed
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20171221/a04e0c83/attachment-0001.html>