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

feat(droproot): support non-Linux platforms #733

Merged
merged 8 commits into from
Jun 20, 2024

Conversation

skarekrow
Copy link
Contributor

Thanks for your work on this! Ideally I'd be having this run on my router (OpenBSD) as a standard user, which causes obvious failures since this relies on libcap for Linux :D

This PR is more a discussion point as I don't understand how to skip certain dependencies based on the GOOS received, but thought I'd show there's interest in running this outside of docker as an unprivileged user. I suspect since you can cross complie with many targets, this opens up the possibility of running on a lot of other UNIX-like systems (*BSD's come to mind)

This compiles fine and runs great on an OpenBSD host with this one dependency dropped however!

Ran using:

CF_API_TOKEN=YOUR-CLOUDFLARE-API-TOKEN DOMAINS=xxxx.org TZ=America/Chicago

Copy link

codecov bot commented Apr 14, 2024

Codecov Report

Attention: Patch coverage is 0% with 49 lines in your changes missing coverage. Please review.

Project coverage is 88.40%. Comparing base (bb5ee7f) to head (618501a).

Files Patch % Lines
internal/droproot/cap_linux.go 0.00% 25 Missing ⚠️
internal/droproot/drop.go 0.00% 24 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #733      +/-   ##
==========================================
- Coverage   88.49%   88.40%   -0.09%     
==========================================
  Files          48       48              
  Lines        1990     1992       +2     
==========================================
  Hits         1761     1761              
- Misses        208      210       +2     
  Partials       21       21              
Flag Coverage Δ
unittests 88.40% <0.00%> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@favonia
Copy link
Owner

favonia commented Apr 14, 2024

Thanks! See #728 for the plan to include a build that does exactly what you suggested. Ideally, it should be a build tag.

PS: any suggestion for the name of the alternative build that works for non-Linux (and apparently Linux containers)?

@skarekrow
Copy link
Contributor Author

Hah, that's serendipitous! I think the tag name of nodrop might be good? I'm not sure since I think the 2 usecases are slightly different. On one hand, you want #728 for debugging issues like #707, but then on the other hand, we want this to be buildable on non-linux systems (like OpenBSD, macOS, etc).

I think maybe a good start is calling it something similar to your suggestions or mine for a build tag, and having a blurb in the README mentioning that alternative operating systems should try using the $BUILDTAG instead?

@favonia
Copy link
Owner

favonia commented Apr 15, 2024

@skarekrow Yes, the two use cases differ, but they have the same solution. ;-) I want #728 to work for both reasons. Perhaps we can focus on portability and generality. I feel nodrop is a bit unclear... any other suggestions inspired by portability?

Or, maybe the original version should be called noroot?

BTW, you should have run go mod tidy :-)

@skarekrow
Copy link
Contributor Author

skarekrow commented Apr 15, 2024

@favonia haha i just didn't commit that after I noticed, I did now :D

I'm honestly not sure, maybe just calling it generic and the other build can be linux for now? Let's you down the road add things that specific OS may call for that linux or the normal generic don't require themselves?

EDIT: If we decide to use noroot it's a bit of a misnomer since it can indeed still be ran as root, it's just the dropping of privilege that doesn't occur. Maybe we can add a quick UID check to make sure it's not ran as root, but allow that nag to be overridden with a cli flag? Seems like a bit more engineering but I suppose it does prevent accidentally running it as root.

@favonia
Copy link
Owner

favonia commented Apr 15, 2024

it can indeed still be ran as root, it's just the dropping of privilege that doesn't occur.

The DDNS updater will try very hard to change process UID and GID if it was 0 (usually the root). However, I do feel noroot might not be a good name. How about puid?

@skarekrow
Copy link
Contributor Author

skarekrow commented Apr 15, 2024

Well not without the droproot stuff we'd be dropping is what I mean. I'm not sure if puid is clear enough either :D perhaps just nocap?

@favonia
Copy link
Owner

favonia commented Apr 15, 2024

@skarekrow I have been thinking about that but it gave me the feeling of "no limit (cap)" instead of "not manipulating capabilities" 🤷 (I don't have a good idea, either.)

@favonia
Copy link
Owner

favonia commented Apr 16, 2024

How about setpuid for the current version that will take PUID?

@favonia
Copy link
Owner

favonia commented Apr 16, 2024

For easier migration, I slightly prefer some UI warning the user that it is no longer taking PUID and PGID. However, this can be done later if you don't have time. The priority now is to make it a build tag instead of deleting the code.

@massijay
Copy link

What about nocapdrop or norootdrop?

@skarekrow
Copy link
Contributor Author

skarekrow commented Apr 20, 2024

@favonia don't worry, i haven't dropped. Just not a lot of time right now irl. I will say though, with a cursory glance at the go build flags, it doesn't seem like it will do what we want without making droproot a complete separate package? I'm pretty new to go packaging in general, so perhaps I'm overlooking something obvious here too.

EDIT: Are you against me including a sample rc file for OpenBSD users to use as well? I got it working nicely in my setup, it's quite simple!

@favonia
Copy link
Owner

favonia commented Apr 21, 2024

Are you against me including a sample rc file for OpenBSD users to use as well? I got it working nicely in my setup, it's quite simple!

No, but please open a new issue or a new PR for that. 🙂

skarekrow added a commit to skarekrow/cloudflare-ddns that referenced this pull request Apr 21, 2024
This has a few prerequisites:
- Creating a `_cloudflare_ddns` user
- Add a `cloudflare_ddns` section to your `/etc/login.conf` (for env variables, this is noted in the accompanying `README` commit

Relies on favonia#733
skarekrow added a commit to skarekrow/cloudflare-ddns that referenced this pull request Apr 21, 2024
This has a few prerequisites:
- Creating a `_cloudflare_ddns` user
- Add a `cloudflare_ddns` section to your `/etc/login.conf` (for env variables, this is noted in the accompanying `README` commit

Relies on favonia#733
@skarekrow
Copy link
Contributor Author

@favonia OK all working locally here! The build fails when the tag is not supplied, as expected.

Ex:

  • GOARCH=amd64 GOOS=openbsd go build ./cmd/ddns -> Fail (current behavior)
  • GOARCH=amd64 GOOS=openbsd go build -tags nocapdrop ./cmd/ddns -> Builds and runds (new behavior)

@skarekrow
Copy link
Contributor Author

skarekrow commented May 3, 2024

I will say that I would prefer instead that we did this by OS, so if we ever need to extend specific support elsewhere. As in something like this:

checker_linux.go 
config_linux.go
drop_linux.go
drop_linux_test.go
drop_openbsd.go
drop_openbsd_test.go

Where we can then add a drop_freebsd for instance that may have some unique things it needs handled instead of a no-op. The downside is the default behavior would now need a build tag of linux for example. But as is, I stuck to what was discussed here and kept it simple.

@favonia
Copy link
Owner

favonia commented Jun 17, 2024

Sorry for the long radar silence. I couldn't meaningfully contribute to this project for months.

I will say that I would prefer instead that we did this by OS, so if we ever need to extend specific support elsewhere. As in something like this:

I like this idea! Maybe a combination of OS tags and nocapdrop, and we always read PGID and PUID. No UI changes are needed.

PS: Theoretically the nocapdrop is not needed if we use OS tags, because I already programed the testing carefully that the code should still run even if the system calls return errors. Who knows that libcap + LXC would crash the runtime (and there's nothing I can do). 😠

@favonia
Copy link
Owner

favonia commented Jun 17, 2024

@skarekrow Do you agree with the suggested approach? If so, do you want to implement it now? If you agree but you are busy now, do you mind me hijacking this PR?

@skarekrow
Copy link
Contributor Author

@skarekrow Do you agree with the suggested approach? If so, do you want to implement it now? If you agree but you are busy now, do you mind me hijacking this PR?

@favonia Hijack away!

This also likely allows other OS's besides Linux to use this, not tested by me however.

Introduces a new build tag called `nocapdrop` that when supplied will use the no-op instead.
@favonia
Copy link
Owner

favonia commented Jun 18, 2024

@skarekrow Could you test if go run ... (without -tags nocapdrop or any other tag) works for you out of box on OpenBSD?

@favonia favonia changed the title OpenBSD Support feat(droproot): support of non-Linux Jun 18, 2024
@favonia favonia changed the title feat(droproot): support of non-Linux feat(droproot): support non-Linux platforms Jun 18, 2024
@favonia favonia force-pushed the skarekrow/openbsd_support branch 3 times, most recently from 0502b2f to fe9f8bd Compare June 20, 2024 15:07
@favonia
Copy link
Owner

favonia commented Jun 20, 2024

Let me merge this first. :-) I need to test the Docker tagging.

@favonia favonia merged commit aaca559 into favonia:main Jun 20, 2024
8 of 10 checks passed
@favonia favonia mentioned this pull request Jun 21, 2024
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.

None yet

3 participants