search

jpochyla / psst

Fast and multi-platform Spotify client with native GUI
MIT License
8.52k stars 218 forks source link

Store user credentials in the system keychain #32

Closed auravoid closed 3 years ago

auravoid commented 3 years ago

Saving the authentication credentials to the disk in a config file is great, but saving them raw is a bit insecure. It would be really helpful if the passwords get encrypted for user's privacy and safety.

jpochyla commented 3 years ago

I understand the sentiment, but we don't really have any key to encrypt with, and it's not really different from all the other stored passwords in other apps. One possible solution would be to use the platform capabilities for this, i.e. Keychain Access on Mac, through the keyring crate: https://crates.io/crates/keyring. PRs welcome!

SpyrosRoum commented 3 years ago

Hey, looking at the keyring crate, this looks easy enough so I'm gonna give it a try!

SpyrosRoum commented 3 years ago

I have one question @jpochyla How do you want to handle the transition from "Password stored in config file" to "Use keyring"?

I am thinking for the next release we make the password field in Config to only be de-serializable but not serializable so we can read it but not write it. Additionally we will save it in the keyring if it's not already there and we will delete it from the file. In the next release we will tell serde to completely skip the password and we can remove some code since we will now assume the the password is either in the keyring or nowhere.

This way we give a transactional period for user and we will only have issues if a user skips the next release. Alternatively we can tell serde to skip the password and just force the user to tell us the password again so we can save it in the keyring.

Thoughts?

jpochyla commented 3 years ago

Thoughts?

  1. I think the choice should be on the user's side, i.e. a checkbox in the credential dialog.
  2. There's a chance that we can avoid storing the credentials at all, through authenticating with the "reusable credentials" that the Spotify session gives us, after the first login. I haven't investigated this much, though. It's possible there's some TTL involved.
BachoSeven commented 3 years ago

@jpochyla Perhaps we could take inspiration from the (recent) switch that ncspot has made, from storing plaintext(albeit base64-encoded) credentials to a librespot token (see https://github.com/hrkfdn/ncspot/commit/d6db7a54d634e7d5015ee03daddb939d7c225668).

Original ncspot issue for reference: https://github.com/hrkfdn/ncspot/issues/447

jpochyla commented 3 years ago

@BachoSeven Yup! master is now storing the token instead of user credentials. Unfortunately it means users need to re-authenticate.