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

Rework Toolchain model and drop relative file path overrides #3340

Merged
merged 1 commit into from
May 15, 2023

Conversation

rbtcollins
Copy link
Contributor

@rbtcollins rbtcollins commented Apr 28, 2023

Rework Toolchain model and drop relative file path overrides

Relative path overrides permit a freshly downloaded source tree to
execute arbitrary code on any rustup command that executes a binary from
the configured toolchain, and its a reasonable tradeoff for us to remove
this feature. Absolute path overrides are kept intact - these were added
to support users of large monorepo tool systems, and can be kept with
reasonable safety.

Introduce an interior model for toolchain names which allows the
elimination of conditional code for custom vs named vs path based
toolchains. Fixes #3130. Close #3174

Finishes the separation of Toolchain vs DistributableToolchain vs
CustomToolchain started some time ago.

Moves some test support into the src tree in order to keep symbols
private and adds a 'test' feature required for self-testing, ensuring
that that code doesn't affect production builds.

Adds in a new build that tests the combination of features together (on just one platform), to ensure all combinations are valid.

@rbtcollins rbtcollins changed the title DRAFT: Drop relative path toolchains Rework Toolchain model and drop relative file path overrides May 7, 2023
@rbtcollins rbtcollins marked this pull request as ready for review May 7, 2023 02:52
@rbtcollins rbtcollins force-pushed the wip branch 14 times, most recently from dfc8730 to d2e42c0 Compare May 10, 2023 20:41
Copy link
Member

@hi-rustin hi-rustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have noticed a few minor issues.

Looks great and much clearer. Thank you!

src/cli/common.rs Show resolved Hide resolved
src/toolchain/names.rs Outdated Show resolved Hide resolved
src/toolchain/names.rs Outdated Show resolved Hide resolved
Relative path overrides permit a freshly downloaded source tree to
execute arbitrary code on any rustup command that executes a binary from
the configured toolchain, and its a reasonable tradeoff for us to remove
this feature. Absolute path overrides are kept intact - these were added
to support users of large monorepo tool systems, and can be kept with
reasonable safety.

Introduce an interior model for toolchain names which allows the
elimination of conditional code for custom vs named vs path based
toolchains. Fixes rust-lang#3130.

Finishes the separation of Toolchain vs DistributableToolchain vs
CustomToolchain started some time ago.

Moves some test support into the src tree in order to keep symbols
private and adds a 'test' feature required for self-testing, ensuring
that that code doesn't affect production builds.

Changes the CI debug builds to use the new test feature and also otel,
so that otel doesn't bitrot.
@rbtcollins rbtcollins merged commit 86ed53c into rust-lang:master May 15, 2023
13 of 15 checks passed
@rbtcollins rbtcollins deleted the wip branch May 15, 2023 17:41
@matklad
Copy link
Member

matklad commented May 29, 2023

Relative path overrides permit a freshly downloaded source tree to
execute arbitrary code on any rustup command that executes a binary from
the configured toolchain, and its a reasonable tradeoff for us to remove
this feature. Absolute path overrides are kept intact - these were added
to support users of large monorepo tool systems, and can be kept with
reasonable safety.

I think relative paths are a subset of absolute paths, so, security-wise, this is a no-op.

Namely, ./foo/bar relative path is equivalent to /proc/self/cwd/foo/bar absolute path.

@matklad
Copy link
Member

matklad commented May 29, 2023

Verified with

[toolchain]
# path = ".cargo"
path = "/proc/self/cwd/.cargo"

on

https://github.com/jonas-schievink/mallory

@jonhoo
Copy link
Contributor

jonhoo commented Jul 30, 2023

That's definitely unfortunate. I think removing path-based toolchains altogether is going to break things, especially for more corporate users with additional build orchestration around Cargo (e.g., Amazon). How feasible is it to disallow specifically /proc (and maybe also /sys)? Do we know of other absolute-to-relative proxies like this?

@matklad
Copy link
Member

matklad commented Aug 21, 2023

@rbtcollins is the issue that absolute paths are also unsafe tracked anywhere?

@rbtcollins
Copy link
Contributor Author

rbtcollins commented Aug 21, 2023

Not yet, please do open a bug from this, and we'll have to discuss whether to give up, filter some absolute paths and hope that's sufficient, or some other thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustup allows rustup toolchain link none
4 participants