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

Questions and Thoughts About New ACLs for Private Keys #721

Closed
DarwinJS opened this issue May 14, 2017 · 17 comments
Closed

Questions and Thoughts About New ACLs for Private Keys #721

DarwinJS opened this issue May 14, 2017 · 17 comments

Comments

@DarwinJS
Copy link

DarwinJS commented May 14, 2017

I some questions and thoughts about the permissions outlined in: https://github.com/PowerShell/Win32-OpenSSH/wiki/Security-protection-of-various-files-in-win32-openssh (can't find the doc right now).

I could be misperceiving the net effect of the permissions changes - let me know if I am.

The security outlined for the Host Private Keys seems likely to create problems for the ongoing maintenance of OpenSSH installs. These possible challenges all relate to the fact that only "NT Service\sshd" could manipulate private key files and their permissions once all or part of the required permissions are applied during first time setup.

Possible Problems:

  1. Cannot verify Host key permissions with setup scripts.
  2. Cannot correct partially incorrect host key permissions when they aren't quite right, but permissions were changed enough where only "NT Service\sshd" has files permissions.
  3. Cannot remove OpenSSH completely. (host keys are permanent once set the first time)
  4. Cannot reset Host Keys (only "NT Service\sshd" could remove private key files). (host keys are permanent once set the first time)
  5. No ability to manipulate the ACLs at all once "NT Service\sshd" is removed.

From a setup / maintenance perspective getting the key permissions correct becomes a "one try to get it right" and then from then on all maintenance activities are locked out.

Thoughts On Solutions

Given the above, would it make sense that sshd.exe ITSELF be responsible for both initial permission configuration as well as resetting permissions for operations such as uninstall and/or key replacement?

Maybe the service could receive signals about the file permissions - but only if they come from an elevated admin? (example signals "Secure Keys", "Validate Key Permissions", "Delete Keys", "Reset Keys")

Maybe sshd could receive these signals, but only from ssh-keygen running elevated?

Maybe the owner should not be sshd, but a well known local account that can still be accessed by setup routines? (FYI - I don't think TrustedInstaller would work as I don't believe anyone outside of Microsoft can access it?).

Publish An Advised Approach As Soon As Possible?

If these problems can be gotten around by taking ownership of the files by another profile - it would be very helpful if the development team would test such procedures and publish working demonstration scripts for all supported OSes for version 0.0.13.0.

This would extremely helpful now because:

  • upon adoption of 0.0.13.0 the way forward for long term management of these files would be well understood and debugged.
  • all openssh users would have working scripts to fix up any installation problems they might create for themselves when attempting to install / upgrade to 0.0.13.0
@manojampalam
Copy link
Contributor

manojampalam commented May 15, 2017

@DarwinJS, @bingbing8 is working updating the permissions related wiki.

Here's the current design behind the permissions for "Win32 OpenSSH" ssh keys general (applicable to both user and host).

  • By default, a key is ACLed only to its owner. As a result, you'd see the following
C:\>whoami
userABC
C:\>ssh-keygen -t rsa -f id_mykey -P password
C:\>icacls id_mykey
id_mykey userABC:(F)
  • If the key is to be used as a "user key" (for client authentication), no additional steps (to change ACLs) are typically needed.
  • If the key is to be used as a "host key" (key itself would typically be created by an admin as part of configuring sshd), following additional step would be needed:
    • hostkey.pub (public key file) needs permissions adjustment to allow read (R) access to "NT Service/SSHD"
    • If the private key would be registered to ssh-agent (this is the recommended way to secure host private keys)
      • Since the private key needs to be registered as "SYSTEM", adjust private key permissions to allow (R) access to "SYSTEM". (We should be able to skip needing this permissions change, by giving access to "SYSTEM" by default when creating the key).
    • If the private key is to be used by SSHD service directly
      • Change its owner to System.
      • Adjust private key permissions to allow (R) access to "NT Service/SSHD"

"NT Service/SSHD" would never need, and should never have ownership or (W) privileges on host keys.

@DarwinJS
Copy link
Author

I will work these changes and the new wiki page into the re-acling utility script I am building (Reset-SSHKeyPermissions.ps1).

Let me know if you would like me to maintain the re-acling script as part of the project.

Note that it has many uses beyond the first migration from an old version. For instance, when you install server on a machine that previously only had the client.

@manojampalam
Copy link
Contributor

@DarwinJS please give us a couple of days to think through the end user implications. I'll get back with what we conclude and take your feedback before proceeding with putting this feature (permission checks) in.

@DarwinJS
Copy link
Author

@manojampalam - ok sounds good.

Personally I feel the solution should NOT be designed with the idea that the primary method to set permissions will be to run something within each user context who needs to set permissions on keys.

If it is designed to be able to work correctly when run from a central, admin type account - then it should work for regular users easily.

However, then it will also allow migration as well as correction of user key permissions by maintenance scripts (via scheduler or a management agent) that would naturally run under an admin profile.

@manojampalam
Copy link
Contributor

@DarwinJS please check https://github.com/PowerShell/Win32-OpenSSH/wiki/Security-protection-of-various-files-in-Win32-OpenSSH (its not complete yet but wanted to take your feedback)

@DarwinJS
Copy link
Author

DarwinJS commented May 25, 2017

@manojampalam - thanks for the feedback invite.

Also thanks for putting effort into documents to help us get this right!

The discussion on elevated versus non-elevated does not conclude with information on which scenario does NOT work. FYI - THE built-in Administrator user account does not have UAC enabled - not sure if you need to mention that as some management frameworks use THE "administrator" rather than system account (e.g. AWS cloud formation) - also packer can be configured to use it.

Present One Best Practice Model First and Alternatives As Footnotes

It seems to me that if "SYSTEM" owns the file and rules the service things stay simple and it seems the default your team has been working against so far. I would present that as the primary approach throughout and then at the END have a section on alternative configurations where SystemAdministrator can own the files and then they must be registered with ssh-agent.
Also I believe your team requires SSH-Agent be running is sshd is? I know the chocolatey package follows earlier advice to register the keys with ssh-agent in SYSTEM context and then DELETE the keys.

In any case I would establish the desired default behavior and outline it first.

Although this keeps the documentation simpler, frankly most development teams test ONLY ONE integration / installation model when doing their main tests. Most of the users of openssh will want to stick closely to that model as others just don't get as much testing as the code moves forward. (even if the design of the software theoretically supports alternative configurations)

If the team feels that there really, really isn't a preferred installation model - then they should do automated QA testing against ALL supported installation models as that would really prove out the desire to not have a preferred one.

I believe this get-acl .\ssh_host_dsa_key | Set-Acl ssh_host*key might be a bad code sample in practical working environments as it will try to re-write owner information which almost always causes a problem - especially if the user running this is not already the owner.

All Configuration Helpers in PowerShell

I know that PowerShell is much more verbose in terms of setting permissions, but I would certainly like to see it used as the only code in this guide.

This will help by:

  • ensuring compatibility with Win 7 through Nano
  • ensure no need for customers nor Microsoft to distribute utility binaries for things like the desired "Enable-SSHRemoting"
  • ensure no 32-bit utilities get caught up in the mix like early versions of the install instructions for openssh.
  • ensure the ongoing ability to easily trigger this through remoting without pushing utility files around (I know that icacls is everywhere already - but keeping to PSH guards against other utilities creeping into install instructions over time)

I am working on a "Reset-SSHKeyPermissions" which drives across the ProfileList registry key to find ssh keys (then you know those have had a valid windows logon) - but it is all PowerShell.

If it seems like there would be too much code - that is probably an indicator that you need to be distributing a "Open-SSH Configuration Helpers" admin module with OpenSSH - providing this has been a huge strength of many of Microsoft's traditional products over the last decade.

Complete, Working Code for Making a System Compliant

As per my most recent blog post (https://cloudywindows.com/post/back-to-basics-testable-reference-pattern-manifesto-with-testable-sample-code/) I am a big advocate of working reference patterns - not a bunch of "off the top of the head" pseudo-coded samples.

Installation Code Testing

IMO the working code examples for installing/configuring should be tested on Windows 7 (PSH 2 syntax, .NET 2) and Nano (.NET Core) as - in my experience with the Chocolatey package - those represent the edge cases that cause the most code adaptation to be necessary.

For These Very limited permissioned keys Create a "How To Delete / Replace / Recover From Misconfiguration" section

We will all need to know how we can remove, updated or re-ACL these keys if only one user owns them. If that info is already in this document, it should have a dedicated section. This should have accompanying working code.

@manojampalam
Copy link
Contributor

manojampalam commented May 25, 2017

@DarwinJS thanks for the quick and detailed review.

  • I added the elevated and non-elevated section in an attempt to give a quick response to questions like - why is it not OK for an admin user to own or have access to host private keys? I'll remove that as I see it deviating from the topic
  • "Best Practices" is great feedback. Will put that right at start.
  • SSH-Agent is a service that runs as SYSTEM. The recommendation is to register the host keys to ssh-agent as SYSTEM and secure them (either by generating them with a passphrase in first place or backing them up at a secure location).
  • We wont be settling on the packaging and distribution model soon. For now, 2/3rds of users are relying on your Chocolatey package. We have a simple way to test out any Win32-OpenSSH installation in general (irrespective of how its installed). This will basically run the same set of tests that are run on every commit validation. @bingbing8 will share instructions and you could incorporate it in your package validation.
  • I'll get rid of get-acl .\ssh_host_dsa_key | Set-Acl ssh_host*key
  • Will add OpenSSH-Health-Check powershell module as part of release payload that could be used for
    • Checking compliance against Best Practices
    • Recommendations to improve
    • AutoFix if needed/requested.
  • We will ensure the above is tested on all platforms.
  • Please share your work Reset-SSHKeyPermissions. That would probably go in the above module.

@DarwinJS
Copy link
Author

DarwinJS commented May 25, 2017

@manojampalam - glad it is helpful.

My feedback was meant to indicate that if there is flexibility in the way permissions are set, that you would put the one that will be tested front and center. In fact, I would not even mention other models unless there is known utility value to a given end user. For instance, the fact that key files could be owned by Administrator should either not be included or be a foot note if in fact your team does not configure it that way on their machines.

This would be a consistent "permissions specification" regardless of what or how many packaging and distribution models you end up supporting.

The name of "OpenSSH-Health-Check" implies it has a disposition to not changing anything. Maybe something more general like "OpenSSH-Utilities"

Also, I would advise you just drop the psm1 in the Openssh folder and not to put it in PSModulesPath - the reason is that the version of the module will then ALWAYS match the installed version of OpenSSH.

Personally, I feel single-point-of-registration code (like PS Modules) takes us backward 20 years to shared DLL hell (COM Registrations) - while every other piece of code on Windows is / has moved to using explicitly private copies of such code.

If I am going to muck with checking Open SSH health or permissions I don't mind explicitly loading the PSM1 at it's full path.

@manojampalam
Copy link
Contributor

@DarwinJS yes, the plan is to put the psm1 in OpenSSH folder. Thanks for enlightening me about the module registration conflicts. I think we can keep things simple by just providing individual ps1 utility scripts. I personally am not a huge fan of hiding configuration complexity behind user friendly automation scripts/utilities. Rather, I believe in making the configuration natively simple and easy to understand in the first place.
I really liked the idea of best practice patterns and I agree it will be very useful to provide a light weight utility that can cross check a configuration for a specific pattern, identify and report deviations and with admin's consent make the appropriate changes to get it in compliance.

@DarwinJS
Copy link
Author

DarwinJS commented May 26, 2017

@manojampalam - I totally agree that simplicity in design should be priority #1.

After that has been pushed as far as possible, then my "Adoption Catalyst" headset kicks in - whatever lights adoption on fire - do it! 99% of individuals and computers that eventually leverage openssh couldn't care less about any of the details - so at that point it should be one-button, mobile app type simplicity if it is at all possible.

I worked at one point creating software deployment automation that ran on 14,000 machines - 1% failures on an "upgrade over a previous version" was 140 machines spread across the planet - that would take over two weeks to look into. So the more something "just worked" out of the box - all the better!

P.S. I also think the "Adoption Catalyst" headset is a sliver of entrepreneurial thinking that is truly relevant to almost any context.

@bingbing8
Copy link
Contributor

bingbing8 commented Jun 16, 2017

@DarwinJS Please see this wiki Run OpenSSH Test. Currently we run tests on win2k12 R2 and win10. The scripts currently don't not compatible with PSH 2.0 yet. We plan to update it to run on win7.

@bingbing8
Copy link
Contributor

@DarwinJS with the commit PowerShell/openssh-portable@eb0ab1b, test should be run on win7. I will close this issue. please reopen if there is any other concerns. thanks!

@DarwinJS
Copy link
Author

@bingbing8 - sounds good - thanks for all your work on making it support all the platforms that openssh supports.

@bingbing8
Copy link
Contributor

@DarwinJS there are a known issue on win7: #770 and #781. You need to workaround the first one before you run the tests. for second one, there maybe some random test failures.

@DarwinJS
Copy link
Author

The universal installer has been giving the SeAssignPrimaryTokenPrivilege to "NT Service\SSHD" for a long time now. Possible since September of last year?

@bingbing8
Copy link
Contributor

yes, but the script does not do the right on win7

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

Not sure what you mean - I have my own code. Are you saying the chocolatey package is not doing the grant of the token privilege correctly?

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

3 participants