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

Add ability to restrict which accounts can work with stacks #283

Merged
merged 14 commits into from
Jul 2, 2019
Merged
Show file tree
Hide file tree
Changes from 12 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
42 changes: 42 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,14 @@ stacks:
staging:
myapp-vpc:
template: myapp_vpc.rb
allowed_accounts: '123456789'
tags:
purpose: front-end
myapp-db:
template: myapp_db.rb
allowed_accounts:
- '1234567890'
- '9876543210'
tags:
purpose: back-end
myapp-web:
Expand Down Expand Up @@ -545,6 +549,44 @@ end

Note though that if a dynamic with the same name exists in your `templates/dynamics/` directory it will get loaded since it has higher precedence.

## 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.

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

Unlike other stack defaults, `allowed_accounts` values specified in the stack definition override values in the stack defaults instead of merging them together. This allows specifying allowed accounts in the stack defaults for all stacks and still have different values for specific stacks. See below example config for an example.
Copy link
Contributor

Choose a reason for hiding this comment

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

From just reading this I'm still not entirely sure what the behaviour will be. Does it allow applying myapp-db to account 555555555 or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it doesn't as the account IDs specified in the stack definition overrides the stack defaults.

I've reworded this section and example stack below. Let me know if it clarifies things.

See 4233345.


```yaml
stack_defaults:
allowed_accounts: '555555555'
stacks:
us-east-1:
myapp-vpc: # inherits allowed account 555555555
template: myapp_vpc.rb
tags:
purpose: front-end
myapp-db:
template: myapp_db.rb
allowed_accounts: # only these accounts
- '1234567890'
- '9876543210'
tags:
purpose: back-end
myapp-web:
template: myapp_web.rb
allowed_accounts: [] # allow all accounts by overriding stack defaults
tags:
purpose: front-end
myapp-redis:
template: myapp_redis.rb
allowed_accounts: '888888888' # only this account
tags:
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.

## Commands

```bash
Expand Down
12 changes: 12 additions & 0 deletions lib/stack_master.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ module StackMaster
autoload :PagedResponseAccumulator, 'stack_master/paged_response_accumulator'
autoload :StackDefinition, 'stack_master/stack_definition'
autoload :TemplateCompiler, 'stack_master/template_compiler'
autoload :Identity, 'stack_master/identity'

autoload :StackDiffer, 'stack_master/stack_differ'
autoload :Validator, 'stack_master/validator'
Expand Down Expand Up @@ -97,6 +98,7 @@ module StackEvents
NON_INTERACTIVE_DEFAULT = false
DEBUG_DEFAULT = false
QUIET_DEFAULT = false
SKIP_ACCOUNT_CHECK_DEFAULT = false

def interactive?
!non_interactive?
Expand Down Expand Up @@ -136,6 +138,16 @@ def quiet?

def reset_flags
@quiet = QUIET_DEFAULT
@skip_account_check = SKIP_ACCOUNT_CHECK_DEFAULT
end

def skip_account_check!
@skip_account_check = true
end
@skip_account_check = SKIP_ACCOUNT_CHECK_DEFAULT

def skip_account_check?
@skip_account_check
end

attr_accessor :non_interactive_answer
Expand Down
39 changes: 33 additions & 6 deletions lib/stack_master/cli.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ def execute!
global_option '-q', '--quiet', 'Do not output the resulting Stack Events, just return immediately' do
StackMaster.quiet!
end
global_option '--skip-account-check', 'Do not check if command is allowed to execute in account' do
StackMaster.skip_account_check!
end

command :apply do |c|
c.syntax = 'stack_master apply [region_or_alias] [stack_name]'
Expand Down Expand Up @@ -178,18 +181,22 @@ def execute!
return
end

stack_name = Utils.underscore_to_hyphen(args[1])
allowed_accounts = []

# Because delete can work without a stack_master.yml
if options.config and File.file?(options.config)
config = load_config(options.config)
region = Utils.underscore_to_hyphen(config.unalias_region(args[0]))
allowed_accounts = config.find_stack(region, stack_name)&.allowed_accounts
else
region = args[0]
end

stack_name = Utils.underscore_to_hyphen(args[1])

StackMaster.cloud_formation_driver.set_region(region)
StackMaster::Commands::Delete.perform(region, stack_name)
execute_if_allowed_account(allowed_accounts) do
StackMaster.cloud_formation_driver.set_region(region)
StackMaster::Commands::Delete.perform(region, stack_name)
end
end
end

Expand Down Expand Up @@ -223,15 +230,35 @@ def execute_stacks_command(command, args, options)
success = false
end
stack_definitions = stack_definitions.select do |stack_definition|
StackStatus.new(config, stack_definition).changed?
running_in_allowed_account?(stack_definition.allowed_accounts) && StackStatus.new(config, stack_definition).changed?
end if options.changed
stack_definitions.each do |stack_definition|
StackMaster.cloud_formation_driver.set_region(stack_definition.region)
StackMaster.stdout.puts "Executing #{command.command_name} on #{stack_definition.stack_name} in #{stack_definition.region}"
success = false unless command.perform(config, stack_definition, options).success?
success = execute_if_allowed_account(stack_definition.allowed_accounts) do
command.perform(config, stack_definition, options).success?
end
end
end
success
end

