Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ The format is based on [Keep a Changelog], and this project adheres to
- `stack_master apply` prints names of parameters with missing values
([#322]).

- `allowed_accounts` stack definition property supports specifying
account aliases along with account IDs ([#325]). This change requires
the `iam:ListAccountAliases` permission to work.

### Fixed

- Error assuming role when default aws region not configured in the
Expand All @@ -26,6 +30,7 @@ The format is based on [Keep a Changelog], and this project adheres to
[#322]: https://github.com/envato/stack_master/pull/322
[#323]: https://github.com/envato/stack_master/pull/323
[#324]: https://github.com/envato/stack_master/pull/324
[#325]: https://github.com/envato/stack_master/pull/325

## [2.3.0] - 2020-03-19

Expand Down
6 changes: 3 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -625,7 +625,7 @@ stacks:

## Allowed accounts

The AWS account the command is executing in can be restricted to a specific list of allowed accounts. This is useful in reducing the possibility of applying non-production changes in a production account. Each stack definition can specify the `allowed_accounts` property with an array of AWS account IDs the stack is allowed to work with.
The AWS account the command is executing in can be restricted to a specific list of allowed accounts. This is useful in reducing the possibility of applying non-production changes in a production account. Each stack definition can specify the `allowed_accounts` property with an array of AWS account IDs or aliases the stack is allowed to work with.

This is an opt-in feature which is enabled by specifying at least one account to allow.

Expand All @@ -644,7 +644,7 @@ stacks:
template: myapp_db.rb
allowed_accounts: # only allow these accounts (overrides the stack defaults)
- '1234567890'
- '9876543210'
- my-account-alias
tags:
purpose: back-end
myapp-web:
Expand All @@ -659,7 +659,7 @@ stacks:
purpose: back-end
```

In the cases where you want to bypass the account check, there is StackMaster flag `--skip-account-check` that can be used.
In the cases where you want to bypass the account check, there is the StackMaster flag `--skip-account-check` that can be used.

## Commands

Expand Down
34 changes: 28 additions & 6 deletions features/apply_with_allowed_accounts.feature
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ Feature: Apply command with allowed accounts
myapp_web:
template: myapp.rb
allowed_accounts: []
myapp_cache:
template: myapp.rb
allowed_accounts: my-account-alias
"""
And a directory named "templates"
And a file named "templates/myapp.rb" with:
Expand All @@ -32,24 +35,43 @@ Feature: Apply command with allowed accounts
| 1 | 1 | myapp-vpc | myapp-vpc | CREATE_COMPLETE | AWS::CloudFormation::Stack | 2020-10-29 00:00:00 |
When I use the account "11111111"
And I run `stack_master apply us-east-1 myapp-vpc`
And the output should match /2020-10-29 00:00:00 (\+|\-)[0-9]{4} myapp-vpc 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-vpc AWS::CloudFormation::Stack CREATE_COMPLETE/
And the exit status should be 0

Scenario: Run apply with stack overriding allowed accounts with its own list
Given I stub the following stack events:
| 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"
And I run `stack_master apply us-east-1 myapp-db`
And the output should contain all of these lines:
Then the output should contain all of these lines:
| Account '11111111' is not an allowed account. Allowed accounts are ["22222222"].|
Then the exit status should be 1
And the exit status should be 1

Scenario: Run apply with stack overriding allowed accounts to allow all accounts
Given I stub the following stack events:
| stack_id | event_id | stack_name | logical_resource_id | resource_status | resource_type | timestamp |
| 1 | 1 | myapp-web | myapp-web | CREATE_COMPLETE | AWS::CloudFormation::Stack | 2020-10-29 00:00:00 |
When I use the account "33333333"
And I run `stack_master apply us-east-1 myapp-web`
And the output should match /2020-10-29 00:00:00 (\+|\-)[0-9]{4} myapp-web 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-web AWS::CloudFormation::Stack CREATE_COMPLETE/
And the exit status should be 0

Scenario: Run apply with stack specifying allowed account alias
Given I stub the following stack events:
| stack_id | event_id | stack_name | logical_resource_id | resource_status | resource_type | timestamp |
| 1 | 1 | myapp-cache | myapp-cache | CREATE_COMPLETE | AWS::CloudFormation::Stack | 2020-10-29 00:00:00 |
When I use the account "44444444" with alias "my-account-alias"
And I run `stack_master apply us-east-1 myapp-cache`
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

Scenario: Run apply with stack specifying disallowed account alias
Given I stub the following stack events:
| stack_id | event_id | stack_name | logical_resource_id | resource_status | resource_type | timestamp |
| 1 | 1 | myapp-cache | myapp-cache | 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-cache`
Then the output should contain all of these lines:
| Account '11111111' (an-account-alias) is not an allowed account. Allowed accounts are ["my-account-alias"].|
And the exit status should be 1
16 changes: 15 additions & 1 deletion features/step_definitions/identity_steps.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
Given(/^I use the account "([^"]*)"$/) do |account_id|
Given(/^I use the account "([^"]*)"(?: with alias "([^"]*)")?$/) do |account_id, account_alias|
Aws.config[:sts] = {
stub_responses: {
get_caller_identity: {
Expand All @@ -8,4 +8,18 @@
}
}
}

if account_alias.present?
Aws.config[:iam] = {
stub_responses: {
list_account_aliases: {
account_aliases: [account_alias],
is_truncated: false
}
}
}
else
# ensure stubs don't leak between steps
Aws.config[:iam]&.delete(:stub_responses)
end
end
1 change: 1 addition & 0 deletions lib/stack_master.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
require 'aws-sdk-s3'
require 'aws-sdk-sns'
require 'aws-sdk-ssm'
require 'aws-sdk-iam'
require 'colorize'
require 'active_support/core_ext/hash/keys'
require 'active_support/core_ext/object/blank'
Expand Down
6 changes: 4 additions & 2 deletions lib/stack_master/cli.rb
Original file line number Diff line number Diff line change
Expand Up @@ -262,13 +262,15 @@ def execute_if_allowed_account(allowed_accounts, &block)
if running_in_allowed_account?(allowed_accounts)
block.call
else
StackMaster.stdout.puts "Account '#{identity.account}' is not an allowed account. Allowed accounts are #{allowed_accounts}."
account_text = "'#{identity.account}'"
account_text << " (#{identity.account_aliases.join(', ')})" if identity.account_aliases.any?
StackMaster.stdout.puts "Account #{account_text} is not an allowed account. Allowed accounts are #{allowed_accounts}."
false
end
end

def running_in_allowed_account?(allowed_accounts)
StackMaster.skip_account_check? || identity.running_in_allowed_account?(allowed_accounts)
StackMaster.skip_account_check? || identity.running_in_account?(allowed_accounts)
end

def identity
Expand Down
2 changes: 1 addition & 1 deletion lib/stack_master/commands/status.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ def sort_params(hash)
end

def running_in_allowed_account?(allowed_accounts)
StackMaster.skip_account_check? || identity.running_in_allowed_account?(allowed_accounts)
StackMaster.skip_account_check? || identity.running_in_account?(allowed_accounts)
end

def identity
Expand Down
29 changes: 25 additions & 4 deletions lib/stack_master/identity.rb
Original file line number Diff line number Diff line change
@@ -1,16 +1,25 @@
module StackMaster
class Identity
def running_in_allowed_account?(allowed_accounts)
allowed_accounts.nil? || allowed_accounts.empty? || allowed_accounts.include?(account)
MissingIamPermissionsError = Class.new(StandardError)

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.

❤️

accounts.nil? ||
accounts.empty? ||
contains_account_id?(accounts) ||
contains_account_alias?(accounts)
end

def account
@account ||= sts.get_caller_identity.account
end

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.

rescue Aws::IAM::Errors::AccessDenied
raise MissingIamPermissionsError, 'Failed to retrieve account aliases. Missing required IAM permission: iam:ListAccountAliases'
end

attr_reader :sts
private

def region
@region ||= ENV['AWS_REGION'] || Aws.config[:region] || Aws.shared_config.region || 'us-east-1'
Expand All @@ -19,5 +28,17 @@ def region
def sts
@sts ||= Aws::STS::Client.new(region: region)
end

def iam
@iam ||= Aws::IAM::Client.new(region: region)
end

def contains_account_id?(ids)
ids.include?(account)
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.

end
end
end
73 changes: 71 additions & 2 deletions spec/stack_master/identity_spec.rb
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
RSpec.describe StackMaster::Identity do
let(:sts) { Aws::STS::Client.new(stub_responses: true) }
let(:iam) { Aws::IAM::Client.new(stub_responses: true) }

subject(:identity) { StackMaster::Identity.new }

before do
allow(Aws::STS::Client).to receive(:new).and_return(sts)
allow(Aws::IAM::Client).to receive(:new).and_return(iam)
end

describe '#running_in_allowed_account?' do
describe '#running_in_account?' do
let(:account) { '1234567890' }
let(:running_in_allowed_account) { identity.running_in_allowed_account?(allowed_accounts) }
let(:running_in_allowed_account) { identity.running_in_account?(allowed_accounts) }

before do
allow(identity).to receive(:account).and_return(account)
Expand Down Expand Up @@ -45,6 +48,41 @@
expect(running_in_allowed_account).to eq(false)
end
end

describe 'with account aliases' do
let(:account_aliases) { ['allowed-account'] }

before do
iam.stub_responses(:list_account_aliases, {
account_aliases: account_aliases,
is_truncated: false
})
end

context "when it's allowed" do
let(:allowed_accounts) { ['allowed-account'] }

it 'returns true' do
expect(running_in_allowed_account).to eq(true)
end
end

context "when it's not allowed" do
let(:allowed_accounts) { ['disallowed-account'] }

it 'returns false' do
expect(running_in_allowed_account).to eq(false)
end
end

context 'with a combination of account id and alias' do
let(:allowed_accounts) { %w(1928374 allowed-account another-account) }

it 'returns true' do
expect(running_in_allowed_account).to eq(true)
end
end
end
end

describe '#account' do
Expand All @@ -60,4 +98,35 @@
expect(identity.account).to eq('account-id')
end
end

describe '#account_aliases' do
before do
iam.stub_responses(:list_account_aliases, {
account_aliases: %w(my-account new-account-name),
is_truncated: false
})
end

it 'returns the current identity account aliases' do
expect(identity.account_aliases).to eq(%w(my-account new-account-name))
end

context "when identity doesn't have the required iam permissions" do
before do
allow(iam).to receive(:list_account_aliases).and_raise(
Aws::IAM::Errors.error_class('AccessDenied').new(
an_instance_of(Seahorse::Client::RequestContext),
'User: arn:aws:sts::123456789:assumed-role/my-role/987654321000 is not authorized to perform: iam:ListAccountAliases on resource: *'
)
)
end

it 'raises an error' do
expect { identity.account_aliases }.to raise_error(
StackMaster::Identity::MissingIamPermissionsError,
'Failed to retrieve account aliases. Missing required IAM permission: iam:ListAccountAliases'
)
end
end
end
end
1 change: 1 addition & 0 deletions stack_master.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ Gem::Specification.new do |spec|
spec.add_dependency "aws-sdk-sns", "~> 1"
spec.add_dependency "aws-sdk-ssm", "~> 1"
spec.add_dependency "aws-sdk-ecr", "~> 1"
spec.add_dependency "aws-sdk-iam", "~> 1"
spec.add_dependency "diffy"
spec.add_dependency "erubis"
spec.add_dependency "colorize"
Expand Down