Sudo-rs dependencies: when less is better

https://tweedegolf.nl/en/blog/119/sudo-rs-depencencies-when-less-is-better

by marbu • 2 years ago

93 57 2 years ago

awoimbee• 2 years ago

In the end, we chose the potential dangers of reimplementing command line parsing over the potential issues of including clap

Have you considered using argh ? Seems like it has the upsides without the downsides.

telotortium• 2 years ago

Don’t think it’s worth it. Looking at sudo’s man page at https://linux.die.net/man/8/sudo, it looks like sudo only uses single-letter flags, some of which take arguments. Argh implements long options, built-in parsing, subcommands, and lots of other nice to have features that nevertheless add a lot of code. It’s normal in traditional UNIX C programs to parse sudo-style flags in a handful of lines without any external dependencies.

0cf8612b2e1e2• 2 years ago

I consider single letter flags only to be a mistake. There should almost always be a verbose double-dash option.

I get it, most of the tooling which uses single letters is totally ossified due to backwards compatibility reasons. However, the sudors team is already breaking backwards compat. Now is the time to make a minor usability improvement.

scbrg• 2 years ago

That's a bit dated. Both regular sudo (1.9.13p3) and sudo-rs (0.2.2) on my machine (Debian) support double dash style options.

IshKebab• 2 years ago

I've used argh a fair bit. It has some weird ideas and restrictions and generally isn't nearly as good as clap. I would definitely recommend clap (unless you have extreme security concerns like this).

Karellen• 2 years ago

Why not use `getopt()` which already exists in libc?

(Or even `getopt_long()` if you're Linux/glibc-only? Author mentions not supporting Windows, but is unclear whether non-Linux Unices, e.g. *BSD, are intended target platforms.)

https://manpages.debian.org/bookworm/manpages-dev/getopt.3.e...

steveklabnik6• 2 years ago

If you're trying to implement as much in Rust as possible, keeping an important part of the codebase in C code feels like the wrong decision, in my opinion.

photonbucket• 2 years ago

Is there any tooling which can tell you exactly which parts of a crate that you actually use and produce a minimized version for vendoring/auditing?

0cf8612b2e1e2• 2 years ago

I like this idea. Theoretically, the compiler already has the machinery to remove dead code. Next step could package up just the source you touch.

Arnavion2• 2 years ago

You can get that info from code coverage, via `cargo llvm-cov` etc, though that would require exercising all code paths into the deps or else you might underestimate how much of the deps you need to vendor. But at least if you underestimate in this way, you'll probably just get a compiler error rather than anything breaking at runtime.

dathinab3• 2 years ago

it's not trivial to do if you have multiple build targets and features

i.e. you would need to vendor one version for each features x target tripple combination combined with cfg expansion and (proc) macro expansion inlining and then a static reachability analysis to prune all unused code (and dependencies). That would likely not be good enough so you probably need to have some runtime code coverage analysis to find "likely dead code" (but not statically provable dead code) and then manual choices to keep/remove combined with some bisecting/testing to make sure the choices are sane.

Afik such tool doesn't exist.

And it's non trivial.

But it's also very viable to create it.

jcgrillo• 2 years ago

I have been spitballing about this recently too [1]. The way I'd imagine it would work is the toolchain takes one pass over your crate, compiles everything, then takes another pass to trim all the dead code from your vendored deps. Then your git diff basically has your code + all the lines of all your deps that didn't get trimmed.

There would probably need to be some more work to make it more user friendly, but I think it's really important that all the code which ultimately ends up in your binary goes in the diff otherwise reviewers won't actually look at it.

Disclaimer: I don't know enough about compilers, or the Rust toolchain specifically, to know if this is even possible or whether it would actually help anyone in the real world. But it seems "naively reasonable" for some definition.

[1] https://news.ycombinator.com/item?id=39828499

lifthrasiir• 2 years ago

This is commonly called the "tree shaking" [1] which is a particular mode of the general dead code elimination. One of main challenges would be the reproduction of somehow readable source code after the tree shaking.

[1] https://en.wikipedia.org/wiki/Tree_shaking

sebazzz• 2 years ago

If they don’t link libc statically it can become a problem if the system-installed libc is corrupt or incompatible. My Arch install broke once and I wasn’t able to run pacman to correct it, because the libc installed was not compatible with pacman. If sudo wouldn’t run, I would not even have a chance to repair the install without booting to live cd.

Arnavion2• 2 years ago

What distros are there that normally dynamically link everything but statically link sudo? OpenSUSE, Debian and Ubuntu (the distros I have on hand) do not, at least.

paholg2• 2 years ago

I just checked on NixOs, and ldd reports sudo is not dynamic.

dralley• 2 years ago

If the system-provided libc is corrupt, isn't sudo the least of your concern? What else is going to work?

gkbrk• 2 years ago

What else is going to work?

Everything that was statically compiled.

wizzwizz4• 2 years ago

A statically-linked busybox, which is often enough.

Quekid5• 2 years ago

I'd invest $10 into a rescue USB stick regardless. I like putting Ventoy on it and having a large number of different distribution ISOs on there, just in case.

jokethrowaway• 2 years ago

Funnily enough something similar happened to me after doing a partial update. Doing a partial update are not supported in arch linux for this very reason.

sudo broke as well as many others command. ssh worked for a bit and then segfaulted. I edited my PATH to have a healthy version of libc but things kept breaking in different ways (version mismatches) In the end I had to use a live usb drive as I couldn't write to /usr/lib

rnijveld2• 2 years ago

This only works on Linux of course since the Linux kernel and libc are not tightly coupled, on any other OS dynamically linking against libc is a necessity. Also, I've never seen anyone statically linking against glibc? Is that even something people do? I'd consider your system provided libc being broken is a similar situation to an unbootable kernel: you just need a rescue stick/partition to fix it, or reinstall your OS.

thevidel• 2 years ago

including crates for platforms such as Windows, which we obviously would not require as a Unix utility.

Probably a little less obvious now that Windows has their sudo?

https://learn.microsoft.com/fr-fr/windows/sudo/

pvg• 2 years ago

This also had a bigass HN discussion recently, for those interested https://news.ycombinator.com/item?id=39305452

MuffinFlavored3• 2 years ago

We replaced it with our own argument parsing once we noticed that adopting clap was taking more code than doing it ourselves.

I feel like it's obvious that there are two sides to this echoed throughout the "programming" community:

1. Don't pull a package in for what you can do yourself because it might have 500 dependenices for no good reason

2. Don't roll your own, use something off-the-shelf third-party that is actively maintained, open-source, well written/easily usable/fleshed out, etc.

They conflict...

cryptos• 2 years ago

Yeah, but that is what makes engineering interesting. You always have to find the right balance with your trade-offs.

steveklabnik6• 2 years ago

It is true that you cannot simply repeat maxims others have declared and expect that the job gets done well. Our profession (like many, many others, if not all!) requires judgement to do the best job. Different situations may call for different decisions.

MuffinFlavored3• 2 years ago

Our profession (like many, many others, if not all!) requires judgement to do the best job.

And is almost permanently open to retrospect + disagreement of "you shouldn't have done that this way and followed maxim A, you should've followed maxim B instead" and vice versa... :)

jbverschoor• 2 years ago

They don't conflict:

1+2 -> Pull a package that is well maintained and doesn't use a ton of packages.

The problem is the language platform / "culture", for example js

MuffinFlavored3• 2 years ago

But in this post we're seeing this "mindset" trickle to the Rust ecosystem, for something as "complicated" as command-line argument parsing

cmrdporcupine2• 2 years ago

100% this, and I really hope we can turn the Rust culture ship around on this front.

I've been ranting about this a lot, and getting about a 50% upvote vs downvote ratio :-)

sunshowers2• 2 years ago

This sets up some bad incentives.

If someone decides part of a package is useful and extracts it out into another crate, then that will count as a demerit going by this rule, even though it should be rewarded.

arp242• 2 years ago

I don't think they necessarily conflict, and good programmers will pick what's appropriate for the context. The problems start when either path 1 or 2 gets followed dogmatically.

I do have to say I'm closer to 2 than 1. Most code really isn't that hard to write, and just solving just your own problems instead of the general case can simplify things a lot. And the code is 100% under your control, which can also have its advantages.

Some programmers seem afraid to write code. The amount of contortions I've seen folks pull just to re-use some package that wasn't really a good fit (or just wasn't a good package to start with) has at times been bewildering. In the most extreme case I replaced a badly working solution someone spent half a year on with something that worked well in just a week, just writing it from scratch (add another week or two for bugfixes, so that's 3 weeks).

On the other hand I've also seen people NIH the silliest of things. In the most extreme case here they had done their own templating, i18n, database layer – the works. That would have been okay if it worked well, but all of it was ad-hoc junk.

For example they did their own flag parsing for $reasons, and only "--flag=value" worked and not "--flag value". I spent quite some time being confused by this because it also didn't error on the wrong usage (it just did the wrong thing...) They gave me shit for nOt REadINg tHe dOCumENtAtiOn. Like, mate, I've never seen any tool where "--a=b" works but not "--a b" before or since, and I just used the space-variant out of habit without thinking. They didn't fix the flag parser, and I wasn't allowed to either. Didn't work long with these spanners.

Nothing wrong with doing your own flag parsing necessarily; I did my own flag parsing for Go because I don't like the stdlib package and others I could find. Waste of time? Maybe. But it's my time to waste and at least it works well.

The problem with NIH usually isn't the NIH part, but shitty programmers writing shitty code part.

epage2• 2 years ago

For some more detail on the choices that went into this, see https://www.reddit.com/r/rust/comments/1b92j0k/sudors_depend...

For myself, I think people focus too much on "dependency count" and not what those dependencies represent. For example

- If a subset of a package is pulled out, it is no longer a "zero dependency" package and some people look down on it.

- Whether you use a dependency or write your own, the logic has to exist. The main question is if there is a difference in priorities.

Applying those

- I really wonder about their claim that using clap took more code than doing it themselves. I also wonder about "not using many features" as there are a lot of usability features in clap that aren't items you check off on a list. If dropping clap, it should have been replaced with https://docs.rs/lexopt/ rather than rolling their own

- While rpassword had its problems, it would have been better to work upstream or create your own competition to upstream, rather than locking away the improvements within sudo-rs

- I think its the right choice to keep glob. So long as it implements the spec of interest, bringing it in doesn't buy you much while keeping it external gives you the whole "many eyes" situation

- I agree about dropping `thiserror`. It can be nice for prototyping or high churn code but if you write-and-forget your errors, you are carrying around that weight for nothing.

- Its unclear why they merged all of the sudo-* packages into sudo-rs. I wonder if those would have been cases where they benefit everyone for being split out for reuse.

nindalf3• 2 years ago

Agreed with everything you’ve pointed out. There seems to be an implicit assumption that all dependencies are bad, even though it’d actually be better to refactor their own code to a crate under their maintenance. Almost as if they think the people evaluating the security of this will apply a simple heuristic like “if number of deps is more than x, this software is insecure”.

dathinab3• 2 years ago

Agree the dependency count is mostly meaninglessly.

What matters is how many vaguely defined "entities" (people/groups/companies) you trust and how trustable each of them is.

Also there are not really zero dependency libraries, you always have some dependencies, e.g. the compiler implicitly is a dependency too. And so is your build system, and your languages standard library, and libc, etc. etc. So obsessing with "0" is like obsessing with "1.0" releases or abusing type systems, i.e. not helpful at all.

Additionally you can have "crate" dependencies, but you pin (or even vendor) them and give them a though "supply chain risk" review and them keep them pinned or require a another review. Sure you still have to keep track of stuff like bug fixed yanked versions etc. But for a lot of smaller crates it's feasible. In difference to some other languages it's quite easy to do so in rust (for many crates, for larger ones which have a lot of functionality where you might need bug fixes, maybe even for security this isn't that viable, but then in most projects there is only a very small number of such dependencies if any (e.g. tokio, rustls).

nindalf3• 2 years ago

how trustable each of them is

I think this is the important point. They’ve removed clap (argument parsing library) as a dependency, but they continue to trust cargo (the rust build tool) that uses that library and is primarily maintained by the same developer?

I feel like if they’re willing to trust the developers of the standard library and the official compiler and build tool, then they might as well trust clap as well.

This feels like removing dependencies just to say they did. But it may turn out well. Maybe there are “dependency skeptics” who will be won over when they see fewer dependencies.

steveklabnik6• 2 years ago

I don't have any special insight to this decision, but

- Its unclear why they merged all of the sudo-* packages into sudo-rs. I wonder if those would have been cases where they benefit everyone for being split out for reuse.

You play to your audience. If someone decides not to use sudo-rs because it's in multiple packages, that may be a bad reason, but they're still not choosing it, and that's a worse world than if they did.

I would probably do the same thing, even though I am very much on the other side of this debate from the zero-dependency folks. The intended audience is probably much more full of folks who do believe that.

rnijveld2• 2 years ago

I'd like to respond to a few things. I think using dependency count as a metric is a bad idea, that metric could easily be lowered by just copying all the code over to your project. As you rightfully say, the logic has to exist one way or another. Our approach definitely wasn't that though: it was and is never our goal to have no dependencies, but we do think that dependencies should be part of the safety story, i.e. is a dependency better or worse than what you would write yourself based on your specific use case. This needle will fall much much quicker towards writing it yourself for something like sudo, but still, at least the considerations you make should stay the same, even if the decision ends up being different. Considerations such as: is the communication with the dependency team worth it for the amount of code we save, are their goals aligned with ours, is the number of transitive dependencies that I take within my codebase with this dependency small enough, how much code am I actually saving, would I even be able to do this myself, could I help the wider community with my contributing back to that dependency, etc. I do feel that right now more often than not dependencies are just bolted onto a project as needed, and no consideration is given to any burden such a dependency might have. Aside from that, I think at least a much better metric would be something like 'teams', 'groups' or 'projects' needed to keep your project working. Still not perfect, nor is any other metric, but sometimes it helps to quantify things.

Some responses to your notes:

- The trouble is that we had to re-implement an existing CLI, and as you might expect with something that evolved over a period of some 30 years, there are quite a few weird behaviors in sudo. We initially had a mostly working implementation based on clap, but could not get some parts of the CLI to parse nicely, i.e. the code just looked hacky, and had to do all kinds of post-processing to complete the parsing of the CLI, resulting in lots of additional code. Maybe we should have looked at something like lexopt, but we just went ahead and did it ourselves initially just to see how it would go, we kind of liked the result and never looked at any alternative implementations. I do believe we looked at clap alternatives for a little while to see if something would make our parsing a little easier, but lexopt didn't surface at that time for whatever reason. We're not perfect either. I do think our parsing is pretty decent though.

- We did think about contributing back, but in the end we wanted a little more control over where the password (or more precisely 'hidden input') was stored in memory, and needed some specific parts for handling TTYs (given our setuid context) resulting in us quickly deconstructing rpassword until almost nothing of it was left. I think it's a little hard to contribute those things back, but as a side project I'd love to contribute some of the changes we made back to rpassword, there just wasn't the time to do it at that time as it would be quite a bit of work.

- Glob is a hard one, as the Rust crate is not entirely compatible with how the original sudo works. But the logic has to be there one way or another and if we have to decide between libc (i.e. probably C code) and Rust, we'd prefer to go with Rust of course. That already resulted in an issue being opened for incompatibilities of course, but it's a hard one: I'd prefer to keep the Rust code, so I hope that someone who manages glob at least agrees that it should be as compatible as possible. But I can't and don't have the expectation that their team has the same priorities, and thus we are back at one of the reasons why a dependency might not always be worth it. There's always choices to be made. For now though, we'll keep the Rust crate dependency, as it works well enough!

- Thiserror is great for prototyping, but loses its value quickly once you know what kind of errors you have, it just takes a few lines of extra code. But, thinking about teams etc: given that it is not that big, and is created and maintained by dtolnay, whose code you probably already use in multiple ways in nearly any other project, it's not the worst either. For sudo-rs though, I still think it was the better choice to remove it.

- All the sudo-* packages were mostly removed because we didn't want to expose any public API for all that internal stuff. Our initial goal is to get sudo the CLI application working, not to provide all the building blocks while the API is still in flux. We initially put it all in separate crates because of compilation time worries, but in the end those worries were unfounded. It's one of those things where Rust is still somewhat limited: we can't specify these sort of semi-private dependencies in the crates ecosystem right now, if we would have been able to specify 'nobody but us can use these as a dependency' they would have probably stayed as separate crates.

BTW: I'd like to thank you for continuing to work on Clap! There might have been a time I would have been a little worried about all the breaking changes and churn happening, but since that has stabilized I couldn't be happier! I don't think there's anyone on the sudo-rs team that had anything against clap, and I did not want to single out clap in our post specifically, so I hope you don't consider it an attack against clap. At least personally I use clap in basically every other project with a CLI.

cchance• 2 years ago

Ya it feels funny projects that think dropping major packages like CLAP that have thousands of eyes on it, for a new hand rolled implementation is somehow ... safer/better by default.

Replacing and removing dependencies is great, if your really sure somehow your code is actually improving the situation and not just shifting the issue into a new chunk of code your going to have to worry about.

ecliptik• 2 years ago

How does this compare to OpenBSD doas[1][2]?

1. https://man.openbsd.org/doas

2. https://cvsweb.openbsd.org/src/usr.bin/doas/

steveklabnik6• 2 years ago

Our current target is to build a drop-in replacement for all common use cases of sudo.

In my understanding, the same general comparison as doas to good old regular Classic (tm) sudo. They're going for "basically the same thing, but with some stuff removed" rather than a re-think of the tool.

It's like harm reduction: the idea is to be able to replace sudo with a memory-safe version where sudo is already entrenched in a workflow, not to be a successor that's somehow better in a more abstract sense.

dathinab3• 2 years ago

There is also cargo vendor (which turns dependencies into path dependencies).

Sometimes if you do security sensitive stuff it can be a good option to either:

1. pin dependencies and give each dependency a review for suspicious code

2. vendor them in some cases (e.g. applying patches, or if pinning seems to not be good enough for whatever reason likely related to offline building)

If you are not a very security sensitive project but still worry about the supply chain then it may also be an option to pin/vendor some dependencies but e.g. trust `tokio`, `regex` or similar.

E.g. not pin some more trusted dependencies but then pin some small utility crate from a random person which you don't want to write yourself and is trivial/self contained enough so that you likely might not care about any updates to it (still include it into security scans check why it was updated etc.).

anonacct37• 2 years ago

I really think that they bury the lede:

As a setuid program meant for elevating privileges, all code that is compiled into sudo-rs has the potential to accidentally (or intentionally) give access to system resources to people who should not have that access. The setuid context additionally puts some constraints on how code is executed, and dependencies might not have accounted for that context. We could not expect any of our dependencies to take into account such a context either.

This is the real problem. I've come to the conclusion that setuid programs basically shouldn't be using most libraries. The setuid environment is just fundamentally different. A normal library can have a debug output file who's location is controlled by an environment variable without that being a security risk. But the instant that program becomes setuid, that's an arbitrary file overwrite security bug. Most libraries aren't built with that in mind. They shouldn't have to be. Setuid is poorly designed.

Sudo-rs dependencies: when less is better

Related Stories