Redirect stdin #8
Conversation
The examples did not specify the `powershell` provider, as a result puppet would use the default `exec` provider on windows instead of `powershell`.
Previously, we were passing the powershell script to execute on the command line. This is undesirable because the script contents are available for any other process to see. Also, CreateProcess has a maximum command line length of 32K characters. I would have liked to create a tempfile and execute powershell with the -File <path> option. However, powershell requires that puppet close the file handle (or resort to native Win32 APIs to open the file with full sharing). The former would expose puppet to TOCTOU race conditions. The latter is hard, since ruby is the one creating the tempfile. This commit executes powershell with redirected stdin. This solves the original issues with passing the script on the commandline. And since puppet never closes the tempfile handle until after powershell exits, the file can't be modified after puppet writes the tempfile, and before powershell writes it. Note current versions of Windows use per-user temp directories with strong permissions to minimize the likelihood of TOCTOU attacks on tempfiles, but I'd rather not make (poor) assumptions, especially on Windows 2003.
Previously, the provider was defining top-level constants POWERSHELL and
PS_ARGS, which resulted in the following warning each time the agent
pluginsynced:
warning: already initialized constant POWERSHELL
|
Has @stack72 seen this? |
|
@rismoney So ruby's tempfile looks in several places when creating tempfiles https://github.com/ruby/ruby/blob/trunk/lib/tmpdir.rb#L25 When running as TMPDIR: TMP: C:\Windows\TEMP TEMP: C:\Windows\TEMP Etc.systmpdir: C:/Windows/System32/config/systemprofile/AppData/Local/Temp I verified that C:\Windows\Temp>icacls . . BUILTIN\Users:(CI)(S,WD,AD,X) BUILTIN\Administrators:(F) BUILTIN\Administrators:(OI)(CI)(IO)(F) NT AUTHORITY\SYSTEM:(F) NT AUTHORITY\SYSTEM:(OI)(CI)(IO)(F) CREATOR OWNER:(OI)(CI)(IO)(F) Any authenticated user can write or append to files, so it's a good thing we are keeping the file handle open. |
|
Awesome, thanks for the good info. Looks like it does the right thing as localsystem. Kudos to you for the locking insight! -------- Original message -------- @rismoneyhttps://github.com/rismoney So ruby's tempfile looks in several places when creating tempfiles https://github.com/ruby/ruby/blob/trunk/lib/tmpdir.rb#L25 When running as LocalSystem I see the following values: TMPDIR: I verified that LocalSystem creates tempfiles in C:\Windows\Temp, presumably based on the System environment setting. Suprisingly, C:\Windows\Temp has fairly lax permissions: C:\Windows\Temp>icacls . Any authenticated user can write or append to files, so it's a good thing we are keeping the file handle open. — |
Earlier versions of the powershell module could not execute 64-bit powershell. This was resolved in commit 9c95b11.
The module tool generates a test/init.pp file with lines longer than 80 characters, which the puppet-lint tool warns about.
When using newer rspec, need to explicitly require rspec expectations, which is what monkey-patches the `Object#should` method. As a result, running rspec spec would fail with newer rspec versions.
Invoke powershell so it reads the script to execute from redirected stdin. This eliminates possible security issues resulting from powershell script being displayed on the command line or in log files, as well as the 32K character maximum length for command lines. It also resolves 'number3' in #1.
Also, we no longer create top-level constants, e.g.
::POWERSHELL. Previously, ruby would issue warnings that the constant was redefined during pluginsync #2. This is now resolved.