Skip to content

Add support for specifying account aliases in the allowed_accounts stack definition property#325

Merged
petervandoros merged 13 commits intomasterfrom
allowed-account-aliases
Apr 2, 2020
Merged

Add support for specifying account aliases in the allowed_accounts stack definition property#325
petervandoros merged 13 commits intomasterfrom
allowed-account-aliases

Conversation

@petervandoros
Copy link
Copy Markdown
Contributor

Add support for specifying account aliases in the allowed_accounts stack definition property.

Copy link
Copy Markdown
Contributor

@stevehodgkiss stevehodgkiss left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Copy Markdown
Member

@orien orien left a comment

Choose a reason for hiding this comment

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

👏

Nice one.

| stack_id | event_id | stack_name | logical_resource_id | resource_status | resource_type | timestamp |
| 1 | 1 | myapp-db | myapp-db | CREATE_COMPLETE | AWS::CloudFormation::Stack | 2020-10-29 00:00:00 |
When I use the account "11111111" with alias "an-account-alias"
And I run `stack_master apply us-east-1 myapp-db`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be testing the stack with account alias?

Suggested change
And I run `stack_master apply us-east-1 myapp-db`
And I run `stack_master apply us-east-1 myapp-cache`

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in bb773ab.

Comment on lines +66 to +67
And the output should match /2020-10-29 00:00:00 (\+|\-)[0-9]{4} myapp-cache AWS::CloudFormation::Stack CREATE_COMPLETE/
Then the exit status should be 0
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should use Then for lines with assertions:

Suggested change
And the output should match /2020-10-29 00:00:00 (\+|\-)[0-9]{4} myapp-cache AWS::CloudFormation::Stack CREATE_COMPLETE/
Then the exit status should be 0
Then the output should match /2020-10-29 00:00:00 (\+|\-)[0-9]{4} myapp-cache AWS::CloudFormation::Stack CREATE_COMPLETE/
And the exit status should be 0

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 6061e54.

class Identity
def running_in_allowed_account?(allowed_accounts)
allowed_accounts.nil? || allowed_accounts.empty? || allowed_accounts.include?(account)
def running_in_account?(accounts)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

❤️


private
def account_aliases
@aliases ||= iam.list_account_aliases.account_aliases
Copy link
Copy Markdown
Member

@orien orien Mar 31, 2020

Choose a reason for hiding this comment

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

With the addition of this new API call, I'm assuming StackMaster requires additional permissions. Perhaps we should document this in the changelog.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added in 1fc44b7.

Copy link
Copy Markdown
Contributor

@patrobinson patrobinson left a comment

Choose a reason for hiding this comment

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

I personally would prefer using IDs, I know they're not meaningful but they never change and you can't deny permission for any user/role to call get-caller-identity

But 👍 nonetheless

end

def contains_account_alias?(aliases)
account_aliases.any? { |account_alias| aliases.include?(account_alias) }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Perhaps this should only be checked (and the API call made) if the provided values look like aliases, ie. they're not numbers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought about this too, but the problem with this approach is that it only covers the case where the check is performed, not when the error message is being generated in the cli.rb file.

I think the way to cover all cases is to rescue the permissions error in the account_aliases method and hard fail with a useful error message.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 Let's do that.

@petervandoros
Copy link
Copy Markdown
Contributor Author

@patrobinson @orien I added a useful error message when the iam:ListAccountAliases permission is missing in 8a528b7.

Please have a look 😄 .

@petervandoros petervandoros merged commit cbc3fa5 into master Apr 2, 2020
@orien orien deleted the allowed-account-aliases branch January 23, 2021 02:22
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.

4 participants