ericsomdahl / python-bittrex

Python bindings for bittrex
MIT License
585 stars 283 forks source link

Make decrypt method read api_key and api_secret from secrets.json #122

Open jannecederberg opened 6 years ago

jannecederberg commented 6 years ago

The decrypt method on the Bittrex class was not actually reading credentials from secrets.json. This commit adds that.

Justification: I don't currently understand how the decrypt method is supposed to be used. The way I understand this and would personally use python-bittrex in production is as follows:

  1. I would first create a secrets.json file (using for example the static encrypt function or preferrably a dedicated secrets.json creator). This could be done anywhere, for example offline.
  2. I would then upload the secrets file to an Internet-connected machine such as a VPS server and start running a trading script that utilizes python-bittrex as a dependency.
  3. When starting up the trading script, the trading script utilizing python-bittrex would call the decrypt method, I would provide the password for decripting credentials from secrets.json and decrypt method then carries on and the trading script can continue doing its thing.
  4. Now if the machine that the trading script was on gets compromized, the api_key and api_secret are not leaked.

In summary: this commit aims to make the above a reality.

Suggested further improvements: standalone secrets.json generator

jannecederberg commented 6 years ago

Of course if a machine is compromized sufficiently badly, the attacker can read live memory and obtain _apikey and _apisecret that way but if attack surface is limited for example to the ability of reading files from disk (say due to some other compromised webapp), then using encryption for credentials will at least make it more work for an attacker to obtain the key and secret.

And forgot to say this initially: thank you for the great work on the module!

ericsomdahl commented 6 years ago

Sorry @jannecederberg , I have been absent for a while.

Personally I do not use the decrypt functionality provided by this module. And in the interest of operation security I will not say what I do to replace it. 😃

To me your change seems reasonable. My only concern is how it might impact other users of the current scheme. I have no sense of how much it is used (beyond the single user who contributed it).

My inclination is to merge and to bump the bittrex library version.

lancechua commented 5 years ago

Pretty late to this conversation, but I was the contributor for the encrypt/decrypt portion. First, I have to say I am not really a security expert.

The original intent was to just have the option of encrypting the contents of secrets.json. A design goal was to minimize the deviation from how the package would normally work without it. With this scheme, the difference between using an encrypted secrets.json versus one that is not is just adding my_bittrex.decrypt() after initializing my_bittrex with credentials.

Hope this gives more clarity for the rationale. My apologies if I've introduced someting unintuitive to this awesome repo.