artiomchi
commented
10 years ago I've just updated that with another commit to allow encrypting custom/vanity addresses as well.
Closed artiomchi closed 10 years ago
artiomchi
commented
10 years ago I've just updated that with another commit to allow encrypting custom/vanity addresses as well.
levino
commented
10 years ago Guys. It's Christmas... Am 26.12.2013 00:09 schrieb "Artiom Chilaru" notifications@github.com:
I've just updated that with another commit to allow encrypting custom/vanity addresses as well.
— Reply to this email directly or view it on GitHubhttps://github.com/cantonbecker/bitcoinpaperwallet/pull/6#issuecomment-31206779 .
artiomchi
commented
10 years ago I'll take that as a compliment? ;)
cantonbecker
commented
10 years ago Hi artiomchi,
This is a great start! Thank for being proactive. Every day or so I get a request for BIP38, so I really appreciate your efforts.
Your implementation unfortunately faithfully reproduces some usability problems I have with how bitaddress.org implements BIP38. For example:
Here's how I think it should work. The reason I haven't implemented BIP38 yet is because I want it to work much better than it does with bitaddress.org. See this flow mockup:
http://cl.ly/image/0e3v1M2B081X
Description:
1) On the "Front" tab interface, The BIP38 encryption option is a single checkbox, that's it.
2) When you click it, a modal dialog box gives you an overview (and warnings about BIP38) and prompts you for the passphrase.
3) When you "turn on BIP38 encryption", then the CURRENT wallet (whether randomly generated or vanity-inputted) gets encrypted, and CSS is used to swap out "WALLET IMPORT FORMAT" with "BIP38 ENCRYPTED PRIVATE KEY" on the front of the wallet desigh. (I'll need to remove "Wallet Import Format" which is currently burned into the 11 or so front jpgs.) Also, the current passphrase and a link to change the passphrase is now displayed on the "Front" tab interface.
4) Un-checking "BIP38 Encrypt" immediately reverts the keypair you are currently looking at to non-encrypted (WIF) format.
What do you think?
artiomchi
commented
10 years ago Hmm.. I think that's a pretty good idea in terms of usability.. And it should be relatively simple to implement as well..
Do you have the mockups working in html, or were these just photoshopped?
cantonbecker
commented
10 years ago That was just Photoshop. If someone (you?) were to implement the very basic functionality as rough html I would happily stylize it.
The other idea I had last night is that the introduction/instructions tab might benefit from a "decode a bip38 key here" so that no additional app (eg bitaddress.org) is required to redeem/decode. What do you think? On Dec 26, 2013 4:52 AM, "Artiom Chilaru" notifications@github.com wrote:
Hmm.. I think that's a pretty good idea in terms of usability.. And it should be relatively simple to implement as well..
Do you have the mockups working in html, or were these just photoshopped?
— Reply to this email directly or view it on GitHubhttps://github.com/cantonbecker/bitcoinpaperwallet/pull/6#issuecomment-31219023 .
artiomchi
commented
10 years ago Hmm.. I looked around the code again, and in my opinion a good way around it would be at least a partial rewrite of the ninja class, since it wasn't designed with what we're doing in mind.
Nonetheless, I've somewhat adapted it for the commits above, so I changed it a bit more :)
Only thing to note is - generating a new BIP0038 address uses the password as entropy, so at no point during the generation can I see a decoded private key.. If the address displayed on screen in encoded and was randomly generated, disabling BIP0038 encoding will have to either run through a decoding procedure, or generate a new address.. Other than that - the rest is simple.
I should have a commit with it pretty soon :)
artiomchi
commented
10 years ago I've comitted the new changes.. They have the new design as you showed..
I've hosted it here, so you should be able to see it immediately: https://bitcoinpaperwallet.azurewebsites.net/
cantonbecker
commented
10 years ago Artiom,
This is exceptional! I could hug you. It's really stellar work, absolutely true to the mockups. Seeing it in action, does the workflow feel right to you?
As for me, I need to play around with it some, do a fair amount of round-trip testing, etc.
Initial impressions (aside from it being altogether excellent and stable):
* Number One * I do hope there's a solution or work-around for this scenario:
1) Load up generator 2) Print out wallet. This is going to be my copy. 3) Now turn on BIP38, and supply passphrase "sneaky". Print it out. This is going to be my friend's copy. 4) Now I want a new wallet, also with the passphrase sneaky, so I click 'random-generate new keys' 5) Print out wallet. This is the BIP38 version for my friend to keep save. 6) Turn off BIP38, because I want to print out an unencrypted copy for my own records.
Result: "No private key available. Do you want to generate a new one."
I really do appreciate that you thought to pop up an alert to handle this scenario, but is there really no way we have access to the private key?
What makes me curious is that of course if I take that same "no private key available" BIP38 private key and paste it into bitaddress.org along with my passphrase, it can reverse-engineer the WIF-format unencrypted key. Our wallet knows the passphrase, it knows the BIP38 key, so can we make it so that you can toggle BIP38 on and off for any given wallet, whether it was generated with the checkbox turned on or not?
* Number Two * There are a few situations where should have some "please wait, doing BIP38 encryption/decryption" feedback. (I would always prefer a message to a spinning graphic as well.) For example:
1) Turn on BIP38, supply a passphrase 2) Click 'supply my own key' 3) Paste in a WIF key (5***)
Result: browser appears to hang with no message until key is generated.
I'd like it if anytime BIP38 work is being done, a clear message is displayed especially for the benefit of people using slow laptops they have dedicated for printing wallets. It can take minutes!
* Number Three * Should we make it so that the "supply your own key" feature allows you to supply either a WIF key (5_) or a BIP38 key (6_)? I guess we'd have to also let them type in the passphrase at that point... Kind of a hairy mess. It would definitely be appreciated by people who intend to sell or give away blank paper wallets to people who provide their own BIP38 keys. It could possibly also give us at least one way to let people decrypt their own BIP38 encrypted wallets, which would be a welcome feature.
Anyway that's my feedback so far.
Again, excellent work. With your permission what I'd like to do by way of compensation is start sharing donations that bitcoinpaperwallet receives. Please email me privately at canton@gmail.com and we'll work out a fair arrangement to compensate you for your excellent work.
Thanks again!
canton@gmail.com • (505) 570-0635 • http://cantonbecker.com
levino
commented
10 years ago Nice work! Share the love Canton. Cheers from Hamburg@30C3, Levin
2013/12/27 cantonbecker notifications@github.com
Artiom,
This is exceptional! I could hug you. It's really stellar work, absolutely true to the mockups. Seeing it in action, does the workflow feel right to you?
As for me, I need to play around with it some, do a fair amount of round-trip testing, etc.
Initial impressions (aside from it being altogether excellent and stable):
* Number One * I do hope there's a solution or work-around for this scenario:
1) Load up generator 2) Print out wallet. This is going to be my copy. 3) Now turn on BIP38, and supply passphrase "sneaky". Print it out. This is going to be my friend's copy. 4) Now I want a new wallet, also with the passphrase sneaky, so I click 'random-generate new keys' 5) Print out wallet. This is the BIP38 version for my friend to keep save. 6) Turn off BIP38, because I want to print out an unencrypted copy for my own records.
Result: "No private key available. Do you want to generate a new one."
I really do appreciate that you thought to pop up an alert to handle this scenario, but is there really no way we have access to the private key?
What makes me curious is that of course if I take that same "no private key available" BIP38 private key and paste it into bitaddress.org along with my passphrase, it can reverse-engineer the WIF-format unencrypted key. Our wallet knows the passphrase, it knows the BIP38 key, so can we make it so that you can toggle BIP38 on and off for any given wallet, whether it was generated with the checkbox turned on or not?
* Number Two * There are a few situations where should have some "please wait, doing BIP38 encryption/decryption" feedback. (I would always prefer a message to a spinning graphic as well.) For example:
1) Turn on BIP38, supply a passphrase 2) Click 'supply my own key' 3) Paste in a WIF key (5***)
Result: browser appears to hang with no message until key is generated.
I'd like it if anytime BIP38 work is being done, a clear message is displayed especially for the benefit of people using slow laptops they have dedicated for printing wallets. It can take minutes!
* Number Three * Should we make it so that the "supply your own key" feature allows you to supply either a WIF key (5_) or a BIP38 key (6_)? I guess we'd have to also let them type in the passphrase at that point... Kind of a hairy mess. It would definitely be appreciated by people who intend to sell or give away blank paper wallets to people who provide their own BIP38 keys. It could possibly also give us at least one way to let people decrypt their own BIP38 encrypted wallets, which would be a welcome feature.
Anyway that's my feedback so far.
Again, excellent work. With your permission what I'd like to do by way of compensation is start sharing donations that bitcoinpaperwallet receives. Please email me privately at canton@gmail.com and we'll work out a fair arrangement to compensate you for your excellent work.
Thanks again!
- Canton
canton@gmail.com • (505) 570-0635 • http://cantonbecker.com
— Reply to this email directly or view it on GitHubhttps://github.com/cantonbecker/bitcoinpaperwallet/pull/6#issuecomment-31269510 .
artiomchi
commented
10 years ago Hi Canton,
I'm really glad you liked what I did.. So I'll continue on it, if you don't mind :)
And that's some great feedback! Really!
I'm glad you noticed this behaviour.. I've only had a couple hours this morning to code, so I forced it to show that message, until I got to discuss with you a better solution...
So far, looking at the code, it seems like generating a BIP0038 encrypted wallet will use the passcode as a seed, to make the generation more random.. As such, the code to generate a new random encrypted wallet is different than making an unencrypted one, and I don't think it provides us with a non encrypted private key..
That being said, nothing stops us from decoding the key, but that would take more processing time, so I've left it at that this morning.
I'm going to check what's the best I can do about it, but at the moment I see two main options:
a) Leave the generation code as it is.. If we switch to an unencoded wallet - we'll run the decoding function to get the wif private key b) Change the generation code so that it generates an unencoded wallet every time, and if we have the encode option turned on - encode the wif key using the supplies passphrase. While in theory this approach should be just as good, I want to make sure we won't compromise the randomness of generated wallets this way.
I think that's a good idea, but for that I think a better interface would be needed. Possibly a popup dialog (similar to BIP0038 settings one) that would let you enter a private key, validate it, and query for a passkey if it's encrypted.
If you're interested, we could even add some code that would use the HTML5 camera controls, so that you could scan the qr code directly, instead of having to key it in.. I'm sure more than a couple users would find that useful, no?
Finally, I know this code is based on bitaddress.org, but how close to the original do you want to keep it? I mean, bitaddress.org has a lot more options than this site uses, the wallet generation has multiple checks and switches to support generating wallets without design, to generate multiple wallets at the same time (controls that are hidden on the page) and so on.. Are you interested in keeping them, or are they left in as legacy code at the moment, and you're not interested in keeping them?
If it's the latter, I could make some changes to the ninja class, that would make it more streamlined and manageable, since I've had to leverage the existing classes to do my bidding, but I don't really find the current solution elegant and to my liking, if you know what I mean :)
But it's your project, and you're the boss, so I'll follow whatever path you choose
Cheers, Artiom
artiomchi
commented
10 years ago Regarding point 1. above - I've just read parts of the BIP0038 spec - there are two ways of generating an encoded address -
The second approach is used when you want a third party to print your paper wallets, or hard coins, but don't want to compromise these wallets. It's a good feature, but considering that we generate the addresses in the same context where we have the passcode - completely unnecessary, since both methods are just as secure, as long as your passkey is secure.
As such, I propose to change the BIP0038 encoded wallet generation to generate a random address, and encode the known private key. This will get rid of that issue whatsoever.
Cheers, Artiom
cantonbecker
commented
10 years ago I'm going to check what's the best I can do about it, but at the moment I see two main options:
a) Leave the generation code as it is.. If we switch to an unencoded wallet - we'll run the decoding function to get the wif private key
I think this is totally acceptable. The use case of someone switching back from BIP38 to non-encrypted is going to be pretty rare, and having the user wait through a decode cycle seems perfectly fine to me. I agree that significantly changing the key generation method / hooks could be worrisome. Now that's interesting.. I've just tested it locally, and it seems to have worked for me
What browsers are you testing on? Me: Safari & Chrome OS X, and Firefox Ubuntu. I'll gladly provide you with a matrix of which browsers demonstrate the problems and in which situations.
I think that's a good idea, but for that I think a better interface would be needed. Possibly a popup dialog (similar to BIP0038 settings one) that would let you enter a private key, validate it, and query for a passkey if it's encrypted.
Let me meditate on this some, including the idea for HTML5 camera controls. Finally, I know this code is based on bitaddress.org, but how close to the original do you want to keep it?
It's fine line. I don't mind gutting out irrelevant stuff, as long as it doesn't make it too much more difficult to compare bitaddress.org code vs. bitcoinpaperwallet.com.
Something I try to do is make sure that someone who wants to verify the integrity of the RNG and such can easily "diff" the code and see that the changes are all interface, not crypto-related.
If it's the latter, I could make some changes to the ninja class, that would make it more streamlined and manageable, since I've had to leverage the existing classes to do my bidding, but I don't really find the current solution elegant and to my liking, if you know what I mean :)
I'm really open to this idea, especially if we can leave all crypto related functions more or less unchanged (and able to follow updates/upgrades/changes to bitaddress.org as they continue to evolve. I do manual merges from time to time to follow updates.)
- Canton
Canton Becker canton@gmail.com • (505) 570-0635 • http://cantonbecker.com
artiomchi
commented
10 years ago If necessary, we can even do better, and allow for both implementations (e.g. to allow bitcoinpaperwallet.com to be used by third parties printing wallets for someone). This could be implemented using a dropdown of supported wallet encryption methods, like you suggested here: http://www.reddit.com/r/Bitcoin/comments/1topvo/bitcoin_paper_wallet_with_bip0038_support_please/ceaggq4
artiomchi
commented
10 years ago as long as it doesn't make it too much more difficult to compare bitaddress.org code vs. bitcoinpaperwallet.com.
All right, that's what I wanted to know..
Basically, the page is made up of open-source scripts (Crypto, EC, Bitcoin, Crypto_Scrypt and so on, that I'd rather leave completely untouched), the ninja.privateKey, ninja.publicKey, ninja.qrcode helper classes that don't really need to be changed either, and ninja.wallets.* that contain the link between the page layout and the rest of the objects.. Only the latter need changing, since your interface is different, some parts are not used, and others might work differently.
This way it can be more manageable, while keeping it close to the source, so that it can be diffed or updated
npudar
commented
10 years ago One additional feature as part of the BIP38 capability is to start from a brainwallet passphrase to create the first WIF private key. I would then use that private key and BIP38 (with a new passphrase) to create the encrypted private key. And finally, it would be great to be able to have a "hint" that I could enter on the form, and then have it printed on the paper wallet. (Otherwise, I would have to write that hint on the paper wallet myself. But having it printed out would be a nice touch.
cantonbecker
commented
10 years ago Regarding what level of BIP38 support to offer, my inclination is to only handle the most common scenario -- someone password-protecting a wallet they are generating for themselves -- and not bother dealing with the "EC multiply" cases. As I read it, EC multiply is fairly specific to selling loaded paper wallets. While this is a practice I won't actively discourage, I'm not at all inspired to put time or money into developing it. The fundamental idea behind bitcoinpaperwallet.com is to empower individuals to print and load their own wallets without any 3rd parties whatsoever.
So:
1) Yes let's only do regular old passphrase BIP38, no need to do EC multiply or have a dropdown at this point with additional encryption methods
2) Similarly, let's not complicate the interface for "supply my own key" -- for now let's leave it so that "supply my own key" only means supply a 5*\ style WIF.
3) Artiom, I believe I'm in total agreement when you say "I propose to change the BIP0038 encoded wallet generation to generate a random address, and encode the known private key. This will get rid of that issue whatsoever." I understand this to mean that by hooking up the BIP38 encryption a little differently, we will always be able to uncheck the "BIP38" checkbox and render the corresponding un-encrypted wallet (and we won't even have to undergo a decrypt cycle.) If we go that route, let's be sure to run the final code to a few folks to verify that we haven't compromised anything e.g. by not using the passphrase as an additional entropic seed (if that's one of the compromises with your approach -- I'm not clear on this.)
A sidebar to #2: If we WERE going to enhance the "supply my own key" function at all, the format I really want to support is being able to type in any significantly long string (say minimum of 100 characters) and have its SHA256 hash used in generating the keypair. I suppose this could be used for brain wallets, but more importantly, we could guide users to type in the results from throwing a 6-sided die 100 times. I'd like to give users at least one option that doesn't involve trusting the RNG.
Question of the moment: What do you think? Instead of rolling a single 6-sided die 100 times, is it just as random to throw ten 6-sided dice 10 times, provided you accurately reflect the actual left-to-right physical position in which the multiple dice fall?
artiomchi
commented
10 years ago Hi Canton,
After reviewing the BIP0038 spec yesterday, I believe my initial assumption that the passcode was used as a random seed was inaccurate.. The code was using the EC multiply version of the encoder, hence the two-call approach in code. I'm really not entirely sure why they did that, instead of encoding a randomly generated address in the first place, since both are the same in terms of security, and both use the secure random generator get a random seed.
I've made some changes to the ninja class as discussed, to make it a bit more manageable, while keeping it close enough to the original source. None of the open source library code was changed at all. I'm generating a new address, and if encryption was enabled- encrypting it with BIP0038. This way we always have a private key as well for the generated address.
The loading progress seems to be working fine, I may know what was happening (browser wars, they will never cease!), so hopefully it should work in any browser now.
I haven't touched the "Supply your own key" for now, but it would be a good idea to have some option to decode a BIF0038 encoded address. I'm sure people would appreciate if we not only allow them to encode their addresses, but also test and decode them back if needed. Could you come up with an interface piece for that?
Regarding the last point - generating a new address is done by the ECKey class from bitcoin-js, and it doesn't allow for a custom seed, it seems. Maybe we could extend the code to do that, but I'm not sure how good of an idea is it to play with the bitcoin-js code...
P.S. Now that's a good question. Without thinking too much into it - I'd say it's completely the same.. But I know statistics are never that easy, so I'm sure there is some difference. I wouldn't be able to say exactly how different it would be, but I think it would still be random enough :)
artiomchi
commented
10 years ago UPD: Nevermind.. I can just run any random text through SHA256, and feed the result to ECKey, which would generate a address using this feed! So if you're interested in adding this functionality - it should be quite simple :)
In fact, if you enter any non valid private key to the page, it will suggest if you want to use the text to generate a new address :)
Technically, this is basically the same code that generates brain wallets, by the way...
npudar
commented
10 years ago This is some great work! I really like the new implementation of the brainwallet capability. Now I am able to run my deterministic encrypted brainwallet sequence very many fewer steps. (Than with the combination of bitaddress.org and bit2factor.org) So after creating my first encrypted private key from the initial brianwallet phrase, I can now feed each generated encrypted private key as my next brainwallet passphrase. Beautiful. I'm really looking forward to these changes becoming part of the official release.
artiomchi
commented
10 years ago I deleted the previous message since it was just wrong. Looking at the source of Crypto_Scrypt, I noticed that it's using web workers to run the processing. It's an HTML 5 feature to run some processing in a background thread without blocking the UI. That's why when you run the page in Chrome and Firefox (which support this new feature) the loading animation works fine, while when you run this in other browsers (IE, Safari) the animation is not displayed/frozen.
The best I can do at the moment is delay the processing 10ms after I show the loading bar.. This way the loading animation will be shown anyway, even if the browser doesn't support web workers. Please tell me if this works on your side
cantonbecker
commented
10 years ago The best I can do at the moment is delay the processing 10ms after I show the loading bar.. This way the loading animation will be shown anyway, even if the browser doesn't support web workers. Please tell me if this works on your side
This seems totally acceptable, even delay 500ms just to be on the safe side in case some browsers/javascripts need some extra time to get up to speed.
BTW, if it's easy (and if you agree this is a good idea) my preference would still be to have an actual explanatory message in a "lightbox" instead of a abstract spinning image, like this:
http://cl.ly/image/1I1g3t1k1316
I was running this code on a single processor core virtual machine last night during testing and encryption really did take 2 minutes, so I think it's generous of us to be as explicit as possible while we completely hijack their browser :)
cantonbecker
commented
10 years ago Artiom,
I think we're getting really close to a release candidate here -- something I can pull down and start tweaking for a few minor design issues and push out for peer review/testing.
I've confirmed two of your fixes this morning:
1) unchecking the BIP38 checkbox now restores the WIF format key (5xxx), and does so instantly with no decryption required. (Huzzah, nice job! Very intuitive, works just as I hoped it would.)
2) The progress indicator now shows on both Safari OS X and IE9. However it doesn't actually spin, just looks like a funny crescent shape which makes it seem like the browser is broken and not doing anything . Clearly not your fault and not something we can fix, but it reinforces my desire to have an actual written-out message instead of an animated symbol that may or may not animate.
* Outstanding issues *
1) The JS console (on firefox at least) shows an error when no vanity key has been entered. Probably not critical but worth addressing:
Line 7317 text is undefined var lengthCalculation = text.length * 8 + 12; // length as calculated by the QRCode
2) Also, before I send this up as a release candidate, I'll want to fold in some of the changes that have taken place on bitaddress.org over the last few weeks -- in particular they've been working on their entropy, and they've finally implemented window.crypto.getrandomvalues themselves. Their wrapper for getrandomvalues() is slightly different than what I implemented (what Gavin sent me) a few months ago -- but I'd like to follow their code letter-by-letter to make auditing / diffing as easy as possible.
Do you want to merge these latest bitaddress.org changes in since you've been touching some of the ninja crypto stuff, or should I?
artiomchi
commented
10 years ago Hey Canton,
Regarding the first second point - yeah, I can't really do much about the animation being stuck on these browsers.. At least it is showing the busy screen, which I can improve to show a progress dialog instead.. I was thinking that it would be nice if it still had some kind of spinner though.. Or should we just go without a spinner completely?
On that note - I always loved this site when I needed one: http://www.ajaxload.info/
Regarding outstanding issues:
Umm.. Could you help me reproduce it? What OS and version were you running this under? Did you just switch to "Supply your own key" and pressed "Apply" with an empty text box?
I just installed Firefox and tried it locally and it worked fine (on a Win 8 machine).. As far as I know - it shouldn't even have triggered the qr code =/
I'm settings up a raspberry pi to test it out as well
cantonbecker
commented
10 years ago As for this JS error:
Line 7317 text is undefined var lengthCalculation = text.length * 8 + 12; // length as calculated by the QRCode
I'm only seeing that in firebug running on an older version of firefox (7.x). I don't see this in a more modern version of firefox so I'd just chalk this up to old browser errata and forget about it.
Regarding the progress indicator, what do you think about my lightbox mockup instead of the spinner?
artiomchi
commented
10 years ago Oh, the progress indicator is perfectly fine.. I'm just not that awesome with css, so it took me more time than I expected to get the dialog working by showing/hiding only a single element, while keeping backwards browser compatibility..
Just for fun, I installed Firefox 7.0.1 and tried to replicate it - still no error.. :(
cantonbecker
commented
10 years ago Hi Artiom,
I was away for a couple days while traveling. So -- do you think we're ready for a release candidate? If you're done with the BIP38 edits you wanted to apply, I'm ready to start doing some extensive reviewing and testing to fold in your pull request for general consumption.
Just wanted to make sure you were at a stopping point before I started digging in deeply.
Thanks for your awesome work so far!
artiomchi
commented
10 years ago Hi Canton,
Yup, I believe we should be ready to go..
In a way, it would be nice to have BIP0038 decryption as well, if you'd want that in - we can come up with some interface for it.. But in terms of encryption - I think we're good to go!
cantonbecker
commented
10 years ago In a way, it would be nice to have BIP0038 decryption as well,
You're right. Let's be sure to implement that alongside this update. Otherwise the bitcoinpaperwallet.com software is no longer stand-alone: You'd have to download bitaddress.org also onto your Ubuntu USB drive or whatever if you wanted to recover a BIP38 wallet.
Do you think web camera QR coding is a remote possibility? Would be damn slick. Just in case, I've included this in the mockup, though we could definitely do without.
MOCKUP: http://f.cl.ly/items/1h22183v120k2i0q0A1L/validate-mockup.jpg
If you decide to implement this, as always, please don't let any layout / CSS stuff slow you down. You can just rough it out and I'll tweak the design.
Mostly this is just a minor redesign of the existing service from bitaddress.org.
Here's the text used in the mockup so you don't have to type it out.
Enter or scan any private key into this form to display all available details and formats for that key. This should work for private keys generated by BitcoinPaperWallet.com as well as any other Bitcoin service.
For example, if you have a wallet encrypted with a BIP38 passphrase, you can use this form to decrypt your private key and retrieve the standard Wallet Import Format (WIF) key suitable for wallet software and services that don’t directly support BIP38 importing.
Canton Becker canton@gmail.com • (505) 570-0635 • http://cantonbecker.com
artiomchi
commented
10 years ago Hi Canton
Do you think web camera QR coding is a remote possibility?
Oh yes! Absolutely! I've played around with it a little, and it definitely is.. It's going to increase the file size, but I don't think it'll be a problem..
The library has support for HTML5 camera APIs, and falls back to flash support if the first is not supported.. To be honest, I'm not sure if I should include flash support (just in case - flash is know for it's security issues, and it's an external plugin that you have to install on the browser), so I'll just leave the HTML5 support only.. What do you think?
I like the new design - looks great! I'll get some time to plug it in within the next couple days.. Shouldn't be a big problem
UPD: I forgot to mention.. I saw you added a link to run the unit tests in the page.. The new code from bitaddress.org also has async tests (for testing things like BIP0038 encryption.. They're started with a slightly different link, as you can see in the code, and run for quite a bit of time.. Do you want them added as a separate link on the bottom?
cantonbecker
commented
10 years ago The library has support for HTML5 camera APIs, and falls back to flash support if the first is not supported.. To be honest, I'm not sure if I should include flash support
I 100% agree. Just do HTML5, leave out flash fallback. I've already got a very nice library installed for detecting browser/version/compatibility so I can be smart about it and hide the feature if I know it won't work.
artiomchi
commented
10 years ago Yup, will do that.. Except I won't use the browser detection library.. In my experience browser detection is really bad, with only a couple exceptions. In your case, I think you're using it for the print calibration, so it sounds like a good solution..
But with the qr scanner, I'll be using feature detection to check if camera APIs are available in the browser. This way if a browser has support for it - it will work immediately.
Artiom
cantonbecker
commented
10 years ago Artiom,
For your next revision, will you be updating the github pull request? I think recently I've only been able to review your latest changes on your live website demo.
PS: This is kind of fun. Look at the graphical display this person (mikewood) includes as a way of giving feedback as entropy is gathered from mouse movements.
Not something I want to pull from -- I don't like the dependency on jquery at all -- but I do think this is a neat implementation.
artiomchi
commented
10 years ago Hi Canton,
Are you sure? As far as I can see, all new comments that I'm pushing are being tracked in this pull request.. Also, if you look at the "Files changed" tab on the top, you can see a "final overview" of all the changes, compared to your master branch.
I haven't finished the last part just yet, been working late the last couple days, but I'll be finishing with the qr code scanning soon, and will drop a message here when I think it's ready for a final review & merge...
The azure site that I linked above is linked to my branch directly, so any time I commit it will publish there automatically.. I guess until we merge, it's a good place to follow the progress, unless you want to run it locally :)
P.S. The offline address thing looks fantastic! Really! I'm pretty sure it should be possible to implement that here as well. After all, it's just drawing it on a canvas, so it hardly depends on jQuery.. The license is permissive as well, which means that's not a problem as far as I can see.. We can look at it, if you want, after we finished with the new functionality
cantonbecker
commented
10 years ago Are you sure? As far as I can see, all new comments that I'm pushing are being tracked in this pull request.
Sorry for the confusion. I think it's something quirky with my github client (or my failure to understand it.) My github client says I'm looking at the latest version of your code (167b59b) however the actual HTML it loads onto my hard drive is for the very first draft version you did. See: http://cl.ly/image/2a2x2d3t082w
Anyway I'll figure it out on my end. The github website definitely has the latest code you've been working on.
P.S. The offline address thing looks fantastic! Really! I'm pretty sure it should be possible to implement that here as well. After all, it's just drawing it on a canvas, so it hardly depends on jQuery.
I agree it looks great, and let's hold off on anything. What I figure would be interesting at some point is a whole "entropy dashboard" which would give you summary information about all your bits of entropy (and ways to increase it, e.g. by rolling dice, or by generating a random code and then appending few die rolls to it and re-using it as a brainwallet.)
Right now I think the whole entropy business is a bit confusing because of how random number generation is an amalgamation of
Canton Becker canton@gmail.com • (505) 570-0635 • http://cantonbecker.com
artiomchi
commented
10 years ago Hi Canton,
Please take a look at the last commit. It should have the webcam scanning functionality working, as well as a tab validating private keys.
I've pulled in the library (with the reference to it in the beginning or the script block), and created a separate block with a javascript class that I wrote to maintain that library.
I've added some html to display and control it, and extended the ninja.wallets.detailwallet class to handle that.
The private key loading is mostly the same code taken from bitaddress.org.
The QR scanner only uses HTML5 camera APIs. The scan button is still available even if not supported, and if you click it - it will display a dialog with a message to try it with a browser that supports the camera APIs.
It probably needs some better wording, and maybe examples or something.. I've just written some basic text to explain it.
At this moment I'm waiting for your feedback, and if there are no bugs/issues - it should be ready to merge, I believe
cantonbecker
commented
10 years ago Please take a look at the last commit.
Super awesome. I've just begun my testing.
So far so good, but one thing: I got the camera to pop up with chrome, but when using an old version of firefox and a new version of safari (neither of which will support the camera) I didn't get any kind of a dialog saying, "not supported." I just got this:
http://cl.ly/image/01252v0R3612
Is that the intentional behavior or should I see a message of some sort for non-supporting browsers?
http://caniuse.com/#search=getUserMedia
artiomchi
commented
10 years ago D'oh! Sorry for that.. It was working in the beginning, then I started the cleanup, and removed the camera controls that I used for testing.
When you used an unsupported browser, it was supposed to hide those controls, but I forgot to clean up that 1 last line of code..
It's working properly now
cantonbecker
commented
10 years ago I've begun some of my most nitty-gritty testing, and here's the first oddity I've run up against. It's a fairly serious issue because it involves a failure to recover a bip38 encrypted key in one case.
Test scenario:
1) click print front 2) click 'supply my own key' and enter 5J4pcwBDPwPY1cdNqxTdmZWr7yCK8rXi9avFvezgYbmoatJpKGn 3) this should result in an unencrypted wallet with vanity address 1ABCDF5v4oaodTPYnKfYvkfwuoa8PJkjMC 4) turn on BIP38 encryption with the following passphrase (no quotes) "घोडा स्टेपल" (If github chews up this UTF8 text, go to google translate and ask google to translate the phrase (no quotes) "horse staple" to Nepali.)
Result:
Chrome, IE9: Works perfectly, encodes to 6PRNXA7M4qEppBJCHM2SEizfna7XTomzXwdCBrEG6Mjo3nU6iziS6vWWXA
Firefox: Works fine, but passphrase appears blank beneath BIP38 checkbox.
Safari OS X: Passphrase appears correctly, but encodes to the wrong address. Danger will robinson, danger! 6PRNXA7M57uqSYXX2TXHkfNJEVMiWarkPkqUv3AsZa5r41u3VpXHLkUD9q
Things get a little weirder yet:
Again using Safari, if you random-generate keys using the same passphrase "घोडा स्टेपल" they appear to bip38 encode properly. Hmm.
Also, it's not a generalized UTF8 character set problem, because vanity key encoding with "马主食" on safari -- google translate of "horse staple" to simplified chinese -- works fine.
cantonbecker
commented
10 years ago PS I may have gotten a similar result / problem using bitaddress.org, so don't go crazy on this until I get a response from pointbiz and his team. It might be something endemic to the code we're borrowing, not the code specifically as you've implemented it.
cantonbecker
commented
10 years ago Did some more research. I'm less convinced that bitaddress.org has any problems with its own encoding of BIP38 on safari when using weird UTF8 characters.
But here's what's totally doing my head in, making me crazy-faced:
To recap, when encrypting "5J4pcwBDPwPY1cdNqxTdmZWr7yCK8rXi9avFvezgYbmoatJpKGn" using Nepali phrase "घोडा स्टेपल", Safari gives us 6PRNXA7M57uqSYXX2TXHkfNJEVMiWarkPkqUv3AsZa5r41u3VpXHLkUD9q while all the other browsers give us 6PRNXA7M4qEppBJCHM2SEizfna7XTomzXwdCBrEG6Mjo3nU6iziS6vWWXA
Check this out:
Using the 'wallet details' tab on bitaddress.org to decrypt the BIP38 key, if using safari, the safari-generated key (_9q) works, and the other-browsers key (_XA) fails to decrypt. If using chrome, the reverse is true. So you have to use the same browser to decrypt as the browser you used to encrypt.
Using the "decrypt private key" function at bit2factor.org, the non-safari key works, the safari-generated key does not.
The matrix of possibilities -- browser used to generate the key vs. browser used to decrypt the key vs. website/service used to decrypt the key -- is going to make my head explode.
My temptation right now is to do some hardcore limiting on the BIP38 allowable characters, something I could do myself pretty easily though I'd like some feedback. I'm thinking:
Only A-Z, a-z, 0-9, plus spaces, dashes, and a few common symbols. Maybe a few more, enough to support typical "Latin" languages. I know this limits the search space / brute force attack space hugely, but I'm really concerned about users creating unrecoverable wallets due to the use of clever/strange characters like braille ⠜⠇⠑⠹ or worse yet, accidental copy-pasted tabs or carriage returns which are impossible to type into a decryption field by hand.
I think we have to aim for a lowest-common denominator solution, because even if we fix this quirkiness between our generator/decrypt and bitaddress.org decrypt, we have to play nice with all other major software and services that support BIP38. For example, let's say coinbase implements BIP38 import, but they totally fail at accepting any form of Nepali characters at all. Technically, it's coinbase's problem, but in reality if someone's holding one of my wallets and coinbase is saying "decryption failed" then heart-attacks will ensue.
Thoughts?
npudar
commented
10 years ago Canton,
That must be incredibly maddening! Clearly underlying quirkiness of special characters. I think your recommended approach of limiting the character space is the prudent one. It should be the user's responsibility to ensure sufficient entropy exists in the passphrase.
That prompts another slight tweak -- maybe provide a dynamic readout of the implied number of bits of entropy near the passphrase field as it is being typed in. Another text line would recommend a target number, or provide a link to somewhere that provides more information. Maybe even refuse to accept passphrases that generate ridiculously low levels of entropy?
Nick
artiomchi
commented
10 years ago Hi Canton,
So there are two issues here, really..
First one is that Firefox does not show the passkey in the page (even though the encryption works fine). That's a new one I learned today - apparently Firefox is the only browser (at least among the major ones) that does NOT support .innerText property =/ Instead is supports the textContent property (which in turn is not supported in older IE versions.. sigh)
http://www.quirksmode.org/dom/w3c_html.html#t04
I haven't used innerHTML, since I want to make sure the passphrase will appear exactly as the user has entered it, even if it has angle brackets, etc.
The second one is that the passkey is "converted" somehow in safari. I've tried in a bunch of Windows browsers (Chrome, Firefox, IE 10 and Safari) and in every case I got the same encrypted passkey. That doesn't say there isn't an issue though.
In the very beginning, I noticed that you changed the character charset on your page from utf-8 (as it is on bitaddress.org) to iso-8859-1. I didn't question it, and hoped we won't run into issues. I suspect this may be the issue in this case.
I've changed the charset in HTML back to utf-8, re-saved the file under UTF-8 with BOM encoding, and pushed the changes to git. Please let me know if it gets it working in Safari on Mac OS X.
I hope it does, since while limiting the characters you can enter as a passkey will technically work, I think it's a very bad solution, that will turn people away from your site.
After all, the beauty of BIP0038 is that it uses unicode characters, which massively increases entropy with the same length password - no wonder someone put up their paper wallet with a 3 char password on Reddit, and it hasn't been cracked for a long time! Brute-forcing an ascii (or even a subset of that) password is massively faster than a full-on unicode one.
Also, if you implement that filter, then if an attacker will see an encrypted paper wallet with your design - he will immediately know that this wallet is immediately hundreds of times less secure then the same one next to it printed from bitaddress.org. Not something you want your site to be known for :(
artiomchi
commented
10 years ago Interesting point regarding whitespace in the passkey field..
I could trim it (and that's what bitaddress.org does, actually), but what if we checked for whitespace, and prompted the user with something like "We detected you have some whitespace characters around your passkey. This usually happens when you accidentally copy the passkey from someone else and accidentally include them. Would you like us to remove them?"
This way if the end user wants his passkey to end in a space (for whatever reason) they can do that, and we don't force-trim the field.
Or should I just trim the passkey field by default?
What do you think?
cantonbecker
commented
10 years ago Hi Artiomchi, Thanks for your speedy response as always. I wondered about the UTF8 vs iso-8859-1 charset as well, but I just checked and it didn't change anything.
My Safari still encodes 5J4pcwBDPwPY1cdNqxTdmZWr7yCK8rXi9avFvezgYbmoatJpKGn with घोडा स्टेपल as 6PRNXA7M57uqSYXX2TXHkfNJEVMiWarkPkqUv3AsZa5r41u3VpXHLkUD9q
whereas other browsers encode it as 6PRNXA7M4qEppBJCHM2SEizfna7XTomzXwdCBrEG6Mjo3nU6iziS6vWWXA
Notice the introduction of the possessive "My" Safari. After further testing I think this may get even worse: it may not just be a matter of needing to use Safari, but the same version (or OS?) of Safari to successfully decode. Argh!
Also I'm pretty sure this isn't just limited to our BIP38 encryption. I was able to produce a Safari-only decodable BIP38 wallet using bitaddress.org as well. See:
https://github.com/pointbiz/bitaddress.org/issues/56#issuecomment-31827302
Crossquote: "At the moment I'm thinking the problem here is something to do with the BIP38 code where depending on the browser you're using (and with Safari in particular) some extended charsets are interpreted differently -- whether encrypting or decrypting -- which is why you are obligated to use the same browser to decrypt as you used to encrypt."
cantonbecker
commented
10 years ago Regarding firefox .innerText vs. IE . textContent, all I can offer is my sincere condolences backed by well over 10 years of having dealt with similar browser-exclusive quirks. It's like when you're smoothing out the bubbles beneath a screen protector and when you smooth out one air bubble, an opposite air bubble pops up...
Anyway, I assume this can be addressed with a wrapper function that tests the property availability or something. Let me know if it's a more serious problem.
As for whitespace, the only whitespace I want to allow is spaces. The problem of people copying/pasting tabs or worse yet carriage returns without realizing it could be very serious, since they might test their ability to decrypt the wallet also by copying/pasting and wouldn't realize the issue until years later when they fail to decrypt their wallet typing in the passphrase by hand for the first time.
As for extended charsets, I'm not yet ready to throw out the baby with the bathwater and disallow anything besides plain-jane aA-zZ0-9. I agree it limits the brute force space too much. Besides which, it's not only our problem -- I believe this is endemic to all services doing javascript BIP38. I'm willing to wait and see if those communities have any thoughts on this issue before we do any limitation.
That said, I've definitely got my "the sky is falling" hat on. Unless someone points out that I've got something seriously wrong with my testing methodology, I'll put up a PSA on reddit on thursday saying, "Hey if you use BIP38, you might not want to use extended charsets for the time being."
artiomchi
commented
10 years ago Don't worry about the innerText vs textContent property field.. I've already fixed it in 22f3817 :)
I'm doing a bit more testing for the other issue.. I've confirmed that it doesn't seem to have anything to do with unicode, and might be an issue with Safari, actualy.. Specifically, Safari 6!
I don't have a Mac to test it with, so I got a trial on www.browserstack.com, and tried it there. As previously discussed, I tested it on Windows 8, latest Chrome + Firefox + IE 11 + Safari, and it works exactly as intended.
I then went to browserstack, and tried it on different combinations..
(private key used: 5J4pcwBDPwPY1cdNqxTdmZWr7yCK8rXi9avFvezgYbmoatJpKGn, passphrase: घोडा स्टेपल )
I've then tried to do something else.. Use a NON unicode passkey:
Just to prove that, I went to bitaddress.org, and tried to test the private keys from the previous test, with the simple passkey:
UPD: Same result on bitaddress.org
What this basically is proving is that the system works fine on Safari 5 and 7, but on Safari 6 the actual encryption/decryption code provides different results, compared to all other browsers. It's not related to UTF-8 as I originally expected, I think it might be something in the actual encryption code working differently on Safari 6, which is actually quite worrying!
cantonbecker
commented
10 years ago What this basically is proving is that the system works fine on Safari 5 and 7, but on Safari 6 the actual encryption/decryption code provides different results, compared to all other browsers.
Holy crapballs.
That is all.
artiomchi
commented
10 years ago Updated post above with confirmation that the issue also exists in bitaddress.org and bit2factor.org =/
Holy crapballs indeed
artiomchi
commented
10 years ago Hi Everyone,
I've spent the last 4-5 hours on this, and it feels like I'm going insane.. I don't have a mac as well, which means I have to do it over a Flash-VM interface, which slows me down a lot!
Anyway, after adding multiple traces to the code, I've eventually reached a conclusion - Safari is terrible.. Among other things, it renders console.log output LONG AFTER you call them, sometimes with completely different results, and after the variables you log have been changed =/
So much for helping developers...
After wasting 4 hours on fake readings, I got some results... It needs more logging, but it's almost 3 AM, and I REALLY need to sleep. At this time, I can see that the call to Crypto_Scrypt is returning different results when started on Safari 6, than in other browsers.
I may be wrong, since I'm pretty silly sired at this point, but that's what seems to be causing the issue here.
I'll try dig deeper tomorrow evening
As discussed in issue #3 - we love this wallet generator, but it lacks BIP0038 support.. I'd love to create a paper wallet, and then print a copy of it and give it to my friend/brother for safe storage, while not worrying that someone might take the money off the wallet.
I've taken the encryption code taken directly from the live bitaddress.org version