Skip to content
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

Store login credentials in encrypted form #81

Open
taktoa opened this issue Mar 10, 2018 · 15 comments
Open

Store login credentials in encrypted form #81

taktoa opened this issue Mar 10, 2018 · 15 comments
Labels
status:desirable Issue described as desirable by owner or community status:discussion type:feature

Comments

@taktoa
Copy link

taktoa commented Mar 10, 2018

I noticed that after running mal login, ~/.config/mal/myanimelist.ini contains my MyAnimeList login credentials in plaintext. I think there should at least be an option to store it as a GPG encrypted blob (so whenever I run mal it will ask the GPG agent to decrypt the blob, prompting me for a password to my GPG private key every so often). I may take a look at implementing this myself.

@ryukinix
Copy link
Owner

ryukinix commented Mar 10, 2018

Yes, this is indeed a problem, I probably warn that in the README (or in https://lerax.me/mal). The problem overall is: there is no Oauth or other more secure way to communicate our credentials with the currently MAL API. It's pretty horrible.

Probably you can implement that, but sincerely I'm not very happy to depend to gpg, the GNU PGP client, to mal works. I use gpg a quite often, but I know that some people don't use it and some of them don't even understand how to deal with PGP keys. I don't want obey people get into all the mess because the MAL API fault.

However, yes, I care about my own credentials and as well the other users of mal. If would possible implement that as optional behavior... I don't know, probably mal/config.py stuff? This is stored at ~/.config/mal/myanimelist.ini. That way people that have a nice setup of GPG can use it and people that don't want to worry about gpg can just ignore it (btw, it's a quite surprise for me, but even don't be supported, there is some people trying to use this client on Windows... so think a little adding gpg dependency for windows users :v ).

I have some hope to MyAnimeList release a better API with oauth in some day... but probably I'm just dreaming a lot about it.

But, anyway, it's free software. If this really bother you, you can fork freely, implement it as default behavior and use your own version.

Thanks for your thoughts @taktoa, this is a important thing. Since you implement a nice optional feature don't being the default behavior, I probably can merge it on master (or at least in some other branch of the repository).

@kutsan
Copy link

kutsan commented Mar 11, 2018

Instead of living with GPG-only world, why not make an eval option for ini file that can be used while reading our passwords?

[login]
username = kutsan
passwordeval = gpg --no-tty --for-your-eyes-only --quiet --decrypt ~/.config/mal/secret.gpg

That way, we -even so-called Windows users- can define whatever command we want.

Besides, as default, other than storing as a plain text file, I'm even okay with ROT13. It just shouldn't be that easy to get. Also, mal definitely needs to chmod 600 to that file.

I have some hope to MyAnimeList release a better API with oauth in some day... but probably I'm just dreaming a lot about it.

They need to rebuild the whole website from scratch. There are a lot of things that wrong about it. API is just one of them. But, well, at least there is an API.

@evanjs
Copy link
Contributor

evanjs commented Mar 11, 2018

This might also be an option: https://github.com/Netflix-Skunkworks/go-jira/blob/master/README.md#keyring-password-source

Not sure what we'd do about Windows, though.
Looks like credential locker is supported here: https://pypi.python.org/pypi/keyring

@ryukinix
Copy link
Owner

ryukinix commented Mar 17, 2018

We can implement that as well:
tea-tiny-encryption-algorithm.pdf

Which will be done with no costs about relying on encrypt software, external dependency or hacky configurations for evaluating encrypt commands.

@kutsan
Copy link

kutsan commented Mar 17, 2018

@ryukinix I really want to have passwordeval functionality. Do you have any plans on that? Can I create another issue?

@ryukinix
Copy link
Owner

ryukinix commented Mar 17, 2018

I don't have any plans to implement something as passwordeval or using gpg explicitly (though passwordeval is indeed better than that). I think that can be a minimal solution, but too dependent to config stuff... I don't like very much that. I would prefer something like TEA, which can work fine by default without the user knows about it because we'll implement all the mechanism of encrypt/decrypt (it's really simple).

Moreover, reducing always the number of dependencies and config we'll have more control of our software... we have until a bug with argparse (#79), this is quite awful.

But, if you really want this, and you want implement this, you are free to implement that by yourself. Since you show your point and we like it, we can merge it. Otherwise, I don't have plans on that, as I said.

I think this issue is already sufficient to discuss about that.

@ryukinix ryukinix added the status:desirable Issue described as desirable by owner or community label Mar 17, 2018
@ryukinix ryukinix mentioned this issue Mar 18, 2018
6 tasks
@kutsan
Copy link

kutsan commented Mar 18, 2018

My point was provide a simple algorithm as a default security model and apart from that let them have passwordeval config if they want more advanced security. Obviously, no one wants passwordeval as a default option.

@ryukinix
Copy link
Owner

My point was provide a simple algorithm as a default security model and apart from that let them have passwordeval config if they want more advanced security. Obviously, no one wants passwordeval as a default option.

Okay, since this your point and I get it right now, I'm ok with that. Probably, if I implement the tea, I'd add it as a especial tag like passwordeval=@tea to avoid conflicts on namespace if for some weird reason have a tea binary.

Are you ok with that, @kutsan? Probably everyone will be happy with that... 🤔 I guess.

@ryukinix
Copy link
Owner

ryukinix commented Mar 18, 2018

I recommend read about tea that I attached here, it's nice and simple. A think as default can be great since we can embed directly without relying on conf/dep of environment of the users, with that we don't have problems with windows users as well, since this can implemented as portable really easily.

@ryukinix
Copy link
Owner

Do you have any plan to help implement that @kutsan ? Or anyone on this thread.

@ryukinix
Copy link
Owner

I can try look implement the TEA by myself, the passwordeval way a help can be useful. We need make that a little flexible to change, although seems a simple thing to do.

@ryukinix
Copy link
Owner

password_eval btw I think it's better since we already have date_format

@kutsan
Copy link

kutsan commented Mar 18, 2018

password_eval btw I think it's better since we already have date_format

Yes, you are right. I agree with that.

Probably, if I implement the tea, I'd add it as a especial tag like passwordeval=@tea to avoid conflicts on namespace if for some weird reason have a tea binary.

I think password_eval should be only contain commands to be executed. Why bother it with @tea thing since there is only one algorithm to select?

IMO make password_eval empty as default, it would mean TEA will in charge for security.

[login]
username = kutsan
password_eval =

If I see that option and read the docs about it, I can fill it with my own commands to activate that functionality?

[login]
username = kutsan
password_eval = gpg --no-tty --for-your-eyes-only --quiet --decrypt ~/.config/mal/secret.gpg

I recommend read about tea that I attached here, it's nice and simple.

I checked out and got the general idea. It looks cool, thanks for sharing!

Do you have any plan to help implement that @kutsan ?

I wish I could but as I said before I don't have any motivation to learn Python right now. :(

@ryukinix
Copy link
Owner

ryukinix commented Mar 18, 2018

IMO make password_eval empty as default, it would mean TEA will in charge for security

Ok, this makes sense and it's even better don't mention tea on the config, can facility hacking.

If I see that option and read the docs about it, I can fill it with my own commands to activate that functionality?

Yes, this indeed is something that the user can look at it and think "hey, there is something strange here... Should I look to the docs". Or they will try so weird thing and break the software and will need read the docs anyway :v -- btw, we need improve this docs... https://mal.readthedocs.io is a lie hahah I just used pandoc to translate the README! So evil... we need change that.

I wish I could but as I said before I don't have any motivation to learn Python right now. :(

Yes... I remember that. That is okay. I probably will delay this a little since I'm pretty busy on college and with a new job... Actually I should being on the bed right now xD ~ I have a cold.

Thanks for all this discussion, I hope this fill your use-case @taktoa

@kutsan
Copy link

kutsan commented Mar 18, 2018

I probably will delay this a little since I'm pretty busy on college and with a new job... Actually I should being on the bed right now xD ~ I have a cold.

That's okay too. Take care of yourself. ><

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:desirable Issue described as desirable by owner or community status:discussion type:feature
Projects
None yet
Development

No branches or pull requests

4 participants