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

to_toml test in auditable-serde writes outside the build directory #82

Closed
alexanderkjall opened this issue Nov 11, 2022 · 22 comments
Closed
Labels
third party Work item for a third-party dependency

Comments

@alexanderkjall
Copy link
Contributor

Hi

I'm investigating if this could simplify the tracking of relationship between rust binaries and vulnerable versions of rust libraries in Debian, and as part of that investigation I started by seeing how easy it would be to package this with the Debian tooling.

I noticed that the to_toml test in auditable-serde fails due to that it tries to write a lock file into Debians crate registry directory and became a bit afraid that the project might be dead in the water due to that it needs to write those lock files during normal operations also, and not just for that unit test.

I realize that you don't control the cargo_metadata crate, but I thought that it might be worth asking here if this is a use case for auditable that you are interested in supporting?

Complete test output below for context:

---- tests::to_toml stdout ----
thread 'tests::to_toml' panicked at 'called `Result::unwrap()` on an `Err` value: CargoMetadata { stderr: "error: failed to write /usr/share/cargo/registry/auditable-serde-0.5.2/Cargo.lock\n\nCaused by:\n  failed to open: /usr/share/cargo/registry/auditable-serde-0.5.2/Cargo.lock\n\nCaused by:\n  Permission denied (os error 13)\n" }', src/lib.rs:506:20
stack backtrace:
   0: rust_begin_unwind
             at /usr/src/rustc-1.62.1/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /usr/src/rustc-1.62.1/library/core/src/panicking.rs:142:14
   2: core::result::unwrap_failed
             at /usr/src/rustc-1.62.1/library/core/src/result.rs:1785:5
   3: core::result::Result<T,E>::unwrap
             at /usr/src/rustc-1.62.1/library/core/src/result.rs:1078:23
   4: auditable_serde::tests::load_own_metadata
             at ./src/lib.rs:506:9
   5: auditable_serde::tests::to_toml
             at ./src/lib.rs:513:24
   6: auditable_serde::tests::to_toml::{{closure}}
             at ./src/lib.rs:512:5
   7: core::ops::function::FnOnce::call_once
             at /usr/src/rustc-1.62.1/library/core/src/ops/function.rs:248:5
   8: core::ops::function::FnOnce::call_once
             at /usr/src/rustc-1.62.1/library/core/src/ops/function.rs:248:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
@Shnatsel
Copy link
Member

Hello Alexander, I'm very great to hear that there is interest from Debian! Yes, that's a use case I am keen to support. FYI Void Linux already uses cargo-auditable that way: void-linux/void-packages#40272

I assume Debian is building packages cargo build --frozen and a local, Debian-supplied copy of the registry? cargo auditable build --frozen should work just as well, but the test isn't using the --frozen flag which may be causing issues (or there could be many other things wrong with the test).

The important part is not whether the to_toml test can run (I can see some additional issues around that) but whether cargo auditable build works for packages, no matter how cargo auditable was actually installed. I suggest ignoring the test for now; we can fix it once the core process works.

Is there a way I can reproduce the issue? It's probably going to be easiest for me to debug it myself than to suggest various changes to you. FWIW I used to be a Debian maintainer years ago, but I've never packaged Rust code, so Rust packaging guides might come in handy.

@Shnatsel
Copy link
Member

#83 may be relevant here but I'm not sure if it is a blocker. I'm looking forward to the reproduction steps so that I could investigate in more detail.

@alexanderkjall
Copy link
Contributor Author

"interest from Debian" is maybe a bit of a stretch, I package some stuff for debian but isn't a core contributor, and since I work with security I think this would both make a good research topic for my 10% time at work, and improve the debian packaging of rust. Current state is that I will try to build a poc and propose that to the rest of the debian-rust team and see what they think about the whole concept.

I can push a WIP-branch as soon as I have it a bit cleaned-up.

Regarding on how debian works: Debian packages each crate as debian package that drops some source code into /usr/share/cargo/registry/. And then stuff build-depends on those packages, and cargo points to that when building the binaries.

Thanks for the pointer to void-linux, will look through that, and I agree that the unit test failing is in itself a non-issue, I was mostly concerned with if it represented a larger issue that was hard to solve :)

@Shnatsel
Copy link
Member

I was mostly concerned with if it represented a larger issue that was hard to solve :)

Yeah, it very well might. I think you're hitting rust-lang/cargo#10096. Passing --offline --frozen might help once the fix for #83 is merged. I'll need a way to reproduce this locally to investigate further.

@alexanderkjall
Copy link
Contributor Author

I'm not fully done, but I'm running out of time for today, and I have something that be workable enough to reproduce/test with, the packaging work is pushed to this branch: https://salsa.debian.org/rust-team/debcargo-conf/-/tree/package-cargo-auditable

Setting up a packaging environment for rust on Debian might be a bit complex, I would recommend to run debian sid in a virtual machine, and this guide that i wrote a long time ago might provide some hints: https://blog.hackeriet.no/packaging-a-rust-project-for-debian/ together with the readme in https://salsa.debian.org/rust-team/debcargo-conf/

Since that branch contains work on multiple packages you need to pack/build them in order so that they can be built in order.

I think the below list of commands run inside the debcargo-conf/ checkout on that branch will yield a cargo-auditable_0.5.2-1_amd64.deb file that can be installed on debian sid (not fully tested due to time constraints).

./repackage.sh enumset-derive
./repackage.sh serde-derive-internals
./repackage.sh schemars-derive
./repackage.sh schemars
./repackage.sh topological-sort
./repackage.sh auditable-serde
./repackage.sh cargo-metadata
./repackage.sh cargo-auditable
cd build
./build enumset-derive
./build serde-derive-internals *deb
./build schemars-derive *deb
./build schemars *deb
./build topological-sort *deb
./build auditable-serde *deb
./build cargo-metadata *deb
./build cargo-auditable *deb

This is maybe a bit too complex, feel free to contact me on OFTC where I go by the nick capitol, or drop me an email if there is any questions that are too off topic to take here.

@Shnatsel
Copy link
Member

Thanks for the links! That looks helpful.

Is there a way for me to take an unrelated package, smuggle cargo-auditable binary into the chroot build environment, and configure it to use cargo auditable $ARGS instead of cargo $ARGS? Years ago I'd do this through Debhelper overrides, but I see no Debhelper files anywhere in src/.

I need to experiment with actual builds under cargo auditable to see if they're problematic or not, and right now I don't see a way to do that.

@Shnatsel
Copy link
Member

Also, I've released cargo-auditable v0.5.3 that fixes #83.

@Shnatsel Shnatsel added the third party Work item for a third-party dependency label Nov 13, 2022
@Shnatsel
Copy link
Member

Shnatsel commented Nov 13, 2022

TL;DR: disable the to_toml test, it's not worth the trouble. Use cargo-auditable v0.5.3 or later, and building other packages with it should work. Please reach out to me if it doesn't.


Okay, so I've installed Debian Sid and poked around a bit. I've found that yes, indeed, we're hitting rust-lang/cargo#10096.

I don't really see how to make it work given the error it throws - this is a long-standing issue in cargo, and I don't see an easy workaround. I suggest you simply run the tests without the toml feature, or ignore that test specifically. Feel free to quote me on that.

The good news is that I think this should not be a blocker for building other packages with cargo auditable. Cargo will refuse to build anything if it cannot update the Cargo.lock file, so the Cargo.lock must already be in place by the time rustc is invoked, which is when a cargo auditable hook calls cargo metadata which needs the lockfile, so it seems building other packages with it should work. It's possible that cargo metadata will try to update the Cargo.lock file and fail, but that is easy to fix by passing --offline --frozen.

@Shnatsel Shnatsel closed this as not planned Won't fix, can't repro, duplicate, stale Nov 13, 2022
@alexanderkjall
Copy link
Contributor Author

Thanks a lot of all the time you spent, I'll make sure to report back the result of my attempt on using it :)

@Shnatsel
Copy link
Member

Shnatsel commented Dec 5, 2022

Just curious - are there any blockers for this work? Anything I can help with?

@alexanderkjall
Copy link
Contributor Author

@Shnatsel no, only blocker is that I'm doing this as a 10% project at work, so I haven't had that much time to look at it sadly

@Shnatsel
Copy link
Member

Shnatsel commented Dec 6, 2022

Alright. No worries!

@Shnatsel
Copy link
Member

Shnatsel commented Dec 6, 2022

NixOS is also rolling out builds with cargo auditable: NixOS/nixpkgs#204686

This might help your case for enabling this in Debian.

@alexanderkjall
Copy link
Contributor Author

I finished the initial packaging effort to get the cargo-auditable binary packaged (but not used yet).

One thing I needed to do was to upgrade the object dependency to version 0.30, as Debian try to not package multiple versions of crates.

It would be interesting to know if you have any input on this patch: https://salsa.debian.org/rust-team/debcargo-conf/-/blob/master/src/cargo-auditable/debian/patches/tweak-deps.patch

The thing that I'm most unsure about is hard coding EI_ABIVERSION to 0, but the elf manual said: Applications conforming to this specification use the value 0.

@Shnatsel
Copy link
Member

Hmmm. Wikipedia says that if the OS ABI is set to 3 aka object::elf::ELFOSABI_LINUX, the EI_ABIVERSION field is significant.

Really we want to be doing whatever object 0.28 was doing for ELF. I'll look up the history for object and try to see what they changed.

@Shnatsel
Copy link
Member

These seem to be the two PRs adding the extra parameters:

gimli-rs/object#438

gimli-rs/object#441

@bjorn3
Copy link

bjorn3 commented Feb 25, 2023

Most Linux executables use ELFOSABI_NONE, not ELFOSABI_LINUX. This is also what rustc itself uses in it's usage of the object crate. Only hermit, freebsd and solaris use a different OS ABI: https://github.com/rust-lang/rust/blob/dcca6a375bd4eddb3deea7038ebf29d02af53b48/compiler/rustc_codegen_ssa/src/back/metadata.rs#L196-L204

@Shnatsel
Copy link
Member

Oh, thanks a lot for the pointer! I'll just sync our local version of this file with the upstream, then.

@alexanderkjall do you want me to make a new release, or carrying a patch that we also have in git master sounds preferable?

@Shnatsel
Copy link
Member

I just synced to the latest code from rustc. Here's the diff: #104

Do you want me to cut a stable release with that?

@alexanderkjall
Copy link
Contributor Author

Thanks a lot for the review and quick work. I really appreciate not having my own guesses around these things distributed in Debian, as I'm not that experienced with it.

I didn't manage to get this work done before the Debian freeze for the next release happened, and my guess is that we will have 1-2 more months of that. So there really isn't any need to do a release before the freeze ends.

@nickbroon
Copy link

@alexanderkjall your work to add cargo-auditable to Debian sounds interesting. Is your plan to integrate it into dh-cargo or perhaps the Debian cargo wrapper so that Debian rust binary packages contain this audit info?

@alexanderkjall
Copy link
Contributor Author

@nickbroon That is my long term plan, I haven't actually done any of that work yet, so I'm not sure how it will work out.

I guess that I need to have it behind a feature toggle in debcargo.toml, so that we can avoid having a package dependency loop. Without that cargo-auditable would need cargo-auditable to build itself, and that wouldn't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
third party Work item for a third-party dependency
Projects
None yet
Development

No branches or pull requests

4 participants