Age | Commit message (Collapse) | Author | Files | Lines |
|
A clang analyzer was able to find this issue.
dbus::utility::checkDbusPathExists(
dbusObjectPath,
[dbusObjectPath(std::move(dbusObjectPath)), username,
password(std::move(password)), roleId(std::move(roleId)), enabled,
locked, asyncResp{std::move(asyncResp)}](int rc) {
"Object 'dbusObjectPath' of type 'std::basic_string' is left in a valid
but unspecified state after move"
This is obviously a mistake, but apparently the undefined behavior works
for us. This commit makes it so we no longer rely on UB.
Tested: Trivial change, code compiles.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I1b82778cb74f4e5abc53edec35a6d276161de1af
|
|
A merge conflict appears to have dropped the patch handling for the
/redfish/v1/AccountService uri. This commit takes the code that was
previously there, and re-adds it back, with the needed adjustments to
privileges that have landed in the meantime.
Tested:
redfishtool -S Always -u root -p 0penBmc -vvvvvvvvv -r 192.168.7.2
AccountService patch "{\"AccountLockoutThreshold\": 21}"
Returns 200, and subsequent reads show AccountLockoutThreshold set to
21.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I552ac95808e0abdc713817479575eee7806688c8
|
|
Clang-13 rightfully warns that the hasWebuiRoute variable isn't declared
as static. This commit resolves that, and adds the static keyword so it
can be used in multiple compile units. It also adds the static keyword
to the privilege registry, and the inline keyword to many methods that
now need it.
clang-format is also updated to version 12 in parse_registies.py, as
that's what CI uses, and what most people have installed.
Tested:
Followed clang-tidy instructions in README.md
"bitbake bmcweb" step now succeeds.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Id43b13606754cb37a404799fce155599ac3a3240
|
|
This commit attempts to automate the creation of our privileges
structures from the redfish privilege registry. It accomplishes this by
updating parse_registries.py to also pull down the privilege registry
from DMTF.
The script then generates privilege_registry.hpp, which include const
defines for all the privilege registry entries in the same format that
the Privileges struct accepts. This allows new clients to simply
reference the variable to these privilege structures, instead of having
to manually (ie error pronely) put the privileges in themselves.
This commit updates all the routes.
For the moment, override and OEM schemas are not considered. Today we
don't have any OEM-specific Redfish routes, so the existing ones inherit
their parents schema. Overrides have other issues, and are already
incorrect as Redfish defines them.
Binary size remains unchanged after this patchset.
Tested:
Ran redfish service validator
Ran test case from f9a6708c4c6490257e2eb6a8c04458f500902476 to ensure
that the new privileges constructor didn't cause us to regress the brace
construction initializer.
Checked binary size with:
gzip -c
$BBPATH/tmp/work/s7106-openbmc-linux-gnueabi/obmc-phosphor-image/1.0-r0/rootfs/usr/bin/bmcweb
| wc -c
1244048
(tested on previous patchset)
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ideede3d5b39d50bffe7fe78a0848bdbc22ac387f
|
|
DMTF published new entity privileges for certificate service classes
which modify entity privilege Certificate, CertificateCollection,
CertificateLocations, and CertificateService on bmcweb. Modification
restricts a user without "ConfigureManager" from accessing the
CertificateCollection and Certificate scehamas
Redfish is a hypermedia API where the parent URI describes sub-URI.
Thus, restricting sub-URI in a parent-URI data helps to forbidden user
access, stricken the rule. So sub-URI only gets display if a user has
access to that URI.
Restricting the link allows the Redfish Validator to pass.
These impact roles without ConfigureManager, which include operator
and read-only. No access is not impacted since it already did not
have access.
The following are bmcweb user consequences:
1. ReadOnly and Operator role users are no longer able to view
certificates or the certificate collection (LDAP, HTTPS, TrustStore)
2. Operator role users are no longer able to replace the certificates
(LDAP, HTTPS, TrustStore), Install certificates (LDAP, HTTPS,
TrustStore) or delete the Truststore Certificate. HTTPS and LDAP
certificates do not have delete methods.
Resolves openbmc/bmcweb#61
Tested: manually tested on Witherspoon system and run Redfish-Service-
Validator with all roles root, operator, read-only, and No access. Test
pass for root, operator, and read-only roles, And new errors get
introduced for no access role.
Signed-off-by: Abhishek Patel <Abhishek.Patel@ibm.com>
Change-Id: Ibc5eed7db7e224e46f8572df8bcfba2a1ff47644
|
|
Fixes #181
Lots of specific details around why the node class have been removed are
in the previous patchsets. This commit actually does the deed and makes
it go away entirely.
Now that this is finally done, we can compare binary size. Surprisingly
enough, this series saves a full 72KB of compressed binary size, which
amounts to about 6.4% of the total code size.
Before: 1197632 bytes
After: 1124688 bytes
This IMO makes it worth it, considering we've significantly reduced the
amount of code at the same time.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I3c8688715f933b381cad0be75a079ccfd72c3130
|
|
As the comment (being deleted in this patchset) says this is an odd
privilege level to have, and doesn't actually match the Redfish
Privilege registry. Now that we're no longer tied to the router to make
privilege decisions, this hack can be removed. This should have no
functional impact, as all users have Login Privilege, and we can now
properly filter users that don't have ConfigureSelf, without having to
rely on a single privilege set.
Tested:
Ran redfish service validator on last patchset in this series; No new
failures (UUID has failures on my system, should be unrelated)
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I0a04ff9001f9045d66e3778f04f3eec4b4ff2ec6
|
|
This is a progression of 7e860f1550c8686eec42f7a75bc5f2ef51e756ad, which
correctly noted that AccountService has a number of class specific
variables. This commit removes the Node class from those in line with
the aformentioned patchset, and at the same time removes the need for
the isAllowedWithoutConfigureSelf method, which was relying on state
captured to do some complex rule checking. Fortunately, it is
relatively easy to check current permissions at runtime using the
Privileges::isSupersetOf check against the current users role. This
significantly reduces the complexity of the code, while still giving the
same result (users with only ConfigureSelf cannot see or modify other
users). Ideally these two things, isAllowedWithoutConfigureSelf, and
the Node moving would've been done in separate commits, but given that
the former would've required moving a number of features out of the node
derived class anyway, separating them would lead to essentially the same
diff twice, hence why they are combined for easier review.
Tested:
Ran Redfish service validator. No new errors. (UUID error present that
appears to be unrelated)
Change-Id: Iad919dbc7ab7e8d47cc1160999ed9f43f685fa56
Signed-off-by: Ed Tanous <edtanous@google.com>
|
|
Reduces the total number of lines and will allow for easier testing of
the redfish responses.
A main purpose of the node class was to set app.routeDynamic(). However
now app.routeDynamic can handle the complexity that was once in critical
to node. The macro app.routeDynamic() provides a shorter cleaner
interface to the unerlying app.routeDyanic call. The old pattern set
permissions for 6 interfaces (get, head, patch, put, delete_, and post)
even if only one interface is created. That pattern creates unneeded
code that can be safely removed with no effect.
Unit test for the responses would have to mock the node the class in
order to fully test responses.
see https://github.com/openbmc/bmcweb/issues/181
The following files still need node to be extracted.
virtual_media.hpp
account_service.hpp
redfish_sessions.hpp
ethernet.hpp
The files above use a pattern that is not trivial to address. Often their
responses call an async lambda capturing the inherited class. ie
(https://github.com/openbmc/bmcweb/blob/ffed87b5ad1797ca966d030e7f979770
28d258fa/redfish-core/lib/account_service.hpp#L1393)
At a later point I plan to remove node from the files above.
Tested:
I ran the docker unit test with the following command.
WORKSPACE=$(pwd) UNIT_TEST_PKG=bmcweb
./openbmc-build-scripts/run-unit-test-docker.sh
I ran the validator and this change did not create any issues.
python3 RedfishServiceValidator.py -c config.ini
Signed-off-by: John Edward Broadbent <jebr@google.com>
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I147a0289c52cb4198345b1ad9bfe6fdddf57f3df
|
|
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
|
|
The nlohmann::json::dump call needs to be called with specific arguments
to avoid throwing in failure cases. http connection already does this
properly, but a bunch of code has snuck in (mostly in redfish) that
ignores this, and calls it incorrectly. This can potentially lead to a
crash if the wrong thing throws on invalid UTF8 characters.
This audits the whole codebase, and replaces every dump() call with the
correct dump(2, ' ', true, nlohmann::json::error_handler_t::replace)
call. For correct output, the callers should expect no change, and in
practice, this would require injecting non-utf8 characters into the
BMC.
Tested:
Ran several of the endpoints/error conditions in question, including
some of the error cases. Observed correct responses. I don't know of a
security issue that would allow injecting invalid utf8 into the BMC, but
in theory if it were possible, this would prevent a crash.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I4a15b8e260e3db129bc20484ade4ed5449f75ad0
|
|
Lots of code gets checked in that does this path checking incorrectly.
So much so, that we have it documented in COMMON_ERRORS.md, yet, we
persist. This patchset starts using the new object_path::filename()
method that was added recently to sdbusplus. Overall, it deletes code,
and makes for a much better developer experience.
Tested:
Pulled down several endpoints and verified that filename() method works
properly, and the collections are returned as expected.
curl -vvvv --insecure --user root:0penBmc https://192.168.7.2/redfish/v1/AccountService/Accounts
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ief1e0584394fb139678d3453265f7011bc931f3c
|
|
camelLower is not a type, camelBack is.
Changes were made automatically with clang-tidy --fix-errors
To be able to apply changes automatically, the only way I've found that
works was to build the version of clang/clang-tidy that yocto has, and
run the fix script within bitbake -c devshell bmcweb. Unfortunately,
yocto has clang-tidy 11, which can apparently find a couple extra errors
in tests we already had enabled. As such, a couple of those are also
included.
Tested:
Ran clang-tidy-11 and got a clean result.
Signed-off-by: Ed Tanous <ed@tanous.net>
Change-Id: I9d1080b67f0342229c2f267160849445c065ca51
|
|
Add meson options to enabled/disabled authentication methods:
- basic-auth : For enable basic authentication, default is enabled
- session-auth : For enable session token authentication, default is
enabled
- xtoken-auth : For enable x-token authentication, default is enabled
- cookie-auth : For enabled cookie authentication, default is enabled
Signed-off-by: Alan Kuo <Alan_Kuo@quantatw.com>
Change-Id: I52e636f2534a14897cb57d35e563ea8841cc68b9
|
|
- This commit would fix a possible null pointer
dereference.
Tested By:
- Code compiles & no functional changes
Signed-off-by: Manojkiran Eda <manojkiran.eda@gmail.com>
Change-Id: Idfb52607869ecc42756cffa68617caecfdef9cd3
|
|
1st, alphabetize the tidy-list for good housekeeping.
Next, enable all the clang-tidy performance checks, and resolve all the
issues. most of the issues boil down to:
1. Using std::move on const variables. This does nothing.
2. Passing big variables (like std::string) by value.
3. Using double quotes on a find call, which constructs an intermediate
string, rather than using the character overload.
Tested
Loaded on system, logged in successfully and pulled down webui-vue. No
new errors.
Walked the Redfish tree a bit, and observed no new problems.
Ran redfish service validator. Got no new failures (although there are
a lot of log service deprecation warnings that we should look at).
Signed-off-by: Ed Tanous <ed@tanous.net>
Change-Id: I2238958c4b22c1e554e09a0a1787c744bdbca43e
|
|
(In the voice of the kid from sixth sense) I see string copies...
Apparently there are a lot of places we make unnecessary copies. This
fixes all of them.
Not sure how to split this up into smaller patches, or if it even needs
split up. It seems pretty easy to review to me, because basically every
diff is identical.
Change-Id: I22b4ae4f96f7e4082d2bc701098a04f7bed95369
Signed-off-by: Ed Tanous <ed@tanous.net>
Signed-off-by: Wludzik, Jozef <jozef.wludzik@intel.com>
|
|
Now that CI can handle clang-tidy, and a lot of the individual fixes
have landed for the various static analysis checks, lets see how close
we are.
This includes bringing a bunch of the code up to par with the checks
that require. Most of them fall into the category of extraneous else
statements, const correctness problems, or extra copies.
Tested:
CI only. Unit tests pass.
Signed-off-by: Ed Tanous <ed@tanous.net>
Change-Id: I9fbd346560a75fdd3901fa40c57932486275e912
|
|
As part of getting the clang-tidy bugprone tests running, these
conditionals were found to have duplicate entries, or be somewhat
nonsensical.
Tested: clang-tidy now passes with the branch-compliance check set.
Change-Id: Ibec106f3bbd866fc471a1fc56bd8cdd5d44024e0
|
|
Lots of code has been checked in that doesn't match the naming
conventions. Lets fix that.
Tested:
Code compiles. Variable/function renames only.
Signed-off-by: Ed Tanous <ed@tanous.net>
Change-Id: I6bd107811d0b724f1fad990016113cdf035b604b
|
|
This commit enables the "unused variables" warning in clang. Throughout
this, it did point out several issues that would've been functional
bugs, so I think it was worthwhile. It also cleaned up several unused
variable from old constructs that no longer exist.
Tested:
Built with clang. Code no longer emits warnings.
Downloaded bmcweb to system and pulled up the webui, observed webui
loads and logs in properly.
Change-Id: I51505f4222cc147d6f2b87b14d7e2ac4a74cafa8
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
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>
|
|
Change-Id: Iafbd209fe2cb4503df995536587d8a80bd887a74
Signed-off-by: Gunnar Mills <gmills@us.ibm.com>
|
|
Seeing "ERROR - AccountTypes: Mandatory prop does not exist" validator
fails. This should not be an error, because bmcweb is using
ManagerAccount.v1_3_0 which doesn't have this property. The workaround
(implement AccountTypes) improves the code, more information and easier
later when moving to a later schema, so bump the ManagerAccount schema
and Implement AccountTypes.
Taking this issue forward with Redfish.
Believe this was introduced in the following Validator commit:
https://github.com/DMTF/Redfish-Service-Validator/commit/de71f0388bf85c920ae48deb6b16aed124f4f23b
From https://redfish.dmtf.org/schemas/ManagerAccount.v1_6_0.json:
"AccountTypes": {
"description": "The account types.",
"items": {
"anyOf": [
{
"$ref": "#/definitions/AccountTypes"
},
...
"versionAdded": "v1_4_0"
},
...
"required": [
"@odata.id",
"@odata.type",
"Id",
"Name",
"AccountTypes"
],
...
"AccountTypes": {
"enum": [
"Redfish",
"SNMP",
"OEM"
],
"enumDescriptions": {
"OEM": "OEM account type.",
"Redfish": "Allow access to the Redfish Service.",
"SNMP": "Allow access to SNMP services."
},
"type": "string"
Tested: Built and latest validator passes on a Witherspoon.
curl -k https://${bmc}/redfish/v1/AccountService/Accounts/root
{
"@odata.id": "/redfish/v1/AccountService/Accounts/root",
"@odata.type": "#ManagerAccount.v1_4_0.ManagerAccount",
"AccountTypes": [
"Redfish"
],
Change-Id: If48c4b8deb5f199f459858bb2c7469f0ebd44781
Signed-off-by: Gunnar Mills <gmills@us.ibm.com>
|
|
This is from openbmc/docs/style/cpp/.clang-format
Other OpenBMC repos are doing the same.
Tested: Built and validator passed.
Change-Id: Ief26c755c9ce012823e16a506342b0547a53517a
Signed-off-by: Gunnar Mills <gmills@us.ibm.com>
|
|
The PasswordChangeRequired commit did not pass the Redfish validator.
It uses the ManagerAccount.PasswordChangeRequired property which was added
in 1_3_0.
Tested: Passed the Redfish Service Validator
Signed-off-by: Joseph Reynolds <joseph-reynolds@charter.net>
Change-Id: Icb3bacd887c018baad107c22ddd7623243232339
|
|
This implements the Redfish PasswordChangeRequired handling. See
section 13.3.7.1 "Password change required handling" in the 1.9.1 spec:
https://www.dmtf.org/sites/default/files/standards/documents/DSP0266_1.9.1.pdf
These portions of the spec are implemented:
- Authenticatation with a correct but expired password creates a
session:
- The session is restricted to the ConfigureSelf privilege which
allows a user to change their own password (via GET and PATCH
Password for their own account). Support for the ConfigureSelf
privilege is already in BMCWeb.
- The session object has the PasswordChangeRequired message.
- All other operations respond with http status code 403 Forbidden
and include the PasswordChangeRequired message.
- The ManagerAccount (URI /redfish/v1/AccountService/Accounts/USER)
PasswordChangeRequired property is implemented for local accounts
but not present for remote accounts.
This has the following additional behavior:
The PasswordChangeRequired property is updated at the start of each new
REST operation, even within an existing session. This behavior
implements a "dynamic" PasswordChangeRequired handling that responds to
changes to the underlying "password expired" status. Specifically:
- Sessions restricted by the PasswordChangeRequired handling lose that
restriction when the underlying account password is changed.
- Sessions become subject to the PasswordChangeRequired handling
restrictions whenever the underlying account password expires.
- The mechanism is to check if the password is expired at the start of
every new REST API operation, effectively updating the ManagerAccount
PasswordChangeRequired property each time. This makes BMCWeb
responsive to changes in the underlying account due to other activity
on the BMC.
Notes:
1. Note that when an account password status is changed (for example,
the password becomes expired or is changed) and that account has
active sessions, those sessions remain. They are not deleted. Any
current operations are allowed to complete. Subsequent operations
with that session pick up the new password status.
2. This does not implement OWASP recommendations which call for sessions
to be dropped when there is a significant change to the underlying
account. For example, when the password is changed, the password
becomes expired, or when the account's Role changes. OWASP's
recommendation is due to the session fixation vulnerability. See the
OWASP Session Management Cheat Sheet section "Renew the Session ID
After Any Privilege Level Change":
https://cheatsheetseries.owasp.org/cheatsheets/Session_Management_Cheat_Sheet.html#renew-the-session-id-after-any-privilege-level-change
BMCWeb protects against session fixation vulnerabilities because it
always regenerates new session IDs when successful authentication
creates a new session.
3. Users authenticating via mTLS are not subject to the
PasswordChangeRequired behavior because mTLS takes precedence over
password-based authentication.
Tested:
0. Setup:
- The `passwd --expire USERNAME` command was used to expire
passwords. The `chage USER` command was also used.
- The following were used to change the password: Redfish API,
passwd command, and the SSH password change dialog.
- Tested the following via Basic Auth, /login, and Redfish login
(except where Basic Auth does not create a persistent session).
- Only local user account were tested.
- Did not test authentication via mTLS or with LDAP users.
1. When the password is not expired, authentication behaves as usual
for both correct and incorrect passwords.
2. When the password is incorrect and expired, authentication fails as
usual.
3. When the password is correct but expired:
A. A session is created and has the PasswordChangeRequired message.
B. That session cannot access resources that require Login privilege
and the 403 message contains the PasswordChangeRequired message.
C. That session can be used to GET the user's account, PATCH the
Password, and DELETE the session object.
D. The account PasswordChangeRequired reports true.
4. While a session is established, try expiring and changing
(unexpiring) the password using various mechanisms. Ensure both the
session object and the ManagerAccount PasswordChangeRequired property
report the correct condition, and ensure PasswordChangeRequired
handling (restricting operations to ConfigureSelf when
PasswordChangeRequired is true) is applied correctly.
Signed-off-by: Joseph Reynolds <joseph-reynolds@charter.net>
Change-Id: Iedc61dea8f949e4b182e14dc189de02d1f74d3e8
|
|
Signed-off-by: Patrick Williams <patrick@stwcx.xyz>
Change-Id: Ia5d0845741f1d8d4bc6fd227c6d2e6f3a8d42b2e
|
|
Support NoAccess privilege user creation from Redfish
Tested:
1. Verified redfish validator passed
2. Create NoAccess user from Redfish
POST: https://<BMC-IP>/redfish/v1/AccountService/Accounts/
Body:
{
"UserName": "user2",
"RoleId": "NoAccess",
"Password": "asdf1234"
}
Response:
{
"@Message.ExtendedInfo": [
{
"@odata.type": "#Message.v1_0_0.Message",
"Message": "The resource has been created successfully",
"MessageArgs": [],
"MessageId": "Base.1.4.0.Created",
"Resolution": "None",
"Severity": "OK"
}
]
}
3. Create NoAccess user with empty privilege
POST: https://<BMC-IP>/redfish/v1/AccountService/Accounts/
Body:
{
"UserName": "user3",
"RoleId": "",
"Password": "asdf1234"
}
Response:
{
"@Message.ExtendedInfo": [
{
"@odata.type": "#Message.v1_0_0.Message",
"Message": "The resource has been created successfully",
"MessageArgs": [],
"MessageId": "Base.1.4.0.Created",
"Resolution": "None",
"Severity": "OK"
}
]
}
4. Verified Patch, by updating the "NoAccess" RoleId to "Operator"
5. Verified Patch, by updating the "" RoleId to "ReadOnly"
6. Display user list
ID Name Callin Link Auth IPMI Msg Channel Priv Limit
1 root false true true ADMINISTRATOR
2 user2 false true true NO ACCESS
3 user3 false true true NO ACCESS
4 user4 false true true USER
5 user5 false true true OPERATOR
Signed-off-by: jayaprakash Mutyala <mutyalax.jayaprakash@intel.com>
Change-Id: Ied8bd452d1a57409bfdbe231332986d36bd07c72
|
|
For the ManagerAccountCollection resource,
/redfish/v1/AccountService/Accounts/, allow a "ConfigureSelf"
user to GET but only return the user's account if the user does
not have ConfigureUsers privilege.
Took this code from other places in account_service.
https://github.com/openbmc/bmcweb/blob/61dbeef97168db1a1f7a351c5f95e09afd361e48/redfish-core/lib/account_service.hpp#L1528
There was some question whether all accounts should be returned,
Redfish clarified that only the user's account should be returned
without ConfigureUsers privilege.
https://redfishforum.com/thread/281/manageraccountcollection-change-allows-account-enumeration
"we assumed that the Login privilege would only pertain to the
current account and not allow viewing of other accounts"
This fixes 2 Redfish validator errors if running the validator
as a Readonly or Operator role.
"ERROR - Accounts: GET of resource at URI
/redfish/v1/AccountService/Accounts returned HTTP 403. Check URI."
"ERROR - /redfish/v1/AccountService/Accounts: URI could not be
acquired: 403"
This was changed in Redfish 2019.3, redfish issue 1914 explains
more.
Tested: Ran the validator as operator role and admin role.
No errors.
As root:
curl -k https://${bmc}/redfish/v1/AccountService/Accounts/
{
"@odata.id": "/redfish/v1/AccountService/Accounts",
"@odata.type": "#ManagerAccountCollection.ManagerAccountCollection",
"Description": "BMC User Accounts",
"Members": [
{
"@odata.id": "/redfish/v1/AccountService/Accounts/readonly"
},
{
"@odata.id": "/redfish/v1/AccountService/Accounts/operator"
},
{
"@odata.id": "/redfish/v1/AccountService/Accounts/JimHalpert"
},
{
"@odata.id": "/redfish/v1/AccountService/Accounts/root"
}
],
"Members@odata.count": 4,
As Operator:
curl -k https://${bmc}/redfish/v1/AccountService/Accounts/
{
"@odata.id": "/redfish/v1/AccountService/Accounts",
"@odata.type": "#ManagerAccountCollection.ManagerAccountCollection",
"Description": "BMC User Accounts",
"Members": [
{
"@odata.id": "/redfish/v1/AccountService/Accounts/operator"
}
],
"Members@odata.count": 1,
Change-Id: I0694011ed3c4ecd3ea0c386fc24d086be39ac804
Signed-off-by: Gunnar Mills <gmills@us.ibm.com>
|
|
AccountProviderType was deprecated in AccountService.v1_5_0.
The latest AccountService is 1.7.0. Move from 1.4.0 to 1.5.0.
Fixes validator warnings:
WARNING - LDAP.AccountProviderType: The given property is deprecated
by revision: This property is deprecated because the account provider
type is known when used in the LDAP and ActiveDirectory objects.
WARNING - ActiveDirectory.AccountProviderType: The given property is
deprecated by revision: This property is deprecated because the
account provider type is known when used in the LDAP and
ActiveDirectory objects.
Although not sure if the validator should have thrown a
warning since the version of AccountService in bmcweb still
allowed this property.
AccountProviderType is not used in the GUI or other places.
Resolves openbmc/bmcweb#118
Tested: Passed validator.
Change-Id: Ifa15cdd2dfd0d740e42add778f30d00e7885ed4b
Signed-off-by: Gunnar Mills <gmills@us.ibm.com>
|
|
Redfish made odata.context optional (1.6.0 of DSP0266).
Redfish has removed odata.context from example payloads in the
specification (1.7.0 of DSP0266), removed it from the mockups,
and Redfish recommended not using.
The reason for making optional and removing from mockups/examples,
"no one could figure out how to use it and it did not add value".
Don't see value in it for our implementation.
From the Redfish issue removing it:
"@odata.context provides little/no value. The common format we use
provides no value/guidance. A generic odata client cannot use it
because we don't return the specific version nor do we require it
be changed with a query parameter. Between @odata.type and the
metadata document and service document/service root, clients get
all of the information they need. And the case where it is
helpful (joins, etc) is something we never do."
https://github.com/DMTF/Redfish-Service-Conformance-Check/pull/171
removes from Redfish-Service-Conformance-Check.
Tested: Ran service validator. No errors.
Ran Redfish-Service-Conformance-Check. No additional errors.
Change-Id: Ic2c33080604ea275cf487e5cd5b9f7948af07db9
Signed-off-by: Gunnar Mills <gmills@us.ibm.com>
|
|
"GET" should be a "Login" Privilege for the AccountService
resource. This makes sense, a "Readonly" and "Operator" user
should be able to see properties like MaxPasswordLength and
MinPasswordLength since they are allowed to change their own
password.
This was changed in Redfish 2019.3, redfish issue 1914 explains
more.
From Redfish_1.0.4_PrivilegeRegistry.json:
"Entity": "AccountService",
"OperationMap": {
"GET": [
{
"Privilege": [
"Login"
]
}
],
"HEAD": [
{
"Privilege": [
"Login"
]
}
],
"PATCH": [
{
"Privilege": [
"ConfigureUsers"
]
}
],
"PUT": [
{
"Privilege": [
"ConfigureUsers"
]
}
],
"DELETE": [
{
"Privilege": [
"ConfigureUsers"
]
}
],
"POST": [
{
"Privilege": [
"ConfigureUsers"
]
}
]
Change-Id: Iab8acbac97a58aed865bf94f665d6c9a32de81dd
Tested: Build for Witherspoon and AccountService looks good.
Signed-off-by: Gunnar Mills <gmills@us.ibm.com>
|
|
This change adds VirtualMedia scheme to Redfish.
Implementation is based on input from virtual-media module
and nbd proxy which is a bmcweb part. The code is used
only in case ndb-proxy is supported in bmcweb
(BMCWEB_ENABLE_VM_NBDPROXY compilation flag).
Tested:
* Manual tests together with nbd proxy and virtual media app
- For requests: Postman and/or HTTPie, started with logs
enabled and Valgrind
- Manual result validation
* Tests ran:
- GET on collection with manual validation
- PUT/POST/DELETE on collection
- GET on item/nonexistent item
- PUT/POST/DELETE on item
* Redfish Service Validator tested, no new issues found.
Signed-off-by: Przemyslaw Czarnowski <przemyslaw.hawrylewicz.czarnowski@intel.com>
Change-Id: I5415dc0ffe52069fd35bc614b0378bbc4ad41ff6
|
|
Enhances BMCWeb to correctly handle the Redfish ConfigureSelf privilege.
Redfish document DSP2046 defines the ConfigureSelf privilege as
"Can change the password for the current user account and log out of
their own sessions." This notion is formalized in the Redfish DSP8011
PrivilegeRegistry where ConfigureSelf appears in three operations:
- ManagerAccount (/redfish/v1/AccountService/Accounts/{account}) GET operation.
- ManagerAccount (/redfish/v1/AccountService/Accounts/{account}) PATCH
Password property override.
- Session (/redfish/v1/SessionService/Sessions/{sessionid}) DELETE operation.
Tested: Yes, tested the above operations using users with various Roles to
determine which operations are allowed.
ReadOnly users (privileges: Login, ConfigureSelf):
- Can GET their own account.
- Can change their password.
- Can log out.
- Cannot change any other properties of their own account.
- Cannot change anyone else's password.
- Cannot GET someone else's account.
- Cannot log out anyone else.
Operator users (privileges: Login, ConfigureComponents, ConfigureSelf):
- Same access as a ReadOnly user.
Administrator users (all privileges):
- Can do everything Operator can do.
- Can change one or more properties of their account
- Can GET and change properties of someone else's account.
- Can logoff any session.
Signed-off-by: Joseph Reynolds <joseph-reynolds@charter.net>
Change-Id: If8efd71cb9743a59b7c5fe1565804d21e788ea29
|
|
User is now able to turn on and off the TLS authentication method.
Tested:
No regression found in manual testing. By default everything works
as before, and disabling TLS method prevents user to authenticate
by it.
Tested with Redfish Service Validator, version 1.2.8
Signed-off-by: Zbigniew Kurzynski <zbigniew.kurzynski@intel.com>
Change-Id: Ib7be1af659db568caa7e5b97e3844617586d7754
|
|
Added NoAccess role to the Redfish, to properly show
users created using IPMI with NoAccess privilege.
This patch will add NoAccess role & will use the same
when the user privilege is empty.
Note: This code was reverted due to redfish validator failure
and the same has been fixed in this patch, by creating
proper json array empty object.
Tested:
1. Verified redfish validator passed and the earlier issue of
failNullCollection for the NoAccess role is resolved.
2. Verified NoAccess role is listed properly
Get: https://<BMC IP>/redfish/v1/AccountService/Roles/NoAccess
{
"@odata.context": "/redfish/v1/$metadata#Role.Role",
"@odata.id": "/redfish/v1/AccountService/Roles/NoAccess",
"@odata.type": "#Role.v1_2_2.Role",
"AssignedPrivileges": [],
"Description": "NoAccess User Role",
"Id": "NoAccess",
"IsPredefined": true,
"Name": "User Role",
"OemPrivileges": [],
"RoleId": "NoAccess"
}
3. Verified user with No Privilege is listed without any error.
Get: https://<BMC IP>/redfish/v1/AccountService/Accounts/user6
{
"@odata.context": "/redfish/v1/$metadata#ManagerAccount.
ManagerAccount",
"@odata.id": "/redfish/v1/AccountService/Accounts/user6",
"@odata.type": "#ManagerAccount.v1_0_3.ManagerAccount",
"Description": "User Account",
"Enabled": false,
"Id": "user6",
"Links": {
"Role": {
"@odata.id": "/redfish/v1/AccountService/Roles/NoAccess"
}
},
"Locked": false,
"Locked@Redfish.AllowableValues": [
"false"
],
"Name": "User Account",
"Password": null,
"RoleId": "NoAccess",
"UserName": "user6"
}
Change-Id: If9577598e0a6215cf76f5db031ad5f8bcf2387a7
Signed-off-by: jayaprakash Mutyala <mutyalax.jayaprakash@intel.com>
Signed-off-by: Richard Marian Thomaiyar <richard.marian.thomaiyar@linux.intel.com>
|
|
priv-callback is valid only for IPMI modem callback, which
was never used, and it's decided to deprecate the same
https://gerrit.openbmc-project.xyz/#/c/openbmc/docs/+/26839/
Removing the support in redfish now.
Tested:
1. Verified callback role was not in list in Get of
https://<BMC IP>/redfish/v1/AccountService/Roles/
2. Redfish validator passed for this change.
Change-Id: Ia16fb584a07bbdf29197cd5dd54e7a9682627c19
Signed-off-by: Richard Marian Thomaiyar <richard.marian.thomaiyar@linux.intel.com>
|
|
This reverts commit 27c10d2ee746b85e9463efb0fc6773c209b2f5ba.
Reason for revert: <Makes the validator fail>
Change-Id: I379d9eda57416476ff1cc17e594c55dedd0bc4eb
Signed-off-by: James Feist <james.feist@linux.intel.com>
|
|
Issue: With IPMI command- If New user created, by defualt created with
"NO ACCESS" Channel priv Limit. But same role is not populating from
Redfish.
This test can be done only with below patch being merged.
https://gerrit.openbmc-project.xyz/#/c/openbmc/phosphor-user-manager/
+/24784/
Tested:
Added "NoAccess" Role to Redfish(if Channel privilege Limit is empty in
userlist).
Below is Snapshot from Redfish:
Get: https://<BMC IP>/redfish/v1/AccountService/Roles/NoAccess
{
"@odata.context": "/redfish/v1/$metadata#Role.Role",
"@odata.id": "/redfish/v1/AccountService/Roles/NoAccess",
"@odata.type": "#Role.v1_2_2.Role",
"AssignedPrivileges": null,
"Description": "NoAccess User Role",
"Id": "NoAccess",
"IsPredefined": true,
"Name": "User Role",
"OemPrivileges": [],
"RoleId": "NoAccess"
}
Get: https://<BMC IP>/redfish/v1/AccountService/Accounts/user6
{
"@odata.context": "/redfish/v1/$metadata#ManagerAccount.
ManagerAccount",
"@odata.id": "/redfish/v1/AccountService/Accounts/user6",
"@odata.type": "#ManagerAccount.v1_0_3.ManagerAccount",
"Description": "User Account",
"Enabled": false,
"Id": "user6",
"Links": {
"Role": {
"@odata.id": "/redfish/v1/AccountService/Roles/NoAccess"
}
},
"Locked": false,
"Locked@Redfish.AllowableValues": [
"false"
],
"Name": "User Account",
"Password": null,
"RoleId": "NoAccess",
"UserName": "user6"
}
Redfish validator test results:
Counter({'skipOptional': 31791, 'pass': 22397, 'passGet': 2293,
'metadataNamespaces': 1047, 'warningPresent': 70,
'serviceNamespaces': 68, 'invalidPropertyValue': 67,
'err.LogEntry.v1_0_0.EventSeverity': 64, 'failProp': 64,
'repeat': 14, 'reflink': 9, 'passAction': 7, 'optionalAction': 6,
'failErrorPresent': 1, 'unverifiedComplexAdditional': 1,
'warnTrailingSlashLink': 1})
Validation has failed: 65 problems found
Signed-off-by: jayaprakash Mutyala <mutyalax.jayaprakash@intel.com>
Change-Id: Ibc74e2fe4519ec6160dd516893d5e542feeabb0d
|
|
Modified POST method to handle redfish user creation
error codes.
Tested:
Tested user creation with below test cases
1)Already user exists
2)Max users reached
3)Username is NULL
4)Username is not starting with alphabet
5)Username exceed more than 16 characters
6)Invalid Password
Redfish validator test results: Passed
Signed-off-by: anil kumar appana <anil.kumarx.appana@intel.com>
Change-Id: I58361ddd4dfd067802f805f9d870b2bc1692ea1d
|
|
Added Oem extension for AccountService allowing user to configure
which authentication methods should be enabled. User is now able
to turn on and off authentication methods like BasicAuth, XToken, etc.
User is not allowed to turn off all of the methods at once - at least
one method has to be active to prevent lock-out. This configuration
is persistent, will be saved on file-system and will be loaded on
bmcweb's restart.
Tested:
No regression found in manual testing. By default everything works as before,
and disabling auth method prevents user to authenticate by it. Tested that
user is not allowed to disable all the methods - either in one PATCH or by
disabling them one at a time.
ServiceValidator run with success.
This change is a fix for this request:
https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/23590/18
which was revert here:
https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/26869
Signed-off-by: Zbigniew Kurzynski <zbigniew.kurzynski@intel.com>
Change-Id: I66b5ad423746f1992070a14f2983a07b1320190e
|
|
Modified doPatch method to populate redfish user update error codes.
Tested:
Tested user updates with below scenarios
1)Provided username is not exist
2)Replace username already user exists
3)Replace Username is NULL/Invalid
4)Replace username is not starting with alphabet
5)Replace username exceeds more than 16 characters
6)Password is not valid for Replace/existing username
Redfish validator test results:
1 failProp errors in /redfish/v1/Systems/system/LogServices/EventLog
1 problemResource errors in /redfish/v1/Systems/system/LogServices/
EventLog/Entries
Counter({'skipOptional': 17887, 'pass': 12133, 'passGet': 1285,
'metadataNamespaces': 1047, 'serviceNamespaces': 69, 'reflink': 9,
'passAction': 7, 'warningPresent': 6, 'optionalAction': 6,
'repeat': 3, 'invalidPropertyValue': 3, 'failErrorPresent': 1,
'err.LogEntryCollection.LogEntryCollection': 1, 'failProp': 1,
'unvalidated': 1, 'problemResource': 1,
'unverifiedComplexAdditional': 1, 'warnTrailingSlashLink': 1})
Validation has failed: 3 problems found
Signed-off-by: jayaprakash Mutyala <mutyalax.jayaprakash@intel.com>
Change-Id: Ibee448c5d5c4f38c5c4cacda757864593f6001fc
|
|
This reverts commit 0ff64dc2cd3a15b4204a477ad2eb5219d66e6110.
Reason for revert: <breaks redfish validator, <edmx:Reference Uri="/redfish/v1/schema/OemAccountService_v1.xml"> but the file name unversioned static/redfish/v1/schema/OemAccountService.xml>
Change-Id: I696dd09bf519e364f5f529a674e047a8eeead578
Signed-off-by: James Feist <james.feist@linux.intel.com>
|
|
Added Oem extension for AccountService allowing user to configure
which authentication methods should be enabled. User is now able
to turn on and off authentication methods like BasicAuth, XToken, etc.
User is not allowed to turn off all of the methods at once - at least
one method has to be active to prevent lock-out. This configuration
is persistent, will be saved on file-system and will be loaded on
bmcweb's restart.
Tested:
No regression found in manual testing. By default everything works as before,
and disabling auth method prevents user to authenticate by it. Tested that
user is not allowed to disable all the methods - either in one PATCH or by
disabling them one at a time.
ServiceValidator run with success.
Change-Id: I3a775d783ac05998d17b8e91800962bffd8cab52
Signed-off-by: Kowalski, Kamil <kamil.kowalski@intel.com>
Signed-off-by: Zbigniew Kurzynski <zbigniew.kurzynski@intel.com>
|
|
We had a couple uses of push_back in the code that could be made more
efficient with emplace(). Use them instead.
Tested: No functional change. Needs tested.
Signed-off-by: Ed Tanous <ed.tanous@intel.com>
Change-Id: I417601e416b1d0be989617a372978d52670135d8
|
|
As per redfish specification (DSP0266), there are
set of predefined privilege roles. In OpenBMC code
has "User" as role name instead of "ReadOnly".
So corrected the same. Updated Redfish.md accordingly.
Spec says:
Role Name = "ReadOnly"
▪ AssignedPrivileges = Login, ConfigureSelf
Tested:
- Role collection shows new role.
- GET on /redfish/v1/AccountService/Roles/ReadOnly
URI shows correct AssignedPrivileges.
- Ran negative test with /redfish/v1/AccountService/Roles/User
and observed error(404 - Not Found).
- Ran Redfish validator and no new issues observed.
Signed-off-by: AppaRao Puli <apparao.puli@linux.intel.com>
Change-Id: I7b0132c628fb4950b6ec095269cd1a12e92aea9a
|
|
using the list of warnings from here:
https://github.com/lefticus/cppbestpractices/blob/e73393f25a85f83fed7399d8b65cb117d00b2231/02-Use_the_Tools_Available.md#L100
Seems like a good place to start, and would improve things a bit
type-wise. This patchset attempts to correct all the issues in one
shot.
Tested:
It builds. Will test various subsystems that have been touched
Signed-off-by: Ed Tanous <ed.tanous@intel.com>
Change-Id: I588c26440e5a97f718a0f0ea74cc84107d53aa1e
|
|
Added PATCH operation support for RemoteRoleMapping property under
LDAP/ActiveDirectory property in AccountService schema.
1. How to add the Role Mapping?
PATCH {"ActiveDirectory":{"RemoteRoleMapping": [{"RemoteGroup":
"Admingroup15","LocalRole": "User"},{"RemoteGroup": "Admingroup13",
"LocalRole": "Administrator"},{"RemoteGroup": "Admingroup14",
"LocalRole": "Operator"}]}}
With the above PATCH request, all the above role mapping gets added.
2. How to delete a specific role mapping?
After adding the above roles mapping, if user want to delete the second mapping
which is ({"RemoteGroup": "Admingroup13", "LocalRole": "Administrator"})
Following PATCH request would be used.
PATCH {"ActiveDirectory":{"RemoteRoleMapping": [{},null,{}]}}
3. How to update specific role mapping ?
Let's take a case where user want to update the second role mapping
PATCH {"ActiveDirectory":{"RemoteRoleMapping": [{},{"RemoteGroup":"Admingroup25","LocalRole": "User"},{}]}}
or
PATCH {"ActiveDirectory":{"RemoteRoleMapping": [{},{"RemoteGroup":"Admingroup25"},{}]}} and \
PATCH {"ActiveDirectory":{"RemoteRoleMapping": [{},{"LocalRole": "User"},{}]}}
Tested:
1. Did a PATCH operation with below given Data:
' {"ActiveDirectory":{"RemoteRoleMapping": [{"RemoteGroup": "Admingroup215","LocalRole": "User"}, \
{"RemoteGroup": "Admingroup213","LocalRole":"Administrator"},{"RemoteGroup":"Admingroup214","LocalRole":"Operator"}]}}'
2. With GET got below given data:
"RemoteRoleMapping": [
{
"LocalRole": "Operator",
"RemoteGroup": "Admingroup214"
},
{
"LocalRole": "Administrator",
"RemoteGroup": "Admingroup213"
},
{
"LocalRole": "User",
"RemoteGroup": "Admingroup215"
}
],
3. Did a PATCH operation with below given Data:
'{"ActiveDirectory":{"RemoteRoleMapping": [{},null,{}]}}'
4. With GET got below given data:
"RemoteRoleMapping": [
{
"LocalRole": "Operator",
"RemoteGroup": "Admingroup214"
},
{
"LocalRole": "User",
"RemoteGroup": "Admingroup215"
}
],
5. Did a PATCH operation with below given Data:
'{"ActiveDirectory":{"RemoteRoleMapping": [null,null]}}'
6. With GET got below given data:
"RemoteRoleMapping": []
7. Did a PATCH operation with below given Data:
'{"ActiveDirectory":{"RemoteRoleMapping": [{"RemoteGroup": "Admingroup215","LocalRole": "User"}, \
{"RemoteGroup": "Admingroup213","LocalRole":"Administrator"},{"RemoteGroup":"Admingroup214","LocalRole":"Operator"}]}}'
8. With GET got below given data:
"RemoteRoleMapping": [
{
"LocalRole": "Administrator",
"RemoteGroup": "Admingroup213"
},
{
"LocalRole": "Operator",
"RemoteGroup": "Admingroup214"
},
{
"LocalRole": "User",
"RemoteGroup": "Admingroup215"
}
],
9. Did a PATCH operation with below given Data:
'{"ActiveDirectory":{"RemoteRoleMapping": [{"RemoteGroup": "Admingroup25"},{},{}]}}'
10.With GET got below given data:
"RemoteRoleMapping": [
{
"LocalRole": "Administrator",
"RemoteGroup": "Admingroup25"
},
{
"LocalRole": "Operator",
"RemoteGroup": "Admingroup214"
},
{
"LocalRole": "User",
"RemoteGroup": "Admingroup215"
}
],
11. Did a PATCH operation with below given Data:
'{"ActiveDirectory":{"RemoteRoleMapping": [{"LocalRole": "User"},{},{}]}}'
12.With GET got below given data:
"RemoteRoleMapping": [
{
"LocalRole": "User",
"RemoteGroup": "Admingroup25"
},
{
"LocalRole": "Operator",
"RemoteGroup": "Admingroup214"
},
{
"LocalRole": "User",
"RemoteGroup": "Admingroup215"
}
],
13. Did a PATCH operation with below given Data:
'{"ActiveDirectory":{"RemoteRoleMapping": [{},{"RemoteGroup": "Admingroup26","LocalRole": "User"},{}]}}'
14.With GET got below given data:
"RemoteRoleMapping": [
{
"LocalRole": "User",
"RemoteGroup": "Admingroup25"
},
{
"LocalRole": "User",
"RemoteGroup": "Admingroup26"
},
{
"LocalRole": "User",
"RemoteGroup": "Admingroup215"
}
],
15. Try to delete the role map when there was no role map entry and get the following error.
"RemoteRoleMapping/1@Message.ExtendedInfo": [
{
"@odata.type": "/redfish/v1/$metadata#Message.v1_0_0.Message",
"Message": "The value null for the property RemoteRoleMapping/0 is of a different type than the property can accept.",
"MessageArgs": [
"null",
"RemoteRoleMapping/0"
],
"MessageId": "Base.1.4.0.PropertyValueTypeError",
"Resolution": "Correct the value for the property in the request body and resubmit the request if the operation failed.",
"Severity": "Warning"
}
Signed-off-by: Ratan Gupta <ratagupt@linux.vnet.ibm.com>
Change-Id: Iaa37221bd6fdc87dbf51755d9425ecd5b07eee6c
|