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

Cannot run jj git push: using ssh_key_from_agent loop #1970

Closed
prestonvanloon opened this issue Aug 3, 2023 · 34 comments
Closed

Cannot run jj git push: using ssh_key_from_agent loop #1970

prestonvanloon opened this issue Aug 3, 2023 · 34 comments

Comments

@prestonvanloon
Copy link
Collaborator

Description

Steps to Reproduce the Problem

  1. jj git push

Not sure how I can provide a repro example. Just started using jj yesterday on an existing git repo.

Expected Behavior

It pushes

Actual Behavior

jj git push -v
2023-08-03T20:04:20.124904Z  INFO jj_cli::cli_util: verbose logging enabled
Branch changes to push to origin:
  Move branch assertion-api from eba925600601 to 2e9b7b55bbeb
2023-08-03T20:04:20.420168Z  INFO run_command: jj_lib::git: using ssh_key_from_agent username="git"
2023-08-03T20:04:20.443888Z  INFO run_command: jj_lib::git: using ssh_key_from_agent username="git"
2023-08-03T20:04:20.467704Z  INFO run_command: jj_lib::git: using ssh_key_from_agent username="git"
... 

The log statement INFO run_command: jj_lib::git: using ssh_key_from_agent username="git" is sent repeatedly in an apparent infinite loop.

Specifications

  • Platform: linux amd64
  • Version: jj 0.8.0-6ee8feea861c95f8246aee9e720ed5db66161006
@martinvonz
Copy link
Owner

Weird! I wonder if that's libssh2 doing retries for some reason (trying different keys? but you probably don't have that many different keys for it to try). Does ssh -F /dev/null git@<hostname> work and what does it output?

@prestonvanloon
Copy link
Collaborator Author

prestonvanloon commented Aug 3, 2023

@martinvonz

ssh -F /dev/null git@github.com                      
PTY allocation request failed on channel 0
Hi prestonvanloon! You've successfully authenticated, but GitHub does not provide shell access.
Connection to github.com closed.

Not sure if this is related or not, but the repository was recently renamed and I still have the old remote url.
The command works for me on other repositories.

@martinvonz
Copy link
Owner

The ssh output looks as expected.

Not sure if this is related or not, but the repository was recently renamed and I still have the old remote url.
The command works for me on other repositories.

That's curious. Even if it's not related to the repo rename, it's a useful piece of data that it works in other repos. Does it work in a fresh clone of the repo?

@prestonvanloon
Copy link
Collaborator Author

Yes -- It works with a fresh clone of the repo. It also works if I clone it under the old remote url too.

@sullyj3
Copy link
Collaborator

sullyj3 commented Aug 4, 2023

same issue on jj 0.8.0-8c9fc9880c1796988f853d055b683f1ab70514b6. In an existing git repo.

@martinvonz
Copy link
Owner

Does it work in a fresh clone for you too, @sullyj3? I have no idea what might cause it.

@sullyj3
Copy link
Collaborator

sullyj3 commented Aug 4, 2023

Yeah, it did work. Bizarre.

@sullyj3
Copy link
Collaborator

sullyj3 commented Aug 4, 2023

Still works regardless of whether it was cloned with git clone followed by jj init --git-repo . or jj git clone.

@martinvonz
Copy link
Owner

Bizarre indeed. I don't know how to troubleshoot it more, so I'm going to rely on you and @prestonvanloon to try to figure out what's different between the old and the new repo. There is no retrying in jj's code anyway. I don't know how best to debug it even if i had access to your repos and environments. Maybe adding println!() statements in jj's code, or maybe libgit2 or libssh2 have some logging you can turn on. Or maybe even Wireshark.

@sullyj3
Copy link
Collaborator

sullyj3 commented Aug 4, 2023

If I run into it again I might become sufficiently curious to investigate further.

@jbcdnr
Copy link

jbcdnr commented Aug 4, 2023

Hello, I am hitting the same problem on a fresh repo #1979. There was no recent renaming of the repo I am trying to fetch or pull from.

@martinvonz
Copy link
Owner

I just looked up what libgit2 does and found https://libgit2.org/docs/guides/authentication/#:~:text=The%20callback%20drives%20the%20authentication,which%20authentication%20methods%20are%20available, which includes this:

If the server doesn’t accept the credentials from the callback, it will be called again. The callback drives the authentication loop. It will get called until it returns credentials accepted by the server or it returns an error code.

I suppose that's the loop you're running into. Can someone report what what ssh-add -l says?

@jbcdnr
Copy link

jbcdnr commented Aug 4, 2023

In my case:

$ ssh-add -l
The agent has no identities.

@martinvonz
Copy link
Owner

In my case:

$ ssh-add -l
The agent has no identities.

That would explain why jj can't get a key from it then. If you run ssh-add (and enter your password if it asks for it), does jj git clone work after that?

I don't know how git would find the key. Is your key password protected?

@jbcdnr
Copy link

jbcdnr commented Aug 4, 2023

Yes, it works now by adding explicitly the SSH key ! I don't know if git tries all of them or if I had configured which one to use at some point (but I don't think so).

@jbcdnr
Copy link

jbcdnr commented Aug 4, 2023

Is your key password protected?

No it is not.

And I don't have anything special in ~/.ssh/config.

Thanks a lot for the help and the great tool. Happy to try some things in my setup (ssh-add -d breaks things again) if you want to investigate.

@martinvonz
Copy link
Owner

Is your key password protected?

No it is not.

Ah, so maybe what happens is that jj finds $SSH_AUTH_SOCK and uses only ssh-agent credential, while git first tries ssh-agent and then falls back to check if any password-less keys are available (or maybe it does it in the opposite order). We should do the same.

@prestonvanloon
Copy link
Collaborator Author

Another data point: I changed nothing (that i can recall) and today it works.

My ssh-add -l has two identities in it. One of them (the first one in the list) is the identity linked to my github. The second one in the list is not linked to github.

@prestonvanloon
Copy link
Collaborator Author

I'm now experiencing this problem on another repo. Not sure how what further information I can add, except that the problem seems to happen sometimes but not other times.

@martinvonz
Copy link
Owner

It was actually easy to reproduce this by just unsetting $SSH_AUTH_SOCK and $SSH_AUTH_PID. It looks like we keep trying to use ssh-agent still, even though it fails. I suspect it's a bug in 104f8e1.

@prestonvanloon, did you say you have $SSH_AUTO_SOCK exported (I would think so since ssh-add -l works), and ssh-add -l includes the right key? And jj git fetch still hangs in that shell?

@prestonvanloon
Copy link
Collaborator Author

$SSH_AUTO_SOCK and $SSH_AUTH_PID are not set. It might have been when I commented on Aug 4, but it's not right now (also ssh-add -l also fails today Could not open a connection to your authentication agent.)

@martinvonz
Copy link
Owner

$SSH_AUTO_SOCK and $SSH_AUTH_PID are not set. It might have been when I commented on Aug 4, but it's not right now (also ssh-add -l also fails today Could not open a connection to your authentication agent.)

Sounds like what I was able to reproduce then. So you should be able to work around the bug by making sure that $SSH_AUTH_SOCK (note that my $SSH_AUTO_SOCK above was a typo!). If ssh-add -l works in your shell and jj git fetch still hangs, let me know (I'd also be happy to hear if you get it to work).

@prestonvanloon
Copy link
Collaborator Author

prestonvanloon commented Aug 7, 2023

@martinvonz no worries about the typo. I did printenv | grep SSH and it wasn't there either.

git fetch / pull / push etc works fine without the ssh-agent running with those environment variables. Is it going to be a requirement to have the agent running? I started it with

eval `ssh-agent -s`

and this shows no identities when running ssh-agent -l ssh-add -l. I'm not sure how it was working on Aug 4th, but my expectation is that jj git commands should work if git commands work. What do you think?

@martinvonz
Copy link
Owner

git fetch / pull / push etc works fine without the ssh-agent running with those environment variables. Is it going to be a requirement to have the agent running?

No, it was just a workaround. The bug is that we repeatedly try the ssh-agent even if it's not there, so we never end up trying the password-less key.

I started it with

eval `ssh-agent -s`

and this shows no identities when running ssh-agent -l.

You'll need to run ssh-add and then ssh-add -l (not ssh-agent -l, but maybe that was also just a typo :)) should include your keys. Once ssh-add -l lists your keys, I would expect jj git fetch to work.

I'll work on a fix for the bug.

@yuja
Copy link
Collaborator

yuja commented Aug 7, 2023

I suspect it's a bug in 104f8e1.

Indeed. git2::Cred::ssh_key_from_agent(username) does nothing other than setting up an empty credential object, and _git_ssh_setup_conn() runs request_creds() and _git_ssh_authenticate_session() in a loop. We'll probably need a callback that remembers we've tried agent.

@martinvonz
Copy link
Owner

We'll probably need a callback that remembers we've tried agent.

Yes, that's what I tried and that seemed to work. I'm thinking I should remove the special case for ssh-agent and have the callback go through a list of methods to attempt. Sounds good?

@yuja
Copy link
Collaborator

yuja commented Aug 7, 2023

Yes, that's what I tried and that seemed to work. I'm thinking I should remove the special case for ssh-agent and have the callback go through a list of methods to attempt. Sounds good?

SGTM. As far as I can tell, Remote::download() nor Remote::push() doesn't set up SSH connection more than once, so we can assume the previous credential attempt would be failed.

martinvonz added a commit that referenced this issue Aug 8, 2023
As reported in #1970, SSH authentication would sometimes run into a
loop where it repeatedly tries to use ssh-agent for authentication
without making progess. The problem can be reproduced by simply
removing `$SSH_AUTH_KEY` from your environment (and not having a Git
credentials helper configured, I think).

This seems to be a bug introduced by b104f8e154c21. That commit meant
to make it so we attempt to use ssh-agent and fall back to using
(password-less) keys after that. The problem is that
`git2::Cred::ssh_key_from_agent()` just returns an object that will be
used later for looking up the credentials from ssh-agent, so the call
will not fail because ssh-agent is not reachable.

This commit attempts to fix the problem by having the credentials
callback attempt to use ssh-agent only once.
@martinvonz
Copy link
Owner

I'm thinking I should remove the special case for ssh-agent and have the callback go through a list of methods to attempt.

That seemed more complicated than I had hoped, so I went with the special case for ssh-agent.

martinvonz added a commit that referenced this issue Aug 8, 2023
As reported in #1970, SSH authentication would sometimes run into a
loop where it repeatedly tries to use ssh-agent for authentication
without making progess. The problem can be reproduced by simply
removing `$SSH_AUTH_KEY` from your environment (and not having a Git
credentials helper configured, I think).

This seems to be a bug introduced by b104f8e154c21. That commit meant
to make it so we attempt to use ssh-agent and fall back to using
(password-less) keys after that. The problem is that
`git2::Cred::ssh_key_from_agent()` just returns an object that will be
used later for looking up the credentials from ssh-agent, so the call
will not fail because ssh-agent is not reachable.

This commit attempts to fix the problem by having the credentials
callback attempt to use ssh-agent only once.
@martinvonz
Copy link
Owner

This is hopefully fixed with #2008 merged. Can you try again with a new binary built from head? If you're on Linux or Mac, use this to build from source:

cargo install --git https://github.com/martinvonz/jj.git --locked --bin jj jj-cli

Please let me know whether it works.

@prestonvanloon
Copy link
Collaborator Author

Interestingly, cargo install command had a similar issue when ssh agent was not running.

cargo install --git https://github.com/martinvonz/jj.git --locked --bin jj jj-cli

    Updating git repository `https://github.com/martinvonz/jj.git`
error: failed to fetch into: /home/preston/.cargo/git/db/jj-1467e3dd78fc324a

Caused by:
  failed to authenticate when downloading repository: ssh://git@github.com/martinvonz/jj.git

  * attempted ssh-agent authentication, but no usernames succeeded: `git`

  if the git CLI succeeds then `net.git-fetch-with-cli` may help here
  https://doc.rust-lang.org/cargo/reference/config.html#netgit-fetch-with-cli

Caused by:
  no authentication methods succeeded

Once installed successfully, I now have an error rather than infinite loop.

jj git fetch   
Error: authentication required but no callback set; class=Ssh (23); code=Auth (-16)
Hint: Jujutsu uses libssh2, which doesn't respect ~/.ssh/config. Does `ssh -F /dev/null` to the host work?

@martinvonz
Copy link
Owner

Hmm, it's supposed to try with the key after ssh-agent fails. Your key is not password protected, is it? What does jj git fetch -v say? And just to make sure, does regular git fetch work?

@prestonvanloon
Copy link
Collaborator Author

2023-08-08T14:14:22.998837Z  INFO jj_cli::cli_util: verbose logging enabled
2023-08-08T14:14:23.027602Z DEBUG run_command:cmd_git_fetch{args=GitFetchArgs { branch: [], remotes: [] }}:fetch{remote_name="origin" branch_name_globs=None git_settings=GitSettings { auto_local_branch: true }}: jj_lib::git: remote.download
2023-08-08T14:14:23.278809Z  INFO run_command:cmd_git_fetch{args=GitFetchArgs { branch: [], remotes: [] }}:fetch{remote_name="origin" branch_name_globs=None git_settings=GitSettings { auto_local_branch: true }}: jj_lib::git: using ssh_key_from_agent username="git"
2023-08-08T14:14:23.300430Z  INFO run_command:cmd_git_fetch{args=GitFetchArgs { branch: [], remotes: [] }}:fetch{remote_name="origin" branch_name_globs=None git_settings=GitSettings { auto_local_branch: true }}:get_ssh_key{_username="git"}: jj_cli::commands::git: no ssh key found path="/home/preston/.ssh/id_rsa"
2023-08-08T14:14:23.300453Z  INFO run_command:cmd_git_fetch{args=GitFetchArgs { branch: [], remotes: [] }}:fetch{remote_name="origin" branch_name_globs=None git_settings=GitSettings { auto_local_branch: true }}: jj_lib::git: using default
Error: authentication required but no callback set; class=Ssh (23); code=Auth (-16)
Hint: Jujutsu uses libssh2, which doesn't respect ~/.ssh/config. Does `ssh -F /dev/null` to the host work?

I don't use an RSA ssh key. I think it should use /home/preston/.ssh/id_ed25519?

@martinvonz
Copy link
Owner

I think it should use /home/preston/.ssh/id_ed25519?

Ah, right that's another limitation I had almost forgotten (#440). I'll see what I can do about that too. Closing this now since the hang has been fixed and we do try to use password-less id_rsa keys afterwards now.

@sullyj3
Copy link
Collaborator

sullyj3 commented Aug 9, 2023

Ah yeah, I'm also using passwordless id_ed25519.pub with no ssh_agent

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Oct 12, 2023
[0.10.0] - 2023-10-04

### Breaking changes

* A default revset-alias function `trunk()` now exists. If you previously defined
  your own `trunk()` alias it will continue to overwrite the built-in one.
  Check [revsets.toml](cli/src/config/revsets.toml) and [revsets.md](docs/revsets.md)
  to understand how the function can be adapted.

### New features

* The `ancestors()` revset function now takes an optional `depth` argument
  to limit the depth of the ancestor set. For example, use `jj log -r
  'ancestors(@, 5)` to view the last 5 commits.

* Support for the Watchman filesystem monitor is now bundled by default. Set
  `core.fsmonitor = "watchman"` in your repo to enable.

* You can now configure the set of immutable commits via
  `revset-aliases.immutable_heads()`. For example, set it to
  `"remote_branches() | tags()"` to prevent rewriting those those. Their
  ancestors are implicitly also immutable.

* `jj op log` now supports `--no-graph`.

* Templates now support an additional escape: `\0`. This will output a literal
  null byte. This may be useful for e.g.
  `jj log -T 'description ++ "\0"' --no-graph` to output descriptions only, but
  be able to tell where the boundaries are

* jj now bundles a TUI tool to use as the default diff and merge editors. (The
  previous default was `meld`.)

* `jj split` supports the `--interactive` flag. (This is already the default if
  no paths are provided.)

* `jj commit` accepts an optional list of paths indicating a subset of files to
  include in the first commit

* `jj commit` accepts the `--interactive` flag.

### Fixed bugs

### Contributors

Thanks to the people who made this release happen!

* Austin Seipp (@thoughtpolice)
* Emily Kyle Fox (@emilykfox)
* glencbz (@glencbz)
* Hong Shin (@honglooker)
* Ilya Grigoriev (@ilyagr)
* James Sully (@sullyj3)
* Martin von Zweigbergk (@martinvonz)
* Philip Metzger (@PhilipMetzger)
* Ruben Slabbert (@rslabbert)
* Vamsi Avula (@avamsi)
* Waleed Khan (@arxanas)
* Willian Mori (@wmrmrx))
* Yuya Nishihara (@yuja)
* Zachary Dremann (@Dr-Emann)


[0.9.0] - 2023-09-06

### Breaking changes

* The minimum supported Rust version (MSRV) is now 1.71.0.

* The storage format of branches, tags, and git refs has changed. Newly-stored
  repository data will no longer be loadable by older binaries.

* The `:` revset operator is deprecated. Use `::` instead. We plan to delete the
  `:` form in jj 0.15+.

* The `--allow-large-revsets` flag for `jj rebase` and `jj new` was replaced by
  a `all:` before the revset. For example, use `jj rebase -d 'all:foo-'`
  instead of `jj rebase --allow-large-revsets -d 'foo-'`.

* The `--allow-large-revsets` flag for `jj rebase` and `jj new` can no longer be
  used for allowing duplicate destinations. Include the potential duplicates
  in a single expression instead (e.g. `jj new 'all:x|y'`).

* The `push.branch-prefix` option was renamed to `git.push-branch-prefix`.

* The default editor on Windows is now `Notepad` instead of `pico`.

* `jj` will fail attempts to snapshot new files larger than 1MiB by default.
  This behavior can be customized with the `snapshot.max-new-file-size`
  config option.

* Author and committer signatures now use empty strings to represent unset
  names and email addresses. The `author`/`committer` template keywords and
  methods also return empty strings.
  Older binaries may not warn user when attempting to `git push` commits
  with such signatures.

* In revsets, the working-copy or remote symbols (such as `@`, `workspace_id@`,
  and `branch@remote`) can no longer be quoted as a unit. If a workspace or
  branch name contains whitespace, quote the name like `"branch name"@remote`.
  Also, these symbols will not be resolved as revset aliases or function
  parameters. For example, `author(foo@)` is now an error, and the revset alias
  `'revset-aliases.foo@' = '@'` will be failed to parse.

* The `root` revset symbol has been converted to function `root()`.

* The `..x` revset is now evaluated to `root()..x`, which means the root commit
  is no longer included.

* `jj git push` will now push all branches in the range `remote_branches()..@`
  instead of only branches pointing to `@` or `@-`.

* It's no longer allowed to create a Git remote named "git". Use `jj git remote
  rename` to rename the existing remote.
  [#1690](martinvonz/jj#1690)

* Revset expression like `origin/main` will no longer resolve to a
  remote-tracking branch. Use `main@origin` instead.

### New features

* Default template for `jj log` now does not show irrelevant information
  (timestamp, empty, message placeholder etc.) about the root commit.

* Commit templates now support the `root` keyword, which is `true` for the root
  commit and `false` for every other commit.

* `jj init --git-repo` now works with bare repositories.

* `jj config edit --user` and `jj config set --user` will now pick a default
  config location if no existing file is found, potentially creating parent
  directories.

* `jj log` output is now topologically grouped.
  [#242](martinvonz/jj#242)

* `jj git clone` now supports the `--colocate` flag to create the git repo
  in the same directory as the jj repo.

* `jj restore` gained a new option `--changes-in` to restore files
  from a merge revision's parents. This undoes the changes that `jj diff -r`
  would show.

* `jj diff`/`log` now supports `--tool <name>` option to generate diffs by
  external program. For configuration, see [the documentation](docs/config.md).
  [#1886](martinvonz/jj#1886)

* A new experimental diff editor `meld-3` is introduced that sets up Meld to
  allow you to see both sides of the original diff while editing. This can be
  used with `jj split`, `jj move -i`, etc.

* `jj log`/`obslog`/`op log` now supports `--limit N` option to show the first
  `N` entries.

* Added the `ui.paginate` option to enable/disable pager usage in commands

* `jj checkout`/`jj describe`/`jj commit`/`jj new`/`jj squash` can take repeated
  `-m/--message` arguments. Each passed message will be combined into paragraphs
  (separated by a blank line)

* It is now possible to set a default description using the new
  `ui.default-description` option, to use when describing changes with an empty
  description.

* `jj split` will now leave the description empty on the second part if the
  description was empty on the input commit.

* `branches()`/`remote_branches()`/`author()`/`committer()`/`description()`
  revsets now support exact matching. For example, `branch(exact:main)`
  selects the branch named "main", but not "maint". `description(exact:"")`
  selects commits whose description is empty.

* Revsets gained a new function `mine()` that aliases `author(exact:"your_email")`.

* Added support for `::` and `..` revset operators with both left and right
  operands omitted. These expressions are equivalent to `all()` and `~root()`
  respectively.

* `jj log` timestamp format now accepts `.utc()` to convert a timestamp to UTC.

* templates now support additional string methods `.starts_with(x)`, `.ends_with(x)`
  `.remove_prefix(x)`, `.remove_suffix(x)`, and `.substr(start, end)`.

* `jj next` and `jj prev` are added, these allow you to traverse the history
  in a linear style. For people coming from Sapling and `git-branchles`
  see [#2126](martinvonz/jj#2126) for
  further pending improvements.

* `jj diff --stat` has been implemented. It shows a histogram of the changes,
  same as `git diff --stat`. Fixes [#2066](martinvonz/jj#2066)

* `jj git fetch --all-remotes` has been implemented. It fetches all remotes
  instead of just the default remote

### Fixed bugs

* Fix issues related to .gitignore handling of untracked directories
  [#2051](martinvonz/jj#2051).

* `jj config set --user` and `jj config edit --user` can now be used outside of
  any repository.

* SSH authentication could hang when ssh-agent couldn't be reached
  [#1970](martinvonz/jj#1970)

* SSH authentication can now use ed25519 and ed25519-sk keys. They still need
  to be password-less.

* Git repository managed by the repo tool can now be detected as a "colocated"
  repository.
  [#2011](martinvonz/jj#2011)

### Contributors

Thanks to the people who made this release happen!

* Alexander Potashev (@aspotashev)
* Anton Bulakh (@necauqua)
* Austin Seipp (@thoughtpolice)
* Benjamin Brittain (@benbrittain)
* Benjamin Saunders (@Ralith)
* Christophe Poucet (@poucet)
* Emily Kyle Fox (@emilykfox)
* Glen Choo (@chooglen)
* Ilya Grigoriev (@ilyagr)
* Kevin Liao (@kevincliao)
* Linus Arver (@listx)
* Martin Clausen (@maacl)
* Martin von Zweigbergk (@martinvonz)
* Matt Freitas-Stavola (@mbStavola)
* Oscar Bonilla (@ob)
* Philip Metzger (@PhilipMetzger)
* Piotr Kufel (@qfel)
* Preston Van Loon (@prestonvanloon)
* Tal Pressman (@talpr)
* Vamsi Avula (@avamsi)
* Vincent Breitmoser (@Valodim)
* Vladimir (@0xdeafbeef)
* Waleed Khan (@arxanas)
* Yuya Nishihara (@yuja)
* Zachary Dremann (@Dr-Emann)
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

No branches or pull requests

5 participants