Don't merge, NEEDS TO BE REWORKED!
Can please somone update @BitcoinPullTester to use current libs?
<pre> paymentrequestplus.cpp: In member function ‘bool PaymentRequestPlus::getMerchant(X509_STORE*, QString&, QString&) const’: paymentrequestplus.cpp:105: error: ‘class QSslCertificate’ has no member named ‘toText’ </pre>
Will rework to be compatible with Qt 4...
So we do not allow Insecure payment requests in general anymore? recipient.authenticatedMerchant is empty in this case, we show a yellow instead of green background there. Maybe handling this like the webbrowsers do?: Show a big fat warning (just a yellow background is too nice for an invalid certificate), but you can still send the transaction if you want to. But strongly recommended to not continue. A button to look at the certificate would be nice IMO.
Yes, I was wondering about issue #3628 too. But as far as I have tested this, there is no bug. The return value of getMerchant() is ignored, this is confusing, but QString& merchant is passed by reference, so recipient.authenticatedMerchant. This string is empty or not whether the certificate is valid or not. So thats our "bool" return value. So if the string is empty we show a yellow background. If not empty green. The only bug here is the yellow background misses a big fat warning.
Yes, I would say on track :) So pki == "none" ends up with the yellow background now.
To be honest I am not sure by myself what the expected behavior should be for an invalid certificate. There could be a temporary issue with the certificate. To not drive a store out of business in this case, maybe give the user an option to make an insecure payment request instead. But I can not judge, if this is a good idea or not.
@cozz I think invalid certificates should be treated like missing certificates but there should be a warning that the certificate is invalid (to aid people building sites that issue them). (as it just as easy for an attacker to strip the signature than corrupt it, it makes no sense to distinguish them in 'severenes' - IMO)
I get this error trying to build this pull request:
Undefined symbols for architecture x86_64: "PaymentRequestPlus::getMerchant(x509_store_st*, QString&, QString&) const", referenced from: PaymentServerTests::paymentServerTests() in test_bitcoin_qt-paymentservertests.o ld: symbol(s) not found for architecture x86_64
@Diapolo Please stop trying to force dependency upgrades without discussion. We should remain compatible with the versions packaged by major stable distros. That currently means Qt 4.6.2 (limited by RedHat). I'm pretty sure PullTester's is not older than that...
To quote myself:
Will rework to be compatible with Qt 4...
In a way it's good that pulltester tests with old libraries. My gut feeling is that none of the devs builds regularly against Qt4 (certainly not pre-4.8) anymore.
Sorry to weigh in late but I was away and am still catching up.
Please don't expose validation errors to users. Just silently fall back to treating them as if they were unsigned (and print errors to the logs). That way we can extend with new features later and even if they aren't fully backwards compatible, the worst that happens is some users see unverified requests instead of errors they almost certainly won't understand.
OK, won't show it to users. But better logging to the debug log is welcome.
Btw, we should change the Qt logging around a bit. Currently all log messages from the GUI are ignored unless -debug=qt is provided. IMO this should only apply to QtDebugMsg. QtWarningMsg/QtCriticalMsg/QtFatalMsg/QtSystemMsg should always be LogPrintf'ed.
Problems with Payment requests should at least have level QtWarningMsg.
Agreed on the logging change for Qt, even if this not the scope of this request.
We still have the serious problem that we don't check request.getMerchant() for success, which means current implementation of PRs is potential "unsafe". Will update this pull so that we log errors and don't allow request.getMerchant() to fail, but don't show the actual errors to users.
Damn, there is a problem with my assumption! Look at request.getMerchant(PaymentServer::certStore, recipient.authenticatedMerchant for insecure payment requests this call is allowed to fail, but for secure payment requests this should not be allowed to fail. An insecure PR has pki_type == none. @mikehearn Can you shed some light on this?
- fixes #3628 by showing certificate error(s) to user
- prevents accepting payment requests when certificate error(s) occur
- insecure payment requests are still possible with this, if they
correctly set paymentRequest.pki_type() == "none"
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p3684_114c38c09a913749c4bdf9d125a944c5024fad58/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
What about making getMerchant() return true if (pki_type == none) and reworking the comment on what getMerchant() returns true for insecure payment requests (that set pki_type = none) and true for secure payment requests only if no cert errors occur?
@diapolo Re: logging see #4448
As for 'secure' and 'insecure' payment requests, getMerchant must return false when no authenticated merchant could be extracted from the request. This can be either due to an error or due to the request not being signed at all. These cases don't need to be distinguished.
Also, the output argument QString merchant must be empty after the call unless there is a successfully authenticated merchant.
Closing this pull, as we don't want to show these problems to the user, but just want to log them (which already happens).
If you want you can move the unrelated message change in src/qt/transactiondesc.cpp to a new pull.