Prevent consul from writing its own config

This fixes issue sous-chefs#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

@@ -41,6 +41,8 @@ suites:
     attributes:
       consul:
         config: &default-config
+          owner: root
+          group: consul
           bootstrap: true
           server: true
           datacenter: FortMeade

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
 

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'

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

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

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

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'

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'

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

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