Skip to content
This repository was archived by the owner on Oct 4, 2024. It is now read-only.

Granular IAM permissions #26

Merged
merged 7 commits into from
Aug 19, 2021
Merged

Conversation

wellsiau-aws
Copy link
Contributor

@wellsiau-aws wellsiau-aws commented Aug 7, 2021

Issue #, if available:

  • Typo in CloudWatch rules for lifecycle event

  • Existing IAM managed policy does not utilize granular resource permissions.

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Contributor

@sshvans sshvans left a comment

Choose a reason for hiding this comment

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

See comments below.

RoleName: !Join ['_', ['NewRelicIntegrationRole', !Ref NewRelicAccountNumber]]
AssumeRolePolicyDocument:
Version: 2012-10-17
Statement:
- Effect: Allow
Principal:
AWS: 'arn:aws:iam::754728514883:root'
AWS: !Sub 'arn:aws:iam::${NewRelicTrustedPrincipal}:root'
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this needs to be 'root'? or can it be a different user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since NewRelic AWS account may use different role to perform STS, we need to assign it to root to allow IAM users/roles with proper external ID to assume it.

@@ -60,13 +64,14 @@ Resources:
Statement:
- Effect: Allow
Principal:
AWS: 'arn:aws:iam::754728514883:root'
AWS: !Sub 'arn:aws:iam::${NewRelicTrustedPrincipal}:root'
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since NewRelic AWS account may use different role to perform STS, we need to assign it to root to allow IAM users/roles with proper external ID to assume it.

@@ -16,6 +16,10 @@ Parameters:
is created using the name you specify. This custom policy includes only minimal permissions
that allow New Relic to monitor your Lambda functions. Note that you are responsible for
managing a custom policy.'
NewRelicTrustedPrincipal:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the value we are passing from the main launch template?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this value is not passed from main launch template at the moment, default value is used during launch

Copy link
Contributor

Choose a reason for hiding this comment

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

If we are not passing it from the main template than I don't see the value of add it as parameter. Idea behind parameterizing it was to allow users to explicitly provide a 3rd party account id which they are giving permission to take actions in their AWS Account.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed and I've document it here #27

this will require another PR to modify the documentation, and perhaps we will also cover different partition, i.e. govcloud if needed.

I added this param on newrelic-stack-set.yml as stop gap from hard-coded principal in the template.

Copy link
Contributor

Choose a reason for hiding this comment

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

When do you think we will have the PR to address it?

@sshvans sshvans merged commit 1fef76e into aws-quickstart:develop Aug 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants