Can we use less space for RSA onion keys on clients?

Our heap profiles say that we're using a HUGE amount of storage for RSA onion keys. That's pretty sad, considering that we only use them for legacy v2 hidden service stuff.

Maybe we can only parse them on-demand, when we need them?

Maybe (if the openssl format is much more expensive than the raw asn1) we can store them in asn1, and only convert them to EVP_PKEY objects when we need them?

changed milestone to %Tor: 0.3.5.x-final in legacy/trac

added 035-roadmap-master in Legacy / Trac 035-triaged-in-20180711 in Legacy / Trac actualpoints::.5 in Legacy / Trac component::core tor/tor in Legacy / Trac milestone::Tor: 0.3.5.x-final in Legacy / Trac parent::27243 in Legacy / Trac priority::medium in Legacy / Trac resolution::implemented in Legacy / Trac reviewer::nickm in Legacy / Trac severity::normal in Legacy / Trac sponsor::8 in Legacy / Trac status::closed in Legacy / Trac type::defect in Legacy / Trac labels

I can confirm that there's a blow-up here -- I ran an experiment with openssl to see how much space it uses to store an RSA that it has loaded from an ASN1 string.

The asn.1 string was 140 bytes long. OpenSSL allocated a total of 408 bytes in order to hold the RSA object and its lock, not counting the malloc headers. (It grabbed one RSA object, one lock, 2 BIGNUM objects, and 2 bitvectors inside the BIGNUMs.)

Here is an attempt: ticket27246_035_01 PR: https://github.com/torproject/tor/pull/292

Test passes so I'm optimistic.

Trac:
Status: new to needs_review

Trac:
Reviewer: N/A to nickm

In experimental context: we used 5.9MB out of 24.7MB total for storing crypto_pk_t and its contents. If we can cut that down by a factor of (140/408), we'll save 3.9 MB, or 15% of our total memory used -- not counting the malloc overhead!

Nice! I expect that the memory savings will be even better than expected in the commit message, since the "408 vs 140" calculation doesn't include malloc overhead.

I suggested a few small changes on the ticket, and found (what I think is) one real bug.

Question: Have you tested this for memory leaks? I didn't see any, but that would be the kind of bug I'd want to watch out for here.

Trac:
Status: needs_review to needs_revision

Replying to nickm:

Nice! I expect that the memory savings will be even better than expected in the commit message, since the "408 vs 140" calculation doesn't include malloc overhead.

I suggested a few small changes on the ticket, and found (what I think is) one real bug.

Question: Have you tested this for memory leaks? I didn't see any, but that would be the kind of bug I'd want to watch out for here.

I have not run this under valgrind but no leaks occur in the test. There are a LOT of code path in the unit tests that touches the md/ri and I know because I had a leak during development and noticed very quickly.

See fixup commits on the PR.

Trac:
Status: needs_revision to needs_review

Fixups LGTM; will squash and merge after CI is done.

Trac:
Status: needs_review to merge_ready

Squashed, added a changes file and some comments, merged.

Trac:
Status: merge_ready to closed
Resolution: N/A to implemented

Trac:
Actualpoints: N/A to .5

closed

added 4h of time spent

moved from legacy/trac#27246 (moved)

added Bug label and removed 1 deleted label

removed 1 deleted label