Fix shadowed variable issues

This patchset is the conclusion of a multi-year effort to try to fix
shadowed variable names. Variables seem to be shadowed all over, and in
most places they exist, the

Fix shadowed variable issues

This patchset is the conclusion of a multi-year effort to try to fix
shadowed variable names. Variables seem to be shadowed all over, and in
most places they exist, there's a "code smell" of things that aren't
doing what the author intended.

This commit attempts to clean up these in several ways by:
1. Renaming variables where appropriate.
2. Preferring to refer to member variables directly when operating
within a class
3. Rearranging code so that pass through variables are handled in the
calling scope, rather than passing them through.

These patterns are applied throughout the codebase, to the point where
-Wshadow can be enabled in meson.build.

Tested: Code compiles, unit tests pass. Still need to run redfish
service validator.

Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: If703398c2282f9e096ca2694fd94515de36a098b

show more ...

http: router_test: fix namespace

Add using directives and namespaces.

Teseted: unit test passed.

Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: I4c04f07484a2c1ce29ae26b523d62071ea0399

http: router_test: fix namespace

Add using directives and namespaces.

Teseted: unit test passed.

Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: I4c04f07484a2c1ce29ae26b523d62071ea03996c

show more ...

http: router_test: fix headers

IWYU. Use <> for dependency headers and "" for bmcweb headers.

Reference:
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#sf12-prefer-the-quoted-form-of-

http: router_test: fix headers

IWYU. Use <> for dependency headers and "" for bmcweb headers.

Reference:
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#sf12-prefer-the-quoted-form-of-include-for-files-relative-to-the-including-file-and-the-angle-bracket-form-everywhere-else

Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: I02537f16974ab96a1e4e4e1ee6e1db1a9c679c68

show more ...

http: utility_test: fix namespace

Removed unnecessary using directives. Add necessary using directives.

Tested: unit test passed.

Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: I36948

http: utility_test: fix namespace

Removed unnecessary using directives. Add necessary using directives.

Tested: unit test passed.

Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: I36948b531c4e75dbae6837601894839b23b2d3d1

show more ...

http: utility_test: fix headers

IWYU. Use <> for dependency headers and "" for bmcweb headers.

Reference:
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#sf12-prefer-the-quoted-form-of

http: utility_test: fix headers

IWYU. Use <> for dependency headers and "" for bmcweb headers.

Reference:
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#sf12-prefer-the-quoted-form-of-include-for-files-relative-to-the-including-file-and-the-angle-bracket-form-everywhere-else

Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: I0338d475990eaf2f0f8105b01d63c2d9bd9fc446

show more ...

HttpClient: Increase httpReadBodyLimit

Testing of https://gerrit.openbmc.org/c/openbmc/bmcweb/+/53310 revealed
that recvMessage() was failing because the body limit was being
exceeded. httpReadBody

HttpClient: Increase httpReadBodyLimit

Testing of https://gerrit.openbmc.org/c/openbmc/bmcweb/+/53310 revealed
that recvMessage() was failing because the body limit was being
exceeded. httpReadBodyLimit was being used to set both the body limit
as well as the buffer size. Add a different variable to set the buffer
size so that the body limit can be doubled to 16K while the fixed
buffer can be set as a smaller size of 4K.

Tested:
Queries using the mentioned patch stopped failing after applying this
change.

Signed-off-by: Carson Labrado <clabrado@google.com>
Change-Id: I33b62dfc12ff078fba9fb92b2f95a05f41acce3d

show more ...

bmcweb: Set Retry Policy Valid Response Codes

Allows individual retry policies to specify what HTTP response codes
are considered valid. Sets functions for the EventService and
Redfish Aggregation

bmcweb: Set Retry Policy Valid Response Codes

Allows individual retry policies to specify what HTTP response codes
are considered valid. Sets functions for the EventService and
Redfish Aggregation retry policies. Those functions expect a
response code and return an error code based on what the response
code is.

This change is needed because EventService only considers 2XX codes
to be valid. Any code outside of that range would trigger a retry
attempt. Redfish Aggregation by design will need to return
errors outside of that range such as 404. It should not retry to
send a message when it receives a 404 from a satellite BMC.

Right now 404 is the only error code that is handled differently
between the services. Going forward, Redfish Aggregation will
likely want to allow other error codes as its functionality is
expanded.

Tested:
Used Redfish-Event-Listener with ssh port forwarding to create 3
subscriptions. I then closed the ssh connection and sent a test
event. Bmcweb made 3 retry attempts for each subscription. At
that point the max retry amount (as defined by EventService) was
reached and bmcweb stop attempting to resend the messages.

