NSS TLS #306
Closed
NSS TLS #306
Conversation
When it is set, include the NSS headers and libraries as appropriate. Doesn't actually use them yet, though.
These are now part of crypto_init.c. The openssl-only parts now live in crypto_openssl_mgt.c. I recommend reviewing this patch with -b and --color-moved.
This is largely conjectural, based on online documentation for NSS and NSPR.
We need this in our unit tests, since otherwise NSS will notice we've forked and start cussing us out. I suspect we'll need a different hack for daemonizing, but this should be enough for tinytest to work.
This was a fairly straightforward port, once I realized which layer I should be calling into.
This is comparatively straightforward too, except for a couple of
twists:
* For as long as we're building with two crypto libraries, we
want to seed _both_ their RNGs, and use _both_ their RNGs to
improve the output of crypto_strongest_rand()
* The NSS prng will sometimes refuse to generate huge outputs.
When it does, we stretch the output with SHAKE. We only need
this for the tests.
We only ever need this to get us a DH ephemeral key object, so make a function that does just that.
Notably, there's a test to make sure that it round-trips with OpenSSL, if OpenSSL is enabled.
(We do this unconditionally, since we still need it for tortls.c)
This cleans up a lot of junk from crypto_rsa_openssl, and will save us duplicated code in crypto_rsa_nss (when it exists). (Actually, it already exists, but I am going to use git rebase so that this commit precedes the creation of crypto_rsa_nss.)
Also, add a stubbed-out nss version of the modules. The tests won't pass with NSS yet since the NSS modules don't do anything. This is a good patch to read with --color-moved.
We used to link both libraries at once, but now that I'm working on TLS, there's nothing left to keep OpenSSL around for when NSS is enabled. Note that this patch causes a couple of places that still assumed OpenSSL to be disabled when NSS is enabled - tor-gencert - pbkdf2
This was a gap that we left in the last commit.
This ensures that our test failure messages actually tell us what strings Tor was expecting. I will need this to debug some test failures.
I'll need this for debugging.
7 unit tests are failing at this point, but they're all TLS-related.
This is enough to get a chutney network to bootstrap, though a bunch of work remains.
This function was supposed to implement a half-duplex mode for our TLS connections. However, nothing in Tor actually uses it (besides some unit tests), and the implementation looks really questionable to me. It's probably best to remove it. We can add a tested one later if we need one in the future.
The tls_log_errors() function now behaves differently for NSS than it did for OpenSSL, so we need to tweak it a bit.
Fix some compiler warnings from clang.
ahf
reviewed
Sep 4, 2018
I love these changes. I think the refactoring of all this crypto code looks very nice. I have left some small comments that I'd like to see changed. I'm gonna go over the code base once more now that I have a good feeling of how it work to see if I can spot any deeper issues.
|
|
||
| if test "x$enable_nss" = "xyes"; then | ||
| AC_DEFINE(ENABLE_NSS, 1, | ||
| [Defined if we're building with NSS in addition to OpenSSL.]) |
The wording here is misleading: you can build Tor with NSS only.
Fixed in 2f0eaaf
| if (connection_speaks_cells(conn)) { | ||
| or_connection_t *or_conn = TO_OR_CONN(conn); | ||
| tor_tls_free(or_conn->tls); | ||
| or_conn->tls = NULL; |
tor_tls_free() should set or_conn->tls to NULL.
Right; the compiler should remove this redundancy for us.
|
|
||
| #include "siphash.h" | ||
|
|
||
| /** Boolean: has OpenSSL's crypto been initialized? */ |
We should probably remove the mentioning of OpenSSL here.
done in 3730b9b
| /** Boolean: has OpenSSL's crypto been initialized? */ | ||
| static int crypto_early_initialized_ = 0; | ||
|
|
||
| /** Boolean: has OpenSSL's crypto been initialized? */ |
Same here. Remove OpenSSL from this comment.
done in 3730b9b
| SSL_set_tlsext_host_name(tls->ssl, NULL); | ||
| #endif | ||
| SSL_free(tls->ssl); | ||
| tor_tls_impl_free_(tls->ssl); |
Why not have a FREE_AND_NULL() variant of this?
tls_impl_free_ and tls_x509_impl_free_ are tiny wrappers around the underlying types.
| #define TORTLS_PRIVATE | ||
| #define TOR_X509_PRIVATE | ||
|
|
||
| #ifdef _WIN32 /*wrkard for dtls1.h >= 0.9.8m of "#include <winsock.h>"*/ |
I know this comes from a code movement, but this comment makes no sense to me at all. Maybe we should fix that afterwards.
aa554cf revises this for the openssl case, and removes the comment for the nss case.
|
This has already been merged. Closing. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
This PR tracks review for https://trac.torproject.org/projects/tor/ticket/26819
The text was updated successfully, but these errors were encountered: