feat: Replace Basic Authentication with JWT Tokens, Added Login Page

[Hazer]

Description

Overtaking of https://github.com/LizardByte/Sunshine/pull/2252, as I can't force push my rebased changes there.

• [x] Fix conflicts
• [x] Apply theme options
• [x] Handle Login redirects, instead of always returning to path /
• [x] Improve autocomplete
• [ ] Move auth logic to a class(?) this protocol will allow for alternative auth implementations, and testing
• [ ] Avoid logging out after applying settings (how to do it safely?)

Screenshot

Original at: https://github.com/LizardByte/Sunshine/pull/2252 TODO

Issues Fixed or Closed

Type of Change

• [ ] Bug fix (non-breaking change which fixes an issue)
• [x] New feature (non-breaking change which adds functionality)
• [x] Breaking change (fix or feature that would cause existing functionality to not work as expected)
• [ ] Dependency update (updates to dependencies)
• [ ] Documentation update (changes to documentation)
• [ ] Repository update (changes to repository files, e.g. .github/...)

Checklist

• [x] My code follows the style guidelines of this project
• [ ] I have performed a self-review of my own code
• [ ] I have commented my code, particularly in hard-to-understand areas
• [ ] I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

[Hazer]

FYI @ReenigneArcher @TheElixZammuto @cgutman let's continue discussion here.

@TheElixZammuto I invited you to my fork, so you can push something yourself if needed. This way we can iterate fast and I can get your amazing work merged.

[codecov[bot]]

Codecov Report

:x: Patch coverage is 0.97087% with 102 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 9.52%. Comparing base (0e153cf) to head (07967ec). :warning: Report is 422 commits behind head on master.

Files with missing lines	Patch %	Lines
src/confighttp.cpp	0.97%	42 Missing and 60 partials :warning:

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #2995      +/-   ##
=========================================
- Coverage    9.57%   9.52%   -0.06%     
=========================================
  Files          73      73              
  Lines       13616   13703      +87     
  Branches     6263    6332      +69     
=========================================
+ Hits         1304    1305       +1     
- Misses       9737    9770      +33     
- Partials     2575    2628      +53     

Flag	Coverage Δ
Windows	5.07% <0.00%> (-0.04%)	:arrow_down:
macOS-12	10.27% <1.06%> (-0.09%)	:arrow_down:
macOS-13	10.18% <0.00%> (-0.09%)	:arrow_down:
macOS-14	10.48% <0.00%> (-0.10%)	:arrow_down:

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

Files with missing lines	Coverage Δ
src/confighttp.cpp	1.16% <0.97%> (-0.01%)	:arrow_down:

[ns6089]

Please correct me if I'm wrong, but won't this require new login prompt each time sunshine settings are "applied"?

[Hazer]

Please correct me if I'm wrong, but won't this require new login prompt each time sunshine settings are "applied"?

@ns6089 You're completely right, just tested it myself and had to login again.

[ns6089]

So on the pro side we have better UI and possibly better compatibility with browser password managers. On the con, more complicated api and this minor annoyance with repeating login prompt.

@TheElixZammuto (or anyone who knows), you mentioned "it's more compatible with password managers" in the original PR. Did you run into problems with basic auth in a particular browser?

[Hazer]

@ns6089 that apply thing can be fixed. What do you mean by more complicated API? JWT Auth is kinda almost the standard nowadays, and we can further provide customization in the future like due date expiring sessions, or maybe allow for custom providers. What will you miss from Basic Auth that is useful to you right now?

Basic Auth won't work with autocomplete in many browser's password manager APIs, or works but won't allow third-party password managers to fill in, only the built-in browser one, that many people don't use. It is an annoyance I want to fix too.

[ns6089]

@Hazer I don't personally use web api for anything, just wanted list all possible pros and cons. My only concern that we may be fixing the problem for 1% of users (custom password managers) by introducing a new problem to 99% of users (that recurring login prompt). If it can be fixed, I'm all for it.

[ns6089]

Also a random thought. Maybe we should consider dropping off username from the authentication scheme leaving only the password. It was required as part of http basic auth, but JWT gives us more freedom.

[TheElixZammuto]

So on the pro side we have better UI and possibly better compatibility with browser password managers. On the con, more complicated api and this minor annoyance with repeating login prompt.

@TheElixZammuto (or anyone who knows), you mentioned "it's more compatible with password managers" in the original PR. Did you run into problems with basic auth in a particular browser?

Not personally since I don't use my password manager for Sunshine, but this feature was asked over Discord and GitHub a couple of times

[TheElixZammuto]

