CERT-Polska / mwdblib

Client library for the mwdb service by CERT Polska.
https://mwdblib.readthedocs.io/en/latest/
MIT License
40 stars 13 forks source link

API authentication is broken when using the cli #90

Closed v-rzh closed 1 year ago

v-rzh commented 1 year ago

Storing the API key with mwdb login command silently fails:

[user@shell]$ mwdb login -a "<valid api key>"
[user@shell]$ mwdb search "name:file.exe"
Error: Not authenticated. Use `mwdb login` first to set credentials.
Aborted!

First half of the issue lies in how store_credentials method of APIClientOptions class validates its arguments. The method will silently fail if self.username is not set. This is likely due to keyring's set_password method requiring some username to store any secret, even an API key.

The second half is in the login command handler (login_command in mwdb/cli/login.py). This function incorrectly calls set_api_key as a member of the mwdb object rather than its member api (which is an APIClient instance). However even if it correctly invokes set_api_key, the API key won't be stored due to the aforementioned issue in store_credentials. Thus login_credentials has to ensure that the username is set prior to storing the API key.

v-rzh commented 1 year ago

Closing my pull request. Grabbing the username from the token makes much more sense.

psrok1 commented 1 year ago

Yeah, but I'm still glad that you have provided detailed explanation. Thanks for that and sorry for not responding on your PR first :grinning:

Actually that username isn't required by MWDB itself, but we use keyring for storing secrets and it accepts only (username, password) pair. We will try to fix it in bugfix release.

v-rzh commented 1 year ago

Yep! Kinda wish keyring's API supported storing single secrets. A little off topic but, @psrok1 what is the preferred method of contribution? I normally just fork off the target repo and create pull requests that way. Is this not desired?

psrok1 commented 1 year ago

Yes, it is OK. @Repumba first thought was to apply changes to your existing PR but finally he decided to make a new one. So don't worry about that comment.

v-rzh commented 1 year ago

Gotcha :D

psrok1 commented 1 year ago

Released: https://github.com/CERT-Polska/mwdblib/releases/tag/v4.4.0