Repository navigation
fix(console): serve an IAM user its own user page and access keys only - #844
LeeroyHannigan wants to merge 5 commits into
Conversation
An IAM-user console session could open every page of its own account. The account page named every user, group and role. A user page showed that user's access key IDs, inline policy names, groups and tags. A group page showed its members and inline policy names, and a role page showed its trust policy, inline policy names and tags. The handlers checked only that the account in the path was the session's own. The management API answers the same reads to the same user with HTTP 403. An IAM-user session now opens only its own user page and the forms that create and delete its own access keys. These are the access key reads and writes the management API gives the same user. The console has no form to change the password or import a key. require_user_session lets an IAM-user session through when the account ID and the user name in the path are its own. The management API applies the same rule to an IAM user's access keys. Any other user's pages redirect it to /console before the handler reads anything about that user. The account page, the group and role pages and the account list are for admins. Their handlers redirect an IAM-user session to /console before they read what the page shows. The admin-only forms under the account also redirect it to /console. Before, some of them redirected to the account page, the group page or the account list. The user's own page lists its access keys and leaves out its inline policies, groups and tags. The management API does not give the user those either. The dashboard links an IAM-user session to its own user page, and the navigation bar shows the Accounts link to admins only. The breadcrumbs of the user page and the new access key page leave out the pages the session may not open. Admin sessions get the same pages as before. The tests that read an IAM user's own account page, its dashboard link and its account list now expect the redirects.
…d keys tests/test_console_iam_user_pages.py logs in as an IAM user that holds only the self-service policy every user created with a console password gets. The account also holds two other users, a group and a role, each with an inline policy, and one user with an access key and a tag. The tests check that the account list, the account forms and every page and form under the user's account other than its own user page and access key forms redirect it to /console, and that no refused form changes the catalog. They check that no page it opens names another user, group or role of the account or links to a page that redirects it, and that the management API refuses the user the same reads. The user still reads, creates and deletes its own access keys and logs out, and an admin session still reads every page.
The getting started guide, the admin guide, the architecture guide and the security model now say that an IAM-user session opens its own user page and manages its own access keys, and that every other page under its account redirects it to the dashboard. The upgrade manual has a v0.1.15 entry for the change. The earlier v0.1.15 entry for console account scoping no longer says that the home page links an IAM user to its own account, or that the pages of the user's own account are unchanged.
account_scope_reads_tests.rs runs the account, group, role and user page handlers against a catalog store that holds no data and counts every call to it. An IAM-user session that requests the account page, a group page, a role page or another user's page gets a redirect to /console (HTTP 303). The handler makes no call to the store. An admin request and the IAM user's own user page each make one call. The integration test posts the create and delete access key forms as the IAM user. It posts each form twice, once with an empty CSRF token and once with a wrong token of the same length. Each answer is HTTP 403, and the user's access keys, as the management API lists them, do not change. The IAM user in the integration test now belongs to a second group and has a tag. The test checks that its own page leaves out that group and tag, and that the admin's view of the same page shows them.
hitesh96db
left a comment
There was a problem hiding this comment.
This fixes #731 as far as I can see, but it needs a rebase before it can merge. #727 and #704 both merged after this branch was cut, and the PR now conflicts with main. The inline comments cover the conflict resolution and two test breaks that the merge does not flag as conflicts.
What I checked
All on this PR's head (1fc6c234), with Rust 1.97.0 (the CI toolchain):
- Every route has a guard. There are 30 routes under
/console/accounts. Three userequire_user_session:user_detail,create_access_keyanddelete_access_key. The other 27 redirect an IAM-user session to/consolebefore their first catalog call. The nine policy routes get this frompolicy_form,put_policyanddelete_policy, which this PR does not change and which were already admin-only. user_scopefails closed. It compares the account ID and user name in the path with the session's own, exactly. It reads nothing, so another user and a name no user has get the same answer. This is the rule the management API applies inauthorize_self_service(management/iam_user_self.rs).- Key deletion is scoped to the path's user. The access key forms trust the
{name}in the path. That is safe becausedelete_access_keymatches on access key ID, account ID and user name on all four backends: the SQLWHEREon SQLite and PostgreSQL, thedelete_onefilter on MongoDB, and the LWTIFon Cassandra. An IAM user cannot delete another user's key by putting that key's ID under its own user. - The own user page shows keys only. It ends after the access keys section for an IAM-user session, so the policies, groups and tags sections are not rendered.
cargo test -p extenddb-server --lib console: 34 passed.- Against a SQLite release build of this PR on 127.0.0.1,
tests/test_console_iam_user_pages.py,test_console_account_scope.py,test_console_integration.pyandtest_cross_account_isolation.py: 159 passed.
Rebase onto main
A trial merge of origin/main conflicts in crates/server/src/console/html.rs, crates/server/src/console/pages/mod.rs and docs/manuals/10-security-model.md. These are the conflicts the description predicts:
pages/mod.rs: #727 added the sameidentity_label->html::NavIdentitychange, so this conflict is the import lines only.10-security-model.md: take this PR's paragraph without the "Until pull request #727 merges, ..." sentence, and keep #727's admin-only metrics bullet.html.rs: see the inline comment.- #704 test breaks: after the merge,
account_scope_reads_tests.rsdoes not compile because of #704's API changes. See the two inline comments there.
Not blocking
- IAM users lose some reads. An IAM user can no longer see its own inline policies, groups and tags in the console. That matches the management API, and the upgrade manual says so. It would be good for a second reviewer to agree to this explicitly.
- CodeQL alerts. The four "Cleartext logging of sensitive information" alerts flag account IDs in redirect handling and test messages. They look like false positives.
Not checked
- The rebased branch, which does not build yet.
- Backends other than SQLite for the Python suites.
This resolves the conflicts with #727 and #704. The navigation bar puts Accounts with the admin-only links of #727, so the bar of an IAM user shows Dashboard, its name and Logout. The security model keeps the paragraph of this branch without its sentence about #727, and keeps the metrics bullet of #727. On main, #704 checks each console request against the password hash of the session's principal. The read-count tests answer that read, count it apart from the other calls, create their sessions with a stamp and set `secure_cookies`. The docs of account_scope.rs say that the account check runs right after the session check. The security model and the upgrade manual say that the session check is the only read before the user check. The navigation bar test of #727 now expects no Accounts link for an IAM user. Signed-off-by: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com>
|
@hitesh96db thanks for the review. I merged main into this branch as e5cd478, with the resolution that your summary and the description give. Your review mentions inline comments, but none reached the PR, so I worked from the summary. The "Merging with main" section of the description lists each change, with the two test breaks from #704. Could you take another look? |
What
An IAM-user console session now reads only its own user page and its own access keys in its account. Every other page and form under
/console/accountsredirects the session to/console. Admin sessions see every page as before./console/accountsare for admin sessions only.require_user_sessioninaccount_scope.rsmakes this check. The management API uses the same rule for the access keys of an IAM user. Another user's name and a name that no user has get the same answer.GET .../users/{self}/policiesandGET .../users/{self}/tagsanswer 403, and no route lists the groups of a user./console. These are the forms that change users, groups, roles, members and policies, the delete account form and the new account form. Before, some of them sent the user to the account page, the group page or/console/accounts.Why
Closes #731.
An IAM-user console session read every page of its own account. It saw every user, group and role name. It also saw the access key IDs, inline policy names, groups and tags of every user. Group pages showed the members and policy names, and role pages showed the trust policy and policy names. The management API refuses the same reads to the same user with HTTP 403.
docs/getting-started.mdsection 8 said that IAM users view their own details and manage their own access keys.Testing done
account_scope_reads_tests.rsuses a store that counts calls. For an IAM-user session, the account page, a group page, a role page and another user's page answer 303 to/console. Each makes one password hash read, for the session check, and no other store call. The user's own page and each admin page make 1 other call.tests/test_console_iam_user_pages.py(53 tests) logs in as an IAM user that holds only the self-service policy that every user with a password gets. It checks these things:/consoleand change nothing.tests/test_console_*.pyandtests/test_cross_account_isolation.py: 168 passed on a SQLite build.cargo fmt --checkandcargo clippy -p extenddb-server --all-targets -- -D warningspass.cargo fmt --checkand CI's clippy command pass. Rust 1.88.0 builds the server tests.tests/test_console_*.pyandtests/test_cross_account_isolation.py: 184 passed and 1 skipped on a SQLite build.account_detailorgroup_detail, and added a second password hash read ingroup_detail.Merging with main
Commit e5cd478 merges main into this branch. On main, #727, #704, #773 and #875 change the console code that this PR changes. The merge does what this section planned:
html.rs: Accounts is now one of theADMIN_LINKSof fix: serve the console metrics dashboard to admin sessions only #727. The admin bar shows Dashboard, Accounts, Metrics, Settings and Cache. The IAM-user bar shows Dashboard, the identity label and Logout.10-security-model.md: the paragraph of this PR without its sentence about fix: serve the console metrics dashboard to admin sessions only #727, and the "Admin-only metrics" bullet of fix: serve the console metrics dashboard to admin sessions only #727.metrics_pages_tests.rs:nav_bar_links_the_admin_only_pages_for_admins_onlynow asserts that the IAM-user bar has no Accounts link, and that Dashboard comes right before the spacer.account_scope_reads_tests.rs:CountingStoreanswers the session check of fix(console): end console sessions when the principal is deleted or its password changes #704 throughPasswordHashStoreand counts that read apart from the other calls. A refused page must make exactly one hash read and no other call. The tests create their sessions with aCredentialStampand setsecure_cookiesfrom fix(console): mark the session cookie Secure when the server serves TLS #773.account_scope.rs, the security model and the upgrade manual: the account and user checks run right after the session check. That check reads only the password hash of the session's own principal.Checklist
cargo test --workspace)cargo fmt --check)cargo clippy -- -W clippy::pedantic)Storagetrait, auth model, on-diskformat, or public CLI surface, an RFC has been accepted or is linked
below. Otherwise, an ADR captures the decision (link below).
ADR / RFC: n/a. The change limits what an IAM-user console session reads to what the management API already gives that user. The API, the storage and the policy evaluation do not change.
Breaking changes
In the console, an IAM user can no longer open its account page or the pages of other users. It can no longer open the group and role pages or the account list either. Its own user page no longer shows its inline policies, groups and tags. It still creates and deletes its own access keys there.
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache License 2.0 and I agree to the Developer Certificate of
Origin (DCO). See CONTRIBUTING.md for details.