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

OpenSSHUtils ProfilePath parsing issues #843

Closed
wants to merge 2 commits into from

Conversation

annulus
Copy link

@annulus annulus commented Aug 7, 2017

Hi,

Regarding the authorized_keys permission fix bits:
I found out that in case there's more than one entry in the registry that matches the regex, then it will throw an exception as it appends all the matching registry paths to the profileItem variable, which won't be a valid registry entry that has a PSChildName field (thus the exception).

  • If you delete the home dir of an user, then login via ssh, windows will append ".bak" to the user's profileList entry's name. If you recreate the user, it will have a new profileList entry, but with the same path.
  • If you have multiple users that share the first part of their username/path, then the shortest user's regex will match all of them.

Regards,
David

@msftclas
Copy link

msftclas commented Aug 7, 2017

@annulus,
Thanks for having already signed the Contribution License Agreement. Your agreement has not been validated yet. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@@ -168,7 +168,9 @@ function Repair-AuthorizedKeyPermission
$userProfilePath = $properties.ProfileImagePath
}
$userProfilePath = $userProfilePath.Replace("\", "\\")
$fullPath -match "^$userProfilePath[\\|\W|\w]+authorized_keys$"
if ( $properties.PSChildName -notmatch '\.bak$') {
Copy link

@DarwinJS DarwinJS Aug 8, 2017

Choose a reason for hiding this comment

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

@bingbing8 - Windows will also create additional, profile when users move between domains or a profile is corrupt. These result it other profile folders that are orphaned (invalid for authorized_keys purposes).

Rather than the exclude, maybe the regex should just be tightened up to not match "...\userprofilepath...", but only "...\userprofilepath\..." ?

Copy link
Author

Choose a reason for hiding this comment

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

@DarwinJS It seems that you wrote "...\userprofilepath..." twice.

Copy link

Choose a reason for hiding this comment

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

Looks like github mistook my second slash for markup - was able to fix it. I am suggesting that the second literal slash that is found in $properties.profileimagepath be left intact so that profiles with ".something" are not picked up as they are generally not in live use.

Copy link
Author

@annulus annulus Aug 8, 2017

Choose a reason for hiding this comment

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

It is two separate thing:

  • appending a slash to the end of the filesystem path in the regex, that's against the value of the registry field ProfileImagePath is important to ignore the registry entries that are belonging to users that share the beginning of their path (this is what my other commit is about)
  • ignoring the registry ProfileList entries that end with .bak are to avoid loading old/backup reg.entries

@bagajjal
Copy link
Collaborator

bagajjal commented Aug 8, 2017

Please submit the changes in the openssh-portable repo...
https://github.com/PowerShell/openssh-portable

This repo is only used for tracking the issues...

@annulus
Copy link
Author

annulus commented Aug 9, 2017

Ok.. PowerShell/openssh-portable#192

@annulus annulus closed this Aug 9, 2017
@bingbing8 bingbing8 added this to the Aug-End milestone Sep 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants