Skip to content
This repository has been archived by the owner on Sep 8, 2021. It is now read-only.

Add separate password for Subsonic API #1544

Closed
wants to merge 7 commits into from

Conversation

fxthomas
Copy link
Contributor

This series of commits adds a single separate password for the Subsonic API:

Screenshot from 2020-02-18 23-26-06

This password ("REST token" in the code for now, should we use "Subsonic API password" instead?) is empty by default, but can be regenerated or cleared in the "Personal Settings" page.

By default, existing users will see no changes:

  • Either the regular user password or this token (if not empty) can authenticate requests to the Subsonic API.
  • Other requests (web UI) cannot use this token and must continue to use the user password as before.

Following #69, the t+s login using the user password can then be removed as a breaking change in 11.0 alongside the introduction of hashed user passwords. After this point, users with apps using the t+s login will need to change their password to use the REST token.

Note that another, more complex proposal was discussed in #69 and implemented in another PR in another fork. I think the issues pointed to by #69 need at least a minimal solution in place for 10.6 before we start breaking things in 11.0, hence this PR.

@fxthomas fxthomas added this to the 10.6.0 milestone Feb 18, 2020
@fxthomas fxthomas added security in: rest Issues in the REST API. labels Feb 18, 2020
@randomnicode
Copy link
Contributor

randomnicode commented Feb 21, 2020

I'm not seeing how this PR addresses any issues posted in #69 at all. In fact, I think this is a step backwards because it now provides an additional attack vector (a REST token), in addition to the passwords that can be compromised.

The original passwords are still not stored securely. And now a new cred (the REST token) isn't stored securely either, which leads to a double fail. Everything is stored openly.

#69's explicit issue (it's in the title itself) is that credentials are stored insecurely (in the db). How is this PR addressing that? The original creds are still insecure, the new creds are also now insecure, further compounding the problem. You may just issue another account and simulate the effects of this PR without the need for any code.

In addition, third party creds remain openly stored.

I think the issues pointed to by #69 need at least a minimal solution in place for 10.6 before we start breaking things in 11.0, hence this PR.

Hmm, the bulk of the other PR (airsonic-advanced/airsonic-advanced#63) is exactly so it won't "break" things and retain backwards-compatibility (and thus does not merit a major version change like 11.0). I switch back and forth between the branches frequently without needing db resets, which is the point of it. A clean break (necessitating a major version change) would've made for a much smaller changeset, but I thought it better to have people on the 10.x versions secured too. Moreover, the other PR actually stores things securely, thus addressing the pivotal focus of #69.

@muff1nman
Copy link
Contributor

muff1nman commented Feb 22, 2020

Rather than add another password, maybe we should just integrate with open id connect instead. That way we can move to not storing anything. If there's interest in this I can see about a POC.

@randomnicode
Copy link
Contributor

Rather than add another password, maybe we should just integrate with open id connect instead. That way we can move to not storing anything.

And lose every person who doesn't have access to the internet or doesn't actually want to use or create an account with an online OAuth provider. This includes internal network or LAN use cases.

I think OAuth/Open ID is an additional auth method you can provide, but making it the sole mechanism of auth is going too far.

@nx6
Copy link

nx6 commented Feb 23, 2020

Rather than add another password, maybe we should just integrate with open id connect instead. That way we can move to not storing anything.

That's seems a rather odd suggestion to make when you have (had?) a project specifically about removing dependence on external services from Airsonic.

It's annoying enough when I can't use my Plex server normally due to "cloud issues".

@fxthomas fxthomas removed this from the 10.6.0 milestone Mar 15, 2020
@eharris
Copy link
Contributor

eharris commented Mar 16, 2020

I strongly disagree with adding an additional external dependency to Airsonic AND making it a mandatory requirement.

@fxthomas
Copy link
Contributor Author

I would... say the same thing, especially since this would mean I would have to install and maintain an OpenID provider on my local network (because I sometimes use Airsonic offline).

I'm putting this on the back burner to be fair, so let's close this.

Anybody who wants to do something about this issue feel free to pick this or the implementation in airsonic-advanced/airsonic-advanced#63 and merge it!

@fxthomas fxthomas closed this Mar 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
in: rest Issues in the REST API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants