Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Several updates to token/index handling. #7973

Merged
merged 5 commits into from
Apr 20, 2020
Merged

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Mar 7, 2020

This attempts to tighten up the usage of token/index handling, to prevent accidental leakage of the crates.io token.

  • Make registry.index config a hard error. This was deprecated 4 years ago in Add support local mirrors of registries, take 2 #2857, and removing it helps simplify things.
  • Don't allow both --index and --registry to be specified at the same time. Otherwise --index was being silently ignored.
  • registry.token is not allowed to be used with the --index flag. The intent here is to avoid possibly leaking a crates.io token to another host.
  • Added a warning if source replacement is used and the token is loaded from registry.token.

Closes #6545

@ehuss
Copy link
Contributor Author

ehuss commented Mar 7, 2020

cc @jtgeibel

I'm a little uncertain about what to do with source replacement. I added a warning, but I'm concerned that there are people using source replacement as a method to completely replace crates.io. If you don't want to use crates.io at all, then source replacement is the best way to do that. Some options:

  • Don't change behavior.
  • Make it a hard error.
  • Make some way to say "it's ok to use registry.token", like a cli flag.
  • Add a new config value for tokens for source replacement.
  • Make some way for alt-registries to work more seamlessly. Like add a registry.use-default-for-dependencies = true config value that would make all dependencies use the registry.default. Then the user would just set the token for that registry. Or maybe a default-registry setting in Cargo.toml.

@jtgeibel
Copy link
Member

jtgeibel commented Mar 7, 2020

Add a new config value for tokens for source replacement.

That is what I had in mind. So you would basically have:

[registry]
token = "private"

[source.crates-io]
registry = "https://github.com/rust-lang/crates.io-index"
replace-with = 'other'

[source.other]
registry = "..."
token = "must-be-provided" # if not provided, should error or warn and send with an empty token

@ehuss
Copy link
Contributor Author

ehuss commented Mar 7, 2020

If we do that, I would want to encourage people to place the token in the credentials file, similar to [registries]. I feel uncomfortable having so many ways to specify a token (it might be a little confusing), but probably isn't too bad?

@alexcrichton
Copy link
Member

Hm I thought tokens were only really ever used for publication/yank/etc, and that I thought those didn't take into account source replacement? I suspect I'm behind the times though!

Could @ehuss or @jtgeibel y'all give me a brief overview of how the current behavior works of where source replacement is taken into account for management of a registry?

@ehuss
Copy link
Contributor Author

ehuss commented Mar 10, 2020

Yes, publishing uses the replaced source. I think that happens around here.

Cargo's own test suite relies on this for the publish tests.

@alexcrichton
Copy link
Member

Ah ok, thanks! This all seems pretty reasonable to me, but I don't have a ton of opinions about how best to do this.

@jtgeibel
Copy link
Member

If we do that, I would want to encourage people to place the token in the credentials file, similar to [registries].

@ehuss sorry I missed your message until now. Adding the token to the credentials file seems reasonable to me, especially from the perspective of matching alternative registries. My key concern is that we should never fall back to the "default" top-level token for source replacement or alternative registries.

@bors
Copy link
Collaborator

bors commented Mar 12, 2020

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

@bors bors added the S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Mar 12, 2020
This has been deprecated for 4 years. This helps simplify this code.
…ime.

Otherwise --index was being silently ignored.
The intent is to avoid leaking the crates.io token to other servers.
@ehuss
Copy link
Contributor Author

ehuss commented Apr 19, 2020

Sorry for the delay. I think I'd like to go with the original plan of this PR to issue a warning. Then, if nobody says anything, we can remove it a few releases later.

@ehuss ehuss added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Apr 19, 2020
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Apr 20, 2020

📌 Commit 65274ea has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 20, 2020
@bors
Copy link
Collaborator

bors commented Apr 20, 2020

⌛ Testing commit 65274ea with merge 9d84c0c...

@bors
Copy link
Collaborator

bors commented Apr 20, 2020

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing 9d84c0c to master...

@bors bors merged commit 9d84c0c into rust-lang:master Apr 20, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 22, 2020
Update cargo, rls

## cargo

17 commits in ebda5065ee8a1e46801380abcbac21a25bc7e755..8751eb3010d4cdb5329b5a6bd2b6d765c95b0dca
2020-04-16 14:28:43 +0000 to 2020-04-21 18:04:35 +0000
- Uplift windows gnu DLL import libraries. (rust-lang/cargo#8141)
- Add windows-gnu CI and fix tests (rust-lang/cargo#8139)
- Several updates to token/index handling. (rust-lang/cargo#7973)
- Add `resolver` opt-in for new feature resolver. (rust-lang/cargo#8129)
- Improve error message when running `cargo install .` (rust-lang/cargo#8137)
- fix mem replace unused (rust-lang/cargo#8138)
- Change `-Cembed-bitcode=no` use to `-Cbitcode-in-rlib=no`. (rust-lang/cargo#8134)
- Refactor BuildContext (rust-lang/cargo#8068)
- Rename allows_underscores to allows_dashes. (rust-lang/cargo#8135)
- Fixed a needless borrow. (rust-lang/cargo#8130)
- Add link to changelog in the Cargo book. (rust-lang/cargo#8126)
- Fix target for doc test cross compilation (rust-lang/cargo#8094)
- Add note about .cargo/config support. (rust-lang/cargo#8125)
- Fix pdb uplift when executable has dashes. (rust-lang/cargo#8123)
- Hint upgrading for future edition keys (rust-lang/cargo#8122)
- Use some fs shorthand functions. (rust-lang/cargo#8124)
- Update documentation to mention "config.toml" instead of "config" (rust-lang/cargo#8121)

## rls

1 commits in 2659cbf14bfb0929a16d7ce9b6858d0bb286ede7..7de2a1f299f8744ffe109139f9f1fdf28bfec909
2020-04-14 22:07:24 +0200 to 2020-04-19 22:41:55 +0000
- Update cargo (rust-lang/rls#1663)
@ehuss ehuss added this to the 1.45.0 milestone Feb 6, 2022
bors added a commit that referenced this pull request Aug 25, 2022
Implement RFC 3289: source replacement ambiguity

### Implements [RFC 3289](rust-lang/rfcs#3289)
* When the crates-io source is replaced, the user needs to specify `--registry <NAME>` when running an API operation to disambiguate which registry to use. Otherwise, cargo will issue a new error.
* In source replacement, the `replace-with` key can reference the name of an alt registry in the `[registries]` table.
* Publishing to source-replaced crates.io is no longer permitted using the crates.io token (`registry.token`). We have had a deprecation warning in place since #7973 (1.45.0).

### Testing
* Tests are updated to add the `--registry dummy-registry` parameter to specify the test registry (otherwise they would get the new error message)
* A few tests that need to verify crates-io-specific configuration use an internal `allow_silent_crates_io_replacement` function to allow the previous behavior of silently replacing crates.io within the testing framework.

Changes are insta-stable.

cc #10894
r? `@Eh2406`
bors added a commit that referenced this pull request Oct 7, 2022
Implement RFC 3289: source replacement ambiguity

### Implements [RFC 3289](rust-lang/rfcs#3289)
* When the crates-io source is replaced, the user needs to specify `--registry <NAME>` when running an API operation to disambiguate which registry to use. Otherwise, cargo will issue a new error.
* In source replacement, the `replace-with` key can reference the name of an alt registry in the `[registries]` table.
* Publishing to source-replaced crates.io is no longer permitted using the crates.io token (`registry.token`). We have had a deprecation warning in place since #7973 (1.45.0).

### Testing
* Tests that interacting with crates.io use the new `replace_crates_io` function, which internally sets an environment variable to change the URL of crates.io.

Changes are insta-stable.

cc #10894
r? `@Eh2406`
bors added a commit that referenced this pull request Oct 7, 2022
Implement RFC 3289: source replacement ambiguity

### Implements [RFC 3289](rust-lang/rfcs#3289)
* When the crates-io source is replaced, the user needs to specify `--registry <NAME>` when running an API operation to disambiguate which registry to use. Otherwise, cargo will issue a new error.
* In source replacement, the `replace-with` key can reference the name of an alt registry in the `[registries]` table.
* Publishing to source-replaced crates.io is no longer permitted using the crates.io token (`registry.token`). We have had a deprecation warning in place since #7973 (1.45.0).

### Testing
* Tests that interacting with crates.io use the new `replace_crates_io` function, which internally sets an environment variable to change the URL of crates.io.

Changes are insta-stable.

cc #10894
r? `@Eh2406`
bors added a commit that referenced this pull request Oct 8, 2022
Implement RFC 3289: source replacement ambiguity

### Implements [RFC 3289](rust-lang/rfcs#3289)
* When the crates-io source is replaced, the user needs to specify `--registry <NAME>` when running an API operation to disambiguate which registry to use. Otherwise, cargo will issue a new error.
* In source replacement, the `replace-with` key can reference the name of an alt registry in the `[registries]` table.
* Publishing to source-replaced crates.io is no longer permitted using the crates.io token (`registry.token`). We have had a deprecation warning in place since #7973 (1.45.0).

### Testing
* Tests that interacting with crates.io use the new `replace_crates_io` function, which internally sets an environment variable to change the URL of crates.io.

Changes are insta-stable.

cc #10894
r? `@Eh2406`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cargo may leak private tokens when overriding the crates.io index
4 participants