Auto merge of #12590 - arlosi:cred-unsupported-error, r=epage

fix: add error for unsupported credential provider version

Cargo currently ignores the version in the `CredentialHello` message, and proceeds to use version `1` regardless of what the credential provider claims it can support.

This change does the following:
* Adds a new error if Cargo doesn't support any of the supported protocol versions offered by the provider.
* Kills the credential provider subprocess if it fails. This prevents it from hanging or printing spurious errors such as "broken pipe" when it's attempting to read the next JSON message.
* Adds a new test for an unsupported credential provider protocol.

Cargo.lock

@@ -334,7 +334,7 @@ dependencies = [
 
 [[package]]
 name = "cargo-credential"
-version = "0.3.0"
+version = "0.3.1"
 dependencies = [
  "anyhow",
  "libc",

credential/cargo-credential/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "cargo-credential"
-version = "0.3.0"
+version = "0.3.1"
 edition.workspace = true
 license.workspace = true
 repository = "https://github.com/rust-lang/cargo"

credential/cargo-credential/src/lib.rs

@@ -197,9 +197,11 @@ pub enum CacheControl {
     Unknown,
 }
 
-/// Credential process JSON protocol version. Incrementing
+/// Credential process JSON protocol version. If the protocol needs to make
-/// this version will prevent new credential providers
+/// a breaking change, a new protocol version should be defined (`PROTOCOL_VERSION_2`).
-/// from working with older versions of Cargo.
+/// This library should offer support for both protocols if possible, by signaling
+/// in the `CredentialHello` message. Cargo will then choose which protocol to use,
+/// or it will error if there are no common protocol versions available.
 pub const PROTOCOL_VERSION_1: u32 = 1;
 pub trait Credential {
     /// Retrieves a token for the given registry.

src/cargo/util/credential/process.rs

@@ -4,12 +4,12 @@
 use std::{
     io::{BufRead, BufReader, Write},
     path::PathBuf,
-    process::{Command, Stdio},
+    process::{Child, Command, Stdio},
 };
 
 use anyhow::Context;
 use cargo_credential::{
-    Action, Credential, CredentialHello, CredentialRequest, CredentialResponse, RegistryInfo,
+    Action, Credential, CredentialHello, CredentialRequest, CredentialResponse, Error, RegistryInfo,
 };
 
 pub struct CredentialProcessCredential {
@@ -22,31 +22,38 @@ impl<'a> CredentialProcessCredential {
             path: PathBuf::from(path),
         }
     }
-}
 
-impl<'a> Credential for CredentialProcessCredential {
+    fn run(
-    fn perform(
         &self,
-        registry: &RegistryInfo<'_>,
+        child: &mut Child,
         action: &Action<'_>,
+        registry: &RegistryInfo<'_>,
         args: &[&str],
-    ) -> Result<CredentialResponse, cargo_credential::Error> {
+    ) -> Result<Result<CredentialResponse, Error>, Error> {
-        let mut cmd = Command::new(&self.path);
-        cmd.stdout(Stdio::piped());
-        cmd.stdin(Stdio::piped());
-        cmd.arg("--cargo-plugin");
-        tracing::debug!("credential-process: {cmd:?}");
-        let mut child = cmd.spawn().context("failed to spawn credential process")?;
         let mut output_from_child = BufReader::new(child.stdout.take().unwrap());
         let mut input_to_child = child.stdin.take().unwrap();
         let mut buffer = String::new();
+
+        // Read the CredentialHello
         output_from_child
             .read_line(&mut buffer)
             .context("failed to read hello from credential provider")?;
         let credential_hello: CredentialHello =
             serde_json::from_str(&buffer).context("failed to deserialize hello")?;
         tracing::debug!("credential-process > {credential_hello:?}");
+        if !credential_hello
+            .v
+            .contains(&cargo_credential::PROTOCOL_VERSION_1)
+        {
+            return Err(format!(
+                "credential provider supports protocol versions {:?}, while Cargo supports {:?}",
+                credential_hello.v,
+                [cargo_credential::PROTOCOL_VERSION_1]
+            )
+            .into());
+        }
 
+        // Send the Credential Request
         let req = CredentialRequest {
             v: cargo_credential::PROTOCOL_VERSION_1,
             action: action.clone(),
@@ -56,14 +63,17 @@ impl<'a> Credential for CredentialProcessCredential {
         let request = serde_json::to_string(&req).context("failed to serialize request")?;
         tracing::debug!("credential-process < {req:?}");
         writeln!(input_to_child, "{request}").context("failed to write to credential provider")?;
-
         buffer.clear();
         output_from_child
             .read_line(&mut buffer)
             .context("failed to read response from credential provider")?;
-        let response: Result<CredentialResponse, cargo_credential::Error> =
+
+        // Read the Credential Response
+        let response: Result<CredentialResponse, Error> =
             serde_json::from_str(&buffer).context("failed to deserialize response")?;
         tracing::debug!("credential-process > {response:?}");
+
+        // Tell the credential process we're done by closing stdin. It should exit cleanly.
         drop(input_to_child);
         let status = child.wait().context("credential process never started")?;
         if !status.success() {
@@ -75,6 +85,31 @@ impl<'a> Credential for CredentialProcessCredential {
             .into());
         }
         tracing::trace!("credential process exited successfully");
-        response
+        Ok(response)
+    }
+}
+
+impl<'a> Credential for CredentialProcessCredential {
+    fn perform(
+        &self,
+        registry: &RegistryInfo<'_>,
+        action: &Action<'_>,
+        args: &[&str],
+    ) -> Result<CredentialResponse, Error> {
+        let mut cmd = Command::new(&self.path);
+        cmd.stdout(Stdio::piped());
+        cmd.stdin(Stdio::piped());
+        cmd.arg("--cargo-plugin");
+        tracing::debug!("credential-process: {cmd:?}");
+        let mut child = cmd.spawn().context("failed to spawn credential process")?;
+        match self.run(&mut child, action, registry, args) {
+            Err(e) => {
+                // Since running the credential process itself failed, ensure the
+                // process is stopped.
+                let _ = child.kill();
+                Err(e)
+            }
+            Ok(response) => response,
+        }
     }
 }

tests/testsuite/credential_process.rs

@@ -656,3 +656,44 @@ CARGO_REGISTRY_INDEX_URL=Some([..])
         )
         .run();
 }
+
+#[cargo_test]
+fn unsupported_version() {
+    let cred_proj = project()
+        .at("new-vers")
+        .file("Cargo.toml", &basic_manifest("new-vers", "1.0.0"))
+        .file(
+            "src/main.rs",
+            &r####"
+                fn main() {
+                    println!(r#"{{"v":[998, 999]}}"#);
+                    assert_eq!(std::env::args().skip(1).next().unwrap(), "--cargo-plugin");
+                    let mut buffer = String::new();
+                    std::io::stdin().read_line(&mut buffer).unwrap();
+                    std::thread::sleep(std::time::Duration::from_secs(1));
+                    panic!("child process should have been killed before getting here");
+                } "####,
+        )
+        .build();
+    cred_proj.cargo("build").run();
+    let provider = toml_bin(&cred_proj, "new-vers");
+
+    let registry = registry::RegistryBuilder::new()
+        .no_configure_token()
+        .credential_provider(&[&provider])
+        .build();
+
+    cargo_process("login -Z credential-process abcdefg")
+        .masquerade_as_nightly_cargo(&["credential-process"])
+        .replace_crates_io(registry.index_url())
+        .with_status(101)
+        .with_stderr(
+            r#"[UPDATING] [..]
+[ERROR] credential provider `[..]` failed action `login`
+
+Caused by:
+  credential provider supports protocol versions [998, 999], while Cargo supports [1]
+"#,
+        )
+        .run();
+}