As for keeping the auth safely, the only way is to store the jwt_key in the state file instead of just in memory, in a default config only admin users should be able to access it. The problem lies in the fact that the we cannot revoke a session when a session gets compromised. Maybe we can add an action to reset the key in the troubleshooting section?

[TheElixZammuto]

Also a random thought. Maybe we should consider dropping off username from the authentication scheme leaving only the password. It was required as part of http basic auth, but JWT gives us more freedom.

I would prefer to keep it. Users are already aware of the combo and they are used to that, also we can use the username in the future if we want to add multiple users/multiple access levels

[ns6089]

As for keeping the auth safely, the only way is to store the jwt_key in the state file instead of just in memory, in a default config only admin users should be able to access it. The problem lies in the fact that the we cannot revoke a session when a session gets compromised. Maybe we can add an action to reset the key in the troubleshooting section?

Can't we just use symmetrical keys so leaking this key doesn't compromise security without knowing client's secret?

[ns6089]

Sorry, assymetrical.

[sonarqubecloud[bot]]

Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[Nonary]

We don't have to replace basic authentication, we can still add a login page just before landing on the index if it's not authenticated instead of asking for it via the browser.

I'm not really a fan of killing off basic authentication entirely as a prerequisite for a simple login page so the browser can autofill it.

I do agree we should harden security at some point, but if you make it annoying then it's not really going to be effective and encourage even more bad behaviors on the user part.

[ReenigneArcher]

We don't have to replace basic authentication

In my opinion, we do need to. Since credentials are sent over plain text, and access to Sunshine can be an easy vector to gain full control of a system, we really need to stop using it. Sooner, rather than later.

Additionally, since Moonlight clients set the pin, it makes it very easy to hardcode a pin and begin a stream. It's pretty trivial to intercept basic auth credentials, and most users are not security/network experts. We shouldn't leave them vulnerable.

We did a search previously using GitHub code search and didn't find anything using basic auth through our API.

I've also discussed with @Hazer a way to set an API token that can be used for automation/third-party integrations... like almost all other self hosted applications are doing.

[Nonary]

We don't have to replace basic authentication

In my opinion, we do need to. Since credentials are sent over plain text, and access to Sunshine can be an easy vector to gain full control of a system, we really need to stop using it. Sooner, rather than later.

Additionally, since Moonlight clients set the pin, it makes it very easy to hardcode a pin and begin a stream. It's pretty trivial to intercept basic auth credentials, and most users are not security/network experts. We shouldn't leave them vulnerable.

We did a search previously using GitHub code search and didn't find anything using basic auth through our API.

I've also discussed with @Hazer a way to set an API token that can be used for automation/third-party integrations... like almost all other self hosted applications are doing.

The data isn't in plain text; it's encrypted. You can't just inspect traffic on a machine and view the contents of the data packets unless the certificate Sunshine uses is the same for every device (which I doubt it is).

As for the risks with basic authentication, they generally require the system to be exploited first. On its own, it's not super risky, but it lacks any protections if someone does get hacked.

Also, implementing JWT authentication poorly can be even less secure than basic auth.

[ClassicOldSong]

@Nonary The biggest problem is that no JS methods can log in with basic auth without manually setting the Authorization header. That means it needs to be set manually on every request made to the server by JS if you still keep basic auth as a login method, and the worse part is that normal resources requests wont't get this header automatically applied(but cookies could) unless logged in through browser's prompted login dialog, so they'll be rejected in the first place, before the page and JS can be loaded.

[Nonary]

@Nonary The biggest problem is that no JS methods can log in with basic auth without manually setting the Authorization header. That means it needs to be set manually on every request made to the server by JS if you still keep basic auth as a login method, and the worse part is that normal resources requests wont't get this header automatically applied(but cookies could) unless logged in through browser's prompted login dialog, so they'll be rejected in the first place, before the page and JS can be loaded.

That's not really a problem because we can either use a decorator function or a tool like Axios to add an interceptor to every request for basic authentication.

You can still create a login page for rejected requests. At my job, I wrote an authentication process using OpenID that works in a similar way, except it refreshes tokens in a hidden frame instead of asking for credentials. Either way, it's not too hard to set up.

As for not landing on a page before JavaScript runs, that's also easy to solve, and we're already doing that. When you log into Sunshine for the first time, you're not prompted for browser credentials; instead, you're asked to create an account.

[Nonary]

So the real question here is if I spend the time to get the login page to work and still keep basic auth will that pull request even be considered? I'd rather not spend time fixing it if it won't be considered as an option basically.

[ClassicOldSong]

