Age | Commit message (Collapse) | Author | Files | Lines |
|
Now that we only support string types in the router we no longer need to
build a "Tag" to be used for constructing argument types. Now, we can
just track the number of arguments, which simplifies the code
significantly, and removes the need to convert to and from the tag to
parameter counts.
This in turn deletes a lot of code in the router, removing the need for
tracking tag types.
Tested: Redfish service validator passes. Unit tests pass.
Change-Id: Ide1d665dc1984552681e8c05952b38073d5e32dd
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
This reverts commit cd40b060ee2df5469077a70d15590f86158f2c60.
Cookie based login is no longer functional with this patch. It looks
like we got a merge conflict that I resolved incorrectly.
Tested: Webui can now log in.
Change-Id: I60b8aeae173b1838d8745a2c499fbcb410813ef3
|
|
Break out this method into a smaller section.
Tested: Redfish service validator passes
Change-Id: I0ca4e9ea14c505a1ed00dae4cba1285e4ac1f36d
Signed-off-by: Ed Tanous <edtanous@google.com>
|
|
Phosphor-rest is no longer supported by the project, and phosphor-webui,
which required some of these workarounds has been archived a year ago.
There's no reason to keep this login type, given that it was
undocumented.
NOTE: Upon inspection, it looks like webui-vue used the same hack.
[1] https://github.com/openbmc/webui-vue/blob/43e3bd26133b06ed117a3a3f10b2bc09e2c2aafc/src/store/modules/Authentication/AuthenticanStore.js#L41
Tested:
Combined with https://gerrit.openbmc.org/c/openbmc/webui-vue/+/65811
Webui Login succceeds.
Change-Id: Ie42380029e799e44b3a7404d4ec6d285b371402b
Signed-off-by: Ed Tanous <edtanous@google.com>
|
|
Similar to what we've done elsewhere, move login and logout into their
own methods. This reduces the amount of scopes that need to be read at
any given time.
Tested: At last commit in series.
Change-Id: Ia2aa8b3fcbed18d7a481876fe4ffd55f31120064
Signed-off-by: Ed Tanous <edtanous@google.com>
|
|
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
|
|
Change the Cache-Control header to what owasp recommends.
Remove the X-XSS-Protection. This has been removed from Chrome, and is
unimplemented in other browsers[1].
Add:
X-Permitted-Cross-Domain-Policies
Clear-Site-Data
Cross-Origin-Embedder-Policy
Cross-Origin-Opener-Policy
Cross-Origin-Resource-Policy
And set them to the OWASP recommended values.
Tested: The OWASP Venom test suite now passes more tests.
[1] https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-XSS-Protection
Change-Id: I2860041c1037f47bb85a6444cec66960d0aa55f9
Signed-off-by: Ed Tanous <edtanous@google.com>
|
|
This is one that I couldn't figure out for a while. Turns out that
fields has both a set() and an insert() method. Whereas set() replaces,
insert() appends, which is what we want in this case.
This allows us to call the actual methods several times, instead of
essentially string injecting our own code, which should make it clearer.
At the same time, there was one unit test that was structured such that
it was using addHeader to clear a header, so this commit adds an
explicit "clearHeader()" method, so we can be explicit.
Tested:
Logging into the webui in chrome (which uses POST /login) shows:
401 with no cookie header if the incorrect password is used
200 with 2 Set-Cookie headers set:
Set-Cookie:
SESSION=<session tag>; SameSite=Strict; Secure; HttpOnly
Set-Cookie:
XSRF-TOKEN=<token tag>; SameSite=Strict; Secure
Change-Id: I9b87a48ea6ba892fc08e66940563dea86edb9a65
Signed-off-by: Ed Tanous <edtanous@google.com>
|
|
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>
|
|
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
|
|
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
|
|
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
|
|
std::string_view causes invalid memory access in multipart branch when
assigned local variable goes out of scope and string_view is passed to
ramAuthenticateUser. Moved MultipartParser to higher scope, to ensure
it is not deleted before std::string_view.
Tested:
- Executed post on /login, got response:
{
"data": "User 'root' logged in",
"message": "200 OK",
"status": "ok"
}
Change-Id: I0b02dddcb1a887d442525ffedb7a08a00087f2f2
Signed-off-by: Krzysztof Grobelny <krzysztof.grobelny@intel.com>
|
|
Per the coding standard, now that C++ supports std::string::starts_with
and std::string::ends_with, we should be using them over the boost
alternatives. This commit goes through and updates all usages.
Arguably some of these are incorrect, and instances of common error 13,
but because this is mostly a mechanical it intentionally doesn't try to
handle it.
Tested: Unit tests pass.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ic4c6e5d0da90f7442693199dc691a47d2240fa4f
|
|
The Session cookie is an HttpOnly cookie.
HttpOnly means the cookie cannot be accessed through client side script
because of this the GUI can not delete this cookie on log out.
Recommendation online was setting this cookie to an expired date.
From https://tools.ietf.org/search/rfc6265
"Finally, to remove a cookie, the server returns a Set-Cookie header
with an expiration date in the past. The server will be successful in
removing the cookie only if the Path and the Domain attribute in the
Set-Cookie header match the values used when the cookie was created."
For more information see
https://stackoverflow.com/questions/5285940/correct-way-to-delete-cookies-server-side
Modern browsers delete expired cookies although based on reading it
might not be right away but on the next request from that domain or
when the browser is cleaning up cookies.
When I tested the cookie is deleted right away.
Also set the SESSION to an empty string.
Discussed in discord here:
https://discord.com/channels/775381525260664832/855566794994221117/982351098998321163
Webui-vue and phosphor-webui both use this /logout route:
https://github.com/openbmc/webui-vue/blob/a5fefd0ad25753e5f7da03d77dfe7fe10255ebb6/src/store/modules/Authentication/AuthenticanStore.js#L50
https://github.com/openbmc/phosphor-webui/blob/339db9a4c8610c5ecb92993c0bbc2219933bc858/app/common/services/userModel.js#L46
It seemed unnecessary to add it to the SessionCollection Post.
Tested: No longer have the cookie after log out on webui-vue.
Tested on Firefox and Chrome.
Signed-off-by: Gunnar Mills <gmills@us.ibm.com>
Change-Id: Ic12b6f628293a80c93ffbbe1bf06c9b2d6a53af7
|
|
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
|
|
Brace initialization of json objects, while quite interesting from an
academic sense, are very difficult for people to grok, and lead to
inconsistencies. This patchset aims to remove a majority of them in
lieu of operator[]. Interestingly, this saves about 1% of the binary
size of bmcweb.
This also has an added benefit that as a design pattern, we're never
constructing a new object, then moving it into place, we're always
adding to the existing object, which in the future _could_ make things
like OEM schemas or properties easier, as there's no case where we're
completely replacing the response object.
Tested:
Ran redfish service validator. No new failures.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Iae409b0a40ddd3ae6112cb2d52c6f6ab388595fe
|
|
clang-tidy readability checks are overall a good thing, and help us to
write consistent and readable code, even if it doesn't change the
result.
All changes done by the robot.
Tested: Code compiles, inspection only (changes made by robot)
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Iee4a0c74a11eef9f158f0044eae675ebc518b549
|
|
There's a number of redundancies in our code that clang can sanitize
out. Fix the existing problems, and enable the checks.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ie63d7b7f0777b702fbf1b23a24e1bed7b4f5183b
|
|
These checks are a nice addition to our static analysis, as they
simplify code quite a bit, as can be seen by this diff being negative
lines.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I60ede4ad23d7e5337e811d70ddcab24bf8986891
|
|
This commit adds two core features to bmcweb:
1. A multipart mime parser that can read multipart form requests into
bmcweb. This is implemented as a generic parser that identifies the
content-type strings and parses them into structures.
2. A /login route that can be logged into with a multipart form. This
is to allow changing the login screen to a purely forms based
implementation, thus removing the very large whitelist we currently have
to maintain, and removing javascript from our threat envelope.
More testing is still needed, as this is a parser that exists outside of
the secured areas, but in this simple example, it seems to work well.
Tested: curl -vvvvv --insecure -X POST -F 'username=root' -F
'password=0penBmc' https://<bmc ip address>:18080/login
Returned; { "data": "User 'root' logged in", "message": "200 OK",
"status": "ok" }
Change-Id: Icc3f4c082d584170b65b9e82f7876926cd38035d
Signed-off-by: Ed Tanous<ed@tanous.net>
Signed-off-by: George Liu <liuxiwei@inspur.com>
|
|
Current HTTP server creates an IPv6 acceptor to accept both IPv4 and
IPv6 connections. In this way, IPv4 address will be presented as IPv6
address in IPv4-mapped format. This patch converts it back to IPv4.
Tested:
Verified the ClientOriginIP in Session is shown in native IPv4 format
instead of IPv4-mapped IPv6 format.
Change-Id: Icd51260b2d4572d52f5c670128b7f07f6b5e6912
Signed-off-by: Jiaqing Zhao <jiaqing.zhao@intel.com>
|
|
Get the core using AsyncResp everywhere, and not have each individual handler
creating its own object.We can call app.handle() without fear of the response
getting ended after the first tree is done populating.
Don't use res.end() anymore.
Tested:
1. Validator passed.
Signed-off-by: zhanghaicheng <zhanghch05@inspur.com>
Change-Id: I867367ce4a0caf8c4b3f4e07e06c11feed0782e8
|
|
Set SameSite to Strict since OpenBMC does not have functionality
that requires Lax or None.
SameSite Strict provides a little protection against CSRF attacks
by ensuring the cookie is only sent to requests originating from
the same site that set the cookie.
This came from some discussion on discord.
From https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie
SameSite=<samesite-value> Optional
Controls whether a cookie is sent with cross-origin requests,
providing some protection against cross-site request forgery attacks
Inline options are:
Strict: The browser sends the cookie only for same-site requests
(that is, requests originating from the same site that set the
cookie). If the request originated from a different URL than the
current one, no cookies with the SameSite=Strict attribute are sent.
Lax: The cookie is not sent on cross-site requests, such as calls
to load images or frames, but is sent when a user is navigating to
the origin site from an external site (e.g. if following a link).
This is the default behavior if the SameSite attribute is not
specified.
None: The browser sends the cookie with both cross-site and
same-site requests. The Secure attribute must also be set when
SameSite=None!
Note: On Firefox 85, FireFox still doesn't have the Default set
to SameSite=Lax. This can be changed via "about:config" and
"network.cookie.sameSite.laxByDefault".
Tested: Webui-vue works. Redfish GUI browser works.
Websockets work on the GUI.
Tested GUI functions that call POST and PATCH.
Can see the XSRF-TOKEN and SESSION cookies are SameSite
Strict with this build. Before were SameSite None.
Browser DevTools -> Storage on Firefox to view cookies.
Signed-off-by: Gunnar Mills <gmills@us.ibm.com>
Change-Id: I4402f2930847c1d47b22696631be26d33c78b6f9
|
|
When the session is created using /login, the ClientOriginIPAddress
is mapped to the clientId parameter which displayed the clientIP
instead of the of clientId.
The similar problem is observed with auth methods other than sessions
created using the SessionService resource
This commit swaps the clientId and clientIp parameters passed to
generateUserSession API, so that the optional clientId is
passed as the last parameter
Tested by :
1. Create session using Redfish command
POST https://${bmc}/login -d '{"username": <>,"password": <>}'
POST https://${bmc}/redfish/v1/SessionService/Sessions
-d '{"username": <>,"password": <>}'
2. Open the GUI session to check the clientId is not displaying the
ClientOriginIPAddress
Signed-off-by: Sunitha Harish <sunithaharish04@gmail.com>
Change-Id: I6cee3de963c489e690d2ad0bb09ba78dca39e4f9
|
|
This commit implements the ClientOriginIPAddress property on
the session resource. The IP address is persisted across the reboot
Tested by:
1. Create session
POST https://${bmc}/redfish/v1/SessionService/Sessions -d '{"UserName":<>, "Password":<>}'
2. Check the session gets updated with the ClientOriginIPAddress
GET https://${bmc}/redfish/v1/SessionService/Sessions/<id>
3. Redfish validator passed
4. Create session and reboot the BMC to ensure the IP address is persisted
5. Tested the basic auth populates the clientIp at req
Signed-off-by: Sunitha Harish <sunharis@in.ibm.com>
Change-Id: Iaa60d0657c991bde4bcf6c86819055c71c92e421
|
|
cppcheck isn't smart enough to recognize these are c++ headers, not c
headers. Considering we're already inconsistent about our naming, it's
easier to just be consistent, and move the last few files to use .hpp
instead of .h.
Tested:
Code builds, no changes.
Signed-off-by: Ed Tanous <ed@tanous.net>
Change-Id: Ic348d695f8527fa4a0ded53f433e1558c319db40
|
|
This commit enables clang warnings, and fixes all warnings that were
found. Most of these fall into a couple categories:
Variable shadow issues were fixed by renaming variables
unused parameter warnings were resolved by either checking error codes
that had been ignored, or removing the name of the variable from the
scope.
Other various warnings were fixed in the best way I was able to come up
with.
Note, the redfish Node class is especially insidious, as it causes all
imlementers to have variables for parameters, regardless of whether or
not they are used. Deprecating the Node class is on my list of things
to do, as it adds extra overhead, and in general isn't a useful
abstraction. For now, I have simply fixed all the handlers.
Tested:
Added the current meta-clang meta layer into bblayers.conf, and added
TOOLCHAIN_pn-bmcweb = "clang" to my local.conf
Signed-off-by: Ed Tanous <ed@tanous.net>
Change-Id: Ia75b94010359170159c703e535d1c1af182fe700
|
|
Middlewares, while kinda cool from an academic standpoint, make our
build times even worse than they already are. Given that we only really
use 1 real middleware today (token auth) and it needs to move into the
parser mode anyway (for security limiting buffer sizes), we might as well
use this as an opportunity to delete some code.
Some other things that happen:
1. Persistent data now moves out of the crow namespace
2. App is no longer a template
3. All request_routes implementations no longer become templates. This
should be a decent (unmeasured) win on compile times.
This commit was part of a commit previously called "various cleanups".
This separates ONLY the middleware deletion part of that.
Note, this also deletes about 400 lines of hard to understand code.
Change-Id: I4c19e25491a153a2aa2e4ef46fc797bcb5b3581a
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
While a cool example of how to do string matching in constexpr space,
the set of verbs available to HTTP has been fixed for a very long time.
This was ported over to beast a while back, but we kept the API for....
mediocre reasons of backward compatibility. Remove that, and delete the
now unused code.
Tested: Built and loaded on a Witherspoon. Validator passes.
Signed-off-by: Ed Tanous <ed.tanous@intel.com>
Change-Id: Iaf048e196f9b6e71983189877203bf80390df286
Signed-off-by: James Feist <james.feist@linux.intel.com>
Signed-off-by: Gunnar Mills <gmills@us.ibm.com>
|
|
Currently we parse the whole message before authenticating,
allowing an attacker the ability to upload a large image,
or keep a connection open for the max amount of time easier
than it should be. This moves the authentication to the
earliest point possible, and restricts unauthenticated users
timeouts and max upload sizes. It also makes it so that
unauthenticated users cannot keep the connection alive
forever by refusing to close the connection.
Tested:
- login/logout
- firmware update
- large POST when unauthenticated
- timeouts when unauthenticated
- slowhttptest
Change-Id: Ifa02d8db04eac1821e8950eb85e71634a9e6d265
Signed-off-by: James Feist <james.feist@linux.intel.com>
|