Use NSS for DH

Diffie-Hellman will be nice intermediate step on the way to getting NSS support. We'll need a way to convert to OpenSSL DH params for now, though, so we can have our TLS layer still work.

changed milestone to %Tor: 0.3.5.x-final

added 035-roadmap-subticket 035-triaged-in-20180711 component::core tor/tor milestone::Tor: 0.3.5.x-final owner::nickm parent::26631 priority::medium resolution::worksforme reviewer::catalyst severity::normal sponsor::8-can status::closed type::enhancement labels

Trac:
Status: new to accepted
Owner: N/A to nickm

I think my nss_dh branch is ready for review. But it's based on the branch #26815 (moved) , so we should probably review and merge that one first. PR at https://github.com/nmathewson/tor/pull/2 ; I'll open another PR against the tor repo once #26815 (moved) is in.

Trac:
Status: accepted to needs_review

Trac:
Reviewer: N/A to catalyst

Thanks! Still looking over the patches. This branch, being based on the same branch as #26815 (moved) and #26816 (moved), has the same (hopefully minor) issues.

Trac:
Status: needs_review to needs_revision

Trac:
Status: needs_revision to needs_review

For CI purposes I've made a squashed and merged branch as nss_dh_squashed_merged. PR at https://github.com/torproject/tor/pull/258 . It includes this branch, and both of the branches it is based on.

Replying to nickm:

For CI purposes I've made a squashed and merged branch as nss_dh_squashed_merged. PR at https://github.com/torproject/tor/pull/258 . It includes this branch, and both of the branches it is based on. Thanks! Looks good so far. I've looked at all of the commits and nothing sticks out as obviously wrong. I want to try to check the memory management more closely in a few places if I can, though.

It looks like the SSL_SignatureMaxCount() prototype warning is still there. (Probably needs a warning disabled in src/lib/crypt_ops/crypto_nss_mgt.c.) Also the Rust build fails during make check due to a duplication of src/lib/crypt_ops/crypto_openssl_mgt.c in src_lib_libtor_crypt_ops_a_SOURCES.

Thanks for the feedback! I've added three more commits to nss_dh_squashed_merged:

• one for the strict-prototypes warning
• one for the rust linking error
• one for a clang warning

make test-network works for non-rust builds and fails for rust builds with:

0/12 nodes are running
Makefile:16278: recipe for target 'test-network' failed
make: *** [test-network] Error 3

I haven't managed to track down why yet.

Seems somewhat likely to be a real bug:

Aug 08 14:45:44.396 [warn] router_compute_hash_final(): Bug: couldn't compute digest (on Tor 0.3.5.0-alpha-dev 56c3282fae496671)
Aug 08 14:45:44.396 [info] dump_desc(): Unable to parse descriptor of type authority cert, and unable to even hash it!
Aug 08 14:45:44.396 [warn] Unable to parse certificate in /home/tlyu/src/chutney/net/nodes/000a/keys/authority_certificate
Aug 08 14:45:44.396 [err] We're configured as a V3 authority, but we were unable to load our v3 authority keys and certificate! Use tor-gencert to generate them. Dying.
Aug 08 14:45:44.396 [warn] options_act(): Bug: Error initializing keys; exiting (on Tor 0.3.5.0-alpha-dev 56c3282fae496671)
Aug 08 14:45:44.396 [err] set_options(): Bug: Acting on config options left us in a broken state. Dying. (on Tor 0.3.5.0-alpha-dev 56c3282fae496671)
Aug 08 14:45:44.396 [err] Reading config failed--see warnings above.

Trac:
Status: needs_review to needs_revision

So here's my working theory:

• This is because we are forking in the chutney tests, because RunAsDaemon is set.
• It only happens with NSS because NSS detects forks and refuses to work afterward without an explicit reinitialization.
• It only happens with Rust because ... I'm investigating this part.

Hm. For me, test-network fails with rust or without.

Replying to nickm:

Hm. For me, test-network fails with rust or without. I confirm. Sorry, my previous result seems to have been due to inconsistent use of --enable-nss between my rust and non-rust builds.

I've added an extra commit as 4f300d547d65e50ac1fd635f8b22714c1544ba33 in nss_dh_squashed_merged that calls crypto_postfork() after a nontrivial finish_daemon(). With this, test-network passes.

Trac:
Status: needs_revision to needs_review

Closing these NSS tickets in favor of #26818 (moved) (the RSA one), since that one includes all the code from these.

Trac:
Resolution: N/A to worksforme
Status: needs_review to closed

closed