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

Fixing issue with security policy always returning nil #321

Merged
merged 6 commits into from
Dec 17, 2015

Conversation

jeremymv2
Copy link
Contributor

All of the Security Policies were evaluating to nil.
This fixes the issues and allows it to work correctly.
I imagine this wasn't caught earlier due to a file artifact (win_secpol.cfg) being left around from a manual execution of the secedit export command.

Here is an example of what happens when trying to evaluate any security_policy:

Failures:

  1) Security Policy EnableAdminAccount should eq 1
     Failure/Error: its('EnableAdminAccount') { should eq 1 }

       expected: 1
            got: nil

       (compared using ==)
     # ./test/integration/default/guest_account_spec.rb:5:in `block (3 levels) in load'
     # /Users/jmiller/.chefdk/gem/ruby/2.1.0/gems/kitchen-inspec-0.9.6/lib/kitchen/verifier/inspec.rb:55:in `call'

Finished in 0.91339 seconds (files took 0.70664 seconds to load)
8 examples, 1 failure

Failed examples:

rspec  # Security Policy EnableAdminAccount should eq 1

@loaded = true

# delete temp file
cmd = inspec.command('Remove-Item win_secpol.cfg')
Copy link
Contributor

Choose a reason for hiding this comment

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

Please ensure that we delete it before we return. Even if we have an error, we should at least try to the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You got it. I'll get that in.

@chris-rock
Copy link
Contributor

@jeremymv2 thanks for bringing this up. I like to move to Get-Content instead of type. Lets just improve it to delete files even if something went wrong. Which OS have you used for testing?

@jeremymv2
Copy link
Contributor Author

@chris-rock I'm testing with:

platforms:
  - name: win-2012r2-standard-amd64-nocm
    driver:
      box: opentable/win-2012r2-standard-amd64-nocm
      box_url: https://atlas.hashicorp.com/opentable/boxes/win-2012r2-standard-amd64-nocm/versions/1.0.0/providers/virtualbox.box

@jeremymv2
Copy link
Contributor Author

@chris-rock Added an ensure block to always delete the file. Tested and works on a windows 2012R2 image.

@chris-rock
Copy link
Contributor

@jeremymv2 that is great. Could you do me one favor and add windows platform as a separate PR to our integration tests https://github.com/chef/inspec/blob/master/test/integration/.kitchen.yml?

On top of this PR I would like to have an integration test added as well to ensure we do not miss this again. Let me know if you need help.

@chris-rock
Copy link
Contributor

@jeremymv2 One idea that came up in my head: At the point we wrote this resource we had no script resource available in InSpec. Now, it may be easier to just write a simple powershell script. Similar to https://github.com/chef/inspec/blob/f092ba3ac3790d074fc0a458cc4f5e3dc4d4e54d/lib/resources/registry_key.rb#L93-L108

That would reduce the calls to one request instead of three. What do you think?

@chris-rock chris-rock added the Type: Bug Feature not working as expected label Dec 15, 2015
@jeremymv2
Copy link
Contributor Author

@chris-rock added a simple integration test for security_policy which should catch this root issue if it occurs again in the future.

A couple of other things:
1.) Although the idea of embedded powershell scripts sometimes seems appealing, in general I find that it adds too much obfuscation when more than just a one or two-liner. Powershell is just hard to read and requires the reader to context switch heavily.
2.) There are several things that aren't working integration-wise for windows. One example is the json_yaml_csv_ini.rb recipe doesn't support the windows platform so it prevents a successful converge. Many of the of the inspec tests assume the platform is Linux so they run on Windows and fail. I didn't want to include all those fixes in this pull request. I will be glad to add those items in other PRs.

@chris-rock
Copy link
Contributor

@jeremymv2 Great work and thanks for the improvement. Regarding the integration tests: we started some work in #314 to make this work for Windows. I invite you to contribute more tests and especially to improve the Windows support.

@chris-rock
Copy link
Contributor

When travis shows green light, I am going to merge it. Please rebase your improvements on the latest master.

chris-rock added a commit that referenced this pull request Dec 17, 2015
Fixing issue with security policy always returning nil
@chris-rock chris-rock merged commit ca33ac9 into inspec:master Dec 17, 2015
@jeremymv2 jeremymv2 deleted the security_policy_fixes branch December 17, 2015 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Feature not working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants