search

status-im / status-github-bot

A bot for github
https://status-github-bot.herokuapp.com/
ISC License
11 stars 12 forks source link

Write script to watch over Status repos for any private keys commited by mistake #23

Closed pedropombeiro closed 6 years ago

pedropombeiro commented 6 years ago

User Story

As a developer, I want to be warned if I happen to commit by mistake a private key of an ETH account which contains funds (ETH or ERC20), so that the risk of losing funds is reduced.

Description

The script should watch all the pushes on https://github.com/status-im repos for strings that match the format of either a private key, or a 12 word recovery key. For all the keys that are found, it should look for the balance of the account, and if non-zero, send a warning over Slack to #devops channel.

Documentation for the bot is available in the wiki.

GitHub
Status
A Mobile Ethereum Client.
status-open-bounty commented 6 years ago

Balance: 0 ETH Tokens: SNT: 2400.00 Contract address: 0x8e37926170502109ff96c97bbf104f7098718f0a Network: Mainnet Paid to: trowacat Visit https://openbounty.status.im to learn more.

trowacat commented 6 years ago

This is a great precaution! I will try my hand at this.

StevenJNPearce commented 6 years ago

Wouldn't it make more sense to use husky or something similar to prevent private keys from being committed in the first place? Once a private key has touched the public repo, the damage has already been done.

trowacat commented 6 years ago

Yes, you're right, that makes more sense. The developer would be warned right away rather than having to be active on the #devops channel to realize the mistake. A hook would warn them where they could not miss it. I'm interested if anyone has a reason for not going this route.

pedropombeiro commented 6 years ago

Hey @StevenJNPearce. you're right, ideally this would be done closer to the source. The thought of leveraging Git hooks for this crossed my mind after creating this issue, and as long as that doesn't introduce a noticeable delay when committing, I think that's fine and a better solution. Since we'd probably be doing this as part of a pre-push hook, that should be OK.

Not sure about relying on npm though, since ideally this would be a solution that's reusable on all our repos, and some of them don't make use of npm.

trowacat commented 6 years ago

@PombeirP would making a hooks directory, something like .hooks, in the root and then setting the hooks path (https://git-scm.com/docs/git-config#git-config-corehooksPath) be a viable solution to not using npm?

Git - git-config Documentation
pedropombeiro commented 6 years ago

@trowacat I think the most important is to have a good script for checking changes before pushing. How we actually implement that on each repo will end up being an implementation detail. For instance, in this repo it would be fine to use husky, but on https://github.com/status-im/status-react/blob/develop/project.clj#L19, for instance we can invoke a script from the ClojureScript project file where we already have support for git hooks.

GitHub
status-im/status-react
status-react - a free (libre) open source, mobile OS for Ethereum
trowacat commented 6 years ago

@PombeirP Ok. Where should I submit a PR in this case?

pedropombeiro commented 6 years ago

@trowacat in this repo is fine.

pablanopete commented 6 years ago

Awesome stuff! 👍@trowacat

pedropombeiro commented 6 years ago

Hey @trowacat, great job! Do you think you could add the integration with husky too for this repo since you're at it?

UPDATE: Just noticed that an important part is missing, the script isn't actually checking if there are funds on the account. The idea behind that was that it is usual for us to submit private keys for instance for unit tests (which have no funds associated with them), but we'd want to throw up a big warning if we're actually checking in something which contains funds there.

trowacat commented 6 years ago

@PombeirP Ok, I will add that part to the script, sorry I missed that bit! I'll update my PR with that as well as the husky implementation for this repo.

edit: I am having trouble figuring out an acceptable way of checking the balance from a private key. Do you have any suggestions?

pedropombeiro commented 6 years ago

@trowacat yeah, I have to admit it didn't occur to me that we'd need to extract the address before an etherscan.io query. This post seems to describe a way to achieve it, but relies on the ethers.js NPM package: https://github.com/ethers-io/ethers.js/issues/31. There is also this one which is maybe lighter weight using ethereumjs-util: https://ethereum.stackexchange.com/a/15836/30590

Ethereum Stack Exchange
How to derive address from private key?
I want to give users a private key so they can Easily get address.. Why? So a single series of digits enables them to have both public address and private key. Of course this is for temporary use...

We could check if npm is installed on the host machine before attempting to check the balance (installing the npm library globally, if not already installed), and showing a warning if not. What do you think?

trowacat commented 6 years ago

@PombeirP , Yes that seems like the best solution, I've looked for possible ways to avoid having to use a package but was unable to find any. However, I am still unsure how I'll be able to convert the seed phrase to the address, is there an easy way of doing that as well?

Also for the warning you mention in your final paragraph, did you mean to display a warning if npm is not installed or a warning if a possible key is found but it is unable to check for funds due to npm being missing?

pedropombeiro commented 6 years ago

You know what? I wouldn't worry about the case of seed phrases. To be honest, it is much more likely for a private key to be a problem than a seed key.

Regarding the warning, I meant the latter, if a possible key is found but it is unable to check for funds due to npm being missing.

On Fri, Jun 22, 2018 at 3:23 AM, Brandon Wissmann notifications@github.com wrote:

@PombeirP https://github.com/PombeirP , Yes that seems like the best solution, I've looked for possible online APIs that could convert the private string to avoid having to use a package but was unable to find any. However, I am still unsure how I'll be able to convert the seed phrase to the address, is there an easy way of doing that as well?

Also for the warning you mention in your final paragraph, did you mean to display a warning if npm is not installed or a warning if a possible key is found but it is unable to check for funds due to npm being missing?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/status-im/status-github-bot/issues/23#issuecomment-399291840, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIbWhC1A0t0KnTr4BIZurPc0NsjWQLcks5t_EcjgaJpZM4UsHRy .

trowacat commented 6 years ago

@PombeirP the library worked like a charm! I rewrote the script in node, I'll update my PR tonight.

trowacat commented 6 years ago

@PombeirP New script is pushed, let me know what you think. Thank you for the help!

AlessandroSpallina commented 6 years ago

is this issue closed or not?

pedropombeiro commented 6 years ago

@AlessandroSpallina there is a new PR from @trowacat that somehow I wasn't notified about, will look at it soon.