Implement RFC 3139: alternative registry authentication support

[arlosi]

Allows registries to request Cargo to send the authentication token for all requests, rather than just publish/yank, implementing RFC 3139.

Items from the tracking issue

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.

[rust-highfive]

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

[arlosi]

@Eh2406 I updated this PR to address your comments. I've also cleaned up the test framework to combine the two test http servers (index and api) into one, and added additional tests.

[Eh2406]

This is looking good.

Some points for discussion at the next available Cargo meeting:

1. 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.
2. It may be better to just remove CARGO_REGISTRY_NAME_OPT
3. This introduces a new error if two alternative registries are configured with the same index URL.
4. With this change, cargo always reads the credentials file when it loads the other configuration.
5. New dep on https://github.com/scottlamb/http-auth with default=false

Thanks for all the hard work!

[Eh2406]

We were able to discuss this PR briefly at yesterday's meeting. I am trying to recall what we were able to discuss and what we weren't, and try and assign people to follow up.

We did not particularly talk about:

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.

It may be better to just remove CARGO_REGISTRY_NAME_OPT

Some more context: This is an environment variable that is provided to a credential provider when a registry name is known. Given that it's never technically necessary, and not consistently available, should we just remove it?

This introduces a new error if two alternative registries are configured with the same index URL.

Some more context: The RFC authorized us to make this an error. This PR implements the error eagerly. This has the advantage that the moment you corrupt your configuration you will get an error message. And the implementation is pretty straightforward, load all configuration and look for conflicts. It has the disadvantage that ms-configuring registry foo will prevent you from building a project that depends only on crates.io.

@rust-lang/cargo any thoughts on the above questions?

We were able to briefly discuss:

With this change, cargo always reads the credentials file when it loads the other configuration.

Some more context: This is a consequence of the eager evaluation above. We need to load all registry configurations in order to figure out if any of them conflict, to fully load a registry we need to look at credentials to see if there's a token associated with it.

In the meeting: We would like to not read credentials if we don't have to, but are not 100% committed to it. we were wondering what the alternative designs would be here, and what their disadvantages would be? Could we eagerly load config's but lazily load credentials? @arlosi what are your thoughts?

New dep on https://github.com/scottlamb/http-auth with default=false

Some more context: Cargo needs some way for a server to tell it how a user can get a token. It would be nice to follow the www-authenticate standard, but it is a bit of a mess. The PR originally had 200 lines of parsing code, but I was not confident in this being completely correct. We found a dependency that does this parsing and switch to using that.

In the meeting: We would like to do a little more research on alternatives to this dependency. One alternative is to parse ourselves using one of the libraries we already use. (@epage you got the most experience with parsing, does this make any sense?) Alternatively we would like to shop around to see how this crate compares to other crates for this purpose. Specifically, What crates are there? And for each one:

• How actively maintained as it / does the code look up to snuff?
• Is it widely used? Reverse dependencies / download counts.
• Is the API plausible for our use?
• Is the license reasonable for our use?
• Are the dependencies reasonable?

We may decide that the one I picked originally is correct, but someone should document the due diligence. @arlosi what are your thoughts, would you be up to collecting this data? (If not I can redo the work, and this time document it.)

[arlosi]

Could we eagerly load config's but lazily load credentials?

Cargo currently works by eagerly loading credentials for operations that are known to require them (publish, etc). The credentials are then merged into the Config.

This change makes it so the credentials may be required for pretty much anything hitting the network. If we wanted to continue to lazy-load the credentials, we'd probably want to do it in auth::registry_credential_config.

Unfortunately, the Config is not mutable, which makes it difficult to merge additional values into it at that point.

Should we use the http-auth crate to parse the www-authenticate header?

• License is OK (Apache 2, or MIT)
• Reverse dependencies are minimal (only 3 small crate)
• Download count is small (3k)
• Code seems actively maintained and good quality
• Has no new transitive dependencies (with default-features = false)
• We're only using a subset of crate functionality. It can do other things such as respond to Basic and Digest auth (if those features are enabled).

If we're going to use an existing external crate, http-auth seems like the best one. Other crates considered:

• www-authenticate has unsafe mem::transmute and has two new transitive deps (hyperx, unicase). It could work, but we'd need to understand the mem::transmutes.
• authentic uses http-auth
• actix-web-httpauth has dependencies on actix
• http-auth-basic supports only "Basic" auth

Another alternative is the parser I wrote within cargo.

• Less well tested
• Avoids a dependency
• Makes best effort to parse
• 69 lines of code (excluding tests)

This PR currently uses the http-auth crate.

[ehuss]

A mostly cursory review of just a few things I noticed.

[ehuss]

I haven't seen any discussion about what this cargo-protocol header is. Can you say more about it?

[arlosi]

It was a suggestion in the RFC process. It would enable the server to potentially do something different if we ever change how the protocol works. It currently doesn't do anything.

[ehuss]

Can you add a comment to that effect to explain what it is for?

[ehuss]

Thanks so much for working on this, it looks great! I particularly like the updates to the tests.

Some notes:

• I think it would be good to keep the CARGO_REGISTRY_NAME_OPT option, and to use that to store as an informational element in the key store. That way, if a user looks at the key store they can see something that has a little more meaning than just a URL.
• It looks like the 1password credential program is broken. It looks like the search code isn't working correctly, as it is looking at the title, but this was changed to remove the title. I think it needs some kind of title. I'm not sure if the search code can search by URL. Can you make sure that all of the credential programs continue to work?
• -Z registry_auth needs to be documented in unstable.md.
• Can you make sure to update the version of crates-io to 0.35.0, and cargo-credential to 0.2.0?
• I would be much more comfortable if credentials were loaded only as needed. I imagine there are several ways that could be done, but one idea is to have two values tables (the normal Config.values and a new one for credentials). The get_cv function could then consult the credentials one first, and then the normal one. In the common case where the credentials aren't loaded, I don't think it should impact performance noticeably. I'm not sure I fully follow what Config.credential_cache is currently doing, but perhaps it could use that?

[bors]

☔ The latest upstream changes (presumably #10698) made this pull request unmergeable. Please resolve the merge conflicts.

[arlosi]

Thanks for all the feedback. I'll get started on figuring out how to lazy load the credentials file.

For 1password:

• The op tool requires a paid 1password account, which I don't have.
• They have recently released a v2 of the op tool. Do we want to switch to that? I can't find documentation for the v1 protocol. https://developer.1password.com/docs/cli/reference/management-commands/item

I can test the other credential providers.

[ehuss]

Hm, I didn't know there was a new version of the 1password client. It would probably be good to update it, but that can probably be done separately.

The key issue is that there needs to be a way to uniquely find the credential. The search function currently filters based on the title which was the registry name. I think perhaps that can be changed to search for the url, something like:

diff --git a/crates/credential/cargo-credential-1password/src/main.rs b/crates/credential/cargo-credential-1password/src/main.rs
index 86fc9fed8..bd713fd0e 100644
--- a/crates/credential/cargo-credential-1password/src/main.rs
+++ b/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 URL {}, \
                         consider deleting the excess entries",
-                        registry_name
+                        index_url
                     )
                     .into());
                 }
@@ -232,6 +228,8 @@ impl OnePasswordKeychain {
                 "Login",
                 &format!("password={}", token),
                 &format!("url={}", index_url),
+                "--title",
+                "Cargo registry token",
                 "--tags",
                 CARGO_TAG,
             ],

[bors]

📌 Commit 9827412 has been approved by ehuss

It is now in the queue for this repository.

[bors]

⌛ Testing commit 9827412 with merge 4d5c036...

[bors]

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 4d5c036 to master...

[jonathanstrong]

very happy to see this merged! awesome work @arlosi and @ehuss! this change is a critical improvement enabling private registries like Shipyard.rs.

[fasterthanlime]

Can the -Z registry-auth option be passed to cargo as an environment variable? Right now (with cargo 2022-11-21 + shipyard.rs), my rust-analyzer setup is broken because... it invokes cargo metadata without -Z registry-auth (that's one of the rust-analyzer cargo invocations you can't override):

[Eh2406]

Yes, the env is CARGO_UNSTABLE_REGISTRY_AUTH=true

[jonathanstrong]

just noting another option, in ~/.cargo/config.toml:

[unstable]
registry-auth = true

[fasterthanlime]

@Eh2406 @jonathanstrong are either of these options documented somewhere? I've reached the "reading cargo sources" stage of banging my head against the wall here, maybe https://doc.rust-lang.org/cargo/reference/environment-variables.html could point somewhere useful?

edit: just found out this page existed: https://doc.rust-lang.org/cargo/reference/unstable.html

[Eh2406]

Documenting unstable options is always a balancing act. If we knew how they were gonna work forever, it would be easy and helpful to document them but they wouldn't be unstable. If we hide the documentation too much to keep people from getting in over their heads, then it's really hard to find.

I would be open to a sentence or two providing breadcrumbs for people to find the unstable documentation. You're the one who most recently experience the confusion, and therefore are in the best place to know where a sentence would be helpful. PRs are welcome.

-Z registry-auth is actually a great example of this, it is likely to gain meaning when #10771 is merged. Your reputation precedes you, if you're planning to create one of your masterpieces about the improvements to authentication please be mindful that the current state of nightly is likely to change. The plan is for the stabilize version of the feature to require the use of asymmetric tokens for authenticated registries.

[jonathanstrong]

please be mindful that the current state of nightly is likely to change. The plan is for the stabilize version of the feature to require the use of asymmetric tokens for authenticated registries.

is there any place this decision is being discussed/debated? would like to weigh in and I imagine this is not the best venue!

[arlosi]

is there any place this decision is being discussed/debated?

Feel free to start a thread on zulip if you'd like to chat, or create a new issue if there's something specific you'd like changed.

You can also keep adding comments here too.

[dasbard]

Why has this been added? I have "e" and "estuary" pointing to the same registry url and one is just an alias. This change makes it impossible to publish to the registry without either modifying the .cargo config.toml file or switching back to stable.

[ehuss]

@dasbard It's better to file a new issue instead of commenting on a closed PR. I opened #11524 to track it.