RGW/STS: honor configured limits when updating max session duration #53842

BBoozmen · 2023-10-05T14:44:17Z

Fixes https://tracker.ceph.com/issues/63109

BEFORE

Creating a role uses the default 3600 sec as the session duration.

$ ./bin/radosgw-admin -c ./ceph.conf role create --role-name=myrole --assume-role-policy-doc="{\"Statement\":[{\"Effect\":\"Allow\",\"Principal\":{\"AWS\":[\"arn:aws:iam:::user/myuser\"]},\"Action\":[\"sts:AssumeRole\"]}]}"
{
    "RoleId": "454b3f8d-3230-457f-9cc2-bb8811382a9c",
    "RoleName": "myrole",
    "Path": "/",
    "Arn": "arn:aws:iam:::role/myrole",
    "CreateDate": "2023-10-04T02:23:57.110Z",
    "MaxSessionDuration": 3600,
    "AssumeRolePolicyDocument": "{\"Statement\":[{\"Effect\":\"Allow\",\"Principal\":{\"AWS\":[\"arn:aws:iam:::user/myuser\"]},\"Action\":[\"sts:AssumeRole\"]}]}"
}

User can set a higher than allowed limit

Although hard-coded (and configured) limit is set to 43200, a user can set the limit higher than that:

$ ./bin/radosgw-admin -c ./ceph.conf role update  --role-name=myrole  --max_session_duration=100000
Max session duration updated successfully for role: myrole

Subsequent updates over that limit fails, though but user already breached the limit. The problem is role-creation and role-update uses the same validation codepath and role-update wrongly uses current session duration value (3600 in this case) rather than the new value (100K) for validation.

$ ./bin/radosgw-admin -c ./ceph.conf role update  --role-name=myrole  --max_session_duration=99000
2023-10-03T22:27:23.462-0400 7fb27678c780  0 ERROR: Invalid session duration, should be between 900 and 43200 seconds

Role creation doesn't honor --max_session_duration option

"role creation" doesn't honor the option --max_session_duration when it's valid:

$ ./bin/radosgw-admin -c ./ceph.conf role create  --role-name=myrole --max-session-duration=43200 --path=/ --assume-role-policy-doc="{\"Statement\":[{\"Effect\":\"Allow\",\"Principal\":{\"AWS\":[\"arn:aws:iam:::user/myuser1\"]},\"Action\":[\"sts:AssumeRole\"]}]}"
{
    "RoleId": "015ef813-5c6b-47c4-98c7-5926bef7e356",
    "RoleName": "myrole",
    "Path": "/",
    "Arn": "arn:aws:iam:::role/myrole",
    "CreateDate": "2023-10-07T01:39:41.379Z",
    "MaxSessionDuration": 3600,
    "AssumeRolePolicyDocument": "{\"Statement\":[{\"Effect\":\"Allow\",\"Principal\":{\"AWS\":[\"arn:aws:iam:::user/myuser1\"]},\"Action\":[\"sts:AssumeRole\"]}]}"
}

Although "43200" is given, role still uses the default "3600".

AFTER

With the fix:

$ ./bin/radosgw-admin -c ./ceph.conf role create --role-name=myrole --assume-role-policy-doc="{\"Statement\":[{\"Effect\":\"Allow\",\"Principal\":{\"AWS\":[\"arn:aws:iam:::user/myuser\"]},\"Action\":[\"sts:AssumeRole\"]}]}"
{
    "RoleId": "bc660f5b-1d39-4c90-ace2-e576536f1a3d",
    "RoleName": "myrole",
    "Path": "/",
    "Arn": "arn:aws:iam:::role/myrole",
    "CreateDate": "2023-10-04T03:25:31.797Z",
    "MaxSessionDuration": 3600,
    "AssumeRolePolicyDocument": "{\"Statement\":[{\"Effect\":\"Allow\",\"Principal\":{\"AWS\":[\"arn:aws:iam:::user/myuser\"]},\"Action\":[\"sts:AssumeRole\"]}]}"
}

We now don't allow going over the limit.

$ ./bin/radosgw-admin -c ./ceph.conf role update  --role-name=myrole  --max_session_duration=100000
2023-10-03T23:25:47.425-0400 7faa6e886780  0 ERROR: Invalid session duration 100000, should be between 900 and 43200 seconds

Moreover, we can also now honor the max_session_duration option when creating the role:

]$ ./bin/radosgw-admin -c ./ceph.conf role create  --role-name=myrole --max-session-duration=100000 --path=/ --assume-role-policy-doc="{\"Statement\":[{\"Effect\":\"Allow\",\"Principal\":{\"AWS\":[\"arn:aws:iam:::user/myuser1\"]},\"Action\":[\"sts:AssumeRole\"]}]}"
2023-10-06T21:38:44.012-0400 7f1a77cb7780  0 ERROR: Invalid session duration, should be between 3600 and 43200 seconds

$ ./bin/radosgw-admin -c ./ceph.conf role create  --role-name=myrole --max-session-duration=43200 --path=/ --assume-role-policy-doc="{\"Statement\":[{\"Effect\":\"Allow\",\"Principal\":{\"AWS\":[\"arn:aws:iam:::user/myuser1\"]},\"Action\":[\"sts:AssumeRole\"]}]}"
{
    "RoleId": "015ef813-5c6b-47c4-98c7-5926bef7e356",
    "RoleName": "myrole",
    "Path": "/",
    "Arn": "arn:aws:iam:::role/myrole",
    "CreateDate": "2023-10-07T01:39:41.379Z",
    "MaxSessionDuration": 43200,
    "AssumeRolePolicyDocument": "{\"Statement\":[{\"Effect\":\"Allow\",\"Principal\":{\"AWS\":[\"arn:aws:iam:::user/myuser1\"]},\"Action\":[\"sts:AssumeRole\"]}]}"

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

pritha-srivastava · 2023-10-06T08:14:03Z

@BBoozmen : Please refer to my comment in the tracker.

pritha-srivastava · 2023-10-06T08:19:18Z

src/rgw/rgw_admin.cc

@@ -6910,7 +6910,7 @@ int main(int argc, const char **argv)
      if (ret < 0) {
        return -ret;
      }
-      if (!role->validate_max_session_duration(dpp())) {
+      if (!role->validate_max_session_duration(dpp(), std::stoull(max_session_duration))) {


The correct way to validate the value of max session duration here is to reverse the order of calls, like the following:

role->update_max_session_duration(max_session_duration); if (!role->validate_max_session_duration(dpp())) { ret = -EINVAL; return ret; }

Thank you @pritha-srivastava for the clarification. We had the wrong impression that the knob can be used to increase higher session-durations than 43200.

I've added the new evidence. It now addresses 2 things:

don't allow "role update" to go over the limit.

let "role create" honor the max-session-duration option.

Also, revised the long_desc of the knob to provide some more clarification.

Please have a look.

pritha-srivastava · 2023-10-09T05:34:37Z

src/rgw/rgw_admin.cc

@@ -6701,6 +6701,7 @@ int main(int argc, const char **argv)
        return -EINVAL;
      }
      std::unique_ptr<rgw::sal::RGWRole> role = driver->get_role(role_name, tenant, path, assume_role_doc);
+      role->update_max_session_duration(max_session_duration);


This will work, but please pass the max_session_duration param in the get_role() call above, something like the following:
std::unique_ptr<rgw::sal::RGWRole> role = driver->get_role(role_name, tenant, path, assume_role_doc, max_session_duration, {});
update_max_session_duration() was meant to update the existing max_session_duration.

That's a good point @pritha-srivastava. Added a new evidence.

pritha-srivastava · 2023-10-09T05:41:11Z

src/common/options/rgw.yaml.in

@@ -3295,8 +3295,13 @@ options:
  type: uint
  level: advanced
  desc: Session token max duration
-  long_desc: Max duration in seconds for which the session token is valid.
+  long_desc: Max duration in seconds for which the session token is valid. This


This option can be used to configure the upper limit of the durationSeconds of temporary credentials returned by 'GetSessionToken'.

Can you please correct the description for rgw_sts_min_session_duration also - This option can be used to configure the lower limit of durationSeconds of temporary credentials returned by 'AssumeRole*' calls.

pritha-srivastava

looks good to me, thanks @BBoozmen , just add the tracker issue fixed to the commit messages.

…ing it If we validate before updating the role's max-session-duration, the validator function wrongly uses the on-disk (existing/old) value for validation. Note that the "role" object being updated is in-memory and only after validation passes, it's persisted on-disk. So, calling role object's update_max_session_duration API function is OK before the role->validate_max_session_duration call. validate_max_session_duration is used by both "role creation" and "role update". The latter wrongly uses existing role's max_session_duration value for validation instead of the new/target duration: $ radosgw-admin ... role create --role-name=myrole ... $ radosgw-admin ... role get --role-name=myrole | jq '.MaxSessionDuration' 3600 where 3600 seconds is the default value. $ radosgw-admin ... role update --role-name=myrole --max_session_duration=100000 Max session duration updated successfully for role: myrole Although above update call should have failed since 100K is higher than 43200 (the default max), it succeeded. $ radosgw-admin ... role get --role-name=myrole | jq '.MaxSessionDuration' 100000 Fixes: https://tracker.ceph.com/issues/63109 Signed-off-by: Oguzhan Ozmen <oozmen@bloomberg.net>

Currently, this option is not honored and the default (3600s) is used regardless of whether this option is provided when creating a role: $ radosgw-admin role create --role-name=myrole --max-session-duration=43200 ... $ radosgw-admin role get --role-name=myrole | jq '.MaxSessionDuration' 3600 With this commit, the value given by the --max-session-duration is considered when creating the role. This would reduce the need for updating the role's max-session-duration using a separate "role update" radosgw-admin command call after the role is created: $ radosgw-admin role create --role-name=myrole --max-session-duration=43200 ... $ radosgw-admin role get --role-name=myrole | jq '.MaxSessionDuration' 43200 Signed-off-by: Oguzhan Ozmen <oozmen@bloomberg.net>

…on_duration Fixes: https://tracker.ceph.com/issues/63109 Signed-off-by: Oguzhan Ozmen <oozmen@bloomberg.net>

cbodley · 2023-11-06T15:43:04Z

passed qa after rerun in https://pulpito.ceph.com/cbodley-2023-11-03_18:34:54-rgw-wip-cbodley-testing-distro-default-smithi/

BBoozmen requested a review from a team as a code owner October 5, 2023 14:44

github-actions bot added the rgw label Oct 5, 2023

mattbenjamin requested a review from pritha-srivastava October 5, 2023 14:50

pritha-srivastava reviewed Oct 6, 2023

View reviewed changes

BBoozmen force-pushed the oozmen_sts_max_duration branch from d4758b4 to c60c1d6 Compare October 7, 2023 02:13

github-actions bot added the common label Oct 7, 2023

BBoozmen force-pushed the oozmen_sts_max_duration branch from c60c1d6 to 48b72a1 Compare October 7, 2023 02:24

pritha-srivastava reviewed Oct 9, 2023

View reviewed changes

BBoozmen force-pushed the oozmen_sts_max_duration branch from 48b72a1 to 2a3f6b4 Compare October 9, 2023 18:38

pritha-srivastava approved these changes Oct 10, 2023

View reviewed changes

pritha-srivastava added the needs-test label Oct 10, 2023

BBoozmen force-pushed the oozmen_sts_max_duration branch from 2a3f6b4 to 62b08de Compare October 10, 2023 13:54

pritha-srivastava added needs-qa and removed needs-test labels Oct 10, 2023

BBoozmen added 3 commits October 20, 2023 16:07

RGW/Roles: add more details to the cfg items rgw_sts_{max, min}_sessi…

e5abfff

…on_duration Fixes: https://tracker.ceph.com/issues/63109 Signed-off-by: Oguzhan Ozmen <oozmen@bloomberg.net>

BBoozmen force-pushed the oozmen_sts_max_duration branch from 62b08de to e5abfff Compare October 20, 2023 20:08

cbodley added the wip-cbodley-testing label Nov 1, 2023

cbodley merged commit fd72776 into ceph:main Nov 6, 2023
10 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RGW/STS: honor configured limits when updating max session duration #53842

RGW/STS: honor configured limits when updating max session duration #53842

BBoozmen commented Oct 5, 2023 •

edited

pritha-srivastava commented Oct 6, 2023

pritha-srivastava Oct 6, 2023

BBoozmen Oct 7, 2023

pritha-srivastava Oct 9, 2023 •

edited

BBoozmen Oct 9, 2023

pritha-srivastava Oct 9, 2023 •

edited

pritha-srivastava Oct 9, 2023

BBoozmen Oct 9, 2023

pritha-srivastava left a comment •

edited

cbodley commented Nov 6, 2023

RGW/STS: honor configured limits when updating max session duration #53842

RGW/STS: honor configured limits when updating max session duration #53842

Conversation

BBoozmen commented Oct 5, 2023 • edited

BEFORE

User can set a higher than allowed limit

Role creation doesn't honor --max_session_duration option

AFTER

Contribution Guidelines

Checklist

pritha-srivastava commented Oct 6, 2023

pritha-srivastava Oct 6, 2023

Choose a reason for hiding this comment

BBoozmen Oct 7, 2023

Choose a reason for hiding this comment

pritha-srivastava Oct 9, 2023 • edited

Choose a reason for hiding this comment

BBoozmen Oct 9, 2023

Choose a reason for hiding this comment

pritha-srivastava Oct 9, 2023 • edited

Choose a reason for hiding this comment

pritha-srivastava Oct 9, 2023

Choose a reason for hiding this comment

BBoozmen Oct 9, 2023

Choose a reason for hiding this comment

pritha-srivastava left a comment • edited

Choose a reason for hiding this comment

cbodley commented Nov 6, 2023

BBoozmen commented Oct 5, 2023 •

edited

pritha-srivastava Oct 9, 2023 •

edited

pritha-srivastava Oct 9, 2023 •

edited

pritha-srivastava left a comment •

edited