RGW: add new cap 'keys' #50313

alimaredia · 2023-02-28T19:56:44Z

This new capability for a user of the admin ops api determines whether the user can write keys via admin api operations and read keys from the response of an operation made via the admin api.

Details of the behavior pull request can be read here:
https://gist.github.com/alimaredia/02fd412aaa024922f9270fc24a01857f

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

src/rgw/driver/rados/rgw_rest_user.cc

alimaredia · 2023-02-28T21:00:53Z

doc/radosgw/adminops.rst

-will be swift type. If only one of ``access-key`` or ``secret-key`` is provided the
-committed key will be automatically generated, that is if only ``secret-key`` is
-specified then ``access-key`` will be automatically generated. By default, a
+will be swift type. If only one of ``access-key`` or ``secret-key`` is provided an


Though this is out of scope for this PR I noticed that both of the keys needed to be included in a create new key request or else the operation would not go through. I get a InvalidSecretKey and InvalidAccessKeyID when either are empty in a request.

alimaredia · 2023-02-28T21:11:46Z

doc/radosgw/admin.rst

@@ -311,7 +311,7 @@ To add administrative capabilities to a user, execute the following::
 You can add read, write or all capabilities to users, buckets, metadata and 
 usage (utilization). For example::

-	--caps="[users|buckets|metadata|usage|zone|amz-cache|info|bilog|mdlog|datalog|user-policy|oidc-provider|roles|ratelimit]=[*|read|write|read, write]"
+	--caps="[users|keys|buckets|metadata|usage|zone|amz-cache|info|bilog|mdlog|datalog|user-policy|oidc-provider|roles|ratelimit]=[*|read|write|read, write]"


@cbodley do you think it would be worth documenting what each of these caps does at a high level in the docs? There's nothing like that so far, though in the admin api does reference which caps are necessary for certain operations.

if the adminapi docs are consistent about showing the required caps, i think that's probably all the documentation we need

cbodley

👍

cbodley · 2023-02-28T20:03:24Z

qa/tasks/radosgw_admin_rest.py

@@ -141,7 +141,7 @@ def task(ctx, config):
    admin_display_name = 'Ms. Admin User'
    admin_access_key = 'MH1WC2XQ1S8UISFDZC8W'
    admin_secret_key = 'dQyrTPA0s248YeN5bBv4ukvKU0kh54LWWywkrpoG'
-    admin_caps = 'users=read, write; usage=read, write; buckets=read, write; zone=read, write; info=read;ratelimit=read, write'
+    admin_caps = 'users=read, write; usage=read, write; buckets=read, write; zone=read, write; info=read;ratelimit=read, write; keys=read, write'


ok, so this verifies that all of the output is the same if you add the keys caps; but i think we also want to test that we don't see keys if we're missing that cap. i think that means we need to create a separate Assistant Admin User with only the user caps?

Yup, I'm going to be adding those tests at the end of the file. I wanted to just get the PR out there and make sure that this tests passes with the new caps on a locked teuthology machine before I start adding more to this test file.

src/rgw/driver/rados/rgw_rest_user.cc

src/rgw/driver/rados/rgw_user.cc

cbodley · 2023-02-28T21:13:21Z

doc/radosgw/admin.rst

@@ -311,7 +311,7 @@ To add administrative capabilities to a user, execute the following::
 You can add read, write or all capabilities to users, buckets, metadata and 
 usage (utilization). For example::

-	--caps="[users|buckets|metadata|usage|zone|amz-cache|info|bilog|mdlog|datalog|user-policy|oidc-provider|roles|ratelimit]=[*|read|write|read, write]"
+	--caps="[users|keys|buckets|metadata|usage|zone|amz-cache|info|bilog|mdlog|datalog|user-policy|oidc-provider|roles|ratelimit]=[*|read|write|read, write]"


if the adminapi docs are consistent about showing the required caps, i think that's probably all the documentation we need

src/rgw/driver/rados/rgw_rest_user.cc

alimaredia · 2023-03-01T21:21:17Z

src/rgw/driver/rados/rgw_cr_tools.cc

+  if(keys_perm < 0) {
+    dump_keys = false;
+  }
+  return RGWUserAdminOp_User::create(dpp, store, op_state, flusher, dump_keys, null_yield);


@cbodley I determined the value of dump_keys from the op_state here. Was that the right thing to do? I don't know which code paths exercise this RGWUserAdminOp_User::create() so I'm not sure if there is an req_state somewhere up the call stack.

@alimaredia i think it's right, but nothing uses this RGWUserCreateCR class after @yuvalif removed pubsub in #48996. i'd rather remove the class entirely than continue to maintain it

cbodley · 2023-03-02T20:58:31Z

looks great, just needs some new test cases

alimaredia · 2023-04-10T14:10:04Z

teuthology run: https://pulpito.ceph.com/amaredia-2023-04-10_02:27:16-rgw:singleton-wip-rgw-add-cap-keys-distro-default-smithi/

alimaredia · 2023-04-10T14:12:17Z

jenkins test make check

alimaredia · 2023-04-10T14:12:25Z

jenkins test api

alimaredia · 2023-04-11T15:15:56Z

jenkins test make check

alimaredia · 2023-04-11T15:16:06Z

jenkins test api

cbodley · 2023-04-12T19:09:11Z

can you please document this change in PendingReleaseNotes? something like:

RGW: A new "keys" capability was added to the /admin/user REST APIs. The "keys=write" capability is now required to create or modify secret keys, and "keys=read" is required to read them.

i think this broke something in the api tests:

FAIL: test_all (tasks.mgr.dashboard.test_rgw.RgwBucketTest)

github-actions · 2023-04-12T20:00:15Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

This new capability for a user of the admin ops api determines whether the user can write keys and read keys from the response of an operation made via the admin api. Tests in teuthology for new cap added as well. Signed-off-by: Ali Maredia <amaredia@redhat.com>

Signed-off-by: Ali Maredia <amaredia@redhat.com>

cbodley · 2023-08-11T16:24:48Z

src/pybind/mgr/dashboard/services/rgw_client.py

@@ -223,6 +223,7 @@ def _get_user_keys(user: str, realm: Optional[str] = None) -> Tuple[str, str]:
                'user', 'create',
                '--uid', user,
                '--display-name', 'Ceph Dashboard',
+                '--caps', 'keys=*'


@alimaredia oh, this is already a --system user. i think our rest apis should treat system users as if they have all caps

Signed-off-by: Ali Maredia <amaredia@redhat.com>

github-actions · 2023-09-07T13:26:10Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

alimaredia · 2023-09-19T19:52:13Z

jenkins test api

alimaredia · 2023-09-19T21:05:37Z

jenkins retest this please

github-actions · 2023-11-18T22:01:26Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

github-actions · 2023-12-18T22:01:31Z

This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution!

alimaredia requested review from a team as code owners February 28, 2023 19:56

github-actions bot added documentation rgw tests labels Feb 28, 2023

alimaredia commented Feb 28, 2023

View reviewed changes

src/rgw/driver/rados/rgw_rest_user.cc Show resolved Hide resolved

alimaredia commented Feb 28, 2023

View reviewed changes

cbodley reviewed Feb 28, 2023

View reviewed changes

alimaredia commented Feb 28, 2023

View reviewed changes

src/rgw/driver/rados/rgw_rest_user.cc Outdated Show resolved Hide resolved

alimaredia commented Mar 1, 2023

View reviewed changes

alimaredia force-pushed the wip-rgw-add-cap-keys branch from 54fe3c9 to d7a273b Compare March 1, 2023 21:26

cbodley added the needs-test label Mar 2, 2023

alimaredia force-pushed the wip-rgw-add-cap-keys branch 2 times, most recently from d69f04b to 1fd7118 Compare April 7, 2023 20:40

cbodley approved these changes Apr 10, 2023

View reviewed changes

alimaredia force-pushed the wip-rgw-add-cap-keys branch from d7d3d66 to 01bba25 Compare April 11, 2023 17:15

alimaredia requested a review from a team as a code owner April 12, 2023 19:59

