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

default username used for git clones over ssh is not generally correct #2399

Closed
eminence opened this issue Feb 19, 2016 · 22 comments
Closed

Comments

@eminence
Copy link
Contributor

This might be a duplicate of #1851 or #2078 but I can't quite tell. My understanding is that cargo is supposed to use ssh-agent for authentication, but I can't get this to work.

I have a dependency with a git-over-ssh path:

[dependencies.sma_utils]
git = "ssh://gitsrvsim/usr/abaqus60/GITrepo/sma_rust.git"

SSH to this machine is password-less (due to ssh-agent, and not due a .ssh/config file):

$ ssh -o BatchMode=yes gitsrvsim hostname
Permission denied (publickey,gssapi-with-mic,password).
$ ssh-add ~/.ssh/sim_internal
Identity added: /u/users/lxd/.ssh/sim_internal (/u/users/lxd/.ssh/sim_internal)
$ ssh -o BatchMode=yes gitsrvsim hostname
gitsrvsim

But cargo build still fails:

cargo build --verbose
    Updating registry `https://github.com/rust-lang/crates.io-index`
    Updating git repository `ssh://gitsrvsim/usr/abaqus60/GITrepo/sma_rust.git`
Unable to update ssh://gitsrvsim/usr/abaqus60/GITrepo/sma_rust.git

Caused by:
  failed to fetch into /rd/gen/lxd/do_not_delete/multirust/.cargo/git/db/sma_rust-0fec95efab468a0c

Caused by:
  failed to authenticate when downloading repository

Caused by:
  [7/-1] Config value 'credential.helper' was not found

My quick googling suggests that git's credential-helper is primarily used for HTTPS authentication. No one seems to use it for ssh (likely because git can use ~/.ssh/config). My git is not new enough to have the credential-helper subsystem.

So I can't really tell where the problem lies. Currently running `cargo 0.9.0-nightly (4739ba1 2016-02-16)

Thanks!

@alexcrichton
Copy link
Member

Yeah I think that this is a dupe, but I'm not 100% sure. Can you try specifying the full URL instead of just gitsrvsim? That .ssh/config alias definitely won't be read by us (or libgit2) I believe

@eminence
Copy link
Contributor Author

gitsrvsim is not an alias. It's resolvable when you tack on a suffix from /etc/resolv.conf. So using the fully qualified domain name in Cargo.toml didn't change anything

@alexcrichton
Copy link
Member

Note that we actually don't shell out to git at all, we use libgit2 as a bundled library which implements all the git operations we need. We specifically try a number of authentication strategies here, and I wonder if perhaps SSH_KEY isn't coming through somehow?

Unfortunately there's not a lot of debug logging around there, so I'm curious:

  • Is there a way for me to reproduce this locally?
  • Would you be willing to try out some debug builds of Cargo to see if it works?

@eminence
Copy link
Contributor Author

I would be happy to try out any and all debug builds!

I do believe this can be reproduced (I don't think there is anything unique about this particular machine). Let me try to put together a repo script. I'll get back to you on this.

@eminence
Copy link
Contributor Author

  • Step 0 (optional) : Configure machine for pubkey authentication only. Actually, disabling keyboard-interactive is not required to reproduce this, but it can be useful to show that your ssh agent is working.

I'm sure you know how to do this, but for anyone else following along at home, the steps are documented here https://gist.github.com/eminence/93aab78611b9279d8b35

  • Create a project that will be the dependency. Add a trivial thing to it, so that it'll be git-cloneable
achin@bigbox ~/tmp/01 $ cargo init mydep 
achin@bigbox ~/tmp/01 $ cd mydep/
achin@bigbox ~/tmp/01/mydep $ echo "fn foo() -> u8 { 4 }" > src/lib.rs 
achin@bigbox ~/tmp/01/mydep $ git add Cargo.toml src/lib.rs 
achin@bigbox ~/tmp/01/mydep $ git commit -m "initial commit"
[master (root-commit) 77cfe74] initial commit
 2 files changed, 7 insertions(+)
 create mode 100644 Cargo.toml
 create mode 100644 src/lib.rs
  • Create a new project that will depend on mydep.
achin@bigbox ~/tmp/01 $ cargo new --bin myapp
achin@bigbox ~/tmp/01 $ cd myapp/
achin@bigbox ~/tmp/01/myapp $ vim Cargo.toml 
achin@bigbox ~/tmp/01/myapp $ cat Cargo.toml 
[package]
name = "myapp"
version = "0.1.0"
authors = ["Andrew Chin <achin@eminence32.net>"]

[dependencies]

[dependencies.mydep]
git = "ssh://localhost/storage/home/achin/tmp/01/mydep"
  • Now try to build:
achin@bigbox ~/tmp/01/myapp $ cargo build --verbose
    Updating git repository `ssh://localhost/storage/home/achin/tmp/01/mydep`
Unable to update ssh://localhost/storage/home/achin/tmp/01/mydep

Caused by:
  failed to fetch into /storage/home/achin/.multirust/toolchains/nightly/cargo/git/db/mydep-f9f860195307f1db

Caused by:
  failed to authenticate when downloading repository

Caused by:
  [7/-1] Config value 'credential.helper' was not found
  • Observe that a plain git-clone is successful:
achin@bigbox ~/tmp/01/myapp $ git clone ssh://localhost/storage/home/achin/tmp/01/mydep
Cloning into 'mydep'...
remote: Counting objects: 5, done.
remote: Compressing objects: 100% (3/3), done.
remote: Total 5 (delta 0), reused 0 (delta 0)
Receiving objects: 100% (5/5), done.
Checking connectivity... done.

@alexcrichton
Copy link
Member

Awesome, thanks @eminence! There's two issues at play here:

  • The root cause is the username we attempt to connect with via ssh. Without any extra information we fall back to the username of git, but that's probably the wrong username for you (it was for me)
  • The error message here is quite bad! I'm working on a PR to fix that.

@eminence
Copy link
Contributor Author

Ah ha!

Yes, explicitly specifying a username works! However, this is not ideal, since the username that I need to access "localhost" (or "gitsrvsim") is not the username that you need to access "localhost" or ("gitsrvsim"). Perhaps if no username is specified, default to the OS user name (and if that can't be determined, then fallback to git) ?

@eminence eminence changed the title cargo, ssh, git, and ssh-agent default username used for git clones over ssh is not generally correct Feb 22, 2016
@alexcrichton
Copy link
Member

Hm unfortunately libgit2 provides a kinda wacky interface to interacting with authorizations, but I think we could track something like that, yeah.

alexcrichton added a commit to alexcrichton/cargo that referenced this issue Feb 25, 2016
This commit is an attempt to improve the error message from failed
authentication attempts as well as attempting more usernames. Right now we only
attempt one username, but there are four different possible choices we could
select (including $USER which we weren't previously trying).

This commit tweaks a bunch of this logic and just in general refactors the
with_authentication function.

Closes rust-lang#2399
bors added a commit that referenced this issue Feb 25, 2016
This commit is an attempt to improve the error message from failed
authentication attempts as well as attempting more usernames. Right now we only
attempt one username, but there are four different possible choices we could
select (including $USER which we weren't previously trying).

This commit tweaks a bunch of this logic and just in general refactors the
with_authentication function.

Closes #2399
@eminence
Copy link
Contributor Author

eminence commented Mar 1, 2016

I'm now running with this change, but things still are not quite working (but failing in a different way this time!)

lxd@east6sim:/scratch/lxd/00/myapp> cargo build --verbose
    Updating git repository `ssh://localhost/scratch/lxd/00/mydep`
Unable to update ssh://localhost/scratch/lxd/00/mydep

Caused by:
  failed to clone into: /rd/gen/lxd/do_not_delete/multirust/.cargo/git/db/mydep-d4bb305298ecb371

Caused by:
  failed to authenticate when downloading repository

Caused by:
  [23/-1] username does not match previous request

The myapp and mydep crates were set up as described the in comment above.

Using cargo 0.9.0-nightly (582dcb7 2016-02-26)

@eminence
Copy link
Contributor Author

eminence commented Mar 1, 2016

(Trying to build/clone without an ssh-agent gives the error error authenticating: no auth sock variable which is much better!)

I see the same problem on another system of mine, so I going to re-open this issue.

Edit: turns out I can't re-open the issue! Please let me know if you would like me to open a new issue for this

@alexcrichton
Copy link
Member

Hm... weird! I'm not sure how this worked for me locally?

@alexcrichton alexcrichton reopened this Mar 2, 2016
@alexcrichton
Copy link
Member

If libgit2 doesn't allow us to change usernames then there may be no recourse here unfortunately.

@eminence
Copy link
Contributor Author

eminence commented Mar 2, 2016

I don't think anyone else is running into this, so I will take an action item to dig into this to try find the source of this particular error

@eminence
Copy link
Contributor Author

I finally dug into this. The situation in with_authentication is as follows:

The first time in here we hit the allowed.contains(git2::USERNAME) block. Since the URL within Cargo.toml doesn't contain any username, the username parameter is None so we simply return "git".

Second time in here we hit the UsernameAttempt::Arg arm of username_attempt match. Now the username parameter is Some("git") so we end up returning "git" via the if let Some(name) = name block.

Since ssh authentication with the username of "git" does not work on my system (I have no user account with that name), we'll then fall back into this callback a third time. We quickly pass through the UsernameAttempt::CredHelper match arm before landing in the UsernameAttempt::Local arm, where Some("achin").

And so libgit2 doesn't like this because first we said to use the username "git" and now we've just said use the username "achin"

So that's the scenario. As for the fix, I am going to argue that returning "git" here is not the correct thing to do.

I can envision 3 scenarios:

  1. You have explicitly specified a username in the URL. In this case, it appears that allowed doesn't contain git2::USERNAME and so we directly go to the UsernameAttempt::Arg arm which is successful, and we are then done without any additional fuss.
  2. You omit the username from the URL, and are relying on your ssh_config file to specify the username. This simply isn't implemented today, and will continue to not work after my proposed change.
  3. You omit the username from the URL and are assuming that your OS username will be used. This is my case. Absent any additional information, we should simply assume that the OS username should be used and not try to recover if that fails.

There might actually be a 4th scenario, when you want to use gitcredentials(7) to do username mapping. I've kind of ignored this here because 1) I'm not sure how often this is actually used for ssh, 2) I'm not sure it currently worked in cargo today, and 3) I'm not sure how this works at all with plain-old-git.

In order to fully complete this story I need to continue to figure out how gitcredentials work. I also cannot explain why things seem to work for you in your environment.

That was a very long explaining for something fairly simple, but hopefully it was worth it by making things clear. With all of this in mind, I have some ideas about how to re-write with_authentication to make it simpler, but I'd like to hear your thoughts first.

@alexcrichton
Copy link
Member

Hm we probably can't change the fallback to git without getting other scenarios to work because that's the username which works for private github repositories, for example. If libgit2 doesn't support multiple usernames in one authentication attempt, however, perhaps we could just make multiple authentication attempts?

Basically I'd imagine that we have a set of usernames we want to try if one isn't specified, and we can just try a bunch in an expensive loop (the network is expensive anyway).

@eminence
Copy link
Contributor Author

Regarding private github repos, how common do you think this is? It seems like https will be far more common than ssh (I'm not even sure ssh urls would work, since the standard operating procedure for github+ssh is to use a ssh_config file, which cargo doesn't support). If they have figured out how to get ssh to work, then hard-coding the "git" username into the URL would be a workaround.

I certainly agree that documented and described behavior is the one we want (try a bunch of usernames/credentials until 1 works). I'll continue to look at this.

@alexcrichton
Copy link
Member

I've at least in the past used ssh URLs I think, but I'm not sure how widely it's used in general. I found that the method of informing git of my https credentials was way more work than just using an SSH url (as my key was already in my agent).

@zchee
Copy link

zchee commented Apr 7, 2016

@alexcrichton @eminence
I also occurred.

but comment-out this lines on ~/.gitconfig, solved the problem.
I'm usually insteadOf url. Maybe this setting popular.
If cargo command does not support for it, I think that there is a need.

I hope this will help.

[http]
    cookiefile = ~/.config/git/.gitcookies
    sslCAInfo = /usr/local/etc/ssl/certs/curl-ca-bundle.crt

# URL shorthands
[url "git@github.com:"]
    insteadOf = "https://github.com/"
    pushInsteadOf = "github:"
    pushInsteadOf = "git://github.com/"

[url "git://github.com/"]
    insteadOf = "github:"

[url "git@gist.github.com:"]
    insteadOf = "gst:"
    pushInsteadOf = "gist:"
    pushInsteadOf = "git://gist.github.com/"

[url "git://gist.github.com/"]
    insteadOf = "gist:"

@gsquire
Copy link

gsquire commented Apr 10, 2016

I too ran into this issue, using the insteadOf configuration for git. I had used this so that I could run go get for private repos.

@alexcrichton
Copy link
Member

Ok, I've attempted a fix for this again at #2584, @eminence could you try that out?

alexcrichton added a commit to alexcrichton/cargo that referenced this issue Apr 18, 2016
This commit corrects the logic for attempting multiple usernames with libgit2.
There is a restriction that for each authentication seession you can only
authenticate with one ssh username, but we were attempting multiple as part of
the same sesssion. Instead restart authentication whenever we try a new username.

Closes rust-lang#2399
bors added a commit that referenced this issue Apr 18, 2016
Correctly attempt multiple usernames with libgit2

This commit corrects the logic for attempting multiple usernames with libgit2.
There is a restriction that for each authentication seession you can only
authenticate with one ssh username, but we were attempting multiple as part of
the same sesssion. Instead restart authentication whenever we try a new username.

Closes #2399
@eminence
Copy link
Contributor Author

@alexcrichton , I'm sorry for taking so long to test this can get back to you, but I have good news! It works! Many thanks for tackling this issue! 🎉

@nhed
Copy link

nhed commented Nov 3, 2018

Note that we actually don't shell out to git at all, we use libgit2 as a bundled library which implements all the git operations we need. We specifically try a number of authentication strategies here, and I wonder if perhaps SSH_KEY isn't coming through somehow?

@alexcrichton there is no way that link still is valid years later ... better to have a hash specific

@Byron Byron mentioned this issue Dec 30, 2022
16 tasks
bors added a commit that referenced this issue Mar 2, 2023
gitoxide integration: fetch

This PR is the first step towards resolving #1171.

In order to get there, we integrate `gitoxide` into `cargo` in such a way that one can control its usage in nightly via `-Zgitoxide` or `Zgitoxide=<feature>[,featureN]`.

Planned features are:

* **fetch** - all fetches are done with `gitxide` (this PR)
* **shallow_index** - the crates index will be a shallow clone (_planned_)
* **shallow_deps** - git dependencies will be a shallow clone (_planned_)
* **checkout** - plain checkouts with `gitoxide` (_planned_)

The above list is a prediction and might change as we understand the requirements better.

### Testing and Transitioning

By default, everything stays as is. However, relevant tests can be re-runwith `gitoxide` using

```
RUSTFLAGS='--cfg always_test_gitoxide' cargo test git
```

There are about 200 tests with 'git' in their name and I plan to enable them one by one. That way the costs for CI stay managable (my first measurement with one test was 2min 30s), while allowing to take one step at a time.

Custom tests shall be added once we realize that more coverage is needed.

That way we should be able to maintain running `git2` and `gitoxide` side by side until we are willing to switch over to `gitoxide` entirely on stable cargo. Then turning on `git2` might be a feature toggle for a while until we finally remove it from the codebase.

_Please see the above paragraph as invitation for discussion, it's merely a basis to explore from and improve upon._

### Tasks

* [x] add feature toggle
* [x] setup test system with one currently successful test
* [x] implement fetch with `gitoxide` (MVP)
* [x] fetch progress
* [x] detect spurious errors
* [x] enable as many git tests as possible (and ignore what's not possible)
* [x] fix all git-related test failures (except for 1: built-in upload-pack, skipped for now)
* [x] validate that all HTTP handle options that come from `cargo` specific values are passed to `gitoxide`
* [x] a test to validate `git2` code can handle crates-index clones created with `gitoxide` and vice-versa
* [x] remove patches that enabled `gitoxide` enabled testing - it's not used anymore
* [x] ~~remove all TODOs and use crates-index version of `git-repository`~~ The remaining 2 TODO's are more like questions for the reviewer.
* [x] run all tests with gitoxide on the fastest platform as another parallel task
* [x] switch to released version
* [x] [Tasks from first review round](#11448 (comment))
* [x] create a new `gitoxide` release and refer to the latest version from crates.io (instead of git-dependency)
* [x] [address 2nd review round comments](#11448 (comment))

### Postponed Tasks

I suggest to go breadth-first and implement the most valuable features first, and then aim for a broad replacement of `git2`. What's left is details and improved compatibility with the `git2` implementation that will be required once `gitoxide` should become the default implementation on stable to complete the transition.

* **built-in support for serving the `file` protocol** (i.e. without using `git`). Simple cases like `clone` can probably be supported quickly, `fetch` needs more work though due to negotiation.
* SSH name fallbacks via a native (probably ~~libssh~~ (avoid LGPL) `libssh2` based) transport. Look at [this issue](#2399) for some history.
* additional tasks from [this tracking issue](GitoxideLabs/gitoxide#450 (comment))

### Proposed Workflow

I am now using [stacked git](https://stacked-git.github.io) to keep commits meaningful during development. This will also mean that before reviews I will force-push a lot as changes will be bucketed into their respective commits.

Once review officially begins I will stop force-pushing and create small commits to address review comments. That way it should be easier to understand how things change over time.

Those review-comments can certainly be squashed into one commit before merging.

_Please let me know if this is feasible or if there are other ways of working you prefer._

### Development notes

* unrelated: [this line](https://github.com/rust-lang/cargo/blob/9827412fee4f5a88ac85e013edd954b2b63f399b/src/cargo/ops/registry.rs#L620) refers to an issue that has since been resolved in `curl`.
* Additional tasks related to a correct fetch implementation are collected in this [tracking issue](GitoxideLabs/gitoxide#450). **These affect how well the HTTP transport can be configured, needs work**
* _authentication_ [is quite complex](https://github.com/rust-lang/cargo/blob/37cad5bd7f7dcd2f6d3e45312a99a9d3eec1e2a0/src/cargo/sources/git/utils.rs#L490) and centred around making SSH connections work. This feature is currently the weakest in `gitoxide` as it simply uses `ssh` (the program) and calls it a day.  No authentication flows are supported there yet and the goal would be to match `git` there at least (which it might already do by just calling `ssh`). Needs investigation. Once en-par with `git` I think `cargo` can restart the whole fetch operation to try different user names like before.
   - the built-in `ssh`-program based transport can now understand permission-denied errors, but the capability isn't used after all since a builtin ssh transport is required.
* It would be possible to implement `git::Progress` and just ignore most of the calls, but that's known to be too slow as the implementation assumes a `Progress::inc()` call is as fast as an atomic increment and makes no attempt to reduce its calls to it.
* learning about [a way to get custom traits in `thiserror`](dtolnay/thiserror#212) could make spurious error checks nicer and less error prone during maintenance. It's not a problem though.
* I am using `RUSTFLAGS=--cfg` to influence the entire build and unit-tests as environment variables didn't get through to the binary built and run for tests.

### Questions

* The way `gitoxide` is configured the user has the opportunity to override these values using more specific git options, for example using url specific http settings. This looks like a feature to me, but if it's not `gitoxide` needs to provide a way to disable applying these overrides. Please let me know what's desired here - my preference is to allow overrides.
* `gitoxide` currently opens repositories similar to how `git` does which respects git specific environment variables. This might be a deviation from how it was before and can be turned off. My preference is to see it as a feature.

### Prerequisite PRs

* #11602
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