-
Notifications
You must be signed in to change notification settings - Fork 133
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
Feature/allow setting template source #196
Feature/allow setting template source #196
Conversation
Ok - what did I miss? All of the issues listed by Travis are in files that I haven't touched. |
Ok, so the unit test failures are because of the version string in metadata.rb for the sysctl cookbook. Because it is using '>= 0.10.0' and because they released 1.0.x which removed the apply recipe as they mentioned in another comment, the unit tests are failing now. I'd suggest using the rocket syntax, is there any reason (specific to this dev-sec cookbook) to use '>=' vs. '>~'? |
I added the rocket syntax to eliminate the ChefSpec unit test issues and the tests are passing now. The integration tests fail since I don't have a DO account. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eyespies thanks for this PR! it makes definitely sense.
Can I also ask you to add the documentation on the new attributes to the README.md?
metadata.rb
Outdated
@@ -33,7 +33,7 @@ | |||
supports 'redhat', '>= 5.0' | |||
supports 'oracle', '>= 6.4' | |||
|
|||
depends 'sysctl', '>= 0.10.0' | |||
depends 'sysctl', '~> 0.10' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eyespies this is already fixed in master :) Could you please rebase your PR?
metadata.rb
Outdated
@@ -22,7 +22,7 @@ | |||
license 'Apache-2.0' | |||
description 'Installs and configures operating system hardening' | |||
long_description IO.read(File.join(File.dirname(__FILE__), 'README.md')) | |||
version '3.0.0' | |||
version '3.0.1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please do not bump the version, this is done prior to the release by us
recipes/pam.rb
Outdated
@@ -48,8 +48,14 @@ | |||
end | |||
|
|||
# configure passwdqc via central module: | |||
template_cookbook = if node['os-hardening']['auth']['pam'].attribute?('passwdqc') && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if this is really a proper way to do that, we have all this attributes configured in the default level. So its safe to assume that they exist, I would suggest to avoid the checks:
template_cookbook = node['os-hardening']['auth']['pam']['passwdqc']['template_cookbook']
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to ask for clarification - do you want me to set the default cookbook name used for the attribute in the attributes file? Or is it sufficient to simply remove the check for the ['pam']
attribute but leave the check for the passwdqc and other attributes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! We can specify the default cookbook name and avoid additional variables and conditions.
recipes/pam.rb
Outdated
template passwdqc_path do | ||
source 'pam_passwdqc.erb' | ||
cookbook template_cookbook unless template_cookbook.nil? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cookbook template_cookbook if template_cookbook
?
attributes/default.rb
Outdated
@@ -76,6 +76,9 @@ | |||
default['os-hardening']['auth']['pam']['passwdqc']['options'] = 'min=disabled,disabled,16,12,8' | |||
default['os-hardening']['auth']['pam']['cracklib']['options'] = 'try_first_pass retry=3 type=' | |||
default['os-hardening']['auth']['pam']['pwquality']['options'] = 'try_first_pass retry=3 type=' | |||
default['os-hardening']['auth']['pam']['tally2']['template_cookbook'] = 'os-security' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use cookbook_name as a value here, e.g:
.... = cookbook_name
Nevermind @artem-sidorenko, This was also confirmed with an integration test which shows the error happening in the context of Chef::Node::Attribute and not Chef::Recipe:
Let me know if there are any other ways to get the cookbook name or if I should just hard code the cookbook name. |
Thanks! I wasn’t aware of that, probably I was lucky enough to use cookbook_name only in recipes in the past. Feel free to hardcode it to os-hardening |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eyespies many thanks!
General question @artem-sidorenko - when do you release a new version? Because I use Chef Server, if the version is not bumped, I have to |
@eyespies I plan to resolve/merge all other currently open MRs and then release a new minor version: auditd support, container support etc. It might take another week or so. Is it okay for you? |
Sure, that is definitely reasonable. Thank you! |
This update allows users to specify an alternate source cookbook for the PAM templates. I needed this because I use FreeIPA, which updates the PAM configuration. When running the dev-sec cookbook, the IPA changes are overwritten.
I had two choices when making the update
I chose the second option as it involves lower risk to existing systems / settings. For unit testing, there are no tests related to these templates, so I did not add tests / make changes. I would be willing to add those if needed, however.
For integration testing, I ran the tests to confirm existing functionality remains unchanged, however I only did so on my local as I don't have a DO account to run the full gamut of tests. I'm open to alternative means of running the integration tests.
To truly run integration tests fully, I would need to add two things:
I wasn't sure if the team wanted to add all those extra pieces, so I haven't done that yet, but if so desired, I could add the new suite and test cookbook.