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

cargo cmd preferefs cargo-cmd bin from it's home directory which makes sandboxing difficult #11020

Closed
dpc opened this issue Aug 24, 2022 · 11 comments · Fixed by #11023
Closed
Labels
A-custom-subcommands Area: custom 3rd party subcommand plugins C-bug Category: bug

Comments

@dpc
Copy link
Contributor

dpc commented Aug 24, 2022

Problem

We are using Nix (nix develop) to prepare a reproducible build environment for the Rust codebase. Some users report that cargo clippy fails due to incompatible version of Rust compiler(?), while it works for other users as the cargo-clippy is reproducibly available in the $PATH.

After some debugging it turns out that users that have cargo-clippy installed in $HOME/.cargo/bin are affected because cargo prefers such binaries over those in $PATH, breaking out of the "sandbox".

Steps

N/A

Possible Solution(s)

  • Change the order of variables in
    let mut dirs = vec![config.home().clone().into_path_unlocked().join("bin")];
    so that Rust home binaries are tried last.

Notes

No response

Version

No response

@dpc dpc added the C-bug Category: bug label Aug 24, 2022
@epage
Copy link
Contributor

epage commented Aug 24, 2022

I'm assuming the reason to prefer home over PATH is if a plugin is installed globally, this allows a user to locally override it.

So how do we reconcile these care abouts?

One idea might be to isolate the cargo homes.

@dpc
Copy link
Contributor Author

dpc commented Aug 24, 2022

I'm assuming the reason to prefer home over PATH is if a plugin is installed globally, this allows a user to locally override it.

That's a good point.

One idea might be to isolate the cargo homes.

Can you explain please?

Some kind of environment variable that switch the preference would work for my use case as well. We generally don't want to disable user's preferred local tooling, just not have get it in the way of reproducibly provided standard ones like cargo-clippy.

@epage
Copy link
Contributor

epage commented Aug 24, 2022

You can override CARGO_HOME

@dpc
Copy link
Contributor Author

dpc commented Aug 24, 2022

You can override CARGO_HOME

But then the user will loose access to any other bins they might have? Actually kind of yes/no, because they can still have them in $PATH. But there might some settings(?) etc. that would stop working maybe? Yeah... the registry indexes and caches etc.? Not the end of the world, I guess, but not exactly desirable if can be avoided.

I'm assuming the reason to prefer home over PATH is if a plugin is installed globally, this allows a user to locally override it.

BTW. If user wanted to override the globally accessible bins, shouldn't they just add $CARGO_HOME/bin to $PATH before the global ones like for anything other software?

I guess the problem comes down to $CARGO_HOME/bin being treated specially in the first place. For any other binaries that are not cargo-<cmd>, to user will need to modify $PATH one way or the other, no?

It seems to me that just not making $CARGO_HOME/bin special and having users add them to $PATH would be the best solution, but might be a backward-compatibility issue.

Maybe just disabling search of $CARGO_HOME/bin(with a env) would be OK for reproducible environments like nix develop.

@weihanglo
Copy link
Member

Out of curious, what kind/level of sandboxing users are working with? From my understanding, reusing CARGO_HOME reuses some other stuff like registry index and cache. Is that what users want?

@dpc
Copy link
Contributor Author

dpc commented Aug 24, 2022

Out of curious, what kind/level of sandboxing users are working with? From my understanding, reusing CARGO_HOME reuses some other stuff like registry index and cache. Is that what users want?

nix develop by default provides a rather casual dev env sandboxing. It comes down to getting env vars sanitized and $PATH populated with the software in expected versions. The host-level stuff is still visible and usable. Only during actual nix build etc. Nix enforces strong sandboxing for reproducibility.

@epage
Copy link
Contributor

epage commented Aug 25, 2022

Maybe another way to frame the problem is that .cargo/ mixes concerns

  • Cross project, cross rustc content
    • This should be accessible within your sandbox
    • This should have precedence higher than system paths by default
    • Should this be higher or lower than a project-specific toolchain (e.g. a project flake)? I can see it going either way (project stuff always wins for reproducibility but user wins for customizing the workflow)
  • rustup specific content
    • This should only be accessed by the same rustup toolchain

If we could separate these concerns, it would take care of most of your problems. That would just leave an open question about whether user or project content wins which has a ,much smaller scope / impact.

@dpc
Copy link
Contributor Author

dpc commented Aug 25, 2022

The way I think about is that PATH=some/dir/a:some/other/dir/b:dir/c leaves to the use decision on the order preference. Since $HOME/.cargo/bin is implicit and hardcoded it can't be composed in this ordering in relation to other paths. It can go only after or before all elements of the $PATH.

One approach would be for cargo to look for binaries in $HOME/.cargo/bin only if $HOME/.cargo/bin is not already in the $PATH somewhere. This way users that did put it in the $PATH would have control about the order, while the users that didn't would keep current behavior.

@epage
Copy link
Contributor

epage commented Aug 25, 2022

One approach would be for cargo to look for binaries in $HOME/.cargo/bin only if $HOME/.cargo/bin is not already in the $PATH somewhere. This way users that did put it in the $PATH would have control about the order, while the users that didn't would keep current behavior.

I think that is a reasonable compromise. In theory, a user could observe the change in behavior but the likelihood is small and probably low impact.

dpc added a commit to dpc/cargo that referenced this issue Aug 25, 2022
This is to allow users to control the order via PATH if they so
desire.

Tested by preparing two different `cargo-foo` scripts in
`$HOME/.cargo/bin` and `$HOME/bin`.

```
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin" ./target/debug/cargo foo
Inside ~/bin/
> env PATH="$HOME/.cargo/bin:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
```

Fix rust-lang#11020
dpc added a commit to dpc/cargo that referenced this issue Aug 25, 2022
This is to allow users to control the order via PATH if they so desire.

Tested by preparing two different `cargo-foo` scripts in `$HOME/.cargo/bin` and `$HOME/bin`.

```
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin" ./target/debug/cargo foo
Inside ~/bin/
> env PATH="$HOME/.cargo/bin:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
```

Fix rust-lang#11020
dpc added a commit to dpc/cargo that referenced this issue Aug 25, 2022
This is to allow users to control the order via PATH if they so desire.

Tested by preparing two different `cargo-foo` scripts in `$HOME/.cargo/bin` and `$HOME/bin`:

```
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin" ./target/debug/cargo foo
Inside ~/bin/
> env PATH="$HOME/.cargo/bin:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
```

and trailing slash:

```
> env PATH="$HOME/.cargo/bin/:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin/" ./target/debug/cargo foo
Inside ~/bin/
```

Fix rust-lang#11020
dpc added a commit to dpc/cargo that referenced this issue Aug 25, 2022
This is to allow users to control the order via PATH if they so desire.

Tested by preparing two different `cargo-foo` scripts in `$HOME/.cargo/bin` and `$HOME/bin`:

```
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin" ./target/debug/cargo foo
Inside ~/bin/
> env PATH="$HOME/.cargo/bin:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
```

and trailing slash:

```
> env PATH="$HOME/.cargo/bin/:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin/" ./target/debug/cargo foo
Inside ~/bin/
```

Fix rust-lang#11020
@dpc
Copy link
Contributor Author

dpc commented Aug 25, 2022

I think that is a reasonable compromise. In theory, a user could observe the change in behavior but the likelihood is small and probably low impact.

Created a PR.

@weihanglo weihanglo added the A-custom-subcommands Area: custom 3rd party subcommand plugins label Aug 26, 2022
dpc added a commit to dpc/cargo that referenced this issue Aug 28, 2022
This is to allow users to control the order via PATH if they so desire.

Tested by preparing two different `cargo-foo` scripts in `$HOME/.cargo/bin` and `$HOME/bin`:

```
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin" ./target/debug/cargo foo
Inside ~/bin/
> env PATH="$HOME/.cargo/bin:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
```

and trailing slash:

```
> env PATH="$HOME/.cargo/bin/:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin/" ./target/debug/cargo foo
Inside ~/bin/
```

Fix rust-lang#11020
dpc added a commit to dpc/cargo that referenced this issue Aug 28, 2022
This is to allow users to control the order via PATH if they so desire.

Tested by preparing two different `cargo-foo` scripts in `$HOME/.cargo/bin` and `$HOME/bin`:

```
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin" ./target/debug/cargo foo
Inside ~/bin/
> env PATH="$HOME/.cargo/bin:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
```

and trailing slash:

```
> env PATH="$HOME/.cargo/bin/:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin/" ./target/debug/cargo foo
Inside ~/bin/
```

Fix rust-lang#11020
@weihanglo
Copy link
Member

dpc added a commit to dpc/cargo that referenced this issue Aug 31, 2022
This is to allow users to control the order via PATH if they so desire.

Tested by preparing two different `cargo-foo` scripts in `$HOME/.cargo/bin` and `$HOME/bin`:

```
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin" ./target/debug/cargo foo
Inside ~/bin/
> env PATH="$HOME/.cargo/bin:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
```

and trailing slash:

```
> env PATH="$HOME/.cargo/bin/:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin/" ./target/debug/cargo foo
Inside ~/bin/
```

Fix rust-lang#11020
dpc added a commit to dpc/cargo that referenced this issue Aug 31, 2022
This is to allow users to control the order via PATH if they so desire.

Tested by preparing two different `cargo-foo` scripts in `$HOME/.cargo/bin` and `$HOME/bin`:

```
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin" ./target/debug/cargo foo
Inside ~/bin/
> env PATH="$HOME/.cargo/bin:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
```

and trailing slash:

```
> env PATH="$HOME/.cargo/bin/:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin/" ./target/debug/cargo foo
Inside ~/bin/
```

Fix rust-lang#11020
dpc added a commit to dpc/cargo that referenced this issue Aug 31, 2022
This is to allow users to control the order via PATH if they so desire.

Tested by preparing two different `cargo-foo` scripts in `$HOME/.cargo/bin` and `$HOME/bin`:

```
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin" ./target/debug/cargo foo
Inside ~/bin/
> env PATH="$HOME/.cargo/bin:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
```

and trailing slash:

```
> env PATH="$HOME/.cargo/bin/:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin/" ./target/debug/cargo foo
Inside ~/bin/
```

Fix rust-lang#11020
dpc added a commit to dpc/cargo that referenced this issue Aug 31, 2022
This is to allow users to control the order via PATH if they so desire.

Tested by preparing two different `cargo-foo` scripts in `$HOME/.cargo/bin` and `$HOME/bin`:

```
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin" ./target/debug/cargo foo
Inside ~/bin/
> env PATH="$HOME/.cargo/bin:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
```

and trailing slash:

```
> env PATH="$HOME/.cargo/bin/:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin/" ./target/debug/cargo foo
Inside ~/bin/
```

Fix rust-lang#11020
dpc added a commit to dpc/cargo that referenced this issue Aug 31, 2022
This is to allow users to control the order via PATH if they so desire.

Tested by preparing two different `cargo-foo` scripts in `$HOME/.cargo/bin` and `$HOME/bin`:

```
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin" ./target/debug/cargo foo
Inside ~/bin/
> env PATH="$HOME/.cargo/bin:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
```

and trailing slash:

```
> env PATH="$HOME/.cargo/bin/:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin/" ./target/debug/cargo foo
Inside ~/bin/
```

Fix rust-lang#11020
dpc added a commit to dpc/cargo that referenced this issue Sep 7, 2022
This is to allow users to control the order via PATH if they so desire.

Tested by preparing two different `cargo-foo` scripts in `$HOME/.cargo/bin` and `$HOME/bin`:

```
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin" ./target/debug/cargo foo
Inside ~/bin/
> env PATH="$HOME/.cargo/bin:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
```

and trailing slash:

```
> env PATH="$HOME/.cargo/bin/:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin/" ./target/debug/cargo foo
Inside ~/bin/
```

Fix rust-lang#11020
dpc added a commit to dpc/cargo that referenced this issue Sep 7, 2022
This is to allow users to control the order via PATH if they so desire.

Tested by preparing two different `cargo-foo` scripts in `$HOME/.cargo/bin` and `$HOME/bin`:

```
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin" ./target/debug/cargo foo
Inside ~/bin/
> env PATH="$HOME/.cargo/bin:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
```

and trailing slash:

```
> env PATH="$HOME/.cargo/bin/:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin/" ./target/debug/cargo foo
Inside ~/bin/
```

Fix rust-lang#11020
dpc added a commit to dpc/cargo that referenced this issue Sep 7, 2022
This is to allow users to control the order via PATH if they so desire.

Tested by preparing two different `cargo-foo` scripts in `$HOME/.cargo/bin` and `$HOME/bin`:

```
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin" ./target/debug/cargo foo
Inside ~/bin/
> env PATH="$HOME/.cargo/bin:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
```

and trailing slash:

```
> env PATH="$HOME/.cargo/bin/:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin/" ./target/debug/cargo foo
Inside ~/bin/
```

Fix rust-lang#11020
dpc added a commit to dpc/cargo that referenced this issue Sep 7, 2022
This is to allow users to control the order via PATH if they so desire.

Fix rust-lang#11020
bors added a commit that referenced this issue Sep 13, 2022
Do not add home bin path to PATH if it's already there

This is to allow users to control the order via PATH if they so desire.

Tested by preparing two different `cargo-foo` scripts in `$HOME/.cargo/bin` and `$HOME/bin`:

```
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin" ./target/debug/cargo foo
Inside ~/bin/
> env PATH="$HOME/.cargo/bin:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
```

and trailing slash:

```
> env PATH="$HOME/.cargo/bin/:/usr/bin/:$HOME/bin" ./target/debug/cargo foo
Inside ~/.cargo/bin/
> env PATH="/usr/bin/:$HOME/bin:$HOME/.cargo/bin/" ./target/debug/cargo foo
Inside ~/bin/
```

Fix #11020
@bors bors closed this as completed in c712f08 Sep 13, 2022
Hezuikn pushed a commit to Hezuikn/cargo that referenced this issue Sep 22, 2022
This is to allow users to control the order via PATH if they so desire.

Fix rust-lang#11020
Hezuikn pushed a commit to Hezuikn/cargo that referenced this issue Sep 22, 2022
This is to allow users to control the order via PATH if they so desire.

Fix rust-lang#11020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-custom-subcommands Area: custom 3rd party subcommand plugins C-bug Category: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants