Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
Already on GitHub? Sign in to your account
Add Ninki Wallet iOS and Android to choose your wallet #863
Conversation
dabura667
commented
May 25, 2015
|
Glad to help. Smooth onboarding experience, now. I'll be watching this thread. |
harding
added
the
Wallets
label
May 26, 2015
harding
self-assigned this
May 28, 2015
|
I have reviewed the Ninki iOS and Android wallets based on the current wallet requirements criteria and my evaluation is below. The summary is that I can recommend these wallets for listing. I concur with the scoring in the pull request. Ninki WalletiOS version 0.5.0Android version 0.5.2Review version 2015053101The wallet list is based on the personal evaluation of the maintainer(s) and regular contributors of this site, according to the criterias detailed below. These requirements are meant to be updated and strengthened over time. Innovative wallets are exciting and encouraged, so if your wallet has a good reason for not following some of the rules below, please submit it anyway and we'll consider updating the rules. NOTE A previous review of the desktop chrome app (which involved using the mobile apps) was completed on March 30, 2015. Some of the information which has been deemed to not have changed (using git commits and other research) was not retested. The target of this review is only the mobile apps, but includes the involvement of the desktop app. Basic requirements:
PASS Responsive to support address and responsive and cordial on Reddit. NOTE Only negative report found at http://www.reddit.com/r/Bitcoin/comments/306d1r/do_not_use_ninki_wallet/ seems to be user error
PASS Standard Internet searches for “ninki wallet” and “Green Densho Godo Kaisha” reveals no humans or animals that have been harmed.
PASS No security issues have been found that have been reported.
PASS No indication. Uses BitcoinJS and CryptoJS. Random numbers from window.crypto.getRandomValues.
PASS No indications of improperly tested code. Test procedures are found at https://github.com/Ninkip2p/NinkiWallet/tree/master/tests but exact testing procedures are unknown.
PASS Desktop wallet released December 9, 2014 after beta period starting August 2014 NOTE Mobile wallets using same code base were released March 17, 2015.
PASS No concerning bugs found. Current issues are logged at https://github.com/Ninkip2p/NinkiWallet/issues
PASS Marketing website www.ninkip2p.com redirects HTTP requests to HTTPS
PASS Marketing website www.ninkip2p.com A rating PASS API site api.ninkip2p.com B rating
NOTE Executable code is served from Chrome store, iOS, and Android app stores PASS API site which requires authentication does not offer HSTS headers, but clients are not browsers and thus do not downgrade automatically.
PASS https://en.bitcoin.it/wiki/Ninki Benjamin Smith - Lead Developer. From the Terms of Service: Green Densho Godo Kaisha registered in Tokyo, Japan.
NOTE This is a 2of3 multisig wallet with only the one server private key stored online
PASS Desktop uses a primitive password meter, but it may be too generous PASS Mobile uses an aggressive “penalty box” with exponentially increasing timeout for bad PINs
N/A
NOTE User has access to 2 of 3 private keys (one offline/cold, one online/hot)
PASS Wallet allows backup of online (non-standard) BIP39 BIP32 seed
PASS Offline recovery tool https://github.com/Ninkip2p/ninki-recover is working. There could be more obvious documentation about its existence and usage
PASS https://github.com/Ninkip2p?tab=repositories
PASS Provides TOTP (RFC 6238) support using Authy and Google Authenticator
PASS Desktop cannot sign up without enabling 2FA NOTE Mobile does not use 2FA, but is subject to transaction limits
PASS Desktop requires 2FA for spending PASS Mobile requires PIN for spending
PASS 2 of 3 multisig: User holds two keys.
N/A
Optional criterias (some could become requirements):
NOTE No. It is a future goal.
PASS Uses m/0/1 chain for change
PASS Uses new receiving addresses each time. Also uses a totally separate chain for each registered contact.
PASS Does not show “received from” addresses
PASS Uses BitcoinJS which employs deterministic k values from RFC 6979. This was verified by signing a transaction with custom code using pybitcointools (which uses deterministic nonces) and verifying that the signatures were the same.
PASS Provides email for comments. Provides issue tracking on github https://github.com/Ninkip2p/NinkiWallet/issues
N/A
PASS Supports BIP32 with a m/0/0 path
PASS Provides a non-standard BIP39 style seed phrase (BIP32 uses mnemonic entropy directly; skips the PBKDF2 step in BIP39)
PASS PBKDF2 is used for password stretching (but only a fixed 1000 iterations is used)
PASS
N/A
|
|
@crwatkins your review looks great as always, and the conclusion to add the mobile Ninki wallets sound good to me. Thanks! @ninkip2p can you please rebase this pull request so that it's in a merge ready state? Right now it's so divergent from our master branch that I can't preview build it locally using our regular build tools. Once I've previewed it and checked for issues, I'll schedule a merge. Thanks! |
|
@harding Hi, I couldn't get the rebase to work so I created Ninkip2p:patch-2 from bitcoin/master, added my changes and it now says can be automatically merged. The pull request is against patch-1 however and not sure if there is a way to change the target of a pull request? |
|
@ninkip2p Thanks for working on it! I don't think you can change the branch for a pull request, but you can force push the contents of patch-2 to your patch-1 branch (overwriting the contents of patch-1). (I already downloaded the refs for patch-1, so I'll be able to compare patch-2 to patch-1 to ensure you have the same scoring @crwatkins reviewed.) To force (
|
|
@harding ok, I ran the command, seems to be ok |
|
@ninkip2p whoops, it didn't update---it looks like I gave you the branches in the wrong order. Can you try: git push -f origin patch-2:patch-1 (Happily it's really hard to lose data when using git.) |
dabura667
commented
Jun 1, 2015
|
https://github.com/bitcoin/bitcoin.org/compare/bitcoin:master...dabura667:patch-2?expand=1 rebased it for you... some of the commits were wonky because of your backwards forced push on patch-2. |
dabura667
commented
Jun 1, 2015
|
http://stackoverflow.com/questions/9153598/how-do-i-fetch-a-branch-on-someone-elses-fork-on-github Use this to fetch my patch-2, then once you copied it to a branch on your repo then you can do
Assuming you saved by branch as patch-dab |
dabura667
commented
Jun 1, 2015
|
Also, check the diff I linked above (diffing master and my branch) it should be fine, but just in case I merged something wrong, double check. |
added some commits
Jun 1, 2015
|
@harding OK, I think should be all good now! |
|
I just sent @ninkip2p this PR to fix the build bugs and to list Ninki in the top-level mobile category as well as Android/iOS. A preview of the page with Ninki added is here: http://dg0.dtrt.org/en/choose-your-wallet Everything looks good to me with regard to the scoring, screenshot, and text. In the absence of critical feedback, I'll merge this pull around 23:00 UTC Saturday or 72 hours after @ninkip2p merges my PR to him, whichever comes later. |
harding
added
the
Merge Scheduled
label
Jun 2, 2015
|
@harding thanks for the fixes. Have merged. |
|
@harding is all ok with this? Am I supposed to close the request? |
harding
closed this
in
95af331
Jun 7, 2015
|
@ninkip2p sorry for the delay merging. It should be live on the site within about 15 minutes. |
dabura667
commented
Jun 7, 2015
|
I have confirmed it is showing up on the website |
Ninkip2p commentedMay 25, 2015
This is a request to include Ninki Wallet in iOS and Android in the choose your wallet section. We have previously been through a full review by Craig for the mobile releases but wanted to hold back until these latest versions were ready.
There is one major change that you should be aware of:
Ninki is a 2 of 3 Multisig wallet where the user controls 2 keys and Ninki controls one. Normally the user will create an account on the Desktop Chrome App and then pair with the phone. The user's keys are generated locally on the Chrome App and written down by the user. One of the keys is used to recover the account in the event that our service is offline and so must be written down at the point of account creation.
This presents a problem if we wish to allow users to sign up using the mobile app. Often they are not in a situation that is appropriate to write down the key and so in our UAT testing we found they were likely to skip this step.
We needed a solution which allowed the user to write down the recovery key at a later time without revealing the key to us. We also need a solution that retains the current security level of the mobile app. That if the phone is compromised, there is not a single target to attack and control the wallet.
@dabura667 proposed a method of achieving this goal.
The Recovery key is derived from 3 sources:
Device key- 128 bits of entropy
Secret key- 128 bits of entropy
Online key- 128 bits of entropy
The Recovery key is the first 128 bits a SHA256 of the device key ECMULT by an EC public key derived from the secret key concatenated with the online key. The EC public key is stored on the Ninki server until the time the user confirms that they key has been written down. Once confirmed the Device key is deleted from the device and the ECPubKey is deleted from the server.
Create Key
Secret key->ECPubKey (Pub key stored on server)
Device key
Online key
SHA256(Device key * ECPubKey || Online key)
Take first 128 bits
Recover the Key
ECPubKey from server
Device key
Online key
SHA256(Device key * ECPubKey || Online key)
Take first 128 bits
This is in effect a method of allowing the user to extend the account creation process over a period of time suited to them.
We have implemented a new Security Checklist to take them through the process of fully securing the account which makes it clear the steps they have to follow before their account setup is considered complete.
We also added reminders to instruct the user to complete the key backup process on address creation and, if the user has a balance of over ~$5, every time they enter their PIN number.