-
Notifications
You must be signed in to change notification settings - Fork 7
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
API Key Authentication #99
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for opensubsonic ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Why do all the discussion outside of this forum where all the involved parties can interact? Anyway the repurpose of p to be an API key is 100% agnostic to the clients, supporting an header that also requires the username as a query param does not really makes sense from a security pov on the contrary. An extension would be needed if we replaced the auth for an API token that does change the API surface and that contains the information about the user and the perms. That also brings the question of the The other issue is the media key, while the need is real, it's a breaking change, as to use them we would need to not pass the actual auth to the endpoints. (And support should be for all endpoints, users can use stream to download, and will require the lyrics and ... should not lead to random results depending on the server). And as a reminder: OS API must not have should on API endpoint support or parameters or anything API surface related. It's either must, or there's an API endpoint that allows for clients to list what is supported and what is not. Let's keep the main purpose of OS: Clients must be able to have something deterministic when they use the OS API. |
|
The whole problem is that if nothing is properly codified then it's the return of the mess on the client side. Refactor of p:
TL;DR; there's no need for an apiExtension to refactor p to be an API key if the server wants, just a new auth error. Even older clients, will see an error and show an error to the user making them go to the server GUI to check. If you want to do more then a proper global solution is needed, new param apiKey that replace u/p/t ..., endpoint to know what the apiKey have access to, and eventually other information like a duration, limited to a media, ... mediaKeys: And if you want them you need to support them in all the endpoints related to playing those media including download, lyrics and everything. Why would an user be able to play a media but not access the lyrics when using those when it would work with another key. And if we add support for apiKey for the auth then we can reuse it for the media access by generating temporary apiKey limited to 1 media. That simplify things and is in line with a proper simple auth scoped API. But to resume, the very most important part will always be: No should and if there's scoping then the scoping needs to be codified for clients to know and warn the user properly. We do not want to return to random behavior between server with what we add, this break the most important contract of OS. TL;DR: Choice 1) allow server to enforce apiKeys as P by adding a new error code and an url for the user to create / manage them. Remaining question about "mediaKeys" what about scrobling ? Personal opinion, since oauth2 would probably be too much work for most servers / clients anyway, I think apiKeys as already suggested in the other threads is an acceptable auth system for apps and so think 2 or 3 are a nice solution as long term permission improvement and would solve a lot of current issues. |
By repurposing |
No, such repurpose as explained in point 1 would be server side only with a new error code and fully optional as it would not change anything at all from client side. Forcing changes to p is a breaking change and not compatible with OS. |
I am not sure to understand. With a given |
Well the obvious way is that if the server want to force apiKeys in p it check if the key exist and return the error? |
Just to add my 5 cents, using the What would actually improve the security, would be to have the concept of user session in the Subsonic API with a temporary session token. This way, the actual permanent API key (or password if you will) would be used only on one handshake message and most of the API communication would happen using a temporary token obtained as a result of the handshake. This would fix (or at least improve the situation with) what I see as the main problem of the current system: Quite often the users post their log files online with their bug reports, without detracting the |
One of the problems with expiring tokens that has been discussed is clients often rely on semi-permanent URLs for streaming tracks (and sometimes fetching images) - for example if the user has loaded up a play queue on a client, and they pause the app for weeks or months at a time, and then return to it, the playback URLs for the tracks in the queue should still be valid (unless explicitly revoked by the user) or else playback may fail. |
I would actually make the argument that that is not compatible with the current spec, since the current spec implies all logins are password-based, so clients will ask users for their "password" not "API key", which could potentially lead to a confusing UX. I suppose if Nextcloud music makes it very clear in the docs or when creating a key that it should be used instead of password for all Subsonic clients, that may clarify things a bit, but it would still be nice if the spec is designed such that the client knows when to ask for an "API key" vs "password" |
Why would the client store the playback URL of all the tracks on the queue? The URL may be formed based on the song ID the moment the playback starts (or is resumed). If then playing the URL (or any other API call) would fail with "session expired", then the client would need to start a new session and retry the operation. Of course that would require adding a small bit of extra logic but there are no free lunches. |
Many (most?) clients don't handle streaming the audio data themselves but pass the stream URL off to a 3rd party framework for playback (libmpv, etc). Clients that play gaplessly must at a minimum have the next-up URL loaded by the playback library ahead of time, alongside the current one. So if a users pauses a play queue for a long time, both the current and next playback URLs should remain valid in order to avoid playback errors. Also, with 3rd party playback engines it may not be possible to detect and handle a specific auth error differently than a generic "playback failed for some reason" error |
I think that mediaKey should be added as an extension of the user instead of a separated into an endpoint.
The streaming token can only stream media and all streaming URLs are generated using this token when it exists. Adding it to the user responses allows a client to build their own URL's with the token
This would all be transparent to older clients without the need for new endpoints. In regards to removing / deprecating / changing auth parameters a new error message to say "deprecated/removed" and then as a server I can allow or deny access to this auth method with a config option on the server. A blocked by default approach means people can still open their servers up and we can push people towards different methods. To extend though; I still think the simplest path is to move these parameters out of the URL and into headers, Ampache uses a Bearer token and supports all types of auth. This would allow a server to block auth from the URL (optionally) and require header auth instead. If you recommend header auth, implement a streaming mediaKey and allow blocking auth-related URL parameters on the server you've solved most of the problems of having reversible / plaintext auth in the URL. |
This is the server job to hide those usually, most tools either directly hide them or have a download redacted logs button.
This is not breaking in sense of API, from API pov a password or an API key does not change anything. With a new error we improve user feedback. (But in all cases I still think that a new auth with apiKey that replace u + p is better globally)
This was explained a lot of time already that this is not possible due to casting and some renderer limitations, it is not possible to have upnp devices send headers for example. If we start to mix a new enforced auth via headers and another one via keys for some endpoints, this becomes really messy and we should directly rewrite new endpoints. |
So @kgarner7 what do you prefer?
|
Sorry for the delay. I think the best (and simplest) way to move forward would be starting with option 2, an api key auth (just API key in header, or |
Ok so : Quick proposal to actually write :) It's a quick global view as busy right now. Generic:
If that works for you and have time to actually build the PR. I think this is more in line with the spirit of OS in regard to API definition, retro compatibility, predictability and everything |
Since I suspect scopes (if they even make it in) would take quite a while, I'll probably advocate for something more like:
Either way, I'll give a go updating the PR. Thanks! |
Unfortunately IMO in the way OS API is supposed to be build, to be able to have temporary token we need a way to know the limits of that token and so the scoping. BTW I forget but maybe also add an error 43 or whatever Invalid apiKey to differentiate from invalid login/password ? |
a2b37ad
to
81965c7
Compare
Thanks. @kgarner7 a few quick notes:
All those are mostly details, but I think there's one thing that needs discussion / validation by more clients / servers:
Any reason / arguments on those specific values? |
...I actually meant to remove key url and other parts |
Lol sorry some merge error then :) Will wait before commenting again :) |
Nah, that's just my bad for not paying attention for commit. Anyway, I've removed any reference of the v2 temporary key stuff (and keyUrl is redundant with changes to error) |
Ok so still the question about "an error 44 Invalid apiKey" to differentiate from the 40 invalid login/password. Else change the message / description of 40 to also cover apiKey ? And api-reference still talk about passing the Authorization header. If this is wanted it should be documented in the extension too. |
@@ -42,6 +42,7 @@ The following error codes are defined: | |||
| 41 | Token authentication not supported for LDAP users. | | |||
| 42 | Password authentication not supported. Use API keys | | |||
| 43 | Multiple conflicting authentication mechanisms provided | | |||
| 44 | Invalid API key or username | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the new apiKey version no more pass an username so it can't be a wrong user name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why I left username is if it's not required (and clients just specify api key), then there's no easy way to get the username. I would potentially be amenable to adding a new endpoint to turn a token into a username
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ho you left username mandatory I missed that. Well then it makes sense but won't it be a problem if we extend to v2 with apiKey that can be limited to a media and don't want to leak the username in the urls ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a new endpoint to exchange token for username (and other things (?) for v2) then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, could return the scope too ;)
I think better to reuse the apiKey for media than adding again something else that would not bring anything more.
content/en/docs/api-reference.md
Outdated
@@ -68,7 +68,7 @@ See [API Key authentication](../extensions/apikeyauth) | |||
|
|||
For servers that implement [API Key authentication](../extensions/apikeyauth), the recommended authentication is to use an API key. | |||
This is a token generated from the Subsonic server. | |||
It may either be passed in as `apiKey=<API key>`, or as a header `Authorization: Bearer <API key>`. | |||
It must be passed in in as `apiKey=<API key>`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double in in
@@ -161,6 +161,7 @@ The following error codes are defined: | |||
| 41 | Token authentication not supported for LDAP users. | | |||
| 42 | Password authentication not supported. Use API keys | | |||
| 43 | Multiple conflicting authentication mechanisms provided | | |||
| 44 | Invalid API key or username | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
Optional question but could there be a case in your needs for v2 where a key could not be tied to an user and we should define a default value? (Probably not but just checking) |
OpenSubsonic: | ||
- Extension | ||
description: > | ||
Add support for synchronized lyrics, multiple languages, and retrieval by song ID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad copy/paste?
If we are allowing API keys to exist that are separate from users (e.g. system account, say for just Jukebox/images), then I think for something like that it would make sense for the |
null is not valid, so if would be empty if not a well know value. For the if that's the question, I can't think of a reason but maybe some were evoked during the initial talk of this hence wanting to confirm. If not, then let's merge this. In the worst case we can amend with a comment the empty state that would not change the API surface unlike a well know value that could be already used by a server as username and we"d need to protect somehow. |
Derived from https://gist.github.com/kgarner7/33a6ab836528837b7ece44409ceb2d95, with numerous suggestions from @deluan and @jeffvli and discussion in Navidrome Discord.
In short:
p
to beAPI Key
, instead of password. Provides guidance for servers on providing these keys, as well as one new endpoint for client benefits for documentation.Media API key
, which uses temporary tokens generated by the server. While I believe this would be a benefit for both clients and servers (especially for things like rpc), I'd be fine backing out this change.Ultimately, this proposal isn't anything revolutionary (like using OAUTH2, refresh tokens/etc).
The primary goal is to provide a way for clients/servers to authenticate without either party having to store the user's passwords in a reversible fashion.
By repurposing
p
for this, clients require no functional changes (although some UX to noteAPI key
and point to URL), and server changes should hopefully be minimal.I also recognize other parts of the page should probably be updated with the new auth scheme (if accepted). This is a draft, after all.