rustPlatform.buildRustPackage: build auditable binaries by figsoda · Pull Request #204686 · NixOS/nixpkgs

figsoda · 2022-12-05T21:23:58Z

Description of changes

closes #200563

bootstraps cargo-auditable and makes all packages built by buildRustPackage auditable unless overridden with auditable = false

built fd on master and ran rust-audit-info on it, it should have no sizable impact on binary size

Things done

zowoq · 2022-12-05T21:31:34Z

Any other distros doing this? Besides void?

figsoda · 2022-12-05T21:36:17Z

just void right now as far as I'm aware

zowoq · 2022-12-05T22:00:31Z

#200563
Figure out whether we want to make this the default

TBH I think making it the default is a bit premature. Keep it opt-in for testing and revisit making it the default has been in master for a few weeks?

figsoda · 2022-12-05T22:06:52Z

IMO not making it the default (in the long run) defeats the purpose since it is not in cache, and you would have to build the package first to be able to audit it

maybe we can opt in for a small set of packages and make it the default when it's observed to be stable?

lukegb · 2022-12-05T22:27:00Z

Given that the changes to the hook will cause every package to be rebuilt anyway I don't think there's necessarily much harm in enabling it by default now, but I'm also not familiar enough with the Rust ecosystem to know whether or not just doing this is likely to be harmful in some edgecase.

zowoq · 2022-12-05T22:39:33Z

maybe we can opt in for a small set of packages and make it the default when it's observed to be stable?

^ This is what I meant when I said this:

Keep it opt-in for testing and revisit making it the default has been in master for a few weeks?

Given that the changes to the hook will cause every package to be rebuilt anyway

Don't see how this makes any difference as packages will be rebuilt in the following staging cycles anyway?

I don't think there's necessarily much harm in enabling it by default now, but I'm also not familiar enough with the Rust ecosystem to know whether or not just doing this is likely to be harmful in some edgecase.

Reverting it after it is in master would mean another staging cycle so I think a change that affects every use of buildRustPackage warrants a bit of testing before making it the default?

lukegb · 2022-12-05T22:46:57Z

I think my main complaint is that "a bit of testing" does not a testing plan make :) (I'm not saying you should single-handedly produce a testing plan, this is more of a comment that it's not clear what sort of rollout plan you'd like to see)

I agree that it is probably a good idea to do some form of testing, but also... this is nixos-unstable. The purest form of smoke testing here would be "running all the NixOS tests that exist and seeing if they pass", which is a good bet to happen in staging-next, and if there are any obvious problems there it's easy to back out. I agree if it escapes into master then it's a bit more annoying to revert but at least we'll have evidence of real problems at that point.

What sort of shape of testing would you be interested in to help build confidence in this change?

zowoq · 2022-12-05T22:55:59Z

What sort of shape of testing would you be interested in to help build confidence in this change?

This sounds good:

maybe we can opt in for a small set of packages and make it the default when it's observed to be stable

figsoda · 2022-12-05T23:47:43Z

changed auditable = false to the default and made some of the most popular rust programs auditable, also added jumpy for some diversity

Shnatsel · 2022-12-05T23:51:51Z

I suggest also trying it with librsvg as an example of a dynamic library written in Rust that exposes C API. So that the testing isn't limited to executables.

figsoda · 2022-12-06T00:02:12Z

It's currently not possible with librsvg since it is not using buildRustPackage or cargoBuildHook, I can do it for rav1e though

zowoq · 2022-12-06T00:05:35Z

Maybe mdbook as well as it's in the nix build closure?

zowoq · 2022-12-06T00:23:25Z

pkgs/tools/video/rav1e/default.nix


  postBuild = ''
-    cargo cbuild --release --frozen --prefix=${placeholder "out"} --target ${rustTargetPlatformSpec}
+    $cargoCommand cbuild --release --frozen --prefix=${placeholder "out"} --target ${rustTargetPlatformSpec}


Doesn't need to happen in this PR but any way we could avoid needing to do this? i.e wrap cargo-auditable as cargo or something like that?

How do we stop the wrapper from recursively calling itself? Looks like cargo-auditable just calls cargo directly

https://github.com/rust-secure-code/cargo-auditable/blob/6275e86f503d9419b1528aa38af128882a648ba4/cargo-auditable/src/cargo_auditable.rs#L38

Maybe we can look at CARGO_AUDITABLE_ORIG_ARGS, but I'm not sure if we should rely on that since it is internal and undocumented

Another way to do it is just to add something like a cargoAuditableHook that sets RUSTC_WORKSPACE_WRAPPER

How do we stop the wrapper from recursively calling itself? Looks like cargo-auditable just calls cargo directly

Make cargo into cargo-unwrapped, substitute cargo for cargo-unwrapped in cargo-auditable and then wrap it?

Make cargo into cargo-unwrapped

Sounds like the most future-proof way of doing it, but it would complicate the bootstrap process a bit.

Due to the complexity of this, I think it is worth a separate issue once this is merged

Due to the complexity of this, I think it is worth a separate issue once this is merged

That's fine but I do think it's something that should be resolved before auditable is made the default.

$cargoCommand in an "extra" step like it is here isn't too bad but much easier for everyone if people don't need to know about it.

Should also mean that it works without changes for packages that have cmake, make, meson, etc build systems calling cargo.

Hey! cargo auditable author here 👋

I'll see what I can do about providing a robust way of using cargo auditable as a drop-in replacement for cargo.

Here's the upstream issue so you can track the progress: rust-secure-code/cargo-auditable#89

Using cargo auditable as a drop-in replacement for cargo is now possible on latest git, see here: rust-secure-code/cargo-auditable#89 (comment)

Let me know if it works for you. If so, I'll document it in more detail and ship a release.

@Shnatsel thanks for all the work on cargo-auditable!
It works. I wasn't able to use an alias since cargoBuildHook uses env, which doesn't pick up aliases, but a shell script wrapper does the job

jcgruenhage · 2022-12-06T11:56:02Z

Any other distros doing this? Besides void?

Also not aware of other distros, but we've been building any update to Rust packages that use the cargo build style with auditable, and we have not run into problems with it.

Are there any other distros where adopting this would be easy to implement? I've looked at both Arch and Alpine, and they don't abstract away the build commands into a build style or hook it something like that, meaning that adopting that for all packages is a way bigger change than it would be for Void or NixOS.

Shnatsel · 2022-12-06T14:26:57Z

I've seen some interest from Debian. They have shared build configurations, so it is feasible. But that effort is not very far along yet. See rust-secure-code/cargo-auditable#82 for more info.

Diff: rust-secure-code/cargo-auditable@v0.5.5...246468d

figsoda · 2022-12-07T05:33:58Z

I added cargo-auditable-cargo-wrapper and based buildRustPackage on it. Not sure about the name, also considered cargo-auditable.passthru.cargoWrapper but didn't do that for the simplicity of this initial implementation. open to suggestions

Made libsrvg auditable as per @Shnatsel's suggestion since now it's possible with the wrapper

It should be simple to add auditable support for buildRustCrate as well, but I am not familiar with it so I'll leave it to someone else

Shnatsel · 2022-12-07T21:51:46Z

I've released cargo auditable v0.6.0 with support for using it as a drop-in replacement for cargo, so that you don't have to use a development snapshot.

Shnatsel · 2022-12-08T20:33:49Z

To clarify the potential failure modes: If something goes wrong, the build should fail. It is highly unlikely that a build would produce a non-functional binary, since the data injection mechanism is lifted directly from the Rust compiler, so it's well-tested. And it simply emits an additional object file for the linker to process, so it simply reuses through all the usual, well-trodden codepaths.

figsoda · 2022-12-09T19:53:14Z

@zowoq is this good to merge?

pkgs/development/libraries/librsvg/default.nix

Mindavi · 2023-01-12T13:21:29Z

When auditable is set to true, this causes a regression when cross-compiling rust packages where a cross-compiled rustc and cargo end up in the closure (causing failures since that doesn't work yet):

rick@nixos-asus:~/nixpkgs$ nom build -f default.nix pkgsCross.aarch64-multiplatform.ripgrep -L
these 12 derivations will be built:
  /nix/store/zk6as569vzji8979214qig7y0y26isyr-cargo-1.66.1.drv
  /nix/store/4zqkw80n5lnc9lg4hngqqxnwwnhx3gva-rustc-aarch64-unknown-linux-gnu-1.66.1.drv
  /nix/store/7v21vgng6qjpg4rw96x8nz8b87snv7nm-cargo-check-hook.sh.drv
  /nix/store/jahm46k3xf68zkf15gvjgpmlm2jjrpv2-cargo-build-hook.sh.drv
  /nix/store/9rhmncrcsza15fii2f6644n4mbrhkvbf-cargo-aarch64-unknown-linux-gnu-1.66.1.drv
  /nix/store/hl8g9lskjiqb30sdml2acnn8lja88rqf-cargo-build-hook.sh.drv
  /nix/store/zs37bfg93h0879lglasm7fd96c9v54nm-cargo-check-hook.sh.drv
  /nix/store/30wx8md8q7wlrkqln6ngd6cskrbw55d4-cargo-auditable-aarch64-unknown-linux-gnu-0.6.0.drv
  /nix/store/csz9pipmxk9p9nwcmarn4fz3d4b9vans-cargo.drv
  /nix/store/9cj5wpz3k9lna0vcr2rfs0vwhc31fwpw-cargo-auditable-aarch64-unknown-linux-gnu-0.6.0.drv
  /nix/store/18vrjsy9fsljw6nv0xzpdn083zfzm9dm-cargo.drv
  /nix/store/3h6lcrmxl2scm42bvr34dsdjbg3nw9dn-ripgrep-aarch64-unknown-linux-gnu-13.0.0.drv
┏━ Dependency Graph:
┃             ┌─ ⏳︎ cargo-check-hook.sh waiting for 1 ⏵︎
┃             │     ┌─ ⏳︎ cargo-build-hook.sh waiting for 1 ⏵︎
┃             │     ├─ ⏳︎ cargo-check-hook.sh waiting for 1 ⏵︎
┃             │     │  ┌─ ⏵︎ cargo-1.66.1 (buildPhase) ⏱︎ 15s (∅︎ 6m42s)
┃             │     ├─ ⏳︎ rustc-aarch64-unknown-linux-gnu-1.66.1
┃             │  ┌─ ⏳︎ cargo-aarch64-unknown-linux-gnu-1.66.1
┃             ├─ ⏳︎ cargo-build-hook.sh
┃          ┌─ ⏳︎ cargo-auditable-aarch64-unknown-linux-gnu-0.6.0
┃       ┌─ ⏳︎ cargo
┃    ┌─ ⏳︎ cargo-auditable-aarch64-unknown-linux-gnu-0.6.0
┃ ┌─ ⏳︎ cargo
┃ ⏳︎ ripgrep-aarch64-unknown-linux-gnu-13.0.0
┣━━━ Builds           
┗━ ∑︎ ⏵︎ 1 │ ✔︎ 0 │ ⏳︎ 11 │ ⏱︎ 22s^C

Setting auditable to false will prevent this issue, but since you want to make it default it would be great if you could ensure that this regression is solved before defaulting to it.

figsoda · 2023-01-12T17:26:01Z

#210394

rustPlatform.buildRustPackage: build auditable binaries

8a041c6

figsoda requested review from LnL7, Mic92 and zowoq as code owners December 5, 2022 21:23

github-actions bot added the 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. label Dec 5, 2022

ofborg bot requested a review from retrry December 5, 2022 21:44

figsoda added 6 commits December 5, 2022 18:46

rustPlatform.buildRustPackage: make it not auditable by default

0e5137f

alacritty: make auditable

0024ffa

rust-analyzer: make auditable

6f52f4b

jumpy: make auditable

e4186cc

fd: make auditable

506762f

ripgrep: make auditable

f053bc8

rav1e: make auditable

b632d78

mdbook: make auditable

9e3db33

zowoq reviewed Dec 6, 2022

View reviewed changes

Shnatsel mentioned this pull request Dec 6, 2022

Provide a documented way to use cargo auditable as a drop-in replacement for cargo rust-secure-code/cargo-auditable#89

Closed

Shnatsel mentioned this pull request Dec 6, 2022

to_toml test in auditable-serde writes outside the build directory rust-secure-code/cargo-auditable#82

Closed

figsoda added 3 commits December 7, 2022 00:22

cargo-auditable: 0.5.5 -> unstable-2022-12-07

ac17e99

Diff: rust-secure-code/cargo-auditable@v0.5.5...246468d

cargo-auditable-cargo-wrapper: init

a6137b7

librsvg: make auditable

4ab0618

ofborg bot added the 8.has: package (new) This PR adds a new package label Dec 7, 2022

cargo-auditable: unstable-2022-12-07 -> 0.6.0

0f4a727

zowoq reviewed Dec 9, 2022

View reviewed changes

pkgs/development/libraries/librsvg/default.nix Show resolved Hide resolved

zowoq approved these changes Dec 9, 2022

View reviewed changes

figsoda merged commit cfc6213 into NixOS:staging Dec 9, 2022

figsoda deleted the auditable branch December 9, 2022 21:46

Mindavi mentioned this pull request Jan 12, 2023

staging-next 2023-01-05 #209180

Merged

zhaofengli mentioned this pull request Jan 18, 2023

cargo-auditable-cargo-wrapper: Use writeShellScriptBin instead of writeShellApplication #211468

Merged

11 tasks

figsoda mentioned this pull request Feb 14, 2023

buildRustPackage: publish sbom of rust dependencies #216355

Closed

12 tasks

This was referenced Mar 27, 2023

Support for building auditable rust binaries with cargo-auditable #200563

Closed

rustPlatform.buildRustPackage: make auditable the default #223320

Merged

Uh oh!

Conversation

figsoda commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Things done

Uh oh!

zowoq commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

figsoda commented Dec 5, 2022

Uh oh!

zowoq commented Dec 5, 2022

Uh oh!

figsoda commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukegb commented Dec 5, 2022

Uh oh!

zowoq commented Dec 5, 2022

Uh oh!

lukegb commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zowoq commented Dec 5, 2022

Uh oh!

figsoda commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shnatsel commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

figsoda commented Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zowoq commented Dec 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shnatsel Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

figsoda Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcgruenhage commented Dec 6, 2022

Uh oh!

Shnatsel commented Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

figsoda commented Dec 7, 2022

Uh oh!

Shnatsel commented Dec 7, 2022

Uh oh!

Shnatsel commented Dec 8, 2022

Uh oh!

figsoda commented Dec 9, 2022

Uh oh!

Uh oh!

Mindavi commented Jan 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

figsoda commented Jan 12, 2023

Uh oh!

figsoda commented Dec 5, 2022 •

edited

Loading

zowoq commented Dec 5, 2022 •

edited

Loading

figsoda commented Dec 5, 2022 •

edited

Loading

lukegb commented Dec 5, 2022 •

edited

Loading

figsoda commented Dec 5, 2022 •

edited

Loading

Shnatsel commented Dec 5, 2022 •

edited

Loading

figsoda commented Dec 6, 2022 •

edited

Loading

Shnatsel Dec 6, 2022 •

edited

Loading

figsoda Dec 7, 2022 •

edited

Loading

Shnatsel commented Dec 6, 2022 •

edited

Loading

Mindavi commented Jan 12, 2023 •

edited

Loading