You're not getting my point. Even after you "logged in" with JavaScript, the resources are still not authorized for the browser to fetch, that means users won't be able to load other pages other than the login page. Or you'll just allow any static resources to be accessible by anyone.

Authorize with cookies can solve this problem.

[Nonary]

You're not getting my point. Even after you "logged in" with JavaScript, the resources are not authorized for the browser to fetch, that means users won't be able to load other pages other than the login page. Or you'll just allow any static resources to be accessible by anyone.

Authorize with cookies can solve this problem.

And as I explained I do know how to program it in such a way where it can return back unauthorized and not prompt the user in the browser but instead have it redirect to a login page.

The prompt is caused by the server side, if we omit the Authenticate header in the response, the browser won't ask for credentials.

Cookie or not has no difference to this problem.

[ClassicOldSong]

I don't want to exaplain again.

If you don't use cookies, there's no other way than the prompted basic auth to allow static resources to get permission checked.

[Nonary]

I don't want to exaplain again.

If you don't use cookies, there's no other way than the prompted basic auth to allow static resources to get permission checked.

What you're saying is technically correct, but the static resources typically don't need to be authenticated. That doesn't mean it's impossible, there are ways to do this as well but I find it completely unnecessary in our use case.

It's only the server sided calls that need authentication. There's nothing inherently dangerous in the static files, we are not hard coding a password in plain text in these files.

This is a single page application, meaning there's only like three files at the end of the day, permission checks on a file by file basis wouldn't really work on an SPA structure anyway.

[ClassicOldSong]

Actually it's not a SPA at the moment. It's the most cursed way of Vue's usage I've ever seen.

And as users usually won't set up this server behind a WAF, it's still important for static resources to be authorized in case of DDoS.

I know there're lots of ways to get it done like using a service worker, but in this application it's totally not worthy of using these techniques to solve basic needs.

[Nonary]

Actually it's not a SPA at the moment. It's the most cursed way of Vue's usage I've ever seen.

And as users usually won't set up this server behind a WAF, it's still important for static resources to be authorized in case of DDoS.

I know there're lots of ways to get it done like using a service worker, but in this application it's totally not worthy of using these techniques to solve basic needs.

When it comes to security, I'm not too concerned about denial of service (DoS) attacks or vulnerabilities that require an exploit to trigger. Even adding authentication to static resources doesn’t magically fix a DDoS issue. A malicious actor can still flood the server with authentication requests, and despite safeguards, enough failed requests can overwhelm the system. Just rejecting traffic can lead to a DoS situation.

This has been my view all along: if the concern is a specific security issue, it takes more than just making static files require authentication. It's a lot of work, and frankly, we won’t see it implemented until AI tools automate much of the process because the effort involved doesn’t match the value it brings compared to other features.

Also, the only real benefit would be against third-party resellers who take our code, sell cloud services, and contribute nothing back to Sunshine. While this is legal due to licensing, it’s hard to justify investing significant time and effort to solve a problem that mainly helps cloud providers. It’s not very motivating.

[ClassicOldSong]

I believe this PR is more about user ergonomics rather than security improvements for vendors.

[Nonary]

I believe this PR is more about user ergonomics rather than security.

Yes, that's part of my concern. We're replacing a functioning authentication system with JWT because it's perceived as more secure. However, since JWT is a more complex form of authentication, I believe it introduces more risk of security vulnerabilities compared to keeping the current basic authentication. Adding a new authentication system isn't straightforward; it requires a lot of safeguards.

The original goal of this PR was to allow people to use password managers to log into Sunshine. What I'm saying is that we can achieve this with minimal changes. There's no need to switch to JWT, and contrary to popular belief, basic authentication doesn't store passwords in plain text. It is secure and valid, though it lacks additional safeguards if a password is stolen or if someone gains admin access to the machine. But in that case, the system is already compromised, so it doesn't really matter.

My proposal is to not implement JWT in this PR. That should be a separate PR, where we thoroughly review it and ensure all best practices are followed to avoid potential exploits. For now, this PR should focus on simply adding a functional login page without changing the authentication mechanism. This approach carries far less risk of security issues and avoids breaking changes.

[ReenigneArcher]

Please move this noise to discord or somewhere else. This space is for code reviews.

[Nonary]

Please move this noise to discord or somewhere else. This space is for code reviews.

Thats also something I am going to have to disagree here. Everything discussed in this PR is relevant to the situation, discussing it on discord instead is a bad idea since you can't easily pull the information. You'll come back to this PR and see no discussions about the valid concerns above and maybe you would have accepted it.

Also, this PR is a draft, which by definition, means it is not ready for a code review. If you want it to be at the code review status, then we should discuss with the owner of this PR to submit it for review status.