Skip to content
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.

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

922 - Replace IAM inline policies by configurable Managed Policies for folder and bucket sharing #1068

Merged
merged 61 commits into from
Mar 4, 2024

Conversation

SofiaSazonova
Copy link
Contributor

@SofiaSazonova SofiaSazonova commented Feb 21, 2024

Feature or Bugfix

  • Feature

Detail

  • For each consumption role and group role, invited to the env 1 managed policy is created
  • User can choose, if the consumption role is data.all managed or not
  • If data.all managed, the created policy is automatically attached to role
  • without this policy attached user can not create a share for this consumption role
  • policy attachment and managed options are displayed in Env->Teams
  • bucket and accesspoint shares are managed through this managed policies

Relates

SecurityN/A

How to test:
See backwards compatibility and local testing comments below.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dlpzx Update:

  • ⚠️ When upgrading customers need to update their environment stacks to update the pivot role if created through CDK. If the pivot role is manually created, they need to update its permissions manually with the latest version
  • ⚠️ For backwards compatibility: for all existing roles both created and imported by data.all. The share-policy will be created in any of the next 3 scenarios:
    a) if a new share request is created
    b) if new items are added to an existing share request
    c) if items are revoked from an existing share request

More details in the comments

Sofia Sazonova added 27 commits January 30, 2024 12:06
+ frontend adjustments
add frontend fields for consumption roles
Frontend display of share policies
@SofiaSazonova
Copy link
Contributor Author

SofiaSazonova commented Feb 22, 2024

ToDoS:
Frontend:

  1. 'Copy' icon in the Consumption role tables now inherits the palette (light|dark theme). In dark theme it appears orange, but it should be white as in the table above
  2. Copy no only the Policy name, but code-snippets for IaC
  3. Tooltips or/and explanations, what "customer/data.all managed" means
  4. 'Submit request' model window is now ugly with scrollbar

BackEnd:

  1. More elegant way to store 'empty' permissions. Right now it's implemented with
    fake_statement = "arn:aws:s3:::initial-fake-empty-bucket"

.checkov.baseline Outdated Show resolved Hide resolved
@dlpzx
Copy link
Contributor

dlpzx commented Mar 4, 2024

Local testing [ With latest changes Mon March 4th]

Newly added consumption roles/teams

Error found if share fails - after more than one policy version the policy cannot be deleted: An error occurred (DeleteConflict) when calling the DeletePolicy operation: This policy has more than one version. Before you delete a policy, you must delete the policy's versions. The default version is deleted with the policy ---> explained in docs --> fixed in commit

1.a Environment Group Policy - IAM role created by data.all

  • Invite group to environment -> IAM policy for shares is created with fake resources (update: with 2 statements one for access points and the other for buckets). Role is created in the environment stack where the policy is attached to role.
  • Create Share request -> it creates successfully. Policy is still attached to role.
  • Add folders and bucket, process Share request (Dataset with KMS key)-> IAM policy is updated with dataset, fake resources are removed from policy. there are 4 statements. Policy is still attached to role.
  • Revoke folder of Share request -> it removes statements for access points, the statements for S3 Bucket remain. Check that poicy has only one version (default) which is the number 4.
  • Revoke bucket of Share request -> IAM policy is updated and s3 resources are removed. Policy is still valid and contains fake resources for BucketSharing and AccessPoint sharing. Policy is still attached to role.
  • Remove group from environment -> IAM policy for shares is deleted
  • IAM role is deleted from environment stack safely

1.b Environment Group Policy - IAM role created by data.all as ENVIRONMENT Admin
In this test use a resource prefix different from the standard dataall

  • Create new environment -> IAM policy for shares is created with fake resources (update: with 2 statements one for access points and the other for buckets). Role is created in the environment stack where the policy is attached to role. -> Issue found 🟥 explained below --> in this case, the role is fully managed by data.all. End user is not impacted. TESTED that environment creation succeeds but it does not create the policy
  • Update environment - > right after the creation, we click on update environment, the policy is created with empty resources ---> ⚠️ see comments below, we have discarded policy creation in environment update in favor of create/process-share
  • Update environment a second time -> policy is attached ---> ⚠️ see comments below, we have discarded policy creation in environment update in favor of create/process-share

2.a Environment Group Policy - IAM role IMPORTED (sharing tested in other scenarios)

  • Invite group to environment importing role -> IAM policy for shares is created with fake resources. Policy is attached to role.
  • Create Share request -> it creates successfully. Policy is still attached to role.
  • Manually delete policy (for a manual user that by mistake deleted the policy) and Remove group from environment -> IAM policy for shares is deleted and removed from role

2.b Environment Group Policy - IAM role IMPORTED as ENVIRONMENT Admin

  • Create new environment importing role -> IAM policy for shares is created with fake resources (update: with 2 statements one for access points and the other for buckets). Role is "REFERENCED" in the environment stack where the policy is attached to role. > Issue found 🟥 explained below --> in this case, the role is managed by customer and data.all. User needs to "update stack" of environment if he wants to directly use the policy after environment creation. Same as 1.b but this time we will test the workaround with creation of shares
  • The share-policy does not exist and we click on Create share -> data.all creates policy with empty statements and attaches it to the role
  1. Data.all managed consumption roles - Use dataset with SSE-S3 encryption
  • Add role to environment with "Let data.all attach policies" = True -> IAM policy for shares is created with fake resources. In Teams tab, that role is displayed as Data.all managed with attached policies. Policy is attached to the role.
  • Create Share request -> it creates successfully. Policy is still attached to role.
  • Add bucket, process Share request -> IAM policy is updated with dataset, empty policy is replaced by S3 BUCKET statement (no KMS statements). Policy is still attached to role.
  • Add folder and process Share request -> IAM policy is updated with dataset, s3 access points statement is added (no KMS statements). Policy is still attached to role.
  • Revoke folder and Bucket of Share request -> IAM policy is updated and ALL s3 resources are removed and replaced by empty policy. Policy is still attached to role.
  • Remove consumption role from environment -> IAM policy for shares is deleted
  1. Customer managed consumption roles (no need to process share again as it is the same as test2)
  • Add role to environment with "Let data.all attach policies" = False -> IAM policy for shares is created with fake resources. In Teams tab, that role is displayed as Customer managed with NO attached policies
  • Copy policy name form UI -> copies the policy name correctly mer-env-0a000q4a-share-policy-aws-quicksight-secretsmanager-role-v0
  • Create Share request with flag "Let data.all attach policies" = False-> Shows warning Selected consumption role is managed by customer, but the share policy mer-env-0a000q4a-share-policy-aws-quicksight-secretsmanager-role-v0 is not attached.Please attach it or let Data.all attach it for you. Send request is disable
  • Attach the policy manually -> Check that in the Teams tab the policy appears as attached.
  • Create Share request without flag "Let data.all attach policies" + directly in IAM manually attach the policy for sharing to the role -> Share request creates successfully.
  • Delete the previous share and remove the policy that you attached manually, Create Share request with flag "Let data.all attach policies" = True-> it creates successfully and the policy is attached to the role
  • Remove consumption role -> IAM policy is deleted. IAM role exists without the data.all sharing policy

data sharing module is disabled

  • no policies are created for any role
  • UI does not break
  • TBD

For testing in AWS add:

  • CICD pipeline completes
  • Migration successfully add consumption role dataallManaged column and backfills existing rows with True

Issues, Remarks and next steps

  • ⚠️ There is one case that is not fully solved. For the first creation of an environment the managed policies cannot be created for the admin group because the cdk pivot role that is used to create those policies does not exist until the environment stack is deployed: create environment API (where we need the pivot role to create policies) > deploy stack with pivot role.

    • The impact is that in the first creation of an environment the policy does not exist. The policy is created when a share request is created or new items are added to the share request. If the role is data.all managed the end-user should not notice any difference in usage. The only case where the customers would be affected is if they import the admin role and want to modify their own IaC with the expected policy name. We should encourage customers to add the policy with guardrails that ensure that the policy exist.
  • At the moment the empty IAM policy statement refers a non-existent AWS service. In the rare case a service called none with action null is launched we will need to think of other ways of implementing an empty IAM policy placeholder.

  • Support for the case where a user accidentally deletes the IAM policy: at the moment it is taken care by the backwards compatibility code. In the future, we will need to adapt the code and in the case the policy does not exist, we call generate_empty_policy instead of create_managed_policy_from_inline_and_delete_inline. If there were existing shares, the user will be able to verify and reapply the unhealthy shares with PR 827 share verify and reapply fix #1062

  • Some KMS statements will be shared between access points and S3 bucket sharing. I think the benefit of keeping both sharing mechanisms isolated overcomes the downside of duplication in the KMS statements

