[Qt] add option to allow self signed root certs (for testing) #5636

pull Diapolo wants to merge 1 commits into bitcoin:master from Diapolo:pr_self-signed changing 2 files +16 −1

Diapolo commented at 1:55 PM on January 10, 2015: none
- it is helpful to be able to test and verify payment request processing by allowing self signed root certificates (e.g. generated by Gavins "certificate authority in a box")
- This option is just shown in the UI options, if -help-debug is enabled. @gavinandresen @laanwj This is a preparation to assist adding extended unit-testing for payment requests, as I intend to add new tests for e.g. #5629 and #5620.
See https://www.openssl.org/docs/apps/verify.html (bottom), which is listing error 18 for self signed certificates (I verified that already).

I already tested this pull, which allows me, that the client treats self signed payment requests as authenticated ones!
laanwj added the label GUI on Jan 11, 2015
laanwj commented at 9:04 AM on January 11, 2015: member

See also #4121 for prior discussion.
Diapolo commented at 6:14 PM on January 11, 2015: none

@laanwj Well, just using e.g. -rootcertificates="D:\paymentrequest-master\ca_in_a_box\certs\cacert.pem" doesn't allow using self signed certs at all, so IMHO this is a good thing to add. I'm going to change the hard-coded 18 into the correct OpenSSL error code.

[Qt] add option to allow self signed root certs (for testing)

- it is helpful to be able to test and verify payment request processing
  by allowing self signed root certificates (e.g. generated by Gavins
  "certificate authority in a box")
- This option is just shown in the UI options, if -help-debug is enabled.

851296a72f

in src/qt/paymentrequestplus.cpp:None in 851296a72f
```
 151 | @@ -150,7 +152,13 @@ bool PaymentRequestPlus::getMerchant(X509_STORE* certStore, QString& merchant) c
 152 |          int result = X509_verify_cert(store_ctx);
 153 |          if (result != 1) {
 154 |              int error = X509_STORE_CTX_get_error(store_ctx);
 155 | -            throw SSLVerifyError(X509_verify_cert_error_string(error));
 156 | +            // For testing payment requests, we allow self signed root certs!
 157 | +            // This option is just shown in the UI options, if -help-debug is enabled.
 158 | +            if (!(error == X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT && GetBoolArg("-allowselfsignedrootcertificates", false))) {
```
jonasschnelli commented at 7:20 PM on January 11, 2015:

Hmm... isn't this BoolArg a bit long? I still struggle with rpcsslcertificatechainfile... IMO: at least certificate could be shorten with cert.

This arg will not get listed anywhere in --help or exposed in GUI settings?

jonasschnelli commented at 7:26 PM on January 11, 2015:

More clear: doing ./bitcoin-qt --help -help-debug won't show the arg in your shell? Only in QT?

Diapolo commented at 6:55 AM on January 12, 2015:

The argument should be listed for bitcoin-qt and just there (did you test it is not?). Windows executable doesn't use shell just the window with the options.

I used the word certificates in the new option, because the related option reads -rootcertificates and alowselfsigned is a prefix for it ;)?
jonasschnelli commented at 9:48 AM on January 12, 2015: contributor

Shouldn't we bring up a dialog, present a minimalistic cert representation and ask the user if he likes to accept the self signed cert? But maybe this is overfed.

conceptual ACK. Needs unit test for definitive ACK.
laanwj commented at 11:58 AM on January 12, 2015: member

No, don't add any UI for accepting self-signed (or otherwise invalid) certificate. Too easy to trick people.

I'm fine with this pull, but keep this a hidden developer-only feature.
Diapolo commented at 12:21 PM on January 12, 2015: none

Agree and indeed, it IS intended as dev-only feature :).
jonasschnelli commented at 12:27 PM on January 12, 2015: contributor

Okay. Sounds better to keep it small and sane.
Diapolo commented at 1:43 PM on January 12, 2015: none

@laanwj So this was an ACK?
Diapolo commented at 8:11 AM on January 15, 2015: none

@gavinandresen Mind also giving an ACK here?
laanwj merged this on Jan 16, 2015
laanwj closed this on Jan 16, 2015
laanwj referenced this in commit a353ad4cdb on Jan 16, 2015
Diapolo deleted the branch on Jan 16, 2015
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#5620 [Qt] Payment request expiration bug fix (re-done)#5629 [Qt] prevent amount overflow problem with payment requests