alimaredia requested review from nizamial09 and Pegonzal and removed request for a team April 12, 2023 19:59

github-actions bot added dashboard needs-rebase labels Apr 12, 2023

github-actions bot added the pybind label Apr 18, 2023

alimaredia force-pushed the wip-rgw-add-cap-keys branch 2 times, most recently from 822011d to 6374e69 Compare July 27, 2023 19:18

github-actions bot removed the needs-rebase label Jul 27, 2023

rgw: add new cap 'keys'

8b4c81b

This new capability for a user of the admin ops api determines whether the user can write keys and read keys from the response of an operation made via the admin api. Tests in teuthology for new cap added as well. Signed-off-by: Ali Maredia <amaredia@redhat.com>

alimaredia force-pushed the wip-rgw-add-cap-keys branch from 670aec9 to 8b4c81b Compare August 11, 2023 13:36

SQUASH-ME: add cap keys to dashboard user

fce1bb0

Signed-off-by: Ali Maredia <amaredia@redhat.com>

cbodley reviewed Aug 11, 2023

View reviewed changes

alimaredia added 3 commits August 11, 2023 14:57

SQUASH-ME: remove unnecessary log statement

e5d2939

Signed-off-by: Ali Maredia <amaredia@redhat.com>

REMOVE-ME: just try to get the api tests to pass

05f8c0b

Signed-off-by: Ali Maredia <amaredia@redhat.com>

SQUASH-ME: add cap key to teuth-test-user and set system_user

1e2e69b

Signed-off-by: Ali Maredia <amaredia@redhat.com>

alimaredia force-pushed the wip-rgw-add-cap-keys branch from 8583817 to 1e2e69b Compare August 14, 2023 02:07

alimaredia added 2 commits August 14, 2023 00:26

SQUASH-ME: changes caps in cap deletion test

c95ba0b

Signed-off-by: Ali Maredia <amaredia@redhat.com>

REMOVE-ME: comment out cap delete dashboard test

444f3a8

Signed-off-by: Ali Maredia <amaredia@redhat.com>

github-actions bot added the needs-rebase label Sep 7, 2023

github-actions bot added the stale label Nov 18, 2023

github-actions bot closed this Dec 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RGW: add new cap 'keys' #50313

RGW: add new cap 'keys' #50313

alimaredia commented Feb 28, 2023 •

edited

Loading

alimaredia Feb 28, 2023

alimaredia Feb 28, 2023

cbodley Feb 28, 2023

cbodley left a comment

cbodley Feb 28, 2023

alimaredia Feb 28, 2023

cbodley Feb 28, 2023

alimaredia Mar 1, 2023

cbodley Mar 2, 2023

cbodley commented Mar 2, 2023

alimaredia commented Apr 10, 2023

alimaredia commented Apr 10, 2023

alimaredia commented Apr 10, 2023

alimaredia commented Apr 11, 2023

alimaredia commented Apr 11, 2023

cbodley commented Apr 12, 2023

github-actions bot commented Apr 12, 2023

cbodley Aug 11, 2023

github-actions bot commented Sep 7, 2023

alimaredia commented Sep 19, 2023

alimaredia commented Sep 19, 2023

github-actions bot commented Nov 18, 2023

github-actions bot commented Dec 18, 2023

RGW: add new cap 'keys' #50313

RGW: add new cap 'keys' #50313

Conversation

alimaredia commented Feb 28, 2023 • edited Loading

Contribution Guidelines

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbodley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbodley commented Mar 2, 2023

alimaredia commented Apr 10, 2023

alimaredia commented Apr 10, 2023

alimaredia commented Apr 10, 2023

alimaredia commented Apr 11, 2023

alimaredia commented Apr 11, 2023

cbodley commented Apr 12, 2023

github-actions bot commented Apr 12, 2023

Choose a reason for hiding this comment

github-actions bot commented Sep 7, 2023

alimaredia commented Sep 19, 2023

alimaredia commented Sep 19, 2023

github-actions bot commented Nov 18, 2023

github-actions bot commented Dec 18, 2023

alimaredia commented Feb 28, 2023 •

edited

Loading