6 ms·

Sudo-rs dependencies: when less is better

93 points by marbu 2y ago

deleted 2y ago
[deleted]
awoimbee 2y 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 2y ago
  Don’t think it’s worth it. Looking at sudo’s man page at https://linux.die.net/man/8/sudo 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.
  - scbrg 2y 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.
  - 0cf8612b2e1e 2y 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.
    - somat 2y ago
      I consider double-dashes to be a mistake. hell, after a few drinks and quiet thought I consider single dashes to be a mistake. Perhaps the dd arg=val form is actually the ideal argv method after all. What if getopt was all a huge mistake. And then I sober up and realize they are just dashes, useless but harmless, not a thing worth worrying about. And then you have the absolutely inane doubledash --arg=value format. Way to carry a bad idea to it's logical conclusion guys. somebody drunk their getopt kool-aid that morning. just get rid of the stupid dashes if you are going to do that.
- IshKebab 2y 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 2y 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.en.html https://manpages.debian.org/bookworm/manpages-dev/getopt.3.e...
  - deleted 2y ago
    [deleted]
  - steveklabnik 2y 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 2y 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?
- 0cf8612b2e1e 2y 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.
- Arnavion 2y 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.
- dathinab 2y 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 2y 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 https://news.ycombinator.com/item?id=39828499
sebazzz 2y 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.
- Arnavion 2y 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.
  - paholg 2y ago
    I just checked on NixOs, and ldd reports sudo is not dynamic.
    - justinsaccount 2y ago
      Did you check the real sudo binary, or the setuid wrapper? On my system sudo is `/run/wrappers/bin/sudo` but that is a setuid wrapper for `/nix/store/z008bzqrl2zc848gjhh04012jhxpl72q-sudo-1.9.15p5/bin/sudo` which is dynamically linked.
      - paholg 2y ago
        Ah, I guess I just checked the wrapper. That's what I get for doing it on my phone over ssh. I would have probably looked deeper with a real keyboard.
- dralley 2y ago
  If the system-provided libc is corrupt, isn't sudo the least of your concern? What else is going to work?
  - gkbrk 2y ago
thevidel 2y 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/ https://learn.microsoft.com/fr-fr/windows/sudo/
- pvg 2y ago
  This also had a bigass HN discussion recently, for those interested https://news.ycombinator.com/item?id=39305452 https://news.ycombinator.com/item?id=39305452
- deleted 2y ago
  [deleted]
MuffinFlavored 2y 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...
- deleted 2y ago
  [deleted]
- cryptos 2y ago
  Yeah, but that is what makes engineering interesting. You always have to find the right balance with your trade-offs.
- steveklabnik 2y 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.
  - MuffinFlavored 2y 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... :)
    - steveklabnik 2y ago
      Yeah, time is one of those factors that can change, and tip the scales one way or the other.
- jbverschoor 2y ago
epage 2y ago
For some more detail on the choices that went into this, see https://www.reddit.com/r/rust/comments/1b92j0k/sudors_dependencies_when_less_is_better/ktuf2t2/ 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/ 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.
- nindalf 2y 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”.
- dathinab 2y 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).
ecliptik 2y ago
How does this compare to OpenBSD doas[1][2]? 1. https://man.openbsd.org/doas https://man.openbsd.org/doas 2. https://cvsweb.openbsd.org/src/usr.bin/doas/ https://cvsweb.openbsd.org/src/usr.bin/doas/
- steveklabnik 2y 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.
dathinab 2y 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 2y 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.

93 points by marbu 2y ago

deleted 2y ago

[deleted]

> 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 2y ago
Don’t think it’s worth it. Looking at sudo’s man page at https://linux.die.net/man/8/sudo 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.
- scbrg 2y 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.
- 0cf8612b2e1e 2y 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.
  - somat 2y ago
    I consider double-dashes to be a mistake. hell, after a few drinks and quiet thought I consider single dashes to be a mistake. Perhaps the dd arg=val form is actually the ideal argv method after all. What if getopt was all a huge mistake. And then I sober up and realize they are just dashes, useless but harmless, not a thing worth worrying about. And then you have the absolutely inane doubledash --arg=value format. Way to carry a bad idea to it's logical conclusion guys. somebody drunk their getopt kool-aid that morning. just get rid of the stupid dashes if you are going to do that.
IshKebab 2y 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 2y 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.en.html https://manpages.debian.org/bookworm/manpages-dev/getopt.3.e...
- deleted 2y ago
  [deleted]
- steveklabnik 2y 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 2y 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?

0cf8612b2e1e 2y 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.
Arnavion 2y 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.
dathinab 2y 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 2y 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 https://news.ycombinator.com/item?id=39828499

sebazzz 2y 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.

Arnavion 2y 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.
- paholg 2y ago
  I just checked on NixOs, and ldd reports sudo is not dynamic.
  - justinsaccount 2y ago
    Did you check the real sudo binary, or the setuid wrapper? On my system sudo is `/run/wrappers/bin/sudo` but that is a setuid wrapper for `/nix/store/z008bzqrl2zc848gjhh04012jhxpl72q-sudo-1.9.15p5/bin/sudo` which is dynamically linked.
    - paholg 2y ago
      Ah, I guess I just checked the wrapper. That's what I get for doing it on my phone over ssh. I would have probably looked deeper with a real keyboard.
dralley 2y ago
If the system-provided libc is corrupt, isn't sudo the least of your concern? What else is going to work?
- gkbrk 2y ago

thevidel 2y 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/ https://learn.microsoft.com/fr-fr/windows/sudo/

pvg 2y ago
This also had a bigass HN discussion recently, for those interested https://news.ycombinator.com/item?id=39305452 https://news.ycombinator.com/item?id=39305452
deleted 2y ago
[deleted]

MuffinFlavored 2y 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...

deleted 2y ago
[deleted]
cryptos 2y ago
Yeah, but that is what makes engineering interesting. You always have to find the right balance with your trade-offs.
steveklabnik 2y 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.
- MuffinFlavored 2y 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... :)
  - steveklabnik 2y ago
    Yeah, time is one of those factors that can change, and tip the scales one way or the other.
jbverschoor 2y ago

epage 2y ago

For some more detail on the choices that went into this, see https://www.reddit.com/r/rust/comments/1b92j0k/sudors_dependencies_when_less_is_better/ktuf2t2/ 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/ 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.

nindalf 2y 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”.
dathinab 2y 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).

ecliptik 2y ago

How does this compare to OpenBSD doas[1][2]? 1. https://man.openbsd.org/doas https://man.openbsd.org/doas 2. https://cvsweb.openbsd.org/src/usr.bin/doas/ https://cvsweb.openbsd.org/src/usr.bin/doas/

steveklabnik 2y 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.

dathinab 2y 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 2y 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.