daspartho / SpotiByeAds

Skip spotify ads by automatically restarting application when ad plays
GNU General Public License v3.0
284 stars 41 forks source link

Encryption of stored client secret. #60

Closed AnonymouX47 closed 3 years ago

AnonymouX47 commented 3 years ago

I suggest that the user be asked to enter a passphrase of at least 8 ASCII (to reduce complexity) characters. Then that can be used to encrypt the client secret when storing it and to decrypt it when reading it.

A simple cipher can be used, i'll say the XOR (maybe with a little extra like a random number) is ok... cracking the cipher with such key length will be quite a pain :)

michaeljgallagher commented 3 years ago

I wrote a quick little way this might be accomplished (with simple XOR encryption), but there's no (good) way in my implementation to check if the user provided the correct password to decrypt their credentials. The only way to know if they got their password wrong would be to see if Spotify throws an error back.

Normally a known string might be used to verify the correctness of a password, but with my shitty XOR cipher, this could just lead to the trivial decoding of the user's credentials.

Not making a PR yet, but feel free to look at the diff in my fork, and maybe you can use / improve on some of it. Or not. No difference to me really.

This also doesn't address the case where a user might already have stored credentials without a password, and updates their repo to use this (they'd have to delete credentials.json and re-enter their credentials the next time they run the script.

Not really making a good sales pitch for my work right now, but I've written too much NOT to post this so ok here goes

AnonymouX47 commented 3 years ago

Nice... That last line's the best 😄.

@michaeljgallagher I'll check out your implementation soon and get back to you. I actually already figured out a method that will make verification possible, just yet to implement.

As for the updating case, I suggest we use an entirely new file name "credentials.bin", since the new store can potentially contain non-printable characters and should be considered binary data.

The program will check for the existence of the JSON file, if found, it'll be loaded, the data will be encrypted and saved in the new file, then the JSON file can be safely deleted.

From this point, versioning of the project will start. So after some versions, the support for the JSON files will be removed and users that update after then will be required to re-enter their credentials.

What do you say about this?

michaeljgallagher commented 3 years ago

As for the updating case, I suggest we use an entirely new file name "credentials.bin", since the new store can potentially contain non-printable characters and should be considered binary data.

That was actually something I was running into issues with initially; I had originally been trying to use simple-crypt, but couldn't write the output to a .json since they were bytes. Hence why I implemented a slipshod version of XOR encryption :p

Your vision for this sounds great. Glad I didn't waste anyone's time by making PR for someone to just have to close it out. I took it on because I thought it would be a fun little exercise to implement some type of XORing algo.

AnonymouX47 commented 3 years ago

What a great spirit you've got, keep it up. Never let closed PRs weigh you down 😄.

Definitely, working with encryption is fun. As for the simple-crypt library, I'm not sure it's necessary. Yes, it'll provide added security but it'll just be another added requirement... too many dependencies for a single script and another quirk for those updating :).

Hopefully, I can work on my implementation today... I'll open a PR once I'm done, you could check it out.