#26817 closed enhancement (worksforme)

Use NSS for DH

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 035-roadmap-subticket, 035-triaged-in-20180711
Cc: nickm, gk, ahf, catalyst Actual Points:
Parent ID: #26631 Points:
Reviewer: catalyst Sponsor: Sponsor8-can

Description

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.

Child Tickets

Change History (17)

comment:1 Changed 13 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:2 Changed 13 months ago by nickm

I think my nss_dh branch is ready for review. But it's based on the branch #26815 , 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 is in.

comment:3 Changed 13 months ago by nickm

Status: acceptedneeds_review

comment:4 Changed 13 months ago by asn

Reviewer: catalyst

comment:5 Changed 13 months ago by catalyst

Status: needs_reviewneeds_revision

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

comment:6 Changed 13 months ago by nickm

Status: needs_revisionneeds_review

comment:7 Changed 13 months ago by 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.

comment:8 in reply to:  7 Changed 13 months ago by catalyst

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.

comment:9 Changed 13 months ago by nickm

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

comment:10 Changed 13 months ago by catalyst

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.

comment:11 Changed 13 months ago by catalyst

Status: needs_reviewneeds_revision

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.

comment:12 Changed 13 months ago by nickm

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.

comment:13 Changed 13 months ago by nickm

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

comment:14 in reply to:  13 Changed 13 months ago by catalyst

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.

comment:15 Changed 13 months ago by nickm

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.

comment:16 Changed 13 months ago by nickm

Status: needs_revisionneeds_review

comment:17 Changed 12 months ago by nickm

Resolution: worksforme
Status: needs_reviewclosed

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

Note: See TracTickets for help on using tickets.