Split/refactor crypto.h into smaller separate modules

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.

changed milestone to %Tor: 0.3.5.x-final

added 033-included-20180320 033-triage-20180320 actualpoints::2 component::core tor/tor milestone::Tor: 0.3.5.x-final owner::nickm priority::medium refactor resolution::fixed review-group-32 review-group-34 reviewer::isis reviewer::nickm severity::normal sponsor::8-can status::closed tor-crypto type::enhancement labels

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, ...

Trac:
Cc: N/A to ffernandezmancera@gmail.com

Trac:
Cc: ffernandezmancera@gmail.com to ffernandezmancera@gmail.com, chelseakomlo

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?

Trac:
Status: new to needs_review

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

Trac:
Milestone: N/A to Tor: 0.3.3.x-final

Trac:
Keywords: N/A deleted, review-group-29 added

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?

Trac:
Status: needs_review to needs_revision

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

Trac:
Status: needs_revision to needs_review

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.

Trac:
Status: needs_review to needs_revision

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

Trac:
Status: needs_revision to needs_review

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

merged. Back into "new" for other refactoring.

Trac:
Status: needs_review to new

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.

Trac:
Status: new to needs_review
Cc: ffernandezmancera@gmail.com, chelseakomlo to ffernandezmancera@gmail.com, chelseakomlo, catalyst

Merged that too; thanks!

Trac:
Status: needs_review to new

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).

2. I didn't consider crypto_pk_obsolete_public_hybrid_encrypt() and crypto_pk_obsolete_private_hybrid_decrypt() as part of the RSA module.

3. 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

Trac:
Status: new to needs_review

Trac:
Keywords: N/A deleted, review-group-31 added

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?