Make decrypt method read api_key and api_secret from secrets.json #122
Conversation
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
|
Of course if a machine is compromized sufficiently badly, the attacker can read live memory and obtain api_key and api_secret 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! |
|
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. |
| except Exception: | ||
| pass | ||
| self.api_key = cipher.decrypt(self.api_key).decode() | ||
| self.api_secret = cipher.decrypt(self.api_secret).decode() |
There was a problem hiding this comment.
Can we add some tests asserting successful assignment of the decrypted data?
mock.patch can be used to simulate the keyboard input the feeds getpass
|
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. Hope this gives more clarity for the rationale. My apologies if I've introduced someting unintuitive to this awesome repo. |
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:
the static encrypt function or preferrably a dedicated secrets.json
creator). This could be done anywhere, for example offline.
machine such as a VPS server and start running a trading script
that utilizes python-bittrex as a dependency.
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.
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