From a5e2516354a5dc735999e761399efcdc1f314187 Mon Sep 17 00:00:00 2001 From: Philip Garrett Date: Sat, 4 Jun 2016 19:07:38 -0400 Subject: [PATCH 1/2] Prevent consul from writing its own config This fixes issue #322. The consul cookbook makes the Consul config writable by the same user that runs the service, which is unnecessary and could be an attack vector. This commit allows the user to specify a different set of permissions for the configuration directories (/etc/consul) than for the runtime data directory (/var/lib/consul). The config owner still defaults to 'consul' since changing it to root is a breaking change. --- .kitchen.yml | 2 ++ Gemfile | 2 +- attributes/default.rb | 2 ++ libraries/consul_config.rb | 23 +++++++----- libraries/consul_service.rb | 12 +++---- recipes/default.rb | 4 --- test/cookbooks/consul_spec/recipes/default.rb | 4 +++ .../default/serverspec/default_spec.rb | 19 +++++++--- test/spec/libraries/consul_config_spec.rb | 35 ++++++++++++++----- .../libraries/consul_service_linux_spec.rb | 1 - 10 files changed, 69 insertions(+), 35 deletions(-) create mode 100644 test/cookbooks/consul_spec/recipes/default.rb diff --git a/.kitchen.yml b/.kitchen.yml index 851bf238..640af0ac 100644 --- a/.kitchen.yml +++ b/.kitchen.yml @@ -41,6 +41,8 @@ suites: attributes: consul: config: &default-config + owner: root + group: consul bootstrap: true server: true datacenter: FortMeade diff --git a/Gemfile b/Gemfile index b35b3b74..f738cae1 100644 --- a/Gemfile +++ b/Gemfile @@ -12,7 +12,7 @@ group :unit, :integration do gem 'chef-sugar' gem 'chefspec' gem 'berkshelf', '~> 4.0' - gem 'test-kitchen' + gem 'test-kitchen', '~> 1.7.3' # 1.8 requires kitchen.yml changes to support policyfile gem 'serverspec' end diff --git a/attributes/default.rb b/attributes/default.rb index 94b62f9d..0c348fda 100644 --- a/attributes/default.rb +++ b/attributes/default.rb @@ -9,6 +9,8 @@ default['consul']['service_user'] = 'consul' default['consul']['service_group'] = 'consul' +default['consul']['config']['owner'] = 'consul' +default['consul']['config']['group'] = 'consul' default['consul']['config']['path'] = join_path config_prefix_path, 'consul.json' default['consul']['config']['data_dir'] = data_path default['consul']['config']['ca_file'] = join_path config_prefix_path, 'ssl', 'CA', 'ca.crt' diff --git a/libraries/consul_config.rb b/libraries/consul_config.rb index f4bc35e5..4062d310 100644 --- a/libraries/consul_config.rb +++ b/libraries/consul_config.rb @@ -20,10 +20,13 @@ class ConsulConfig < Chef::Resource attribute(:path, kind_of: String, name_attribute: true) # @!attribute owner # @return [String] - attribute(:owner, kind_of: String, default: 'consul') + attribute(:owner, kind_of: String, default: lazy { node['consul']['config']['owner'] }) # @!attribute group # @return [String] - attribute(:group, kind_of: String, default: 'consul') + attribute(:group, kind_of: String, default: lazy { node['consul']['config']['group'] }) + # @!attribute config_dir + # @return [String] + attribute(:config_dir, kind_of: String, default: lazy { node['consul']['service']['config_dir'] }) # @!attribute options # @return [Hash] attribute(:options, option_collector: true) @@ -110,14 +113,16 @@ def tls? action(:create) do notifying_block do - directory ::File.dirname(new_resource.path) do - recursive true - unless node.platform?('windows') - owner new_resource.owner - group new_resource.group - mode '0755' + [::File.dirname(new_resource.path), new_resource.config_dir].each do |dir| + directory dir do + recursive true + unless node.platform?('windows') + owner new_resource.owner + group new_resource.group + mode '0755' + end + not_if { dir == '/etc' } end - not_if { ::File.dirname(new_resource.path) == '/etc' } end file new_resource.path do diff --git a/libraries/consul_service.rb b/libraries/consul_service.rb index 0da2b834..d382047e 100644 --- a/libraries/consul_service.rb +++ b/libraries/consul_service.rb @@ -70,13 +70,11 @@ class ConsulService < Chef::Provider def action_enable notifying_block do - [new_resource.data_dir, new_resource.config_dir].each do |dirname| - directory dirname do - recursive true - owner new_resource.user - group new_resource.group - mode '0755' - end + directory new_resource.data_dir do + recursive true + owner new_resource.user + group new_resource.group + mode '0750' end end super diff --git a/recipes/default.rb b/recipes/default.rb index 85c0f116..9c82d640 100644 --- a/recipes/default.rb +++ b/recipes/default.rb @@ -47,10 +47,6 @@ service_name = node['consul']['service_name'] config = consul_config service_name do |r| - unless windows? - owner node['consul']['service_user'] - group node['consul']['service_group'] - end node['consul']['config'].each_pair { |k, v| r.send(k, v) } notifies :reload, "consul_service[#{service_name}]", :delayed end diff --git a/test/cookbooks/consul_spec/recipes/default.rb b/test/cookbooks/consul_spec/recipes/default.rb new file mode 100644 index 00000000..02ff2271 --- /dev/null +++ b/test/cookbooks/consul_spec/recipes/default.rb @@ -0,0 +1,4 @@ + +include_recipe 'consul::default' +include_recipe 'consul_spec::consul_definition' +include_recipe 'consul_spec::consul_watch' diff --git a/test/integration/default/serverspec/default_spec.rb b/test/integration/default/serverspec/default_spec.rb index 6ccdbb04..cd595a26 100644 --- a/test/integration/default/serverspec/default_spec.rb +++ b/test/integration/default/serverspec/default_spec.rb @@ -37,21 +37,30 @@ its(:stdout) { should match %r{\bdc=fortmeade\b} } end +config_dir = '/etc/consul' config_file = '/etc/consul/consul.json' -config_dir = '/etc/consul/conf.d' +confd_dir = '/etc/consul/conf.d' data_dir = '/var/lib/consul' +describe file(config_dir) do + it { should be_directory } + it { should be_owned_by 'root' } + it { should be_grouped_into 'consul' } + + it { should be_mode 755 } +end + describe file(config_file) do it { should be_file } - it { should be_owned_by 'consul' } + it { should be_owned_by 'root' } it { should be_grouped_into 'consul' } it { should be_mode 640 } end -describe file(config_dir) do +describe file(confd_dir) do it { should be_directory } - it { should be_owned_by 'consul' } + it { should be_owned_by 'root' } it { should be_grouped_into 'consul' } it { should be_mode 755 } @@ -62,7 +71,7 @@ it { should be_owned_by 'consul' } it { should be_grouped_into 'consul' } - it { should be_mode 755 } + it { should be_mode 750 } end if os[:family] == 'ubuntu' diff --git a/test/spec/libraries/consul_config_spec.rb b/test/spec/libraries/consul_config_spec.rb index d9ff0a66..054e2a56 100644 --- a/test/spec/libraries/consul_config_spec.rb +++ b/test/spec/libraries/consul_config_spec.rb @@ -8,8 +8,18 @@ before do recipe = double('Chef::Recipe') allow_any_instance_of(Chef::RunContext).to receive(:include_recipe).and_return([recipe]) + default_attributes['consul'] = { + 'service' => { + 'config_dir' => '/etc/consul/conf.d' + }, + 'config' => { + 'owner' => 'root', + 'group' => 'consul' + } + } end + context 'sets options directly' do recipe do consul_config '/etc/consul/default.json' do @@ -20,14 +30,23 @@ end end - it { is_expected.to render_file('/etc/consul/default.json').with_content(<<-EOH.chomp) } -{ - "recursor": "foo", - "translate_wan_addrs": true, - "verify_incoming": false, - "verify_outgoing": false -} -EOH + it do + is_expected.to create_directory('/etc/consul/conf.d') + .with(user: 'root', group: 'consul', mode: '0755') + end + + it do + is_expected.to create_file('/etc/consul/default.json') + .with(user: 'root', group: 'consul', mode: '0640') + .with(content: <<-EOH.chomp.gsub(/^ /,'')) + { + "recursor": "foo", + "translate_wan_addrs": true, + "verify_incoming": false, + "verify_outgoing": false + } + EOH + end end context 'deletes configuration' do diff --git a/test/spec/libraries/consul_service_linux_spec.rb b/test/spec/libraries/consul_service_linux_spec.rb index c40426e3..8260a7a2 100644 --- a/test/spec/libraries/consul_service_linux_spec.rb +++ b/test/spec/libraries/consul_service_linux_spec.rb @@ -8,7 +8,6 @@ context 'with default properties' do recipe 'consul::default' - it { expect(chef_run).to create_directory('/etc/consul/conf.d') } it { is_expected.to create_directory('/var/lib/consul') } end end From ab8d631236fd8daa6198037d69c4dc92af3ece64 Mon Sep 17 00:00:00 2001 From: Philip Garrett Date: Thu, 9 Jun 2016 22:05:35 -0400 Subject: [PATCH 2/2] Revert to static owner/group defaults --- libraries/consul_config.rb | 4 ++-- test/spec/libraries/consul_config_spec.rb | 5 +---- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/libraries/consul_config.rb b/libraries/consul_config.rb index 4062d310..d60df00d 100644 --- a/libraries/consul_config.rb +++ b/libraries/consul_config.rb @@ -20,10 +20,10 @@ class ConsulConfig < Chef::Resource attribute(:path, kind_of: String, name_attribute: true) # @!attribute owner # @return [String] - attribute(:owner, kind_of: String, default: lazy { node['consul']['config']['owner'] }) + attribute(:owner, kind_of: String, default: 'consul') # @!attribute group # @return [String] - attribute(:group, kind_of: String, default: lazy { node['consul']['config']['group'] }) + attribute(:group, kind_of: String, default: 'consul') # @!attribute config_dir # @return [String] attribute(:config_dir, kind_of: String, default: lazy { node['consul']['service']['config_dir'] }) diff --git a/test/spec/libraries/consul_config_spec.rb b/test/spec/libraries/consul_config_spec.rb index 054e2a56..70f9e98c 100644 --- a/test/spec/libraries/consul_config_spec.rb +++ b/test/spec/libraries/consul_config_spec.rb @@ -11,10 +11,6 @@ default_attributes['consul'] = { 'service' => { 'config_dir' => '/etc/consul/conf.d' - }, - 'config' => { - 'owner' => 'root', - 'group' => 'consul' } } end @@ -23,6 +19,7 @@ context 'sets options directly' do recipe do consul_config '/etc/consul/default.json' do + owner 'root' options do recursor 'foo' translate_wan_addrs true