Opened 2 years ago

Closed 18 months ago

Last modified 11 months ago

#24658 closed enhancement (fixed)

Split/refactor crypto.h into smaller separate modules

Reported by: isis Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-crypto, refactor, review-group-32, review-group-34, 033-triage-20180320, 033-included-20180320
Cc: ffernandezmancera@…, chelseakomlo, catalyst Actual Points: 2
Parent ID: Points:
Reviewer: nickm, isis Sponsor: Sponsor8-can

Description

This will make it easier to maintain, as well as easier to create new/alternate implementations of portions of the code (e.g. in Rust). crypto.h is already somewhat neatly partitioned into sections. nickm said that likely appropriate categories for code for the new modules are

something like: rsa, stream cipher, digest+xof, prime-field dh, openssl management, PRNG, and derived functions.

Child Tickets

TicketTypeStatusOwnerSummary
#26036defectclosedcatalystmacOS regression in crypto_rand.c refactor

Change History (61)

comment:1 Changed 2 years ago by nickm

I'd suggest structuring this as a series of commits, each of which splits out exactly one piece of functionality from crypto.[ch].

Also, let's review the first commit or two before we do the whole file -- that way, we can discuss our approach without having to throw away a huge amount of work.

I'd suggest that the new modules get names like crypto_rsa, crypto_dh, crypto_stream, ...

comment:2 Changed 2 years ago by ffmancera

Cc: ffernandezmancera@… added

comment:3 Changed 2 years ago by chelseakomlo

Cc: chelseakomlo added

comment:4 Changed 2 years ago by ffmancera

Status: newneeds_review

I have been working on refactoring OpenSSL management from crypto.[ch] into a smaller module. It is working now and all tests run succesfully. But as nickm said I only uploaded the first commits to get them reviewed.

If everything is fine, then I will upload all related with compilation and dependencies in other files. Check my github branch bug24658-openssl.

PD: Should we create one ticket per module?

comment:5 Changed 2 years ago by nickm

Milestone: Tor: 0.3.3.x-final

I think one ticket should be okay for now, but let's revisit that decision if this ticket gets too complicated.

comment:6 Changed 2 years ago by nickm

Keywords: review-group-29 added

comment:7 Changed 2 years ago by nickm

Status: needs_reviewneeds_revision

Looks like a good start! I have some suggestions.

1) Let's rename the new file to crypto_openssl_mgt or crypto_openssl_setup or something? We might someday want to have a crypto_openssl file that is not openssl management, but that contains openssl backends.

2) free_openssl() is not our usual naming style; something more like crypto_openssl_free_all() would be more like what we usually do.

3) n_openssl_mutexes and openssl_mutexes should not be globals declared in the header. They should remain static variables. (Doing it like that doesn't work the way you expect in C, I think.)

4) The functions that used to be static should not become nonstatic, except maybe for setup_openssl_threading(). The others can all be static in their new module, I think?

comment:8 Changed 23 months ago by ffmancera

Status: needs_revisionneeds_review

Okay, all changes done. Also I committed the changes related with compiling and dependency issues. All tests pass succesfully.

If everything is fine I will write the proper changes file :D

Last edited 23 months ago by ffmancera (previous) (diff)

comment:9 Changed 23 months ago by nickm

Thanks!

Can you move the openssl includes back into the .c files? We're trying not to have all the other modules in Tor include the openssl namespace.


comment:10 Changed 23 months ago by nickm

Status: needs_reviewneeds_revision

comment:11 Changed 23 months ago by ffmancera

Status: needs_revisionneeds_review

Changes done! All tests pass succesfully. Also we would need a squashed version of commits right?

comment:12 Changed 23 months ago by nickm

The history here looks clean to me; I'll merge it as-is.

comment:13 Changed 23 months ago by nickm

Status: needs_reviewnew

merged. Back into "new" for other refactoring.

comment:14 Changed 23 months ago by catalyst

Cc: catalyst added
Status: newneeds_review

clang complained about some missing static keywords: https://travis-ci.org/tlyu/tor/jobs/332496284
Patch in the static-version-str branch in my GitHub repository.

comment:15 Changed 23 months ago by nickm

Status: needs_reviewnew

Merged that too; thanks!

comment:16 Changed 23 months ago by ffmancera

Status: newneeds_review

I have been working on splitting the RSA module from crypto.[ch]. It is working now and all tests pass succesfully, also Travis CI seems happy. I have some notes in order to do the patch easier to review.

  1. crypto.h is included in crypto_rsa.c because we need common functions as memwipe and the digest module. This include should be removed when we split common and digest modules (xof+digest as we decided).
  1. I didn't consider crypto_pk_obsolete_public_hybrid_encrypt() and crypto_pk_obsolete_private_hybrid_decrypt() as part of the RSA module.
  1. crypto_get_rsa_padding_overhead() and crypto_get_rsa_padding() are not static inline anymore, I think we should discuss that.

I hope it isn't really hard to review, check my github branch bug24658-rsa

comment:17 Changed 23 months ago by nickm

Keywords: review-group-31 added

comment:18 Changed 23 months ago by nickm

hm. I like all of this except for the part that makes the contents of crypto_pk_t public. Generally it's better to keep structure contents like this opaque whenever we can.

Two options here would be: moving the crypto_pk_digest functions into crypto_rsa, or making them use crypto_pk_asn1_encode() instead. I tried the latter approach in a squash commit I made on top of bug24658-rsa in my public branch of the same name. Does it look ok to you?

comment:19 in reply to:  18 Changed 23 months ago by ffmancera

Seems fine to me, I agree that the second approach looks better than the first one :-)

Last edited 23 months ago by ffmancera (previous) (diff)

comment:20 Changed 23 months ago by nickm

Keywords: review-group-29 review-group-31 removed
Status: needs_reviewnew

squashed and merged; thanks!

comment:21 Changed 22 months ago by ffmancera

Status: newneeds_review

I have been working on splitting the xof+digest module from crypto.[ch]. It is working now and all tests pass succesfully but Travis CI complains about clang compilation. src/test/test is failing when rust options are enabled, here is the report.

I hope everything is fine, check my github branch bug24658-xof_digest.

comment:22 Changed 22 months ago by catalyst

It looks like the Travis CI failures are due to the same cause as #25127.

comment:23 Changed 22 months ago by dgoulet

Owner: set to ffmancera
Status: needs_reviewassigned

comment:24 Changed 22 months ago by dgoulet

Status: assignedneeds_review

comment:25 Changed 22 months ago by nickm

Keywords: review-group-32 added

comment:26 Changed 22 months ago by isis

Reviewer: isis

Assigning tickets from review-group-32 to myself for review this week.

comment:27 Changed 22 months ago by isis

Status: needs_reviewneeds_revision

Can confirm that the errors in Travis were due to #25127, rebasing onto the latest master fixes the CI builds.

A couple things about the RSA code:

  1. Maybe remove the crypto_curve25519.h included in crypto_rsa.c?
  2. crypto_curve25519.h is included in both crypto_rsa.c and crypto_rsa.h.
  3. Maybe remove the crypto_ed25519.h included in crypto_rsa.c?
  4. Why didn't crypto_pk_obsolete_public_hybrid_encrypt() or crypto_pk_obsolete_private_hybrid_decrypt() move from crypto.[ch] to crypto_rsa.[ch]?

And just one small note on the digest code:

  1. Maybe move the crypto_pk_* functions into crypto_rsa.[ch]? It probably would not occur to me to look for key fingerprint, signing, and verification functions in a file called crypto_digest.c, even though all those tasks do use digests.

comment:28 Changed 22 months ago by ffmancera

Status: needs_revisionneeds_review

Thanks :-)

Maybe remove the crypto_curve25519.h included in crypto_rsa.c?

I removed the crypto_curve25519.h included in crypto_rsa.h in order to not include useless modules everywhere. If you don't agree, let me know and I will remove it from crypto_rsa.c.

Why didn't crypto_pk_obsolete_public_hybrid_encrypt() or crypto_pk_obsolete_private_hybrid_decrypt() move from crypto.[ch] to crypto_rsa.[ch]?

I thought it could fit better into the cipher module but, after think more about it, it should be in rsa module.

Everything done in the bug24658-xof_digest branch! :)

comment:29 Changed 22 months ago by isis

Status: needs_reviewmerge_ready

Looks good! Thanks!

comment:30 Changed 22 months ago by isis

I have a tiny fixup patch in my bug24658-rm-curve25519-header branch (based upon ffmancera's bug24658-xof_digest branch above) which removes an unnecessary #include "crypto_curve25519.h" from crypto_digest.h (which was causing rust-bindgen to generate bindings for so much stuff). It also removes the #include "crypto_rsa.h" from crypto_digest.c, and adds the appropriate includes in a couple places to fix some missing resolutions.

Last edited 22 months ago by isis (previous) (diff)

comment:31 Changed 22 months ago by nickm

Keywords: review-group-34 added

comment:32 Changed 21 months ago by isis

Status: merge_readyneeds_review

Changing back to needs_review. Please review my patches in comment:30; I've already reviewed ffmancera's latest code. Thanks!

comment:33 Changed 21 months ago by nickm

Keywords: 033-triage-20180320 added

Marking all tickets reached by current round of 033 triage.

comment:34 Changed 21 months ago by nickm

Keywords: 033-included-20180320 added

Mark needs_review tickets as included for 033.

We can revisit this if they turn out to need much more review and/or revision than expected.

comment:35 Changed 21 months ago by nickm

Reviewer: isisnickm

comment:36 Changed 21 months ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final
Resolution: implemented
Status: needs_reviewclosed

This looks okay, but IMO it's too late for 0.3.3. Merging it in 0.3.4 instead. Thanks!

Have a look at 0eed0899cdeadd84dc5323f8ca0a3a13cd3779de for the merge conflicts.

comment:37 Changed 20 months ago by isis

Resolution: implemented
Status: closedreopened

Reopening because we didn't do the dh and rng parts yet.

comment:38 Changed 20 months ago by isis

I'll do the digest part next, it's fast and I need it for #24660.

comment:39 Changed 20 months ago by isis

Do we also want to move the tor_weak_rng_t stuff from src/common/util.c to the new src/common/crypto_rand.c module, or keep it where it is?

comment:40 Changed 20 months ago by isis

Status: reopenedneeds_review

Okay, this part is done in my bug24658-rand branch (Travis passes).

comment:41 Changed 20 months ago by nickm

I think I reviewed that as #24660 ?

comment:42 in reply to:  41 Changed 20 months ago by isis

Status: needs_reviewmerge_ready

Replying to nickm:

I think I reviewed that as #24660 ?


Yep! It looks like that was done here: https://github.com/torproject/tor/compare/master...isislovecruft:bug24660#commitcomment-28607519

After this is merged, all that's left to do is stream and DH refactoring, I believe.

comment:43 Changed 20 months ago by nickm

Status: merge_readyneeds_revision

comment:44 Changed 19 months ago by isis

Status: needs_revisionnew

Setting as new; there's still the stream and dh stuff to do, iirc.

comment:45 Changed 19 months ago by ffmancera

Status: newneeds_review

DH module done! Check my github branch bug24658-dh_stream. Here is the travis CI report!

comment:46 Changed 19 months ago by isis

Reviewer: nickmnickm, isis

Setting myself as reviewer.

comment:47 Changed 19 months ago by isis

Status: needs_reviewneeds_revision

I've left my review here. There's just a couple small changes that need to be addressed. For the moved code, I've verified it's identical using a mix between good ol' diff and git show -p --color-moved.

comment:48 Changed 19 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: 0.3.5.x-final

Move most needs_revision tickets from 0.3.4 to 0.3.5: we're about to hit the feature freeze.

If you really need to get this into 0.3.4, please drop me a line. :)

comment:49 Changed 19 months ago by ffmancera

Status: needs_revisionneeds_review

Changes done! I pushed a new commit in the branch =)

comment:50 Changed 19 months ago by isis

Status: needs_reviewmerge_ready

Great, thanks! Looks good to me!

Last up we have stream stuff to do.

comment:51 in reply to:  50 Changed 19 months ago by ffmancera

Replying to isis:

Great, thanks! Looks good to me!

Last up we have stream stuff to do.

Nice! I will work on it when this one get merged =)

comment:52 Changed 18 months ago by nickm

Owner: changed from ffmancera to nickm
Status: merge_readyaccepted

Looks good to me. Merging to master.

PLEASE use child tickets for any more things that need to be split out of or.h.

comment:53 Changed 18 months ago by nickm

Status: acceptednew

Err, out of crypto.[ch]

comment:54 Changed 18 months ago by bc348

compile error with mingw after commit 3640c63

src/common/crypto_dh.c:90:5: error: unknown type name 'BN_ULONG'; did you mean 'PULONG'?
     BN_ULONG residue = BN_mod_word(p, 24);
     ^~~~~~~~
     PULONG

After adding #include <openssl/bn.h>
to crypto-dh.c compiles ok

comment:55 Changed 18 months ago by teor

Status: newneeds_revision

comment:56 Changed 18 months ago by nickm

Does 599b53f0469e47 solve that for you?

comment:57 in reply to:  56 Changed 18 months ago by bc348

Replying to nickm:

Does 599b53f0469e47 solve that for you?

Yes. compiles ok, but after today's bunch of commits some tests do not pass

geoip/load_file: [forking]
  FAIL src/test/test_geoip.c:445: assert(dhex OP_EQ geoip_db_digest(AF_INET)): <9350CC266D3467466AB28C354DFAD7FB7706A5D7> vs <B4682B8832074D2F8386675A47040243775D588E>
  [load_file FAILED]
geoip/load_file6: [forking]
  FAIL src/test/test_geoip.c:520: assert(dhex OP_EQ geoip_db_digest(AF_INET6)): <FAD0815096C1EF4A36491782F7E100BE781DD235> vs <9181B66BD1932D195B50713C6EA4762AF0498BCB>
  [load_file6 FAILED]

comment:58 Changed 18 months ago by nickm

Better now? That other issue should be fixed with #26480.

comment:59 in reply to:  58 Changed 18 months ago by bc348

Replying to nickm:

Better now? That other issue should be fixed with #26480.

Yes. Tests pass after commit 3cc0a145bd2bef

comment:60 Changed 18 months ago by nickm

Resolution: fixed
Status: needs_revisionclosed

Cool!

comment:61 Changed 11 months ago by nickm

Actual Points: 2
Note: See TracTickets for help on using tickets.