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 Copay wallet #888
Conversation
maraoz
and others
added some commits
Oct 1, 2014
harding
added
the
Wallets
label
Jun 12, 2015
This was referenced Jun 12, 2015
|
I made several updates related to adding Copay to bitcoin.org. Added CopayCopay has been added, platforms:
MobileDesktopWebOne thing to note – Copay is no longer distributed as a remote web app, only as a chrome extension. I left it in the Added Windows PhoneAdditional ClassificationsSince Copay’s primary app makes it very simple for end users to use an alternative or self-hosted (open source) wallet node, I think it would be valuable to add a classification for wallets which enable the users to run their own nodes. I tried to stay as true to the other categories as possible, but I’d appreciate other thoughts on the below. "Remote validation":
|
bitjson
changed the title from
WIP: Add Copay wallet
to
Add Copay wallet
Jun 22, 2015
|
@bitjson, Thanks for your updates, and thanks for taking the time to understand the scoring so well and make some excellently worded suggestions.
We have been recently listing extensions in the desktop (not web) category and the issue was just discussed here #827 . A quick summary of my thoughts on the issue is that our web category is a selection criteria that users employ to find a wallet which they do not need to "install" on a computer (which they may not have control over).
A welcome addition!
I think you bring up an excellent point which we should address at some point. Scoring for self-run full nodes also came up here #878 . To summarize my thoughts in that thread: I support making some changes in this area eventually, but I would like to hold off adding more classifications until we step back and take a full holistic view of the classifications (and then go back to re-score all the wallets). Also, as I recently mentioned in the MultiBit HD PR #887 : |
|
I have reviewed Copay based on the current wallet requirements criteria and my evaluation is below. The summary is that the wallet passes on security and overall design. However, because the website that links to executable code does not yet support HSTS and the website does not work with the Qualys SSL tester, I cannot at this time recommend it for listing. I am willing employ alternate SSL testing methods if there is a compelling reason why this testing tool won't work on copay.io. BitPay is working on these issues and I will be glad to recommend Copay for listing once these website issues are resolved. CopayChrome app version v1.0.1iOS app version v1.0.0Android app version v1.0.1Review version 20150602401The 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. Basic requirements:
PASS This wallet has been well publicized for over a year without any current concerning issues.
PASS No indication that users have been harmed. NOTE Issues when users need help such as http://www.reddit.com/r/Bitcoin/comments/38sucy/funds_stuck_in_copay_2of3_wallet_need_help/ are addressed quickly.
PASS A significant bug was found during beta http://blog.coinspect.co/copay-wallet-emptying-vulnerability that was quickly addressed.
PASS Uses bitcore and sjcl
PASS Tests are included in github and services are tested using mocha and karma.
PASS This wallet has been in public beta with completely public issue tracking https://github.com/bitpay/copay/issues/ for over a year, so the 3 month requirement is waived.
PASS The only concerning bug found during testing (regarding balance updates) was fixed bitpay/copay#2733 before being reported
PASS Redirects copay.io and www.copay.io
FAIL The Qualys tester actually fails on this site (i.e. copay.io does not fail the test, the test fails to run): Assessment failed: Failed to communicate with the secure server NOTE An alternative set of tests may be constructed to satisfy this requirement NOTE Other tests also fail:
FAIL No HSTS
NOTE There is no reference to this on the copay.io site nor any reference to the BitPay site.
N/A
N/A User has full access over private keys
PASS Allows backup from settings of a JSON formatted AES encrypted blob NOTE Syntax of blob is not yet documented, so the only supported restore is with a working Copay wallet
PASS Wallets can be restored (and cloned) using the backup blob NOTE Bitpay is working on a standalone sweep tool http://www.reddit.com/r/Bitcoin/comments/38sucy/funds_stuck_in_copay_2of3_wallet_need_help/crxq2f1 NOTE Using custom code it was proven possible to independently sweep a Copay 1of1 P2SH wallet using the backup blob
PASS https://github.com/bitpay/copay
N/A While this is a multsig wallet, the service does not hold any of the keys
N/A
Optional criterias (some could become requirements):
NOTE No known independent audits
PASS A new change address is used for each transaction, confirmed using custom code to verify each address was used only once
PASS A new receive address is displayed for each transaction
PASS Does not show received from addresses
PASS Uses deterministic nonces NOTE Currently not compatible with RFC 6979. BitPay is correcting this bitpay/bitcore#1269 NOTE Uses LowS signature values per BIP62
FAIL Only links on website are to github repository NOTE While the address is not provided, support@bitpay.com is backed by a professional support organization, with prompt responses and excellent followthrough
N/A
PASS Uses BIP32 NOTE Uses BIP45 style paths (e.g. m/45'/2147483647/c/i for 1of1 wallets) NOTE Note that single signature wallets use P2SH, not P2PKH. This is less efficient.
PASS If a backup has not been done, a full screen reminder is presented on the Receive tab
PASS Uses PBKDF2 with 10000 iterations and 128 bit AES
FAIL Wallets are not encrypted by default, but: NOTE The wallet’s private key (only) can be encrypted from the settings page
N/A |
|
@crwatkins I think the issue w/ Qualys is that it's trying to do the scan based off of the common name on the certificate, which is Bitcore.io - https://bitcore.io/ times out when accessed via HTTPS. Edit: Also just wanted to add, thank you for doing a thorough and comprehensive review. Also, awesome job to @bitjson dialing all of this in. @crwatkins - if they get the SSL issue fixed, so Qualys runs (perhaps by enabling HTTPS on Bitcore.io), is that sufficient then to get your recommendation for listing? |
|
Will, that may be; Qualys does a lot of different types of connections to test the site. However, I get similar connection problems using curl (see above) and even openssl s_client:
I haven't done any recent debugging but when I very quickly wiresharked it a couple weeks ago the SSL alert occurred immediately after the ClientHello (i.e. no ServerHello received). I haven't looked beyond that. |
matiu
commented
Jun 26, 2015
|
Thanks a lot @crwatkins for the in-depth review and all the hard work! We will be working on SSL described issue and adding HSTS at the landing page (copay.io) as soon as we can, and will update this thread when is ready. |
|
@matiu, I believe the issue with the broken SSL connection is related to SNI. If I send a SNI with copay.io, things work as expected (completes a full SSL handshake), e.g.
The issue may relate to your server handling of SNI. |
matiu
commented
Jun 26, 2015
|
Thanks @crwatkins Following this guide https://raymii.org/s/tutorials/Strong_SSL_Security_On_nginx.html we had remake our webserver configuration files, so now https://www.ssllabs.com/ssltest/analyze.html?d=copay.io shows "A+" as general rate. We have also added HSTS. Thanks a lot for the suggestions. Could you please review it now? |
|
@crwatkins great review! I wanted to point out a couple interesting points for anyone who missed them:
.... that's some really detailed reviewing. For the RFC6979 part, Craig took the time to independently sign a copy of a Copay-made transaction with a separate RFC6979-compliant Bitcoin implementation. He noticed it didn't match, contacted BitPay, and helped them discover the issue described in bitpay/bitcore#1269 . I find this very impressive---thanks, @crwatkins!. @matiu I see a "B" (not "A+") as the general rating on https://www.ssllabs.com/ssltest/analyze.html?d=copay.io , but that's a passing score and it now says HSTS is set to 730 days. Based on this and @crwatkins's review, I support adding Copay to the site. In the absence of critical feedback, this PR will be merged Tuesday. |
harding
added
the
Merge Scheduled
label
Jun 26, 2015
matiu
commented
Jun 26, 2015
|
thanks @harding. I do agree that @crwatkins did a terrific, in-depth review. I see "A+" on https://www.ssllabs.com/ssltest/analyze.html?d=copay.io&latest maybe try "clear cache"? |
|
A+ now confirmed, I think appending |
|
@crwatkins just sent me an email reminding me that he had multiple objections to the scoring text changes as described in this comment. @bitjson can you look into those please? |
|
Nice work @bitjson @matiu and @crwatkins. |
|
@harding Thanks for noting my objections while I was offline. As for the review:
PASS copay.io A+ rating
PASS max-age is 730 days Contingent upon resolution of the scoring in the PR request, I am glad to recommend Copay for listing. |
|
Agreed with @crwatkins , huge thanks for this amazing review work! |
harding
removed
the
Merge Scheduled
label
Jun 28, 2015
|
Note, in addition to coming to agreement on the scoring text, this now also needs to be rebased to prevent a conflict in |
|
Thank you @crwatkins for your very thorough review. I just made changes based on your comments.
Makes sense. I've removed the
I also agree about the value of reducing "classification creep”. If users can't understand the difference between criteria, the value of this list is diminished. Since this page is often used (even by very technical bitcoin users) to quickly understand the security and privacy implications of various wallets, I also think accuracy is a top priority. There are already a number of bitcoin companies using Copay as a corporate wallet – in fact, this is the primary demographic Copay is designed to accommodate. The self-hosting feature is of production-critical importance to them. It would be unacceptable for BitPay or another entity to have the ability to DOS (or even observe the usage of) their wallets. I’m not an expert, but I believe this architecture is arguably more secure and privacy-protecting than an SPV wallet. A self-hosted BWS node is fully validating, and can even insulate the Copay wallet using whatever privacy-enhancing measures the organization chooses to implement. To promote secure usage and avoid misconceptions, I think it's valuable to add this architecture to the possible classifications. As an added benefit, these additional classifications may motivate other wallets providers to open source their servers and provide the option to self-host a remote node. ValidationWe currently have the following choices:
Obviously, it could give a false sense of security to mark Copay with "Full validation”, though that is the level our business users are getting. Similarly, I’m worried that “Centralized validation” will promote serious misconceptions. I think the "Remote validation” option in this PR completes this list and provides valuable additional information. If we wanted to try to simplify the list further, I'd propose we widen the meaning of "Decentralized validation" to fit Copay's architecture too, rather than limiting it to just the randomized node selection implementation in GreenAddress. I think more information is preferable to oversimplification in this case though, as validation is a security-defining concept in wallet architecture. 5 classifications seem reasonable to me for this particular area. PrivacyFor privacy we currently have:
Here again, the proper category for self-hosting users would be "avoids disclosing information" (depending on configuration, this can provide better privacy than an SPV client). For non-hosting users, some information is disclosed to the remote node. I think the "Discloses information to a remote node" classification in this PR completes the spectrum of possible privacy configurations for wallets, and is likely to be used by other wallets currently in development. Again thanks for the detailed review. I’d appreciate your thoughts on this, and if possible, I’d really like to win you over. I’d prefer to finalize Copay’s scoring in this PR rather than waiting for a rewrite of this page. As I see it, I can go the extra mile to clarify details here, or I can spend much of my time in the next few months answering emails and calming the fears of potential business users. |
|
This PR request is to add the Copay wallet. If we want to make changes to the scoring criteria that will affect other wallets I believe we should have a separate PR with a subject that publicly indicates our intent. (My guess without checking is that the majority of the wallets that are currently scored to use a central server will tell you that you can set up your own if you want.) I believe that PR should include the changes to the scoring for wallets that are affected. |
|
I concur w/ @crwatkins that changes to scoring criteria should be a separate PR. Also @bitjson, FWIW, I agree with you that the scoring criteria could be improved. Thanks for putting the thought and effort into elaborating on your perspective. |
|
@crwatkins @Coderwill Agreed, makes sense! |
bitjson
referenced this pull request
Jul 1, 2015
Closed
Add "Remote Node" Wallet Classifications #927
|
@crwatkins @Coderwill @saivann thank you for the input – I separated the classifications from this PR and submitted them in #927. I'll continue this thread there. Are there any further changes that need to be made to this pull request? Thank you all for reviewing. |
|
@bitjson, thanks for helping us move forward with the wallets classification. On this pull request, I would suggest that With that change, I would recommend this PR for merge. @bitjson, thanks for all your extra work here. |
|
@crwatkins good idea, just made the change. Thanks for keeping the wallet section so well-maintained! |
|
LGTM for merge. |
|
LGTM up to commit cbcde1b . Thanks! |
|
In the absence of critical feedback, this will be merged on Friday. Thanks! |






bitjson commentedJun 12, 2015
Continuing from bitcoin#587.