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

genkeys, yggdrasilctl: Use pledge(2) on OpenBSD #1193

Merged
merged 4 commits into from
Dec 12, 2024

Conversation

klemensn
Copy link
Contributor

@klemensn klemensn commented Oct 20, 2024

Restrict system operations of CLI tools with https://man.openbsd.org/pledge.2.

https://pkg.go.dev/suah.dev/protect abstracts the OS specific code, i.e. is a NOOP on non-OpenBSD systems.

This PR is to gauge upstream interest in this direction; my OpenBSD port of yggdrasil already pledges the daemon,
resulting in minimal runtime privileges, but there are still a few rough edges:
https://github.com/jasperla/openbsd-wip/blob/master/net/yggdrasil/patches/patch-cmd_yggdrasil_main_go#L80

The simplest tool first:  genkeys only does standard I/O, nothing else.
The CLI is simple, but parses config files and communicates over the network
with arbitrary endpoints.

Limit system operations to that is needed before doing anything and drop all
priviledges after config file and socket handling is done, i.e. do parse and
speak over the network completely unprivileged.
@klemensn
Copy link
Contributor Author

klemensn commented Oct 30, 2024

Rebased onto develop.

@neilalexander any thoughts on the general direction of adding pledge and unveil to yggdrasil?

It would be code effectively running only on OpenBSD, but it being there should help developers on other OSes applying similar security mechanisms, e.g. Linux/seccomp or FreeBSD/capsicum.

@neilalexander
Copy link
Member

In principle I'm not opposed but I am a little bit concerned about absorbing more and more platform-specific code for platforms that neither I nor Arceliar run or can easily test. If you're volunteering to maintain OpenBSD-specific code on an on-going basis, that's a different story.

@klemensn
Copy link
Contributor Author

In principle I'm not opposed but I am a little bit concerned about absorbing more and more platform-specific code for platforms that neither I nor Arceliar run or can easily test.

That's expected, hence my asking before investing my time and yours upstream, only to keep work as local patches after all because of stuff like this.

If you're volunteering to maintain OpenBSD-specific code on an on-going basis, that's a different story.

Sure, I'll gladly put the had on for any OpenBSD-specific bits in yggdrasil, if that helps.

I've already ported it and am actively maintaining it; it still lives in jasperla/openbsd-wip until I'm confident enough that everything will work as expected for OpenBSD users, at which point I'll send my port out for review and import it into openbsd/ports proper.

All my PRs so far came out of that, making stuff "just work" on my nodes.

@klemensn
Copy link
Contributor Author

klemensn commented Nov 2, 2024

I've already ported it and am actively maintaining it; it still lives in jasperla/openbsd-wip until I'm confident enough that everything will work as expected for OpenBSD users, at which point I'll send my port out for review and import it into openbsd/ports proper.

I missed that we already had a heavily outdated net/yggdrasil-go port at 0.5.3, so I just polished and updated that and took maintainership to take care of it in the future.

@klemensn
Copy link
Contributor Author

klemensn commented Nov 2, 2024

my OpenBSD port of yggdrasil already pledges the daemon, resulting in minimal runtime privileges, but there are still a few rough edges

Such as https://github.com/yggdrasil-network/yggdrasil-go/pull/927/files#r1826606055

The lack of serialisation between goroutines makes them race against each other.
The main goroutines wants to drop privileges, but has no way of knowing whether all privileged tasks on startup have been performed already.

@klemensn
Copy link
Contributor Author

@neilalexander Is this something you'd consider merging upstream?

If not, I'll carry local patches in the OpenBSD port and drop these PRs.

If so, then perhaps merge this one and refine, i.e. tighten security, in follow-up ones?

@neilalexander
Copy link
Member

Happy to merge this for now, yep.

@neilalexander neilalexander merged commit 2d58774 into yggdrasil-network:develop Dec 12, 2024
20 checks passed
@klemensn klemensn deleted the pledge branch December 12, 2024 18:50
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