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

Addition of Ninki to the wallets section #760

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

Ninkip2p commented Feb 21, 2015

Hi, I am Ben from Ninki, I have create this request for the inclusion of Ninki Wallet on the 'Choose your wallet' page of the site.

I have setup the config (hopefully correctly) and uploaded an image of our logo.

Cheers

Ben

@Ninkip2p Ninkip2p changed the title from Patch 1 to Addition of Ninki to the wallets section Feb 21, 2015

@harding harding added the Wallets label Feb 21, 2015

Contributor

Ninkip2p commented Feb 24, 2015

Adding some further info:

We announced alpha Testing on Testnet in Jun 2014
https://www.reddit.com/r/Bitcoin/comments/27oy8w/introducing_ninki_wallet_alpha_version/

We began Beta testing in August 2014
http://www.coindesk.com/japanese-wallet-service-integrates-social-networking-bitcoin-payments/

We Released on 6 Dec 2014
https://www.reddit.com/r/Bitcoin/comments/2ogb2d/ninki_wallet_version_101_released/

Our client side code is open sourced here:
https://github.com/Ninkip2p

We have a full testnet API environment here:
https://testnet.ninkip2p.com/api/1/u/amialive

Ninki Wallet is a Chrome App delivered via the Google Chrome App Store. It is a server based service that holds one key in a 2 of 3 multi-signature configuration. The other two keys are managed and stored by the user completely. (ie. they are NOT stored on our servers at all) Authentication to our service is via a password and a two factor authentication token. All transactions are generated and signed on the client, then countersigned by the server.

Some futher documentation here:
http://ninkip2p.github.io/

Cheers

Ben

@harding harding added the Help Needed label Feb 27, 2015

Ninkip2p added some commits Mar 30, 2015

Update choose-your-wallet.html
Updated transparency to checkpasstransparencyopensource
Update en.yml
Added text for Ninki Wallet

@harding harding self-assigned this Mar 31, 2015

@harding harding removed the Help Needed label Mar 31, 2015

Contributor

harding commented Mar 31, 2015

@Ninkip2p Hi, Ben. Can you fix the following:

  1. Add an icon and a screenshot to your pull (required)
  2. Remove the <br/> tag in your app name, as it appears literally in the link text. (Required; see preview screenshot below)
  3. Change checkfailprivacynetworksupporttorproxy to checkfailprivacynetworknosupporttor to use the correct translation tag (required [although I admit I like your naming scheme better])
  4. Reduce the amount of text in your description (required to make the scrollbars go away as seen in the preview below)
  5. Rebase this branch to current master, and possibly squash your commits together (optional: I'll do it at merge if necessary)

2015-03-31-094609_1280x800_scrot

Contributor

Ninkip2p commented Apr 1, 2015

@harding OK, I think this is done.

harding added a commit to harding/bitcoin.org that referenced this pull request Apr 1, 2015

Wallets: Add Ninki Wallet
Rebased-from: b25d62a
Rebased-by: David A. Harding <dave@dtrt.org>
Closes #760
Contributor

harding commented Apr 1, 2015

Craig Watkins (who is currently setting up a GitHub account) has reviewed Ninki wallet. With his permission, I'm appending his review notes below. I've checked over his notes and discussed them with him via email, and we're in agreement that Ninki wallet qualifies for the site. I've squashed the commits in this pull request into 3ac8280 and made one further change: lowercasing Bitcoin in the description when it refers to bitcoins (currency).

In the absence of critical feedback, I'll merge the commit ID mentioned above around 12:00 UTC Saturday.

Craig's notes: (reformatted for GitHub Markdown; no changes to Craig's text but I added a few notes in square brackets)

Ninki Wallet

review version: 2015033001

Basic requirements:

  • Sufficient users and/or developers feedback can be found without concerning issues, or independent security audit(s) is available

    Chrome app: 326 users, no reviews (from Chome store)

    Responsive to support address and responsive and cordial on Reddit.

  • No indication that users have been harmed considerably by any issue in relation to the wallet

    Standard Internet search (using Google and Bing for “ninki
    wallet” and “Green Densho Godo Kaisha” reveals no humans or
    animals that have been harmed.

  • No indication that security issues have been concealed, ignored, or not addressed correctly in order to prevent new or similar issues from happening in the future

    No security issues have been found that have been reported.

  • No indication that the wallet uses unstable or unsecure libraries

    No indication. Uses BitcoinJS and CryptoJS. Random numbers from window.crypto.getRandomValues

  • No indication that changes to the code are not properly tested

    No indications of improperly tested code. Test procedures are at
    https://github.com/Ninkip2p/NinkiWallet/tree/master/tests but exact
    testing procedures are unknown.

  • Wallet was publicly announced and released since at least 3 months

    Chrome wallet released Dec 9, 2014 after beta period starting August
    2014 Mobile wallets release March 2015. [Note: this pull does not
    include the mobile wallets; they'll be submitted separately. -Dave]

  • No concerning bug is found when testing the wallet

    No concerning bugs found.

    Current issues are logged at https://github.com/Ninkip2p/NinkiWallet/issues

  • Website supports HTTPS and 301 redirects HTTP requests

    Marketing website www.ninkip2p.com redirects HTTP requests to HTTPS

  • SSL certificate passes Qualys SSL Labs SSL test

    Marketing site www.ninkip2p.com Passes (CloudFlare protected site)

    API site api.ninkip2p.com Passes

  • Website serving executable code or requiring authentication uses HSTS with a max-age of at least 180 days

    Executable code is served from Chrome store, iOS, and Android app stores

    API site which requires authentication does not offer HSTS headers,
    but clients are not browsers and thus do not downgrade
    automatically.

  • The identity of CEOs and/or developers is public

    Yes: https://en.bitcoin.it/wiki/Ninki

    Benjamin Smith - Lead Developer

    From the Terms of Service: Green Densho Godo Kaisha registered in Tokyo, Japan

If private keys or encryption keys are stored online:

Note that being a multisig wallet, the hot key is stored online,
however it cannot sign transactions by itself. That hot key is
encrypted and requires assistance from the server to decrypt, which
is gated by a password and 2FA.

  • Refuses weak passwords (short passwords and/or common passwords) used to secure access to any funds, or provides an aggressive account lock-out feature in response to failed login attempts along with a strict account recovery process.

    Refuses passwords less than 10 characters.

    Displays a primitive password meter; may be too generous

    Doc at http://ninkip2p.github.io/ claims 4 attempts allowed,
    but that restriction has been changed to a throttle of
    20/minute in the API

If user has no access over its private keys:

User has no access to the multisig key stored on the server.

  • Provides 2FA authentication feature

    Yes: TOTP (RFC6238) support using Authy and Google Authenticator

  • Reminds the user to enable 2FA by email or in the main UI of the wallet

    Desktop: Cannot sign up without enabling 2FA

    Mobile does not use 2FA

  • User session is not persistent, or requires authentication for spending

    Desktop: 2FA is required for spending

    Mobile: PIN is required for spending

  • Provides account recovery feature

    Provides offline recovery tool to recover funds (not dependent on
    server, uses chain.com for blockchain information and to send tx)

If user has exclusive access over its private keys:

User has access to 2 of 3 private keys (one offline/cold, one
online/hot)

If user has no access to some of the private keys in a multi-signature wallet:

User does not have access to server private key

  • Provides 2FA authentication feature

    Yes, see above

  • Reminds the user to enable 2FA by email or in the main UI of the wallet

    Yes, see above

  • User session is not persistent, or requires authentication for spending

    Yes, see above

  • Gives control to the user over moving their funds out of the multi-signature wallet

    Yes. 2 of 3 wallet. User holds 2 keys.

For hardware wallets:

N/A

Optional criterias (some could become requirements):

  • Received independent security audit(s)

    No. It is a future goal.

  • Avoid address reuse by using a new change address for each transaction

    Yes. Uses M/0/1 chain for change. [I confirmed with Craig that a
    non-hardened branch is used for change despite the recommendations
    to the contrary in BIP32. Craig suspects that this is because the
    server wants to validate the change address for the user's chain
    using the server's copy of the master public key. Although I'd
    personally prefer to see a hardened branch, I think that's a
    reasonable technical decision. -Dave]

  • Avoid address reuse by displaying a new receiving address for each transaction in the wallet UI

    Yes. In addition uses a separate address chain for each registered contact.

  • Does not show "received from" Bitcoin addresses in the UI

    Does not show.

  • Uses deterministic ECDSA nonces (RFC 6979)

    Uses BitcoinJS which employs deterministic k values from RFC6979.
    This was verified by signing a transaction “by hand” using
    pybitcointools (which uses deterministic nonces) and verifying that
    the signatures were the same.

  • Provides a bug reporting policy on the website

    Provides email for comments

    Provides issue tracking on github https://github.com/Ninkip2p/NinkiWallet/issues

  • If user has no access over its private keys:

    N/A

  • Supports HD wallets (BIP32)

    Supports BIP32 with a M/0/0 path

  • Provides users with step to print or write their wallet seed on setup

    Yes. Provides a non-standard BIP39 style seed phrase (BIP32 uses
    mnemonic entropy directly; does not use PBKDF2 step in BIP39)

  • Uses a strong KDF and key stretching for wallet storage and backups

    Seeds are 256 bits with no need for stretching

    PBKDF2 is used for password stretching (but only a fixed 1000 iterations is used)

  • On desktop platform:

    • Encrypt the wallet by default

      Yes

  • For hardware wallets:

    N/A

Contributor

saivann commented Apr 2, 2015

A huge thanks to Craig for this thorough review and testing!

A few comments on the pull request (but I didn't check everything):

  • "Bitcoin" should probably use lowercase letters in the description?
  • The transparency score should probably be "checkfailtransparencynew" given that it's new?
Contributor

Ninkip2p commented Apr 2, 2015

Hi @saivann

I have updated bitcoin references to lowercase

checkfailtransparencynew- what is the criteria for this? is it in reference to a new listing on bitcoin.org or a new wallet? We have been in production for around 5 months. (our alpha release was June 2014)

Cheers

Ben

Contributor

saivann commented Apr 2, 2015

@Ninkip2p You can find the criteria here. This is not a new requirement and other wallets were also applied this score at first. The countdown is usually based on the release date of the first public final release and codebase (not alpha or beta releases).

"The codebase and final releases must be public since at least 6 months and previous commits must remain unchanged."

Contributor

Ninkip2p commented Apr 2, 2015

@saivann Ah, ok, I thought it was 3 months, have updated the string.

Cheers

Ben

@harding harding closed this in 74e42fa Apr 4, 2015

Contributor

saivann commented Apr 4, 2015

@Ninkip2p I just noticed you provided an icon that is 113px long, can you provide one that fits within 96 X 96px?

Contributor

Ninkip2p commented Apr 5, 2015

@saivann Have checked in to patch-1

Contributor

saivann commented Apr 5, 2015

@Ninkip2p Thanks! I just cherry-picked and pushed your commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment