search

seccomp / libseccomp

The main libseccomp repository
GNU Lesser General Public License v2.1
788 stars 170 forks source link

RFE: investigate Rust bindings #323

Open pcmoore opened 3 years ago

pcmoore commented 3 years ago

Given the growing popularity of Rust for system development, it seems like it might be a good idea to support a set of libseccomp Rust bindings. We could develop our own, but it seems that a developer has already created a set of bindings for Rust:

I'm not a fan of reimplementing perfectly good code, so if the bindings above seem reasonable we may want to offer the developer a chance to migrate those bindings under https://github.com/seccomp (assuming they would want to do this). Of course, if the developer doesn't want to move their repo, and we agree that their bindings seem reasonable, we can just close this out and point people at the appropriate developers repo.

I mention this as I feel that there is value in having the bindings in one central location; not only does it help discoverability, I feel it helps keep the bindings current.

pcmoore commented 3 years ago

@drakenclimber thoughts on this? If this sounds good I can reach out to polachok to see if they are interested.

At the very least I would want to make sure that they are okay with the stuff in CONTRIBUTING.md and MAINTAINER_PROCESS.md; it seems like any project under github.com/seccomp should adopt similar policies. Thoughts?

drakenclimber commented 3 years ago

thoughts on this?

Yes, I think rust support would be a very good thing. (Aside - the rust kernel RFC that was recently sent out seems very interesting.)

At the very least I would want to make sure that they are okay with the stuff in CONTRIBUTING.md and MAINTAINER_PROCESS.md; it seems like any project under github.com/seccomp should adopt similar policies. Thoughts?

Sounds great to me. If possible, I would love to credit and use an existing project rather than reinventing a wheel.

KentaTada commented 3 years ago

Hello, @pcmoore @drakenclimber

I'm sorry to interrupt you, but I'd like to introduce you to our related project. We have been developing a set of bindings for Rust https://github.com/ManaSugi/libseccomp-rs to use it for Kata container and our rust-based container runtime. We will continue to maintain our libseccomp-rs because it will be adopted for Kata. Could you check our project if you are interested in that?

@ManaSugi Could you explain feature and status of https://github.com/ManaSugi/libseccomp-rs?

ManaSugi commented 3 years ago

@KentaTada Thank you for the introduction!

Hello, @pcmoore, @drakenclimber As described above, we are developing the libseccomp-rs that is a set of projects (high-level bindings, low-level bindings, and tool for generating low-level bindings automatically).

The libseccomp crate wraps libseccomp-sys of low-level bindings in a nice to use, mostly safe API. The libseccomp-sys in libseccomp-rs seems like seccomp-sys developed by polachok. The main difference between seccomp-sys and our libseccomp-sys is that libseccomp-sys is generated using bindgen automatically.

Currently, we are working on adding seccomp feature to kata-containers using our libseccomp-rs. https://github.com/kata-containers/kata-containers/pull/1788 We are preparing for the merge, discussing it with the community.

In addition, the libseccomp-rs has already been used in our rust-based container runtime that I presented at Cloud Native Rust Day a few weeks ago. https://static.sched.com/hosted_files/cloudnativerustdayeu21/e8/Rust_based_Secure_and_Lightweight_Container_Runtime_for_Embedded_Systems.pdf

Please let me know if you are interested in our libseccomp crate.

pcmoore commented 3 years ago

Thanks @KentaTada and @ManaSugi, this is something worth looking into! Unfortunately I am still new to Rust so it is hard for me to judge things at the moment but I definitely prefer bindings which are generated by hand over automated bindings; they tend to hold up better over the long run. This is one of the reasons why the Python bindings were done by hand using Cython and not automated via SWIG.

I also very much like the idea of adopting bindings from people who have been active and contributed patches to the general libseccomp project already. These people are less of an unknown and I feel like I can trust them more than people who have never contributed :)

@drakenclimber any thoughts on the projects mentioned above?

ManaSugi commented 3 years ago

@pcmoore Thank you for your valuable opinion.

I definitely prefer bindings which are generated by hand over automated bindings; they tend to hold up better over the long >run. This is one of the reasons why the Python bindings were done by hand using Cython and not automated via SWIG.

I understand the reason. We can stop using bindgen and maintain the crate by hand. In that case, all we have to do is change libseccomp-sys which is low-level bindings inside libseccomp-rs.

drakenclimber commented 3 years ago

I looked through libseccomp-rs. Nice work, @ManaSugi and @KentaTada. It looks really solid to me.

I have a handful of thoughts:

drakenclimber commented 3 years ago

I see that calls to the libseccomp C library are surrounded by unsafe, e.g. let ret = > unsafe { seccomp_export_pfc(self.ctx.as_ptr(), fd.as_raw_fd()) };. Did you experience memory issues without the unsafe flag?

After looking at the Rust documentation, are you using the unsafe block for the pointer dereference? Would it be possible to only unsafe the dereference so that we can take advantage of Rust's memory checking in more places?

ManaSugi commented 3 years ago

Thank you very much for the review of our crate, @drakenclimber.

After looking at the Rust documentation, are you using the unsafe block for the pointer dereference? Would it be possible to only unsafe the dereference so that we can take advantage of Rust's memory checking in more places?

The reason why all calls to the libseccomp C library are surrounded by unsafe block is that we must use unsafe when we use Rust's FFI(Foreign Function Interface). Foreign Functions are assumed to be unsafe so calls to them need to be wrapped with unsafe as a promise to Rust's compiler that everything contained within truly is safe. This is described in Rust documentation about FFI. Therefore, we can not call libseccmp C library without unsafe.

Would it be possible to only unsafe the dereference so that we can take advantage of Rust's memory checking in more places?

Unfortunately, we can not call the libseccomp C library from Rust code without unsafe as I mentioned above. Therefore, we have to create safe wrapper functions to hide unsafe internal details as much as possible. For example, we use NonNull in ScmpFilterContext to ensure the return value of seccomp_init of libseccomp C is non-null. Ref: https://github.com/ManaSugi/libseccomp-rs/blob/v0.1.3/libseccomp/src/lib.rs#L416

I saw in your README that you show how to statically link with libseccomp. I don't have a strong feeling about this one way or the other, but it's been a pain point for some C users over the years. I don't yet know if rust will hit the same issues.

Yes, we recommend that developers should use libseccomp crate with dynamically linked the libseccomp C library. Therefore, the crate is linked dynamically by default. Howevever, some users want to link the crate statically, so the README shows how to statically link with libseccomp. In fact, kata-containers will use the libseccomp crate with statically linked the llibrary.

What are others' thoughts on licensing? (I try to stay out of the licensing quagmire entirely.) I see the Rust bindings are Apache or MIT where libseccomp C code is LGPL.

Our libseccomp crate includes only the function interface of the libseccomp C library, not the C code itself. Therefore, we can publish the crate under the MIT or Apaches 2.0. The reason I selected the license is that Rust community recommends that crates should be dual-licensed, under either the MIT or Apaches 2.0 licenses. Ref: https://rust-lang.github.io/api-guidelines/necessities.html#crate-and-its-dependencies-have-a-permissive-license-c-permissive

However, if the crate is used with statically linked the libseccomp library that is under LGPL-2.1, we should comply with the LGPL-2.1 (6(a)) when we distribute the binary including the crate. I should add an explanation about that in README. Could you tell me your thoughts about the license?

We had talked about splitting out the python bindings from the libcgroup github repo and putting them in their own repo. Do we consider doing that and figuring out a way to share as much automated testing as possible between C, Go, Python, and Rust?

I'm interested in sharing automated testing between various languages, but I think it is difficult because of the different function interfaces. Could you tell me your idea about a way to share libseccomp test?

drakenclimber commented 3 years ago

Thank you very much for the review of our crate, @drakenclimber.

After looking at the Rust documentation, are you using the unsafe block for the pointer dereference? Would it be possible to only unsafe the dereference so that we can take advantage of Rust's memory checking in more places?

The reason why all calls to the libseccomp C library are surrounded by unsafe block is that we must use unsafe when we use Rust's FFI(Foreign Function Interface). Foreign Functions are assumed to be unsafe so calls to them need to be wrapped with unsafe as a promise to Rust's compiler that everything contained within truly is safe. This is described in Rust documentation about FFI. Therefore, we can not call libseccmp C library without unsafe.

Makes sense. Thank you. I'm newer to rust as well and hadn't used the FFI portion yet, but did hit the memory checking once.

I saw in your README that you show how to statically link with libseccomp. I don't have a strong feeling about this one way or the other, but it's been a pain point for some C users over the years. I don't yet know if rust will hit the same issues.

Yes, we recommend that developers should use libseccomp crate with dynamically linked the libseccomp C library. Therefore, the crate is linked dynamically by default. Howevever, some users want to link the crate statically, so the README shows how to statically link with libseccomp. In fact, kata-containers will use the libseccomp crate with statically linked the llibrary.

Sounds good.

What are others' thoughts on licensing? (I try to stay out of the licensing quagmire entirely.) I see the Rust bindings are Apache or MIT where libseccomp C code is LGPL.

Our libseccomp crate includes only the function interface of the libseccomp C library, not the C code itself. Therefore, we can publish the crate under the MIT or Apaches 2.0. The reason I selected the license is that Rust community recommends that crates should be dual-licensed, under either the MIT or Apaches 2.0 licenses. Ref: https://rust-lang.github.io/api-guidelines/necessities.html#crate-and-its-dependencies-have-a-permissive-license-c-permissive

However, if the crate is used with statically linked the libseccomp library that is under LGPL-2.1, we should comply with the LGPL-2.1 (6(a)) when we distribute the binary including the crate. I should add an explanation about that in README. Could you tell me your thoughts about the license?

As I said originally, I try to stay out of license discussions and leave them to the lawyers :). But what you outlined above is exactly the scenario I was afraid of. It might be good to run these scenarios by your company's legal team.

We had talked about splitting out the python bindings from the libcgroup github repo and putting them in their own repo. Do we consider doing that and figuring out a way to share as much automated testing as possible between C, Go, Python, and Rust?

I'm interested in sharing automated testing between various languages, but I think it is difficult because of the different function interfaces. Could you tell me your idea about a way to share libseccomp test?

(Warning - not a fully thought out idea here.)

The automated tests within libseccomp do a really good job of covering every major feature from a functional point of view. And the framework supports both C and Python already. I don't think it would be too tough to add support for more languages to the framework. Over time we could add Golang and Rust tests alongside the existing tests.

But where should these tests reside in github? In libcgroup, we split the tests into their own repo - which has had some definite pros/cons. We could leave the tests in the seccomp/libseccomp repo, then have the other languages add the tests (and libcgroup library code) as a git submodule. Again, thinking out loud here, that could work pretty well...

Ultimately my real goal is to get better functional code coverage for Golang and Rust. I'm open in how we get there.

Thanks!

ManaSugi commented 3 years ago

@drakenclimber, Thank you for the kind reply.

I'm sorry, I misunderstood about the automated tests, but now I understand that. Do you mean that we should aggregate tests all in one tests directory of libseccomp? I think it sounds great because we can get better function code coverage for various languages as you mentioned. I'm interested in the work!

First of all, I'll start to create more tests of our libseccomp crate while referencing the existing functional tests.

Thanks again.

drakenclimber commented 3 years ago

Do you mean that we should aggregate tests all in one tests directory of libseccomp?

Possibly, but honestly I don't really know. I am also the maintainer of libcgroup, and a while back we split our tests into a separate git repo. Overall I have mixed feelings on the split. The tests can grow and change without excess code churn in the main repo, but keeping the two repos in sync is more work than I like. Also, using git submodules is less than ideal.

tl;dr - I'm not sure what the right answer is regarding tests for multiple languages and multiple platforms. I don't think there's an easy answer :(

ManaSugi commented 2 years ago

Edited 2022/01/05 Hello, @pcmoore @drakenclimber

https://github.com/libseccomp-rs/libseccomp-rs I'd like to share the updates for our libseccomp crate that is rust bindings. We released v0.2.0 including major updates a few days ago (The latest version is v0.2.2). The current libseccomp crate provides many features that are close to the libseccomp-golang.

In addition, we have started managing the libseccomp-sys as low-level bindings by hand over automated bindings as mentioned in the comment and improved the documentation for the users.

Currently, I'm considering a way to add automated testing to the existing framework that supports C and Python already. Of course, the libseccomp crate has already had many unit tests and the current code coverage is 97%. However, as @drakenclimber mentioned above, I think it would be better to create 58 tests like C and Python for the libseccomp crate and manage them by the upstream libseccomp repo or an outside repo. Then I created a proof of concept for adding Rust bindings tests to the existing framework. ~The PoC commit: https://github.com/ManaSugi/libseccomp/commit/90d23a8a667a8acd83d8ecebb40c0ea325c014b6~ The PoC commit: https://github.com/ManaSugi/libseccomp/commit/b2499186d6d311ef27bc1c1920a09b1dd20d7c96

In the commit, I added the rust directory including tests programs, utils, and Makefile under tests directory. ~For the convenience of the Rust project, I create a project(directory) for each test program under the rust directory. Each test directory such as 01-sim-allow is managed by cargo so that the test programs can be compiled by just cargo build command. In addition, we can build all test programs by just cargo build command under the rust directory because each test directory is a cargo workspace. Currently, there are 5 tests in the rust directory in order to confirm that the tests can run correctly.~

rust                              
├── 01-sim-allow.rs            
├── 02-sim-basic.rs            
├── 03-sim-basic_chains.rs     
├── 04-sim-multilevel_chains.rs
├── 05-sim-long_jumps.rs                
├── Cargo.toml                 
├── Makefile                   
└── utils.rs

The tests can be run by the following commands:

$ ./configure --enable-rust
$ cd tests && make check-build
$ ./regression -m rust

Could you give me your thoughts about the libseccomp crate and tests if you do not mind? Thank you.

drakenclimber commented 2 years ago

Hello, @pcmoore @drakenclimber

https://github.com/libseccomp-rs/libseccomp-rs I'd like to share the updates for our libseccomp crate that is rust bindings. We released v0.2.0 including major updates a few days ago (The latest version is v0.2.2). The current libseccomp crate provides many features that are close to the libseccomp-golang.

In addition, we have started managing the libseccomp-sys as low-level bindings by hand over automated bindings as mentioned in the comment and improved the documentation for the users.

I really like what you've done with these two crates. Hiding the ugly C bindings in the low-level sys crate makes for a clean interface. The interfaces in the user-facing libseccomp crate map nicely to the low-level C while still taking advantage of Rust and its nifty features. Nice work.

Currently, I'm considering a way to add automated testing to the existing framework that supports C and Python already. Of course, the libseccomp crate has already had many unit tests and the current code coverage is 97%. However, as @drakenclimber mentioned above, I think it would be better to create 58 tests like C and Python for the libseccomp crate and manage them by the upstream libseccomp repo or an outside repo. Then I created a proof of concept for adding Rust bindings tests to the existing framework. The PoC commit: ManaSugi@90d23a8

Could you give me your thoughts about the libseccomp crate and tests if you do not mind? Thank you.

Your integration of the Rust code and the existing libcgroup library/tests is creative. Very cool. I have a few questions, but I'll post them inline to the RFC patch you sent.

Thanks so much @ManaSugi

ManaSugi commented 2 years ago

Thank you for your review. JFYI: I squashed the commits and refactored the test programs: https://github.com/ManaSugi/libseccomp/commit/b2499186d6d311ef27bc1c1920a09b1dd20d7c96 Thank you.

ManaSugi commented 1 year ago

Hello, @pcmoore @drakenclimber

I've released libseccomp-rs v0.3.0 recently, but I'm sorry for the late update on the test programs for rust.

Currently, I prepared for 12 tests for rust, but there are still a lot of tests and it could take some time to create all tests (60 tests). Is it nice to start considering that libseccomp-rs will be migrated under the seccomp project after preparing for the all tests? Thank you.

pcmoore commented 1 year ago

Thanks for the update @ManaSugi

ManaSugi commented 1 year ago

Hello, @pcmoore @drakenclimber

We created 60 tests and confirmed the CI works properly. Some tests still fail because the libseccomp-rs doesn't support yet the latest functions or architectures that will include in the next release, v2.6.0. (we have plan to support them soon.) But, we are ready to happily migrate the libseccomp-rs under your seccomp project. https://github.com/ManaSugi/libseccomp/commit/e60406319712477af63a0cb4a85c8811c71efb9c

Thank you.

drakenclimber commented 1 year ago

Thanks, @ManaSugi. You've put a lot of time into this work; thank you.

I briefly looked through your changes, and they seem like a reasonable patchset to start the discussion. I recommend you open a pull request.

A couple questions off the top of my head:

ManaSugi commented 1 year ago

@drakenclimber, @pcmoore Cc @rusty-snake

Thanks for the review and I'm glad to start the disucussion. I opened a pull request #411 to add the tests (1-60) for Rust bindings and the all tests passed.

It feels weird that the rust test directory uses a Makefile and not Makefile.am. (Aside - the Makefile is really simple.) How hard would it be to convert this to autotools like the rest of the project?

I removed the Makefile and converted it to the exsting Makefile.am.

In the past, @pcmoore and I have discussed moving the python bindings to their own repository. (The libseccomp go bindings are separate.) There are pros/cons to colocating, and I can't remember what we ended up preferring. Inertia is powerful, and merging rust into the main repo seems to make sense. @pcmoore do you have any thoughts?

I also think merging the Rust bindings into the main repo seems to be powerful, but some problems are caused.

Merging the Rust bidings into the main repo:

pcmoore commented 5 months ago

I think it is time to revisit this discussion @drakenclimber and @ManaSugi - back when this issue was created in 2021 I both had more time to devote to libseccomp as well as hopes to learn Rust in the near future. Now, three (almost four) years later, I still have not had the time to learn Rust and the amount of time I am able to allot to libseccomp feature development has decreased, or at least been de-prioritized. This is a long way of saying that I'm not certain I would be able to provide the necessary back-up support for including libseccomp Rust bindings in the main GH seccomp organization at this point in time.

@drakenclimber, how about you? Do you both have the cycles and a level of familiarity with Rust to back-up @ManaSugi for inclusion of Rust bindings for libseccomp?

drakenclimber commented 5 months ago

@drakenclimber, how about you? Do you both have the cycles and a level of familiarity with Rust to back-up @ManaSugi for inclusion of Rust bindings for libseccomp?

I am in a similar position. My goal of growing my rust knowledge remains a goal :(. And as has been apparent the last few months, I also have had limited time for libseccomp.

With that said, I think libseccomp rust bindings are important and would be a tremendous asset to the community. Given that @pcmoore and I are struggling to find time for libseccomp, I'm now wondering if a separate libseccomp/rust repository is the best solution. I'm definitely open to ideas here, though.

pcmoore commented 5 months ago

With that said, I think libseccomp rust bindings are important and would be a tremendous asset to the community. Given that @pcmoore and I are struggling to find time for libseccomp, I'm now wondering if a separate libseccomp/rust repository is the best solution. I'm definitely open to ideas here, though.

My only concern with bringing the Rust bindings into this GH org is that if the current devs and maintainers go off to work on other things @drakenclimber and I are going to be responsible for maintaining the bindings which is not something I can agree to support at this point in time.

The good news is that there is no harm caused by the Rust bindings living elsewhere.

ManaSugi commented 5 months ago

@pcmoore @drakenclimber Thank you so much for your comment! I exactly understand your current situation and opinions.

From the perspective of the libseccomp-rs side, bringing the Rust bindings into the GH org can help itself to keep up with the same quality of the original C library and the Python bindings by leveraging the common test framework added in the PR #411, and both @rusty-snake (the other maintainer of the Rust binding) and at least I can take so enough time to maintain it as usual even after it is moved to the GH org. Besides, as @pcmoore mentioned on the first comment in this issue, I also feel that there is value in having the bindings in one central location like the libseccomp-golang, which helps it's discoverability. But, I don't have a strong opinion about that and I also agree with the fact that there is no harm caused by Rust bindings living elsewhere.

@rusty-snake Could you tell me your thoughts?

ManaSugi commented 4 months ago

@rusty-snake Do you have any comments?

rusty-snake commented 4 months ago

I've no clear preference between the repo being in the seccomp or libseccomp-rs org. Test suites can run it both. Only merging in one repo (i.e. one PR for changes) would make a difference. seccomp seems to still have some stricter, arguable DCO policy. Visibility/visible as official might be increased, but you can find it on crates.io (search for a (lib)seccomp crate for rust vs. search for language bindings of libseccomp) and it would be possible to link from here.