Skip to content

Support the wildcard principal ("*") in STS role config #8257

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 10, 2024
Merged

Support the wildcard principal ("*") in STS role config #8257

merged 1 commit into from
Sep 10, 2024

Conversation

Neon-White
Copy link
Contributor

@Neon-White Neon-White commented Aug 5, 2024 •
edited
Loading

Explain the changes

  1. Currently, if a user sets the principal which can assume a role via STS as "", it always fails since it compares "" to the email of the requester. This PR checks whether the policy principle is "*" and allows it

Testing Instructions:

  1. Create two NooBaa accounts - 'assumed' and 'assumer'
  2. Assign a role config to assumed that allows anyone ("principal": ["*"]) to assume it
  3. Try to assume it with the credentials of assumer
  4. Test the received credentials
  • Doc added/updated
  • Tests added

@Neon-White Neon-White requested a review from romayalon August 5, 2024 12:43
@Neon-White Neon-White force-pushed the support-any-sts-principal branch from 662ef1d to 7548ab2 Compare August 5, 2024 15:28
@Neon-White Neon-White changed the title Support any principal ("*") in STS role config Support the wildcard principal ("*") in STS role config Aug 5, 2024
shirady
shirady reviewed Aug 6, 2024
View reviewed changes
src/endpoint/sts/sts_rest.js
@@ -216,7 +216,7 @@ function _is_statements_fit(statements, method, cur_account_email) {
// who can do that action
for (const principal of statement.principal) {
dbg.log0('assume_role_policy: principal fit?', principal.unwrap().toString(), cur_account_email);
if (principal.unwrap() === cur_account_email) {
if ((principal.unwrap() === cur_account_email) || (principal.unwrap() === '*')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why STS doesn't use the code in s3_bucket_policy_utils?
It seems that this fix is already there...

noobaa-core/src/endpoint/s3/s3_bucket_policy_utils.js

Lines 169 to 173 in 3882f49

for (const principal of _.flatten([statement_principal])) {
dbg.log1('bucket_policy: ', statement.Principal ? 'Principal' : 'NotPrincipal', ' fit?', principal, account);
if ((principal.unwrap() === '*') || (principal.unwrap() === account)) {
principal_fit = true;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One difference I found is that the STS checks check for action === 'sts:*', which the S3 bucket utils don't, and rightfully so.

@romayalon - do you remember why you reimplemented the checks instead of importing them? Any additional reasons?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shirady this is out of this PR scope
Could you open a GH issue for it?

@romayalon
Copy link
Contributor

romayalon commented Aug 8, 2024 •
edited
Loading

Hey @Neon-White

  1. Regarding your question here - Support the wildcard principal ("*") in STS role config #8257 (comment) - The check of "*" removal seems to be on purpose IMO, but I don't remember, it's been more than 3 years...
  2. Looks like the whole assume_bucket_policy code is very outdated, @nadavMiz completely transformed the S3 bucket policy with updated NotPrinciple, Conditions etc, when do we plan to do that for STS?
  3. Please add appropriate unit tests.
  4. We need to also remove STS tests from the black list of ceph tests, did you try to run them while working on enabling STS? Do you know what is the status there?
  5. Is this bug coming from QE? if so, can you please add it to the PR description?

@romayalon
Copy link
Contributor

romayalon commented Aug 13, 2024 •
edited
Loading

Adding @liranmauda @nimrod-becker for assigning #8257 (comment)
As it's not a part of this PR, but we do want to -

  1. Apply the new bucket policy structure to STS.
  2. Enable STS Ceph tests.

liranmauda
liranmauda previously approved these changes Aug 13, 2024
View reviewed changes
src/endpoint/sts/sts_rest.js
@@ -216,7 +216,7 @@ function _is_statements_fit(statements, method, cur_account_email) {
// who can do that action
for (const principal of statement.principal) {
dbg.log0('assume_role_policy: principal fit?', principal.unwrap().toString(), cur_account_email);
if (principal.unwrap() === cur_account_email) {
if ((principal.unwrap() === cur_account_email) || (principal.unwrap() === '*')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shirady this is out of this PR scope
Could you open a GH issue for it?

@Neon-White Neon-White force-pushed the support-any-sts-principal branch from 7548ab2 to 13f0093 Compare August 13, 2024 11:01
@pull-request-size pull-request-size bot added size/L and removed size/XS labels Sep 4, 2024
@Neon-White Neon-White marked this pull request as draft September 4, 2024 13:13
@Neon-White Neon-White changed the title Support the wildcard principal ("*") in STS role config Refactor S3 and STS to utilize a unified has_policy_permission function Sep 4, 2024
@Neon-White Neon-White force-pushed the support-any-sts-principal branch from 2f0ad5f to 27df4f7 Compare September 5, 2024 14:54
@pull-request-size pull-request-size bot added size/XS and removed size/L labels Sep 9, 2024
@Neon-White Neon-White changed the title Refactor S3 and STS to utilize a unified has_policy_permission function Support the wildcard principal ("*") in STS role config Sep 9, 2024
@Neon-White Neon-White marked this pull request as ready for review September 9, 2024 10:05
@Neon-White Neon-White force-pushed the support-any-sts-principal branch from 4607118 to 8a1d91d Compare September 10, 2024 12:56
@Neon-White Neon-White merged commit 8961f1d into noobaa:master Sep 10, 2024
10 checks passed
@Neon-White Neon-White deleted the support-any-sts-principal branch September 10, 2024 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liranmauda liranmauda liranmauda approved these changes

@romayalon romayalon Awaiting requested review from romayalon

@shirady shirady Awaiting requested review from shirady

Assignees
No one assigned
Labels
size/XS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Neon-White @romayalon @liranmauda @shirady