Fix op ssh signing #292
Merged
Fix op ssh signing #292
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
drewdeponte
reviewed
Feb 12, 2024
src/ps/private/git/create_commit.rs
Outdated
| @@ -35,81 +35,91 @@ pub fn create_commit( | |||
| .map_err(CreateCommitError::GetCommitGpgsignFailed)? | |||
| .unwrap_or(false); | |||
|
|
|||
| let gpg_program_option = config_get_string(config, "gpg.program") | |||
| .map_err(|e| CreateCommitError::Unhandled(e.into()))?; | |||
|
|
|||
| if sign_commit_flag { | |||
| let gpg_format_option = config_get_string(config, "gpg.format") | |||
| .map_err(CreateCommitError::GetGpgFormatFailed)?; | |||
There was a problem hiding this comment.
While we are trueing all this stuff up. We should instead of throwing an error when the gpg.format is missing we should instead default to openpgp the same way Git propper does.
drewdeponte
reviewed
Feb 12, 2024
src/ps/private/git/create_commit.rs
Outdated
| let gpg_program_option = config_get_string(config, &format!("gpg.{}.program", val)) | ||
| .and_then(|opt| { | ||
| opt.map_or_else( | ||
| || config_get_string(config, "gpg.program"), |
There was a problem hiding this comment.
if we make it this far and we don't have the gpg.program specified then we should default to gpg as Git propper does.
There was a problem hiding this comment.
The default for the program is happening in the different signers already.
|
@alondahari Only thing I noticed where those few comments about defaulting. It would just be good to true it up in terms of matching Git proper behavior in terms of configuration. |
Add a function that returns a signing key if it is a literal value in gitconfig, or None if it's not (would be a path). This is done since with SSH signing, the user.signingKey in gitconfig can either be a path to a file with the key, or a literal key (like with gpg). See: https://git-scm.com/docs/git-config#Documentation/git-config.txt-usersigningKey #290 <!-- ps-id: 46319555-2a7b-44a4-ac17-c1e1fd8cd72b -->
Add a function to create a temp file containing a ssh key if one is supplied literally in the gitconfig. The function accepts a path, since it will be in a temp dir that we need to live long enough to complete the signing. #290 <!-- ps-id: eee809a0-baba-4965-8364-b045a3f8e8a3 -->
We will use the tempfile crate to create a tempfile to be used when signing commits with SSH, so we will need to use this dependency outside of the test utils. #290 [changelog] added: tempfile crate dependency <!-- ps-id: 7bfe5444-fccf-4ed9-bca2-f451e8ad1828 -->
Add a function to sign the ssh string using a signing key (either a path or a literal key) and an optional program (fallback to ssh-keygen). The function will be used by the ssh signer and will replace current implementation. <!-- ps-id: 7b3f2b55-9949-4335-8f93-a754f952cd5a -->
The program option in git config can live under [`gpg.<format>.program`](https://git-scm.com/docs/git-config#Documentation/git-config.txt-gpgltformatgtprogram) in gitconfig, so we would need to respect that. I moved fetching the program option to inside the format option match, so we can use that to namespace the call to get the program configuration, falling back to the legacy `gpg.program` config. This small refactor is done since the ssh branch should also have an optional custom program to run (the 1Password binary for example) and we would want to follow the same general path with both methods. #290 <!-- ps-id: 10064b8f-e14f-4f9e-aa06-97b8ac5b10f0 -->
Getting the signing key from the config should be identical no matter what the format is, so take it out of the match. This is done in a general effort to improve the readability and maintainability of this piece of code. #290 <!-- ps-id: b24c7a77-97d3-48ff-8e32-2956148724ba -->
When signing commits with SSH, we need to use the program in gitconfig if specified. Otherwise, fallback to ssh-keygen. This aligns with how git proper is doing it. #290 [changelog] updated: ssh commit signing respects literal keys in config updated: ssh commit signing respects custom program updated: default ssh commit signing uses ssh-keygen <!-- ps-id: 7c04af23-f05d-43f3-91aa-8ebac634ffdf -->
The ss-key dependency is no longer used, since ssh-keygen is used instead. #290 [changelog] removed: ssh-key crate dependency <!-- ps-id: 112c8505-5862-4da8-b9e4-ba0d62549997 -->
Following how git does things, we will default to openpgp if the signing format is not defined in gitconfig #290 <!-- ps-id: 4e68a19d-bdc5-40f9-8e01-8dbb8d6f3b3f -->
alondahari
force-pushed
the
fix-op-ssh-signing
branch
from
e7be5ce
to
19a85af
Compare
drewdeponte
approved these changes
Feb 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #290
The commits in this PR have all been signed by 1Password's ssh signing integration.
When using a local SSH key generated by ssh-keygen, we get a message in the console from ssh-keygen: "Signing data on standard input". This seems to be the result of how we sign the commits (from stdin), and it's not a warning, but I'm not sure what we can do about that. Other than create a buffer to sign instead, like git does.
Did not address the issues around errors bubbling up and panic in
cherry_picking.rs, probably should go into separate issues.