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

Enable on OpenBSD and FreeBSD #103

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

alvinpeters
Copy link

@alvinpeters alvinpeters commented Jun 8, 2024

Replaces the ioctl-sys with the ioctl macros from nix crate.
The nix crate allows this library to be built on other platforms like FreeBSD.

Passes all tests until the states test, although the main branch fails as well for literally the same reasons. Tested on macOS Sonoma 14.5 on an M1 Pro MacBook. Done FreeBSD tests through cross-rs with the same result.

Pending full testing on macOS and FreeBSD.

Edit:
Currently passing on macOS test runs: https://github.com/striczkof/pfctl-rs/actions/runs/9457974825.
Fails anchors test on FreeBSD and OpenBSD: https://github.com/striczkof/pfctl-rs/actions/runs/9458110537.


This change is Reviewable

@alvinpeters alvinpeters force-pushed the switch-to-nix-ioctl branch 2 times, most recently from 6c25733 to afa29f7 Compare June 11, 2024 00:48
Allows to be built on other platforms like FreeBSD.
Needs to stick to nix 0.26 because nix >= 0.27 requires Rust >= 1.65.
@alvinpeters alvinpeters force-pushed the switch-to-nix-ioctl branch from afa29f7 to da881c0 Compare June 11, 2024 01:03
@faern
Copy link
Member

faern commented Jun 11, 2024

Thanks for wanting to contribute! Can you please try to minimize changes to Cargo.lock? 🙏 This PR pulls in serde_derive and does a bunch of other things it probably should not do. When I try to add nix locally it just adds bitflags and nix to Cargo.lock. We don't want unnecessary churn on the lockfile.

If this supposedly works for integrating to the FreeBSD version of PF then I think we should have CI tests for that. Does Github actions support that? If we don't have tests it will inevitably break without us noticing. Could you look into adding such tests maybe?

@faern
Copy link
Member

faern commented Jun 11, 2024

Would it make sense to try to upstream FreeBSD support to ioctl-sys instead/also? They added OpenBSD support a while back, so looks like it should be possible?

@alvinpeters
Copy link
Author

alvinpeters commented Jun 15, 2024

Hey, thanks for checking out my PR and apologies for not replying ASAP. I hyperfocused into trying to make this crate actually work on OpenBSD and then FreeBSD after. I just discovered that they are way too different now. (after 20 years of divergence obviously) For example; OpenBSD doesn't use pool addresses (pf_pool is just wired funny), and FreeBSD doesn't have types for port ranges. I'm changing this PR to reflect that.

I'm developing on OpenBSD 7.5 and FreeBSD 14.1.
Using these man pages for reference on enabling OpenBSD and FreeBSD respectively:
https://man.openbsd.org/pf
https://man.freebsd.org/cgi/man.cgi?pf(4)

If anyone knows where to find pf(4) manual page for macOS Sonoma or closer please lemme know! I need it to cross-reference and Apple loves being a PITA.

Thanks for wanting to contribute! Can you please try to minimize changes to Cargo.lock? 🙏 This PR pulls in serde_derive and does a bunch of other things it probably should not do. When I try to add nix locally it just adds bitflags and nix to Cargo.lock. We don't want unnecessary churn on the lockfile.

I manually modified Cargo.toml for the exact same reason of trying to get rid of random churn (but failed). error-chain is somehow bringing other dependencies along the way. I suggest getting rid of Cargo.lock altogether like other libraries, and only using major versions in the Cargo.toml. Plus getting rid of error-chain would be a great bonus too. Perhaps this work will justify bumping this crate to 0.5? I can also try to 'pre-expand' the macros so we can drop the ioctl macro dependency altogether. I can either put that in this crate or create 'pf-sys' containing the bindings and the ioctl calls. The caveat with that is that it might be a bit hard to read/maintain and could be a non-singular point of failure.

If this supposedly works for integrating to the FreeBSD version of PF then I think we should have CI tests for that. Does Github actions support that? If we don't have tests it will inevitably break without us noticing. Could you look into adding such tests maybe?

Right now, I've added tests for the 2 BSDs using VM on ubuntu-latest. I've also modified the generate-bindings.sh to be able to generate bindings for those BSDs. Plus some initial (non-compiling) work for OpenBSD done already. (Also, sorry for the random commits lol, I'll squash them once everything's done.)

Would it make sense to try to upstream FreeBSD support to ioctl-sys instead/also? They added OpenBSD support a while back, so looks like it should be possible?

I'll try that once I'm done with this one. Judging by the last update date, I don't reckon the maintainers are active enough, plus I want a working ioctl wrapper right now.

@alvinpeters alvinpeters marked this pull request as draft June 15, 2024 23:02
@alvinpeters alvinpeters changed the title Switch from ioctl_sys to nix's ioctl Enable on OpenBSD and FreeBSD Jun 15, 2024
@alvinpeters alvinpeters force-pushed the switch-to-nix-ioctl branch from 2ac9af7 to c7922d5 Compare June 19, 2024 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants