prezto-inactive-community-fork / prezto

THIS FORK HAS SHUT DOWN – use the original!
https://github.com/sorin-ionescu/prezto
MIT License
115 stars 18 forks source link

[FIX] Enable SSH module for macOS Sierra #5

Closed SeanSith closed 7 years ago

SeanSith commented 7 years ago

Due to a change in macOS Sierra (10.12), reenabling the SSH module for Sierra (and presumably later versions) is advantageous.

facastagnini commented 7 years ago

This works for me. @rjcoelho what do you think?

rjcoelho commented 7 years ago

I don't use Sierra, but code looks ok @facastagnini

slarew commented 7 years ago

For Sierra, there appears to be a new UseKeychain option in ssh_config(5). According to ssh_config(5) for Mavericks 10.9, the keychain functionality was controlled through the AskPassGUI and KeychainIntegration options. Those options do not appear in the Sierra manpage.

I would propose reverting this commit and adding a prominent note for Sierra users to enable the UseKeychain option. Thoughts?

Edit: I suppose this change set could be expanded such that for default Sierra installations the ssh-agent is launched and the user could optionally enable a prezto flag indicating the UseKeychain option is enabled and ssh-agent should not be launched.

SeanSith commented 7 years ago

I don't think that the functionality surrounding macOS's UseKeychain (or KeychainIntegration) is truly a core concern for this module. Please allow me to explain:

ssh-agent performs two functions: it allows for the storage of multiple private keys AND it aids in secure authentication when a user, who is on workstation A needs to authenticate to server C when bouncing through server B in the following scenario:

User -> A ------ [network] ------> B -----> [network] -----> C

This functionality allows a user to store their private key solely on workstation A. C requests authentication from B. B then asks A to provide the necessary authentication because B is aware that ssh-agent is running on A.

The code as it was originally written did not take this second scenario into consideration. If anything, the checking for macOS should be removed from the module completely, as it is disabling core ssh-agent functionality. This, in particular, is the reason why I wrote the patch to begin with. I didn't remove the check completely only to maintain backward compatibility -- this was probably nearsighted of me. I probably should have removed it and explained why.

Referencing the rationale in the README, the original intent of the check should have only been to disable lines 40-47 (adding identities to the agent), as all other ssh-agent functionality would have been useful to all macOS users. Interestingly enough, lines 40-47 would probably have no net effect if someone was using the Keychain for their storage. I know this because I support an organization of developers who have been effectively working in this manner for at least 3 years since I taught them how to do this. This particular module has only come to my attention because one of them starting using prezto the other day.

I agree that UseKeychain (default 'no') could be recommended to Sierra users in order for them to gain back functionality that has become non-default. However, I believe that may be outside the scope of this project, given its cross-platform nature.

If a PR removing the check is desired, I am willing to provide one.

slarew commented 7 years ago

As best as I can tell, the functionality of this module is entirely provided by macOS by default.

The ssh-agent is provided by the launchd agent (/System/Library/LaunchAgents/com.openssh.ssh-agent.plist) and thus it should not be launched automatically and SSH_AUTH_SOCK should not be changed.

edit: accidentally submitted this comment prematurely... whoops

slarew commented 7 years ago

@SeanSith I hit the exact problem you illustrate relating to ssh-agent forwarding. As far as I can tell, one must manually add keys to ssh-agent before the forwarding will work. I usually do ssh-add -K to add my identities from keychain saved passwords.

SeanSith commented 7 years ago

@slarew: I apologize if I am misunderstanding your comment here -- I'm not sure if you're agreeing or disagreeing. I just want to clarify and mean no ill will.

I would disagree that the entirety of the functionality of this module is provided by macOS:

As an ancillary: I also found this discussion interesting, as I also use an OpenPGP card for private key storage. What I'm proposing enables this functionality.

All I'm saying is that the check to see if the OS is macOS is unnecessary, and its current implementation blocks useful functionality that isn't baked into macOS.

Edit: fix improper link construction

slarew commented 7 years ago

@SeanSith I'm poorly communicating, no issue taken no worries.

I propose that line 36 not modify the SSH_AUTH_SOCK env var if uname=Darwin.

I propose that at the load identities phase (line 40), if uname=Darwin then run ssh-add -A first before trying to load other identities.

I'd gladly work on a patch if desired.

SeanSith commented 7 years ago

@slarew I would agree on your evaluation of the SSH_AUTH_SOCK modification. It's bad practice, and I'd love to see a convincing argument to keep it as is. Thanks for pointing it out.

I've created a PR #7 to further this conversation. I believe I've addressed your concerns while maintaining backward compatibility.