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

Remove access without constraints on the pivotRole #875

Closed
zsaltys opened this issue Nov 16, 2023 · 6 comments
Closed

Remove access without constraints on the pivotRole #875

zsaltys opened this issue Nov 16, 2023 · 6 comments

Comments

@zsaltys
Copy link
Contributor

zsaltys commented Nov 16, 2023

The pivot role installed on AWS accounts has policies which are picked up by Checkov scanner where the role has unrestricted access. By unrestricted I mean **granting actions on resource ''*.

This is the list of these actions picked up:

CheckID		: CKV_AWS_111
CheckName	: Ensure IAM policies does not allow write access without constraints
File		: /deploy/pivot_role/pivotRole.yaml:49-218
Resource	: AWS::IAM::ManagedPolicy.PivotRolePolicy0
Guideline	: CKV_AWS_111 
CheckID		: CKV_AWS_111
CheckName	: Ensure IAM policies does not allow write access without constraints
File		: /deploy/pivot_role/pivotRole.yaml:220-338
Resource	: AWS::IAM::ManagedPolicy.PivotRolePolicy1
Guideline	: CKV_AWS_111 

Expected resolution

Please ensure that the pivotRole never grants an action which is completely unrestricted. Ideally everything should be restricted by asking for dataall prefix OR if it is an imported resource then it has to be solved in a way where the imported resource is mentioned specifically.

@noah-paige
Copy link
Contributor

Thanks for raising this issue @zsaltys. Data.all supports 2 ways of creating the pivotRole, via CloudFormation before linking Environment, or via CDK when linking Environment. Some of the pain points you referenced above with imported resources has been resolved with the CDK way of creating the pivotRole but may not be fully addressed with the CloudFormation way since it is a manual creation via upload stack to CloudFormation (this is why we suggest to use CDK to create PivotRole as best practice)

We should further evaluate if scanners are reporting any issues for the CDK pivot role creation and also see what ways we can restrict both pivotRole policies further - to take a look at this when some capacity frees up from the team

@zsaltys
Copy link
Contributor Author

zsaltys commented Jan 26, 2024

@noah-paige let's use this ticket for the purpose of removing unconstrained access on the pivotRole installed by data.all. As an example I'll use this statement from the pivot role policies:

{
       "Action": [
        "iam:PutRolePolicy",
        "iam:DeleteRolePolicy"
       ],
       "Effect": "Allow",
       "Resource": "*",
       "Sid": "IAMRolePolicy"
      },

This allows the pivot role to modify any policy. I as owner of data.all on the infra account can easily write code to assume the pivot role through the graphql role and then effectively I can modify any policy on any account... Effectively if the pivotRole is ever compromised it gives attackers access to resources which were never created by data.all or imported to data.all. When we onboard users to data.all and they see such roles installed on their accounts it really scares them.

As general rule we need to make sure that pivotRole only has access to things that data.all itself created or things that were imported to data.all like kms keys, buckets, consumer roles.. and nothing else.

@mourya-33
Copy link
Contributor

Created the PR - #1075 to restrict logging, and cloudformation stacks. The remaining exceptions will be taken up in a different ticket.

noah-paige added a commit that referenced this issue Mar 4, 2024
### Feature or Bugfix
- Bugfix

### Detail
This PR is to restrict few overly permissive policy permissions on the
auto created Pivot Role

### Relates
[- <URL or Ticket>](#875)

### 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)? No
  - Is the input sanitized? N/A
- What precautions are you taking before deserializing the data you
consume? N/A
  - Is injection prevented by parametrizing queries?N/A
  - Have you ensured no `eval` or similar functions are used?N/A
- Does this PR introduce any functionality or component that requires
authorization?N/A
- How have you ensured it respects the existing AuthN/AuthZ
mechanisms?N/A
  - Are you logging failed auth attempts?N/A
- Are you using or adding any cryptographic features?N/A
  - Do you use a standard proven implementations?Yes
- Are the used keys controlled by the customer? Where are they
stored?N/A
- Are you introducing any new policies/roles/users?N/A
  - Have you used the least-privilege principle? How?N/A


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>
@mourya-33
Copy link
Contributor

The pending items that are not fixed in this ticket are tracked here

#1195 - RAM permissions
#1189 - Glue and KMS permissions

@mourya-33
Copy link
Contributor

@zsaltys i updated the ticket with the details on pending items and their corresponding tickets. Can we close this?

@zsaltys
Copy link
Contributor Author

zsaltys commented Jun 6, 2024

@mourya-33 thanks

@zsaltys zsaltys closed this as completed Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants