This patch adds a –disable-bip70 configure option that disables BIP70 payment request support in the wallet GUI. BIP70 support remains enabled by default.
When disabled, this breaks the dependency of the GUI on OpenSSL and Protobuf.
(triggered by discussion on IRC today)
For information, I plan to make BIP70 deprecated into NBitcoin. (By removing it from main lib, and moving it to separate package)
This led me to too much dependency issues, as well as cross implementation issues as you can’t check correctly the signature of the payment request without serializing the payment request exactly as all other implementations does. (Typically, my implementation works against all wallets, except copay for reasons…)
As a library/service developer, I would love to see another simple standard, filling the same need, Json simple binary based, just using HTTPS for securing the communication between wallet and server.
I remember there was discussions about making a new payment protocol long time ago, I think it was initiated by @sipa.
Concept ACK
Concept ACK generally, though I’m curious where you see this going longer-term
My opinion on it is really divided. I like BIP70 in concept (automatic refund addresses, key expiration, allowing the wallet to directly authenticate vendors, direct transaction submission), but not the technical implementation, and also not the dependency burden it puts on bitcoin core.
Additionally it also seems the code is not maintained. No one is working on BIP70 support in bitcoin core.
So I’m ok with signalling that in the future we might be going to deprecate it, without any commitments - this would be the first step, to allow builders to disable it.
(my personal motivation is that I want to have the option to build the GUI without protobuf and without OpenSSL after the last remnants of OpenSSL use are removed from the rest of the code)
For information, I plan to make BIP70 deprecated into NBitcoin. (By removing it from main lib, and moving it to separate package)
Seems to be better design anyhow. Here it’s been peppered all over the GUI code :( Also for lib-ifying, isolating the parts affected like this is the first step.
Should add to travis matrix?
Agree that that’s a good idea to warrant that this keeps working.
159@@ -161,6 +160,9 @@ QT_MOC_CPP = \
160 qt/moc_walletmodel.cpp \
161 qt/moc_walletview.cpp
162
163+QT_MOC_CPP_BIP70 = \
QT_MOC_CPP += (and BITCOIN_QT_CPP +=) the actual files below, keeping all the stuff together.
1312@@ -1292,6 +1313,7 @@ echo " with wallet = $enable_wallet"
1313 echo " with gui / qt = $bitcoin_enable_qt"
1314 if test x$bitcoin_enable_qt != xno; then
1315 echo " qt version = $bitcoin_qt_got_major_vers"
1316+ echo " with bip70 = $enable_bip70"
251@@ -250,8 +252,10 @@ public Q_SLOTS:
252 ClientModel *clientModel;
253 BitcoinGUI *window;
254 QTimer *pollShutdownTimer;
255-#ifdef ENABLE_WALLET
256+#if defined(ENABLE_WALLET) && defined(ENABLE_BIP70)
#if defined(ENABLE_BIP70) would be enough all over the place.
666@@ -659,7 +667,7 @@ int main(int argc, char *argv[])
667 // Re-initialize translations after changing application name (language in network-specific settings can be different)
668 initTranslations(qtTranslatorBase, qtTranslator, translatorBase, translator);
669
670-#ifdef ENABLE_WALLET
671+#if defined(ENABLE_WALLET) && defined(ENABLE_BIP70)
bitcoin: URIs?
bitcoin: URL support too.
(it probably shouldn’t)
8+
9 #include "coincontroldialog.h"
10 #include "ui_coincontroldialog.h"
11
12 #include "addresstablemodel.h"
13+#include "base58.h"
paymentserver.h doesn’t indirectly include them anymore.
8@@ -9,6 +9,8 @@
9 #include "qvalidatedlineedit.h"
10 #include "walletmodel.h"
11
12+#include "base58.h"
13+#include "chainparams.h"
6+#include "config/bitcoin-config.h"
7+#endif
8+
9+#if defined(ENABLE_WALLET) && defined(ENABLE_BIP70)
10 #include "paymentrequestplus.h" // this includes protobuf's port.h which defines its own bswap macos
11+#endif
0@@ -1,5 +1,6 @@
1 #include "wallettests.h"
2
3+#include "base58.h"
Concept ACK.
I suggest also indenting nested #ifs with a single space.
“this breaks the dependency” -> “this removes the dependency”?
I know the goal is to get rid of OpenSLL, but what’s with Protobuf? Just fewer dependencies, or is there a specific problem with it?
Breaking bitcoin://1A1...Na&amount=0.001 would be bad; it’s used by various services. It’s often embedded in a QR code, which isn’t that useful on a desktop, but it can also be a link on a webpage or email too.
I know the goal is to get rid of OpenSLL, but what’s with Protobuf? Just fewer dependencies, or is there a specific problem with it?
Advantage of less dependencies is mainly: less stuff to build while cross-compiling, less attack surface (yet another parsing library), etc.
Breaking bitcoin://1A1…Na&amount=0.001 would be bad; it’s used by various services. It’s often embedded in a QR code, which isn’t that useful on a desktop, but it can also be a link on a webpage or email too.
I agree. It’s --disable-bip70 not --disable-bip21. @luke-jr already commented that, though. I’ll see if I can keep that part.
if (PaymentServer::ipcSendCommandLine()) is still #ifdef’d out…
But if (PaymentServer::ipcSendCommandLine()) is still #ifdef’d out…
Whoops, missed that. Fixed.
Nice! Concept ACK. I could see this for 0.16 and a default to disable in 0.17.
(my personal motivation is that I want to have the option to build the GUI without protobuf and without OpenSSL after the last remnants of OpenSSL use are removed from the rest of the code)
AFAIK OpenSSL (crypto) is still in use for the PRNG seeding (see currently closed #10299, waiting for new approach).
And as far as I can see there is a RAND_event in winshutdownmonitor.cpp.
AFAIK OpenSSL (crypto) is still in use for the PRNG seeding (see currently closed #10299, waiting for new approach).
Agree. The rand_ stuff should be removed in one go in a separate PR, it’s orthogonal to the changes here. (I don’t change the build system with regard to OpenSSL in this PR)
650@@ -647,7 +651,7 @@ int main(int argc, char *argv[])
651 QMessageBox::critical(0, QObject::tr(PACKAGE_NAME), QObject::tr("Error: %1").arg(e.what()));
652 return EXIT_FAILURE;
653 }
654-#ifdef ENABLE_WALLET
I did a make clean, ./autogen.sh, ./configure --disable-bip70 (which shows with bip70 = no) and make deploy.
I then tested BIP-21 using: ./Bitcoin-Qt.app/Contents/MacOS/Bitcoin-Qt bitcoin://1F1tAaz5x1HUXrCNLbtMDqcw6o5GNn4xqX?amount=0.1. This worked, but it did throw a warning (?):
QObject::connect: No such signal PaymentServer::receivedPaymentACK(QString) in qt/paymentserver.cpp:234
I think you need another #ifdef there.
QObject::connect: No such signal PaymentServer::receivedPaymentACK(QString) in qt/paymentserver.cpp:234
Thanks, added, also rebased and squashed.
//, see #11645 (comment)).
This patch adds a --disable-bip70 configure option that disables BIP70
payment request support. When disabled, this removes the dependency of
the GUI on OpenSSL and Protobuf.
Reduces the number of separate `#ifdefs` spans.
I disagree. This is just reinventing the wheel.
If you want to separate the payment processor from the merchant, just make so the payment processor give an https link to the merchant, and show the merchant domain of the merchant (+ certificate information if something could parse it before handling to the app) in the wallet inside the payment confirmation screen.
Adding your own protocol for signing a payment request is a bad idea that should never have existed in the first place. It bring this kind of heavy weight dependency without any advantage.
I am not sure I understand.
I think we should at least be targeting moving towards a world in which the bitcoin URI which generates a payment request includes a signature over the payment request (or a pubkey which will sign it)
Which I disagree, because the payment request already come from HTTPS link. HTTPS can be used to show who you are paying to on the mobile wallet. (no need of adding your own signature on top of that)
..If you are purchasing from a merchant, then you’re already interacting with their website and it is what gave you the payment link, we should be using that as a root-of-trust where at all possible, not using both that and existing SSL CAs
Which I completely agree but which seems to be the contrary of your previous sentence.
bitcoin:, how does the signer know that the pubkey signing it is from Coinbase? Response to this question is about re-implementing PKI, which browsers does out of the box, or is there any other alternatives?
The PKI usage in BIP70 is rarely used to verify the merchant itself, but is instead some third-party payment services provider (eg coinbase/bitpay/etc). Expecting to know which payment services provider a given merchant is using is not realistic for users, so I’m skeptical the PKI usage here is of really any value
Yes I agree, PKI as used in BIP70 is useless. What I am arguing is that by successfully downloading the payment request over a HTTPS channel, you also verified PKI through whatever arbitrary trust store you use underneath. (OpenSSL, Windows trust store, or iOS trust store) Most of HTTP libraries in all languages are already doing it out of the box, so there is no need to implement our own.
a better solution than requiring a signature over the payment request in the URI is requiring a public key which signs the eventual payment request in the URI
This is better, but you still have the problem of identity misbinding. How do you know that the public key which sign is coming from the merchant/coinbase? how can you be sure about the identity behind the key without relying on a PKI stack (SSL/TLS) or a web of trust?
Yes I agree, PKI as used in BIP70 is useless. What I am arguing is that by successfully downloading the payment request over a HTTPS channel, you also verified PKI through whatever arbitrary trust store you use underneath. (OpenSSL, Windows trust store, or iOS trust store)
So what? You haven’t gained anything in doing so aside from validating that your attacker is capable of getting an SSL cert issued, which is a free and automated process these days. Not sure what your point is here.
This is better, but you still have the problem of identity misbinding. How do you know that the public key which sign is coming from the merchant/coinbase? how can you be sure about the identity behind the key without relying on a PKI stack (SSL/TLS) or a web of trust?
My point is you dont get that via BIP70 either, and I dont see that changing at any point in the future. Still, you do get this (somewhat) from bitcoin: URIs, as you may expect a user to only click such a link on the expected website’s checkout flow (no different from where they enter their credit card). That is a very weak guarantee, but it is infinitely better than what BIP70 provides.
Ah ok I think I understand you now.
0bitcoin://?r=https://paymentprovider/id=invoiceId&merchantPubKey=abc
You then remove the PKI verification at BIP70 level (which is, as you said and I agree, useless). You replace is by a signature embedded in the payment request of this pubkey.
By doing so, you are sure that the payment uri and the payment request are at least bound together.
I was confused because in general this bitcoin: uri is not generated by the merchant’s website but by the payment processor. If both, the uri and the payment request is controlled by the payment processor, this scheme is not very useful.
--disable-bip70.