There were no errors when the Redfish-Event-Listener was correctly
connected. Test events resulted in messages being sent for each
subscription.

Signed-off-by: Carson Labrado <clabrado@google.com>
Change-Id: Ifdfaf638d28982ed18998f3ca05280a288e0020a

show more ...

http/app: iwyu

While working on tests, I found that |app.hpp| is missing some boost
headers. I added them manually in this commit.

Tested: code compiles.

Signed-off-by: Nan Zhou <nanzhoumails@gmai

http/app: iwyu

While working on tests, I found that |app.hpp| is missing some boost
headers. I added them manually in this commit.

Tested: code compiles.

Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: I1d2fb0f312e1810d836c986e320263a9581f13f2

show more ...

http_server: remove unused variable

|useSsl| was not referenced anywhere else, so delete it.

Tested: code compiles.

Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: Ie6ba9c122ab0897254e

http_server: remove unused variable

|useSsl| was not referenced anywhere else, so delete it.

Tested: code compiles.

Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: Ie6ba9c122ab0897254ed3cb0e278d43e8b6a283c

show more ...

Fix www-authenticate behavior

bmcweb is in a weird position where, on the one hand, we would like to
support Redfish to the specification, while also supporting a secure
webui. For better or worse,

Fix www-authenticate behavior

bmcweb is in a weird position where, on the one hand, we would like to
support Redfish to the specification, while also supporting a secure
webui. For better or worse, the webui can't currently use non-cookie
auth because of the impacts to things outside of Redfish like
websockets.

This has lead to some odd code in bmcweb that tries to "detect" whether
the browser is present, so we don't accidentally pop up the basic auth
window if a user happens to get logged out on an xhr request. Basic
auth in a browser actually causes CSRF vulnerabilities, as the browser
caches the credentials, so we don't want to make that auth method
available at all.

Previously, this detection was based on the presence of the user-agent
header, but in the years since this code was originally written, a
majority of implementations have moved to sending a user-agent by
default, which makes this check pretty much useless for its purpose. To
work around that, this patchset relies on the X-Requested-With header,
to determine if a json payload request was done by xhr. In theory, all
browsers will set this header when doing xhr requests, so this should
provide a "more correct" solution to this issue.

Background:
https://en.wikipedia.org/wiki/List_of_HTTP_header_fields
"X-Requested-With Mainly used to identify Ajax requests (most JavaScript
frameworks send this field with value of XMLHttpRequest)"

Tested:
curl -vvvv --insecure https://192.168.7.2/redfish/v1/SessionService/Sessions
Now returns a WWW-Authenticate header

Redfish-protocol-validator now passes 7 more tests from the
RESP_HEADERS_WWW_AUTHENTICATE category.

Launched webui-vue and logged in. Responses in network tab appear to
work, and data populates the page as expected.
Used curl to delete redfish session from store with
DELETE /redfish/v1/SessionService/Sessions/<SessionId>
Then clicked an element on the webui, page forwarded to login page as
expected.

Opened https://localhost:8000/redfish/v1/CertificateService in a
browser, and observed that page forwarded to the login page as it
should.

Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I60345caa41e520c23fe57792bf2e8c16ef144a7a

show more ...

Make code compile on clang again

The usual updates to make code compile on clang again. Extra semicolons
that have snuck in, missing inline and static definitions.

Tested: Code compiles on clang.

Make code compile on clang again

The usual updates to make code compile on clang again. Extra semicolons
that have snuck in, missing inline and static definitions.

Tested: Code compiles on clang.

Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Id7f889de98cafaa89471d75ed3e3bb97ab3855cd

show more ...

Allow boost url and url_view to be added to json

The latest version of nlohmann seems to have support for adding any
arbitrary iterable object as an array in json. Unfortunately, because
boost::url

Allow boost url and url_view to be added to json

The latest version of nlohmann seems to have support for adding any
arbitrary iterable object as an array in json. Unfortunately, because
boost::urls::url produces at iterable of unsigned char, this means that
trying to encode urls leads to something like:

"@odata.id": [
47,
114,
101,
100,
102
]

Which is super unhelpful in that it does this implicitly. Given this
behavior, there are two options here, make it so that code doesn't
compile, or rely on the adl_serializer to just do the expected thing.

This patchset opts for the later, to simply to the reasonable behavior,
and call string() on the url before loading it into the json.

Tested: Unit tests passing. Fixes bug in subsequent patchset.

Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Id2f49bc8bd7153a0ad0c0fa8be2e13ce7c538e7f

show more ...

Try to fix the lambda formatting issue

