Conversation
|
|
|
Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement Learn more about why HashiCorp requires a CLA and what the CLA includes Have you signed the CLA already but the status is still pending? Recheck it. |
JenGoldstrich
left a comment
There was a problem hiding this comment.
Thanks for this change @anshulsharma-hashicorp, I think it's looking good, although I do wonder if handling the inclusion of this logic using a go:build directive is best, I would defer to @lbajolet-hashicorp's opinions for code style there though.
Can you provide me some more context on where that Windows pipe path comes from, and can you also sign the CLA agreement
| ) | ||
|
|
||
| func GetSSHAgentConnection() (net.Conn, error) { | ||
| pipePath := "\\\\.\\pipe\\openssh-ssh-agent" |
There was a problem hiding this comment.
Where does this pipe path come from?
There was a problem hiding this comment.
this is a static named pipe in windows, which holds the ssh private key for ssh-agent forwarding.
There was a problem hiding this comment.
Can we declare this pipe name as constant on top of the file? It would give that some context as to a magic string.
There was a problem hiding this comment.
Agree, it gives me a bit of pause that this is jsut a magic string, Lucas and I did some digging in docs and couldn't find any direct reference to this, only like stack overflow pages, so I worry that someone else reading this code in the future wouldn't have any context on what this is, I think making it a constant + adding a comment would be wise here
There was a problem hiding this comment.
Thanks for the suggestion i have done this.
There was a problem hiding this comment.
here adding the log of testing on windows machine https://docs.google.com/document/d/1wVG_NntN2OpCcF2QPUeJiA6WndSvEaoE-VTOY5xKHrM/edit?tab=t.0
There was a problem hiding this comment.
also the official doc from microsoft https://learn.microsoft.com/en-gb/windows/win32/ipc/pipe-names
|
The change looks harmless to me, although i don't have enough expertise on windows named pipes. |
anurag5sh
left a comment
There was a problem hiding this comment.
Changes look good to me. Additionally we could add any reference to the named pipe path in windows if any.
JenGoldstrich
left a comment
There was a problem hiding this comment.
LGTM! Thank you for this contribution @anshulsharma-hashicorp I know you've put a lot of work into this, you did a good job understanding the full SSH stack here, and helping improve my knowledge. 🚀 Ship it!
f5e4d4c
be5fb31 to
f5e4d4c
Compare
Issue: hashicorp/packer#3670
I am adding the changes to handle the ssh-agent forwarding in windows, before this changes the packer code would always go and check the SSH_AUTH_SOCK environment variable for socket file, which is the behavior in linux but in windows a named static files gets used for this purpose, so have added changes for the same.
This was reported by multiple users for reference i have added the github issue link above.
Tests:
Have added the Unit Test cases for the new changes.
Closes hashicorp/packer#3670