Allow logged in users to upload incrementally

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 h

Allow logged in users to upload incrementally

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>

show more ...

Take boost error_code by reference

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 co

Take boost error_code by reference

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

show more ...

Pass string views by value

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] http

Pass string views by value

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

show more ...

HTTP Client: Increase max conns and reqs

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 req

HTTP Client: Increase max conns and reqs

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

show more ...

Add option for validating content-type header

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

Add option for validating content-type header

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>

show more ...

Fix missing include

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 su

Fix missing include

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

show more ...

Fix some more includes

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

Fix overwriting mTLS session

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 p

Fix overwriting mTLS session

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>

show more ...

Aggregation: Better handle dropped requests

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

Aggregation: Better handle dropped requests

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

show more ...

Timer not started if user session is logged in

In startDeadline(), If user session is logged in then we simply return
without starting the timer. This fix fixes that issue.

Tested:
Loaded code ch

Timer not started if user session is logged in

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>

show more ...

Replace "Fix keepalive false" with patchset 3

This reverts commit 5ae6f9254161f7229216c08b591e31eaf10f69e4.

And replaces it with patchset 3 from the same review, for which was
tested to work proper

Replace "Fix keepalive false" with patchset 3

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

show more ...

Fix a boatload of #includes

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

Fix a boatload of #includes

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

show more ...

Add check for globals

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-ti

Add check for globals

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

show more ...

Fix keepalive false

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 commi

Fix keepalive false

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

show more ...

Clean up http end_of_stream traces

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 er

Clean up http end_of_stream traces

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>

show more ...

Refactor mtls callbacks into their own file

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 pr

Refactor mtls callbacks into their own file

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

show more ...

Add CBOR support

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 a

Add CBOR support

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

show more ...

Add missing usage of new verb class in router

Despite introduction of a new enum class containing method verbs,
some functions were still using the one from Boost. This had caused
erratic behaviour

Add missing usage of new verb class in router

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>

show more ...

Return bad request if can't construct

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 ba

Return bad request if can't construct

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>

show more ...

Change variable scopes

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 pas

Change variable scopes

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

show more ...

Prepare for boost::url upgrade

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 pulle

Prepare for boost::url upgrade

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

show more ...

Implement If-Match header in Http layer

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 implementati

Implement If-Match header in Http layer

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

show more ...

Code move to prevent circular dependency

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

Code move to prevent circular dependency

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

show more ...

Make router take up less space for verbs

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 b

Make router take up less space for verbs

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

show more ...

Implement If-None-Match support for caching client

This commit implements support for the If-None-Match header on http
requests. This can be combined with the
89f180089bce9cc431d0b1053410f262f157b9

Implement If-None-Match support for caching client

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

show more ...