clang-tidy has a setting, LambdaBodyIndentation, which it says:
"For callback-heavy code, it may improve readability to have the
signature indented two levels

Try to fix the lambda formatting issue

clang-tidy has a setting, LambdaBodyIndentation, which it says:
"For callback-heavy code, it may improve readability to have the
signature indented two levels and to use OuterScope."

bmcweb is very callback heavy code. Try to enable it and see if that
improves things. There are many cases where the length of a lambda call
will change, and reindent the entire lambda function. This is really
bad for code reviews, as it's difficult to see the lines changed. This
commit should resolve it. This does have the downside of reindenting a
lot of functions, which is unfortunate, but probably worth it in the
long run.

All changes except for the .clang-format file were made by the robot.

Tested: Code compiles, whitespace changes only.

Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ib4aa2f1391fada981febd25b67dcdb9143827f43

show more ...

meson option: make the insecure-disable-auth macro more accurate

The "auth" term is overloaded in meson option and macros. This commit
changes the macro from BMCWEB_INSECURE_DISABLE_AUTHENTICATION t

meson option: make the insecure-disable-auth macro more accurate

The "auth" term is overloaded in meson option and macros. This commit
changes the macro from BMCWEB_INSECURE_DISABLE_AUTHENTICATION to
BMCWEB_INSECURE_DISABLE_AUTHX, given that if "insecure-disable-auth"
is enabled, both authentication and authorization are disabled.

Tested:
1. set 'insecure-disable-auth=enabled', no authz nor authn is performed,
no crash on AccountService as well.

Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: Iddca1f866d16346bcc2017338fa6f077cb89cef9

show more ...

Include-what-you-use in http connection

Lots of #includes were missing in this file that we tangentially got
through boost/beast/websocket.hpp.

Tested: Code builds.

Signed-off-by: Ed Tanous <edtan

Include-what-you-use in http connection

Lots of #includes were missing in this file that we tangentially got
through boost/beast/websocket.hpp.

Tested: Code builds.

Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Iac5198f2f65eabaecf47d0fb6bb05bfa5a261f32

show more ...

auth: change authorization.hpp to authentication.hpp

The existing authorization header is actually doing "authentication"
work. The authorization is happening in routing.hpp where we fetch the
role

auth: change authorization.hpp to authentication.hpp

The existing authorization header is actually doing "authentication"
work. The authorization is happening in routing.hpp where we fetch the
role of the authenticated user and get their privilege set.

This commits changes the name of the file, as well as the namespace, to
be more precise on what the file actually does.

Tested:
1. Trivial change, it builds

Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: Ib91ed70507a7308522c7e5363ed2f4dc279a19d9

show more ...

bmcweb: Remove hardcoded HTTP verbs and headers

Modifies HttpClient so that the HTTP verb and headers can be set
for each individual message sent. Right now those fields are set
when a connection i

bmcweb: Remove hardcoded HTTP verbs and headers

Modifies HttpClient so that the HTTP verb and headers can be set
for each individual message sent. Right now those fields are set
when a connection is first created and then reused by each
message sent using that connection.

Tested:
Launched two Event Listener servers that created 6 and 2
subscriptions. Sending a test event resulted in the servers
receiving 6 requests and 2 requests, respectively.

Change-Id: I8d7e2d54385bc2c403498293820adb584bff8b57
Signed-off-by: Carson Labrado <clabrado@google.com>
Signed-off-by: Ed Tanous <edtanous@google.com>

show more ...

Add callback for response handling to HttpClient

Adds sendDataWithCallback() which allows the caller to include a
callback specifying how to handle the response to that
request. This will be utiliz

Add callback for response handling to HttpClient

Adds sendDataWithCallback() which allows the caller to include a
callback specifying how to handle the response to that
request. This will be utilized for Redfish Aggregation
including returning the responses received when forwarding
requests to satellite BMCs.

Change-Id: I93826c8b254a5f28a982295d4145453352a90fae
Signed-off-by: Carson Labrado <clabrado@google.com>

show more ...

Handle HEAD and Allow headers per the spec

The Redfish specification calls out that the Allow header should be
returned for all resources to give a client an indication of what
actions are allowed o

Handle HEAD and Allow headers per the spec

The Redfish specification calls out that the Allow header should be
returned for all resources to give a client an indication of what
actions are allowed on that resource. The router internally has all
this data, so this patchset allows the router to construct an allow
header value, as well as return early on a HEAD request.

This was reverted once here:
https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/53637

Due to a redfish validator failure. With the previous patches
workaround, this error has now been resolved.

