boustrophedon
feat(builtin): Add/extend various builtins
This PR combines #45, #46, #47, #48, #49, #50 and #51 to see and test the full picture. It might be easier to merge this PR after reviewing the other to avoid rebase issues.
Hey, thanks for working on this! I think the macro seems helpful, but it's a bit hard to audit simply because Rust macros are hard to read.
I'm wondering if it's possible to put the macro in a scripts/ folder (possibly in a crate inside that folder) and then use something like cargo-expand to pre-expand and prettify the resulting code.
Then we'd get the ergonomics of the macro but it would be a lot easier to audit the code.
I don't really know if there's much utility in the tests for most of these commits. They're mostly just "The code has the identifier X in it, test that it gives me identifier X." I don't think there are any bugs that these tests can catch, and if the code needs to be changed the tests also necessarily need to be changed.
I can see why they're useful for testing that the allow! macro works, but for the individual rulesets I would rather have actual integration tests that show the syscalls being used.
More generally, would you consider creating an extrasafe-ext crate (or something similar) and putting these rulesets in it? I think truncate/ftruncate could be added directly to SystemIO, but the others seem a bit more niche.
Do you have a usecase for these such that they don't just fit inside the crate they're being used in?
Sorry for the late answer, I somehow forgot, I opened this PR m).
it's a bit hard to audit simply because Rust macros are hard to read.
Well, either that or quite a lot of repetitive code. E.g. the 40 (+9 empty) lines of allow! in src/builtins/time.rs expand to 74 lines.
The use of this macro avoids checking each method for mistakes. You only have to check if 1. the macro expands correctly in principle, 2. the declarations inside a allow! { … }, which is like 49% documentation and 51% rust-like function-signatures.
I will however try to add some more documentation to make the macro itself more readable/auditable.
I'm wondering if it's possible to put the macro in a scripts/ folder (possibly in a crate inside that folder) and then use something like cargo-expand to pre-expand and prettify the resulting code.
With rust-analyzer and e.g. VS Code/VS Codium and certainly other IDEs as well, each use of allow can be expanded in a separate tab by a simple click. Making it somehow part of the build-system would only complicate it even more. This would combine the disadvantages of both.
I prefer to expand each usage in my IDE, to verify that the macro works as intended, but ignore the expanded code for the most time.
I don't really know if there's much utility in the tests for most of these commits. They're mostly just "The code has the identifier X in it, test that it gives me identifier X." I don't think there are any bugs that these tests can catch, and if the code needs to be changed the tests also necessarily need to be changed.
The code was actually part of another (private) crate where code coverage was measured with cargo tarpaulin. These simple tests where necessary to get 100% coverage.
I would rather have actual integration tests that show the syscalls being used.
Well, they use Sysno::$syscall. Testing syscalls would therefore test the libc/linux rather than this crate.
More generally, would you consider creating an extrasafe-ext crate (or something similar) and putting these rulesets in it?
IMHO, this is a good idea. However, how to decide which should be part of which crate? IMHO, either everything currently in builtins is moved to this crate or nothing. Should this crate be part of this repository? Because it heavily depends on this crate and (almost) nothing else.
Do you have a usecase for these such that they don't just fit inside the crate they're being used in?
I wanted to share my implementations to others can use them. IMHO, we should have a ever growing crate for all rulesets so others do not need to implement them theirself. E.g. SocketPair is somehow required when tokio/async stuff was used IIRC. The only really niche thing is the netlink-part. This is only required in combination with e.g. libnftables.
I refactored the allow!-macro and added some documentation/comments. However, The issue of a second crate remains.
Hi, sorry for the delay.
I think if you don't want to write integration tests for these new rulesets it would probably be best to make a new crate in a separate repo for them. While there are a few unused builtins (and I'd like to fill those gaps at some point), for the most part I have working tests that show Rust std functions failing when the builtin rulesets are enabled.
Thanks again for doing this work - I think it's useful and if you do decide to make a separate crate I'd be glad to link to it from the readme.
Hi, sorry for the delay.
No worries, I didn’t respond for like 3 months :sweat_smile:
Thanks again for doing this work - I think it's useful and if you do decide to make a separate crate I'd be glad to link to it from the readme.
I somewhat dislike the idea to have this crate in a separate repository, because extrasafe will the only dependency. What do you think about moving src and Cargo.toml to a extrasafe directory inside this repository, add a extrasafe-rulesets directory with its own src, Cargo.toml, etc., and add a workspace Cargo.toml to the root, similar to e.g. https://github.com/serde-rs/serde. With this, the dependencies, etc. can be declared in the workspace-manifest while both extrasafe and extrasafe-rulesets only use e.g. libc.workspace = true. See https://doc.rust-lang.org/cargo/reference/workspaces.html for more information. The code in examples and tests is requiring builtins, so should be moved to the extrasafe-ruleset crate.
With this workspace-setup, additional crates can be added easily in the future and all dependencies, etc. will keep in-sync across crates. Developing, testing, etc. will stay connected, yet the conceptual different parts – the framework on one hand and some ready-to-use rulesets on the other – are separate.
I can open another PR for that.
Hey, wasn't sure whether I should respond here or in one of the PRs. I think this work is useful but I think having a required separate crate in your dependencies is something I want to avoid. I made the rulesets in builtins because they make it easy to do most basic IO operations/syscalls. I think a lot of the other ones are more niche - in particular the netlink and raw socket ones are extra-unsafe and I really don't want them in extrasafe itself.
Also, I still don't want to merge the new rulesets into this repo without having actual tests that exercise them. Using actual std calls like File::create etc has in fact caught bugs / missing syscalls while writing code.
For these reasons I would really encourage you to make a extrasafe-rulesets crate that you can publish and update at your own pace, with your desired amount of testing.
I'm getting over a cold and have to catch up on work this week so I might continue to be slow to respond, sorry.
I'm getting over a cold and have to catch up on work this week so I might continue to be slow to respond, sorry.
Get well first, no need to hurry.
I have to do other more urgent tasks right now anyway, so no need to push this quite huge changes.