| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
Partial revert of 915d2d4e59be56958b04a79ba96e0242ef735f44
Request object was being moved out of the owning Connection object,
which would then try to use the Request again in completeRequest(). Just
pass around a reference instead of taking ownership.
The obvious symptom was that Redfish pages were served as json in the
browser instead of HTML, because the headers in the Request were no
longer valid after being moved.
Tested: /redfish/v1 is served as HTML in the browser again.
Change-Id: Iae68a68817146c28377bbcade04716725e4a6096
Signed-off-by: Jonathan Doman <jonathan.doman@intel.com>
|
|
With Redfish aggregation, responses from satellite BMCs can be on the
order of MBs due to use cases like logging or binary payloads.
Offloading $expand could similar result in responses that exceed the
current read limit of 128 KB.
Splits the connection pools used for aggregation and EventService so
that the response read limit is 50MB for responses associated with
aggregation. Pools used by EventService keep the current limit of 2^17
bytes or 128 KB. It also propogates a ConnectionPolicy object that gets
instantiated within HttpClient, which allows per-client policies for
retry/byte limits. This allows EventService and aggregation to have
different policies.
Tested:
With aggregation enabled I was able to return a response from a
satellite BMC which was than 2MB. Ran the Redfish Mockup Creator and it
was able to successfully query all aggregated resources as part of
walking the tree. Also verified that HTTP push events still work with
EventListener.
Change-Id: I91de6f82aadf8ad6f7bc3f58dfa0d14c0759dd47
Signed-off-by: Carson Labrado <clabrado@google.com>
Signed-off-by: Ed Tanous <edtanous@google.com>
|
|
This code is doing some copy operations instead of moves. This commit
moves to passing a Request&& through the validate function, so that we
don't have to split the usage of req between the two paths.
Ideally someday we'd run Request as a shared_ptr like we do with
Response and remove the possibility of this, but that's a longer term
thing.
This fixes a regression introduced in
7e9093e625961f533250a6c193c1a474e98007c4
Tested:
Redfish service validator passes.
/redfish/v1/Systems/system passes.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ib6d99726a64326b7c8bad15bc9d4ca774ab6256d
|
|
Any of our things taking URLs should be taking url_view by value,
similar to how we take string_view.
From the beast documentation:
"...it acts like a string_view in terms of ownership." [1]
Therefore, we should treat it like we treat string_view, and take by value, not reference.
[1] https://www.boost.org/doc/libs/master/libs/url/doc/html/url/ref/boost__urls__url_view.html
Tested:
Stacked these patches. Redfish service validator passes.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I696b495f4aa04984225853f653cc175c0eaad79d
|
|
This commit enables privilege check for user(s) in case of upgraded
connections.
Currently users with no privileges will also be able to access
Websockets connections (Ex: KVM).
The privilege check was already in place for normal connections (i.e.
router->handle()). This commit lifts off the privilege check code and
moves it into a common function (validatePrivilege()), which can be used
both by handle() and handleUpgrade() and register required callback to
be called.
Also, the const qualifier for Request in the handleUpgrade() function's
signature is removed to enable setting "isConfigureSelf" field of
request. The signature of handleUpgrade() is made identical to handle()
Tested:
- websocket_test.py Passed
- Admin and Operator users are able to access KVM on WebUI
- Readonly User was unable to access KVM on WebUI
Signed-off-by: P Dheeraj Srujan Kumar <p.dheeraj.srujan.kumar@intel.com>
Change-Id: I6f743c27e7e6077f1c6c56e6958922027e4404e8
|
|
handleUpgrade is pretty simple, and has no methods that can throw. This
was there previously because of handling exceptions in handle() and was
copied to handleUpgrade(), even though it doesn't make a ton of sense to
do so, given the throw conditions don't really exist, and start()
doesn't call path handlers directly anymore.
Tested: Code compiles. Only affects error conditions.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Iedd7e42b7e908282ab2c2d698e9f6c815b88e857
|
|
Tested: Tested in 46991
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ia714c7de02d714e636d5624ea884dbb6633baee5
|
|
This lambda was very large, and needs broken into a method.
Tested: Tested in 46991
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I86c1ac749580eb5b42c347808b4660c894a9bb9b
|
|
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>
|
|
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
|
|
Per cpp core guidelines, these should be methods.
Tested: on last patchset of the series.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ib16479db9d2b68da68e7ad6e825c7e205c64f1de
|
|
Per cpp core guidelines, we should be returning this via a function
call, not a direct member variable. Doing this also improves the
safety, as we don't have to remember to move the references over in a
move.
Tested: Tested as part of top patch in series.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I837d6fd277ffa076ba5425003d6e6ee79204d014
|
|
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>
|
|
By convention, we should be following boost here, and passing error_code
by reference, not by value. This makes our code consistent, and removes
the need for a copy in some cases.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Id42ea4a90b6685a84818b87d1506c11256b3b9ae
|
|
string_view should always be passed by value; This commit is a sed
replace of the code to make all string_views pass by value, per general
coding guidelines[1].
[1] https://quuxplusone.github.io/blog/2021/11/09/pass-string-view-by-value/
Tested: Code compiles.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I55b342a29a0fbfce0a4ed9ea63db6014d03b134c
|
|
With Redfish Aggregation there is a need for a large amount of
requests to be sent to a satellite BMC. Our current throughput of
4 connections with a 50 request buffer can be easily overwhelmed due
to the use of $expand. Also, BMCWeb itself can support 100 active
connections so multiple clients could be attempting to query a
satellite BMC that is being aggregated.
Increase the maximum number of connections to a destination to 20 and
increase the buffer request size to 500. These figures should be fine
since the requests themselves have a very small memory footprint and
the number of active connections is only a fifth of what the BMCWeb
in the satellite BMC should be able to support.
Note that these figures are an improvement over the current values.
They allowed making multi-level $expand requests without dropping any
subrequests due to the buffer becoming full. Further tuning will be
done in a future patch if it is determined that optimal performance can
be obtained by choosing different values.
Tested:
Debug logs after sending a multi-level $expand query showed the entire
connection pool being filled as well as well over 100 Requests being
queued. The additional connections provided enough throughput to
handle repeated simultaneous requests.
Signed-off-by: Carson Labrado <clabrado@google.com>
Change-Id: I96f165b5fbc76086e55b65faaaa49eb2753f8ef6
|
|
For systems implementing to the OWASP security guidelines[1] (of which all
should ideally) we should be checking the content-type header all times
that we parse a request as JSON.
This commit adds an option for parsing content-type, and sets a default
of "must get content-type". Ideally this would not be a breaking
change, but given the number of guides and scripts that omit the content
type, it seems worthwhile to add a trapdoor, such that people can opt
into their own model on how they would like to see this checking work.
Tested:
```
curl --insecure -H "Content-Type: application/json" -X POST -D headers.txt https://${bmc}/redfish/v1/SessionService/Sessions -d '{"UserName":"root", "Password":"0penBmc"}'
```
Succeeds.
Removing Content-Type argument causes bmc to return
Base.1.13.0.UnrecognizedRequestBody.
[1] cheatsheetseries.owasp.org/cheatsheets/REST_Security_Cheat_Sheet.html
Change-Id: Iaa47dd563b40036ff2fc2cacb70d941fd8853038
Signed-off-by: Ed Tanous <edtanous@google.com>
|
|
This arguably isn't a missing include in boost::beast, but because beast
only includes iofwd (the forward declaration of iostreams) we need to
include the full header here, to support converting the verb to and from
strings.
Tested: clang-tidy stops complaining.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I93a930e456617cead3ff3928d29c2bfea3163ba6
|
|
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
|
|
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>
|
|
It's possible for HTTP client's request buffer to become full
(especially when $expand is used). Instead of ignoring the requests
we should provide a 429 Too Many Requests response for the provided
callback to process.
The aggregator's response handling also needs to account for this
possibility so that it only completely overwrites the asyncResp
object when it receives a response from a satellite.
Also added more test cases for the response processing functions.
Tested:
Unit tests passed
Flooded aggregator with requests for satellite resources. Requests
began returning 429 Too Many Requests errors after the request buffer
became full.
Signed-off-by: Carson Labrado <clabrado@google.com>
Change-Id: Ib052dc0454d759de7fae761977ca26d6b8d208e5
|
|
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>
|
|
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
|
|
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
|
|
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
|
|
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
|
|
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>
|
|
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
|
|
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
|
|
Despite introduction of a new enum class containing method verbs,
some functions were still using the one from Boost. This had caused
erratic behaviour when trying to create a websocket
(e.g. /nbd/<str>), because enum value of old type was compared to the
one of new type. This change fixes that.
Tested:
Verified that websockets are now created without errors.
Change-Id: I52c874de9b02463618143d3b031f5c795dd42ad8
Signed-off-by: Michal Orzel <michalx.orzel@intel.com>
Signed-off-by: Snehalatha Venkatesh <snehalathax.v@intel.com>
|
|
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>
|
|
cppcheck correctly notes that a lot of our variables can be declared at
more specific scopes, and in every case, it seems to be correct.
Tested: Redfish service validator passes. Unit test coverage on others.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ia4414410d0e8f74a3bd40fdc0e0232450d1a6416
|
|
The new boost URL now interops properly with std::string_view, which is
great, and cleans up a bunch of mediocre code to convert one to another.
It has also been pulled into boost-proper, so we no longer need a
boost-url dependency that's separate.
Unfortunately, boost url makes these improvements by changing
boost::string_view for boost::urls::const_string, which causes us to
have some compile errors on the missing type.
The bulk of these changes fall into a couple categories, and have to be
executed in one commit.
string() is replaced with buffer() on the url and url_view types
boost::string_view is replaced by std::string_view for many times, in
many cases removing a temporary that we had in the code previously.
Tested: Code compiles with boost 1.81.0 beta.
Redfish service validator passes.
Pretty good unit test coverage for URL-specific use cases.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I8d3dc89b53d1cc390887fe53605d4867f75f76fd
|
|
If-Match is a header in the HTTP specification[1] designed for handling
atomic operations within a given HTTP tree. It allows a mechanism for
an implementation to explicitly declare "only take this action if the
resource has not been changed". While most things within the Redfish
tree don't require this level of interlocking, it continues to round out
our redfish support for the specific use cases that might require it.
Redfish specification 6.5 states:
If a service supports the return of the ETag header on a resource, the
service may respond with HTTP 428 status code if the If-Match or
If-None-Match header is missing from the PUT or PATCH request for the
same resource, as specified in RFC6585
This commit implements that behavior for all handlers to follow the
following flow.
If If-Match is present
Repeat the same request as a GET
Compare the ETag produced by the GET, to the one provided by If-Match
If they don't match, return 428
if they do match, re-run the query.
[1] https://www.rfc-editor.org/rfc/rfc2616#section-14.24
As a consequence, this requires declaring copy and move constructors
onto the Request object, so the request object can have its lifetime
extended through a request, which is very uncommon.
Tested:
Tests run on /redfish/v1/AccountService/Accounts/root
PATCH with correct If-Match returns 200 success
PATCH with an incorrect If-Match returns 419 precondition required
GET returns the resource as expected
Redfish service validator passes
Redfish protocol validator passes! ! ! ! !
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I530ab255259c32fe4402eb8e5104bd091925c77b
|
|
While implementing https://gerrit.openbmc.org/c/openbmc/bmcweb/+/57932,
there has been an issue where there is a circular dependency between
routing.hpp and privileges.hpp. This code move predates this change
to resolve it before implementing the heart of redfish authz.
Circular dependency will occur when we try to use the http verb
index variables in privilege.hpp. If this occurs routing.hpp
and privilege.hpp will co-depend on each other
and this code move prevents this from occuring.
Tested:
bitbake bmcweb
Code compiles (code move only)
Redfish Validator passed on next commit
Signed-off-by: Edward Lee <edwarddl@google.com>
Change-Id: I46551d9fe222e702d239ed3ea6d3d7e505d488c8
|
|
As is, the router designates routes for every possible boost verb, of
which there are 31. In bmcweb, we only make use of 6 of those verbs, so
that ends up being quite a bit of wasted space and cache non-locality.
This commit invents a new enum class for declaring a subset of boost
verbs that we support, and a mapping between bmcweb verbs and boost
verbs.
Then it walks through and updates the router to support converting one
to another.
Tested:
Unit Tested
Redfish Service Validator performed on future commit
Signed-off-by: Ed Tanous <edtanous@google.com>
Signed-off-by: Edward Lee <edwarddl@google.com>
Change-Id: I3c89e896c632a5d4134dbd08a30b313c12a60de6
|
|
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
|
|
Add helper function to append pieces to existing url to allow more
flexible control over the url. This allows us to avoid have each
resource append the pieces outside of the utility functions and help
maintain all url modifications in a central place for easy management.
Tested: Does not affect Redfish Tree. Unit Test passed.
Change-Id: I751f3c120cbadb465915b12aa253edd53ef32123
Signed-off-by: Willy Tu <wltu@google.com>
|
|
Now that we are using timer.async_wait() with the async http
operations we need to account for the scenario where the timer fails
before the operation fails. When that occurs we need to abort the
operation once its callback gets called.
Currently we proceed as if the timer doesn't exist. This causes a
fault if one of the operations times out. This patch adds a check
to the start of each async operation so that we do not continue with
the normal message sending flow when an operation times out.
Tested:
In order to create a connection timeout I created a dummy interface
and set the IP of my satellite BMC to route to the interface:
ip link add dummy0 type dummy
ip link set dev dummy0 up
ip route add 120.60.30.15 dev dummy0
All packets sent to 120.60.30.15 will get dropped and thus connection
attempts will timeout. This does not cause bmcweb to crash.
To make the satellite reachable again I used
this command to delete the routing:
ip route del 120.60.31.15 dev dummy0
After doing so messages were once again able to be forwarded correctly
to the satellite.
Signed-off-by: Carson Labrado <clabrado@google.com>
Change-Id: Ie8d022c2195838e383eefcd0e12ae8cfab76e3e1
|
|
This commit fixed several places (but not all) where wrong include
directory is specified and prevent the clean up in the chidren changes.
Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: Ibbba62e2c0cfe3583a65f1befa1b233bd3eebf19
|
|
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
|
|
Like other C++ projects, unit tests normally are in a separate repo and
respect the folder structure of the file under test.
This commit deleted all "ut" folder and move tests to a "test" folder.
The test folder also has similar structure as the main folder.
This commit also made neccessary include changes to make codes compile.
Unused tests are untouched.
Tested: unit test passed.
Reference:
[1] https://github.com/grpc/grpc/tree/master/test
[2] https://github.com/boostorg/core/tree/414dfb466878af427d33b36e6ccf84d21c0e081b/test
[3] Many other OpenBMC repos: https://github.com/openbmc/entity-manager/tree/master/test
[4] https://stackoverflow.com/questions/2360734/whats-a-good-directory-structure-for-larger-c-projects-using-makefile
Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: I4521c7ef5fa03c47cca5c146d322bbb51365ee96
|
|
The existing code doesn't propogate errors of subqueries correctly.
This commit corrects the behavior, so that the final response gets all
error message of subqueries and the "highest priority" HTTP code.
DMTF doesn't specify how expand queries handle error codes, since using
subqueries is an implementation choice that we made in this project.
What we did here follows existing behavior of this project, and follows
the error message section of the Redfish spec;
[1] https://redfish.dmtf.org/schemas/DSP0266_1.15.1.html#error-responses
As for now, this commit uses the worst HTTP code among all the error
code. See query_param.hpp, function |propogateErrorCode| for detailed
order of the errror codes.
Tested:
1. this is difficult to test, but I hijacked the code so it returns
errors in TaskServices, then I verified that "/redfish/v1?$expand=."
correctly returns 500 and the gets the error message set.
2. unit test so that when there are multiple errors, the final response
gets a generate error message.
Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: I0c1ebdd9015f389801db9150d687027485f1203c
|
|
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
|
|
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
|
|
There's a few last calls that are inline lambdas. As we've been moving
away from inline lambdas due to the problems with debug logging and
usability, this file is the next cleanup.
Tested:
Ran Redfish-Event-Listener, and set an event into /var/log/redfish with
```
echo "2022-08-04T21:37:49.156798+00:00 OpenBMC.0.1.ServiceFailure,com.intel.DomainMapper.service" > /var/log/redfish
```
Simulating a crash. Saw the event get sent to Redfish-Event-Listener.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I61361eef03d1301dc126a28c695fdd74e504f891
|
|
Similar to the 404 handler, add a 405 handler for registering custom 405
handlers for a given tree. The primary use case is for protocols like
redfish that support specific messages for 405 handlers that don't have
an empty body.
Tested: Unit tests pass.
PATCH /redfish/v1 returns 405 Method Not Allowed
POST /redfish/v1/Chassis returns 405 Method Not Allowed
POST /redfish/v1/Chassis/foo returns 405 Method Not Allowed
PATCH /redfish/v1/foo/bar returns 404 Not Found
GET /redfish/v1 returns ServiceRoot
GET /redfish/v1/Chassis returns Chassis collection
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ib0afd23d46bb5b88f89cf1e3f4e0606a48ae47ca
Signed-off-by: Carson Labrado <clabrado@google.com>
|
|
Different HTTP protocols have different http responses for 404. This
commit adds support for registering a route designed to host a handler
meant for when a response would otherwise return. This allows
registering a custom 404 handler for Redfish, for which all routes will
now return a Redfish response.
This was in response to the 404 handler not working in all cases (in the
case of POST/PATCH/DELETE). Allowing an explicit registration helps to
give the intended behavior in all cases.
Tested:
GET /redfish/v1/foo returns 404 Not found
PATCH /redfish/v1/foo returns 404 Not found
GET /redfish/v1 returns 200 OK, and content
PATCH /redfish/v1 returns 405 Method Not Allowed
With Redfish Aggregation:
GET /redfish/v1/foo gets forwarded to satellite BMC
PATCH /redfish/v1/foo does not get forwarded and returns 404
PATCH /redfish/v1/foo/5B247A_bar gets forwarded
Unit tests pass
Redfish-service-validator passes
Redfish-Protocol-Validator fails 7 tests (same as before)
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I731a5b4e736a2480700d8f3e81f9c9c6cbe6efca
Signed-off-by: Carson Labrado <clabrado@google.com>
|
|
tcp_stream has several problems in its current implementation. First,
it takes up a significant amount of binary size, for features that we
don't use. Next, it has some implied guarantees about timeouts that we
erronously rely on, namely that async_connect will timeout
appropriately given the tcp_stream timeout (it doesn't).
We already have a timer present in the ConnectionInfo class anyway, this
commit just makes use of it for ALL timeout operations, rather than just
the connect timeout operations. This flow is roughly analogous to what
we do in http_connection.hpp already, so the pattern is well troden.
This saves 2.8% of the binary size of bmcweb, and solves several race
conditions.
Tested:
Relying on Carson: Aggregated a sub-bmc, and ensured that top level
collections returned correctly under GET /redfish/v1/Chassis
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I72e8e97b667826f272bb4921afc2b16907f3b271
|
|
This commit adds the initial SSL support for http_client which can be
used for sending asynchronous Events/MetricReports to subscribed Event
Listener servers over secure channel.
Current implementation of http client only works for http protocol.
With current implementation, http client can be configured to work
with secure http (HTTPS). As part of implementation it adds the SSL
handshake mechanism and enforces the peer ceritificate verification.
The http-client uses the cipher suites which are supported by mozilla
browser and as recommended by OWASP. For better security enforcement
its disables the SSLv2, SSLv3, TLSv1, TLSv1.1 as described in below
OWASP cheetsheet.
It is validated with RootCA certificate(PEM) for now. Adding support
for different certificates can be looked in future as need arises.
[1]: https://cheatsheetseries.owasp.org/cheatsheets/TLS_Cipher_String_Cheat_Sheet.html
Tested:
- Created new subscription with SSL destination(https) and confirmed
that events are seen on EventListener side.
URI: /redfish/v1/EventService/Subscriptions
Method: POST
Body:
{
"Context": "CustomText",
"Destination": "https://<IP>:4000/service/collector/event_logs",
"EventFormatType": "Event",
"DeliveryRetryPolicy": "RetryForever",
"Protocol": "Redfish"
}
- Unit tested the non-SSL connection by disabling the check in code
(Note: EventService blocks all Non-SSL destinations). Verified that
all events are properly shown on EventListener.
URI: /redfish/v1/EventService/Subscriptions
Method: POST
Body:
{
"Context": "CustomText",
"Destination": "http://<IP>:4001/service/collector/event_logs",
"EventFormatType": "Event",
"Protocol": "Redfish"
}
- Combined above two tests and verified both SSL & Non-SSL work fine in
congention.
- Created subscription with different URI paths on same IP, Port and
protocol and verified that events sent as expected.
Change-Id: I13b2fc942c9ce6c55cd7348aae1e088a3f3d7fd9
Signed-off-by: AppaRao Puli <apparao.puli@intel.com>
Signed-off-by: Ed Tanous <edtanous@google.com>
|
