Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 110 additions & 0 deletions files/sqlserver.psm1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@

function Get-RunDiscovery(){
$k = gi 'HKLM:\\Software\\Microsoft\\Microsoft SQL Server'
$sqlserver = @{}
$shared_code = ""
$ver_info = @{}
@("110","120") | %{
$version = $k.OpenSubKey($_)
if ( $version -ne $null) {
$version_info = @{}
@("SharedCode","VerSpecificRootDir") | %{
if ($version.GetValue($_,$null) -ne $null){
$version_info.Add($_, $version.GetValue($_))
}
}
if ($version_info.Count -ne 0) {
$ver_info.Add($_,$version_info)
}
}
}
$report_locations = @("C:\\Program Files\\Microsoft SQL Server\\*\\Setup Bootstrap\\Log\\*\\SqlDiscoveryReport.xml",
"C:\\Program Files (x86)\\Microsoft SQL Server\\*\\Setup Bootstrap\\Log\\*\\SqlDiscoveryReport.xml")
$sqlserver.Add('VerSpecific',$ver_info)
$ver_hash = @{'120'='SqlServer2014';'110' = 'SqlServer2012'}
$ver_hash.Keys | %{
if ($ver_info.ContainsKey($_) -and
$ver_info[$_].ContainsKey("VerSpecificRootDir")) {
$verSpecificRootDir = $sqlserver[$_].VerSpecificRootDir
}

$sqlversion = $ver_hash[$_]
if(Test-Path "$verSpecificRootDir\\Setup Bootstrap\\$sqlversion\\setup.exe"){
pushd "$verSpecificRootDir\\Setup Bootstrap\\$sqlversion\\"
Start-Process -FilePath .\\setup.exe -ArgumentList @("/Action=RunDiscovery","/q") -Wait -WindowStyle Hidden
$report_locations += "$verSpecificRootDir\\Setup Bootstrap\\Log\\*\\SqlDiscoveryReport.xml"
popd
}
elseif(Test-Path "C:\\Program Files\\Microsoft SQL Server\\$_\\Setup Bootstrap\\$sqlversion\\setup.exe"){
pushd "C:\\Program Files\\Microsoft SQL Server\\$_\\Setup Bootstrap\\$sqlversion\\"
Start-Process -FilePath .\\setup.exe -ArgumentList @("/Action=RunDiscovery","/q") -Wait -WindowStyle Hidden
popd
}
elseif(Test-Path "C:\\Program Files (x86)\\Microsoft SQL Server\\$_\\Setup Bootstrap\\$sqlversion\\setup.exe"){
pushd "C:\\Program Files (x86)\\Microsoft SQL Server\\$_\\Setup Bootstrap\\$sqlversion\\"
Start-Process -FilePath .\\setup.exe -ArgumentList @("/Action=RunDiscovery","/q") -Wait -WindowStyle Hidden
popd
}
}
$instances = @($k.GetValue("InstalledInstances"))
if ($instances -ne $null -and $instances.Count -gt 0) {
$instances | foreach {
$instancedata = @{}
if ($k.GetSubKeyNames().Contains("MSSQL12.$_")){
$subkey = $k.OpenSubKey("MSSQL12.$_")
}elseif($k.GetSubKeyNames().Contains("MSSQL11.$_")){
$subkey = $k.OpenSubKey("MSSQL11.$_")
}
@("LoginMode", "DefaultData", "DefaultLog","BackupDirectory") | % {
$value = $subkey.OpenSubKey("MSSQLServer").GetValue($_,$null)
if ($value -ne $null){
$instancedata[$_] = $value
}
}
@("Version","SQLPath","SQLBinRoot","Language","Edition","SQLGroup","Colation","SQLDataRoot") | % {
$value = $subkey.OpenSubKey("Setup").GetValue($_, $null)
if ($value -ne $null){
$instancedata[$_] = $value
}
}
$sqlserver.Add($_,$instancedata)
}
$sqlserver.Add("Instances",@($instances))
}else{
$instances = @()
}
$file = gci $report_locations -ErrorAction Ignore | sort -Descending | select -First 1
if($file -ne $null) {
[xml] $xml = cat $file
$json = $xml.ArrayOfDiscoveryInformation.DiscoveryInformation
foreach($instance in ($json | % { $_.Instance } | Get-Unique )){
$features = @()
$json | %{
if ($_.Features -eq "None"){
break
}
if($_.instance -eq $instance){
$features += $_.feature
}
}
if($instance -eq "" ){
if ($features -ne $null) {
$sqlserver.Add("Features",@($features))
}
}else{
if (!($sqlserver.ContainsKey($instance))){
$sqlserver.Add($instance,@{})
}
if ($features -ne $null){
$sqlserver[$instance].Add("Features",@($features))
}
}
}
if ($instances -eq $null -or $instances.Count -eq 0){
$sqlserver.Add("Instances",@(($json | % { $_.Instance } | Get-Unique )))
}

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

27 changes: 27 additions & 0 deletions lib/facter/sqlserver_hash.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
require 'tempfile'

Facter.add(:sqlserver_hash) do
confine :osfamily => :windows

setcode do
powershell = if File.exists?("#{ENV['SYSTEMROOT']}\\sysnative\\WindowsPowershell\\v1.0\\powershell.exe")
"#{ENV['SYSTEMROOT']}\\sysnative\\WindowsPowershell\\v1.0\\powershell.exe"
else
'powershell.exe'
end
ps_args = '-NoProfile -NonInteractive -NoLogo -ExecutionPolicy Bypass'

discovery = <<-DISCOVERY
Import-Module 'C:\\Program Files\\Microsoft SQL Server\\.puppet\\sqlserver.psm1';
Copy link
Contributor

Choose a reason for hiding this comment

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

32bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Get-RunDiscovery
DISCOVERY
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on not introducing unintentional security issues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

discovery_script = Tempfile.new(['puppet-sqlserver', '.ps1'])
discovery_script.write(discovery)
discovery_script.flush
begin
result = Facter::Core::Execution.exec("#{powershell} #{ps_args} -Command - < \"#{discovery_script.path}\"")
JSON.parse(result) unless result.empty?
rescue
end
end
end
31 changes: 31 additions & 0 deletions lib/facter/sqlserver_instances_array.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
require 'tempfile'

Facter.add(:sqlserver_instance_array) do
confine :osfamily => :windows

setcode do
powershell = if File.exists?("#{ENV['SYSTEMROOT']}\\sysnative\\WindowsPowershell\\v1.0\\powershell.exe")
"#{ENV['SYSTEMROOT']}\\sysnative\\WindowsPowershell\\v1.0\\powershell.exe"
elsif File.exists?("#{ENV['SYSTEMROOT']}\\system32\\WindowsPowershell\\v1.0\\powershell.exe")
"#{ENV['SYSTEMROOT']}\\system32\\WindowsPowershell\\v1.0\\powershell.exe"
else
'powershell.exe'
end
ps_args = '-NoProfile -NonInteractive -NoLogo -ExecutionPolicy Bypass'

instances = <<-INSTANCES
Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed, I think we want to move this into Ruby code, because launching PowerShell for the sake of enumerating a registry key is going to be total overkill.

It should be some variation on:

Win32::Registry::HKEY_LOCAL_MACHINE.open('SOFTWARE\\Microsoft\\Microsoft SQL Server\\InstalledInstances', Win32::Registry::KEY_READ) do
    # read values
end

Another alternative is to use WMI to query over the namespace root\Microsoft\SqlServer\ComputerManagement10 like I mention in https://github.com/puppetlabs/puppetlabs-sqlserver/pull/83/files#r28835387

Note that I don't have that reg key locally, but if we need to enumerate keys and are concerned about Puppet 4, then we should follow the approach from puppetlabs/puppetlabs-registry@a31b0f9#diff-49264ae9265dc4f64a667cdbfa903b95R193

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is going to be removed, with recent discoveries x86 doesn't even populate the same registry keys as the 64bit version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, so if you use WMI instead you shouldn't have that problem. WMI is the agnostic way to get at the instances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, not that the keys aren't accessible, they don't exist, I have opened up the regedit to ensure I am not missing something, they are not creating them with the 32 bit version.

$k = gi 'HKLM:\\SOFTWARE\\Microsoft\\Microsoft SQL Server'
$values = $k.GetValue('InstalledInstances')
$instances = $values -join ','
Write-Host $instances
INSTANCES
tempfile = Tempfile.new(['instances', '.ps1'])
tempfile.write(instances)
tempfile.flush
begin
result = Facter::Core::Execution.exec("#{powershell} #{ps_args} -Command - < \"#{tempfile.path}\"")
JSON.parse(result) unless result.empty?
rescue
end
end
end
41 changes: 0 additions & 41 deletions lib/puppet/provider/sqlserver.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,47 +78,6 @@ def not_nil_and_not_empty?(obj)
!obj.nil? and !obj.empty?
end

def self.run_discovery_script
discovery = <<-DISCOVERY
if(Test-Path 'C:\\Program Files\\Microsoft SQL Server\\120\\Setup Bootstrap\\SQLServer2014\\setup.exe'){
pushd 'C:\\Program Files\\Microsoft SQL Server\\120\\Setup Bootstrap\\SQLServer2014\\'
Start-Process -FilePath .\\setup.exe -ArgumentList @("/Action=RunDiscovery","/q") -Wait -WindowStyle Hidden
popd
}elseif(Test-Path 'C:\\Program Files\\Microsoft SQL Server\\110\\Setup Bootstrap\\SQLServer2012\\setup.exe'){
pushd 'C:\\Program Files\\Microsoft SQL Server\\110\\Setup Bootstrap\\SQLServer2012\\'
Start-Process -FilePath .\\setup.exe -ArgumentList @("/Action=RunDiscovery","/q") -Wait -WindowStyle Hidden
popd
}

$file = gci 'C:\\Program Files\\Microsoft SQL Server\\*\\Setup Bootstrap\\Log\\*\\SqlDiscoveryReport.xml' -ErrorAction Ignore | sort -Descending | select -First 1
if($file -ne $null) {
[xml] $xml = cat $file
$json = $xml.ArrayOfDiscoveryInformation.DiscoveryInformation
$hash = @{"instances" = @();"TimeStamp"= ("{0:yyyy-MM-dd HH:mm:ss}" -f $file.CreationTime)}
foreach($instance in ($json | % { $_.Instance } | Get-Unique )){
$features = @()
$json | %{
if($_.instance -eq $instance){
$features += $_.feature
}
}
if($instance -eq "" ){
$hash.Add("Generic Features",$features)
}else{
$hash["instances"] += $instance
$hash.Add($instance,@{"features"=$features})
}
}
$file.Directory.Delete($true)
Write-Host (ConvertTo-Json $hash)
}else{
Write-host ("{}")
}
DISCOVERY
result = powershell([discovery])
JSON.parse(result)
end

def self.run_install_dot_net
install_dot_net = <<-DOTNET
Install-WindowsFeature NET-Framework-Core
Expand Down
8 changes: 4 additions & 4 deletions lib/puppet/provider/sqlserver_features/mssql.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@

def self.instances
instances = []
jsonResult = Puppet::Provider::Sqlserver.run_discovery_script
debug "Parsing json result #{jsonResult}"
if jsonResult.has_key?('Generic Features')
sqlserver_hash = Facter.value(:sqlserver_hash)
debug "Parsing json result #{sqlserver_hash}"
if sqlserver_hash != nil && sqlserver_hash.has_key?('Features')
existing_instance = {:name => "Generic Features",
:ensure => :present,
:features =>
PuppetX::Sqlserver::ServerHelper.translate_features(
jsonResult['Generic Features']).sort!
sqlserver_hash['Features']).sort!
}
debug "Parsed features = #{existing_instance[:features]}"

Expand Down
21 changes: 5 additions & 16 deletions lib/puppet/provider/sqlserver_instance/mssql.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,15 @@

def self.instances
instances = []
jsonResult = Puppet::Provider::Sqlserver.run_discovery_script
debug "Parsing json result #{jsonResult}"
if jsonResult.has_key?('instances')
jsonResult['instances'].each do |instance_name|
sqlserver_hash = Facter.value(:sqlserver_hash)
debug "Parsing json result #{sqlserver_hash}"
if sqlserver_hash != nil && sqlserver_hash.has_key?('Instances')
sqlserver_hash['Instances'].each do |instance_name|
existing_instance = {:name => instance_name,
:ensure => :present,
:features =>
PuppetX::Sqlserver::ServerHelper.translate_features(
jsonResult[instance_name]['features']).sort!
sqlserver_hash[instance_name]['Features']).sort!
}
instance = new(existing_instance)
instances << instance
Expand Down Expand Up @@ -70,17 +70,6 @@ def create
else
installNet35
add_features(@resource[:features])
# cmd_args = build_cmd_args(@resource[:features])
# begin
# config_file = create_temp_for_install_switch
# cmd_args << "/ConfigurationFile=\"#{config_file.path}\"" unless config_file.nil?
# try_execute(cmd_args)
# ensure
# if config_file
# config_file.close
# config_file.unlink
# end
# end
end
end

