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 cdk custom execution policy #876

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

Comments

@zsaltys
Copy link
Contributor

zsaltys commented Nov 16, 2023

The cdk custom execution policy installed on AWS accounts has policies which are picked up by Checkov scanner where the policy 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/cdk_exec_policy/cdkExecPolicy.yaml:12-269
Resource	: AWS::IAM::ManagedPolicy.CDKCustomExecutionPolicy0
Guideline	: CKV_AWS_111 

Expected resolution

Please ensure that the cdk execution policy never grants an action which is completely unrestricted. Ideally everything should be restricted by asking for dataall prefix.

@noah-paige
Copy link
Contributor

Thanks for raising this issue @zsaltys, I think we need to evaluate how best to format this CDK Execution Policy to be as strict as possible while also not impacting any of the functionality of data.all. For now we provide this execution policy as an alternative to the default AdministratorAccess policy that CDK will use to deploy infra, but there is still room to optimize on the above

We will do some further investigation on the above and report back with our findings

@zsaltys
Copy link
Contributor Author

zsaltys commented Jan 26, 2024

I was looking a bit more into this. Effectively the CDK policy gets attached to cdk-exec-role which is used by cloudformation only. This logically tells us that this is will only be used to manage resources that data.all itself created.. It would never use these permissions on existing resources.. However!

In theory it is still possible to run lambdas in CloudFormation as custom resources so technically you COULD delete existing roles or modify existing roles not created by data.all. When we ask users to onboard data.all when they see permissions like this:

- Sid: IAM
        Effect: Allow
        Action:
          - 'iam:CreatePolicy*'
          - 'iam:DeletePolicy*'
          - 'iam:DetachRolePolicy'
          - 'iam:DeleteRole'
          - 'iam:CreateRole'
          - 'iam:DeleteRolePolicy'
          - 'iam:*Tag*'
          - 'iam:PassRole'
          - 'iam:AttachRolePolicy'
          - 'iam:*ServiceLinkedRole'
          - 'iam:Get*'
          - 'iam:List*'
          - 'iam:UpdateAssumeRolePolicy'
          - 'iam:PutRolePolicy'
        Resource:
          - !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:policy/*'
          - !Sub 'arn:${AWS::Partition}:iam::${AWS::AccountId}:role/*'

It scares the daylights out of them. Id rather avoid the explaining that it is very unlikely that anyone could use this role to do anything to their existing resources. Logically it seems to me it should be entirely possible to require that everything in this policy uses a resource prefix because all of the resources managed by CDK-exec-role should have been created by data.all itself.

noah-paige added a commit that referenced this issue Apr 5, 2024
)

### Feature or Bugfix
BugFix

### Detail
This PR is to address overly excessive permissions for cdk custom
execution policy for linking accounts that are reported by checkov.

### Relates
#876

### 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? 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? N/A
- 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? Yes. Restricted
overly permissive permissions to required resources.


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>
@zsaltys
Copy link
Contributor Author

zsaltys commented Jun 6, 2024

@mourya-33 can we close this ? does checkov still complain on this ?

@mourya-33
Copy link
Contributor

This can be closed as well @zsaltys

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

4 participants