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

Fixes issues with openssh-win32 agent. #295

Closed
wants to merge 7 commits into from
Closed

Fixes issues with openssh-win32 agent. #295

wants to merge 7 commits into from

Conversation

nitin88
Copy link

@nitin88 nitin88 commented Jun 29, 2016

When Openssh-win32 agent is installed, Get-Command ssh-agent ssh-add returns openssh related commands which are not compat with Poshgit. Use ssh stuff provided with git only

Fixes #258 #292

GitUtils.ps1 Outdated
@@ -282,15 +281,30 @@ function Find-Pageant() {

# Attempt to guess $program's location. For ssh-agent/ssh-add.
function Find-Ssh($program = 'ssh-agent') {
$sshLocation = 'C:\Program Files\Git\usr\bin\'
Copy link
Author

Choose a reason for hiding this comment

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

This has to be given as preference from user perspective. not sure where to keep this and how to import that var here. temp hardcoded

Copy link
Owner

Choose a reason for hiding this comment

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

Is this actually necessary? I would expect the existing code in Find-Ssh to be able to resolve the correct location relative to the discovered git command.

GitUtils.ps1 Outdated
@@ -256,8 +256,7 @@ function Get-SshAgent() {
} else {
$agentPid = $Env:SSH_AGENT_PID
if ($agentPid) {
$sshAgentProcess = Get-Process | Where-Object { $_.Id -eq $agentPid -and $_.Name -eq 'ssh-agent' }
if ($null -ne $sshAgentProcess) {
if (((Get-Process -Id $agentPid -FileVersionInfo -ErrorAction SilentlyContinue).Filename).Contains("Git")) {
Copy link
Owner

Choose a reason for hiding this comment

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

If $agentPid is valid but the process has been killed, this will blow up when you try to access Filename.

Copy link

Choose a reason for hiding this comment

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

(Get-Process -Id 11111111 -FileVersionInfo -ErrorAction SilentlyContinue).Filename

doesn't throw any error. you will get empty data

@nitin88
Copy link
Author

nitin88 commented Jun 30, 2016

Reworked on this. Win32-OpenSsh install ssh as service. leverage that functionality. Posh-git no longer requires Git Ssh agent if openssh agent installed

@nitin88
Copy link
Author

nitin88 commented Jun 30, 2016

@dahlbyk please review the same.

GitUtils.ps1 Outdated
return Get-Service -Name $service -ErrorAction SilentlyContinue
}

function Find-SericePID($service = 'ssh-agent') {
Copy link
Owner

Choose a reason for hiding this comment

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

Typo: Serice

@dahlbyk
Copy link
Owner

dahlbyk commented Jun 30, 2016

I like this approach better than the explicit check for Git in the ssh-agent path. I'm not a Win32-OpenSSH user (yet, I suppose), but I'm hoping we can get broader confirmation that this works as expected.

Thanks for taking this on!

GitUtils.ps1 Outdated

function Find-SericePID($service = 'ssh-agent') {
# Find if wind32-openssh available and use it
return (get-wmiobject win32_service | where { $_.name -eq $service}).processID
Copy link
Collaborator

Choose a reason for hiding this comment

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

(Get-Service $Service | Get-Process).Id will be much faster than this.

@DarwinJS
Copy link

DarwinJS commented Jul 1, 2016

I'm late to the game here, but here is how I figure out if a [1] running instance of a process, is [2] from the win32-openssh folder:

$TargetFolder = "C:\Program Files\OpenSSH-Win64"
$SSHServiceInstanceExistsAndIsOurs = ([bool]((Get-WmiObject win32_service | ?{$_.Name -ilike 'sshd'} | select -expand PathName) -ilike "*$TargetFolder*"))

This would let you understand which package the running service is coming from.

Also - I think the SSH in git is much more debugged than Win32-OpenSSH, especially for git-ish stuff. So I would stick with it since you depend on git being installed anyway.

Maybe if another ssh is loaded you warn the user that poshgit may not work with it?

@nitin88
Copy link
Author

nitin88 commented Jul 1, 2016

@DarwinJS

  1. I don't want to hard code any path. the path might change in the next build.
  2. ssh is independent tool. the one which comes in "git" is open ssh one. Microsoft forked it and baking into Windows.
  3. Upcoming releases of Windows, will have "openssh-win32" in it's core.

So Its good to support and if we look at memory/cpu utilization of openssh-win32, is around 80% better than git one

@DarwinJS
Copy link

DarwinJS commented Jul 1, 2016

Good info. Thanks.

GitUtils.ps1 Outdated
}

function Get-TempEnv($key) {
$path = Join-Path ($Env:TEMP) ".ssh\$key.env"
if (Test-Path $path) {
$value = Get-Content $path
[void][Environment]::SetEnvironmentVariable($key, $value, [EnvironmentVariableTarget]::Process)
Set-Item Env:$key -Value $value -ErrorAction SilentlyContinue
Copy link
Owner

Choose a reason for hiding this comment

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

Why switch from [Environment]::SetEnvironmentVariable(...) to Set-Item? I don't remember why I used SetEnvironmentVariable() in the first place, but I'm curious what the difference is from a OpenSsh-Win32 perspective?

@dahlbyk
Copy link
Owner

dahlbyk commented Aug 12, 2016

One last question, but I think this is looking pretty good. Can you pull in dahlbyk:master and resolve the conflicts?

@nitin88
Copy link
Author

nitin88 commented Aug 22, 2016

downmerged from master

@dahlbyk
Copy link
Owner

dahlbyk commented Aug 23, 2016

I think we're good here.

@theaquamarine I believe this resolves the environment variable issue in #302?

@rkeithhill
Copy link
Collaborator

Another thing to consider is how this should work on PowerShell v6 on Linux and macOS.

@dahlbyk
Copy link
Owner

dahlbyk commented Dec 29, 2016

@nitin88 sorry to have let this gone stale - can you either resolve the conflict or let us edit this PR?

Another thing to consider is how this should work on PowerShell v6 on Linux and macOS.

Does Get-Service exist on Linux/macOS? If it doesn't, we should be able to add guards rather easily ahead of 1.0 support for Core.

From a cross-platform standpoint, is there a useful distinction between these?

Set-Item Env:$Key -Value $Value -ErrorAction SilentlyContinue
[void][Environment]::SetEnvironmentVariable($Key, $Value)

@dahlbyk dahlbyk mentioned this pull request Dec 29, 2016
5 tasks
@nitin88
Copy link
Author

nitin88 commented Dec 30, 2016

@dahlbyk both are same. One is pure powershell implementation while the other one is C# implementation.

I am not sure whether 'Get-Service' exists in *nix. Ideally the code is consistent between Windows/*nix except calling layer underneath has changed.

Note: Now you can contribute

@rkeithhill
Copy link
Collaborator

rkeithhill commented Dec 30, 2016

As of 6.0.0-alpha.14, Get-Service is not available on Linux.

Update: As of 6.0.2 neither Get-Service nor Start-Service are available on Linux/macOS.

@brunoyb
Copy link

brunoyb commented May 3, 2018

Hi! With Windows 10 April 2018 Update Win32-OpenSSH was installed automatically for me, so the ssh-agent stopped working properly (I have Start-SshAgent in my profile.ps1). As a workaround I simply removed the Win32-OpenSSH tools from PATH but it would be nice if it could work normally, and I believe this is what this PR is doing! Any ETA on when we can go forward with this? Is there something we can do?

Thanks!

@dahlbyk
Copy link
Owner

dahlbyk commented May 3, 2018

I suppose this impacting more people via Windows Update is a good reason to wrap up the ssh-agent situation. The goal for v1 is to move everything ssh-ish into a separate module, but it would be good if everything worked reasonably in v0.x.

There are a handful of open ssh PRs that are significantly out of date, if anyone cares to consolidate/update them targeting the v0 branch.

@rkeithhill
Copy link
Collaborator

rkeithhill commented May 3, 2018

FWIW I'm using the new OpenSSH implementation in Windows and as a result I do not need any of the SSH support in posh-git. I set up the new ssh-agent service to start automatically and start it. Then, when you add your private keys with the OpenSSH version of ssh-add, that key will be remembered across reboots and because ssh-agent is started (automatically) as a service, you don't need to start it in your PowerShell profile anymore.

In order to get Git to use the new OpenSSH, you will want to set this system or user env var:

GIT_SSH = C:\Windows\system32\OpenSSH\ssh.exe  

Double check that path. I'm actually using the latest version of OpenSSH for Windows installed via choco install OpenSSH. If you use Chocolatey to install the latest version, then use the path it installs to i.e. C:\Program Files\OpenSSH-Win64\ssh.exe.


function Find-Service($service = 'ssh-agent') {
# Find if wind32-openssh available and use it
return Get-Service -Name $service -ErrorAction SilentlyContinue
Copy link
Collaborator

Choose a reason for hiding this comment

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

All references to Get-Service and Start-Service need to be behind a "Windows-only" firewall.

Copy link
Collaborator

@rkeithhill rkeithhill left a comment

Choose a reason for hiding this comment

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

See comment about putting Windows specific cmdlets like Get/Start-Service behind a "Windows-only" firewall.

@dahlbyk
Copy link
Owner

dahlbyk commented Jun 18, 2018

Closing in deference to #586. Sorry we didn't review this in a more timely manner!

@dahlbyk dahlbyk closed this Jun 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants