(FM-2303) Future feature release, install_switches param enhancements #83

cyberious · 2015-03-24T16:05:16Z

Fixed some issues around sqlserver instance and features and enhanced some of the ways which we detect what is installed.

Iristyle · 2015-04-03T21:30:06Z

spec/acceptance/sqlserver_instance_spec.rb

+      let(:beaker_host) { beaker_host }
+      let(:sqlserver_version) { version }
+      let(:sqlserver_iso) { version =~ /2012/ ? SQL_2012_ISO : SQL_2014_ISO }
+      it_behaves_like 'server_prefetch'


Since server_prefetch and install sqlserver have no assertions, why are we using it_behaves_like?

I realize that you want to share setup steps, but why do we have to jam them into rspec constructs like it_behave_likes? This feels like an abuse of the system to run arbitrary code.... why not just have a plain old ruby method to call that will execute the manifest required to install SQL Server?

I don't believe that using a shared context is the best approach. I think that shared context can be difficult to debug. I would prefer to refactor these out into helper methods. The simpler and more explicit we keep the tests the easier it will be for any QA/QE team member to maintain them.

@Iristyle I was going to add some checks in there to ensure we have a starting good point, it is better to catch those failures fast and ahead of time such as did the drive get created as prefetch and is the executable there.

@zreichert I agree that they can get complex but I believe no less complex than methods that could take upwards of 5 parameters. Also do you want to keep passing those variables to each one or time you pass them. For prefetch I can see your point but thinking more in the general concept such as for the sqlcmd_query shared context which will be shared amongst pretty much every test of a sql object.

You can still throw early in your helpers -- is there some value that it_behaves_like adds here that a plain old Ruby method doesn't? I think using it_behaves_like obfuscates what you're really doing (executing some procedural steps to get SQL Installed for the sake of other tests).

FWIW, I think explicitly passing some variables inside a test (rather than them getting picked up from global state) is less complex b/c it makes the test more explicit. IMHO, the less shared setup for a suite of tests the better. Again, this lends itself to later maintenance / understanding of the tests.

The author of NUnit, then later XUnit.net has a really good blog post on the perils of complex setup / teardown that you might want to take a look at - http://jamesnewkirk.typepad.com/posts/2007/09/why-you-should-.html

cyberious · 2015-04-06T20:02:27Z

@Iristyle @zreichert updated.

Iristyle · 2015-04-06T23:12:56Z

@cyberious I think that @zreichert is going to take over on the acceptance testing front, so let's keep this PR up to bae4ba0 only... I'll have some more comments on actual implementation shortly.

Iristyle · 2015-04-06T23:23:11Z

lib/facter/sqlserver_hash.rb

+ }
+}
+Write-Host (ConvertTo-Json $sqlserver)
+    DISCOVERY


This whole approach makes me a bit 🐼

I would like to see this all in Ruby code if we can manage it:

We pay the Powershell initialization tax (and this code assumes powershell.exe is in the path - are you sure this PS code behaves the same on 32-bit and 64-bit PowerShell?)

ConvertTo-Json is Powershell 3+ (.NET 4) - which rules out Windows 7 / 2008R2 as a target installation platform.

Writing code to disk and then executing can be a bad idea from a security standpoint

+1 on not introducing unintentional security issues

We already have this as powershell, this is a move from puppet_x/sqlserver/server_helper.rb into a fact instead. A rewrite of this will take significant more work. Also we ONLY work with 2012+ for this module. It is not supported for anything else.

Also this is a tempfile that is thrown away, the attack vector for this is extremely hard to pull off. There would have to be bad data in the registry values we are reading in order for this to be an issue but at that point you are already in a heap of trouble.

http://www.dwheeler.com/secure-class/Secure-Programs-HOWTO/avoid-race.html

You can create multiple ps1 files on single one that outputs multiple key value pairs. Even if you create one file per fact it's no different than what you are doing now, executing powershell multiple times...

This will be a single powershell execution. Again I want to know a concrete example of security vulnerability with a tempfile execution. Otherwise it it worth the overhead to refactor this code into multiple powershell facts as this is very critical to sqlserver_instance and sqlserver_features provider.

The security vulnerability is a well known race condition referred to as time of check to time of use (http://en.wikipedia.org/wiki/Time_of_check_to_time_of_use). In this context, an attacker has a window of opportunity to modify the contents of the temp file contents after you close the file handle and before powershell starts executing. As a result, it can lead to arbitrary code execution, which is significant given that the puppet agent typically runs as LocalSystem.

Currently you are assuming no one else has permission to write to the tempfile, e.g. a regular User, but you're not explicitly managing file permissions, and you know what they say about assuming.

The powershell module avoids this by never closing the file handle. So no other process can open a handle to the file for writing.

Reading the file as stdin will never work as we would be escaping and not possible for the file. Can facter read another file from install location reliably where the file would be hard coded and we execute the powershell from that file instead of a tempfile? Permissions would then be set and would remove that possible security attack.

@joshcooper @ferventcoder what are your thoughts around instead, locking the file before release to read only and only Admin privileges? I have the report working here, but running arbitrary sql is of the same boat. We need to be able to create temporary files for sqlcmd to execute, but if it is process locked it will fail on it's face again and again. Invoke-Sqlcmd cmdlet does the same thing.

zreichert · 2015-04-10T22:48:35Z

@cyberious I have PR'd #88 and merged in 911f1d3 and beyond. Can you please remove those form this PR.

cyberious · 2015-04-20T05:19:32Z

@zreichert I removed those commits from this.

ferventcoder · 2015-04-21T18:55:20Z

lib/facter/sqlserver_hash.rb

+    ps_args = '-NoProfile -NonInteractive -NoLogo -ExecutionPolicy Bypass'
+
+    discovery = <<-DISCOVERY
+Import-Module 'C:\\Program Files\\Microsoft SQL Server\\.puppet\\sqlserver.psm1';


Good point, this is targeted for 2012, it was copy past from powershell.

I don't understand why we're using .puppet in the path here.

No particular reason. We are storing the config and powershell module that we are using in this location. I would prefer it to not be hidden in the Microsoft SQL Server path. This is at least logical to what it is in this directory.

I still don't understand. IMHO, we should never be placing files inside of another applications Program Files directory, because it will cause files and directories to be left over after an uninstall.

Do you have a suggested location, I am open to viable solutions

Are you using a file resource to pull the psm1 due to permissions issues?

Due to process locking yes, and it allows us to keep it more simplistic I believe, and ensure we are locking that file down, more things will be added to the module to help manage the sqlserver instance.

@Iristyle would you prefer storing in "C:\ProgramData\PuppetLabs"

I think we should fix the underlying problem rather than employ work-arounds shuffling files around with the file resource, applying perms, etc. Again, I'm also not sure why we can't just grab WMI data for the facts we actually need.

I'd like to unwind this is a bit to figure out which facts are actually critical to return in your current sqlserver.psm1. If we can use WMI to capture the same info then:

we don't introduce a PowerShell dependency

we don't incur the perf hit of calling PowerShell

we don't have to worry about adjusting perms / file locking issues on a temp PS file

the code is therefore easier to maintain

Let's discuss later today.

cyberious · 2015-04-22T16:20:36Z

@Iristyle the reason for the is_svrrolemember change is due to behavior does not always report back if it can't reach the AD.

Add init class to insert file required to run sqlserver discovery Refactor discovery to be ran with facter instead of manually by provider, this will help to expose other facts to different defines and providers on the system

cyberious · 2015-04-29T23:33:19Z

@Iristyle removed the sqlcmd logic in favor of #96 and moving away from sqlcmd.exe

cyberious · 2015-05-04T18:05:33Z

Working to go with WMI instead of powershell, closing PR

cyberious changed the title ~~(DO NOT MERGE) Future feature release, install_switches param enhancements~~ (FM-2303) Future feature release, install_switches param enhancements Apr 2, 2015

Iristyle reviewed Apr 3, 2015
View reviewed changes

Iristyle reviewed Apr 6, 2015
View reviewed changes

ferventcoder mentioned this pull request Apr 7, 2015

Edits to grammar, markdown, and format to match current styleguide. puppetlabs/puppetlabs-powershell#50

Merged

cyberious mentioned this pull request Apr 9, 2015

(DO NOT MERGE) Test/cyberious release/fm 2405 sqlserver login #86

Closed

zreichert mentioned this pull request Apr 13, 2015

Test/release/fm 2405 sqlserver login #88

Closed

ferventcoder reviewed Apr 21, 2015
View reviewed changes

cyberious closed this May 4, 2015

Iristyle mentioned this pull request May 27, 2015

(FM-2303, FM-2790, FM-2445) WMI / Registry impl of Discovery Report #116

Merged

(FM-2303) Future feature release, install_switches param enhancements #83

(FM-2303) Future feature release, install_switches param enhancements #83

Uh oh!

Conversation

cyberious commented Mar 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyberious commented Apr 6, 2015

Uh oh!

Iristyle commented Apr 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zreichert commented Apr 10, 2015

Uh oh!

cyberious commented Apr 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyberious commented Apr 22, 2015

Uh oh!

cyberious commented Apr 29, 2015

Uh oh!

cyberious commented May 4, 2015

Uh oh!

Uh oh!