Move API token into the separate file.

[fa7ca7]

Fix of #3748. BTW, it's not clear what to do with old config.
Should I add a check for old config and try to remove [repository.token] field from it every time user add a new token?
Or should I just prefer to use a token field from a new config over the old one?

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[alexcrichton]

Thanks for the PR! Looking good to me! Some thoughts:

• Could you add some tests for this? Both that the old and the new file works
• I think we may want to not print out the token to the console?
• Could the crates.io documentation get updated?

[alexcrichton]

cc @matklad (just to make you aware)

[matklad]

@alexcrichton

What are your thoughts on

Should I add a check for old config and try to remove [repository.token] field from it every time user add a new token?
Or should I just prefer to use a token field from a new config over the old one?

I think we definitely should remove support for old token location some time in the future, because leaving security-related backwards comparability code paths is not the right thing.

[alexcrichton]

Oh right sorry forgot to mention that. Yeah let's issue a warning if .cargo/config contains a token on cargo publish, and then we'll advertise this change and eventually remove that configuration.

[fa7ca7]

Got it. I'll update the PR by the end of the week

[bors]

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

[fa7ca7]

After diving into the code I found that it's a bit tricky to operate both tokens (from ~/.cargo/config and from ~/.cargo/credentials) due to ConfigValue::merge() implementation. It just skips duplicated values. For example, if we have 'token' key in the both old and new config files the second one will be ignored during the Config::load_values() invocation.

So I propose the next options:

1. Ignore the value from ~/.cargo/config during the merge. Read ~/.cargo/config during the publish in order to check if old configuration exists.
2. Tune ConfigValue::merge() somehow in order to keep both values during the loading.

[alexcrichton]

Perhaps the token could be handled specially? We don't necessarily want to load it from all possible config files, right? Just the home dir?

[fa7ca7]

@alexcrichton thank you for advice and sorry for a long delay. I've updated the code according to your recommendations. I'll introduce tests on the next week.

[alexcrichton]

Thanks @dethoter! One thing I just remembered (sorry for the runaround here) is that let's actually hold off on the warning for now. Users which use Cargo across multiple channels (stable/beta/nightly) would otherwise have no way to not get warnings, so let's wait for this support to hit stable and then we can enable warnings.

[matklad]

I am now reading rust-lang/rfcs#2006 which proposes to add alternative registers to crates.io, which can be super useful in corporate scenarios.

One of the issues there is that when there are multiple registers, you probably want to use different access tokens for them.

I wonder if we can future proof this pull request for that sort of requirements changes. What if we use TOML for credentials, and store them like

[crates.io]
token = "DEADBEEF"

?

That way, we'll be able to add additional keys in the future easily.

[matklad]

cc #3365

[alexcrichton]

Seems reasonable to me!

[fa7ca7]

@alexcrichton no problem. I'll remove the code that warns.

@matklad good proposal! It should be easy to switch file to TOML one. But there are some questions about compatibility.

The old config supports just one registry, so users ask for token like
cfg.get_string("registry.token").

A new one consists from different tokens for different registries. For example I'll store them like:

registry.<host1>.token
registry.<host2>.token

How can I handle a backward compatibility? Should a user now ask for a concrete token like:
cfg.get_string("registry.host1.token")? Or maybe we should have some default token?

[matklad]

How can I handle a backward compatibility?

Cargo as a library is not 1.0, so we can, and do, break backwards compatibility. However I think in this PR we can keep the API exactly the same by returning the first token from the table.

Then, in the PR implementing #3365 we can switch API to cfg.get_string("registry.host1.token").

Or do you think than it'd be easier to add multiple hosts from the start here? I am not super familiar with implementation here :)

[fa7ca7]

However I think in this PR we can keep the API exactly the same by returning the first token from the table.

I believe it's the best option for now. I'll submit an update in a few days.

[bors]

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

[fa7ca7]

During the implementation, I decided that it'd be better to return the old token if cargo couldn't find a token for a host. Returning the first from the map is actually a bad idea because it's associated with another host. If you have any better idea I'll be happy to discuss it.

[matklad]

Let's stop on this approach?

👍

[alexcrichton]

Ok but to be clear we don't even know what the "url of a registry" is today. The url we currently use in lots of places, the index, seems clearly inferior to the actual url like "https://crates.io".

What I'm worried about is that trying to preemptively support multiple registries when we don't actually know what it will look like will drive us into a corner which is very difficult to support in the future. Let's say we encode the index url today, but we'd rather "https://crates.io" tomorrow? We now have to support both urls! What if we decide to use bare identifiers for all registries instead of urls?

I find token = "..." to be the least invasive in terms of forwards compatibility. Issues like #3365 are good for the long run but there's not really much use in fixing it one-off in a PR like this? If we fix it we're not really any closer to supporting custom registries...

[matklad]

Ok but to be clear we don't even know what the "url of a registry" is today. The url we currently use in lots of places, the index, seems clearly inferior to the actual url like "https://crates.io".

Hm, an excellent point! So yeah, it makes sense to support a single token only in this PR, and implement multiple registers support later. Fixing #3365 is not really important now, because it affects only crates.io maintainers.

Aslo, credentials file is not really meant to be edited/viewed by humans, so we can use whatever format we like, as long as it is extensible.

Unrelated, but we might want to change token location in the docs over here: https://github.com/rust-lang/cargo/blob/master/src/doc/crates-io.md#before-your-first-publish.

[fa7ca7]

@alexcrichton you're right, I'll remove support for multiple hosts from this PR.

Unrelated, but we might want to change token location in the docs over here: https://github.com/rust-lang/cargo/blob/master/src/doc/crates-io.md#before-your-first-publish.

@matklad thank you for advice, it's already done in 7c515a0.

[fa7ca7]

@alexcrichton @matklad seems like appveyor build is failed due to nmake issue. Could you please restart the build or point me how can I fix it?

[alexcrichton]

Ah no worries that should hopefully be fixed by the time this reaches the head of the queue. Thanks again @dethoter!

@bors: r+

[bors]

📌 Commit 1ccdc8b has been approved by alexcrichton

[bors]

⌛ Testing commit 1ccdc8b with merge 1716d0f...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 1716d0f to master...

[fa7ca7]

@alexcrichton @matklad thank you for supporting.

[jan-hudec]

@dethoter, the cargo::util::config::Config::load_config method looks for .cargo/config in current working directory and all its parents. Why does the load_credentials method not do the same except looking for .cargo/credentials?

[fa7ca7]

Because .cargo/credentials is global, but .cargo/config could be both global and local.

[jan-hudec]

@dethoter, but why can't credentials be local too? If the local .cargo/config overrides registry index/host, the corresponding credentials need to be stored somewhere. And local .cargo/credentials is the most logical (and without named registries only) place for it.

[fa7ca7]

AFAIK cargo will not support unnamed registries. So there is no reason to handle credentials per project. $CARGO_HOME/credentials is the only place to store tokens/passwords for all the registries. Am I right, @alexcrichton @carols10cents?

[alexcrichton]

The precise design for multiple registries is still up in the air, but I think it's reasonable to consider loading any .cargo/credentials a possibility. The format will probably get tweaked (extended) slightly to support custom registries as well.