Out of scope for this PR:

  • S3 access points are granting permissions to the Bucket. I think we should restrict permissions to the access point only
  • S3 access points sharing still faces some issues with the lag between access point creation and putPolicy operation.

@dlpzx
Copy link
Contributor

dlpzx commented Mar 4, 2024

Backwards compatibility

For data.all created and imported roles, we create the share-policy and backfill the permissions in 3 situations:
a) if a new share request is created
b) if new items are added to an existing share request
c) if items are revoked from an existing share request

If for any reason the backfilling process fails, the inline policies should not be deleted (that way we can always fix the existing shares)

For data.all created roles, we make sure the policy is attached in the IaC definition in the environment stack with new updates. If the policy does not exist, the stack should not fail and the inline policies should not be deleted

⚠️ The environment stack needs to be updated because additional pivot role permissions are needed!

  1. Environment Group Policy - IAM role created by data.all - on a team that already has a share request with s3 bucket with a dataset using KMS.
  • click on update Environment stack -> the stack is updated with additional permissions to the pivot role. The role keeps the old inline policies after the stack is updated.
  • Create new share request -> creates and backfills share-policy, deletes inline policies
  • click update Environment after there is a share-policy, the IAM role is updated and contains the share-policy
  1. Imported - Environment Group Policy GRANT - on a team that already has NO share request with access points and s3 bucket with a dataset using KMS.
  • Create NEW Share request -> it creates successfully. Policy is created containing the S3 and KMS resources of the inline policies of the previous shares. Policy is attached to role (Empty policy)

  1. Data.all managed consumption roles GRANT - on a team that already has a share request with access points and s3 bucket with a dataset using KMS. 'Quicksight service role`
  • After upgrading the consumption role appears as data.all Managed and policies not attached
  • Create NEW Share request -> it creates successfully. Policy is created containing the S3 ONLY resources of the inline policies of the previous shares. Policy is attached to role.
  1. Data.all managed consumption roles ADD ITEMS - on a team that already has a share request with s3 bucket NOT ACCESS POINTS with a dataset using KMS.
  • After upgrading the consumption role appears as data.all Managed and policies not attached
  • add folders to share from the share request -> Policy is created containing the S3 ONLY resources of the inline policies of the previous shares, plus the added folders. Policy should be attached to role.
  1. Data.all managed consumption roles REVOKE - on a team that already has a share request with access points and s3 bucket with a dataset NOT using KMS. Admin
  • After upgrading the consumption role appears as data.all Managed and policies not attached
  • Revoke folders from the share request -> Policy is created containing the S3 ONLY resources of the inline policies of the previous shares, only S3 bucket. Policy should be attached to role.

Copy link
Contributor

@noah-paige noah-paige left a comment

Choose a reason for hiding this comment

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

lgtm - approving

@noah-paige noah-paige merged commit adb165b into data-dot-all:main Mar 4, 2024
9 checks passed
noah-paige added a commit that referenced this pull request Mar 8, 2024
### Feature or Bugfix
- Bugfix

### Detail
For S3 bucket shares and for S3 access point shares (folder sharing)
- In share-verify workflow add a check to verify the managed policy for
shares is attached to the target role. Log the error and raise an
unhealthy status
- In share-approve workflow add a check to verify the managed policy for
shares is attached to the target role and attach the policy if the
requester is a Group or a data.all Managed Consumption role

### Relates
- #1062 
- #1068 

### Security
Please answer the questions below briefly where applicable, or write
`N/A`. Based on
[OWASP 10](https://owasp.org/Top10/en/).

- Does this PR introduce or modify any input fields or queries - this
includes
fetching data from storage outside the application (e.g. a database, an
S3 bucket)?
  - Is the input sanitized?
- What precautions are you taking before deserializing the data you
consume?
  - Is injection prevented by parametrizing queries?
  - Have you ensured no `eval` or similar functions are used?
- Does this PR introduce any functionality or component that requires
authorization?
- How have you ensured it respects the existing AuthN/AuthZ mechanisms?
  - Are you logging failed auth attempts?
- Are you using or adding any cryptographic features?
  - Do you use a standard proven implementations?
  - Are the used keys controlled by the customer? Where are they stored?
- Are you introducing any new policies/roles/users?
  - Have you used the least-privilege principle? How?


By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.

---------

Co-authored-by: Noah Paige <noahpaig@amazon.com>
@SofiaSazonova SofiaSazonova deleted the 922-consumer-ima-role branch October 3, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consumer IAM role updates can get wiped out by CloudFormation
5 participants