Tested:
Called curl with various parameters and observed the Allow header
curl -vvvv --insecure -X <VERB> --user root:0penBmc https://<bmc>/url

HEAD /redfish/v1/SessionService/Sessions returned Allow: GET, POST
HEAD /redfish/v1 returned Allow: GET
HEAD /redfish/v1/SessionService returned Allow: GET, PATCH

POST /redfish/v1 returned Allow: GET (method not allowed)

GET /redfish/v1 returned Allow: GET
GET /redfish/v1/SessionService returned Allow: GET, PATCH

Redfish-Protocol-Validator now reports more tests passing.
Prior to this patch:
Pass: 255, Warning: 0, Fail: 27, Not tested: 45

After this patch:
Pass: 262, Warning: 0, Fail: 21, Not tested: 43

Diff: 7 more tests passing

All tests under RESP_HEADERS_ALLOW_METHOD_NOT_ALLOWED and
RESP_HEADERS_ALLOW_GET_OR_HEAD are now passing

Included unit tests passing.

Redfish service validator is now passing.

Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ibd52a7c2babe19020a0e27fa1ac79a9d33463f25

show more ...

Refactor HttpClient Class

Refactors HttpClient with the following changes:
- Convert class to singleton
- Replace circular buffers with devectors
- Sending queued requests and closing connections ha

Refactor HttpClient Class

Refactors HttpClient with the following changes:
- Convert class to singleton
- Replace circular buffers with devectors
- Sending queued requests and closing connections handled
within their own callback
- Add connection pooling (max size 4)
- HttpClient supports multiple connections to multiple clients
- Retry policies can be set for specific use cases

Also modifies its use in the Subscription class to be compatible
with the refactored code.

It is assumed that a BMC will be able to handle 4 parallel
connections and thus the max pool size is set as 4. The max
number of queued messages was left unchanged at 50. Eventually
we may want to allow tuning of these limits to boost performance.
That would come in a future patch.

Tested:
Launched two Event Listener servers that created 6 and 2
subscriptions. Sending a test event created a connection pool
for each server. 4 and 2 connections were added to each pool,
respectively and were used to send the test request. For the first
pool the 2 extra requests were placed into a queue until
connections became available. After a request completed, its
associated connection was used to send the next request in
the queue. Resending the test event caused those prior connections
to be reused instead of new connections being added to the pools.

Signed-off-by: Carson Labrado <clabrado@google.com>
Change-Id: Iba72b3e342cdc05d1fb972e2e9856763a0a1b3c5

show more ...

Revert "Handle HEAD and Allow headers per the spec"

This reverts commit 867b2056d44300db9769e0d0b8883435a179834c.

Apparently we have broken the Redfish spec in a way that adding this
feature now al

Revert "Handle HEAD and Allow headers per the spec"

This reverts commit 867b2056d44300db9769e0d0b8883435a179834c.

Apparently we have broken the Redfish spec in a way that adding this
feature now allows the service validator to find.

@odata.id /redfish/v1/UpdateService
ERROR - Allow header should NOT contain POST for UpdateService.v1_5_0.UpdateService

Need to figure out what to do, but for now, revert to get the build
passing again.

Change-Id: Ieef20573b9caa03aba6fd2bbc999e517e4b7de3d
Signed-off-by: Ed Tanous <edtanous@google.com>

show more ...

Handle HEAD and Allow headers per the spec

The Redfish specification calls out that the Allow header should be
returned for all resources to give a client an indication of what
actions are allowed o

Handle HEAD and Allow headers per the spec

The Redfish specification calls out that the Allow header should be
returned for all resources to give a client an indication of what
actions are allowed on that resource. The router internally has all
this data, so this patchset allows the router to construct an allow
header value, as well as return early on a HEAD request.

Tested:
Called curl with various parameters and observed the Allow header
curl -vvvv --insecure -X <VERB> --user root:0penBmc https://<bmc>/url

HEAD /redfish/v1/SessionService/Sessions returned Allow: GET, POST
HEAD /redfish/v1 returned Allow: GET
HEAD /redfish/v1/SessionService returned Allow: GET, PATCH

POST /redfish/v1 returned Allow: GET (method not allowed)

GET /redfish/v1 returned Allow: GET
GET /redfish/v1/SessionService returned Allow: GET, PATCH

Redfish-Protocol-Validator now reports more tests passing.
Prior to this patch:
Pass: 255, Warning: 0, Fail: 27, Not tested: 45

After this patch:
Pass: 262, Warning: 0, Fail: 21, Not tested: 43

Diff: 7 more tests passing

