Add credential prompts for U2F-backed SSH keys

[bdach]

This would close #2230 for me. Apologies if unwanted, but the issue has been hanging there without a response for a bit now so I figure that maybe I can just PR this for everyone's sake and get things sorted this way?

PR Description

The 8.2 release of OpenSSH added support for FIDO/U2F hardware authenticators, which manifests in being able to create new types of SSH key, named ecdsa-sk nad ed25519-sk. This is relevant to lazygit, as those SSH keys can be used to authorise git operations over SSH, as well as signing git commits. Actual code changes are required for correct support, as the authentication process for these types of keys is different than the process for types supported previously.

When an operation requiring credentials is initialised with a U2F authenticator-backed key, the first prompt is:

Enter PIN for ${key_type} key ${path_to_key}:

at which point the user is supposed to enter a numeric (and secret) PIN, specific to the particular FIDO/U2F authenticator using which the SSH keypair was generated. Upon entering the correct key, the user is supposed to physically interact with the authenticator to confirm presence. Sometimes this is accompanied by the following text prompt:

Confirm user presence for key ${key_type} ${key_fingerprint}

This second prompt does not always occur and it is presumed that the user will know to perform this step even if not prompted specifically. At this stage some authenticator devices may also begin to blink a LED to indicate that they're waiting for input.

To facilitate lazygit's interoperability with these types of keys, add support for the first PIN prompt, which allows "fetch", "pull", and "push" git operations to complete.

Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run go run scripts/cheatsheet/main.go generate)
  • No changes.
• Code has been formatted (see here)
• Tests have been added/updated (see here for the integration test guide)
  • The requirements for generating a U2F-backed SSH key seem prohibitive to adding testing for this enhancement.
• Text is internationalised (see here)
• Docs (specifically docs/Config.md) have been updated if necessary
  • Don't think there's much to update there.
• You've read through your own file changes for silly mistakes etc

Preview video

It probably doesn't help much to include this but I figure I might as well, as baseline proof that this patch does Something™:

2022-10-31.22-59-58.mp4

[jesseduffield]

Thanks for making this! I really appreciate the detailed PR description. The code looks good, and the change is simple enough. Regarding testing, I'll take your word for it that actually coming up with a key to test is hard, though I wonder if we could still add a test that takes a less direct approach. For example we could write a test which sets core.askPass to a bash script that mimics what would happen in the real world i.e. print to stdout Enter PIN for foo key bar: and then verify that the entered pin is some arbitrary number. Then inside lazygit we can assert that the prompt comes up asking for the pin and that upon entering the pin, the push is successful.

What are your thoughts on something like that? I'm happy to help out in any way I can.

[bdach]

I'm not super sure why I didn't think of this approach, but it sounds viable in theory. I'll give it a whirl.

I may be unavailable for a while this week and the next, so I'm not sure how soon I will be able to try this out and get something along those lines going. If you think it's best that I close this for now and reopen with testing (or upon deciding that adding tests is unlikely - although I'd really loath to do that), then let me know and I'll get out of your hair for the time being.

[jesseduffield]

Sweet! I'm happy to keep the PR up in the meantime

[marijanp]

I've successfully tried out this patch, however there is one thing I observed: lazygit fetches some repo data on start, you can see that the command log will ask you to enter the pin for the security key. We will probably want to have a prompt for those cases too.

[bdach]

@marijanp You are correct, that said I do feel like that particular issue is out of scope for this pull request as I'm relatively sure the same will happen with a standard SSH key with a passphrase that you don't have cached in an SSH agent or otherwise. I'm basing this on the following trace of execution:

lazygit/pkg/gui/gui.go

Lines 529 to 532 in 924a152

	if userConfig.Git.AutoFetch {
	fetchInterval := userConfig.Refresher.FetchInterval
	if fetchInterval > 0 {
	go utils.Safe(gui.startBackgroundFetch)

lazygit/pkg/gui/global_handlers.go

Lines 165 to 166 in 924a152

	func (gui *Gui) backgroundFetch() (err error) {
	err = gui.git.Sync.Fetch(git_commands.FetchOptions{Background: true})

lazygit/pkg/commands/git_commands/sync.go

Lines 81 to 82 in 924a152

	if opts.Background {
	cmdObj.DontLog().FailOnCredentialRequest()

which suggests that lazygit is designed to silently fail whenever a fetch is made, but the SSH key's credentials aren't readily available in cache. I personally have autofetch off and don't really need the feature, and I feel trying to fix that in this pull would muddy the waters.

See also: #2235 (comment)

[bdach]

@jesseduffield I've given writing a test a go today, but didn't get it to work as I wanted. This is the branch I ended up with, and this is an actual commit with a test. There is a working two-repository setup there, but the pull command I'm issuing isn't using the installed core.askpass helper. I'm fearing that testing this would require spinning up a HTTP server at least, if not even that plus SSH.

I'll give it another think in a few days but it's looking a bit grim. Google isn't helping much either to find out how to force git to go through the credential helper, since most results are about making it shut up (figures...). I may need to read up on git source to find the answer.

[jesseduffield]

@bdach nice work on making it this far. I'm playing around with the test locally and I'm also not getting the prompt. I think you might be right about needing to get the remote itself to prompt for a password. But I want to do some more digging to see if it really has to come to that

[jesseduffield]

so here's an idea: we could set GIT_SSH to the path of a bespoke script which itself invokes the core.askpass script. Then before starting the test we change set the url of our remote to some fake ssh url like [email protected]:jesseduffield/foo.git, to ensure that GIT_SSH is triggered. we would need to add a string slice field to our test struct called 'env' which would then be appended to the cmd before we run it (in pkg/integration/components/runner.go:111). But that's a bit convoluted.

Another approach could be to mock out git itself (we'd need to pass a modified PATH env var to lazygit to get the mocked git).

Neither of these options strike me as very pleasant, I'm just spitballing.

[mark2185]

Why not just host a git repo locally for that test? It's as close to the real use case as it gets.

[jesseduffield]

Yeah the more I think about it, the more I think it might be worth biting the bullet and actually getting an git server going, so that we don't need to do anything hacky. But to do that in a sandboxed way we'd basically have to get docker involved (correct me if I'm wrong) which is a shame cos it's nice to be able to run tests without docker.

That's a big task that I don't want to block this PR, so if we decide that's what's best, I'm happy for this to go in without an integration test

[marijanp]

@jesseduffield did you ever hear of our lord and savior Nix which allows for real sandboxing unlike stateful docker

[jesseduffield]

I don't really see how docker is stateful, unless you opt to use a volume. Even then you can mount a volume as read only so that nothing changes on the host system once you down the containers

[bdach]

I've read the proposals above, then done some googling, and I can't say that I'm particularly thrilled to try any of the options proposed. Mocking an SSH client would involve somehow understanding the input/output format of any of the three available variants of SSH client that git supports, I guess? Which doesn't look to be documented anywhere in plain language, as far as I can tell, and is a moving target (if any of those change anything in terms of behaviour, then it's update test time). And that's just the authorisation step, without any of the actual file operations.

And as stated spinning up an actual real server instance feels like shooting a fly with a bazooka in the context of this change. Not to mention that it likely doesn't even help that much because the problem still remains that the key type that this PR aims to support can't be instantiated without a corresponding hardware authenticator anyway, so we'd be at GIT_ASKPASS again, plus a-whole-nother mock ssh server spun up just to be able to use it.

[jesseduffield]

Agreed @bdach. Will merge after tests pass