def execute_if_allowed_account(allowed_accounts, &block)
raise ArgumentError, "Block required to execute this method" unless block_given?
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}."
false
end
end

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

def identity
@identity ||= StackMaster::Identity.new
end
end
end
13 changes: 11 additions & 2 deletions lib/stack_master/commands/status.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,13 @@ def perform
progress if @show_progress
status = @config.stacks.map do |stack_definition|
stack_status = StackStatus.new(@config, stack_definition)
allowed_accounts = stack_definition.allowed_accounts
progress.increment if @show_progress
{
region: stack_definition.region,
stack_name: stack_definition.stack_name,
stack_status: stack_status.status,
different: stack_status.changed_message,
stack_status: running_in_allowed_account?(allowed_accounts) ? stack_status.status : "Disallowed account",
different: running_in_allowed_account?(allowed_accounts) ? stack_status.changed_message : "N/A",
}
end
tp.set :max_width, self.window_size
Expand All @@ -41,6 +42,14 @@ def progress
def sort_params(hash)
hash.sort.to_h
end

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

def identity
@identity ||= StackMaster::Identity.new
end
end
end
end
1 change: 1 addition & 0 deletions lib/stack_master/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ def load_stacks(stacks)
'base_dir' => @base_dir,
'template_dir' => @template_dir,
'additional_parameter_lookup_dirs' => @region_to_aliases[region])
stack_attributes['allowed_accounts'] = attributes['allowed_accounts'] if attributes['allowed_accounts']
@stacks << StackDefinition.new(stack_attributes)
end
end
Expand Down
23 changes: 23 additions & 0 deletions lib/stack_master/identity.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
module StackMaster
class Identity
def running_in_allowed_account?(allowed_accounts)

Choose a reason for hiding this comment

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

Seems odd to pass a nil or empty variable to a function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe if nil was the default value for allowed_accounts (in StackDefinition, indicating unspecified) the expected values would either be nil or an array of account IDs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Done in b30dede.

allowed_accounts.nil? || allowed_accounts.empty? || allowed_accounts.include?(account)
end

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

private

attr_reader :sts

def region
@region ||= ENV['AWS_REGION'] || Aws.config[:region] || Aws.shared_config.region || 'us-east-1'
end
Copy link
Contributor

Choose a reason for hiding this comment

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

This method isn't necessary. If we don't specify a region the SDK does a lookup for us automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right it does, however I added this method to ensure that a default region is used if it couldn't find a region from the environment. It's also the same as the CloudFormation driver class except for the default value.

This was me being more defensive, but happy to remove it if you think it'll be fine without it?


def sts
@sts ||= Aws::STS::Client.new(region: region)
end
end
end
4 changes: 4 additions & 0 deletions lib/stack_master/stack_definition.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ class StackDefinition
:template,
:tags,
:role_arn,
:allowed_accounts,
:notification_arns,
:base_dir,
:template_dir,
Expand All @@ -23,8 +24,10 @@ def initialize(attributes = {})
@notification_arns = []
@s3 = {}
@files = []
@allowed_accounts = nil
super
@template_dir ||= File.join(@base_dir, 'templates')
@allowed_accounts = Array(@allowed_accounts)
end

def ==(other)
Expand All @@ -34,6 +37,7 @@ def ==(other)
@template == other.template &&
@tags == other.tags &&
@role_arn == other.role_arn &&
@allowed_accounts == other.allowed_accounts &&
@notification_arns == other.notification_arns &&
@base_dir == other.base_dir &&
@secret_file == other.secret_file &&
Expand Down
6 changes: 6 additions & 0 deletions spec/fixtures/stack_master.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ region_aliases:
production: us-east-1
staging: ap-southeast-2
stack_defaults:
allowed_accounts:
- '555555555'
tags:
application: my-awesome-blog
s3:
Expand Down Expand Up @@ -35,6 +37,7 @@ stacks:
role_arn: test_service_role_arn2
myapp_web:
template: myapp_web.rb
allowed_accounts: '1234567890'
myapp_vpc_with_secrets:
template: myapp_vpc.json
ap-southeast-2:
Expand All @@ -45,5 +48,8 @@ stacks:
role_arn: test_service_role_arn4
myapp_web:
template: myapp_web
allowed_accounts:
- '1234567890'
- '9876543210'
tags:
test_override: 2
70 changes: 66 additions & 4 deletions spec/stack_master/commands/status_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
subject(:status) { described_class.new(config, false) }
let(:config) { instance_double(StackMaster::Config, stacks: stacks) }
let(:stacks) { [stack_definition_1, stack_definition_2] }
let(:stack_definition_1) { double(:stack_definition_1, region: 'us-east-1', stack_name: 'stack1') }
let(:stack_definition_2) { double(:stack_definition_2, region: 'us-east-1', stack_name: 'stack2', stack_status: 'CREATE_COMPLETE') }
let(:stack_definition_1) { double(:stack_definition_1, region: 'us-east-1', stack_name: 'stack1', allowed_accounts: []) }
let(:stack_definition_2) { double(:stack_definition_2, region: 'us-east-1', stack_name: 'stack2', stack_status: 'CREATE_COMPLETE', allowed_accounts: []) }
let(:cf) { Aws::CloudFormation::Client.new(region: 'us-east-1') }

before do
Expand All @@ -23,7 +23,13 @@
let(:proposed_stack2) { double(:proposed_stack2, template_body: "{}", template_format: :json, parameters_with_defaults: {a: 1}) }

it "returns the status of call stacks" do
out = "REGION | STACK_NAME | STACK_STATUS | DIFFERENT\n----------|------------|-----------------|----------\nus-east-1 | stack1 | UPDATE_COMPLETE | No \nus-east-1 | stack2 | CREATE_COMPLETE | Yes \n * No echo parameters can't be diffed\n"
out = <<~OUTPUT
REGION | STACK_NAME | STACK_STATUS | DIFFERENT
----------|------------|-----------------|----------
us-east-1 | stack1 | UPDATE_COMPLETE | No
us-east-1 | stack2 | CREATE_COMPLETE | Yes
* No echo parameters can't be diffed
OUTPUT
expect { status.perform }.to output(out).to_stdout
end
end
Expand All @@ -35,10 +41,66 @@
let(:proposed_stack2) { double(:proposed_stack2, template_body: "{}", template_format: :json, parameters_with_defaults: {a: 1}) }

it "returns the status of call stacks" do
out = "REGION | STACK_NAME | STACK_STATUS | DIFFERENT\n----------|------------|-----------------|----------\nus-east-1 | stack1 | UPDATE_COMPLETE | Yes \nus-east-1 | stack2 | CREATE_COMPLETE | No \n * No echo parameters can't be diffed\n"
out = <<~OUTPUT
REGION | STACK_NAME | STACK_STATUS | DIFFERENT
----------|------------|-----------------|----------
us-east-1 | stack1 | UPDATE_COMPLETE | Yes
us-east-1 | stack2 | CREATE_COMPLETE | No
* No echo parameters can't be diffed
OUTPUT
expect { status.perform }.to output(out).to_stdout
end
end

context 'when identity account is not allowed' do
let(:sts) { Aws::STS::Client.new(stub_responses: true) }
let(:stack_definition_1) { double(:stack_definition_1, region: 'us-east-1', stack_name: 'stack1', allowed_accounts: ['not-account-id']) }
let(:stack1) { double(:stack1, template_body: '{"foo": "bar"}', template_hash: {foo: 'bar'}, template_format: :json, parameters_with_defaults: {a: 1}, stack_status: 'UPDATE_COMPLETE') }
let(:stack2) { double(:stack2, template_body: '{}', template_hash: {}, template_format: :json, parameters_with_defaults: {a: 1}, stack_status: 'CREATE_COMPLETE') }
let(:proposed_stack1) { double(:proposed_stack1, template_body: "{}", template_format: :json, parameters_with_defaults: {a: 1}) }
let(:proposed_stack2) { double(:proposed_stack2, template_body: "{}", template_format: :json, parameters_with_defaults: {a: 1}) }

before do
allow(Aws::STS::Client).to receive(:new).and_return(sts)
sts.stub_responses(:get_caller_identity, {
account: 'account-id',
arn: 'an-arn',
user_id: 'a-user-id'
})
end

it 'sets stack status and different fields accordingly' do
out = <<~OUTPUT
REGION | STACK_NAME | STACK_STATUS | DIFFERENT
----------|------------|--------------------|----------
us-east-1 | stack1 | Disallowed account | N/A
us-east-1 | stack2 | UPDATE_COMPLETE | Yes
* No echo parameters can't be diffed
OUTPUT
expect { status.perform }.to output(out).to_stdout
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this test expect an error to be returned? If the account IDs don't match it shouldn't print the status?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a got a special case now: Disallowed account | N/A.
It would not be great if it threw an error just because one stack is unavailable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What @viraptor said.

Alternatively, I could leave them blank instead? I didn't go with that option because I thought it would be better to let the user know why no status is included for that stack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option is to include SKIPPED for the status field and Disallowed account for different field.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right I see. Are we able to have an integration test for when someone runs stack_master apply on a stack in the wrong account then? I thought that's what this was doing, but I didn't read the spec name 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Added integration tests in f02603c.

end

context 'when --skip-account-check flag is set' do
before do
StackMaster.skip_account_check!
end

after do
StackMaster.reset_flags
Copy link
Contributor

Choose a reason for hiding this comment

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

I think spec_helper already handles this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't handle this. The cucumber features do though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right you are

end

it "returns the status of call stacks" do
out = <<~OUTPUT
REGION | STACK_NAME | STACK_STATUS | DIFFERENT
----------|------------|-----------------|----------
us-east-1 | stack1 | UPDATE_COMPLETE | Yes
us-east-1 | stack2 | CREATE_COMPLETE | No
* No echo parameters can't be diffed
OUTPUT
expect { status.perform }.to output(out).to_stdout
end
end
end
end

end
Loading