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

add support for external ssh signing #2198

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

seanaye
Copy link

@seanaye seanaye commented Apr 19, 2024

This Pull Request fixes/closes #2188.

It changes the following:

  • Adds support for external bin signing with ssh keys

EDIT: I have added a test for the new feature, let me know if this is not sufficient or what to change, its a bit hard to test external binaries.

I ran make check without errors except for the python check, I don't have python on my system which I'm guessing is the cause of the failure

I followed the checklist:

  • I added unittests
  • I ran make check without errors
  • I tested the overall application
  • I added an appropriate item to the changelog

@extrawurst
Copy link
Owner

@yanganto you are the most prominent user of ssh-signing. can you review this?

@extrawurst extrawurst assigned extrawurst and unassigned extrawurst Apr 21, 2024
@extrawurst
Copy link
Owner

@seanaye is there a way to write a test for this?

@seanaye
Copy link
Author

seanaye commented Apr 29, 2024

I'll try to write some tests this week

@kucho
Copy link

kucho commented Apr 29, 2024

Very excited about this feature to start using gitui as my daily driver! Thank you, @seanaye 🙏

@dkarter
Copy link

dkarter commented Jul 4, 2024

Thank you for working on this! would love to see it released

@robrecord
Copy link

Hi @seanaye - sorry to ping and realise you must be busy; hoping you can still throw resources at this; Myself, I currently have to stop using 1p ssh signing due to breaking my gitui workflow. Thanks for your time spent thus far.

@seanaye
Copy link
Author

seanaye commented Aug 31, 2024

@robrecord no worries! I actually totally forgot about this, I think I can find some time this week to finish it up

@seanaye seanaye force-pushed the master branch 2 times, most recently from 169e177 to 4dd4bbf Compare August 31, 2024 20:30
@seanaye
Copy link
Author

seanaye commented Aug 31, 2024

@extrawurst @yanganto please let me know what else I can do to help get this merged

@yanganto
Copy link
Contributor

yanganto commented Sep 1, 2024

@extrawurst @yanganto please let me know what else I can do to help get this merged

Hi @seanaye,
The previous suggestion about no additional struct in the PR may not help you merge this PR, but it is good to respond even if you don't think we need any changes.

@extrawurst
Copy link
Owner

@kucho @dkarter @robrecord anyone who can help by building this branch and testing it with their setup and reporting back here can help get this merged.

@seanaye
Copy link
Author

seanaye commented Sep 2, 2024

@extrawurst @yanganto please let me know what else I can do to help get this merged

Hi @seanaye,

The previous suggestion about no additional struct in the PR may not help you merge this PR, but it is good to respond even if you don't think we need any changes.

I'm not quite sure what you mean by this, I don't see any previous feedback. If you review the PR I'm happy to make changes

@yanganto
Copy link
Contributor

Does it work with gpg.program = gpg2?
What program have we already tested?

It will be good to have a CI if we want to make sure this feature always works in the future.

@seanaye
Copy link
Author

seanaye commented Sep 14, 2024

Does it work with gpg.program = gpg2?

What program have we already tested?

It will be good to have a CI if we want to make sure this feature always works in the future.

I have tested so far with the 1Password ssh agent but the CLI arguments are standardized as far as I know

https://developer.1password.com/docs/ssh/git-commit-signing/

I think adding this to CI may be difficult as it would require adding new system dependencies to the image where the CI runs. For example installing the 1Password CLI inside a docker image.

@yonas
Copy link

yonas commented Oct 1, 2024

Ping


impl SSHProgram {
pub fn new(config: &git2::Config) -> Self {
match dbg!(config.get_string("gpg.ssh.program")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the debug code dbg! here?

asyncgit/src/sync/sign.rs Show resolved Hide resolved
asyncgit/src/sync/sign.rs Show resolved Hide resolved
asyncgit/src/sync/sign.rs Show resolved Hide resolved
@yanganto
Copy link
Contributor

I thought the dbg!, eprintln!, and debug_cmd_print! in this project is only in used in test cases, if I am wrong please also let me know. Thanks. 🙏

@robrecord
Copy link

robrecord commented Oct 19, 2024

@kucho @dkarter @robrecord anyone who can help by building this branch and testing it with their setup and reporting back here can help get this merged.

Thank you for requesting my feedback.

I have built this branch and tried to make it work but I am getting the same error: "sign builder error: Failed to retrieve 'user.signingkey' from the git configuration: Currently, we only support a pair of ssh key in disk."

As far as I can see I have the same essential config as seaneye.

@seanaye I assume it is working for you; are there any special steps I need to take to allow this to work please?

Using 1Password for Mac 8.10.48 (81048025)

$ gitui --version 
gitui nightly 2024-10-19 (f45a5a4)
# global git config:
[user]
	signingkey = /path/to/my/key.pub
[commit]
	gpgsign = true
[gpg]
	format = ssh
[gpg "ssh"]
	program = /Applications/1Password.app/Contents/MacOS/op-ssh-sign

Nothing overridden in my local config.

I also tried using a program called Secretive (Version 2.4.1 (1.7648958148)). I commented out the ssh program and changed the key path to my Secretive key. I got the same error as above.

Lastly I fell back to using plain SSH on disk without a signing program. On committing usign gitui, the error was now different: "the private key is encrypted", as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ssh signing] support custom signing program
7 participants