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

Allow to bind to priveleged ports (i.e. 80/443) #5894

Merged
merged 3 commits into from Dec 12, 2021

Conversation

brianjmurrell
Copy link
Contributor

Changes
Add AmbientCapabilities=CAP_NET_BIND_SERVICE to the [Service]
section of the unit file to allow the server to bind to ports 80 and 443.

This is useful on stand-alone instances of JF, where standing up a separate "reverse proxy" is simply overkill just to allow "standard port" access for a web service.

Add "AmbientCapabilities=CAP_NET_BIND_SERVICE" to the "[Service]"
section of the unit file to allow the server to bind to ports 80 and 443.

Signed-off-by: Brian J. Murrell <brian@interlinx.bc.ca>
@brianjmurrell
Copy link
Contributor Author

Anyone care to review/comment?

@crobibero
Copy link
Member

I don't think this is something we want to allow by default.
I've been told that you can just run systemctl edit jellyfin.service where you can create overrides for everything and they survive upgrades.

@brianjmurrell
Copy link
Contributor Author

This is a very specific and targeted capability. It's not like setting a blanket permission such as SUID-root or anything. All it does is allows JF to bind to a port <1024. The harm in that, even if JF was exploited would be minimal IMHO.

@stale
Copy link

stale bot commented Sep 3, 2021

This issue has gone 120 days without comment. To avoid abandoned issues, it will be closed in 21 days if there are no new comments.
If you're the original submitter of this issue, please comment confirming if this issue still affects you in the latest release or nightlies, or close the issue if it has been fixed. If you're another user also affected by this bug, please comment confirming so. Either action will remove the stale label.
This bot exists to prevent issues from becoming stale and forgotten. Jellyfin is always moving forward, and bugs are often fixed as side effects of other changes. We therefore ask that bug report authors remain vigilant about their issues to ensure they are closed if fixed, or re-confirmed - perhaps with fresh logs or reproduction examples - regularly. If you have any questions you can reach us on Matrix or Social Media.

@stale stale bot added the stale Stale and will be closed if no activity occurs label Sep 3, 2021
@Shadowghost
Copy link
Contributor

Shadowghost commented Sep 3, 2021

I don't like this change. There is no reason or need justifying allowing to bind to privileged ports by default.
I disagree with your opinion that this is a very specific and targeted capability. The attack surface is way broader if you can bind to ports usually used by well known services just by modifying the Jellyfin config. There is a reason those ports are called privileged and need special permissions in the first place.
If people really want to run Jellyfin on port 80/443 they can just edit the service. Making it an explicit choice is the corret approach in my opinion.

@stale stale bot removed the stale Stale and will be closed if no activity occurs label Sep 3, 2021
@brianjmurrell
Copy link
Contributor Author

brianjmurrell commented Sep 3, 2021

The thing is, the supposed security enhancement of not allowing binding to lower ports is antiquated and pretty much entirely moot any more.

It came from a time when a computing resource was scarce and expensive and one usually used a well-known resource of some large organization. Preventing non-root from impersonating well-known services on such a system had value.

Now everyone has (multiple even) personal computing devices, that they are root on and can impersonate all they want.

(Apparently) even Windows doesn't impose this restriction any more, most likely as they have come to the same conclusion that it doesn't really matter any more.

You know, really, whatever. It really should not be this difficult to try to contribute to a project to make it easier to use.

Move "AmbientCapabilities=CAP_NET_BIND_SERVICE" to the "[Service]"
section of the optional "lowport" unit drop-in file and package that
drop-in in a separate, optionally installable jellyfin-server-lowports
subpackage.

This isolates the CAP_NET_BIND_SERVICE capability to only installations
that desire it.

Signed-off-by: Brian J. Murrell <brian@interlinx.bc.ca>
@brianjmurrell
Copy link
Contributor Author

brianjmurrell commented Nov 30, 2021

I've reworked this PR to put the additional capability into a separate subpckage so that it's only active/applicable to sites that want it and get it by installing the jellyfin-server-lowports RPM.

If somebody could please review, I would appreciate it.

@@ -12,14 +12,15 @@ Release: 1%{?dist}
Summary: The Free Software Media System
License: GPLv3
URL: https://jellyfin.org
# Jellyfin Server tarball created by `make -f .copr/Makefile srpm`, real URL ends with `v%{version}.tar.gz`
# Jellyfin Server tarball created by `make -f .copr/Makefile srpm`, real URL ends with `v%%{version}.tar.gz`
Source0: jellyfin-server-%{version}.tar.gz
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the additional % accidentally added?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. It's an escape. Macros get expanded, even inside comments which is typically not what is desired. The RPM linter will complain about them.

@cvium
Copy link
Member

cvium commented Nov 30, 2021

Why another package? Can't people just edit the systemd service with a small override?

@brianjmurrell
Copy link
Contributor Author

Why another package? Can't people just edit the systemd service with a small override?

Because my target audience is real end users, not engineers or hobbyists, etc. The kind of end users that just want a media server that installs straightforwardly and don't want to have to scour websites, and readmes, and forums, and help boards, etc. for instructions on how to fiddle their installation to have it act "normally" (i.e. web services normally are on ports 80 and 443 -- so normal that the ports don't even need to be part of the URL -- not unusual high ports which need specifying in the URL).

My goal is the highest standards of ease-of-use.

I am happy that a lot of Jellyfin is already in that camp. Other areas less so, but if I can make any part of running it on regular/usual ports all that much easier, why not?

I might ask you the contrary, why not a package that somebody can easily install without having to know how to muck with systemd? What is the problem with a package that enables more ease-of-use?

The previous rejections on this PR were that it was opening up a security hole for people that didn't need/want the feature. I have resolved that now. It's now completely optional and targeted at people who only want the feature.

@brianjmurrell
Copy link
Contributor Author

Any more review/comments/etc.?

@brianjmurrell
Copy link
Contributor Author

Could we please get further review/comments on this PR? @joshuaboniface perhaps you have some thoughts?

Copy link
Member

@joshuaboniface joshuaboniface left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with this, makes sense to me.

@crobibero crobibero merged commit 0e8c97e into jellyfin:master Dec 12, 2021
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

5 participants