search

rust-secure-code / cargo-auditable

Make production Rust binaries auditable
Apache License 2.0
646 stars 28 forks source link

Provide a documented way to use `cargo auditable` as a drop-in replacement for `cargo` #89

Closed Shnatsel closed 1 year ago

Shnatsel commented 1 year ago

This came up in the pull request about enabling cargo auditable in Nix by defaut.

There are two hurdles here:

  1. cargo auditable calls cargo internally quite a lot. We need to make sure it doesn't recurse.
  2. Some targets are currently not supported, notably WebAssembly, and cargo auditable will error out in this case. This is not an option for a drop-in replacement for cargo - we need to print a warning and keep going.
Shnatsel commented 1 year ago

Cargo sets only one environment variable when calling subcommands, and that's a path to itself in the CARGO env var.

Just following that convention (and letting people set it to the real cargo when using as a drop-in replacement) sounds good. I'm not going to require that variable to be set because I'm not sure if that would break anything (especially exotic things like sccache).

I think if I add another env var for ignoring unsupported platforms, we should be good. So something like this would work, if placed in $PATH and called cargo:

#!/bin/sh

export CARGO='/path/to/real/cargo'
export CARGO_AUDITABLE_IGNORE_UNSUPPORTED=true
cargo-auditable auditable "$@"

btw cargo-auditable auditable is not a typo, that's how Cargo calls subcommands and we actually take advantage of it.

@figsoda @zowoq would this work for Nix?

I could write this as a Rust executable as well, but I think a shell script is way easier to understand and debug - a compiled Rust binary would just be a black box. Transparency is key when you override default behavior.

figsoda commented 1 year ago

Didn't test anything out, but this should work for nix. I think a shell script in nixpkgs is good enough, no rust needed here.

Shnatsel commented 1 year ago

Alright, I'll implement support for these two environment variables then!

Shnatsel commented 1 year ago

As of commit 00030f078a137f6313976bbd341a07faa2e46912 the CARGO variable is implemented, so you can use the above snippet.

The CARGO_AUDITABLE_IGNORE_UNSUPPORTED isn't implemented yet, but shouldn't be a blocker for initial testing.

Shnatsel commented 1 year ago

Support for CARGO_AUDITABLE_IGNORE_UNSUPPORTED has landed in 74a2d3aed04a7932f9ded8a80f085e12a8562a5e

Shnatsel commented 1 year ago

Actually you can just use alias cargo="cargo auditable" without going through all this trouble. I've documented it in the README.

Shnatsel commented 1 year ago

I'm open to feedback on whether continuing the build but printing a warning when targeting an unsupported platform should just be the default.

Maybe a configuration option is overkill, since this behavior is basically never desirable.

figsoda commented 1 year ago

I am not sure about the semver aspect of this, but it (warning instead of error) definitely sounds like a saner default to me

Shnatsel commented 1 year ago

I'll ship it as 0.6 just to be on the safe side