Auto merge of #2407 - alexcrichton:better-failed-auth-error, r=brson

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

Author: bors

src/cargo/sources/git/utils.rs

@@ -1,6 +1,7 @@
+use std::env;
 use std::fmt;
-use std::path::{Path, PathBuf};
 use std::fs::{self, File};
+use std::path::{Path, PathBuf};
 
 use rustc_serialize::{Encodable, Encoder};
 use url::Url;
@@ -358,66 +359,169 @@ impl<'a> GitCheckout<'a> {
     }
 }
 
+/// Prepare the authentication callbacks for cloning a git repository.
+///
+/// The main purpose of this function is to construct the "authentication
+/// callback" which is used to clone a repository. This callback will attempt to
+/// find the right authentication on the system (without user input) and will
+/// guide libgit2 in doing so.
+///
+/// The callback is provided `allowed` types of credentials, and we try to do as
+/// much as possible based on that:
+///
+/// * Prioritize SSH keys from the local ssh agent as they're likely the most
+///   reliable. The username here is prioritized from the credential
+///   callback, then from whatever is configured in git itself, and finally
+///   we fall back to the generic user of `git`.
+///
+/// * If a username/password is allowed, then we fallback to git2-rs's
+///   implementation of the credential helper. This is what is configured
+///   with `credential.helper` in git, and is the interface for the OSX
+///   keychain, for example.
+///
+/// * After the above two have failed, we just kinda grapple attempting to
+///   return *something*.
+///
+/// If any form of authentication fails, libgit2 will repeatedly ask us for
+/// credentials until we give it a reason to not do so. To ensure we don't
+/// just sit here looping forever we keep track of authentications we've
+/// attempted and we don't try the same ones again.
 fn with_authentication<T, F>(url: &str, cfg: &git2::Config, mut f: F)
                              -> CargoResult<T>
     where F: FnMut(&mut git2::Credentials) -> CargoResult<T>
 {
-    // Prepare the authentication callbacks.
-    //
-    // We check the `allowed` types of credentials, and we try to do as much as
-    // possible based on that:
-    //
-    // * Prioritize SSH keys from the local ssh agent as they're likely the most
-    //   reliable. The username here is prioritized from the credential
-    //   callback, then from whatever is configured in git itself, and finally
-    //   we fall back to the generic user of `git`.
-    //
-    // * If a username/password is allowed, then we fallback to git2-rs's
-    //   implementation of the credential helper. This is what is configured
-    //   with `credential.helper` in git, and is the interface for the OSX
-    //   keychain, for example.
-    //
-    // * After the above two have failed, we just kinda grapple attempting to
-    //   return *something*.
-    //
-    // Note that we keep track of the number of times we've called this callback
-    // because libgit2 will repeatedly give us credentials until we give it a
-    // reason to not do so. If we've been called once and our credentials failed
-    // then we'll be called again, and in this case we assume that the reason
-    // was because the credentials were wrong.
     let mut cred_helper = git2::CredentialHelper::new(url);
     cred_helper.config(cfg);
-    let mut called = 0;
+
+    let mut attempted = git2::CredentialType::empty();
+    let mut failed_cred_helper = false;
+
+    // We try a couple of different user names when cloning via ssh as there's a
+    // few possibilities if one isn't mentioned, and these are used to keep
+    // track of that.
+    enum UsernameAttempt {
+        Arg,
+        CredHelper,
+        Local,
+        Git,
+    }
+    let mut username_attempt = UsernameAttempt::Arg;
+    let mut username_attempts = Vec::new();
+
     let res = f(&mut |url, username, allowed| {
-        called += 1;
-        if called >= 2 {
-            return Err(git2::Error::from_str("no authentication available"))
+        let allowed = allowed & !attempted;
+
+        // libgit2's "USERNAME" authentication actually means that it's just
+        // asking us for a username to keep going. This is currently only really
+        // used for SSH authentication and isn't really an authentication type.
+        // The logic currently looks like:
+        //
+        //      let user = ...;
+        //      if (user.is_null())
+        //          user = callback(USERNAME, null, ...);
+        //
+        //      callback(SSH_KEY, user, ...)
+        //
+        // So if we have a USERNAME request we just pass it either `username` or
+        // a fallback of "git". We'll do some more principled attempts later on.
+        if allowed.contains(git2::USERNAME) {
+            attempted = attempted | git2::USERNAME;
+            return git2::Cred::username(username.unwrap_or("git"))
         }
-        if allowed.contains(git2::SSH_KEY) ||
-                       allowed.contains(git2::USERNAME) {
-            let user = username.map(|s| s.to_string())
-                               .or_else(|| cred_helper.username.clone())
-                               .unwrap_or("git".to_string());
-            if allowed.contains(git2::USERNAME) {
-                git2::Cred::username(&user)
-            } else {
-                git2::Cred::ssh_key_from_agent(&user)
+
+        // An "SSH_KEY" authentication indicates that we need some sort of SSH
+        // authentication. This can currently either come from the ssh-agent
+        // process or from a raw in-memory SSH key. Cargo only supports using
+        // ssh-agent currently.
+        //
+        // We try a few different usernames here, including:
+        //
+        //  1. The `username` argument, if provided. This will cover cases where
+        //     the user was passed in the URL, for example.
+        //  2. The global credential helper's username, if any is configured
+        //  3. The local account's username (if present)
+        //  4. Finally, "git" as it's a common fallback (e.g. with github)
+        if allowed.contains(git2::SSH_KEY) {
+            loop {
+                let name = match username_attempt {
+                    UsernameAttempt::Arg => {
+                        username_attempt = UsernameAttempt::CredHelper;
+                        username.map(|s| s.to_string())
+                    }
+                    UsernameAttempt::CredHelper => {
+                        username_attempt = UsernameAttempt::Local;
+                        cred_helper.username.clone()
+                    }
+                    UsernameAttempt::Local => {
+                        username_attempt = UsernameAttempt::Git;
+                        env::var("USER").or_else(|_| env::var("USERNAME")).ok()
+                    }
+                    UsernameAttempt::Git => {
+                        attempted = attempted | git2::SSH_KEY;
+                        Some("git".to_string())
+                    }
+                };
+                if let Some(name) = name {
+                    let ret = git2::Cred::ssh_key_from_agent(&name);
+                    username_attempts.push(name);
+                    return ret
+                }
             }
-        } else if allowed.contains(git2::USER_PASS_PLAINTEXT) {
-            git2::Cred::credential_helper(cfg, url, username)
-        } else if allowed.contains(git2::DEFAULT) {
-            git2::Cred::default()
-        } else {
-            Err(git2::Error::from_str("no authentication available"))
         }
+
+        // Sometimes libgit2 will ask for a username/password in plaintext. This
+        // is where Cargo would have an interactive prompt if we supported it,
+        // but we currently don't! Right now the only way we support fetching a
+        // plaintext password is through the `credential.helper` support, so
+        // fetch that here.
+        if allowed.contains(git2::USER_PASS_PLAINTEXT) {
+            attempted = attempted | git2::USER_PASS_PLAINTEXT;
+            let r = git2::Cred::credential_helper(cfg, url, username);
+            failed_cred_helper = r.is_err();
+            return r
+        }
+
+        // I'm... not sure what the DEFAULT kind of authentication is, but seems
+        // easy to support?
+        if allowed.contains(git2::DEFAULT) {
+            attempted = attempted | git2::DEFAULT;
+            return git2::Cred::default()
+        }
+
+        // Whelp, we tried our best
+        Err(git2::Error::from_str("no authentication available"))
     });
-    if called > 0 {
-        res.chain_error(|| {
-            human("failed to authenticate when downloading repository")
-        })
-    } else {
-        res
+
+    if attempted.bits() == 0 || res.is_ok() {
+        return res
     }
+
+    // In the case of an authentication failure (where we tried something) then
+    // we try to give a more helpful error message about precisely what we
+    // tried.
+    res.chain_error(|| {
+        let mut msg = "failed to authenticate when downloading \
+                       repository".to_string();
+        if attempted.contains(git2::SSH_KEY) {
+            let names = username_attempts.iter()
+                                         .map(|s| format!("`{}`", s))
+                                         .collect::<Vec<_>>()
+                                         .join(", ");
+            msg.push_str(&format!("\nattempted ssh-agent authentication, but \
+                                   none of the usernames {} succeeded", names));
+        }
+        if attempted.contains(git2::USER_PASS_PLAINTEXT) {
+            if failed_cred_helper {
+                msg.push_str("\nattempted to find username/password via \
+                              git's `credential.helper` support, but failed");
+            } else {
+                msg.push_str("\nattempted to find username/password via \
+                              `credential.helper`, but maybe the found \
+                              credentials were incorrect");
+            }
+        }
+        human(msg)
+    })
 }
 
 pub fn fetch(repo: &git2::Repository, url: &str,

tests/test_cargo_build_auth.rs

@@ -108,6 +108,7 @@ Caused by:
 
 Caused by:
   failed to authenticate when downloading repository
+attempted to find username/password via `credential.helper`, but [..]
 
 To learn more, run the command again with --verbose.
 ",