Add Support for Extra Configuration Options #173

Merged
merged 1 commit into from Apr 10, 2017

Conversation

Projects
None yet
4 participants
@bdwyertech
Contributor

bdwyertech commented Mar 31, 2017

Add support for extra configuration options at the end of the config files for override support.

This assists with the flexibility issue discussed in #89

# => Example Extra Crap
default['ssh-hardening']['ssh']['server']['extras'].tap do |extra|
  extra['#Some Comment'] = 'Heres the Comment'
  extra['AuthenticationMethods'] =  'publickey,keyboard-interactive'
end

The block syntax swap is somewhat opinionated, but is more readable for long hash's

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 31, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 917c9a3 on bdwyertech:extras into d4dc236 on dev-sec:master.

Coverage Status

Coverage remained the same at 100.0% when pulling 917c9a3 on bdwyertech:extras into d4dc236 on dev-sec:master.

@artem-sidorenko

@bdwyertech thanks for this PR! It makes definitely sense, open points from my POV:

  • Documentation in README.md
  • Tests - if something is configured and not, please see GH-155 if you want an example
  • See the suggestions on the implementation in the template and attributes, this would allow to have such tests
attributes/default.rb
@@ -1,4 +1,5 @@
# encoding: utf-8
+# rubocop: disable BlockLength

This comment has been minimized.

@artem-sidorenko

artem-sidorenko Mar 31, 2017

Member

Can we have it for a particular block and not for the entire file?

@artem-sidorenko

artem-sidorenko Mar 31, 2017

Member

Can we have it for a particular block and not for the entire file?

This comment has been minimized.

@bdwyertech

bdwyertech Mar 31, 2017

Contributor

Yeah, I can swap that to just cover the two blocks

@bdwyertech

bdwyertech Mar 31, 2017

Contributor

Yeah, I can swap that to just cover the two blocks

-# http://undeadly.org/cgi?action=article&sid=20160114142733
-default['ssh-hardening']['ssh']['client']['roaming'] = false
-default['ssh-hardening']['ssh']['client']['send_env'] = ['LANG', 'LC_*', 'LANGUAGE']
+default['ssh-hardening']['ssh']['client'].tap do |client|

This comment has been minimized.

@artem-sidorenko

artem-sidorenko Mar 31, 2017

Member

.tap is a cool idea and improvement :-)

@artem-sidorenko

artem-sidorenko Mar 31, 2017

Member

.tap is a cool idea and improvement :-)

attributes/default.rb
+ client['send_env'] = ['LANG', 'LC_*', 'LANGUAGE']
+
+ # Extra Client Configuration Options
+ client['extras'].tap do |extra|

This comment has been minimized.

@artem-sidorenko

artem-sidorenko Mar 31, 2017

Member

I think we should have this in the documentation (and maybe add an example chapter/code 'how to add extra options'). As we do not use/fill it with defaults, I would expect to have only client['extras'] = {} here

@artem-sidorenko

artem-sidorenko Mar 31, 2017

Member

I think we should have this in the documentation (and maybe add an example chapter/code 'how to add extra options'). As we do not use/fill it with defaults, I would expect to have only client['extras'] = {} here

templates/default/openssh.conf.erb
@@ -117,3 +117,8 @@ UseRoaming <%= @node['ssh-hardening']['ssh']['client']['roaming'] ? 'yes' : 'no'
# Send locale environment variables
SendEnv <%= @node['ssh-hardening']['ssh']['client']['send_env'].join(' ') %>
<% end %>
+
+# Extra Configuration Options

This comment has been minimized.

@artem-sidorenko

artem-sidorenko Mar 31, 2017

Member

Maybe we should do here something like

<% if @node['ssh-hardening']['ssh']['client']['extras'].empty? %>
# Extra Configuration Options
<%- @node['ssh-hardening']['ssh']['client']['extras'].each do |key, value| %>
<%= key %> <%= value %>
<% end -%>
<% end -%>

This would allow us two tests:

  • Test if something configured - additional option will be added and '# Extra Configuration Options' is included in the file
  • Test if nothing is configured - there should be no '# Extra Configuration Options' in the file
@artem-sidorenko

artem-sidorenko Mar 31, 2017

Member

Maybe we should do here something like

<% if @node['ssh-hardening']['ssh']['client']['extras'].empty? %>
# Extra Configuration Options
<%- @node['ssh-hardening']['ssh']['client']['extras'].each do |key, value| %>
<%= key %> <%= value %>
<% end -%>
<% end -%>

This would allow us two tests:

  • Test if something configured - additional option will be added and '# Extra Configuration Options' is included in the file
  • Test if nothing is configured - there should be no '# Extra Configuration Options' in the file
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 31, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 741635f on bdwyertech:extras into d4dc236 on dev-sec:master.

Coverage Status

Coverage remained the same at 100.0% when pulling 741635f on bdwyertech:extras into d4dc236 on dev-sec:master.

@bdwyertech

This comment has been minimized.

Show comment
Hide comment
@bdwyertech

bdwyertech Mar 31, 2017

Contributor

Should be good to go @artem-sidorenko

I am traveling and having trouble getting the coveralls gem to install, but here is what I think a test might look like:

  context 'without custom extraconfig value' do
    cached(:chef_run) do
      ChefSpec::ServerRunner.new do |node|
        node.normal['ssh-hardening']['ssh']['server']['extras'] = {}
      end.converge(described_recipe)
    end

    it 'does not have any extraconfig options' do
      expect(chef_run).to render_file('/etc/ssh/sshd_config').
        without_content(/^# Extra Configuration Options/)
    end
  end

  context 'with custom extraconfig value' do
    cached(:chef_run) do
      ChefSpec::ServerRunner.new do |node|
        node.normal['ssh-hardening']['ssh']['server']['extras']['#ExtraConfig'] = 'Value'
      end.converge(described_recipe)
    end

    it 'uses the extraconfig attributes' do
      expect(chef_run).to render_file('/etc/ssh/sshd_config').
        with_content(/^# Extra Configuration Options/).
        with_content(/^#ExtraConfig Value/)
    end
  end
Contributor

bdwyertech commented Mar 31, 2017

Should be good to go @artem-sidorenko

I am traveling and having trouble getting the coveralls gem to install, but here is what I think a test might look like:

  context 'without custom extraconfig value' do
    cached(:chef_run) do
      ChefSpec::ServerRunner.new do |node|
        node.normal['ssh-hardening']['ssh']['server']['extras'] = {}
      end.converge(described_recipe)
    end

    it 'does not have any extraconfig options' do
      expect(chef_run).to render_file('/etc/ssh/sshd_config').
        without_content(/^# Extra Configuration Options/)
    end
  end

  context 'with custom extraconfig value' do
    cached(:chef_run) do
      ChefSpec::ServerRunner.new do |node|
        node.normal['ssh-hardening']['ssh']['server']['extras']['#ExtraConfig'] = 'Value'
      end.converge(described_recipe)
    end

    it 'uses the extraconfig attributes' do
      expect(chef_run).to render_file('/etc/ssh/sshd_config').
        with_content(/^# Extra Configuration Options/).
        with_content(/^#ExtraConfig Value/)
    end
  end
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 31, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling e65f964 on bdwyertech:extras into d4dc236 on dev-sec:master.

Coverage Status

Coverage remained the same at 100.0% when pulling e65f964 on bdwyertech:extras into d4dc236 on dev-sec:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 31, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling e65f964 on bdwyertech:extras into d4dc236 on dev-sec:master.

Coverage Status

Coverage remained the same at 100.0% when pulling e65f964 on bdwyertech:extras into d4dc236 on dev-sec:master.

attributes/default.rb
+ client['send_env'] = ['LANG', 'LC_*', 'LANGUAGE']
+
+ # extra client configuration options
+ client['extras'].tap = {}

This comment has been minimized.

@artem-sidorenko

artem-sidorenko Mar 31, 2017

Member

@bdwyertech .tap ;-)

templates/default/openssh.conf.erb
@@ -117,3 +117,10 @@ UseRoaming <%= @node['ssh-hardening']['ssh']['client']['roaming'] ? 'yes' : 'no'
# Send locale environment variables
SendEnv <%= @node['ssh-hardening']['ssh']['client']['send_env'].join(' ') %>
<% end %>
+
+<%- unless Array(@node['ssh-hardening']['ssh']['client']['extras']).empty? %>

This comment has been minimized.

@artem-sidorenko

artem-sidorenko Mar 31, 2017

Member

@bdwyertech you don't have to use Array(), the value is defined as Hash in the default attributes. You can do unless @node[...].empty?

@artem-sidorenko

artem-sidorenko Mar 31, 2017

Member

@bdwyertech you don't have to use Array(), the value is defined as Hash in the default attributes. You can do unless @node[...].empty?

@artem-sidorenko

This comment has been minimized.

Show comment
Hide comment
@artem-sidorenko

artem-sidorenko Mar 31, 2017

Member

@bdwyertech looks great, some suggestions:

  describe 'extra configuration values' do # <---- describe would be a cool container here

  context 'without custom extra config value' do
    cached(:chef_run) do
      ChefSpec::ServerRunner.new.converge(described_recipe) # <----- you can work with defaults here
    end

    it 'does not have any extra config options' do
      expect(chef_run).to render_file('/etc/ssh/sshd_config').
        without_content(/^# Extra Configuration Options/)
    end
  end

  context 'with custom extra config value' do
    cached(:chef_run) do
      ChefSpec::ServerRunner.new do |node|
        node.normal['ssh-hardening']['ssh']['server']['extras']['#ExtraConfig'] = 'Value'
      end.converge(described_recipe)
    end

    it 'uses the extra config attributes' do
      expect(chef_run).to render_file('/etc/ssh/sshd_config').with_content(/^# Extra Configuration Options/)
      expect(chef_run).to render_file('/etc/ssh/sshd_config').with_content(/^#ExtraConfig Value/) # <----- as far I remember chefspec has some problems with chained matchers (`.with_content.with_content`), so its better to have two expects
    end
  end

  end

Member

artem-sidorenko commented Mar 31, 2017

@bdwyertech looks great, some suggestions:

  describe 'extra configuration values' do # <---- describe would be a cool container here

  context 'without custom extra config value' do
    cached(:chef_run) do
      ChefSpec::ServerRunner.new.converge(described_recipe) # <----- you can work with defaults here
    end

    it 'does not have any extra config options' do
      expect(chef_run).to render_file('/etc/ssh/sshd_config').
        without_content(/^# Extra Configuration Options/)
    end
  end

  context 'with custom extra config value' do
    cached(:chef_run) do
      ChefSpec::ServerRunner.new do |node|
        node.normal['ssh-hardening']['ssh']['server']['extras']['#ExtraConfig'] = 'Value'
      end.converge(described_recipe)
    end

    it 'uses the extra config attributes' do
      expect(chef_run).to render_file('/etc/ssh/sshd_config').with_content(/^# Extra Configuration Options/)
      expect(chef_run).to render_file('/etc/ssh/sshd_config').with_content(/^#ExtraConfig Value/) # <----- as far I remember chefspec has some problems with chained matchers (`.with_content.with_content`), so its better to have two expects
    end
  end

  end

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 6, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 1de5714 on bdwyertech:extras into d4dc236 on dev-sec:master.

Coverage Status

Coverage remained the same at 100.0% when pulling 1de5714 on bdwyertech:extras into d4dc236 on dev-sec:master.

@bdwyertech

This comment has been minimized.

Show comment
Hide comment
@bdwyertech

bdwyertech Apr 6, 2017

Contributor

Should be good to go @artem-sidorenko

Thanks for the lesson in ChefSpec! Right now I'm only using Inspec

Contributor

bdwyertech commented Apr 6, 2017

Should be good to go @artem-sidorenko

Thanks for the lesson in ChefSpec! Right now I'm only using Inspec

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 6, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling d8cd272 on bdwyertech:extras into d4dc236 on dev-sec:master.

Coverage Status

Coverage remained the same at 100.0% when pulling d8cd272 on bdwyertech:extras into d4dc236 on dev-sec:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 6, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling d8cd272 on bdwyertech:extras into d4dc236 on dev-sec:master.

Coverage Status

Coverage remained the same at 100.0% when pulling d8cd272 on bdwyertech:extras into d4dc236 on dev-sec:master.

@@ -124,6 +124,24 @@ Configure attributes:
This will enable the SFTP Server and chroot every user in the `sftpusers` group to the `/home/sftp/%u` directory.
+## Extra Configuration Options

This comment has been minimized.

@artem-sidorenko

artem-sidorenko Apr 7, 2017

Member

@bdwyertech cool, thanks for adding this to the docs. Can you please add something like

....
* `['ssh-hardening']['ssh']['server']['extras']` - `{}`. Add extra configuration options, see [below](#extra-configuration-options) for details
....
* `['ssh-hardening']['ssh']['client']['extras']` - `{}`. Add extra configuration options, see [below](#extra-configuration-options) for details

to the attribute documentation above?

Can you do me a favor and cleanup a commit history by squashing the commits?

Then we can merge it :-)

@artem-sidorenko

artem-sidorenko Apr 7, 2017

Member

@bdwyertech cool, thanks for adding this to the docs. Can you please add something like

....
* `['ssh-hardening']['ssh']['server']['extras']` - `{}`. Add extra configuration options, see [below](#extra-configuration-options) for details
....
* `['ssh-hardening']['ssh']['client']['extras']` - `{}`. Add extra configuration options, see [below](#extra-configuration-options) for details

to the attribute documentation above?

Can you do me a favor and cleanup a commit history by squashing the commits?

Then we can merge it :-)

@artem-sidorenko

This comment has been minimized.

Show comment
Hide comment
@artem-sidorenko

artem-sidorenko Apr 7, 2017

Member

@atomic111 any remarks?

Member

artem-sidorenko commented Apr 7, 2017

@atomic111 any remarks?

Add support for extra configuration options and improve readability w…
…ith block syntax

Signed-off-by: Brian Dwyer <bdwyer@IEEE.org>
@bdwyertech

This comment has been minimized.

Show comment
Hide comment
@bdwyertech

bdwyertech Apr 7, 2017

Contributor

Alrighty @artem-sidorenko , I think it's finally good!

Contributor

bdwyertech commented Apr 7, 2017

Alrighty @artem-sidorenko , I think it's finally good!

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 7, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 17194d7 on bdwyertech:extras into d4dc236 on dev-sec:master.

Coverage Status

Coverage remained the same at 100.0% when pulling 17194d7 on bdwyertech:extras into d4dc236 on dev-sec:master.

@artem-sidorenko

@bdwyertech many thanks for very good collaboration! :)

@atomic111 LGTM

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 7, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 925f81e on bdwyertech:extras into d4dc236 on dev-sec:master.

Coverage Status

Coverage remained the same at 100.0% when pulling 925f81e on bdwyertech:extras into d4dc236 on dev-sec:master.

@atomic111

great, thanks for adding the extra config attribute

@atomic111 atomic111 merged commit eaf6c11 into dev-sec:master Apr 10, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment