SSL fixes for validator list (RIPD-1558) #2275

bachase · 2017-11-20T16:41:42Z

Configures domain name for SNI
On windows, uses wincrypt API to use system's root certificates.

Support Server Name Indication Ensure windows uses available certificates

bachase · 2017-11-20T16:43:54Z

The first commit is the only one required for published validator lists. I believe the second commit fixes remaining spots in rippled that with a similar issue, but I'm not sure that the current use of those classes requires the changes.

codecov-io · 2017-11-20T17:33:36Z

Codecov Report

Merging #2275 into develop will decrease coverage by 0.01%.
The diff coverage is 33.33%.

@@             Coverage Diff             @@
##           develop    #2275      +/-   ##
===========================================
- Coverage    70.11%   70.09%   -0.02%     
===========================================
  Files          689      690       +1     
  Lines        50800    50808       +8     
===========================================
- Hits         35617    35613       -4     
- Misses       15183    15195      +12

Impacted Files	Coverage Δ
src/ripple/app/misc/detail/WorkSSL.h	`0% <0%> (ø)`	⬆️
src/ripple/net/AutoSocket.h	`0% <0%> (ø)`	⬆️
src/ripple/net/impl/RegisterSSLCerts.cpp	`100% <100%> (ø)`
src/ripple/net/impl/HTTPClient.cpp	`5.96% <33.33%> (-0.06%)`	⬇️
src/ripple/basics/DecayingSample.h	`77.77% <0%> (-8.34%)`	⬇️
src/ripple/app/misc/SHAMapStoreImp.cpp	`78.92% <0%> (-1.03%)`	⬇️
src/ripple/net/impl/RPCSub.cpp	`37.33% <0%> (-0.83%)`	⬇️
src/ripple/app/main/Application.cpp	`60.42% <0%> (-0.12%)`	⬇️
src/ripple/protocol/impl/Serializer.cpp	`70.3% <0%> (+0.34%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cafe18c...af0d609. Read the comment docs.

wilsonianb

From what I can tell, HTTPClient is only used by RPCSub (Subscription object for JSON-RPC), which seem to be deprecated
https://ripple.com/build/rippled-apis/#subscriptions

JSON-RPC support for subscription callbacks is deprecated and may not work as expected.

wilsonianb · 2017-11-21T18:51:30Z

src/ripple/net/impl/RegisterSSLCerts.cpp

+            pContext->cbCertEncoded);
+        if (x509 != NULL)
+        {
+            X509_STORE_add_cert(store, x509);


I'm not sure how this would fail, but should we log if it does?

Probably worth doing. I'll look into making an error_code.

wilsonianb · 2017-11-22T00:31:34Z

src/ripple/net/impl/RegisterSSLCerts.cpp

+        }
+    }
+
+    CertFreeCertificateContext(pContext);


Isn't pContext guaranteed to be NULL here?

Yes, will fix.

wilsonianb · 2017-11-22T00:37:48Z

src/ripple/net/impl/RegisterSSLCerts.cpp

+    CertFreeCertificateContext(pContext);
+    CertCloseStore(hStore, 0);
+
+    SSL_CTX_set_cert_store(ctx.native_handle(), store);


Just to confirm, the reason we don't use SSL_CTX_set1_cert_store here is because
https://www.openssl.org/docs/manmaster/man3/SSL_CTX_set_cert_store.html

SSL_CTX_set_cert_store() does not increment the store's reference count, so it should not be used to assign an X509_STORE that is owned by another SSL_CTX.

Correct. I believe since we create the store above, we do not need to increment the reference count.

Also, it looks like the reference counted version is not available in the 1.0.* versions we are using.

wilsonianb · 2017-11-22T00:43:53Z

src/ripple/net/impl/RegisterSSLCerts.cpp

+//==============================================================================
+#include <BeastConfig.h>
+#include <ripple/net/RegisterSSLCerts.h>
+#if BEAST_WINDOWS


It looks like BEAST_WINDOWS is only used in src/ripple/beast/
Should a different macro be used here?

I can use boost predef instead.

bachase · 2017-11-22T02:01:19Z

src/ripple/net/impl/RegisterSSLCerts.cpp

+    {
+        X509* x509 = d2i_X509(
+            NULL,
+            (const unsigned char**)&pContext->pbCertEncoded,


d2i_X509 actually modifies the pointer taken as the second argument; I will fix to use a local instead.

bachase · 2017-11-22T14:40:30Z

@HowardHinnant can you do a pass on the use of unique_ptr to manage the structures allocated from C apis? This is all in https://github.com/ripple/rippled/pull/2275/files#diff-f52b8187d06f944bb1c240a3c83c5d6a

HowardHinnant

This looks good to me! The ownership properties of these functions looks complex and poorly documented. Nice use of comments to clarify what owns what and when.

wilsonianb

I didn't test on Windows but LGTM 👍
I think you could take or leave the HTTPClient changes if it really is deprecated.

wilsonianb · 2017-11-22T19:20:49Z

src/ripple/net/impl/RegisterSSLCerts.cpp

+            ec = boost::system::error_code(
+                static_cast<int>(::ERR_get_error()),
+                boost::asio::error::get_ssl_category());
+            return;


This is ok as is (returning with ec and throwing if anything goes wrong), but I'm wondering if we could alternatively be ok calling continue here (and on line 85) and just having the caller log ec.

miguelportilla · 2017-11-29T15:49:52Z

src/ripple/net/RegisterSSLCerts.h

+//------------------------------------------------------------------------------
+/*
+    This file is part of rippled: https://github.com/ripple/rippled
+    Copyright (c) 2016 Ripple Labs Inc.


miguelportilla · 2017-11-29T15:50:49Z

SConstruct

@@ -754,6 +754,7 @@ def config_env(toolchain, variant, env):
            'uuid.lib',
            'odbc32.lib',
            'odbccp32.lib',
+	    'crypt32.lib'


nit: misaligned

miguelportilla

👍

bachase · 2017-11-30T21:55:51Z

Merged as a4a43a4

bachase added 2 commits November 20, 2017 11:35

Improve WorkSSL:

b35a160

Support Server Name Indication Ensure windows uses available certificates

[FOLD] Improve HTTPClient SSL

696c81d

bachase assigned miguelportilla, nbougalis and wilsonianb Nov 20, 2017

bachase requested review from miguelportilla, nbougalis and wilsonianb November 20, 2017 16:41

wilsonianb reviewed Nov 22, 2017

View reviewed changes

bachase commented Nov 22, 2017

View reviewed changes

[FOLD] Improve error handling

af0d609

bachase requested a review from HowardHinnant November 22, 2017 14:39

bachase assigned HowardHinnant Nov 22, 2017

HowardHinnant approved these changes Nov 22, 2017

View reviewed changes

wilsonianb approved these changes Nov 22, 2017

View reviewed changes

nbougalis approved these changes Nov 29, 2017

View reviewed changes

bachase added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Nov 29, 2017

miguelportilla reviewed Nov 29, 2017

View reviewed changes

miguelportilla approved these changes Nov 29, 2017

View reviewed changes

bachase closed this Nov 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SSL fixes for validator list (RIPD-1558) #2275

SSL fixes for validator list (RIPD-1558) #2275

bachase commented Nov 20, 2017

bachase commented Nov 20, 2017

codecov-io commented Nov 20, 2017 •

edited

Loading

wilsonianb left a comment

wilsonianb Nov 21, 2017

bachase Nov 22, 2017

wilsonianb Nov 22, 2017

bachase Nov 22, 2017

wilsonianb Nov 22, 2017

bachase Nov 22, 2017

bachase Nov 22, 2017

wilsonianb Nov 22, 2017

bachase Nov 22, 2017

bachase Nov 22, 2017

bachase commented Nov 22, 2017 •

edited

Loading

HowardHinnant left a comment

wilsonianb left a comment

wilsonianb Nov 22, 2017

miguelportilla Nov 29, 2017

miguelportilla Nov 29, 2017

miguelportilla left a comment

bachase commented Nov 30, 2017

SSL fixes for validator list (RIPD-1558) #2275

SSL fixes for validator list (RIPD-1558) #2275

Conversation

bachase commented Nov 20, 2017

bachase commented Nov 20, 2017

codecov-io commented Nov 20, 2017 • edited Loading

Codecov Report

wilsonianb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bachase commented Nov 22, 2017 • edited Loading

HowardHinnant left a comment

Choose a reason for hiding this comment

wilsonianb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelportilla left a comment

Choose a reason for hiding this comment

bachase commented Nov 30, 2017

codecov-io commented Nov 20, 2017 •

edited

Loading

bachase commented Nov 22, 2017 •

edited

Loading