Auto merge of #10592 - arlosi:auth, r=ehuss

Implement RFC 3139: alternative registry authentication support

Allows registries to request Cargo to send the authentication token for all requests, rather than just publish/yank, implementing [RFC 3139](#10474).

### Items from the [tracking issue](#10474)

> Do registries need a more fine-grained switch for which API commands require authentication?

This PR uses the `auth_required` boolean as described in the RFC.

> The RFC mentions adding --token to additional commands like install and search

These flags are not added by this PR.

> Consider changing the name and form of the X- header

Changed to the `www-authenticate` header as suggested by the comments.

> Will there be any concerns with the interaction with rust-lang/rfcs#3231

Not that I know of.

-------------

Adds a new field `"auth-required": true` to `config.json` that indicates Cargo should include the token in all requests to a registry.

For HTTP registries, Cargo first attempts an un-authenticated request, then if that fails with HTTP 401, an authenticated request is attempted. The registry server may include a `www-authenticate` header with the HTTP 401 to instruct Cargo with URL the user can visit to acquire a token (crates.io/me).

Since the API URL is not known (because it's stored in the index), the unstable credential provider feature is modified to key off the index url, and the registry name is no longer provided.

To handle the case where an alternative registry's name is not known (such as coming from a lock file, or via `--index`), Cargo can now look up the token in the configuration by matching on the index URL. This introduces a new error if two alternative registries are configured with the same index URL.

Several operations, such as `cargo install` could have had a `--token` argument added, however it appears that Cargo would like to move away from passing the token on the command line for security reasons. In this case, users would need to configure the registry via the config file (or environment variables) when using `cargo install --index ...` or similar.

Author: bors

CHANGELOG.md

@@ -326,7 +326,7 @@
 - Added `-Zcheck-cfg=output` to support build-scripts declaring their
   supported set of `cfg` values with `cargo:rustc-check-cfg`.
   [#10539](https://github.com/rust-lang/cargo/pull/10539)
-- `-Z http-registry` now uses https://index.crates.io/ when accessing crates-io.
+- `-Z sparse-registry` now uses https://index.crates.io/ when accessing crates-io.
   [#10725](https://github.com/rust-lang/cargo/pull/10725)
 - Fixed formatting of `.workspace` key in `cargo add` for workspace inheritance.
   [#10705](https://github.com/rust-lang/cargo/pull/10705)

Cargo.toml

@@ -33,6 +33,7 @@ git2-curl = "0.16.0"
 glob = "0.3.0"
 hex = "0.4"
 home = "0.5"
+http-auth = { version = "0.1.6", default-features = false }
 humantime = "2.0.0"
 indexmap = "1"
 ignore = "0.4.7"

crates/cargo-test-support/src/registry.rs

@@ -11,7 +11,7 @@ use std::fs::{self, File};
 use std::io::{BufRead, BufReader, Read, Write};
 use std::net::{SocketAddr, TcpListener, TcpStream};
 use std::path::PathBuf;
-use std::thread;
+use std::thread::{self, JoinHandle};
 use tar::{Builder, Header};
 use url::Url;
 
@@ -61,6 +61,8 @@ pub struct RegistryBuilder {
     alternative: Option<String>,
     /// If set, the authorization token for the registry.
     token: Option<String>,
+    /// If set, the registry requires authorization for all operations.
+    auth_required: bool,
     /// If set, serves the index over http.
     http_index: bool,
     /// If set, serves the API over http.
@@ -76,7 +78,7 @@ pub struct RegistryBuilder {
 }
 
 pub struct TestRegistry {
-    _server: Option<HttpServerHandle>,
+    server: Option<HttpServerHandle>,
     index_url: Url,
     path: PathBuf,
     api_url: Url,
@@ -98,6 +100,17 @@ impl TestRegistry {
             .as_deref()
             .expect("registry was not configured with a token")
     }
+
+    /// Shutdown the server thread and wait for it to stop.
+    /// `Drop` automatically stops the server, but this additionally
+    /// waits for the thread to stop.
+    pub fn join(self) {
+        if let Some(mut server) = self.server {
+            server.stop();
+            let handle = server.handle.take().unwrap();
+            handle.join().unwrap();
+        }
+    }
 }
 
 impl RegistryBuilder {
@@ -106,6 +119,7 @@ impl RegistryBuilder {
         RegistryBuilder {
             alternative: None,
             token: None,
+            auth_required: false,
             http_api: false,
             http_index: false,
             api: true,
@@ -160,6 +174,14 @@ impl RegistryBuilder {
         self
     }
 
+    /// Sets this registry to require the authentication token for
+    /// all operations.
+    #[must_use]
+    pub fn auth_required(mut self) -> Self {
+        self.auth_required = true;
+        self
+    }
+
     /// Operate the index over http
     #[must_use]
     pub fn http_index(mut self) -> Self {
@@ -207,6 +229,7 @@ impl RegistryBuilder {
                 registry_path.clone(),
                 dl_path,
                 token.clone(),
+                self.auth_required,
                 self.custom_responders,
             );
             let index_url = if self.http_index {
@@ -226,7 +249,7 @@ impl RegistryBuilder {
         let registry = TestRegistry {
             api_url,
             index_url,
-            _server: server,
+            server,
             dl_url,
             path: registry_path,
             token,
@@ -293,6 +316,11 @@ impl RegistryBuilder {
             }
         }
 
+        let auth = if self.auth_required {
+            r#","auth-required":true"#
+        } else {
+            ""
+        };
         let api = if self.api {
             format!(r#","api":"{}""#, registry.api_url)
         } else {
@@ -302,7 +330,7 @@ impl RegistryBuilder {
         repo(&registry.path)
             .file(
                 "config.json",
-                &format!(r#"{{"dl":"{}"{api}}}"#, registry.dl_url),
+                &format!(r#"{{"dl":"{}"{api}{auth}}}"#, registry.dl_url),
             )
             .build();
         fs::create_dir_all(api_path.join("api/v1/crates")).unwrap();
@@ -442,6 +470,7 @@ pub fn alt_init() -> TestRegistry {
 
 pub struct HttpServerHandle {
     addr: SocketAddr,
+    handle: Option<JoinHandle<()>>,
 }
 
 impl HttpServerHandle {
@@ -456,10 +485,8 @@ impl HttpServerHandle {
     pub fn dl_url(&self) -> Url {
         Url::parse(&format!("http://{}/dl", self.addr.to_string())).unwrap()
     }
-}
 
-impl Drop for HttpServerHandle {
-    fn drop(&mut self) {
+    fn stop(&self) {
         if let Ok(mut stream) = TcpStream::connect(self.addr) {
             // shutdown the server
             let _ = stream.write_all(b"stop");
@@ -468,6 +495,12 @@ impl Drop for HttpServerHandle {
     }
 }
 
+impl Drop for HttpServerHandle {
+    fn drop(&mut self) {
+        self.stop();
+    }
+}
+
 /// Request to the test http server
 #[derive(Clone)]
 pub struct Request {
@@ -504,6 +537,7 @@ pub struct HttpServer {
     registry_path: PathBuf,
     dl_path: PathBuf,
     token: Option<String>,
+    auth_required: bool,
     custom_responders: HashMap<&'static str, Box<dyn Send + Fn(&Request, &HttpServer) -> Response>>,
 }
 
@@ -512,6 +546,7 @@ impl HttpServer {
         registry_path: PathBuf,
         dl_path: PathBuf,
         token: Option<String>,
+        auth_required: bool,
         api_responders: HashMap<
             &'static str,
             Box<dyn Send + Fn(&Request, &HttpServer) -> Response>,
@@ -524,10 +559,11 @@ impl HttpServer {
             registry_path,
             dl_path,
             token,
+            auth_required,
             custom_responders: api_responders,
         };
-        thread::spawn(move || server.start());
-        HttpServerHandle { addr }
+        let handle = Some(thread::spawn(move || server.start()));
+        HttpServerHandle { addr, handle }
     }
 
     fn start(&self) {
@@ -615,7 +651,7 @@ impl HttpServer {
     /// Route the request
     fn route(&self, req: &Request) -> Response {
         let authorized = |mutatation: bool| {
-            if mutatation {
+            if mutatation || self.auth_required {
                 self.token == req.authorization
             } else {
                 assert!(req.authorization.is_none(), "unexpected token");
@@ -676,7 +712,9 @@ impl HttpServer {
     pub fn unauthorized(&self, _req: &Request) -> Response {
         Response {
             code: 401,
-            headers: vec![],
+            headers: vec![
+                r#"WWW-Authenticate: Cargo login_url="https://test-registry-login/me""#.to_string(),
+            ],
             body: b"Unauthorized message from server.".to_vec(),
         }
     }

crates/crates-io/lib.rs

@@ -21,6 +21,8 @@ pub struct Registry {
     token: Option<String>,
     /// Curl handle for issuing requests.
     handle: Easy,
+    /// Whether to include the authorization token with all requests.
+    auth_required: bool,
 }
 
 #[derive(PartialEq, Clone, Copy)]
@@ -199,11 +201,17 @@ impl Registry {
     /// handle.useragent("my_crawler (example.com/info)");
     /// let mut reg = Registry::new_handle(String::from("https://crates.io"), None, handle);
     /// ```
-    pub fn new_handle(host: String, token: Option<String>, handle: Easy) -> Registry {
+    pub fn new_handle(
+        host: String,
+        token: Option<String>,
+        handle: Easy,
+        auth_required: bool,
+    ) -> Registry {
         Registry {
             host,
             token,
             handle,
+            auth_required,
         }
     }
 
@@ -377,7 +385,7 @@ impl Registry {
         headers.append("Accept: application/json")?;
         headers.append("Content-Type: application/json")?;
 
-        if authorized == Auth::Authorized {
+        if self.auth_required || authorized == Auth::Authorized {
             let token = match self.token.as_ref() {
                 Some(s) => s,
                 None => bail!("no upload token found, please run `cargo login`"),

crates/credential/cargo-credential-1password/Cargo.toml

@@ -1,12 +1,12 @@
 [package]
 name = "cargo-credential-1password"
-version = "0.1.0"
+version = "0.2.0"
 edition = "2021"
 license = "MIT OR Apache-2.0"
 repository = "https://github.com/rust-lang/cargo"
 description = "A Cargo credential process that stores tokens in a 1password vault."
 
 [dependencies]
-cargo-credential = { version = "0.1.0", path = "../cargo-credential" }
+cargo-credential = { version = "0.2.0", path = "../cargo-credential" }
 serde = { version = "1.0.117", features = ["derive"] }
 serde_json = "1.0.59"

crates/credential/cargo-credential-1password/src/main.rs

@@ -41,7 +41,7 @@ struct ListItem {
 
 #[derive(Deserialize)]
 struct Overview {
-    title: String,
+    url: String,
 }
 
 impl OnePasswordKeychain {
@@ -175,11 +175,7 @@ impl OnePasswordKeychain {
         Ok(buffer)
     }
 
-    fn search(
-        &self,
-        session: &Option<String>,
-        registry_name: &str,
-    ) -> Result<Option<String>, Error> {
+    fn search(&self, session: &Option<String>, index_url: &str) -> Result<Option<String>, Error> {
         let cmd = self.make_cmd(
             session,
             &[
@@ -196,15 +192,15 @@ impl OnePasswordKeychain {
             .map_err(|e| format!("failed to deserialize JSON from 1password list: {}", e))?;
         let mut matches = items
             .into_iter()
-            .filter(|item| item.overview.title == registry_name);
+            .filter(|item| item.overview.url == index_url);
         match matches.next() {
             Some(login) => {
                 // Should this maybe just sort on `updatedAt` and return the newest one?
                 if matches.next().is_some() {
                     return Err(format!(
-                        "too many 1password logins match registry name {}, \
+                        "too many 1password logins match registry `{}`, \
                         consider deleting the excess entries",
-                        registry_name
+                        index_url
                     )
                     .into());
                 }
@@ -214,7 +210,13 @@ impl OnePasswordKeychain {
         }
     }
 
-    fn modify(&self, session: &Option<String>, uuid: &str, token: &str) -> Result<(), Error> {
+    fn modify(
+        &self,
+        session: &Option<String>,
+        uuid: &str,
+        token: &str,
+        _name: Option<&str>,
+    ) -> Result<(), Error> {
         let cmd = self.make_cmd(
             session,
             &["edit", "item", uuid, &format!("password={}", token)],
@@ -226,20 +228,24 @@ impl OnePasswordKeychain {
     fn create(
         &self,
         session: &Option<String>,
-        registry_name: &str,
-        api_url: &str,
+        index_url: &str,
         token: &str,
+        name: Option<&str>,
     ) -> Result<(), Error> {
+        let title = match name {
+            Some(name) => format!("Cargo registry token for {}", name),
+            None => "Cargo registry token".to_string(),
+        };
         let cmd = self.make_cmd(
             session,
             &[
                 "create",
                 "item",
                 "Login",
                 &format!("password={}", token),
-                &format!("url={}", api_url),
+                &format!("url={}", index_url),
                 "--title",
-                registry_name,
+                &title,
                 "--tags",
                 CARGO_TAG,
             ],
@@ -276,36 +282,36 @@ impl Credential for OnePasswordKeychain {
         env!("CARGO_PKG_NAME")
     }
 
-    fn get(&self, registry_name: &str, _api_url: &str) -> Result<String, Error> {
+    fn get(&self, index_url: &str) -> Result<String, Error> {
         let session = self.signin()?;
-        if let Some(uuid) = self.search(&session, registry_name)? {
+        if let Some(uuid) = self.search(&session, index_url)? {
             self.get_token(&session, &uuid)
         } else {
             return Err(format!(
                 "no 1password entry found for registry `{}`, try `cargo login` to add a token",
-                registry_name
+                index_url
             )
             .into());
         }
     }
 
-    fn store(&self, registry_name: &str, api_url: &str, token: &str) -> Result<(), Error> {
+    fn store(&self, index_url: &str, token: &str, name: Option<&str>) -> Result<(), Error> {
         let session = self.signin()?;
         // Check if an item already exists.
-        if let Some(uuid) = self.search(&session, registry_name)? {
-            self.modify(&session, &uuid, token)
+        if let Some(uuid) = self.search(&session, index_url)? {
+            self.modify(&session, &uuid, token, name)
         } else {
-            self.create(&session, registry_name, api_url, token)
+            self.create(&session, index_url, token, name)
         }
     }
 
-    fn erase(&self, registry_name: &str, _api_url: &str) -> Result<(), Error> {
+    fn erase(&self, index_url: &str) -> Result<(), Error> {
         let session = self.signin()?;
         // Check if an item already exists.
-        if let Some(uuid) = self.search(&session, registry_name)? {
+        if let Some(uuid) = self.search(&session, index_url)? {
             self.delete(&session, &uuid)?;
         } else {
-            eprintln!("not currently logged in to `{}`", registry_name);
+            eprintln!("not currently logged in to `{}`", index_url);
         }
         Ok(())
     }