From 5939ed0d4f8b14cb5956f37b6231d9adc2bbcf0a Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Mon, 12 Dec 2022 17:49:22 +0000 Subject: [PATCH 01/22] clap for new login args --- src/bin/cargo/commands/login.rs | 13 +++++++++++++ src/cargo/ops/registry.rs | 13 ++++++++++++- tests/testsuite/login.rs | 30 ++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 1 deletion(-) diff --git a/src/bin/cargo/commands/login.rs b/src/bin/cargo/commands/login.rs index 05595abca..3efa75fb7 100644 --- a/src/bin/cargo/commands/login.rs +++ b/src/bin/cargo/commands/login.rs @@ -11,6 +11,16 @@ pub fn cli() -> Command { .arg_quiet() .arg(Arg::new("token").action(ArgAction::Set)) .arg(opt("registry", "Registry to use").value_name("REGISTRY")) + .arg(flag("generate-keypair", "Generate a public/secret keypair").conflicts_with("token")) + .arg( + flag("secret-key", "Prompt for secret key") + .conflicts_with_all(&["generate-keypair", "token"]), + ) + .arg( + opt("key-subject", "Set the key subject for this registry") + .value_name("SUBJECT") + .conflicts_with("token"), + ) .after_help("Run `cargo help login` for more detailed information.\n") } @@ -19,6 +29,9 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult { config, args.get_one("token").map(String::as_str), args.get_one("registry").map(String::as_str), + args.flag("generate-keypair"), + args.flag("secret-key"), + args.get_one("key-subject").map(String::as_str), )?; Ok(()) } diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 3346e5b41..7dc0728b4 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -738,9 +738,17 @@ fn http_proxy_exists(config: &Config) -> CargoResult { } } -pub fn registry_login(config: &Config, token: Option<&str>, reg: Option<&str>) -> CargoResult<()> { +pub fn registry_login( + config: &Config, + token: Option<&str>, + reg: Option<&str>, + generate_keypair: bool, + secret_key: bool, + key_subject: Option<&str>, +) -> CargoResult<()> { let source_ids = get_source_id(config, None, reg)?; let reg_cfg = auth::registry_credential_config(config, &source_ids.original)?; + let login_url = match registry(config, token, None, reg, false, false) { Ok((registry, _)) => Some(format!("{}/me", registry.host())), Err(e) if e.is::() => e @@ -751,6 +759,9 @@ pub fn registry_login(config: &Config, token: Option<&str>, reg: Option<&str>) - Err(e) => return Err(e), }; + assert!(!generate_keypair); + assert!(!secret_key); + assert!(key_subject.is_none()); let token = match token { Some(token) => token.to_string(), None => { diff --git a/tests/testsuite/login.rs b/tests/testsuite/login.rs index b645e8bf6..a6bc93f95 100644 --- a/tests/testsuite/login.rs +++ b/tests/testsuite/login.rs @@ -123,3 +123,33 @@ fn empty_login_token() { .with_status(101) .run(); } + +#[cargo_test] +fn bad_asymmetric_token_args() { + // These cases are kept brief as the implementation is covered by clap, so this is only smoke testing that we have clap configured correctly. + cargo_process("login --key-subject=foo tok") + .with_stderr_contains( + "[ERROR] The argument '--key-subject ' cannot be used with '[token]'", + ) + .with_status(1) + .run(); + + cargo_process("login --generate-keypair tok") + .with_stderr_contains( + "[ERROR] The argument '--generate-keypair' cannot be used with '[token]'", + ) + .with_status(1) + .run(); + + cargo_process("login --secret-key tok") + .with_stderr_contains("[ERROR] The argument '--secret-key' cannot be used with '[token]'") + .with_status(1) + .run(); + + cargo_process("login --generate-keypair --secret-key") + .with_stderr_contains( + "[ERROR] The argument '--generate-keypair' cannot be used with '--secret-key'", + ) + .with_status(1) + .run(); +} From d8df1425ea539ce082665627f242f5bafe3a6bff Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Mon, 12 Dec 2022 17:49:22 +0000 Subject: [PATCH 02/22] unstable_cli_options --- src/cargo/core/features.rs | 2 +- src/cargo/ops/registry.rs | 15 ++++ src/cargo/util/auth.rs | 134 ++++++++++++++++++++++------------ src/cargo/util/config/mod.rs | 20 +++++ tests/testsuite/config_cli.rs | 14 ++++ 5 files changed, 138 insertions(+), 47 deletions(-) diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index f295f063b..9132f0e08 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -682,7 +682,7 @@ unstable_cli_options!( panic_abort_tests: bool = ("Enable support to run tests with -Cpanic=abort"), host_config: bool = ("Enable the [host] section in the .cargo/config.toml file"), sparse_registry: bool = ("Support plain-HTTP-based crate registries"), - registry_auth: bool = ("Authentication for alternative registries"), + registry_auth: bool = ("Authentication for alternative registries, and generate registry authentication tokens using asymmetric cryptography"), target_applies_to_host: bool = ("Enable the `target-applies-to-host` key in the .cargo/config.toml file"), rustdoc_map: bool = ("Allow passing external documentation mappings to rustdoc"), separate_nightlies: bool = (HIDDEN), diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 7dc0728b4..44bee42e6 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -44,6 +44,8 @@ pub enum RegistryCredentialConfig { Token(String), /// Process used for fetching a token. Process((PathBuf, Vec)), + /// Secret Key and subject for Asymmetric tokens. + AsymmetricKey((String, Option)), } impl RegistryCredentialConfig { @@ -59,6 +61,12 @@ impl RegistryCredentialConfig { pub fn is_token(&self) -> bool { matches!(self, Self::Token(..)) } + /// Returns `true` if the credential is [`Key`]. + /// + /// [`Key`]: Credential::Key + pub fn is_asymmetric_key(&self) -> bool { + matches!(self, Self::AsymmetricKey(..)) + } pub fn as_token(&self) -> Option<&str> { if let Self::Token(v) = self { Some(&*v) @@ -73,6 +81,13 @@ impl RegistryCredentialConfig { None } } + pub fn as_asymmetric_key(&self) -> Option<&(String, Option)> { + if let Self::AsymmetricKey(v) = self { + Some(v) + } else { + None + } + } } pub struct PublishOpts<'cfg> { diff --git a/src/cargo/util/auth.rs b/src/cargo/util/auth.rs index d67f874f1..39165863d 100644 --- a/src/cargo/util/auth.rs +++ b/src/cargo/util/auth.rs @@ -26,6 +26,8 @@ pub fn registry_credential_config( index: Option, token: Option, credential_process: Option, + secret_key: Option, + secret_key_subject: Option, #[serde(rename = "default")] _default: Option, } @@ -46,26 +48,44 @@ pub fn registry_credential_config( let RegistryConfig { token, credential_process, + secret_key, + secret_key_subject, .. } = config.get::("registry")?; let credential_process = credential_process.filter(|_| config.cli_unstable().credential_process); + let secret_key = secret_key.filter(|_| config.cli_unstable().registry_auth); + let secret_key_subject = secret_key_subject.filter(|_| config.cli_unstable().registry_auth); - return Ok(match (token, credential_process) { - (Some(_), Some(_)) => { - return Err(format_err!( - "both `token` and `credential-process` \ - were specified in the config`.\n\ - Only one of these values may be set, remove one or the other to proceed.", - )) - } - (Some(token), _) => RegistryCredentialConfig::Token(token), - (_, Some(process)) => RegistryCredentialConfig::Process(( - process.path.resolve_program(config), - process.args, - )), - (None, None) => RegistryCredentialConfig::None, - }); + let err_both = |token_key: &str, proc_key: &str| { + Err(format_err!( + "both `{token_key}` and `{proc_key}` \ + were specified in the config`.\n\ + Only one of these values may be set, remove one or the other to proceed.", + )) + }; + return Ok( + match (token, credential_process, secret_key, secret_key_subject) { + (Some(_), Some(_), _, _) => return err_both("token", "credential-process"), + (Some(_), _, Some(_), _) => return err_both("token", "secret-key"), + (_, Some(_), Some(_), _) => return err_both("credential-process", "secret-key"), + (_, _, None, Some(_)) => { + return Err(format_err!( + "`secret-key-subject` was set but `secret-key` was not in the config.\n\ + Ether set the `secret-key` or remove the `secret-key-subject`." + )); + } + (Some(token), _, _, _) => RegistryCredentialConfig::Token(token), + (_, Some(process), _, _) => RegistryCredentialConfig::Process(( + process.path.resolve_program(config), + process.args, + )), + (None, None, Some(key), subject) => { + RegistryCredentialConfig::AsymmetricKey((key, subject)) + } + (None, None, None, _) => RegistryCredentialConfig::None, + }, + ); } // Find the SourceId's name by its index URL. If environment variables @@ -133,52 +153,71 @@ pub fn registry_credential_config( } } - let (token, credential_process) = if let Some(name) = &name { + let (token, credential_process, secret_key, secret_key_subject) = if let Some(name) = &name { log::debug!("found alternative registry name `{name}` for {sid}"); let RegistryConfig { token, + secret_key, + secret_key_subject, credential_process, .. } = config.get::(&format!("registries.{name}"))?; let credential_process = credential_process.filter(|_| config.cli_unstable().credential_process); - (token, credential_process) + let secret_key = secret_key.filter(|_| config.cli_unstable().registry_auth); + let secret_key_subject = secret_key_subject.filter(|_| config.cli_unstable().registry_auth); + (token, credential_process, secret_key, secret_key_subject) } else { log::debug!("no registry name found for {sid}"); - (None, None) + (None, None, None, None) }; let name = name.as_deref(); - Ok(match (token, credential_process) { - (Some(_), Some(_)) => { - return { - Err(format_err!( - "both `token` and `credential-process` \ - were specified in the config for registry `{name}`.\n\ - Only one of these values may be set, remove one or the other to proceed.", - name = name.unwrap() - )) + let err_both = |token_key: &str, proc_key: &str| { + Err(format_err!( + "both `{token_key}` and `{proc_key}` \ + were specified in the config for registry `{name}`.\n\ + Only one of these values may be set, remove one or the other to proceed.", + name = name.unwrap() + )) + }; + Ok( + match (token, credential_process, secret_key, secret_key_subject) { + (Some(_), Some(_), _, _) => return err_both("token", "credential-process"), + (Some(_), _, Some(_), _) => return err_both("token", "secret-key"), + (_, Some(_), Some(_), _) => return err_both("credential-process", "secret-key"), + (_, _, None, Some(_)) => { + return Err(format_err!( + "`secret-key-subject` was set but `secret-key` was not in the config \ + for registry `{}`.\n\ + Ether set the `secret-key` or remove the `secret-key-subject`.", + name.unwrap() + )); } - } - (Some(token), _) => RegistryCredentialConfig::Token(token), - (_, Some(process)) => { - RegistryCredentialConfig::Process((process.path.resolve_program(config), process.args)) - } - (None, None) => { - // If we couldn't find a registry-specific credential, try the global credential process. - if let Some(process) = config - .get::>("registry.credential-process")? - .filter(|_| config.cli_unstable().credential_process) - { - RegistryCredentialConfig::Process(( - process.path.resolve_program(config), - process.args, - )) - } else { - RegistryCredentialConfig::None + (Some(token), _, _, _) => RegistryCredentialConfig::Token(token), + (_, Some(process), _, _) => RegistryCredentialConfig::Process(( + process.path.resolve_program(config), + process.args, + )), + (None, None, Some(key), subject) => { + RegistryCredentialConfig::AsymmetricKey((key, subject)) } - } - }) + (None, None, None, _) => { + // If we couldn't find a registry-specific credential, try the global credential process. + if let Some(process) = config + .get::>("registry.credential-process")? + .filter(|_| config.cli_unstable().credential_process) + { + RegistryCredentialConfig::Process(( + process.path.resolve_program(config), + process.args, + )) + } else { + RegistryCredentialConfig::None + } + } + }, + ) } #[derive(Debug, PartialEq)] @@ -288,6 +327,9 @@ fn auth_token_optional(config: &Config, sid: &SourceId) -> CargoResult { run_command(config, &process, sid, Action::Get)?.unwrap() } + RegistryCredentialConfig::AsymmetricKey((_secret_key, _secret_key_subject)) => { + todo!("sign the token") + } }; cache.insert(url.clone(), token.clone()); diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 5743f9baf..27f172255 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -1360,6 +1360,26 @@ impl Config { ); } + if toml_v + .get("registry") + .and_then(|v| v.as_table()) + .and_then(|t| t.get("secret-key")) + .is_some() + { + bail!( + "registry.secret-key cannot be set through --config for security reasons" + ); + } else if let Some((k, _)) = toml_v + .get("registries") + .and_then(|v| v.as_table()) + .and_then(|t| t.iter().find(|(_, v)| v.get("secret-key").is_some())) + { + bail!( + "registries.{}.secret-key cannot be set through --config for security reasons", + k + ); + } + CV::from_toml(Definition::Cli(None), toml_v) .with_context(|| format!("failed to convert --config argument `{arg}`"))? }; diff --git a/tests/testsuite/config_cli.rs b/tests/testsuite/config_cli.rs index 27b5cf9af..92e145d96 100644 --- a/tests/testsuite/config_cli.rs +++ b/tests/testsuite/config_cli.rs @@ -435,6 +435,20 @@ fn no_disallowed_values() { config.unwrap_err(), "registries.crates-io.token cannot be set through --config for security reasons", ); + let config = ConfigBuilder::new() + .config_arg("registry.secret-key=\"hello\"") + .build_err(); + assert_error( + config.unwrap_err(), + "registry.secret-key cannot be set through --config for security reasons", + ); + let config = ConfigBuilder::new() + .config_arg("registries.crates-io.secret-key=\"hello\"") + .build_err(); + assert_error( + config.unwrap_err(), + "registries.crates-io.secret-key cannot be set through --config for security reasons", + ); } #[cargo_test] From a917fa0d4e8582a417c1df8602899f2067a93a76 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Mon, 12 Dec 2022 17:49:22 +0000 Subject: [PATCH 03/22] login/out --- src/cargo/ops/registry.rs | 114 ++++++++++++++++++++++++----------- src/cargo/util/auth.rs | 13 +++- src/cargo/util/config/mod.rs | 60 +++++++++++++----- 3 files changed, 133 insertions(+), 54 deletions(-) diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 44bee42e6..25d06fc1d 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -8,7 +8,7 @@ use std::task::Poll; use std::time::Duration; use std::{cmp, env}; -use anyhow::{bail, format_err, Context as _}; +use anyhow::{anyhow, bail, format_err, Context as _}; use cargo_util::paths; use crates_io::{self, NewCrate, NewCrateDependency, Registry}; use curl::easy::{Easy, InfoType, SslOpt, SslVersion}; @@ -27,7 +27,9 @@ use crate::core::{Package, SourceId, Workspace}; use crate::ops; use crate::ops::Packages; use crate::sources::{RegistrySource, SourceConfigMap, CRATES_IO_DOMAIN, CRATES_IO_REGISTRY}; -use crate::util::auth::{self, AuthorizationError}; +use crate::util::auth::{ + check_format_like_paserk_secret, {self, AuthorizationError}, +}; use crate::util::config::{Config, SslVersionConfig, SslVersionConfigRange}; use crate::util::errors::CargoResult; use crate::util::important_paths::find_root_manifest_for_wd; @@ -37,7 +39,7 @@ use crate::{drop_print, drop_println, version}; /// Registry settings loaded from config files. /// /// This is loaded based on the `--registry` flag and the config settings. -#[derive(Debug)] +#[derive(Debug, PartialEq)] pub enum RegistryCredentialConfig { None, /// The authentication token. @@ -758,7 +760,7 @@ pub fn registry_login( token: Option<&str>, reg: Option<&str>, generate_keypair: bool, - secret_key: bool, + secret_key_required: bool, key_subject: Option<&str>, ) -> CargoResult<()> { let source_ids = get_source_id(config, None, reg)?; @@ -773,51 +775,91 @@ pub fn registry_login( .map(|u| u.to_string()), Err(e) => return Err(e), }; - - assert!(!generate_keypair); - assert!(!secret_key); - assert!(key_subject.is_none()); - let token = match token { - Some(token) => token.to_string(), - None => { - if let Some(login_url) = login_url { - drop_println!( - config, - "please paste the token found on {} below", - login_url - ) - } else { - drop_println!( - config, - "please paste the token for {} below", - source_ids.original.display_registry_name() - ) + let new_token; + if generate_keypair || secret_key_required || key_subject.is_some() { + if !config.cli_unstable().registry_auth { + panic!("-registry_auth required."); + } + assert!(token.is_none()); + // we are dealing with asymmetric tokens + let (old_secret_key, old_key_subject) = match ®_cfg { + RegistryCredentialConfig::AsymmetricKey((old_secret_key, old_key_subject)) => { + (Some(old_secret_key), old_key_subject.clone()) } - + _ => (None, None), + }; + let secret_key: String; + if generate_keypair { + assert!(!secret_key_required); + secret_key = "key".to_owned(); + // todo!("PASETO: generate a keypair") + } else if secret_key_required { + assert!(!generate_keypair); + drop_println!(config, "please paste the API secret key below"); let mut line = String::new(); let input = io::stdin(); input .lock() .read_line(&mut line) .with_context(|| "failed to read stdin")?; - // Automatically remove `cargo login` from an inputted token to - // allow direct pastes from `registry.host()`/me. - line.replace("cargo login", "").trim().to_string() + secret_key = line.trim().to_string(); + } else { + secret_key = old_secret_key + .cloned() + .ok_or_else(|| anyhow!("need a secret_key to set a key_subject"))?; } - }; + if !check_format_like_paserk_secret(&secret_key) { + panic!("not a validly formated PASERK secret key"); + } + new_token = RegistryCredentialConfig::AsymmetricKey(( + secret_key, + match key_subject { + Some(key_subject) => Some(key_subject.to_string()), + None => old_key_subject, + }, + )); + } else { + new_token = RegistryCredentialConfig::Token(match token { + Some(token) => token.to_string(), + None => { + if let Some(login_url) = login_url { + drop_println!( + config, + "please paste the token found on {} below", + login_url + ) + } else { + drop_println!( + config, + "please paste the token for {} below", + source_ids.original.display_registry_name() + ) + } - if token.is_empty() { - bail!("please provide a non-empty token"); - } + let mut line = String::new(); + let input = io::stdin(); + input + .lock() + .read_line(&mut line) + .with_context(|| "failed to read stdin")?; + // Automatically remove `cargo login` from an inputted token to + // allow direct pastes from `registry.host()`/me. + line.replace("cargo login", "").trim().to_string() + } + }); - if let RegistryCredentialConfig::Token(old_token) = ®_cfg { - if old_token == &token { - config.shell().status("Login", "already logged in")?; - return Ok(()); + if let Some(tok) = new_token.as_token() { + if tok.is_empty() { + bail!("please provide a non-empty token"); + } } } + if ®_cfg == &new_token { + config.shell().status("Login", "already logged in")?; + return Ok(()); + } - auth::login(config, &source_ids.original, token)?; + auth::login(config, &source_ids.original, new_token)?; config.shell().status( "Login", diff --git a/src/cargo/util/auth.rs b/src/cargo/util/auth.rs index 39165863d..a3f96d4b0 100644 --- a/src/cargo/util/auth.rs +++ b/src/cargo/util/auth.rs @@ -328,7 +328,7 @@ fn auth_token_optional(config: &Config, sid: &SourceId) -> CargoResult { - todo!("sign the token") + todo!("PASETO: sign a read token") } }; @@ -343,9 +343,13 @@ enum Action { } /// Saves the given token. -pub fn login(config: &Config, sid: &SourceId, token: String) -> CargoResult<()> { +pub fn login(config: &Config, sid: &SourceId, token: RegistryCredentialConfig) -> CargoResult<()> { match registry_credential_config(config, sid)? { RegistryCredentialConfig::Process(process) => { + let token = token + .as_token() + .expect("credential_process can not use login with a secret_key") + .to_owned(); run_command(config, &process, sid, Action::Store(token))?; } _ => { @@ -355,6 +359,11 @@ pub fn login(config: &Config, sid: &SourceId, token: String) -> CargoResult<()> Ok(()) } +pub(crate) fn check_format_like_paserk_secret(_s: &str) -> bool { + // TODO: PASETO: check for valid PASERK secret format + true +} + /// Removes the token for the given registry. pub fn logout(config: &Config, sid: &SourceId) -> CargoResult<()> { match registry_credential_config(config, sid)? { diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 27f172255..472d2f79e 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -69,7 +69,7 @@ use self::ConfigValue as CV; use crate::core::compiler::rustdoc::RustdocExternMap; use crate::core::shell::Verbosity; use crate::core::{features, CliUnstable, Shell, SourceId, Workspace, WorkspaceRootConfig}; -use crate::ops; +use crate::ops::{self, RegistryCredentialConfig}; use crate::util::errors::CargoResult; use crate::util::validate_package_name; use crate::util::CanonicalUrl; @@ -2101,7 +2101,7 @@ pub fn homedir(cwd: &Path) -> Option { pub fn save_credentials( cfg: &Config, - token: Option, + token: Option, registry: &SourceId, ) -> CargoResult<()> { let registry = if registry.is_crates_io() { @@ -2151,26 +2151,50 @@ pub fn save_credentials( if let Some(token) = token { // login - let (key, mut value) = { - let key = "token".to_string(); - let value = ConfigValue::String(token, Definition::Path(file.path().to_path_buf())); - let map = HashMap::from([(key, value)]); - let table = CV::Table(map, Definition::Path(file.path().to_path_buf())); - if let Some(registry) = registry { - let map = HashMap::from([(registry.to_string(), table)]); - ( - "registries".into(), - CV::Table(map, Definition::Path(file.path().to_path_buf())), - ) - } else { - ("registry".into(), table) + let path_def = Definition::Path(file.path().to_path_buf()); + let (key, mut value) = match token { + RegistryCredentialConfig::Token(token) => { + // login with token + + let key = "token".to_string(); + let value = ConfigValue::String(token, path_def.clone()); + let map = HashMap::from([(key, value)]); + let table = CV::Table(map, path_def.clone()); + + if let Some(registry) = registry { + let map = HashMap::from([(registry.to_string(), table)]); + ("registries".into(), CV::Table(map, path_def.clone())) + } else { + ("registry".into(), table) + } } + RegistryCredentialConfig::AsymmetricKey((secret_key, key_subject)) => { + // login with key + + let key = "secret-key".to_string(); + let value = ConfigValue::String(secret_key, path_def.clone()); + let mut map = HashMap::from([(key, value)]); + if let Some(key_subject) = key_subject { + let key = "secret-key-subject".to_string(); + let value = ConfigValue::String(key_subject, path_def.clone()); + map.insert(key, value); + } + let table = CV::Table(map, path_def.clone()); + + if let Some(registry) = registry { + let map = HashMap::from([(registry.to_string(), table)]); + ("registries".into(), CV::Table(map, path_def.clone())) + } else { + ("registry".into(), table) + } + } + _ => unreachable!(), }; if registry.is_some() { if let Some(table) = toml.as_table_mut().unwrap().remove("registries") { - let v = CV::from_toml(Definition::Path(file.path().to_path_buf()), table)?; + let v = CV::from_toml(path_def, table)?; value.merge(v, false)?; } } @@ -2185,6 +2209,8 @@ pub fn save_credentials( format_err!("expected `[registries.{}]` to be a table", registry) })?; rtable.remove("token"); + rtable.remove("secret-key"); + rtable.remove("secret-key-subject"); } } } else if let Some(registry) = table.get_mut("registry") { @@ -2192,6 +2218,8 @@ pub fn save_credentials( .as_table_mut() .ok_or_else(|| format_err!("expected `[registry]` to be a table"))?; reg_table.remove("token"); + reg_table.remove("secret-key"); + reg_table.remove("secret-key-subject"); } } From b907a7f7ea74d5465623477a75d3bc0ccf5d900b Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Mon, 12 Dec 2022 17:49:22 +0000 Subject: [PATCH 04/22] Add test for wrighting keys --- tests/testsuite/registry.rs | 45 +++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs index 6932f1a8d..1de19aa7a 100644 --- a/tests/testsuite/registry.rs +++ b/tests/testsuite/registry.rs @@ -1127,6 +1127,51 @@ fn login_with_token_on_stdin() { assert_eq!(credentials, "[registry]\ntoken = \"some token\"\n"); } +#[cargo_test] +fn login_with_asymmetric_token_and_subject_on_stdin() { + let registry = registry::init(); + let credentials = paths::home().join(".cargo/credentials"); + fs::remove_file(&credentials).unwrap(); + cargo_process("login --key-subject=foo --secret-key -v -Z registry-auth") + .masquerade_as_nightly_cargo(&["registry-auth"]) + .replace_crates_io(registry.index_url()) + .with_stdout("please paste the API secret key below") + .with_stdin("some token") + .run(); + let credentials = fs::read_to_string(&credentials).unwrap(); + assert!(credentials.starts_with("[registry]\n")); + assert!(credentials.contains("secret-key-subject = \"foo\"\n")); + assert!(credentials.contains("secret-key = \"some token\"\n")); +} + +#[cargo_test] +fn login_with_asymmetric_token_on_stdin() { + let registry = registry::init(); + let credentials = paths::home().join(".cargo/credentials"); + fs::remove_file(&credentials).unwrap(); + cargo_process("login --secret-key -v -Z registry-auth") + .masquerade_as_nightly_cargo(&["registry-auth"]) + .replace_crates_io(registry.index_url()) + .with_stdout("please paste the API secret key below") + .with_stdin("some token") + .run(); + let credentials = fs::read_to_string(&credentials).unwrap(); + assert_eq!(credentials, "[registry]\nsecret-key = \"some token\"\n"); +} + +#[cargo_test] +fn login_with_asymmetric_key_subject_without_key() { + let registry = registry::init(); + let credentials = paths::home().join(".cargo/credentials"); + fs::remove_file(&credentials).unwrap(); + cargo_process("login --key-subject=foo -Z registry-auth") + .masquerade_as_nightly_cargo(&["registry-auth"]) + .replace_crates_io(registry.index_url()) + .with_stderr_contains("error: need a secret_key to set a key_subject") + .with_status(101) + .run(); +} + #[cargo_test] fn bad_license_file_http() { let registry = setup_http(); From 40325c4d4a1e5d581425be064b1eeeacff80a397 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Mon, 12 Dec 2022 17:49:22 +0000 Subject: [PATCH 05/22] generate and check secret_key --- Cargo.toml | 1 + src/cargo/ops/registry.rs | 10 +++++++--- src/cargo/util/auth.rs | 7 ++++--- tests/testsuite/registry.rs | 40 +++++++++++++++++++++++++++++++++---- 4 files changed, 48 insertions(+), 10 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index d3352269b..99225e4ce 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -46,6 +46,7 @@ libgit2-sys = "0.14.0" memchr = "2.1.3" opener = "0.5" os_info = "3.5.0" +pasetors = { version = "0.6.4", features = ["v3", "paserk", "std", "serde"] } pathdiff = "0.2" percent-encoding = "2.0" rustfix = "0.6.0" diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 25d06fc1d..c0e5943c4 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -13,6 +13,8 @@ use cargo_util::paths; use crates_io::{self, NewCrate, NewCrateDependency, Registry}; use curl::easy::{Easy, InfoType, SslOpt, SslVersion}; use log::{log, Level}; +use pasetors::keys::{AsymmetricKeyPair, Generate}; +use pasetors::paserk::FormatAsPaserk; use percent_encoding::{percent_encode, NON_ALPHANUMERIC}; use termcolor::Color::Green; use termcolor::ColorSpec; @@ -791,8 +793,10 @@ pub fn registry_login( let secret_key: String; if generate_keypair { assert!(!secret_key_required); - secret_key = "key".to_owned(); - // todo!("PASETO: generate a keypair") + let kp = AsymmetricKeyPair::::generate().unwrap(); + let mut key = String::new(); + FormatAsPaserk::fmt(&kp.secret, &mut key).unwrap(); + secret_key = key; } else if secret_key_required { assert!(!generate_keypair); drop_println!(config, "please paste the API secret key below"); @@ -809,7 +813,7 @@ pub fn registry_login( .ok_or_else(|| anyhow!("need a secret_key to set a key_subject"))?; } if !check_format_like_paserk_secret(&secret_key) { - panic!("not a validly formated PASERK secret key"); + bail!("not a validly formated PASERK secret key"); } new_token = RegistryCredentialConfig::AsymmetricKey(( secret_key, diff --git a/src/cargo/util/auth.rs b/src/cargo/util/auth.rs index a3f96d4b0..8e84446c1 100644 --- a/src/cargo/util/auth.rs +++ b/src/cargo/util/auth.rs @@ -4,6 +4,7 @@ use crate::util::{config, config::ConfigKey, CanonicalUrl, CargoResult, Config, use anyhow::{bail, format_err, Context as _}; use cargo_util::ProcessError; use core::fmt; +use pasetors::keys::AsymmetricSecretKey; use serde::Deserialize; use std::collections::HashMap; use std::error::Error; @@ -359,9 +360,9 @@ pub fn login(config: &Config, sid: &SourceId, token: RegistryCredentialConfig) - Ok(()) } -pub(crate) fn check_format_like_paserk_secret(_s: &str) -> bool { - // TODO: PASETO: check for valid PASERK secret format - true +pub(crate) fn check_format_like_paserk_secret(secret_key: &str) -> bool { + let key: Result, _> = secret_key.try_into(); + key.is_ok() } /// Removes the token for the given registry. diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs index 1de19aa7a..7b3251501 100644 --- a/tests/testsuite/registry.rs +++ b/tests/testsuite/registry.rs @@ -1136,12 +1136,12 @@ fn login_with_asymmetric_token_and_subject_on_stdin() { .masquerade_as_nightly_cargo(&["registry-auth"]) .replace_crates_io(registry.index_url()) .with_stdout("please paste the API secret key below") - .with_stdin("some token") + .with_stdin("k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36") .run(); let credentials = fs::read_to_string(&credentials).unwrap(); assert!(credentials.starts_with("[registry]\n")); assert!(credentials.contains("secret-key-subject = \"foo\"\n")); - assert!(credentials.contains("secret-key = \"some token\"\n")); + assert!(credentials.contains("secret-key = \"k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36\"\n")); } #[cargo_test] @@ -1153,10 +1153,10 @@ fn login_with_asymmetric_token_on_stdin() { .masquerade_as_nightly_cargo(&["registry-auth"]) .replace_crates_io(registry.index_url()) .with_stdout("please paste the API secret key below") - .with_stdin("some token") + .with_stdin("k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36") .run(); let credentials = fs::read_to_string(&credentials).unwrap(); - assert_eq!(credentials, "[registry]\nsecret-key = \"some token\"\n"); + assert_eq!(credentials, "[registry]\nsecret-key = \"k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36\"\n"); } #[cargo_test] @@ -1170,6 +1170,38 @@ fn login_with_asymmetric_key_subject_without_key() { .with_stderr_contains("error: need a secret_key to set a key_subject") .with_status(101) .run(); + + // ok so ad a secret_key to the credentials + cargo_process("login --secret-key -v -Z registry-auth") + .masquerade_as_nightly_cargo(&["registry-auth"]) + .replace_crates_io(registry.index_url()) + .with_stdout("please paste the API secret key below") + .with_stdin("k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36") + .run(); + + // and then it shuld work + cargo_process("login --key-subject=foo -Z registry-auth") + .masquerade_as_nightly_cargo(&["registry-auth"]) + .replace_crates_io(registry.index_url()) + .run(); + + let credentials = fs::read_to_string(&credentials).unwrap(); + assert!(credentials.starts_with("[registry]\n")); + assert!(credentials.contains("secret-key-subject = \"foo\"\n")); + assert!(credentials.contains("secret-key = \"k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36\"\n")); +} + +#[cargo_test] +fn login_with_generate_asymmetric_token() { + let registry = registry::init(); + let credentials = paths::home().join(".cargo/credentials"); + fs::remove_file(&credentials).unwrap(); + cargo_process("login --generate-keypair -Z registry-auth") + .masquerade_as_nightly_cargo(&["registry-auth"]) + .replace_crates_io(registry.index_url()) + .run(); + let credentials = fs::read_to_string(&credentials).unwrap(); + assert!(credentials.contains("secret-key = \"k3.secret.")); } #[cargo_test] From ccd69bcf2c80ed5c18165629ee1b683426d59c88 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Mon, 12 Dec 2022 17:49:22 +0000 Subject: [PATCH 06/22] Send Asymmetric Tokens to read endpoints --- Cargo.toml | 1 + src/cargo/util/auth.rs | 64 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 99225e4ce..4472f5f3e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -60,6 +60,7 @@ strip-ansi-escapes = "0.1.0" tar = { version = "0.4.38", default-features = false } tempfile = "3.0" termcolor = "1.1" +time = { version = "0.3", features = ["parsing", "formatting"]} toml_edit = { version = "0.15.0", features = ["serde", "easy", "perf"] } unicode-xid = "0.2.0" url = "2.2.2" diff --git a/src/cargo/util/auth.rs b/src/cargo/util/auth.rs index 8e84446c1..4a49d1c85 100644 --- a/src/cargo/util/auth.rs +++ b/src/cargo/util/auth.rs @@ -4,13 +4,15 @@ use crate::util::{config, config::ConfigKey, CanonicalUrl, CargoResult, Config, use anyhow::{bail, format_err, Context as _}; use cargo_util::ProcessError; use core::fmt; -use pasetors::keys::AsymmetricSecretKey; +use pasetors::keys::{AsymmetricPublicKey, AsymmetricSecretKey}; use serde::Deserialize; use std::collections::HashMap; use std::error::Error; use std::io::{Read, Write}; use std::path::PathBuf; use std::process::{Command, Stdio}; +use time::format_description::well_known::Rfc3339; +use time::OffsetDateTime; use url::Url; use crate::core::SourceId; @@ -328,8 +330,40 @@ fn auth_token_optional(config: &Config, sid: &SourceId) -> CargoResult { run_command(config, &process, sid, Action::Get)?.unwrap() } - RegistryCredentialConfig::AsymmetricKey((_secret_key, _secret_key_subject)) => { - todo!("PASETO: sign a read token") + RegistryCredentialConfig::AsymmetricKey((secret_key, secret_key_subject)) => { + let secret: AsymmetricSecretKey = + secret_key.as_str().try_into()?; + let public: AsymmetricPublicKey = (&secret).try_into()?; + let kip: pasetors::paserk::Id = (&public).try_into()?; + let iat = OffsetDateTime::now_utc(); + + let message = Message { + iat: &iat.format(&Rfc3339)?, + sub: secret_key_subject.as_deref(), + mutation: None, + name: None, + vers: None, + cksum: None, + challenge: None, // todo: PASETO with challenges + v: None, + }; + let footer = Footer { + url: &sid.url().to_string(), + kip, + }; + + pasetors::version3::PublicToken::sign( + &secret, + serde_json::to_string(&message) + .expect("cannot serialize") + .as_bytes(), + Some( + serde_json::to_string(&footer) + .expect("cannot serialize") + .as_bytes(), + ), + None, + )? } }; @@ -337,6 +371,30 @@ fn auth_token_optional(config: &Config, sid: &SourceId) -> CargoResult { + iat: &'a str, + #[serde(skip_serializing_if = "Option::is_none")] + sub: Option<&'a str>, + #[serde(skip_serializing_if = "Option::is_none")] + mutation: Option<&'a str>, + #[serde(skip_serializing_if = "Option::is_none")] + name: Option<&'a str>, + #[serde(skip_serializing_if = "Option::is_none")] + vers: Option<&'a str>, + #[serde(skip_serializing_if = "Option::is_none")] + cksum: Option<&'a str>, + #[serde(skip_serializing_if = "Option::is_none")] + challenge: Option<&'a str>, + #[serde(skip_serializing_if = "Option::is_none")] + v: Option, +} +#[derive(serde::Serialize)] +struct Footer<'a> { + url: &'a str, + kip: pasetors::paserk::Id, +} + enum Action { Get, Store(String), From 6d1c07bcc5ea6265f29ca9fdda8f57e7e5870202 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Mon, 12 Dec 2022 17:49:22 +0000 Subject: [PATCH 07/22] Send asymmetric tokens with mutations --- crates/crates-io/lib.rs | 4 + src/cargo/ops/registry.rs | 59 ++++++++++++--- src/cargo/sources/registry/download.rs | 2 +- src/cargo/sources/registry/http_remote.rs | 2 +- src/cargo/util/auth.rs | 89 ++++++++++++++++++++--- src/cargo/util/config/mod.rs | 4 +- 6 files changed, 136 insertions(+), 24 deletions(-) diff --git a/crates/crates-io/lib.rs b/crates/crates-io/lib.rs index 86217d1d2..0e48b1ca5 100644 --- a/crates/crates-io/lib.rs +++ b/crates/crates-io/lib.rs @@ -215,6 +215,10 @@ impl Registry { } } + pub fn set_token(&mut self, token: Option) { + self.token = token; + } + pub fn host(&self) -> &str { &self.host } diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index c0e5943c4..15ec988d8 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -167,6 +167,10 @@ pub fn publish(ws: &Workspace<'_>, opts: &PublishOpts<'_>) -> CargoResult<()> { ); } } + // This is only used to confirm that we can create a token before we build the package. + // This causes the credential provider to be called an extra time, but keeps the same order of errors. + let ver = pkg.version().to_string(); + let mutation = auth::Mutation::PrePublish; let (mut registry, reg_ids) = registry( opts.config, @@ -174,7 +178,7 @@ pub fn publish(ws: &Workspace<'_>, opts: &PublishOpts<'_>) -> CargoResult<()> { opts.index.as_deref(), publish_registry.as_deref(), true, - !opts.dry_run, + Some(mutation).filter(|_| !opts.dry_run), )?; verify_dependencies(pkg, ®istry, reg_ids.original)?; @@ -198,6 +202,24 @@ pub fn publish(ws: &Workspace<'_>, opts: &PublishOpts<'_>) -> CargoResult<()> { )? .unwrap(); + let hash = cargo_util::Sha256::new() + .update_file(tarball.file())? + .finish_hex(); + let mutation = auth::Mutation::Publish { + name: pkg.name().as_str(), + vers: &ver, + cksum: &hash, + }; + + if !opts.dry_run { + registry.set_token(Some(auth::auth_token( + &opts.config, + ®_ids.original, + None, + Some(mutation), + )?)); + } + opts.config .shell() .status("Uploading", pkg.package_id().to_string())?; @@ -494,11 +516,11 @@ fn registry( index: Option<&str>, registry: Option<&str>, force_update: bool, - token_required: bool, + token_required: Option>, ) -> CargoResult<(Registry, RegistrySourceIds)> { let source_ids = get_source_id(config, index, registry)?; - if token_required && index.is_some() && token_from_cmdline.is_none() { + if token_required.is_some() && index.is_some() && token_from_cmdline.is_none() { bail!("command-line argument --index requires --token to be specified"); } if let Some(token) = token_from_cmdline { @@ -525,8 +547,13 @@ fn registry( let api_host = cfg .api .ok_or_else(|| format_err!("{} does not support API commands", source_ids.replacement))?; - let token = if token_required || cfg.auth_required { - Some(auth::auth_token(config, &source_ids.original, None)?) + let token = if token_required.is_some() || cfg.auth_required { + Some(auth::auth_token( + config, + &source_ids.original, + None, + token_required, + )?) } else { None }; @@ -768,7 +795,7 @@ pub fn registry_login( let source_ids = get_source_id(config, None, reg)?; let reg_cfg = auth::registry_credential_config(config, &source_ids.original)?; - let login_url = match registry(config, token, None, reg, false, false) { + let login_url = match registry(config, token, None, reg, false, None) { Ok((registry, _)) => Some(format!("{}/me", registry.host())), Err(e) if e.is::() => e .downcast::() @@ -914,13 +941,15 @@ pub fn modify_owners(config: &Config, opts: &OwnersOptions) -> CargoResult<()> { } }; + let mutation = auth::Mutation::Owners { name: &name }; + let (mut registry, _) = registry( config, opts.token.as_deref(), opts.index.as_deref(), opts.registry.as_deref(), true, - true, + Some(mutation), )?; if let Some(ref v) = opts.to_add { @@ -993,13 +1022,25 @@ pub fn yank( None => bail!("a version must be specified to yank"), }; + let message = if undo { + auth::Mutation::Unyank { + name: &name, + vers: &version, + } + } else { + auth::Mutation::Yank { + name: &name, + vers: &version, + } + }; + let (mut registry, _) = registry( config, token.as_deref(), index.as_deref(), reg.as_deref(), true, - true, + Some(message), )?; let package_spec = format!("{}@{}", name, version); @@ -1087,7 +1128,7 @@ pub fn search( reg: Option, ) -> CargoResult<()> { let (mut registry, source_ids) = - registry(config, None, index.as_deref(), reg.as_deref(), false, false)?; + registry(config, None, index.as_deref(), reg.as_deref(), false, None)?; let (crates, total_crates) = registry.search(query, limit).with_context(|| { format!( "failed to retrieve search results from the registry at {}", diff --git a/src/cargo/sources/registry/download.rs b/src/cargo/sources/registry/download.rs index bde75b9da..723c55ffd 100644 --- a/src/cargo/sources/registry/download.rs +++ b/src/cargo/sources/registry/download.rs @@ -71,7 +71,7 @@ pub(super) fn download( } let authorization = if registry_config.auth_required { - Some(auth::auth_token(config, &pkg.source_id(), None)?) + Some(auth::auth_token(config, &pkg.source_id(), None, None)?) } else { None }; diff --git a/src/cargo/sources/registry/http_remote.rs b/src/cargo/sources/registry/http_remote.rs index a64d3a5e6..bb136e527 100644 --- a/src/cargo/sources/registry/http_remote.rs +++ b/src/cargo/sources/registry/http_remote.rs @@ -592,7 +592,7 @@ impl<'cfg> RegistryData for HttpRegistry<'cfg> { if self.auth_required { self.check_registry_auth_unstable()?; let authorization = - auth::auth_token(self.config, &self.source_id, self.login_url.as_ref())?; + auth::auth_token(self.config, &self.source_id, self.login_url.as_ref(), None)?; headers.append(&format!("Authorization: {}", authorization))?; trace!("including authorization for {}", full_url); } diff --git a/src/cargo/util/auth.rs b/src/cargo/util/auth.rs index 4a49d1c85..ec01d600a 100644 --- a/src/cargo/util/auth.rs +++ b/src/cargo/util/auth.rs @@ -296,14 +296,19 @@ pub fn cache_token(config: &Config, sid: &SourceId, token: &str) { let url = sid.canonical_url(); config .credential_cache() - .insert(url.clone(), token.to_string()); + .insert(url.clone(), (true, token.to_string())); } /// Returns the token to use for the given registry. /// If a `login_url` is provided and a token is not available, the /// login_url will be included in the returned error. -pub fn auth_token(config: &Config, sid: &SourceId, login_url: Option<&Url>) -> CargoResult { - match auth_token_optional(config, sid)? { +pub fn auth_token( + config: &Config, + sid: &SourceId, + login_url: Option<&Url>, + mutation: Option>, +) -> CargoResult { + match auth_token_optional(config, sid, mutation.as_ref())? { Some(token) => Ok(token), None => Err(AuthorizationError { sid: sid.clone(), @@ -315,12 +320,20 @@ pub fn auth_token(config: &Config, sid: &SourceId, login_url: Option<&Url>) -> C } /// Returns the token to use for the given registry. -fn auth_token_optional(config: &Config, sid: &SourceId) -> CargoResult> { +fn auth_token_optional( + config: &Config, + sid: &SourceId, + mutation: Option<&'_ Mutation<'_>>, +) -> CargoResult> { let mut cache = config.credential_cache(); let url = sid.canonical_url(); - if let Some(token) = cache.get(url) { - return Ok(Some(token.clone())); + if let Some((overridden_on_commandline, token)) = cache.get(url) { + // Tokens for endpoints that do not involve a mutation can always be reused. + // If the value is put in the cach by the command line, then we reuse it without looking at the configuration. + if *overridden_on_commandline || mutation.is_none() { + return Ok(Some(token.clone())); + } } let credential = registry_credential_config(config, sid)?; @@ -328,6 +341,7 @@ fn auth_token_optional(config: &Config, sid: &SourceId) -> CargoResult return Ok(None), RegistryCredentialConfig::Token(config_token) => config_token.to_string(), RegistryCredentialConfig::Process(process) => { + // todo: PASETO with process run_command(config, &process, sid, Action::Get)?.unwrap() } RegistryCredentialConfig::AsymmetricKey((secret_key, secret_key_subject)) => { @@ -340,10 +354,41 @@ fn auth_token_optional(config: &Config, sid: &SourceId) -> CargoResult return None, + Mutation::Publish { .. } => "publish", + Mutation::Yank { .. } => "yank", + Mutation::Unyank { .. } => "unyank", + Mutation::Owners { .. } => "owners", + }) + }), + name: mutation.and_then(|m| { + Some(match m { + Mutation::PrePublish => return None, + Mutation::Publish { name, .. } + | Mutation::Yank { name, .. } + | Mutation::Unyank { name, .. } + | Mutation::Owners { name, .. } => *name, + }) + }), + vers: mutation.and_then(|m| { + Some(match m { + Mutation::PrePublish | Mutation::Owners { .. } => return None, + Mutation::Publish { vers, .. } + | Mutation::Yank { vers, .. } + | Mutation::Unyank { vers, .. } => *vers, + }) + }), + cksum: mutation.and_then(|m| { + Some(match m { + Mutation::PrePublish + | Mutation::Yank { .. } + | Mutation::Unyank { .. } + | Mutation::Owners { .. } => return None, + Mutation::Publish { cksum, .. } => *cksum, + }) + }), challenge: None, // todo: PASETO with challenges v: None, }; @@ -367,10 +412,32 @@ fn auth_token_optional(config: &Config, sid: &SourceId) -> CargoResult { + PrePublish, + Publish { + name: &'a str, + vers: &'a str, + cksum: &'a str, + }, + Yank { + name: &'a str, + vers: &'a str, + }, + Unyank { + name: &'a str, + vers: &'a str, + }, + Owners { + name: &'a str, + }, +} + #[derive(serde::Serialize)] struct Message<'a> { iat: &'a str, diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 472d2f79e..57d255d01 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -193,7 +193,7 @@ pub struct Config { updated_sources: LazyCell>>, /// Cache of credentials from configuration or credential providers. /// Maps from url to credential value. - credential_cache: LazyCell>>, + credential_cache: LazyCell>>, /// Lock, if held, of the global package cache along with the number of /// acquisitions so far. package_cache_lock: RefCell, usize)>>, @@ -468,7 +468,7 @@ impl Config { } /// Cached credentials from credential providers or configuration. - pub fn credential_cache(&self) -> RefMut<'_, HashMap> { + pub fn credential_cache(&self) -> RefMut<'_, HashMap> { self.credential_cache .borrow_with(|| RefCell::new(HashMap::new())) .borrow_mut() From 2ac15086fb70bae924f5c38f7abd2c350641b6fb Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Mon, 12 Dec 2022 17:49:22 +0000 Subject: [PATCH 08/22] end-to-end tests --- crates/cargo-test-support/Cargo.toml | 3 + crates/cargo-test-support/src/registry.rs | 313 ++++++++++++++++++---- tests/testsuite/credential_process.rs | 4 +- tests/testsuite/owner.rs | 66 +++++ tests/testsuite/publish.rs | 80 +++++- tests/testsuite/registry_auth.rs | 34 +++ tests/testsuite/yank.rs | 38 +++ 7 files changed, 481 insertions(+), 57 deletions(-) diff --git a/crates/cargo-test-support/Cargo.toml b/crates/cargo-test-support/Cargo.toml index 20a129863..65e0f7566 100644 --- a/crates/cargo-test-support/Cargo.toml +++ b/crates/cargo-test-support/Cargo.toml @@ -15,10 +15,13 @@ crates-io = { path = "../crates-io" } snapbox = { version = "0.4.0", features = ["diff", "path"] } filetime = "0.2" flate2 = { version = "1.0", default-features = false, features = ["zlib"] } +pasetors = { version = "0.6.4", features = ["v3", "paserk", "std", "serde"] } +time = { version = "0.3", features = ["parsing", "formatting"]} git2 = "0.15.0" glob = "0.3" itertools = "0.10.0" lazy_static = "1.0" +serde = { version = "1.0.123", features = ["derive"] } serde_json = "1.0" tar = { version = "0.4.38", default-features = false } termcolor = "1.1.2" diff --git a/crates/cargo-test-support/src/registry.rs b/crates/cargo-test-support/src/registry.rs index 5290a83ed..fb2fb5ae3 100644 --- a/crates/cargo-test-support/src/registry.rs +++ b/crates/cargo-test-support/src/registry.rs @@ -5,6 +5,9 @@ use cargo_util::paths::append; use cargo_util::Sha256; use flate2::write::GzEncoder; use flate2::Compression; +use pasetors::keys::{AsymmetricPublicKey, AsymmetricSecretKey}; +use pasetors::paserk::FormatAsPaserk; +use pasetors::token::UntrustedToken; use std::collections::{BTreeMap, HashMap}; use std::fmt; use std::fs::{self, File}; @@ -13,6 +16,8 @@ use std::net::{SocketAddr, TcpListener, TcpStream}; use std::path::PathBuf; use std::thread::{self, JoinHandle}; use tar::{Builder, Header}; +use time::format_description::well_known::Rfc3339; +use time::{Duration, OffsetDateTime}; use url::Url; /// Gets the path to the local index pretending to be crates.io. This is a Git repo @@ -55,12 +60,30 @@ fn generate_url(name: &str) -> Url { Url::from_file_path(generate_path(name)).ok().unwrap() } +#[derive(Clone)] +pub enum Token { + Plaintext(String), + Keys(String, Option), +} + +impl Token { + /// This is a valid PASETO secret key. + /// This one is already publicly available as part of the text of the RFC so is safe to use for tests. + pub fn rfc_key() -> Token { + Token::Keys( + "k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36" + .to_string(), + Some("sub".to_string()), + ) + } +} + /// A builder for initializing registries. pub struct RegistryBuilder { /// If set, configures an alternate registry with the given name. alternative: Option, - /// If set, the authorization token for the registry. - token: Option, + /// The authorization token for the registry. + token: Option, /// If set, the registry requires authorization for all operations. auth_required: bool, /// If set, serves the index over http. @@ -83,7 +106,7 @@ pub struct TestRegistry { path: PathBuf, api_url: Url, dl_url: Url, - token: Option, + token: Token, } impl TestRegistry { @@ -96,9 +119,17 @@ impl TestRegistry { } pub fn token(&self) -> &str { - self.token - .as_deref() - .expect("registry was not configured with a token") + match &self.token { + Token::Plaintext(s) => s, + Token::Keys(_, _) => panic!("registry was not configured with a plaintext token"), + } + } + + pub fn key(&self) -> &str { + match &self.token { + Token::Plaintext(_) => panic!("registry was not configured with a secret key"), + Token::Keys(s, _) => s, + } } /// Shutdown the server thread and wait for it to stop. @@ -169,8 +200,8 @@ impl RegistryBuilder { /// Sets the token value #[must_use] - pub fn token(mut self, token: &str) -> Self { - self.token = Some(token.to_string()); + pub fn token(mut self, token: Token) -> Self { + self.token = Some(token); self } @@ -219,7 +250,9 @@ impl RegistryBuilder { let dl_url = generate_url(&format!("{prefix}dl")); let dl_path = generate_path(&format!("{prefix}dl")); let api_path = generate_path(&format!("{prefix}api")); - let token = Some(self.token.unwrap_or_else(|| format!("{prefix}sekrit"))); + let token = self + .token + .unwrap_or_else(|| Token::Plaintext(format!("{prefix}sekrit"))); let (server, index_url, api_url, dl_url) = if !self.http_index && !self.http_api { // No need to start the HTTP server. @@ -287,32 +320,48 @@ impl RegistryBuilder { } if self.configure_token { - let token = registry.token.as_deref().unwrap(); let credentials = paths::home().join(".cargo/credentials"); - if let Some(alternative) = &self.alternative { - append( - &credentials, - format!( - r#" + match ®istry.token { + Token::Plaintext(token) => { + if let Some(alternative) = &self.alternative { + append( + &credentials, + format!( + r#" [registries.{alternative}] token = "{token}" "# - ) - .as_bytes(), - ) - .unwrap(); - } else { - append( - &credentials, - format!( - r#" + ) + .as_bytes(), + ) + .unwrap(); + } else { + append( + &credentials, + format!( + r#" [registry] token = "{token}" "# - ) - .as_bytes(), - ) - .unwrap(); + ) + .as_bytes(), + ) + .unwrap(); + } + } + Token::Keys(key, subject) => { + let mut out = if let Some(alternative) = &self.alternative { + format!("\n[registries.{alternative}]\n") + } else { + format!("\n[registry]\n") + }; + out += &format!("secret-key = \"{key}\"\n"); + if let Some(subject) = subject { + out += &format!("secret-key-subject = \"{subject}\"\n"); + } + + append(&credentials, out.as_bytes()).unwrap(); + } } } @@ -536,16 +585,24 @@ pub struct HttpServer { listener: TcpListener, registry_path: PathBuf, dl_path: PathBuf, - token: Option, + addr: SocketAddr, + token: Token, auth_required: bool, custom_responders: HashMap<&'static str, Box Response>>, } +pub struct Mutation<'a> { + pub mutation: &'a str, + pub name: Option<&'a str>, + pub vers: Option<&'a str>, + pub cksum: Option<&'a str>, +} + impl HttpServer { pub fn new( registry_path: PathBuf, dl_path: PathBuf, - token: Option, + token: Token, auth_required: bool, api_responders: HashMap< &'static str, @@ -558,6 +615,7 @@ impl HttpServer { listener, registry_path, dl_path, + addr, token, auth_required, custom_responders: api_responders, @@ -648,17 +706,135 @@ impl HttpServer { } } - /// Route the request - fn route(&self, req: &Request) -> Response { - let authorized = |mutatation: bool| { - if mutatation || self.auth_required { - self.token == req.authorization - } else { - assert!(req.authorization.is_none(), "unexpected token"); - true + fn check_authorized(&self, req: &Request, mutation: Option) -> bool { + let (private_key, private_key_subject) = if mutation.is_some() || self.auth_required { + match &self.token { + Token::Plaintext(token) => return Some(token) == req.authorization.as_ref(), + Token::Keys(private_key, private_key_subject) => { + (private_key.as_str(), private_key_subject) + } } + } else { + assert!(req.authorization.is_none(), "unexpected token"); + return true; }; + macro_rules! t { + ($e:expr) => { + match $e { + Some(e) => e, + None => return false, + } + }; + } + + let secret: AsymmetricSecretKey = private_key.try_into().unwrap(); + let public: AsymmetricPublicKey = (&secret).try_into().unwrap(); + let pub_key_id: pasetors::paserk::Id = (&public).into(); + let mut paserk_pub_key_id = String::new(); + FormatAsPaserk::fmt(&pub_key_id, &mut paserk_pub_key_id).unwrap(); + // https://github.com/rust-lang/rfcs/blob/master/text/3231-cargo-asymmetric-tokens.md#how-the-registry-server-will-validate-an-asymmetric-token + + // - The PASETO is in v3.public format. + let authorization = t!(&req.authorization); + let untrusted_token = t!( + UntrustedToken::::try_from(authorization) + .ok() + ); + + // - The PASETO validates using the public key it looked up based on the key ID. + #[derive(serde::Deserialize, Debug)] + struct Footer<'a> { + url: &'a str, + kip: &'a str, + } + let footer: Footer = t!(serde_json::from_slice(untrusted_token.untrusted_footer()).ok()); + if footer.kip != paserk_pub_key_id { + return false; + } + let trusted_token = + t!( + pasetors::version3::PublicToken::verify(&public, &untrusted_token, None, None,) + .ok() + ); + + // - The URL matches the registry base URL + if footer.url != "https://github.com/rust-lang/crates.io-index" + && footer.url != &format!("sparse+http://{}/index/", self.addr.to_string()) + { + dbg!(footer.url); + return false; + } + + // - The PASETO is still within its valid time period. + #[derive(serde::Deserialize)] + struct Message<'a> { + iat: &'a str, + sub: Option<&'a str>, + mutation: Option<&'a str>, + name: Option<&'a str>, + vers: Option<&'a str>, + cksum: Option<&'a str>, + _challenge: Option<&'a str>, // todo: PASETO with challenges + v: Option, + } + let message: Message = t!(serde_json::from_str(trusted_token.payload()).ok()); + let token_time = t!(OffsetDateTime::parse(message.iat, &Rfc3339).ok()); + let now = OffsetDateTime::now_utc(); + if (now - token_time) > Duration::MINUTE { + return false; + } + if private_key_subject.as_deref() != message.sub { + dbg!(message.sub); + return false; + } + // - If the claim v is set, that it has the value of 1. + if let Some(v) = message.v { + if v != 1 { + dbg!(message.v); + return false; + } + } + // - If the server issues challenges, that the challenge has not yet been answered. + // todo: PASETO with challenges + // - If the operation is a mutation: + if let Some(mutation) = mutation { + // - That the operation matches the mutation field an is one of publish, yank, or unyank. + if message.mutation != Some(mutation.mutation) { + dbg!(message.mutation); + return false; + } + // - That the package, and version match the request. + if message.name != mutation.name { + dbg!(message.name); + return false; + } + if message.vers != mutation.vers { + dbg!(message.vers); + return false; + } + // - If the mutation is publish, that the version has not already been published, and that the hash matches the request. + if mutation.mutation == "publish" { + if message.cksum != mutation.cksum { + dbg!(message.cksum); + return false; + } + } + } else { + // - If the operation is a read, that the mutation field is not set. + if message.mutation.is_some() + || message.name.is_some() + || message.vers.is_some() + || message.cksum.is_some() + { + return false; + } + } + true + } + + /// Route the request + fn route(&self, req: &Request) -> Response { // Check for custom responder if let Some(responder) = self.custom_responders.get(req.url.path()) { return responder(&req, self); @@ -666,39 +842,53 @@ impl HttpServer { let path: Vec<_> = req.url.path()[1..].split('/').collect(); match (req.method.as_str(), path.as_slice()) { ("get", ["index", ..]) => { - if !authorized(false) { + if !self.check_authorized(req, None) { self.unauthorized(req) } else { self.index(&req) } } ("get", ["dl", ..]) => { - if !authorized(false) { + if !self.check_authorized(req, None) { self.unauthorized(req) } else { self.dl(&req) } } // publish - ("put", ["api", "v1", "crates", "new"]) => { - if !authorized(true) { - self.unauthorized(req) - } else { - self.publish(req) - } - } + ("put", ["api", "v1", "crates", "new"]) => self.check_authorized_publish(req), // The remainder of the operators in the test framework do nothing other than responding 'ok'. // // Note: We don't need to support anything real here because there are no tests that // currently require anything other than publishing via the http api. - // yank - ("delete", ["api", "v1", "crates", .., "yank"]) - // unyank - | ("put", ["api", "v1", "crates", .., "unyank"]) + // yank / unyank + ("delete" | "put", ["api", "v1", "crates", crate_name, version, mutation]) => { + if !self.check_authorized( + req, + Some(Mutation { + mutation, + name: Some(crate_name), + vers: Some(version), + cksum: None, + }), + ) { + self.unauthorized(req) + } else { + self.ok(&req) + } + } // owners - | ("get" | "put" | "delete", ["api", "v1", "crates", .., "owners"]) => { - if !authorized(true) { + ("get" | "put" | "delete", ["api", "v1", "crates", crate_name, "owners"]) => { + if !self.check_authorized( + req, + Some(Mutation { + mutation: "owners", + name: Some(crate_name), + vers: None, + cksum: None, + }), + ) { self.unauthorized(req) } else { self.ok(&req) @@ -813,7 +1003,7 @@ impl HttpServer { } } - pub fn publish(&self, req: &Request) -> Response { + pub fn check_authorized_publish(&self, req: &Request) -> Response { if let Some(body) = &req.body { // Get the metadata of the package let (len, remaining) = body.split_at(4); @@ -824,6 +1014,19 @@ impl HttpServer { let (len, remaining) = remaining.split_at(4); let file_len = u32::from_le_bytes(len.try_into().unwrap()); let (file, _remaining) = remaining.split_at(file_len as usize); + let file_cksum = cksum(&file); + + if !self.check_authorized( + req, + Some(Mutation { + mutation: "publish", + name: Some(&new_crate.name), + vers: Some(&new_crate.vers), + cksum: Some(&file_cksum), + }), + ) { + return self.unauthorized(req); + } // Write the `.crate` let dst = self @@ -860,7 +1063,7 @@ impl HttpServer { serde_json::json!(new_crate.name), &new_crate.vers, deps, - &cksum(file), + &file_cksum, new_crate.features, false, new_crate.links, diff --git a/tests/testsuite/credential_process.rs b/tests/testsuite/credential_process.rs index 566508c86..92a038179 100644 --- a/tests/testsuite/credential_process.rs +++ b/tests/testsuite/credential_process.rs @@ -158,7 +158,9 @@ fn get_token_test() -> (Project, TestRegistry) { // API server that checks that the token is included correctly. let server = registry::RegistryBuilder::new() .no_configure_token() - .token("sekrit") + .token(cargo_test_support::registry::Token::Plaintext( + "sekrit".to_string(), + )) .alternative() .http_api() .build(); diff --git a/tests/testsuite/owner.rs b/tests/testsuite/owner.rs index a5f177f74..83252e410 100644 --- a/tests/testsuite/owner.rs +++ b/tests/testsuite/owner.rs @@ -91,6 +91,39 @@ Caused by: .run(); } +#[cargo_test] +fn simple_add_with_asymmetric() { + let registry = registry::RegistryBuilder::new() + .http_api() + .token(cargo_test_support::registry::Token::rfc_key()) + .build(); + setup("foo", None); + + let p = project() + .file( + "Cargo.toml", + r#" + [project] + name = "foo" + version = "0.0.1" + authors = [] + license = "MIT" + description = "foo" + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + // The http_api server will check that the authorization is correct. + // If the authorization was not sent then we wuld get an unauthorized error. + p.cargo("owner -a username") + .arg("-Zregistry-auth") + .masquerade_as_nightly_cargo(&["registry-auth"]) + .replace_crates_io(registry.index_url()) + .with_status(0) + .run(); +} + #[cargo_test] fn simple_remove() { let registry = registry::init(); @@ -124,3 +157,36 @@ Caused by: ) .run(); } + +#[cargo_test] +fn simple_remove_with_asymmetric() { + let registry = registry::RegistryBuilder::new() + .http_api() + .token(cargo_test_support::registry::Token::rfc_key()) + .build(); + setup("foo", None); + + let p = project() + .file( + "Cargo.toml", + r#" + [project] + name = "foo" + version = "0.0.1" + authors = [] + license = "MIT" + description = "foo" + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + // The http_api server will check that the authorization is correct. + // If the authorization was not sent then we wuld get an unauthorized error. + p.cargo("owner -r username") + .arg("-Zregistry-auth") + .replace_crates_io(registry.index_url()) + .masquerade_as_nightly_cargo(&["registry-auth"]) + .with_status(0) + .run(); +} diff --git a/tests/testsuite/publish.rs b/tests/testsuite/publish.rs index 91fe0c3b6..8cfafec8a 100644 --- a/tests/testsuite/publish.rs +++ b/tests/testsuite/publish.rs @@ -134,6 +134,83 @@ See [..] // Check that the `token` key works at the root instead of under a // `[registry]` table. +#[cargo_test] +fn simple_publish_with_http() { + let _reg = registry::RegistryBuilder::new() + .http_api() + .token(registry::Token::Plaintext("sekrit".to_string())) + .build(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + authors = [] + license = "MIT" + description = "foo" + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + p.cargo("publish --no-verify --token sekrit --registry dummy-registry") + .with_stderr( + "\ +[UPDATING] `dummy-registry` index +[WARNING] manifest has no documentation, [..] +See [..] +[PACKAGING] foo v0.0.1 ([CWD]) +[PACKAGED] [..] files, [..] ([..] compressed) +[UPLOADING] foo v0.0.1 ([CWD]) +[UPDATING] `dummy-registry` index +", + ) + .run(); +} + +#[cargo_test] +fn simple_publish_with_asymmetric() { + let _reg = registry::RegistryBuilder::new() + .http_api() + .http_index() + .alternative_named("dummy-registry") + .token(registry::Token::rfc_key()) + .build(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + authors = [] + license = "MIT" + description = "foo" + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + p.cargo("publish --no-verify -Zregistry-auth -Zsparse-registry --registry dummy-registry") + .masquerade_as_nightly_cargo(&["registry-auth", "sparse-registry"]) + .with_stderr( + "\ +[UPDATING] `dummy-registry` index +[WARNING] manifest has no documentation, [..] +See [..] +[PACKAGING] foo v0.0.1 ([CWD]) +[PACKAGED] [..] files, [..] ([..] compressed) +[UPLOADING] foo v0.0.1 ([CWD]) +[UPDATING] `dummy-registry` index +", + ) + .run(); +} + #[cargo_test] fn old_token_location() { // `publish` generally requires a remote registry @@ -2579,7 +2656,8 @@ fn wait_for_subsequent_publish() { *lock += 1; if *lock == 3 { // Run the publish on the 3rd attempt - server.publish(&publish_req2.lock().unwrap().as_ref().unwrap()); + let rep = server.check_authorized_publish(&publish_req2.lock().unwrap().as_ref().unwrap()); + assert_eq!(rep.code, 200); } server.index(req) }) diff --git a/tests/testsuite/registry_auth.rs b/tests/testsuite/registry_auth.rs index 3aa42e8f6..839fc3054 100644 --- a/tests/testsuite/registry_auth.rs +++ b/tests/testsuite/registry_auth.rs @@ -64,6 +64,19 @@ fn simple() { cargo(&p, "build").with_stderr(SUCCCESS_OUTPUT).run(); } +#[cargo_test] +fn simple_with_asymmetric() { + let _registry = RegistryBuilder::new() + .alternative() + .auth_required() + .http_index() + .token(cargo_test_support::registry::Token::rfc_key()) + .build(); + + let p = make_project(); + cargo(&p, "build").with_stderr(SUCCCESS_OUTPUT).run(); +} + #[cargo_test] fn environment_config() { let registry = RegistryBuilder::new() @@ -100,6 +113,27 @@ fn environment_token() { .run(); } +#[cargo_test] +fn environment_token_with_asymmetric() { + let registry = RegistryBuilder::new() + .alternative() + .auth_required() + .no_configure_token() + .http_index() + .token(cargo_test_support::registry::Token::Keys( + "k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36" + .to_string(), + None, + )) + .build(); + + let p = make_project(); + cargo(&p, "build") + .env("CARGO_REGISTRIES_ALTERNATIVE_SECRET_KEY", registry.key()) + .with_stderr(SUCCCESS_OUTPUT) + .run(); +} + #[cargo_test] fn missing_token() { let _registry = RegistryBuilder::new() diff --git a/tests/testsuite/yank.rs b/tests/testsuite/yank.rs index b9da40780..c36c6ef92 100644 --- a/tests/testsuite/yank.rs +++ b/tests/testsuite/yank.rs @@ -50,6 +50,44 @@ Caused by: .run(); } +#[cargo_test] +fn explicit_version_with_asymmetric() { + let registry = registry::RegistryBuilder::new() + .http_api() + .token(cargo_test_support::registry::Token::rfc_key()) + .build(); + setup("foo", "0.0.1"); + + let p = project() + .file( + "Cargo.toml", + r#" + [project] + name = "foo" + version = "0.0.1" + authors = [] + license = "MIT" + description = "foo" + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + // The http_api server will check that the authorization is correct. + // If the authorization was not sent then we wuld get an unauthorized error. + p.cargo("yank --version 0.0.1") + .arg("-Zregistry-auth") + .masquerade_as_nightly_cargo(&["registry-auth"]) + .replace_crates_io(registry.index_url()) + .run(); + + p.cargo("yank --undo --version 0.0.1") + .arg("-Zregistry-auth") + .masquerade_as_nightly_cargo(&["registry-auth"]) + .replace_crates_io(registry.index_url()) + .run(); +} + #[cargo_test] fn inline_version() { let registry = registry::init(); From c4eb6707beb0f21f62f55d95ebf860c36654af04 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Tue, 13 Dec 2022 00:08:02 +0000 Subject: [PATCH 09/22] remove duplication in auth --- src/cargo/util/auth.rs | 115 +++++++++++++++++++++-------------------- 1 file changed, 59 insertions(+), 56 deletions(-) diff --git a/src/cargo/util/auth.rs b/src/cargo/util/auth.rs index ec01d600a..ece79508a 100644 --- a/src/cargo/util/auth.rs +++ b/src/cargo/util/auth.rs @@ -55,39 +55,14 @@ pub fn registry_credential_config( secret_key_subject, .. } = config.get::("registry")?; - let credential_process = - credential_process.filter(|_| config.cli_unstable().credential_process); - let secret_key = secret_key.filter(|_| config.cli_unstable().registry_auth); - let secret_key_subject = secret_key_subject.filter(|_| config.cli_unstable().registry_auth); - - let err_both = |token_key: &str, proc_key: &str| { - Err(format_err!( - "both `{token_key}` and `{proc_key}` \ - were specified in the config`.\n\ - Only one of these values may be set, remove one or the other to proceed.", - )) - }; - return Ok( - match (token, credential_process, secret_key, secret_key_subject) { - (Some(_), Some(_), _, _) => return err_both("token", "credential-process"), - (Some(_), _, Some(_), _) => return err_both("token", "secret-key"), - (_, Some(_), Some(_), _) => return err_both("credential-process", "secret-key"), - (_, _, None, Some(_)) => { - return Err(format_err!( - "`secret-key-subject` was set but `secret-key` was not in the config.\n\ - Ether set the `secret-key` or remove the `secret-key-subject`." - )); - } - (Some(token), _, _, _) => RegistryCredentialConfig::Token(token), - (_, Some(process), _, _) => RegistryCredentialConfig::Process(( - process.path.resolve_program(config), - process.args, - )), - (None, None, Some(key), subject) => { - RegistryCredentialConfig::AsymmetricKey((key, subject)) - } - (None, None, None, _) => RegistryCredentialConfig::None, - }, + return registry_credential_config_iner( + true, + None, + token, + credential_process, + secret_key, + secret_key_subject, + config, ); } @@ -165,23 +140,46 @@ pub fn registry_credential_config( credential_process, .. } = config.get::(&format!("registries.{name}"))?; - let credential_process = - credential_process.filter(|_| config.cli_unstable().credential_process); - let secret_key = secret_key.filter(|_| config.cli_unstable().registry_auth); - let secret_key_subject = secret_key_subject.filter(|_| config.cli_unstable().registry_auth); (token, credential_process, secret_key, secret_key_subject) } else { log::debug!("no registry name found for {sid}"); (None, None, None, None) }; - let name = name.as_deref(); + registry_credential_config_iner( + false, + name.as_deref(), + token, + credential_process, + secret_key, + secret_key_subject, + config, + ) +} + +fn registry_credential_config_iner( + is_crates_io: bool, + name: Option<&str>, + token: Option, + credential_process: Option, + secret_key: Option, + secret_key_subject: Option, + config: &Config, +) -> CargoResult { + let credential_process = + credential_process.filter(|_| config.cli_unstable().credential_process); + let secret_key = secret_key.filter(|_| config.cli_unstable().registry_auth); + let secret_key_subject = secret_key_subject.filter(|_| config.cli_unstable().registry_auth); let err_both = |token_key: &str, proc_key: &str| { + let registry = if is_crates_io { + "".to_string() + } else { + format!(" for registry `{}`", name.unwrap_or("UN-NAMED")) + }; Err(format_err!( "both `{token_key}` and `{proc_key}` \ - were specified in the config for registry `{name}`.\n\ - Only one of these values may be set, remove one or the other to proceed.", - name = name.unwrap() + were specified in the config{registry}.\n\ + Only one of these values may be set, remove one or the other to proceed.", )) }; Ok( @@ -190,11 +188,15 @@ pub fn registry_credential_config( (Some(_), _, Some(_), _) => return err_both("token", "secret-key"), (_, Some(_), Some(_), _) => return err_both("credential-process", "secret-key"), (_, _, None, Some(_)) => { + let registry = if is_crates_io { + "".to_string() + } else { + format!(" for registry `{}`", name.as_ref().unwrap()) + }; return Err(format_err!( - "`secret-key-subject` was set but `secret-key` was not in the config \ - for registry `{}`.\n\ - Ether set the `secret-key` or remove the `secret-key-subject`.", - name.unwrap() + "`secret-key-subject` was set but `secret-key` was not in the config{}.\n\ + Ether set the `secret-key` or remove the `secret-key-subject`.", + registry )); } (Some(token), _, _, _) => RegistryCredentialConfig::Token(token), @@ -206,18 +208,19 @@ pub fn registry_credential_config( RegistryCredentialConfig::AsymmetricKey((key, subject)) } (None, None, None, _) => { - // If we couldn't find a registry-specific credential, try the global credential process. - if let Some(process) = config - .get::>("registry.credential-process")? - .filter(|_| config.cli_unstable().credential_process) - { - RegistryCredentialConfig::Process(( - process.path.resolve_program(config), - process.args, - )) - } else { - RegistryCredentialConfig::None + if !is_crates_io { + // If we couldn't find a registry-specific credential, try the global credential process. + if let Some(process) = config + .get::>("registry.credential-process")? + .filter(|_| config.cli_unstable().credential_process) + { + return Ok(RegistryCredentialConfig::Process(( + process.path.resolve_program(config), + process.args, + ))); + } } + RegistryCredentialConfig::None } }, ) From 83387da3059b116367044680ba7ea846a857da0b Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Tue, 13 Dec 2022 00:29:37 +0000 Subject: [PATCH 10/22] put some RFC text into documentation --- src/cargo/ops/registry.rs | 4 +-- src/doc/src/reference/unstable.md | 42 ++++++++++++++++++++++++++++++- tests/testsuite/publish.rs | 3 ++- 3 files changed, 45 insertions(+), 4 deletions(-) diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 15ec988d8..217617e3e 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -65,9 +65,9 @@ impl RegistryCredentialConfig { pub fn is_token(&self) -> bool { matches!(self, Self::Token(..)) } - /// Returns `true` if the credential is [`Key`]. + /// Returns `true` if the credential is [`AsymmetricKey`]. /// - /// [`Key`]: Credential::Key + /// [`AsymmetricKey`]: RegistryCredentialConfig::AsymmetricKey pub fn is_asymmetric_key(&self) -> bool { matches!(self, Self::AsymmetricKey(..)) } diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md index 64fef581d..f1473b1ea 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -100,7 +100,7 @@ Each new feature described below should explain how to use it. * [`cargo logout`](#cargo-logout) — Adds the `logout` command to remove the currently saved registry token. * [sparse-registry](#sparse-registry) — Adds support for fetching from static-file HTTP registries (`sparse+`) * [publish-timeout](#publish-timeout) — Controls the timeout between uploading the crate and being available in the index - * [registry-auth](#registry-auth) — Adds support for authenticated registries. + * [registry-auth](#registry-auth) — Adds support for authenticated registries, and generate registry authentication tokens using asymmetric cryptography. ### allow-features @@ -897,6 +897,46 @@ can go to get a token. WWW-Authenticate: Cargo login_url="https://test-registry-login/me ``` +This same flag is also used to enable asymmetric authentication tokens. +* Tracking Issue: [10519](https://github.com/rust-lang/cargo/issues/10519) +* RFC: [#3231](https://github.com/rust-lang/rfcs/pull/3231) + +Add support for Cargo to authenticate the user to registries without sending secrets over the network. + +In [`config.toml`](https://doc.rust-lang.org/cargo/reference/config.html) and `credentials.toml` files there is a field called `private-key`, which is a private key formatted in the secret [subset of `PASERK`](https://github.com/paseto-standard/paserk/blob/master/types/secret.md) and is used to sign asymmetric tokens + +A keypair can be generated with `cargo login --generate-keypair` which will: +- generate a public/private keypair in the currently recommended fashion. +- save the private key in `credentials.toml`. +- print the public key in [PASERK public](https://github.com/paseto-standard/paserk/blob/master/types/public.md) format. + +It is recommended that the `private-key` be saved in `credentials.toml`. It is also supported in `config.toml`, primarily so that it can be set using the associated environment variable, which is the recommended way to provide it in CI contexts. This setup is what we have for the `token` field for setting a secret token. + +There is also an optional field called `private-key-subject` which is a string chosen by the registry. +This string will be included as part of an asymmetric token and should not be secret. +It is intended for the rare use cases like "cryptographic proof that the central CA server authorized this action". Cargo requires it to be non-whitespace printable ASCII. Registries that need non-ASCII data should base64 encode it. + +Both fields can be set with `cargo login --registry=name --private-key --private-key-subject="subject"` which will prompt you to put in the key value. + +A registry can have at most one of `private-key`, `token`, or `credential-process` set. + +All PASETOs will include `iat`, the current time in ISO 8601 format. Cargo will include the following where appropriate: +- `sub` an optional, non-secret string chosen by the registry that is expected to be claimed with every request. The value will be the `private-key-subject` from the `config.toml` file. +- `mutation` if present, indicates that this request is a mutating operation (or a read-only operation if not present), must be one of the strings `publish`, `yank`, or `unyank`. + - `name` name of the crate related to this request. + - `vers` version string of the crate related to this request. + - `cksum` the SHA256 hash of the crate contents, as a string of 64 lowercase hexadecimal digits, must be present only when `mutation` is equal to `publish` +- `challenge` the challenge string received from a 401/403 from this server this session. Registries that issue challenges must track which challenges have been issued/used and never accept a given challenge more than once within the same validity period (avoiding the need to track every challenge ever issued). + +The "footer" (which is part of the signature) will be a JSON string in UTF-8 and include: +- `url` the RFC 3986 compliant URL where cargo got the config.json file, + - If this is a registry with an HTTP index, then this is the base URL that all index queries are relative to. + - If this is a registry with a GIT index, it is the URL Cargo used to clone the index. +- `kid` the identifier of the private key used to sign the request, using the [PASERK IDs](https://github.com/paseto-standard/paserk/blob/master/operations/ID.md) standard. + +PASETO includes the message that was signed, so the server does not have to reconstruct the exact string from the request in order to check the signature. The server does need to check that the signature is valid for the string in the PASETO and that the contents of that string matches the request. +If a claim should be expected for the request but is missing in the PASETO then the request must be rejected. + ### credential-process * Tracking Issue: [#8933](https://github.com/rust-lang/cargo/issues/8933) * RFC: [#2730](https://github.com/rust-lang/rfcs/pull/2730) diff --git a/tests/testsuite/publish.rs b/tests/testsuite/publish.rs index 8cfafec8a..d402569b5 100644 --- a/tests/testsuite/publish.rs +++ b/tests/testsuite/publish.rs @@ -2656,7 +2656,8 @@ fn wait_for_subsequent_publish() { *lock += 1; if *lock == 3 { // Run the publish on the 3rd attempt - let rep = server.check_authorized_publish(&publish_req2.lock().unwrap().as_ref().unwrap()); + let rep = server + .check_authorized_publish(&publish_req2.lock().unwrap().as_ref().unwrap()); assert_eq!(rep.code, 200); } server.index(req) From c2a1daab63f4e05dc0fae40fff455df13faeb9cd Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Tue, 13 Dec 2022 18:57:47 +0000 Subject: [PATCH 11/22] print the public key on login --- src/cargo/ops/registry.rs | 8 +++++--- src/cargo/util/auth.rs | 10 +++++++--- tests/testsuite/registry.rs | 18 +++++++++++++++--- 3 files changed, 27 insertions(+), 9 deletions(-) diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 217617e3e..331b4b113 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -30,7 +30,7 @@ use crate::ops; use crate::ops::Packages; use crate::sources::{RegistrySource, SourceConfigMap, CRATES_IO_DOMAIN, CRATES_IO_REGISTRY}; use crate::util::auth::{ - check_format_like_paserk_secret, {self, AuthorizationError}, + paserk_public_from_paserk_secret, {self, AuthorizationError}, }; use crate::util::config::{Config, SslVersionConfig, SslVersionConfigRange}; use crate::util::errors::CargoResult; @@ -807,7 +807,7 @@ pub fn registry_login( let new_token; if generate_keypair || secret_key_required || key_subject.is_some() { if !config.cli_unstable().registry_auth { - panic!("-registry_auth required."); + panic!("-Zregistry_auth required."); } assert!(token.is_none()); // we are dealing with asymmetric tokens @@ -839,7 +839,9 @@ pub fn registry_login( .cloned() .ok_or_else(|| anyhow!("need a secret_key to set a key_subject"))?; } - if !check_format_like_paserk_secret(&secret_key) { + if let Some(p) = paserk_public_from_paserk_secret(&secret_key) { + drop_println!(config, "{}", &p); + } else { bail!("not a validly formated PASERK secret key"); } new_token = RegistryCredentialConfig::AsymmetricKey(( diff --git a/src/cargo/util/auth.rs b/src/cargo/util/auth.rs index ece79508a..a56a0b2f2 100644 --- a/src/cargo/util/auth.rs +++ b/src/cargo/util/auth.rs @@ -3,6 +3,7 @@ use crate::util::{config, config::ConfigKey, CanonicalUrl, CargoResult, Config, IntoUrl}; use anyhow::{bail, format_err, Context as _}; use cargo_util::ProcessError; +use pasetors::paserk::FormatAsPaserk; use core::fmt; use pasetors::keys::{AsymmetricPublicKey, AsymmetricSecretKey}; use serde::Deserialize; @@ -488,9 +489,12 @@ pub fn login(config: &Config, sid: &SourceId, token: RegistryCredentialConfig) - Ok(()) } -pub(crate) fn check_format_like_paserk_secret(secret_key: &str) -> bool { - let key: Result, _> = secret_key.try_into(); - key.is_ok() +pub(crate) fn paserk_public_from_paserk_secret(secret_key: &str) -> Option { + let secret: AsymmetricSecretKey = secret_key.try_into().ok()?; + let public: AsymmetricPublicKey = (&secret).try_into().ok()?; + let mut paserk_pub_key = String::new(); + FormatAsPaserk::fmt(&public, &mut paserk_pub_key).unwrap(); + Some(paserk_pub_key) } /// Removes the token for the given registry. diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs index 7b3251501..2a90f3aa8 100644 --- a/tests/testsuite/registry.rs +++ b/tests/testsuite/registry.rs @@ -1135,7 +1135,11 @@ fn login_with_asymmetric_token_and_subject_on_stdin() { cargo_process("login --key-subject=foo --secret-key -v -Z registry-auth") .masquerade_as_nightly_cargo(&["registry-auth"]) .replace_crates_io(registry.index_url()) - .with_stdout("please paste the API secret key below") + .with_stdout( + "\ + please paste the API secret key below +k3.public.AmDwjlyf8jAV3gm5Z7Kz9xAOcsKslt_Vwp5v-emjFzBHLCtcANzTaVEghTNEMj9PkQ", + ) .with_stdin("k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36") .run(); let credentials = fs::read_to_string(&credentials).unwrap(); @@ -1152,7 +1156,11 @@ fn login_with_asymmetric_token_on_stdin() { cargo_process("login --secret-key -v -Z registry-auth") .masquerade_as_nightly_cargo(&["registry-auth"]) .replace_crates_io(registry.index_url()) - .with_stdout("please paste the API secret key below") + .with_stdout( + "\ + please paste the API secret key below +k3.public.AmDwjlyf8jAV3gm5Z7Kz9xAOcsKslt_Vwp5v-emjFzBHLCtcANzTaVEghTNEMj9PkQ", + ) .with_stdin("k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36") .run(); let credentials = fs::read_to_string(&credentials).unwrap(); @@ -1175,7 +1183,10 @@ fn login_with_asymmetric_key_subject_without_key() { cargo_process("login --secret-key -v -Z registry-auth") .masquerade_as_nightly_cargo(&["registry-auth"]) .replace_crates_io(registry.index_url()) - .with_stdout("please paste the API secret key below") + .with_stdout( + "please paste the API secret key below +k3.public.AmDwjlyf8jAV3gm5Z7Kz9xAOcsKslt_Vwp5v-emjFzBHLCtcANzTaVEghTNEMj9PkQ", + ) .with_stdin("k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36") .run(); @@ -1199,6 +1210,7 @@ fn login_with_generate_asymmetric_token() { cargo_process("login --generate-keypair -Z registry-auth") .masquerade_as_nightly_cargo(&["registry-auth"]) .replace_crates_io(registry.index_url()) + .with_stdout("k3.public.[..]") .run(); let credentials = fs::read_to_string(&credentials).unwrap(); assert!(credentials.contains("secret-key = \"k3.secret.")); From 29ff25f6d903e173b7d1a78b961882cf2cb4bf0a Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Tue, 13 Dec 2022 23:46:20 +0000 Subject: [PATCH 12/22] cleanups round 1 --- Cargo.toml | 2 +- crates/cargo-test-support/src/registry.rs | 14 +++++++------- crates/crates-io/Cargo.toml | 2 +- src/bin/cargo/commands/login.rs | 19 ++++++++++++++----- src/cargo/ops/registry.rs | 6 +++++- src/cargo/util/auth.rs | 2 +- src/doc/src/reference/unstable.md | 2 +- tests/testsuite/login.rs | 17 +++++++++++++++++ tests/testsuite/owner.rs | 4 ++-- tests/testsuite/registry.rs | 2 +- tests/testsuite/yank.rs | 2 +- 11 files changed, 51 insertions(+), 21 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 4472f5f3e..ff2dff68b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,7 +19,7 @@ path = "src/cargo/lib.rs" bytesize = "1.0" cargo-platform = { path = "crates/cargo-platform", version = "0.1.2" } cargo-util = { path = "crates/cargo-util", version = "0.2.3" } -crates-io = { path = "crates/crates-io", version = "0.35.0" } +crates-io = { path = "crates/crates-io", version = "0.35.1" } curl = { version = "0.4.44", features = ["http2"] } curl-sys = "0.4.59" env_logger = "0.10.0" diff --git a/crates/cargo-test-support/src/registry.rs b/crates/cargo-test-support/src/registry.rs index fb2fb5ae3..cf4069b79 100644 --- a/crates/cargo-test-support/src/registry.rs +++ b/crates/cargo-test-support/src/registry.rs @@ -294,8 +294,8 @@ impl RegistryBuilder { &config_path, format!( " - [registries.{alternative}] - index = '{}'", + [registries.{alternative}] + index = '{}'", registry.index_url ) .as_bytes(), @@ -306,11 +306,11 @@ impl RegistryBuilder { &config_path, format!( " - [source.crates-io] - replace-with = 'dummy-registry' + [source.crates-io] + replace-with = 'dummy-registry' - [registries.dummy-registry] - index = '{}'", + [registries.dummy-registry] + index = '{}'", registry.index_url ) .as_bytes(), @@ -799,7 +799,7 @@ impl HttpServer { // todo: PASETO with challenges // - If the operation is a mutation: if let Some(mutation) = mutation { - // - That the operation matches the mutation field an is one of publish, yank, or unyank. + // - That the operation matches the mutation field and is one of publish, yank, or unyank. if message.mutation != Some(mutation.mutation) { dbg!(message.mutation); return false; diff --git a/crates/crates-io/Cargo.toml b/crates/crates-io/Cargo.toml index f399ef9b4..7d641da9d 100644 --- a/crates/crates-io/Cargo.toml +++ b/crates/crates-io/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "crates-io" -version = "0.35.0" +version = "0.35.1" edition = "2021" license = "MIT OR Apache-2.0" repository = "https://github.com/rust-lang/cargo" diff --git a/src/bin/cargo/commands/login.rs b/src/bin/cargo/commands/login.rs index 3efa75fb7..d2bd9c166 100644 --- a/src/bin/cargo/commands/login.rs +++ b/src/bin/cargo/commands/login.rs @@ -11,15 +11,24 @@ pub fn cli() -> Command { .arg_quiet() .arg(Arg::new("token").action(ArgAction::Set)) .arg(opt("registry", "Registry to use").value_name("REGISTRY")) - .arg(flag("generate-keypair", "Generate a public/secret keypair").conflicts_with("token")) .arg( - flag("secret-key", "Prompt for secret key") + flag( + "generate-keypair", + "Generate a public/secret keypair (unstable)", + ) + .conflicts_with("token"), + ) + .arg( + flag("secret-key", "Prompt for secret key (unstable)") .conflicts_with_all(&["generate-keypair", "token"]), ) .arg( - opt("key-subject", "Set the key subject for this registry") - .value_name("SUBJECT") - .conflicts_with("token"), + opt( + "key-subject", + "Set the key subject for this registry (unstable)", + ) + .value_name("SUBJECT") + .conflicts_with("token"), ) .after_help("Run `cargo help login` for more detailed information.\n") } diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 331b4b113..dea7f703f 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -807,7 +807,11 @@ pub fn registry_login( let new_token; if generate_keypair || secret_key_required || key_subject.is_some() { if !config.cli_unstable().registry_auth { - panic!("-Zregistry_auth required."); + // todo use fail_if_stable_opt + bail!( + "asymmetric token options are unstable and require the \ + `-Z registry-auth` option on the nightly channel" + ); } assert!(token.is_none()); // we are dealing with asymmetric tokens diff --git a/src/cargo/util/auth.rs b/src/cargo/util/auth.rs index a56a0b2f2..be1208ffa 100644 --- a/src/cargo/util/auth.rs +++ b/src/cargo/util/auth.rs @@ -3,9 +3,9 @@ use crate::util::{config, config::ConfigKey, CanonicalUrl, CargoResult, Config, IntoUrl}; use anyhow::{bail, format_err, Context as _}; use cargo_util::ProcessError; -use pasetors::paserk::FormatAsPaserk; use core::fmt; use pasetors::keys::{AsymmetricPublicKey, AsymmetricSecretKey}; +use pasetors::paserk::FormatAsPaserk; use serde::Deserialize; use std::collections::HashMap; use std::error::Error; diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md index f1473b1ea..3b5626a24 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -903,7 +903,7 @@ This same flag is also used to enable asymmetric authentication tokens. Add support for Cargo to authenticate the user to registries without sending secrets over the network. -In [`config.toml`](https://doc.rust-lang.org/cargo/reference/config.html) and `credentials.toml` files there is a field called `private-key`, which is a private key formatted in the secret [subset of `PASERK`](https://github.com/paseto-standard/paserk/blob/master/types/secret.md) and is used to sign asymmetric tokens +In [`config.toml`](config.md) and `credentials.toml` files there is a field called `private-key`, which is a private key formatted in the secret [subset of `PASERK`](https://github.com/paseto-standard/paserk/blob/master/types/secret.md) and is used to sign asymmetric tokens A keypair can be generated with `cargo login --generate-keypair` which will: - generate a public/private keypair in the currently recommended fashion. diff --git a/tests/testsuite/login.rs b/tests/testsuite/login.rs index a6bc93f95..20ef8d1ba 100644 --- a/tests/testsuite/login.rs +++ b/tests/testsuite/login.rs @@ -153,3 +153,20 @@ fn bad_asymmetric_token_args() { .with_status(1) .run(); } + +// todo why do theas hang when run as a test? +// #[cargo_test] +// fn asymmetric_requires_nightly() { +// cargo_process("login --key-subject=foo") +// .with_status(101) +// .with_stderr_contains("asymmetric token options are unstable and require the `-Z registry-auth` option on the nightly channel") +// .run(); +// cargo_process("login --generate-keypair") +// .with_status(101) +// .with_stderr_contains("asymmetric token options are unstable and require the `-Z registry-auth` option on the nightly channel") +// .run(); +// cargo_process("login --secret-key") +// .with_status(101) +// .with_stderr_contains("asymmetric token options are unstable and require the `-Z registry-auth` option on the nightly channel") +// .run(); +// } diff --git a/tests/testsuite/owner.rs b/tests/testsuite/owner.rs index 83252e410..9fc960c92 100644 --- a/tests/testsuite/owner.rs +++ b/tests/testsuite/owner.rs @@ -115,7 +115,7 @@ fn simple_add_with_asymmetric() { .build(); // The http_api server will check that the authorization is correct. - // If the authorization was not sent then we wuld get an unauthorized error. + // If the authorization was not sent then we would get an unauthorized error. p.cargo("owner -a username") .arg("-Zregistry-auth") .masquerade_as_nightly_cargo(&["registry-auth"]) @@ -182,7 +182,7 @@ fn simple_remove_with_asymmetric() { .build(); // The http_api server will check that the authorization is correct. - // If the authorization was not sent then we wuld get an unauthorized error. + // If the authorization was not sent then we would get an unauthorized error. p.cargo("owner -r username") .arg("-Zregistry-auth") .replace_crates_io(registry.index_url()) diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs index 2a90f3aa8..7f5d1e8dd 100644 --- a/tests/testsuite/registry.rs +++ b/tests/testsuite/registry.rs @@ -1179,7 +1179,7 @@ fn login_with_asymmetric_key_subject_without_key() { .with_status(101) .run(); - // ok so ad a secret_key to the credentials + // ok so add a secret_key to the credentials cargo_process("login --secret-key -v -Z registry-auth") .masquerade_as_nightly_cargo(&["registry-auth"]) .replace_crates_io(registry.index_url()) diff --git a/tests/testsuite/yank.rs b/tests/testsuite/yank.rs index c36c6ef92..684a04508 100644 --- a/tests/testsuite/yank.rs +++ b/tests/testsuite/yank.rs @@ -74,7 +74,7 @@ fn explicit_version_with_asymmetric() { .build(); // The http_api server will check that the authorization is correct. - // If the authorization was not sent then we wuld get an unauthorized error. + // If the authorization was not sent then we would get an unauthorized error. p.cargo("yank --version 0.0.1") .arg("-Zregistry-auth") .masquerade_as_nightly_cargo(&["registry-auth"]) From 6f8b15ce518d8f9ca7335cef336cff473d76e7c9 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Wed, 14 Dec 2022 20:10:55 +0000 Subject: [PATCH 13/22] add a test for the unstableness of registry-auth --- src/cargo/core/features.rs | 23 ++++++++-------------- src/cargo/ops/registry.rs | 15 +++++++++++--- tests/testsuite/login.rs | 40 ++++++++++++++++++++++---------------- 3 files changed, 43 insertions(+), 35 deletions(-) diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index 9132f0e08..394ff2a5d 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -988,29 +988,22 @@ impl CliUnstable { pub fn fail_if_stable_opt(&self, flag: &str, issue: u32) -> CargoResult<()> { if !self.unstable_options { let see = format!( - "See https://github.com/rust-lang/cargo/issues/{} for more \ - information about the `{}` flag.", - issue, flag + "See https://github.com/rust-lang/cargo/issues/{issue} for more \ + information about the `{flag}` flag." ); // NOTE: a `config` isn't available here, check the channel directly let channel = channel(); if channel == "nightly" || channel == "dev" { bail!( - "the `{}` flag is unstable, pass `-Z unstable-options` to enable it\n\ - {}", - flag, - see + "the `{flag}` flag is unstable, pass `-Z unstable-options` to enable it\n\ + {see}" ); } else { bail!( - "the `{}` flag is unstable, and only available on the nightly channel \ - of Cargo, but this is the `{}` channel\n\ - {}\n\ - {}", - flag, - channel, - SEE_CHANNELS, - see + "the `{flag}` flag is unstable, and only available on the nightly channel \ + of Cargo, but this is the `{channel}` channel\n\ + {SEE_CHANNELS}\n\ + {see}" ); } } diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index dea7f703f..d70e86cd1 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -807,10 +807,19 @@ pub fn registry_login( let new_token; if generate_keypair || secret_key_required || key_subject.is_some() { if !config.cli_unstable().registry_auth { - // todo use fail_if_stable_opt + let flag = if generate_keypair { + "generate-keypair" + } else if secret_key_required { + "secret-key" + } else if key_subject.is_some() { + "key-subject" + } else { + unreachable!("how did whe get here"); + }; bail!( - "asymmetric token options are unstable and require the \ - `-Z registry-auth` option on the nightly channel" + "the `{flag}` flag is unstable, pass `-Z registry-auth` to enable it\n\ + See https://github.com/rust-lang/cargo/issues/10519 for more \ + information about the `{flag}` flag." ); } assert!(token.is_none()); diff --git a/tests/testsuite/login.rs b/tests/testsuite/login.rs index 20ef8d1ba..533309301 100644 --- a/tests/testsuite/login.rs +++ b/tests/testsuite/login.rs @@ -1,7 +1,7 @@ //! Tests for the `cargo login` command. use cargo_test_support::install::cargo_home; -use cargo_test_support::registry::RegistryBuilder; +use cargo_test_support::registry::{self, RegistryBuilder}; use cargo_test_support::{cargo_process, t}; use std::fs::{self}; use std::path::PathBuf; @@ -154,19 +154,25 @@ fn bad_asymmetric_token_args() { .run(); } -// todo why do theas hang when run as a test? -// #[cargo_test] -// fn asymmetric_requires_nightly() { -// cargo_process("login --key-subject=foo") -// .with_status(101) -// .with_stderr_contains("asymmetric token options are unstable and require the `-Z registry-auth` option on the nightly channel") -// .run(); -// cargo_process("login --generate-keypair") -// .with_status(101) -// .with_stderr_contains("asymmetric token options are unstable and require the `-Z registry-auth` option on the nightly channel") -// .run(); -// cargo_process("login --secret-key") -// .with_status(101) -// .with_stderr_contains("asymmetric token options are unstable and require the `-Z registry-auth` option on the nightly channel") -// .run(); -// } +#[cargo_test] +fn asymmetric_requires_nightly() { + let registry = registry::init(); + cargo_process("login --key-subject=foo") + .replace_crates_io(registry.index_url()) + .with_status(101) + .with_stderr_contains("[ERROR] the `key-subject` flag is unstable, pass `-Z registry-auth` to enable it\n\ + See https://github.com/rust-lang/cargo/issues/10519 for more information about the `key-subject` flag.") + .run(); + cargo_process("login --generate-keypair") + .replace_crates_io(registry.index_url()) + .with_status(101) + .with_stderr_contains("[ERROR] the `generate-keypair` flag is unstable, pass `-Z registry-auth` to enable it\n\ + See https://github.com/rust-lang/cargo/issues/10519 for more information about the `generate-keypair` flag.") + .run(); + cargo_process("login --secret-key") + .replace_crates_io(registry.index_url()) + .with_status(101) + .with_stderr_contains("[ERROR] the `secret-key` flag is unstable, pass `-Z registry-auth` to enable it\n\ + See https://github.com/rust-lang/cargo/issues/10519 for more information about the `secret-key` flag.") + .run(); +} From f20ab0fa9b76b0312cd7cdc08c65d9da9a0e9262 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Wed, 14 Dec 2022 20:40:59 +0000 Subject: [PATCH 14/22] move login tests --- tests/testsuite/login.rs | 143 +++++++++++++++++++++++++++++++++++- tests/testsuite/registry.rs | 139 ----------------------------------- 2 files changed, 142 insertions(+), 140 deletions(-) diff --git a/tests/testsuite/login.rs b/tests/testsuite/login.rs index 533309301..11621a6b4 100644 --- a/tests/testsuite/login.rs +++ b/tests/testsuite/login.rs @@ -1,8 +1,10 @@ //! Tests for the `cargo login` command. +use cargo_test_support::cargo_process; use cargo_test_support::install::cargo_home; +use cargo_test_support::paths::{self, CargoPathExt}; use cargo_test_support::registry::{self, RegistryBuilder}; -use cargo_test_support::{cargo_process, t}; +use cargo_test_support::t; use std::fs::{self}; use std::path::PathBuf; use toml_edit::easy as toml; @@ -176,3 +178,142 @@ fn asymmetric_requires_nightly() { See https://github.com/rust-lang/cargo/issues/10519 for more information about the `secret-key` flag.") .run(); } + +#[cargo_test] +fn login_with_no_cargo_dir() { + // Create a config in the root directory because `login` requires the + // index to be updated, and we don't want to hit crates.io. + let registry = registry::init(); + fs::rename(paths::home().join(".cargo"), paths::root().join(".cargo")).unwrap(); + paths::home().rm_rf(); + cargo_process("login foo -v") + .replace_crates_io(registry.index_url()) + .run(); + let credentials = fs::read_to_string(paths::home().join(".cargo/credentials")).unwrap(); + assert_eq!(credentials, "[registry]\ntoken = \"foo\"\n"); +} + +#[cargo_test] +fn login_with_differently_sized_token() { + // Verify that the configuration file gets properly truncated. + let registry = registry::init(); + let credentials = paths::home().join(".cargo/credentials"); + fs::remove_file(&credentials).unwrap(); + cargo_process("login lmaolmaolmao -v") + .replace_crates_io(registry.index_url()) + .run(); + cargo_process("login lmao -v") + .replace_crates_io(registry.index_url()) + .run(); + cargo_process("login lmaolmaolmao -v") + .replace_crates_io(registry.index_url()) + .run(); + let credentials = fs::read_to_string(&credentials).unwrap(); + assert_eq!(credentials, "[registry]\ntoken = \"lmaolmaolmao\"\n"); +} + +#[cargo_test] +fn login_with_token_on_stdin() { + let registry = registry::init(); + let credentials = paths::home().join(".cargo/credentials"); + fs::remove_file(&credentials).unwrap(); + cargo_process("login lmao -v") + .replace_crates_io(registry.index_url()) + .run(); + cargo_process("login") + .replace_crates_io(registry.index_url()) + .with_stdout("please paste the token found on [..]/me below") + .with_stdin("some token") + .run(); + let credentials = fs::read_to_string(&credentials).unwrap(); + assert_eq!(credentials, "[registry]\ntoken = \"some token\"\n"); +} + +#[cargo_test] +fn login_with_asymmetric_token_and_subject_on_stdin() { + let registry = registry::init(); + let credentials = paths::home().join(".cargo/credentials"); + fs::remove_file(&credentials).unwrap(); + cargo_process("login --key-subject=foo --secret-key -v -Z registry-auth") + .masquerade_as_nightly_cargo(&["registry-auth"]) + .replace_crates_io(registry.index_url()) + .with_stdout( + "\ + please paste the API secret key below +k3.public.AmDwjlyf8jAV3gm5Z7Kz9xAOcsKslt_Vwp5v-emjFzBHLCtcANzTaVEghTNEMj9PkQ", + ) + .with_stdin("k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36") + .run(); + let credentials = fs::read_to_string(&credentials).unwrap(); + assert!(credentials.starts_with("[registry]\n")); + assert!(credentials.contains("secret-key-subject = \"foo\"\n")); + assert!(credentials.contains("secret-key = \"k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36\"\n")); +} + +#[cargo_test] +fn login_with_asymmetric_token_on_stdin() { + let registry = registry::init(); + let credentials = paths::home().join(".cargo/credentials"); + fs::remove_file(&credentials).unwrap(); + cargo_process("login --secret-key -v -Z registry-auth") + .masquerade_as_nightly_cargo(&["registry-auth"]) + .replace_crates_io(registry.index_url()) + .with_stdout( + "\ + please paste the API secret key below +k3.public.AmDwjlyf8jAV3gm5Z7Kz9xAOcsKslt_Vwp5v-emjFzBHLCtcANzTaVEghTNEMj9PkQ", + ) + .with_stdin("k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36") + .run(); + let credentials = fs::read_to_string(&credentials).unwrap(); + assert_eq!(credentials, "[registry]\nsecret-key = \"k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36\"\n"); +} + +#[cargo_test] +fn login_with_asymmetric_key_subject_without_key() { + let registry = registry::init(); + let credentials = paths::home().join(".cargo/credentials"); + fs::remove_file(&credentials).unwrap(); + cargo_process("login --key-subject=foo -Z registry-auth") + .masquerade_as_nightly_cargo(&["registry-auth"]) + .replace_crates_io(registry.index_url()) + .with_stderr_contains("error: need a secret_key to set a key_subject") + .with_status(101) + .run(); + + // ok so add a secret_key to the credentials + cargo_process("login --secret-key -v -Z registry-auth") + .masquerade_as_nightly_cargo(&["registry-auth"]) + .replace_crates_io(registry.index_url()) + .with_stdout( + "please paste the API secret key below +k3.public.AmDwjlyf8jAV3gm5Z7Kz9xAOcsKslt_Vwp5v-emjFzBHLCtcANzTaVEghTNEMj9PkQ", + ) + .with_stdin("k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36") + .run(); + + // and then it shuld work + cargo_process("login --key-subject=foo -Z registry-auth") + .masquerade_as_nightly_cargo(&["registry-auth"]) + .replace_crates_io(registry.index_url()) + .run(); + + let credentials = fs::read_to_string(&credentials).unwrap(); + assert!(credentials.starts_with("[registry]\n")); + assert!(credentials.contains("secret-key-subject = \"foo\"\n")); + assert!(credentials.contains("secret-key = \"k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36\"\n")); +} + +#[cargo_test] +fn login_with_generate_asymmetric_token() { + let registry = registry::init(); + let credentials = paths::home().join(".cargo/credentials"); + fs::remove_file(&credentials).unwrap(); + cargo_process("login --generate-keypair -Z registry-auth") + .masquerade_as_nightly_cargo(&["registry-auth"]) + .replace_crates_io(registry.index_url()) + .with_stdout("k3.public.[..]") + .run(); + let credentials = fs::read_to_string(&credentials).unwrap(); + assert!(credentials.contains("secret-key = \"k3.secret.")); +} diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs index 7f5d1e8dd..16d36c378 100644 --- a/tests/testsuite/registry.rs +++ b/tests/testsuite/registry.rs @@ -1077,145 +1077,6 @@ fn dev_dependency_not_used(cargo: fn(&Project, &str) -> Execs) { .run(); } -#[cargo_test] -fn login_with_no_cargo_dir() { - // Create a config in the root directory because `login` requires the - // index to be updated, and we don't want to hit crates.io. - let registry = registry::init(); - fs::rename(paths::home().join(".cargo"), paths::root().join(".cargo")).unwrap(); - paths::home().rm_rf(); - cargo_process("login foo -v") - .replace_crates_io(registry.index_url()) - .run(); - let credentials = fs::read_to_string(paths::home().join(".cargo/credentials")).unwrap(); - assert_eq!(credentials, "[registry]\ntoken = \"foo\"\n"); -} - -#[cargo_test] -fn login_with_differently_sized_token() { - // Verify that the configuration file gets properly truncated. - let registry = registry::init(); - let credentials = paths::home().join(".cargo/credentials"); - fs::remove_file(&credentials).unwrap(); - cargo_process("login lmaolmaolmao -v") - .replace_crates_io(registry.index_url()) - .run(); - cargo_process("login lmao -v") - .replace_crates_io(registry.index_url()) - .run(); - cargo_process("login lmaolmaolmao -v") - .replace_crates_io(registry.index_url()) - .run(); - let credentials = fs::read_to_string(&credentials).unwrap(); - assert_eq!(credentials, "[registry]\ntoken = \"lmaolmaolmao\"\n"); -} - -#[cargo_test] -fn login_with_token_on_stdin() { - let registry = registry::init(); - let credentials = paths::home().join(".cargo/credentials"); - fs::remove_file(&credentials).unwrap(); - cargo_process("login lmao -v") - .replace_crates_io(registry.index_url()) - .run(); - cargo_process("login") - .replace_crates_io(registry.index_url()) - .with_stdout("please paste the token found on [..]/me below") - .with_stdin("some token") - .run(); - let credentials = fs::read_to_string(&credentials).unwrap(); - assert_eq!(credentials, "[registry]\ntoken = \"some token\"\n"); -} - -#[cargo_test] -fn login_with_asymmetric_token_and_subject_on_stdin() { - let registry = registry::init(); - let credentials = paths::home().join(".cargo/credentials"); - fs::remove_file(&credentials).unwrap(); - cargo_process("login --key-subject=foo --secret-key -v -Z registry-auth") - .masquerade_as_nightly_cargo(&["registry-auth"]) - .replace_crates_io(registry.index_url()) - .with_stdout( - "\ - please paste the API secret key below -k3.public.AmDwjlyf8jAV3gm5Z7Kz9xAOcsKslt_Vwp5v-emjFzBHLCtcANzTaVEghTNEMj9PkQ", - ) - .with_stdin("k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36") - .run(); - let credentials = fs::read_to_string(&credentials).unwrap(); - assert!(credentials.starts_with("[registry]\n")); - assert!(credentials.contains("secret-key-subject = \"foo\"\n")); - assert!(credentials.contains("secret-key = \"k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36\"\n")); -} - -#[cargo_test] -fn login_with_asymmetric_token_on_stdin() { - let registry = registry::init(); - let credentials = paths::home().join(".cargo/credentials"); - fs::remove_file(&credentials).unwrap(); - cargo_process("login --secret-key -v -Z registry-auth") - .masquerade_as_nightly_cargo(&["registry-auth"]) - .replace_crates_io(registry.index_url()) - .with_stdout( - "\ - please paste the API secret key below -k3.public.AmDwjlyf8jAV3gm5Z7Kz9xAOcsKslt_Vwp5v-emjFzBHLCtcANzTaVEghTNEMj9PkQ", - ) - .with_stdin("k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36") - .run(); - let credentials = fs::read_to_string(&credentials).unwrap(); - assert_eq!(credentials, "[registry]\nsecret-key = \"k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36\"\n"); -} - -#[cargo_test] -fn login_with_asymmetric_key_subject_without_key() { - let registry = registry::init(); - let credentials = paths::home().join(".cargo/credentials"); - fs::remove_file(&credentials).unwrap(); - cargo_process("login --key-subject=foo -Z registry-auth") - .masquerade_as_nightly_cargo(&["registry-auth"]) - .replace_crates_io(registry.index_url()) - .with_stderr_contains("error: need a secret_key to set a key_subject") - .with_status(101) - .run(); - - // ok so add a secret_key to the credentials - cargo_process("login --secret-key -v -Z registry-auth") - .masquerade_as_nightly_cargo(&["registry-auth"]) - .replace_crates_io(registry.index_url()) - .with_stdout( - "please paste the API secret key below -k3.public.AmDwjlyf8jAV3gm5Z7Kz9xAOcsKslt_Vwp5v-emjFzBHLCtcANzTaVEghTNEMj9PkQ", - ) - .with_stdin("k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36") - .run(); - - // and then it shuld work - cargo_process("login --key-subject=foo -Z registry-auth") - .masquerade_as_nightly_cargo(&["registry-auth"]) - .replace_crates_io(registry.index_url()) - .run(); - - let credentials = fs::read_to_string(&credentials).unwrap(); - assert!(credentials.starts_with("[registry]\n")); - assert!(credentials.contains("secret-key-subject = \"foo\"\n")); - assert!(credentials.contains("secret-key = \"k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36\"\n")); -} - -#[cargo_test] -fn login_with_generate_asymmetric_token() { - let registry = registry::init(); - let credentials = paths::home().join(".cargo/credentials"); - fs::remove_file(&credentials).unwrap(); - cargo_process("login --generate-keypair -Z registry-auth") - .masquerade_as_nightly_cargo(&["registry-auth"]) - .replace_crates_io(registry.index_url()) - .with_stdout("k3.public.[..]") - .run(); - let credentials = fs::read_to_string(&credentials).unwrap(); - assert!(credentials.contains("secret-key = \"k3.secret.")); -} - #[cargo_test] fn bad_license_file_http() { let registry = setup_http(); From 5e709d45f15914b220e072d44ccbefd8ebacc86d Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Wed, 14 Dec 2022 20:50:51 +0000 Subject: [PATCH 15/22] add comment to Mutation --- crates/cargo-test-support/src/registry.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/cargo-test-support/src/registry.rs b/crates/cargo-test-support/src/registry.rs index cf4069b79..eabf7d089 100644 --- a/crates/cargo-test-support/src/registry.rs +++ b/crates/cargo-test-support/src/registry.rs @@ -591,6 +591,8 @@ pub struct HttpServer { custom_responders: HashMap<&'static str, Box Response>>, } +/// A helper struct that collects the arguments for [HttpServer::check_authorized]. +/// Based on looking at the request, these are the fields that the authentication header should attest to. pub struct Mutation<'a> { pub mutation: &'a str, pub name: Option<&'a str>, From 1c52bbd4f8c5ff089a6b6ddb38b0a89fc157c708 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Wed, 14 Dec 2022 21:04:05 +0000 Subject: [PATCH 16/22] add CredentialCacheValue --- src/cargo/util/auth.rs | 26 +++++++++++++++++++------- src/cargo/util/config/mod.rs | 22 ++++++++++++++++++++-- 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/src/cargo/util/auth.rs b/src/cargo/util/auth.rs index be1208ffa..d99ce6fd0 100644 --- a/src/cargo/util/auth.rs +++ b/src/cargo/util/auth.rs @@ -19,6 +19,8 @@ use url::Url; use crate::core::SourceId; use crate::ops::RegistryCredentialConfig; +use super::config::CredentialCacheValue; + /// Get the credential configuration for a `SourceId`. pub fn registry_credential_config( config: &Config, @@ -298,9 +300,13 @@ my-registry = {{ index = "{}" }} // Store a token in the cache for future calls. pub fn cache_token(config: &Config, sid: &SourceId, token: &str) { let url = sid.canonical_url(); - config - .credential_cache() - .insert(url.clone(), (true, token.to_string())); + config.credential_cache().insert( + url.clone(), + CredentialCacheValue { + from_commandline: true, + token_value: token.to_string(), + }, + ); } /// Returns the token to use for the given registry. @@ -332,11 +338,11 @@ fn auth_token_optional( let mut cache = config.credential_cache(); let url = sid.canonical_url(); - if let Some((overridden_on_commandline, token)) = cache.get(url) { + if let Some(cache_token_value) = cache.get(url) { // Tokens for endpoints that do not involve a mutation can always be reused. // If the value is put in the cach by the command line, then we reuse it without looking at the configuration. - if *overridden_on_commandline || mutation.is_none() { - return Ok(Some(token.clone())); + if cache_token_value.from_commandline || mutation.is_none() { + return Ok(Some(cache_token_value.token_value.clone())); } } @@ -417,7 +423,13 @@ fn auth_token_optional( }; if mutation.is_none() { - cache.insert(url.clone(), (false, token.clone())); + cache.insert( + url.clone(), + CredentialCacheValue { + from_commandline: false, + token_value: token.to_string(), + }, + ); } Ok(Some(token)) } diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 57d255d01..62d130eac 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -136,6 +136,24 @@ enum WhyLoad { FileDiscovery, } +/// A previously generated authentication token and the data needed to determine if it can be reused. +pub struct CredentialCacheValue { + /// If the command line was used to override the token then it must always be reused, + /// even if reading the configuration files would lead to a different value. + pub from_commandline: bool, + pub token_value: String, +} + +impl fmt::Debug for CredentialCacheValue { + /// This manual implementation helps ensure that the token value is redacted from all logs. + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("CredentialCacheValue") + .field("from_commandline", &self.from_commandline) + .field("token_value", &"REDACTED") + .finish() + } +} + /// Configuration information for cargo. This is not specific to a build, it is information /// relating to cargo itself. #[derive(Debug)] @@ -193,7 +211,7 @@ pub struct Config { updated_sources: LazyCell>>, /// Cache of credentials from configuration or credential providers. /// Maps from url to credential value. - credential_cache: LazyCell>>, + credential_cache: LazyCell>>, /// Lock, if held, of the global package cache along with the number of /// acquisitions so far. package_cache_lock: RefCell, usize)>>, @@ -468,7 +486,7 @@ impl Config { } /// Cached credentials from credential providers or configuration. - pub fn credential_cache(&self) -> RefMut<'_, HashMap> { + pub fn credential_cache(&self) -> RefMut<'_, HashMap> { self.credential_cache .borrow_with(|| RefCell::new(HashMap::new())) .borrow_mut() From 6b83b3dc54b66624745478a59a19772939d949b1 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Wed, 14 Dec 2022 21:45:25 +0000 Subject: [PATCH 17/22] more tests --- tests/testsuite/registry_auth.rs | 79 ++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/tests/testsuite/registry_auth.rs b/tests/testsuite/registry_auth.rs index 839fc3054..794be9285 100644 --- a/tests/testsuite/registry_auth.rs +++ b/tests/testsuite/registry_auth.rs @@ -134,6 +134,85 @@ fn environment_token_with_asymmetric() { .run(); } +#[cargo_test] +fn bad_environment_token_with_asymmetric_subject() { + let registry = RegistryBuilder::new() + .alternative() + .auth_required() + .no_configure_token() + .http_index() + .token(cargo_test_support::registry::Token::Keys( + "k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36" + .to_string(), + None, + )) + .build(); + + let p = make_project(); + cargo(&p, "build") + .env("CARGO_REGISTRIES_ALTERNATIVE_SECRET_KEY", registry.key()) + .env( + "CARGO_REGISTRIES_ALTERNATIVE_SECRET_KEY_SUBJECT", + "incorrect", + ) + .with_stderr_contains( + " token rejected for `alternative`, please run `cargo login --registry alternative`", + ) + .with_status(101) + .run(); +} + +#[cargo_test] +fn bad_environment_token_with_asymmetric_incorrect_subject() { + let registry = RegistryBuilder::new() + .alternative() + .auth_required() + .no_configure_token() + .http_index() + .token(cargo_test_support::registry::Token::rfc_key()) + .build(); + + let p = make_project(); + cargo(&p, "build") + .env("CARGO_REGISTRIES_ALTERNATIVE_SECRET_KEY", registry.key()) + .env( + "CARGO_REGISTRIES_ALTERNATIVE_SECRET_KEY_SUBJECT", + "incorrect", + ) + .with_stderr_contains( + " token rejected for `alternative`, please run `cargo login --registry alternative`", + ) + .with_status(101) + .run(); +} + +#[cargo_test] +fn bad_environment_token_with_incorrect_asymmetric() { + let _registry = RegistryBuilder::new() + .alternative() + .auth_required() + .no_configure_token() + .http_index() + .token(cargo_test_support::registry::Token::Keys( + "k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36" + .to_string(), + None, + )) + .build(); + + let p = make_project(); + cargo(&p, "build") + .env( + "CARGO_REGISTRIES_ALTERNATIVE_SECRET_KEY", + "k3.secret.9Vxr5hVlI_g_orBZN54vPz20bmB4O76wB_MVqUSuJJJqHFLwP8kdn_RY5g6J6pQG", + ) + .with_stderr_contains( + " token rejected for `alternative`, please run `cargo login --registry alternative`", + ) + .with_status(101) + .run(); +} + #[cargo_test] fn missing_token() { let _registry = RegistryBuilder::new() From cd967098eaa7c566548725712fa329dca2a4adbc Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Mon, 19 Dec 2022 13:55:54 -0500 Subject: [PATCH 18/22] Indentation and spelling --- crates/cargo-test-support/src/registry.rs | 12 ++++++------ src/cargo/util/auth.rs | 14 +++++++------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/crates/cargo-test-support/src/registry.rs b/crates/cargo-test-support/src/registry.rs index eabf7d089..d55d6fbec 100644 --- a/crates/cargo-test-support/src/registry.rs +++ b/crates/cargo-test-support/src/registry.rs @@ -328,9 +328,9 @@ impl RegistryBuilder { &credentials, format!( r#" - [registries.{alternative}] - token = "{token}" - "# + [registries.{alternative}] + token = "{token}" + "# ) .as_bytes(), ) @@ -340,9 +340,9 @@ impl RegistryBuilder { &credentials, format!( r#" - [registry] - token = "{token}" - "# + [registry] + token = "{token}" + "# ) .as_bytes(), ) diff --git a/src/cargo/util/auth.rs b/src/cargo/util/auth.rs index d99ce6fd0..4653226f0 100644 --- a/src/cargo/util/auth.rs +++ b/src/cargo/util/auth.rs @@ -58,7 +58,7 @@ pub fn registry_credential_config( secret_key_subject, .. } = config.get::("registry")?; - return registry_credential_config_iner( + return registry_credential_config_inner( true, None, token, @@ -149,7 +149,7 @@ pub fn registry_credential_config( (None, None, None, None) }; - registry_credential_config_iner( + registry_credential_config_inner( false, name.as_deref(), token, @@ -160,7 +160,7 @@ pub fn registry_credential_config( ) } -fn registry_credential_config_iner( +fn registry_credential_config_inner( is_crates_io: bool, name: Option<&str>, token: Option, @@ -181,8 +181,8 @@ fn registry_credential_config_iner( }; Err(format_err!( "both `{token_key}` and `{proc_key}` \ - were specified in the config{registry}.\n\ - Only one of these values may be set, remove one or the other to proceed.", + were specified in the config{registry}.\n\ + Only one of these values may be set, remove one or the other to proceed.", )) }; Ok( @@ -198,7 +198,7 @@ fn registry_credential_config_iner( }; return Err(format_err!( "`secret-key-subject` was set but `secret-key` was not in the config{}.\n\ - Ether set the `secret-key` or remove the `secret-key-subject`.", + Either set the `secret-key` or remove the `secret-key-subject`.", registry )); } @@ -490,7 +490,7 @@ pub fn login(config: &Config, sid: &SourceId, token: RegistryCredentialConfig) - RegistryCredentialConfig::Process(process) => { let token = token .as_token() - .expect("credential_process can not use login with a secret_key") + .expect("credential_process cannot use login with a secret_key") .to_owned(); run_command(config, &process, sid, Action::Store(token))?; } From e50589bd147be50c57b40fcc6375f139d83d92a2 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Mon, 19 Dec 2022 19:28:10 +0000 Subject: [PATCH 19/22] cache more tokens --- src/cargo/util/auth.rs | 40 ++++++++++++++++++++++-------------- src/cargo/util/config/mod.rs | 3 +++ 2 files changed, 28 insertions(+), 15 deletions(-) diff --git a/src/cargo/util/auth.rs b/src/cargo/util/auth.rs index 4653226f0..f8a2e27a5 100644 --- a/src/cargo/util/auth.rs +++ b/src/cargo/util/auth.rs @@ -304,6 +304,7 @@ pub fn cache_token(config: &Config, sid: &SourceId, token: &str) { url.clone(), CredentialCacheValue { from_commandline: true, + independent_of_endpoint: true, token_value: token.to_string(), }, ); @@ -341,15 +342,18 @@ fn auth_token_optional( if let Some(cache_token_value) = cache.get(url) { // Tokens for endpoints that do not involve a mutation can always be reused. // If the value is put in the cach by the command line, then we reuse it without looking at the configuration. - if cache_token_value.from_commandline || mutation.is_none() { + if cache_token_value.from_commandline + || cache_token_value.independent_of_endpoint + || mutation.is_none() + { return Ok(Some(cache_token_value.token_value.clone())); } } let credential = registry_credential_config(config, sid)?; - let token = match credential { + let (independent_of_endpoint, token) = match credential { RegistryCredentialConfig::None => return Ok(None), - RegistryCredentialConfig::Token(config_token) => config_token.to_string(), + RegistryCredentialConfig::Token(config_token) => (true, config_token.to_string()), RegistryCredentialConfig::Process(process) => { // todo: PASETO with process run_command(config, &process, sid, Action::Get)?.unwrap() @@ -407,18 +411,21 @@ fn auth_token_optional( kip, }; - pasetors::version3::PublicToken::sign( - &secret, - serde_json::to_string(&message) - .expect("cannot serialize") - .as_bytes(), - Some( - serde_json::to_string(&footer) + ( + false, + pasetors::version3::PublicToken::sign( + &secret, + serde_json::to_string(&message) .expect("cannot serialize") .as_bytes(), - ), - None, - )? + Some( + serde_json::to_string(&footer) + .expect("cannot serialize") + .as_bytes(), + ), + None, + )?, + ) } }; @@ -427,6 +434,7 @@ fn auth_token_optional( url.clone(), CredentialCacheValue { from_commandline: false, + independent_of_endpoint, token_value: token.to_string(), }, ); @@ -527,7 +535,7 @@ fn run_command( process: &(PathBuf, Vec), sid: &SourceId, action: Action, -) -> CargoResult> { +) -> CargoResult> { let index_url = sid.url().as_str(); let cred_proc; let (exe, args) = if process.0.to_str().unwrap_or("").starts_with("cargo:") { @@ -552,6 +560,8 @@ fn run_command( Action::Erase => bail!(msg("log out")), } } + // todo: PASETO with process + let independent_of_endpoint = false; let action_str = match action { Action::Get => "get", Action::Store(_) => "store", @@ -622,7 +632,7 @@ fn run_command( } buffer.truncate(end); } - token = Some(buffer); + token = Some((independent_of_endpoint, buffer)); } Action::Store(token) => { writeln!(child.stdin.as_ref().unwrap(), "{}", token).with_context(|| { diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 62d130eac..d30e09441 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -141,6 +141,9 @@ pub struct CredentialCacheValue { /// If the command line was used to override the token then it must always be reused, /// even if reading the configuration files would lead to a different value. pub from_commandline: bool, + /// If nothing depends on which endpoint is being hit, then we can reuse the token + /// for any future request even if some of the requests involve mutations. + pub independent_of_endpoint: bool, pub token_value: String, } From a3857f7673ec56390caea86893faef0bd34d6e8f Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Mon, 19 Dec 2022 20:04:22 +0000 Subject: [PATCH 20/22] add documentation --- src/cargo/util/auth.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/cargo/util/auth.rs b/src/cargo/util/auth.rs index f8a2e27a5..25f912cd4 100644 --- a/src/cargo/util/auth.rs +++ b/src/cargo/util/auth.rs @@ -442,26 +442,44 @@ fn auth_token_optional( Ok(Some(token)) } +/// A record of what kind of operation is happening that we should generate a token for. pub enum Mutation<'a> { + /// Before we generate a crate file for the users attempt to publish, + /// we need to check if we are configured correctly to generate a token. + /// This variant is used to make sure that we can generate a token, + /// to error out early if the token is not configured correctly. PrePublish, + /// The user is attempting to publish a crate. Publish { + /// The name of the crate name: &'a str, + /// The version of the crate vers: &'a str, + /// The checksum of the crate file being uploaded cksum: &'a str, }, + /// The user is attempting to yank a crate. Yank { + /// The name of the crate name: &'a str, + /// The version of the crate vers: &'a str, }, + /// The user is attempting to unyank a crate. Unyank { + /// The name of the crate name: &'a str, + /// The version of the crate vers: &'a str, }, + /// The user is attempting to unyank a crate. Owners { + /// The name of the crate name: &'a str, }, } +/// The main body of an asymmetric token as describe in RFC 3231. #[derive(serde::Serialize)] struct Message<'a> { iat: &'a str, @@ -477,9 +495,11 @@ struct Message<'a> { cksum: Option<&'a str>, #[serde(skip_serializing_if = "Option::is_none")] challenge: Option<&'a str>, + /// This field is not yet used. This field can be set to a value >1 to indicate a breaking change in the token format. #[serde(skip_serializing_if = "Option::is_none")] v: Option, } +/// The footer of an asymmetric token as describe in RFC 3231. #[derive(serde::Serialize)] struct Footer<'a> { url: &'a str, @@ -509,6 +529,7 @@ pub fn login(config: &Config, sid: &SourceId, token: RegistryCredentialConfig) - Ok(()) } +/// Checks that a secret key is valid, and returns the associated public key in Paserk format. pub(crate) fn paserk_public_from_paserk_secret(secret_key: &str) -> Option { let secret: AsymmetricSecretKey = secret_key.try_into().ok()?; let public: AsymmetricPublicKey = (&secret).try_into().ok()?; From d464dbbb5ebe6d7891b304b8763164fa80d31bb0 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Mon, 19 Dec 2022 21:16:04 +0000 Subject: [PATCH 21/22] add tests --- tests/testsuite/registry_auth.rs | 91 ++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/tests/testsuite/registry_auth.rs b/tests/testsuite/registry_auth.rs index 794be9285..440cc9095 100644 --- a/tests/testsuite/registry_auth.rs +++ b/tests/testsuite/registry_auth.rs @@ -134,6 +134,97 @@ fn environment_token_with_asymmetric() { .run(); } +#[cargo_test] +fn warn_both_asymmetric_and_token() { + let _server = RegistryBuilder::new() + .alternative() + .no_configure_token() + .build(); + let p = project() + .file( + ".cargo/config", + r#" + [registries.alternative] + token = "sekrit" + secret-key = "k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36" + "#, + ) + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + description = "foo" + authors = [] + license = "MIT" + homepage = "https://example.com/" + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("publish --no-verify --registry alternative") + .masquerade_as_nightly_cargo(&["credential-process", "sparse-registry", "registry-auth"]) + .arg("-Zsparse-registry") + .arg("-Zregistry-auth") + .with_status(101) + .with_stderr( + "\ +[UPDATING] [..] +[ERROR] both `token` and `secret-key` were specified in the config for registry `alternative`. +Only one of these values may be set, remove one or the other to proceed. +", + ) + .run(); +} + +#[cargo_test] +fn warn_both_asymmetric_and_credential_process() { + let _server = RegistryBuilder::new() + .alternative() + .no_configure_token() + .build(); + let p = project() + .file( + ".cargo/config", + r#" + [registries.alternative] + credential-process = "false" + secret-key = "k3.secret.fNYVuMvBgOlljt9TDohnaYLblghqaHoQquVZwgR6X12cBFHZLFsaU3q7X3k1Zn36" + "#, + ) + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + description = "foo" + authors = [] + license = "MIT" + homepage = "https://example.com/" + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("publish --no-verify --registry alternative") + .masquerade_as_nightly_cargo(&["credential-process", "sparse-registry", "registry-auth"]) + .arg("-Zcredential-process") + .arg("-Zsparse-registry") + .arg("-Zregistry-auth") + .with_status(101) + .with_stderr( + "\ +[UPDATING] [..] +[ERROR] both `credential-process` and `secret-key` were specified in the config for registry `alternative`. +Only one of these values may be set, remove one or the other to proceed. +", + ) + .run(); +} + #[cargo_test] fn bad_environment_token_with_asymmetric_subject() { let registry = RegistryBuilder::new() From b6adac1a6b2786e5beeee56d31732046da5b7ca3 Mon Sep 17 00:00:00 2001 From: Jacob Finkelman Date: Wed, 21 Dec 2022 21:46:18 +0000 Subject: [PATCH 22/22] count calls to credential process --- src/cargo/util/auth.rs | 4 ++-- tests/testsuite/credential_process.rs | 22 ++++++++++++++++++++-- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/cargo/util/auth.rs b/src/cargo/util/auth.rs index 25f912cd4..0235f517a 100644 --- a/src/cargo/util/auth.rs +++ b/src/cargo/util/auth.rs @@ -429,7 +429,7 @@ fn auth_token_optional( } }; - if mutation.is_none() { + if independent_of_endpoint || mutation.is_none() { cache.insert( url.clone(), CredentialCacheValue { @@ -582,7 +582,7 @@ fn run_command( } } // todo: PASETO with process - let independent_of_endpoint = false; + let independent_of_endpoint = true; let action_str = match action { Action::Get => "get", Action::Store(_) => "store", diff --git a/tests/testsuite/credential_process.rs b/tests/testsuite/credential_process.rs index 92a038179..cd1794bda 100644 --- a/tests/testsuite/credential_process.rs +++ b/tests/testsuite/credential_process.rs @@ -2,7 +2,7 @@ use cargo_test_support::registry::{Package, TestRegistry}; use cargo_test_support::{basic_manifest, cargo_process, paths, project, registry, Project}; -use std::fs; +use std::fs::{self, read_to_string}; fn toml_bin(proj: &Project, name: &str) -> String { proj.bin(name).display().to_string().replace('\\', "\\\\") @@ -168,7 +168,22 @@ fn get_token_test() -> (Project, TestRegistry) { let cred_proj = project() .at("cred_proj") .file("Cargo.toml", &basic_manifest("test-cred", "1.0.0")) - .file("src/main.rs", r#"fn main() { println!("sekrit"); } "#) + .file( + "src/main.rs", + r#" + use std::fs::File; + use std::io::Write; + fn main() { + let mut f = File::options() + .write(true) + .create(true) + .append(true) + .open("runs.log") + .unwrap(); + write!(f, "+"); + println!("sekrit"); + } "#, + ) .build(); cred_proj.cargo("build").run(); @@ -219,6 +234,9 @@ fn publish() { ", ) .run(); + + let calls = read_to_string(p.root().join("runs.log")).unwrap().len(); + assert_eq!(calls, 1); } #[cargo_test]