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

PowerShell Permissions Helpers Are Not Compatible with PSH 2.0 #758

Closed
DarwinJS opened this issue Jun 6, 2017 · 15 comments
Closed

PowerShell Permissions Helpers Are Not Compatible with PSH 2.0 #758

DarwinJS opened this issue Jun 6, 2017 · 15 comments
Assignees
Milestone

Comments

@DarwinJS
Copy link

DarwinJS commented Jun 6, 2017

I originally commented in #721 under the heading "Installation Code Testing" that all PowerShell code should be tested on the target platforms of openssh, which I believe still includes Windows 7 / Server 2008 R2 RTM.

FYI - the code does appear to work for Nano.

FixHostFilePermissions, FixUserFilePermissions.ps1 and OpenSSHUtils.psm1 all have syntax or assumptions that are not compatible with PowerShell 2.0.

I fixed some, but I realized I don't have time to test if the scripts still actually perform their intended duties. I also noticed that some permissions do not seem to be updating after the below changes.

Here are the ones I know about:
FixHostFilePermissions, FixUserFilePermissions.ps1 = $psscriptroot was introduced in version 3.
Fixup - right under params block: If (!(Test-Path variable:PSScriptRoot)) {$PSScriptRoot = Split-Path -Parent $MyInvocation.MyCommand.Definition}

OpenSSHUtils.psm1: '-in' operator is not in PSH 2
Untested Fixup: change $abc -in $xyzarray to $xyzarray -icontains $abc (unsure if this is a full emulation of -in)

OpenSSHUtils.psm1: '.contains' method is not in PSH 2 (string objects?)
Untested Fixup: change $abc.contains($somedata) to $abc -contains $somedata (unsure if this is working)

FYI - I personally avoid testing with "powershell.exe -version 2" for compatibility with Win 7 / Server 2008 R2. There are just too many other differences.

If these scripts can be made compatible with PSH 2.0 and a pull request made - I will grab the code from the pull request and include it in the chocolatey package for 0.0.15.0 as an override of the files in the zip archives.

@bingbing8
Copy link
Contributor

bingbing8 commented Jun 6, 2017

@DarwinJS Do you see any problem that user download WMF 5.1 on win7 machine? Here is WMF chocolatey package

@DarwinJS
Copy link
Author

DarwinJS commented Jun 6, 2017

@bingbing8 - yes there are a lot of challenges with requiring PSH 5.1 (or any PSH upgrade) for something as small as this.

The below (plus more if I had time to think and write) are why I always maintain PowerShell 2.0 compatibility with any of my code that has to run on Win7/Server 2008 R2 RTM.

Compatibility:

  • PowerShell Upgrades (especially from 2 to 5) require .NET upgrades - many machines have software stacks (especially servers) that are sensitive to the .NET version and cannot be moved forward. Many times the sensitivity is not in software under control of the machine owner - but rather is a sensitivity in some other dependency they require for their own or Off The Shelf software.
  • There can be PowerShell version sensitivities in the main business software stack - especially if any of it has elected to internally use PowerShell for scripting or other types if integration with PowerShell
  • PowerShell upgrades the entire WMF stack - which can also have similar challenges as those mentioned above - but for the management stack (software used to manage the node).

Logistics:

  • Environment consistency - most shops will not want just the machines they install OpenSSH on to have a different version of PowerShell than everything else, so then the first software to require a new version begs the question of upgrading the entire fleet of similar purposed machines.
  • getting an upgrade as fundamental as PowerShell 5.1 out is quite a logistical chore and a project in itself and rarely can large installations give 100% assurance the new version will make it out everywhere. For example 10% exceptions on 14,000 machines is 1400 - can't really follow up on why for every case.
  • Even if the administrator of the target environment is willing and able to qualify whether they can upgrade PowerShell - it's a lot of work to get OpenSSH on the machine when simply using a different openssh client would not require this. In my opinion it is a huge burden to put in front of getting openssh on a machine - especially when resolvable by changing code syntax.

Putting My Money Where My Mouth Is:

  • the above reasons are why I have maintained the Chocolatey PowerShell (full version) package to NEVER forcibly upgrade .NET to the required version - instead the package fails and makes the administrator responsible. Why? - because I don't want automatic dependency resolution to toast 500 servers under the care of an admin who elects to use my package to upgrade.
  • it is also why the chocolatey installer for openssh uses WMIC instead of Get-wmiobject - the former is present on Nano and the later is not (at least not on RTM version)

@bingbing8
Copy link
Contributor

@DarwinJS I will update the script work with ps 2.0

@bingbing8
Copy link
Contributor

@DarwinJS the fix is checked in PowerShell/openssh-portable#160 and it works on ps2 on win7. Please let me know if you see any other issue

@bingbing8 bingbing8 added this to the June-Mid milestone Jun 7, 2017
@DarwinJS
Copy link
Author

DarwinJS commented Jun 8, 2017

@bingbing8 - thank you for this!

I have tested and I get some errors and the file permission does not change - please see the screenshot.

"ProtectedAdmin" is a custom user who is a regular admin - running elevated when the code runs.

image

@bingbing8
Copy link
Contributor

bingbing8 commented Jun 8, 2017

@DarwinJS what os is this repro? this is due to a bug in default .net (localsystem is not allowed to be file owner, but icacls can set it) on the machine. I saw this on my win7 and workaround it like:
https://github.com/PowerShell/openssh-portable/blob/latestw_all/contrib/win32/openssh/OpenSSHUtils.psm1#L48-L66
Your machine may have different clr version from what I see.
Can you provide the output of these cmdlet? Thanks!
$PSVersionTable
[environment]::OSVersion

@DarwinJS
Copy link
Author

DarwinJS commented Jun 8, 2017

This Windows 7 machine also has .NET 4.5.1 because Chocolatey requires it.

image

@bingbing8
Copy link
Contributor

@DarwinJS based on the screen you provided. the code should set owner to admin groups instead of system. Can you check the OpenSSHUtil.psm1 has the below lines?
https://github.com/PowerShell/openssh-portable/blob/latestw_all/contrib/win32/openssh/OpenSSHUtils.psm1#L48-L66

@DarwinJS
Copy link
Author

DarwinJS commented Jun 9, 2017

@bingbing8 - yes I have those lines.

@DarwinJS
Copy link
Author

DarwinJS commented Jun 9, 2017

Ok - I got this one figured out.

I remembered that chocolatey installs .NET 4.0 and then on PSH 2.0 systems it still uses PSH 2.0, but it changes the CLR to 4.0 when running PowerShell (only under chocolatey - not for the entire system). I forget why but I think it had to do with some serious CLR 2.0 limitations.

The below screen show shows output of $psversiontable running under chocolatey.

Also I updated the If statements to ($psversiontable.psversion.major -gt 2) which works for chocolatey and should also work for non-chocolatey psh 2.
This was updated on these lines:
https://github.com/PowerShell/openssh-portable/blob/latestw_all/contrib/win32/openssh/OpenSSHUtils.psm1#L20
https://github.com/PowerShell/openssh-portable/blob/latestw_all/contrib/win32/openssh/OpenSSHUtils.psm1#L48
https://github.com/PowerShell/openssh-portable/blob/latestw_all/contrib/win32/openssh/OpenSSHUtils.psm1#L59

The screen shots also show successful permissions setting and owner setting.

The package also cleanly uninstalls.

You can test with this exact chocolatey package (with the $psversiontable debug output still being emitted) from a test chocolatey feed detailed in #763

image

@bingbing8
Copy link
Contributor

@DarwinJS got it. I will include the changes in my next PR

@DarwinJS
Copy link
Author

DarwinJS commented Jun 9, 2017

@bingbing8 - I had not yet tested with authorized_keys files in profiles other than the one running the opensshutils.psm1 code.

It looks like we have the same problem with setting permissions on the authorized_keys files of other user profiles on Win7 because this line: https://github.com/PowerShell/openssh-portable/blob/latestw_all/contrib/win32/openssh/OpenSSHUtils.psm1#L104 Does not have an alternative for PSH 2.0 like the other corrections you made earlier.

You can see in the screenshot the other fixed lines are working correctly.

image

@bingbing8
Copy link
Contributor

bingbing8 commented Jun 9, 2017

@DarwinJS I don't repro this on my win7 machine. but I found a workaround. I think the commit I added fix the issue.
Since we are discussing how to handle the future breaking changes and release of scripts, I can't submit the changes soon. Below include multiple fixes if you are interested:

  1. use valid verb,
  2. psd1 file to specify version,
  3. -confirm support,
  4. workaround the error "the security identifier is not allowed to be the owner of this object" on downlevel machine.

@DarwinJS
Copy link
Author

DarwinJS commented Jun 11, 2017

Thanks for the updates.

Have you tried deleting and recreating your test authorized_users files in other profiles on Win7 - or were they present during a previous run?

I am guessing here - but in the case that an authorized_users file ALREADY has the correct owner (I created my test ones WHILE logged in as the specific user), then doesn't this line: https://github.com/PowerShell/openssh-portable/blob/latestw_all/contrib/win32/openssh/OpenSSHUtils.psm1#L190

read the ENTIRE acl (including the owner), the code in these lines is SKIPPED: https://github.com/PowerShell/openssh-portable/blob/latestw_all/contrib/win32/openssh/OpenSSHUtils.psm1#L196-L224

which might affect the subsequent lines that try to set only DACLs because the entire ACL was read and we are in that weird set-acl lockout condition where we are denied writing the existing owner as a new owner (when we don't care about writing the owner) because we read the entire ACL, but only changed DACLs?

When you tested on Win7, did you create the authorized_keys files as the actual users whose profiles they are in? If not, could you give it a run that way?

@DarwinJS
Copy link
Author

@bingbing8 - I guess I didn't have the right branch. Just tested with the recently updated latestw_all and it works fine on Win 7 now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants