From 39db61e26e6411246445d502df608031d7f8d45a Mon Sep 17 00:00:00 2001 From: Arlo Siemsen Date: Tue, 29 Aug 2023 14:52:05 -0500 Subject: [PATCH] fix: add error for unsupported credential provider version --- Cargo.lock | 2 +- credential/cargo-credential/Cargo.toml | 2 +- credential/cargo-credential/src/lib.rs | 8 +-- src/cargo/util/credential/process.rs | 67 ++++++++++++++++++++------ tests/testsuite/credential_process.rs | 41 ++++++++++++++++ 5 files changed, 99 insertions(+), 21 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 58dbaeb45d8..6003a68e978 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -334,7 +334,7 @@ dependencies = [ [[package]] name = "cargo-credential" -version = "0.3.0" +version = "0.3.1" dependencies = [ "anyhow", "libc", diff --git a/credential/cargo-credential/Cargo.toml b/credential/cargo-credential/Cargo.toml index 8cd1348be5a..66708ee86b7 100644 --- a/credential/cargo-credential/Cargo.toml +++ b/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" diff --git a/credential/cargo-credential/src/lib.rs b/credential/cargo-credential/src/lib.rs index 0fb495ed388..838d712d0ee 100644 --- a/credential/cargo-credential/src/lib.rs +++ b/credential/cargo-credential/src/lib.rs @@ -197,9 +197,11 @@ pub enum CacheControl { Unknown, } -/// Credential process JSON protocol version. Incrementing -/// this version will prevent new credential providers -/// from working with older versions of Cargo. +/// Credential process JSON protocol version. If the protocol needs to make +/// a breaking change, a new protocol version should be defined (`PROTOCOL_VERSION_2`). +/// 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. diff --git a/src/cargo/util/credential/process.rs b/src/cargo/util/credential/process.rs index 89eac1af6c4..6a6a0a69e01 100644 --- a/src/cargo/util/credential/process.rs +++ b/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 perform( + fn run( &self, - registry: &RegistryInfo<'_>, + child: &mut Child, action: &Action<'_>, + registry: &RegistryInfo<'_>, args: &[&str], - ) -> Result { - 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")?; + ) -> Result, Error> { 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 = + + // Read the Credential Response + let response: Result = 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 { + 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, + } } } diff --git a/tests/testsuite/credential_process.rs b/tests/testsuite/credential_process.rs index a5439039600..2f5d4d9dcd9 100644 --- a/tests/testsuite/credential_process.rs +++ b/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(); +}