summaryrefslogtreecommitdiff
path: root/http/http_connection.hpp
AgeCommit message (Collapse)AuthorFilesLines
2024-05-28Implement Chunking for unix socketsEd Tanous1-1/+7
Response::openFd was added recently to allow handlers to pass in a file descriptor to be used to read. This worked great for files, but had some trouble with unix sockets. First, unix sockets have no known length that we can get. They are fed by another client until that client decides to stop sending data and sends an EOF. HTTP in general needs to set the Content-Length header before starting a reply, so the previous code just passes an error back. HTTP has a concept of HTTP chunking, where a payload might not have a known size, but can still be downloaded in chunks. Beast has handling for this that we can enable that just deals with this at the protocol layer silently. This patch enables that. In addition, a unix socket very likely might not have data and will block on the read call. Blocking in an async reactor is bad, and especially bad when you don't know how large a payload is to be expected, so it's possible those bytes will never come. This commit sets all FDs into O_NONBLOCK[1] mode when they're sent to a response, then handles the subsequent EWOULDBLOCK and EAGAIN messages when beast propagates them to the http connection class. When these messages are received, the doWrite loop is simply re-executed directly, attempting to read from the socket again. For "slow" unix sockets, this very likely results in some wasted cycles where we read 0 bytes from the socket, so shouldn't be used for eventing purposes, given that bmcweb is essentially in a spin loop while waiting for data, but given that this is generally used for handling chunking of large payloads being generated, and while spinning, other reactor operations can still progress, this seems like a reasonable compromise. [1] https://www.gnu.org/software/libc/manual/html_node/Open_002dtime-Flags.html Tested: The next patch in this series includes an example of explicitly adding a unix socket as a response target, using the CredentialsPipe that bmcweb already has. When this handler is present, curl shows the response data, including the newlines (when dumped to a file) ``` curl -vvvv -k --user "root:0penBmc" https://192.168.7.2/testpipe -o output.txt ``` Loading the webui works as expected, logging in produces the overview page as expected, and network console shows no failed requests. Redfish service validator passes. Change-Id: I8bd8586ae138f5b55033b78df95c798aa1d014db Signed-off-by: Ed Tanous <ed@tanous.net>
2024-05-10Fix lesser used optionsEd Tanous1-25/+28
25b54dba775b31021a3a4677eb79e9771bcb97f7 missed several cases where we had ifndef instead of ifdef. because these weren't the defaults, these don't show up as failures when testing. Tested: Redfish service validator passes. Inspection primarily. Mechanical change. Change-Id: I3f6915a97eb44d071795aed76476c6bee7e8ed27 Signed-off-by: Ed Tanous <ed@tanous.net>
2024-05-03Manage Request with shared_ptrJonathan Doman1-25/+25
This is an attempt to solve a class of use-after-move bugs on the Request objects which have popped up several times. This more clearly identifies code which owns the Request objects and has a need to keep it alive. Currently it's just the `Connection` (or `HTTP2Connection`) (which needs to access Request headers while sending the response), and the `validatePrivilege()` function (which needs to temporarily own the Request while doing an asynchronous D-Bus call). Route handlers are provided a non-owning `Request&` for immediate use and required to not hold the `Request&` for future use. Tested: Redfish validator passes (with a few unrelated fails). Redfish URLs are sent to a browser as HTML instead of raw JSON. Change-Id: Id581fda90b6bceddd08a5dc7ff0a04b91e7394bf Signed-off-by: Jonathan Doman <jonathan.doman@intel.com> Signed-off-by: Ed Tanous <ed@tanous.net>
2024-05-01Bring consistency to config optionsEd Tanous1-7/+7
The configuration options that exist in bmcweb are an amalgimation of CROW options, CMAKE options using #define, pre-bmcweb ifdef mechanisms and meson options using a config file. This history has led to a lot of different ways to configure code in the codebase itself, which has led to problems, and issues in consistency. ifdef options do no compile time checking of code not within the branch. This is good when you have optional dependencies, but not great when you're trying to ensure both options compile. This commit moves all internal configuration options to: 1. A namespace called bmcweb 2. A naming scheme matching the meson option. hyphens are replaced with underscores, and the option is uppercased. This consistent transform allows matching up option keys with their code counterparts, without naming changes. 3. All options are bool true = enabled, and any options with _ENABLED or _DISABLED postfixes have those postfixes removed. (note, there are still some options with disable in the name, those are left as-is) 4. All options are now constexpr booleans, without an explicit compare. To accomplish this, unfortunately an option list in config/meson.build is required, given that meson doesn't provide a way to dump all options, as is a manual entry in bmcweb_config.h.in, in addition to the meson_options. This obsoletes the map in the main meson.build, which helps some of the complexity. Now that we've done this, we have some rules that will be documented. 1. Runtime behavior changes should be added as a constexpr bool to bmcweb_config.h 2. Options that require optionally pulling in a dependency shall use an ifdef, defined in the primary meson.build. (note, there are no options that currently meet this class, but it's included for completeness.) Note, that this consolidation means that at configure time, all options are printed. This is a good thing and allows direct comparison of configs in log files. Tested: Code compiles Server boots, and shows options configured in the default build. (HTTPS, log level, etc) Change-Id: I94e79a56bcdc01755036e4e7278c7e69e25809ce Signed-off-by: Ed Tanous <ed@tanous.net>
2024-04-24Fix large content error codesEd Tanous1-88/+203
When pushing multi-part payloads, it's quite helpful if the server supports the header field of "Expect: 100-Continue". What this does, is on a large file push, allows the server to possibly reject a request before the payload is actually sent, thereby saving bandwidth, and giving the user more information. Bmcweb, since commit 3909dc82a003893812f598434d6c4558107afa28 by James (merged July 2020) has simply closed the connection if a user attempts to send too much data, thereby making the bmcweb implementation simpler. Unfortunately, to a security tester, this has the appearance on the network as a crash, which will likely then get filed as a "verify this isn't failing" bug. In addition, the default args on curl multipart upload enable the Expect: 100-Continue behavior, so folks testing must've just been disabling that behavior. Bmcweb should just support the right thing here. Unfortunately, closing a connection uncleanly is easy. Closing a connection cleanly is difficult. This requires a pretty large refactor of the http connection class to accomplish. Tested: Create files of various size and try to send them (Note, default body limit is 30 MB) and upload them with an without a username. ``` dd if=/dev/zero of=zeros-file bs=1048576 count=16 of=16mb.txt curl -k --location POST https://192.168.7.2/redfish/v1/UpdateService/update -F 'UpdateParameters={"Targets":["/redfish/v1/Managers/bmc"]} ;type=application/json' -F UpdateFile=@32mb.txt -v ``` No Username: 32MB returns < HTTP/1.1 413 Payload Too Large 16MB returns < HTTP/1.1 401 Unauthorized With Username 32MB returns < HTTP/1.1 413 Payload Too Large 16MB returns < HTTP/1.1 400 Bad Request Note, in all cases except the last one, the payload is never sent from curl. Redfish protocol validator fails no new tests (SSE failure still present). Redfish service validator passes. Change-Id: I72bc8bbc49a05555c31dc7209292f846ec411d43 Signed-off-by: Ed Tanous <ed@tanous.net>
2024-04-20Change ssl stream implementationsEd Tanous1-2/+1
Boost beast ssl_stream is just a wrapper around asio ssl_stream, and aims to optimize the case where we're writing small payloads (one or two bytes.) which needs to be optimized in SSL. bmcweb never writes one or two bytes, we almost always write the full payload of what we received, so there's no reason to take the binary size overhead, and additional boost headers that this implementation requires. Tested: This drops the on-target binary size by 2.6% Redfish service validator passes. Change-Id: Ie1ae6f197f8e5ed70cf4abc6be9b1b382c42d64d Signed-off-by: Ed Tanous <ed@tanous.net>
2024-04-14Use beast message_generatorEd Tanous1-5/+5
Beast 331 added the message_generator class, which allows deduplicating some templated code for the HTTP parser. When we use it, we can drop our binary size, and ensure that we have code reuse. This saves 2.2% on the compressed binary size. Tested: Redfish service validator passes. Change-Id: I5540d52dc256adfb62507c67ea642a9ea86d27ee Signed-off-by: Ed Tanous <ed@tanous.net>
2024-04-01Up the max connectionCount to 200Gunnar Mills1-1/+1
Have seen defects where hitting the max connection limit with multiple server managers attached. Although not common to exceed 100, can hit this when using 2 or 3 webui-vue GUIs and a server manager attached. webui-vue can use ~30 of these on its own; this isn't that hard to hit. Nginx by default sets 512 connections[1] , so 200 for an embedded target doesn't seem that unreasonable: Apache sets 256 by default [2] lighttpd sets 1024 [3] We're in line for the defaults for other webservers. Tested: Sent 180 basic auth requests seen bmcweb memory at 2189 2178 root R 29080 4% 49% ./bmcweb This was on a AST2600 (p10bmc) The connections open got to: [DEBUG "http_connection.hpp":79] 0x19bb5c8 Connection open, total 161 Came back down as expected: [DEBUG "http_connection.hpp":89] 0x1a41440 Connection closed, total 1 Didn't see this with multiple webui-vues / server managers. [1] https://nginx.org/en/docs/ngx_core_module.html#worker_connections [2] https://httpd.apache.org/docs/2.4/mod/mpm_common.html#maxrequestworkers [3] https://redmine.lighttpd.net/projects/1/wiki/Server_max-connectionsDetails Change-Id: I807302e32e61e31212850a480d721d89d484593f Signed-off-by: Gunnar Mills <gmills@us.ibm.com>
2024-03-19Rename FileBody to HttpBodyEd Tanous1-2/+3
Now that our custom body type does things more than files, it makes sense to rename it. This commit renames the header itself, then all instances of the class. Tested: Basic GET requests succeed. Change-Id: If4361ac8992fc7c268f48a336707f96e68d3576c Signed-off-by: Ed Tanous <ed@tanous.net>
2024-02-16Simplify bodyEd Tanous1-83/+39
Now that we have a custom boost http body class, we can use it in more cases. There's some significant overhead and code when switching to a file body, namely removing all the headers. Making the body class support strings would allow us to completely avoid that inefficiency. At the same time, it would mean that we can now use that class for all cases, including HttpClient, and http::Request. This leads to some code reduction overall, and means we're reliant on fewer beast structures. As an added benefit, we no longer have to take a dependency on boost::variant2. Tested: Redfish service validator passes, with the exception of badNamespaceInclude, which is showing warnings prior to this commit. Change-Id: I061883a73230d6085d951c15891465c2c8445969 Signed-off-by: Ed Tanous <ed@tanous.net>
2024-01-19Remove some boost includesEd Tanous1-4/+4
The less we rely on boost, and more on std algorithms, the less people have to look up, and the more likely that our code will deduplicate. Replace all uses of boost::algorithms with std alternatives. Tested: Redfish Service Validator passes. Change-Id: I8a26f39b5709adc444b4178e92f5f3c7b988b05b Signed-off-by: Ed Tanous <edtanous@google.com>
2024-01-09Fix spelling mistakesEd Tanous1-1/+1
These were found with: codespell -w $(git ls-files | grep "\.[hc]\(pp\)\?$") At some point in the future, we might want to get this enabled in CI. Change-Id: Iccb57b2adfd06a2e177e99db2923fe4e8e329118 Signed-off-by: Ed Tanous <ed@tanous.net>
2023-12-05Fix missing dateEd Tanous1-1/+1
At some point, the date got removed from http1 requests. HTTP2 does not show this issue, but this showed up in unit tests (which is why the prior commit is adding unit tests). The Date Header is useful for synchronizing things like Cache-Control-Policy, with the actual server time, instead of the local system time. Tested: Unit tests pass. Change-Id: I8f105f0cbb6c816c5ec6b14cbeae587d728a20d2 Signed-off-by: Ed Tanous <edtanous@google.com>
2023-12-05Unit test ConnectionEd Tanous1-67/+88
Boost asio provides a test stream object that we can use to begin unit testing the connection object. This patchset uses it to re-enable some simple http1.1 tests. There's some features that have snuck into the connection class that aren't compatible with a stream (like ip address getting), so unfortunately we do need the connection class to be aware if it's in test mode, but that tradeoff seems worthwhile. Tested: Unit test pass. Change-Id: Id8b1f8866582b58502dbafe6139f841bf64b8ef3 Signed-off-by: Ed Tanous <edtanous@google.com>
2023-10-31Move to file_body in boostEd Tanous1-40/+39
As is, it reads the whole file into memory before sending it. While fairly fast for the user, this wastes ram, and makes bmcweb less useful on less capable systems. This patch enables using the boost::beast::http::file_body type, which has more efficient serialization semantics than using a std::string. To do this, it adds a openFile() handler to http::Response, which can be used to properly open a file. Once the file is opened, the existing string body is ignored, and the file payload is sent instead. openFile() also returns success or failure, to allow users to properly handle 404s and other errors. To prove that it works, I moved over every instance of direct use of the body() method over to using this, including the webasset handler. The webasset handler specifically should help with system load when doing an initial page load of the webui. Tested: Redfish service validator passes. Change-Id: Ic7ea9ffefdbc81eb985de7edc0fac114822994ad Signed-off-by: Ed Tanous <ed@tanous.net>
2023-10-24clang-format: copy latest and re-formatPatrick Williams1-2/+2
clang-format-17 has some backwards incompatible changes that require additional settings for best compatibility and re-running the formatter. Copy the latest .clang-format from the docs repository and reformat the repository. Change-Id: I2f9540cf0d545a2da4d6289fc87b754f684bc9a7 Signed-off-by: Patrick Williams <patrick@stwcx.xyz>
2023-08-20mTLS: Fix handshake failuresMichal Orzel1-3/+5
Change introduced in [1] has exposed significant problem in mTLS verification process, during which an attempt to an uninitialized object was made. This change removes that attempt and replaces it with resource that is available at this specific moment of connection lifetime. Tested: 1. Created and uploaded a set of certificates by following instructions from TLS Configuration guide [2]. 2. Attempted to access /redfish/v1/SessionService/Sessions endpoint using mTLS authentication method. With this fix connection has been successful. [1] https://github.com/openbmc/bmcweb/commit/e01d0c36af115ed46d54b5dbbacfe3ad92226bd3 [2] https://github.com/openbmc/docs/blob/master/security/TLS-configuration.md Change-Id: I434dbf27169d7ea0207dfd139868d5bf398d24b0 Signed-off-by: Michal Orzel <michalx.orzel@intel.com>
2023-08-07Fix bugprone-unchecked-optional-access findingsEd Tanous1-2/+21
Clang-tidy has the aforementioned check, which shows a few places in the core where we ignored the required optional checks. Fix all uses. Note, we cannot enable the check that this time because of some weird code in health.hpp that crashes tidy[1]. That will need to be a future improvement. There are tests that call something like ASSERT(optional) EXPECT(optional->foo()) While this isn't an actual violation, clang-tidy doesn't seem to be smart enough to deal with it, so add some explicit checks. [1] https://github.com/llvm/llvm-project/issues/55530 Tested: Redfish service validator passes. Change-Id: Ied579cd0b957efc81aff5d5d1091a740a7a2d7e3 Signed-off-by: Ed Tanous <edtanous@google.com>
2023-07-20Replace logging with std::formatEd Tanous1-53/+54
std::format is a much more modern logging solution, and gives us a lot more flexibility, and better compile times when doing logging. Unfortunately, given its level of compile time checks, it needs to be a method, instead of the stream style logging we had before. This requires a pretty substantial change. Fortunately, this change can be largely automated, via the script included in this commit under scripts/replace_logs.py. This is to aid people in moving their patchsets over to the new form in the short period where old patches will be based on the old logging. The intention is that this script eventually goes away. The old style logging (stream based) looked like. BMCWEB_LOG_DEBUG << "Foo " << foo; The new equivalent of the above would be: BMCWEB_LOG_DEBUG("Foo {}", foo); In the course of doing this, this also cleans up several ignored linter errors, including macro usage, and array to pointer deconstruction. Note, This patchset does remove the timestamp from the log message. In practice, this was duplicated between journald and bmcweb, and there's no need for both to exist. One design decision of note is the addition of logPtr. Because the compiler can't disambiguate between const char* and const MyThing*, it's necessary to add an explicit cast to void*. This is identical to how fmt handled it. Tested: compiled with logging meson_option enabled, and launched bmcweb Saw the usual logging, similar to what was present before: ``` [Error include/webassets.hpp:60] Unable to find or open /usr/share/www/ static file hosting disabled [Debug include/persistent_data.hpp:133] Restored Session Timeout: 1800 [Debug redfish-core/include/event_service_manager.hpp:671] Old eventService config not exist [Info src/webserver_main.cpp:59] Starting webserver on port 18080 [Error redfish-core/include/event_service_manager.hpp:1301] inotify_add_watch failed for redfish log file. [Info src/webserver_main.cpp:137] Start Hostname Monitor Service... ``` Signed-off-by: Ed Tanous <ed@tanous.net> Change-Id: I86a46aa2454be7fe80df608cb7e5573ca4029ec8
2023-06-28HTTP/2 supportEd Tanous1-1/+30
HTTP/2 gives a number of optimizations, while keeping support for the protocol. HTTP/2 support was recently added to the Redfish specification. The largest performance increase in bmc usage is likely header compression. Almost all requests reuse the same header values, so the hpack based compression scheme in HTTP/2 allows OpenBMC to be more efficient as a transport, and has the potential to significantly reduce the number of bytes we're sending on the wire. This commit adds HTTP2 support to bmcweb through nghttp2 library. When static linked into bmcweb, this support adds 53.4KB to the bmcweb binary size. nghttp2 is available in meta-oe already. Given the experimental nature of this option, it is added under the meson option "experimental-http2" and disabled by default. The hope is to enable it at some point in the future. To accomplish the above, there a new class, HTTP2Connection is created. This is intended to isolate HTTP/2 connections code from HttpConnection such that it is far less likely to cause bugs, although it does duplicate about 20 lines of code (async_read_some, async_write_some, buffers, etc). This seems worth it for the moment. In a similar way to Websockets, when an HTTP/2 connection is detected through ALPN, the HTTP2Connection class will be instantiated, and the socket object passed to it, thus allowing the Connection class to be destroyed, and the HTTP2Connection to take over for the user. Tested: Redfish service validator passes with option enabled With option disabled GET /redfish/v1 in curl shows ALPN non negotiation, and fallback to http1.1 With the option enable GET /redfish/v1 in curl shows ALPN negotiates to HTTP2 Change-Id: I7839e457e0ba918b0695e04babddd0925ed3383c Signed-off-by: Ed Tanous <edtanous@google.com>
2023-06-24Break out request completion from connectionEd Tanous1-55/+2
There's a large part of the http::Connection class that has nothing to do with the connection at all, and is all about parsing, and finalizing the response. Break that portion out into its own method that can (in the future) be unit tested. Tested: Redfish service validator passes Change-Id: Ic608d432e69e25c0e0a1555ecc24ed62adba2664 Signed-off-by: Ed Tanous <edtanous@google.com>
2023-06-08Remove urlEncodeEd Tanous1-1/+1
All new uses should be using boost::urls::url now. This was the last usage. Tested: Logged into webui, and observed the correct URL behavior. In browser window /foobar Forwarded to /?next=/foobar#/login Which is correct. Note, this is different behavior slightly than before. It was found that the URI precedence goes query string THEN fragment, rather than the other way around that we had it. This was flagged when moving over to boost url structures. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: Ifb354537d71a43c531d7d380dd889cf646731e39
2023-06-05Break out serializer into its own cpp fileEd Tanous1-9/+1
This commit is entirely just moving code, such that not all compile units need to pull in the full html serializer. Tested: Unit tests pass. Pretty good coverage. Redfish service validator passes. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: Ifaebe9534c0693dc678fd994517563b89aca0cc5
2023-06-01Server-sent-event fixesEd Tanous1-2/+4
This makes several changes to server-sent events to allow it to merge to master. The routing system has been removed in leiu of using content-type eventstream detection. Timers have been added to the sse connections, and sse connections now rely on async_wait, rather than a full read. Tested: WIP Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: Id0ff0ebc2b3a795b3dba008e440556a9fdd882c2
2023-06-01Add Server-Sent-Event supportV-Sanjana1-4/+5
Server-Sent-Event is a standard describing how servers can initiate data transmission towards clients once an initial client connection has been established. Unlike websockets (which are bidirectional), Server-Sent-Events(SSE) are unidirectional and commonly used to send message updates or continuous data streams to a browser client. This is base patch for adding Server-Sent-Events routing support to bmcweb. Redfish EventService SSE style subscription uses SSE route for sending the Events/MetricReports to client which establishes the connection. Tested this patch with along with EventService SSE support patches and verified the functionalty on browser. Tested: - Tested using follow-up patches on top which adds support for Redfish EventService SSE style subscription and observed events are getting sent periodically. - Created SSE subscription from the browser by visiting https://<BMC IP>/redfish/v1/EventService/SSE Change-Id: I36956565cbba30c2007852c9471f477f6d1736e9 Signed-off-by: AppaRao Puli <apparao.puli@linux.intel.com> Signed-off-by: P Dheeraj Srujan Kumar <p.dheeraj.srujan.kumar@intel.com> Signed-off-by: V-Sanjana <sanjana.v@intel.com>
2023-05-19Capture all boost::system::error_codes by refEd Tanous1-1/+1
Capturing these possibly overloaded values by reference can avoid a copy in some cases, and it's good to be consistent. This change was made automatically by grep/sed. Tested: Code compiles. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: Iafeaca2a5dc52f39753b5a3880419d6bc943f81b
2023-05-19http_connection: Allow empty json objectsEd Tanous1-1/+1
Currently http_connection will produce empty body in the response if the res.jsonValue is empty, including empty array, object. This makes the output confusing in case a response does contain an empty object or array. Change the code to print the json object even if it's empty object or array. This patchset was previously reverted because of a regression, but this regression is fixed in 63529. Tested on previous commit: With an OEM URL that returns empty array depending on the system config, the response becomes `[]` instead of empty. Signed-off-by: Lei YU <yulei.sh@bytedance.com> Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I1d1bf01a5277ff1bc953b15d9fc410e10f941e70
2023-05-19Clean up preparePayloadEd Tanous1-10/+0
boost::beast::http::message::prepare_payload [1] can throw, which isn't really the behavior we want (as it throws to the io_context). Luckily, every part of that function is using public methods, and we can simplify it. In past commits, we've worked around this issue: 6295becabb9edba2edb53a3c0dddc13d2ffac8dd This is an attempt to fix it properly. [1] https://github.com/boostorg/beast/blob/ae01f0201dbf940cbc32d96d7a78dc584a02ab26/include/boost/beast/http/impl/message.hpp#L398 Redfish service validator passes Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: Ie88ddeecfd226bba75a7659cfb7ddddd38eb27cb
2023-05-12Revert "http_connection: Allow empty json objects"Ed Tanous1-1/+1
This reverts commit 02e01b5108d46720a0b438c0d79952464320d954. This commit is being reverted because it causes login failures on Firefox browsers. This commit originally was added with the idea that it did not fix anything on upstream, but made some peoples forks better. It appears to have broken some upstream things, so the right thing to do is to revert it until those breakages can be understood. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I04de84fca1a8de657f6941653f2a3e595ee725d5
2023-05-11clang-format: copy latest and re-formatPatrick Williams1-2/+2
clang-format-16 has some backwards incompatible changes that require additional settings for best compatibility and re-running the formatter. Copy the latest .clang-format from the docs repository and reformat the repository. Change-Id: I75f89d2959b0f1338c20d72ad669fbdc1d720835 Signed-off-by: Patrick Williams <patrick@stwcx.xyz>
2023-04-25http_connection: Allow empty json objectsLei YU1-1/+1
Currently http_connection will produce empty body in the response if the res.jsonValue is empty, including empty array, object. This makes the output confusing in case a response does contain an empty object or array. Change the code to print the json object even if it's empty object or array, so that the output is consistent with the `res.jsonValue`. Tested: With an OEM URL that returns empty array depending on the system config, the response becomes `[]` instead of empty. Signed-off-by: Lei YU <yulei.sh@bytedance.com> Change-Id: Ie97378a2cffce7b1fd6586a56b6cfa7d5c476dc1
2023-03-15Add asyncResp support to handleUpgradeP Dheeraj Srujan Kumar1-4/+15
This commit enables passing down the asyncResp (of the connection) to the handler of upgraded connections. This is already in place for normal requests (i.e. Class Router -> handle()) This change would enable any async calls that would be required before upgrade of the connection. For example, as on today, we have only Authentication of user in place for upgraded connection, but not Authorization. So, this asyncResp could further be used for such dbus calls to return informative response. This commit updates the signature of all the handleUpgrade() functions present in router.hpp to take in asyncResp object instead of normal response. Tested : - websocket_test.py Passed - KVM was functional in WebUI. Change-Id: I1c6c91f126b734e1b5573d5ef204fe2bf6ed6c26 Signed-off-by: P Dheeraj Srujan Kumar <p.dheeraj.srujan.kumar@intel.com>
2023-03-11Make url by value in RequestEd Tanous1-5/+7
There's some tough-to-track-down safety problems in http Request. This commit is an attempt to make things more safe, even if it isn't clear how the old code was wrong. Previously, the old code took a url_view from the target() string for a given URI. This was effectively a pointer, and needed to be updated in custom move/copy constructors that were error prone to write. This commit moves to taking the URI by non-view, which involves a copy, but allows us to use the default move and copy constructors, as well as have no internal references within Request, which should improve the safety and reviewability. There's already so many string copies in bmcweb, that this is unlikely to show up as any sort of performance regression, and simple code is much better in this case. Note, because of a bug in boost::url, we have to explicitly construct a url_view in any case where we want to use segments() or query() on a const Request. This has been reported to the boost maintainers, and is being worked for a long term solution. https://github.com/boostorg/url/pull/704 Tested: Redfish service validator passed on last commit in series. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I49a7710e642dff624d578ec1dde088428f284627
2023-03-03Allow logged in users to upload incrementallyEd Tanous1-15/+40
There are use cases where logged in users might want to upload a large file over a slow connection, and would exceed the 60 second timeout that bmcweb has. This commit would theoretically allow the user timer to be per-segment, allowing very long timeouts in the case of slow connections, so long as some progress was made within the 15 second window, which seems reasonable. If user authentication is disabled then there is no user session active in this case timer will be refreshed as long as progress was made. This seems like a better alternative compared to setting a very long (5-20 minute) timeout. Testing: - Loaded image on the system $ curl -k -H 'X-Auth-Token: <token>' -H 'Content-Type: application/octet-stream' -X POST -T ./obmc-phosphor-image-p10bmc.ext4.mmc.tar https://${bmc}:443/redfish/v1/UpdateService/update { "@odata.id": "/redfish/v1/TaskService/Tasks/0", "@odata.type": "#Task.v1_4_3.Task", "Id": "0", "TaskState": "Running", "TaskStatus": "OK" } - Tested image load using disable authentication and insecure http connections. - Ran few querries and those are fine. * curl -s -k https://${bmc}:443/redfish/v1/Managers * curl -s -k https://${bmc}:443/redfish/v1/Managers/bmc * curl -s -k https://${bmc}:443/redfish/v1/AccountService/Accounts * curl -s -k https://${bmc}:443/redfish/v1/Systems/system * curl -s -k https://${bmc}:443/redfish/v1/Chassis/chassis * curl -s -k https://${bmc}:443/redfish/v1/AccountService/LDAP/Certificates * curl -k -X POST https://${bmc}:443/redfish/v1/AccountService/Accounts -d '{"UserName": "user99", "Password": "pass123", "RoleId": "Administrator"}' * curl -k https://${bmc}:443/redfish/v1/AccountService/Accounts/user99 * curl -k -X DELETE https://${bmc}:443/redfish/v1/AccountService/Accounts/user99 * curl -k -H 'Content-Type: application/json' -X POST https://${bmc}:443/login -d '{"username" : "admin", "password" : "newpas1"}' * curl -k -H 'X-Auth-Token: ' -X PATCH https://${bmc}:443/redfish/v1/AccountService/Accounts/admin -d '{"Password":"newpas2"}' * curl -k -H 'X-Auth-Token: ' -X POST https://${bmc}:443/logout Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I579c86defdd199c140891a986d70ae2eca63b2aa Signed-off-by: Ninad Palsule <ninadpalsule@us.ibm.com>
2023-02-16Fix some more includesEd Tanous1-0/+1
clang-tidy warns on these when run directly in a header file. Fix them. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: Ib3366699c36e85644107690c23467f2ed22e398d
2023-02-11Fix overwriting mTLS sessionBoleslaw Ogonczyk Makowski1-15/+11
sessionIsFromTransport variable was always being overwritten to false [1], it caused userSession to get cleared [2] while it was also being used for mTLS session which prevented authentication in next requests and made cleanup code inaccessible [3]. Using the same variable for session from request and session created by mTLS broke single responsibility principle. Tested: Follow the guide in [4] to create a valid certificate for a user that can access some resource (for example /redfish/v1/Chassis) and create a file containing the address to it more than once in following format: url=https://BMC_IP/redfish/v1/Chassis url=https://BMC_IP/redfish/v1/Chassis curl --cert client-cert.pem --key client-key.pem -vvv --cacert CA-cert.pem -K addr_file -H "Connection: keep-alive" Before this change requests after first would fail with "401 Unauthorized" [1]: https://github.com/openbmc/bmcweb/blob/770b3ff239f96b419a791bed732f914899b8c202/http/http_connection.hpp#L468 [2]: https://github.com/openbmc/bmcweb/blob/770b3ff239f96b419a791bed732f914899b8c202/http/http_connection.hpp#L555 [3]: https://github.com/openbmc/bmcweb/blob/770b3ff239f96b419a791bed732f914899b8c202/http/http_connection.hpp#L283 [4]: https://github.com/openbmc/docs/blob/f4febd002df578bad816239b70950f84ea4567e8/security/TLS-configuration.md Change-Id: I4cf70ceb23c7a9b2668b2fcb44566f9971ac9ad4 Signed-off-by: Karol Niczyj <karol.niczyj@intel.com> Signed-off-by: Boleslaw Ogonczyk Makowski <boleslawx.ogonczyk-makowski@intel.com>
2023-01-25Timer not started if user session is logged inNinad Palsule1-1/+0
In startDeadline(), If user session is logged in then we simply return without starting the timer. This fix fixes that issue. Tested: Loaded code change on system and verified timeout now works. Change-Id: Ia4359b6dffb3015eb20a2a9d0ff2e5e6dab3500d Signed-off-by: Ninad Palsule <ninadpalsule@us.ibm.com>
2023-01-21Replace "Fix keepalive false" with patchset 3Ed Tanous1-3/+6
This reverts commit 5ae6f9254161f7229216c08b591e31eaf10f69e4. And replaces it with patchset 3 from the same review, for which was tested to work properly. This commit was merged erroneously. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I201924ad27d33923d43bdf82ecb016a0f214b4dd
2023-01-18Fix a boatload of #includesEd Tanous1-4/+4
Most of these missing includes were found by running clang-tidy on all files, including headers. The existing scripts just run clang-tidy on source files, which doesn't catch most of these. Tested: Code compiles Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: Ic741fbb2cc9e5e92955fd5a1b778a482830e80e8
2023-01-17Add check for globalsEd Tanous1-0/+1
We don't follow this cpp core guidelines rule well. This is something that we should aspire to cleaning up in the future, but for the moment, lets turn the rule on in clang-tidy to stop the bleeding, add ignores for the things that we know need some better abstractions, and work on these over time. Most of this commit is just adding NOLINTNEXTLINE exceptions for all of our globals. There was one case in the sensor code where clang correctly noted that those globals weren't actually const, which got missed because of the use of auto. Tested: CI should be good enough for this. Passes clang-tidy. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: Ieda08fee69a3b209d4b3e9771809a6c41524f066
2023-01-12Fix keepalive falseEd Tanous1-4/+4
When we changed [1] to using a std::move of the Request object instead of an unsafe reference, we missed one spot where we were using the request object, post handle. This commit moves the keepalive function to be set on the response object before calling handle. Tested: curl request with -H "Connection: close", -H "Connection: keep-alive" and no header all return the correct values. [1] 2d6cb56b6b47c3fbb0d234ade5c1208edb69ef1f Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I91fe32162407f1e1bdfcc06f1751e02d11f8a697
2023-01-12Clean up http end_of_stream tracesMyung Bae1-1/+2
http::end_of_stream log is not properly detected and thus it is being traced as error. It will be fixed to be traced as warning so they do not show up when just error traces are enabled (new default) Here's the log we was getting: Jan 10 13:34:32 ever6bmc bmcweb[2496]: (2023-01-10 13:34:32) \ [ERROR "http_connection.hpp":784] 0x14bd360 Error while \ reading: end of stream Change-Id: I6ecee813c4f7806a676ba0ad3e4ab1a8f78747fd Signed-off-by: Myung Bae <myungbae@us.ibm.com>
2023-01-12Refactor mtls callbacks into their own fileEd Tanous1-161/+20
Mutual TLS is non-trivial enough that it definitely shouldn't be done in an inline lambda method. This commit moves the code. Tested: WIP. This is a pretty negligible code move; Minimal touch testing should be good. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I7a15a6bc66f4d8fb090411509549628f6d1045a5
2022-12-28Add CBOR supportEd Tanous1-2/+8
CBOR is a more efficient way to represent json, and something that, as you can see from this patch, is relatively trivial to implement in our current nlohmann json handlers. This allows users that specify an accepts header of "application/cbor" to request the BMC produce a cbor response. This feature adds 1520 bytes (1.48KB) to the binary size of bmcweb. For ServiceRoot GET /redfish/v1 Accepts: application/json - returns json GET /redfish/v1 Accepts: application/cbor - returns cbor GET /redfish/v1 Accepts: */* - returns json GET /redfish/v1 Accepts: text/html - returns html GET /redfish/v1 no-accepts header - returns json For service root, CBOR encoding drops the payload size from 1520 bytes on my system, to 1021 byes, which is a significant improvement in the number of bytes we need to compress. Redfish-service-validator passes. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I847e678cf79dfd7d55e6d3b26960c419e47063af
2022-12-21Return bad request if can't constructGunnar Mills1-0/+2
If given a bad URI, e.g. "https://$bmc/?a=id;" return http bad request (400) like we do other places, e.g. a few lines below. Certain scanners expect to see bad request when crawling and probing for vulnerabilities and using not valid URIs. Just dropping the connection causes errors with these scanners. They think connection problem or worse the server was taken down. Tested: Tested downstream https://$bmc/?a=id; returns bad request. Change-Id: I99a52d4c5e7f366046c5de1cf22c4db95ab39e13 Signed-off-by: Gunnar Mills <gmills@us.ibm.com>
2022-11-02Implement If-None-Match support for caching clientEd Tanous1-0/+9
This commit implements support for the If-None-Match header on http requests. This can be combined with the 89f180089bce9cc431d0b1053410f262f157b987 commit for producing ETag to allow a client to have a highly efficient cache, while still pulling data from the BMC. This behavior is documented several places, in W3C produced docs[1], as well as section 7.1 of the Redfish specification: ''' A service only returns the resource if the current ETag of that resource does not match the ETag sent in this header. If the ETag in this header matches the resource's current ETag, the GET operation returns the HTTP 304 status code. ''' Inside bmcweb, this behavior is accomplished in a relatively naive way, by creating the complete request, then doing a direct ETag comparison between the generated data and the request header. In the event the two match, 304 not-modified is returned, in-line with both the Redfish specification and the HTTP RFC. [1] https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/If-None-Match Tested (on previous rebase): First, request ServiceRoot curl --insecure -vvvv --user root:0penBmc https://192.168.7.2/redfish/v1 This returns a header similar to: < ETag: "ECE52663" Taking that ETag, and putting it into an If-None-Match header: ``` curl --insecure -vvvv -H "If-None-Match: \"ECE52663\"" \ --user root:0penBmc https://192.168.7.2/redfish/v1 ``` Returns: < HTTP/1.1 304 Not Modified ... < Content-Length: 0 Showing that the payload was not repeated, and the response size was much.... much smaller on the wire. Performance was not measured as part of this testing, but even if it has no performance impact (which is unlikely), this change is still worthwhile to implement more of the Redfish specification. Redfish-service-validator passes. Redfish-protocol-validator passes 1 more atom in comparison to previous. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I1e7d41738884593bf333e4b9b53d318838808008
2022-10-07Move ClientID parameter out of OEMEd Tanous1-2/+1
In 2022.2, Redfish added support for the Context parameter on the Session Resource. This parameter has the same function that the OemSession.ClientId field served. This commit moves all the existing ClientId code to produce Context as well. Functionally, this has one important difference, in that Context in Redfish is optionally provided by the user, which means we need to omit it if not given by the user. The old implementation left it set to empty string (""). Because of this, a few minor interfaces need to change to use std::optional. Existing uses of clientId are moved to using value_or("") to keep the same behavior as before. Tested: curl --insecure -X POST -d "{\"UserName\": \"root\", \"Password\": \"0penBmc\"}" https://192.168.7.2/redfish/v1/SessionService/Sessions Returns a Session object with no Context key present curl --insecure -X POST -d "{\"UserName\": \"root\", \"Password\": \"0penBmc\", \"Context\": \"Foobar\"}" https://192.168.7.2/redfish/v1/SessionService/Sessions Returns a Session object with: "Context": "Foobar" Subsequent Gets of /redfish/v1/SessionService/Sessions/<sid> return the same session objects, both with and without Context. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I4df358623f93f3e6cb659e99970ad909cefebc62
2022-09-13Make Accepts: */* default to JSONEd Tanous1-2/+2
There are apparently libraries that use an Accepts header of */*, instead of simply omitting the Accepts header, or providing a more correct value for that header. 99351cd856038475cac146029e5db03767a1459c Improve content type The above commit attempted to refine our handling of this header, and changed the behavior for */* to default to HTML. While this is arguably "correct" to the HTTP RFC, and the clients that do this are definitely wrong in their implementation, we can try to shield them a little from their incorrectness, and we can certainly avoid compatibility issues with these clients, without effecting the clients that implement the spec correctly. This commit changes the priority order of Accepts header to JSON then HTML, instead of the other way around. Tested: curl --insecure -H "Accept: */*" --user root:0penBmc \ https://192.168.7.2/redfish/v1 Now returns a json payload. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I7850a3afb0b5d635f8d632fb0a9f790e53fe4466
2022-09-13Improve content typeEd Tanous1-1/+10
We have a number of specialized content-type functions for varying levels of degree, and most of them rely on quite a few strings. This commit changes them to consolidate on two APIs. isContentTypeSupported, which as the name implies, takes a single content type, and returns a bool about whether or not that content type is allowed. getPreferedContentType, which takes an array of multiple options, and fine the first one in the list that matches the clients expected string. These two functions makes these functions more able to be reused in the future, and don't require specialized entries for each possible type or combination of types that we need to check for. Tested: Unit tests passing. Pretty good coverage. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I8b976d0cefec5f24e62fbbfae33d12cc803cb373
2022-08-06Use enum overload for field settingEd Tanous1-1/+2
There are two overloads of addHeader, one that takes a string, and one that takes a boost enum. For most common headers, boost contains a string table with all of those entries anyway, so there's no point in duplicating the strings, and ensures that we don't make trivial mistakes, like capitalization or - versus underscore that aren't caught at compile time. Tested: This saves a trivial amount (572 bytes) of compressed binary size. curl --insecure -vvv --user root:0penBmc https://192.168.7.2/redfish/v1 returns < Content-Type: application/json curl --insecure -vvv -H "Accept: text/html" --user root:0penBmc https://192.168.7.2/redfish/v1 Returns < Content-Type: text/html;charset=UTF-8 Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I34c198b4f9e219247fcfe719f9b3616d35aea3dc
generated by cgit v1.2.3 (git 2.39.0) at 2024-06-14 15:17:49 +0300