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

Cross-account parameter resolving #292

Merged
merged 21 commits into from
Dec 23, 2019
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
d72a685
Add RoleAssumer class
petervandoros Nov 12, 2019
98c3b03
Update ParameterResolver to support assuming roles
petervandoros Nov 12, 2019
f14b567
Use existing cloudformation driver's class to create new instance
petervandoros Nov 12, 2019
7015ef3
Add integration test for assuming a role for parameter resolving
petervandoros Nov 12, 2019
c96653a
Add parameter resolver assume role docs to readme
petervandoros Nov 12, 2019
d8a7bd6
Minor typo fix in readme
petervandoros Nov 12, 2019
31bbaa3
Be explicit about all parameters can assume a role
petervandoros Nov 13, 2019
83934f3
Remove unused code in SnsTopicName
petervandoros Nov 14, 2019
fbc6d65
Don't cache AmiFinder instance in latest_ami resolver
petervandoros Dec 11, 2019
8a9f15d
Don't cache SSM client in parameter_store resolver
petervandoros Dec 11, 2019
fd78c9d
Fix stack_output tests verifying stack caching
petervandoros Dec 11, 2019
bb447b6
Update stack_output resolver to take credentials into account
petervandoros Dec 11, 2019
530ed3d
Update ejson parameter resolve to take credentials into account
petervandoros Dec 11, 2019
95a8825
Refactor RoleAssumer for clarity
petervandoros Dec 16, 2019
7ff4180
Merge branch 'master' into assume-role-parameter-resolving
petervandoros Dec 16, 2019
41592b3
Update changelog to include cross-account parameter resolving
petervandoros Dec 16, 2019
56cc3a6
Whitespace between rspec contexts
petervandoros Dec 16, 2019
4ad4ade
Minor grammatical tweak to the README
petervandoros Dec 16, 2019
2905ad2
Don't install bundler in Travis CI
petervandoros Dec 16, 2019
9da0c49
Revert "Don't install bundler in Travis CI"
petervandoros Dec 17, 2019
b21ea00
Merge branch 'master' into assume-role-parameter-resolving
petervandoros Dec 17, 2019
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
36 changes: 35 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,40 @@ One benefit of using parameter resolvers instead of hard coding values like VPC
IDs and resource ARNs is that the same configuration works cross
region/account, even though the resolved values will be different.

### Cross-account parameter resolving

One way to resolve parameter values from different accounts to the one StackMaster runs in, is to
assume a role in another account with the relevant IAM permissions to execute successfully.

This is supported in StackMaster by specifying the `role` and `account` properties for any
parameter resolver in the stack's parameters file.

```yaml
vpc_peering_id:
role: cross-account-parameter-resolver
account: 1234567890
stack_output: vpc-peering-stack-in-other-account/peering_name
petervandoros marked this conversation as resolved.
Show resolved Hide resolved

an_array_param:
role: cross-account-parameter-resolver
account: 1234567890
stack_outputs:
- stack-in-account1/output
- stack-in-account1/another_output

another_array_param:
- role: cross-account-parameter-resolver
account: 1234567890
stack_output: stack-in-account1/output
- role: cross-account-parameter-resolver
account: 0987654321
stack_output: stack-in-account2/output
```

An example of use case where cross-account parameter resolving is particularly useful is when
setting up VPC peering where you need the VPC ID of the peer. Without the ability to assume
a role in another account, the only option was to hard code the peer's VPC ID.

### Stack Output

The stack output parameter resolver looks up outputs from other stacks in the
Expand Down Expand Up @@ -270,7 +304,7 @@ db_password:
```

### 1Password Lookup
An Alternative to the alternative secret store is accessing 1password secrets using the 1password cli (`op`).
An alternative to the alternative secret store is accessing 1password secrets using the 1password cli (`op`).
petervandoros marked this conversation as resolved.
Show resolved Hide resolved
You declare a 1password lookup with the following parameters in your parameters file:

```
Expand Down
57 changes: 57 additions & 0 deletions features/apply_with_assume_role_parameter_resolvers.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
Feature: Apply command with assume role parameter resolvers
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️


Background:
Given a file named "stack_master.yml" with:
"""
stacks:
us-east-2:
vpc:
template: vpc.rb
myapp_web:
template: myapp_web.rb
"""
And a directory named "parameters"
And a file named "parameters/myapp_web.yml" with:
"""
vpc_id:
role: my-role
account: 1234567890
stack_output: vpc/vpc_id
"""
And a directory named "templates"
And a file named "templates/myapp_web.rb" with:
"""
SparkleFormation.new(:myapp_web) do
description "Test template"
set!('AWSTemplateFormatVersion', '2010-09-09')

parameters.vpc_id do
description 'VPC ID'
type 'AWS::EC2::VPC::Id'
end

resources.test_sg do
type 'AWS::EC2::SecurityGroup'
properties do
group_description 'Test SG'
vpc_id ref!(:vpc_id)
end
end
end
"""

Scenario: Run apply and create a new stack
Given I stub the CloudFormation driver
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 |
And I stub the following stacks:
| stack_id | stack_name | parameters | outputs | region |
| 1 | vpc | VpcCidr=10.0.0.16/22 | VpcId=vpc-id | us-east-2 |
| 2 | myapp_web | | | us-east-2 |
Then I expect the role "my-role" is assumed in account "1234567890"
When I run `stack_master apply us-east-2 myapp_web --trace`
And the output should contain all of these lines:
| +--- |
| +VpcId: vpc-id |
Then the exit status should be 0
6 changes: 6 additions & 0 deletions features/step_definitions/asume_role_steps.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Then(/^I expect the role "([^"]*)" is assumed in account "([^"]*)"$/) do |role, account|
expect(Aws::AssumeRoleCredentials).to receive(:new).with(
role_arn: "arn:aws:iam::#{account}:role/#{role}",
role_session_name: instance_of(String)
)
end
4 changes: 4 additions & 0 deletions features/step_definitions/stack_steps.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ def extract_hash_from_kv_string(string)
allow(StackMaster.cloud_formation_driver).to receive(:validate_template).and_raise(Aws::CloudFormation::Errors::ValidationError.new('', message))
end

Given(/^I stub the CloudFormation driver$/) do
allow(StackMaster.cloud_formation_driver.class).to receive(:new).and_return(StackMaster.cloud_formation_driver)
end

When(/^an S3 file in bucket "([^"]*)" with key "([^"]*)" exists with content:$/) do |bucket, key, body|
file = StackMaster.s3_driver.find_file(bucket: bucket, object_key: key)
expect(file).to eq body
Expand Down
1 change: 1 addition & 0 deletions lib/stack_master.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ module StackMaster
autoload :SecurityGroupFinder, 'stack_master/security_group_finder'
autoload :ParameterLoader, 'stack_master/parameter_loader'
autoload :ParameterResolver, 'stack_master/parameter_resolver'
autoload :RoleAssumer, 'stack_master/role_assumer'
autoload :ResolverArray, 'stack_master/resolver_array'
autoload :Resolver, 'stack_master/resolver_array'
autoload :Utils, 'stack_master/utils'
Expand Down
37 changes: 31 additions & 6 deletions lib/stack_master/parameter_resolver.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,16 +49,37 @@ def resolve_parameter_value(key, parameter_value)
return parameter_value.to_s if Numeric === parameter_value || parameter_value == true || parameter_value == false
return resolve_array_parameter_values(key, parameter_value).join(',') if Array === parameter_value
return parameter_value unless Hash === parameter_value
validate_parameter_value!(key, parameter_value)
resolve_parameter_resolver_hash(key, parameter_value)
rescue Aws::CloudFormation::Errors::ValidationError
raise InvalidParameter, $!.message
end

def resolve_parameter_resolver_hash(key, parameter_value)
# strip out account and role
resolver_hash = parameter_value.except('account', 'role')
account, role = parameter_value.values_at('account', 'role')

resolver_name = parameter_value.keys.first.to_s
validate_parameter_value!(key, resolver_hash)

resolver_name = resolver_hash.keys.first.to_s
load_parameter_resolver(resolver_name)

value = parameter_value.values.first
value = resolver_hash.values.first
resolver_class_name = resolver_name.camelize
call_resolver(resolver_class_name, value)
rescue Aws::CloudFormation::Errors::ValidationError
raise InvalidParameter, $!.message

assume_role_if_present(account, role, key) do
call_resolver(resolver_class_name, value)
end
end

def assume_role_if_present(account, role, key)
return yield if account.nil? && role.nil?
if account.nil? || role.nil?
raise InvalidParameter, "Both 'account' and 'role' are required to assume role for parameter '#{key}'"
end
role_assumer.assume_role(account, role) do
yield
end
end

def resolve_array_parameter_values(key, parameter_values)
Expand Down Expand Up @@ -94,5 +115,9 @@ def validate_parameter_value!(key, parameter_value)
raise InvalidParameter, "#{key} hash contained more than one key: #{parameter_value.inspect}"
end
end

def role_assumer
@role_assumer ||= RoleAssumer.new
end
end
end
59 changes: 59 additions & 0 deletions lib/stack_master/role_assumer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
require 'active_support/core_ext/object/deep_dup'

module StackMaster
class RoleAssumer
BlockNotSpecified = Class.new(StandardError)

def initialize
@credentials = {}
end

def assume_role(account, role, &block)
raise BlockNotSpecified unless block_given?
raise ArgumentError, "Both 'account' and 'role' are required to assume a role" if account.nil? || role.nil?

original_aws_config = replace_aws_global_config
Aws.config[:credentials] = assume_role_credentials(account, role)
begin
original_cf_driver = replace_cf_driver
block.call
ensure
restore_aws_global_config(original_aws_config)
restore_cf_driver(original_cf_driver)
end
end

private

def replace_aws_global_config
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 the replace/restore functions are not very pretty. Part of that logic also exists in assume_role.

How about:

def assume_role(...)
...
  role_credentials = assume_role_credentials(account, role)
  with_temporary_role(role_credentials) do
    with_temporary_driver do
      block.call
    end
  end
...
end

def with_temporary_role(credentials, &block)
  original_aws_config = Aws.config
  Aws.config = original_aws_config.deep_dup
  block.call
ensure
  Aws.config = original_aws_config
end

def with_temporary_driver(&block)
  original_driver = StackMaster.cloud_formation_driver
  StackMaster.cloud_formation_driver = original_driver.class.new
  block.call
ensure
  StackMaster.cloud_formation_driver = original_driver
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea. Done in 95a8825.

config = Aws.config
Aws.config = config.deep_dup
config
end

def restore_aws_global_config(config)
Aws.config = config
end

def replace_cf_driver
driver = StackMaster.cloud_formation_driver
StackMaster.cloud_formation_driver = driver.class.new
driver
end

def restore_cf_driver(driver)
return if driver.nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

If the driver is nil, driver.class.new in replace_cf_driver would fail earier.

StackMaster.cloud_formation_driver = driver
end

def assume_role_credentials(account, role)
credentials_key = "#{account}:#{role}"
@credentials.fetch(credentials_key) do
@credentials[credentials_key] = Aws::AssumeRoleCredentials.new(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to set the hash here if fetch is being used?

Suggested change
@credentials[credentials_key] = Aws::AssumeRoleCredentials.new(
Aws::AssumeRoleCredentials.new(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The block passed to the fetch method specifies the return value if the key is not found. It doesn't set the value in the hash if it's missing. We need to set the hash value for it to persist.

irb(main):001:0> h = {a: 'a', b: 'b'}
=> {:a=>"a", :b=>"b"}
irb(main):002:0> h.fetch(:z) do
irb(main):003:1* 'z'
irb(main):004:1> end
=> "z"
irb(main):005:0> h
=> {:a=>"a", :b=>"b"}
irb(main):006:0> h.fetch(:z) do
irb(main):007:1* h[:z] = 'z'
irb(main):008:1> end
=> "z"
irb(main):009:0> h
=> {:a=>"a", :b=>"b", :z=>"z"}

role_arn: "arn:aws:iam::#{account}:role/#{role}",
role_session_name: "stack-master-assume-role-parameter-resolver"
)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we want to save credentials to an instance variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it doesn't have to assume the role for every parameter wishing to assume the same role.

end
end
end
85 changes: 85 additions & 0 deletions spec/stack_master/parameter_resolver_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,91 @@ def resolve(params)
end
end

context 'when assuming a role' do
let(:role_assumer) { instance_double(StackMaster::RoleAssumer, assume_role: nil ) }
let(:account) { '1234567890' }
let(:role) { 'my-role' }

before do
allow(StackMaster::RoleAssumer).to receive(:new).and_return(role_assumer)
end

context 'with valid assume role properties' do
let(:params) do
{
param: {
'account' => account,
'role' => role,
'my_resolver' => 2
}
}
end

it 'assumes the role' do
expect(StackMaster::RoleAssumer).to receive(:new)
expect(role_assumer).to receive(:assume_role).with(account, role)

parameter_resolver.resolve
end
end

context 'when multiple params assume roles' do
let(:params) do
{
param: {
'account' => account,
'role' => role,
'my_resolver' => 1
},
param2: {
'account' => account,
'role' => 'different-role',
'my_resolver' => 2
}
}
end

it 'caches the role assumer' do
expect(StackMaster::RoleAssumer).to receive(:new).once

parameter_resolver.resolve
end
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 now


it 'calls assume role once for every param' do
expect(role_assumer).to receive(:assume_role).with(account, role).once
expect(role_assumer).to receive(:assume_role).with(account, 'different-role').once

parameter_resolver.resolve
end
end

context 'with missing assume role properties' do
it 'does not assume a role' do
expect(StackMaster::RoleAssumer).not_to receive(:new)

parameter_resolver.resolve
end
end

context "with missing 'account' property" do
it 'raises an invalid parameter error' do
expect {
resolve(param: { 'role' => role, 'my_resolver' => 2 })
}.to raise_error StackMaster::ParameterResolver::InvalidParameter,
match("Both 'account' and 'role' are required to assume role for parameter 'param'")
end
end

context "with missing 'role' property" do
it 'raises an invalid parameter error' do
expect {
resolve(param: { 'account' => account, 'my_resolver' => 2 })
}.to raise_error StackMaster::ParameterResolver::InvalidParameter,
match("Both 'account' and 'role' are required to assume role for parameter 'param'")
end
end
end

context 'resolver class caching' do
it "uses the same instance of the resolver for the duration of the resolve run" do
expect(my_resolver).to receive(:new).once.and_call_original
Expand Down
Loading