All tests under RESP_HEADERS_ALLOW_METHOD_NOT_ALLOWED and
RESP_HEADERS_ALLOW_GET_OR_HEAD are now passing

Included unit tests passing.

Change-Id: Ib99835050b15eb4f419bfd21375b26e4db74fa2c
Signed-off-by: Ed Tanous <edtanous@google.com>

show more ...

Implement etag headers

This commit implements a limited support for the production of etags on
json resources. It is intended to cause two things:

1. To get bmcweb to pass the PROTO_ETAG_ON_GET_AC

Implement etag headers

This commit implements a limited support for the production of etags on
json resources. It is intended to cause two things:

1. To get bmcweb to pass the PROTO_ETAG_ON_GET_ACCOUNT check, as well as
the redfish spec, which states: "Implementations shall support the
return of ETag headers for GET requests of ManagerAccount resources."

2. Begin discussions on what client-facing caching could look like in
the future, and to implement the fewest lines of code this author
could think of, with the hope of extending it later.

As written, it injects into the Response class a method that, for json
responses, uses std::hash<json> to generate an etag. This was chosen
under the assumption that it caused the least binary impact, and is
already a function provided by nlohmann, so required minimal
implementation effort to get something that functioned to the standard:
https://json.nlohmann.me/api/basic_json/std_hash/#version-history

I'm open to discussions if this should be improved in the future to
include more entropy, or to be a "weak" etag, but I think starting
with std::hash is a good first step.

This patchset intentionally does notimplement handling of the
If-None-Match, or If-Match headers that a caching client would likely
send that implements this. That is not explicitly required by the spec,
relatively complex, and probably has consequences that this author
doesn't want to write the test cases for (yet). This lack of support
makes this patchset largely only "useful" in passing the tests, and
implementing the spec to the letter, it does not generalize a caching
client feature that improves performance.

Tested:
curl -vvvv --insecure --user root:0penBmc https://192.168.7.2/redfish/v1

Contains in the response:
ETag: "765E4548"

The redfish protocol validator now passes the PROTO_ETAG_ON_GET_ACCOUNT
test, which increases our passing test count by 4 compared to previously.

Current counts are 352 passing, 30 failing, 36 not tested.

Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I3389b2ba98bf1276e1cb2d9c5954437b924f2d94

show more ...

Delete forked function_traits

The function_traits class was very clearly "borrowed" from
boost::function traits, then added to to support lambdas.
boost::function_traits has been superceeded by boos

Delete forked function_traits

The function_traits class was very clearly "borrowed" from
boost::function traits, then added to to support lambdas.
boost::function_traits has been superceeded by boost::callable_traits,
which fixes the same shortcomings that we have fixed here.

This commit replaces almost the entirety of the uses of function_traits
with callable traits, with one exception: arg<i>. In the callable
traits model, arg_t is a std::tuple, which, while better, doesn't unpack
easily into a variadic pack that our router code expects. Ideally, at
some point, we would rewrite the router core to not rely on
std::make_integer_sequence, but that's a much more invasive change.

Tested:
Called
curl --insecure --user root:0penBmc https://192.168.7.2/redfish/v1/Managers/bmc/LogServices/Journal/Entries/1646953359619803
and verified callback return the correct result (not 404). That API has
several flexible router parameters, which is the only thing this commit
could break.

Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Icf3299b2d5c1a5ff111f68858bb46139735aaabe

show more ...

Remove regex uses in event service and consolidate

As the patch at
https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/50994 can attest,
parsing urls with a regex is error prone. We should avoid

Remove regex uses in event service and consolidate

As the patch at
https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/50994 can attest,
parsing urls with a regex is error prone. We should avoid it where
possible, and we have boost::urls that implements a full, correct, and
unit tested parser.

Ideally, eventually this helper function would devolve into just the
parse_uri, and setting defaults portion, and we could rely on the
boost::urls::url class to pass into things like http_client.

As a side note, because boost url implements port as a proper type-safe
uint16, some interfaces that previously accepted port by std::string&
needed to be modified, and is included in this patch.

Also, once moved, the branch on the ifdef for HTTP push support was
failing a clang-tidy validation. This is a known limitation of using
ifdefs for our code, and something we've solved with the header file, so
move the http push enabler to the header file.

Also note that given this reorganization, two EXPECT statements are
added to the unit tests for user input behaviors that the old code
previously did not handle properly.

Tested: Unit tests passing

Ran Redfish-Event-Listener, saw subscription create properly:
Subcription is successful for https://192.168.7.2, /redfish/v1/EventService/Subscriptions/2197426973

Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ia4127c6cbcde6002fe8a50348792024d1d615e8f

show more ...