Support ssh-agent and external sign binaries for commit signing with ssh #2464
Conversation
chirpcel
force-pushed
the
feature/external-ssh-signer
branch
from
14f71cc to
ebd8fc1
Compare
|
There's one drawback. Like before, we don't support encrypted private keys which are not loaded to agent. The error message isn't as clear as before, but from a functionality point of view it isn't different. I would make a new issue for this behavior and take a look into supporting encrypted private keys. |
cant we make it as clear as before and then followup
|
|
Actually I didn't figured out how to capture the passphrase prompt, as it's spawned directly on the tty. |
|
@extrawurst Worked, we now have a meaningful error message. I tested with 1Password SSH-Agent, Default SSH-Agent, non encrypted keys on disk and encrypted keys on disk. |
chirpcel
force-pushed
the
feature/external-ssh-signer
branch
from
9d934d6 to
19f0891
Compare
chirpcel
force-pushed
the
feature/external-ssh-signer
branch
from
19f0891 to
004b135
Compare
|
Any movement on this? This would allow me to use gitui without needing to fallback to shell to actually commit. |
|
@DaRacci Think maintainer is currently busy as asking for co-maintainers and migrating his repo to an org for better support by more people. |
There was a problem hiding this comment.
Hey @chirpcel. Thank you for this implementation. I support this fully and agree that this should be the way commit signing is implemented at the moment. It is almost in compliance with git commit's implementation. Where changes will be required for currently unsupported git commit signing features, I have remarked them in this review. I wouldn't say that these are blockers, though.
I also have minor questions and nitpicks, mostly about corner cases, here and there and some FYIs.
On which platforms have you tested this? I have tested it on Linux and it works as promised. It would be great if someone tested this on Windows and Mac, too, though. (A test should consist of at least signing a commit and checking whether that commit's signature can be validated.)
| if &self.program == "ssh-keygen" { | ||
| cmd.arg("-P").arg("\"\""); | ||
| } |
There was a problem hiding this comment.
Huh, ssh-keygen's documentation reads as if -P isn't supported, but it obviously works. Cool. So this is just an FYI:
-U is also a valid and documented option and ensures that ssh-keygen exits with an error message instead of prompting for a password:
I'm fine with either, I suppose. Just be aware that, should we ever allow stdin/stdout interacting with signing tools, I guess we will need to switch to -U if we want to conform with git's behavior, which is (a) prompting for passwords if files are provided as keys and there's no private key in the agent and (b) requiring an SSH agent to provide the private key if the public key is literal.
There was a problem hiding this comment.
Seems like a more clean solution for me. I'll like to change it to that way
| signing_key: &str, | ||
| ) -> Result<PathBuf, SignBuilderError> { | ||
| let key_path = PathBuf::from(signing_key); | ||
| if signing_key.starts_with("ssh-") { |
There was a problem hiding this comment.
| let temp_file = temp_file.keep().map_err(|err| { | ||
| SignBuilderError::SSHSigningKey(err.to_string()) | ||
| })?; |
There was a problem hiding this comment.
It would be nice if we would not clutter /tmp with public keys in case a handled error occurs before you get to remove it later on. Maybe we can keep the NamedTempFile instance around in SSHSign?
| let tmp_path = std::env::temp_dir(); | ||
| if self.signing_key.starts_with(tmp_path) { | ||
| // Not handling error, as its not that bad. OS maintenance tasks will take care of it at a later point. | ||
| let _ = std::fs::remove_file(PathBuf::from( | ||
| &self.signing_key, | ||
| )); | ||
| } |
There was a problem hiding this comment.
I'd like to argue in favor of not deleting keys just because they start with the temporary files prefix. 😅
(See also my remark above. Keeping the NamedTempFile instance around solves this.)
| .arg("-f") | ||
| .arg(&self.signing_key); | ||
|
|
||
| if &self.program == "ssh-keygen" { |
There was a problem hiding this comment.
Sorry, I don't really understand why you only pass -P if this condition is fulfilled.
There was a problem hiding this comment.
Cause there are some implementations which doesn't support empty -P with fallback to agent. For example the 1Password SSH-Agent and it's cli op-sign throws an error in that case. On default ssh-keygen its tested to work but it hasn't to be on 3rd parties.
Co-authored-by: Johannes Agricola <[email protected]>
|
Thanks @naseschwarz for your cr! I commited your suggestions and commented on questions. I'll take a look at the remaining annotations of edge cases/optimizations. I'll update the pr soon. I've tested the implementation on linux and macOS. Windows currently missing, maybe I can test it in a VM. Currently don't know about license/activation. Keep you updated. |
This Pull Request fixes/closes #2188.
It changes the following:
I followed the checklist:
make checkwithout errors