Expand Down
1 change: 1 addition & 0 deletions lib/puppet/type/sqlserver_features.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
Puppet::Type::newtype(:sqlserver_features) do
ensurable

autorequire('class') { 'sqlserver' }

newparam(:name, :namevar => true) do
#Due to our prefetch and unaware of what name the user will provide we munge the value to meet our expecations.
Expand Down
2 changes: 2 additions & 0 deletions lib/puppet/type/sqlserver_instance.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

ensurable

autorequire('class') { 'sqlserver' }

newparam(:name, :namevar => true) do
munge do |value|
value.upcase
Expand Down
10 changes: 3 additions & 7 deletions manifests/config.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,11 @@
#possible future parameter if we do end up supporting different install directories
$install_dir ='C:/Program Files/Microsoft SQL Server'
$config_dir = "${install_dir}/.puppet"
$config_file = "${config_dir}/.${instance_name}.cfg"
if !defined(File[$config_dir]){
file{ $config_dir:
ensure => directory
}
}
$_instance = upcase($instance_name)
$config_file = "${config_dir}/.${_instance}.cfg"

file{ $config_file:
content => template('sqlserver/instance_config.erb'),
require => File[$config_dir],
}

$acl_permissions = [{ identity => 'Administrators', rights => ['full'] } ]
Expand Down
21 changes: 21 additions & 0 deletions manifests/init.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
##
#
##
class sqlserver {

$config_dir = 'C:\Program Files\Microsoft SQL Server\.puppet'
ensure_resource('file',['C:\Program Files\Microsoft SQL Server',$config_dir],{ 'ensure' => 'directory', 'recurse' => 'true' })
Copy link
Contributor

Choose a reason for hiding this comment

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

32 vs 64 bit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again we assume and only support the 64 bit install version.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cyberious if that is true, it needs to be stated prominently in the readme. It doesn't currently say as much so my assumption (and most likely the community assumption) is that it is supported. Start at overview and read down to setup requirements - https://github.com/puppetlabs/puppetlabs-sqlserver#overview

I looked over the whole thing and there is nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also this is a config dir, it doesn't have to sit right next to the sql install so is it really relevant? Also side note, should we put somewhere else.


file { "${config_dir}\\sqlserver.psm1":
source => 'puppet:///modules/sqlserver/sqlserver.psm1',
source_permissions => ignore,
}

$acl_permissions = [{ identity => 'Administrators', rights => ['full'] } ]
acl { "${config_dir}\\sqlserver.psm1":
purge => true,
inherit_parent_permissions => false,
permissions => $acl_permissions,
require => File["${config_dir}\\sqlserver.psm1"],
}
}