[tor-bugs] #27112 [Core Tor/Stem]: Decouple payload processing from pop/unpack + tune abstraction layers

[Tor Bug Tracker & Wiki]

#27112: Decouple payload processing from pop/unpack + tune abstraction layers
---------------------------+------------------------------
 Reporter:  dmr            |          Owner:  dmr
     Type:  enhancement    |         Status:  needs_review
 Priority:  Medium         |      Milestone:
Component:  Core Tor/Stem  |        Version:
 Severity:  Normal         |     Resolution:
 Keywords:  client         |  Actual Points:
Parent ID:                 |         Points:
 Reviewer:  atagar         |        Sponsor:
---------------------------+------------------------------

Comment (by atagar):

 Good morning Dave. You dropped off irc so responding to ya here.

 {{{
 01:21 < dmr> hey atagar: w.r.t. 486a8d2e5c4724146d6647b7f69ffbd10adbfc1d
              / 8e83553e578aa0f8819eead434ca7ca26d57d5d9 (combined
              encryption and packing), this probably needs to be undone
              for multi-hop circuits
 01:22 < dmr> atagar: and w.r.t. 7d8f1f151d8e2c815e68e0197513f0449a12edd0,
              clients *do* need to check the digest when fully decrypted,
              for 2 reasons: (1) to confirm that we didn't get unlucky
              with 'recognized' == 0; (2) to work against various
              corruption / attacks that would affect the integrity of
              our payload
 01:24 < dmr> atagar: finally, I think the new decrypt() will probably
              need to be tweaked back to not trying to construct a
              RelayCell until it is known that the cell is fully
              decrypted (to support multi-hop circuits)
 01:27 < dmr> atagar: I'll have to look harder at the changes, but I
              fear that (as you noted in
              8e83553e578aa0f8819eead434ca7ca26d57d5d9), some of the
              design was done intentionally in a future-looking sense
              for multi-hop circuits and more centralized ORPort
              reads/sends
 01:27 < dmr> atagar: so now it seems like ground was lost there
 01:28 < dmr> atagar: I do agree, however, that _too much_ was
              factored out (e.g. some of the @staticmethod junk),
              but I was pretty solid with the encrypt() and
              decrypt() interface / methods
 01:30 < dmr> atagar: if you glance at my
              5baa2e37728ab50a5132d81225070e097e6bd058, in the
              commit message you'll see an example use of
              decrypt() that I believe suits multi-hop circuits
              very well, as well as centralized ORPort reads
 01:34 < dmr> uh, so, I think moving forward I'll try to write
              up a hierarchy / method interface layout (as
              discussed last week) and propose that before any
              further code changes
 }}}

 I think we might be misunderstanding each other, and it boils down to one
 simple question: **When you say 'for multi-hop circuits' which of the
 following do you mean?**

 a. So we can **relay**. That is to say, be in the **middle** of the
 circuit.
 b. So we can **make circuits**. That is to say, we're the **originator**
 of a multi-hop circuit.

 If you mean 'a' then yes, much of this is necessary, but that is **not**
 our task at present. If the later then I'm unsure why much of this is
 necessary but quite possible I'm missing something.

 > (1) to confirm that we didn't get unlucky with 'recognized' == 0;

 If we're a client then all cells we receive are decryptable. The
 'recognized' field is nothing more than a poor man's 'is this cell still
 encrypted?' check. This is only relevant if we implement relaying.

 > (2) to work against various corruption / attacks that would affect the
 integrity of our payload

 As for this part, yup. I'd actually be **delighted** to merge a targeted,
 simple patch that implements decryption digests (with tests). I stared at
 your branch for hours this weekend hoping to integrate that, but honestly
 I couldn't make heads or tails of this code. The myriad of helpers
 (especially with names like 'coerce' and 'interpret') made my head spin.

 > but I was pretty solid with the encrypt() and decrypt() interface /
 methods

 I kept the encrypt/decrypt interfaces the same. The **only** thing I
 dropped was...

 a. Implementation of decryption digests. This would be nice to have.

 b. Rearchitecture of our RelayCell class, splitting this up into dozens of
 helpers. I wasn't a fan of this.

 Sorry! I know it sucks to get this kind of pushback on a branch you've
 worked so hard on.

 TL;DR I think is...

 1. I really liked your branche's simplification of Circuit by moving
 encryption/decryption down into the RelayCell class. That part is now
 merged.

 2. I also really liked a myriad of small fixes you made (docs and such).
 Those are also now merged.

 3. I **want** to merge your decryption digest implementation (there's now
 a TODO comment in decrypt() about it) but I couldn't make sense of this
 code.

 4. I **do not** want to split RelayCell or excessively complicate this
 class unless there's a very good reason.

 Hope that helps!

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/27112#comment:7>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online