[tor-bugs] #19043 [Core Tor/Tor]: prop224: Implementation of ESTABLISH_INTRO cell

[Tor Bug Tracker & Wiki]

#19043: prop224: Implementation of ESTABLISH_INTRO cell
-------------------------------------------------+-------------------------
 Reporter:  alec.heif                            |          Owner:  asn
     Type:  enhancement                          |         Status:
                                                 |  merge_ready
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.3.0.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  tor-hs, prop224, 6.s194,             |  Actual Points:
  TorCoreTeam201611, review-group-12             |
Parent ID:  #17241                               |         Points:  6
 Reviewer:  asn, dgoulet                         |        Sponsor:
                                                 |  SponsorR-must
-------------------------------------------------+-------------------------

Comment (by chelseakomlo):

 Replying to [comment:38 asn]:
 > Replying to [comment:36 chelseakomlo]:
 > > All of the new modules look great!
 >
 > > - There are some magic numbers in encode_establish_intro_cell_legacy-
 some of these could be extracted into defines
 >
 > Hmm, that function is (very) old code. I tried to leave it alone as much
 as possible.
 >
 > I'm inclined to suggest that any improvements to that old code should be
 done on a new ticket, so that reviewers of this ticket have to worry as
 little as possible about regressions.

 Sure, let's do that. I like the rule of thumb of refactoring/adding tests
 to legacy code when touching it, but there is of course a limit to how
 much improvement should be made/how testable the old code is.

 >
 > > - the_hs_circuitmap might be renamed for specificity- the_ is a bit
 unclear.
 >
 > Hmm, I could rename that to `global_hs_circuitmap` or something.
 >
 > That said, this silly `the_` prefix is used in multiple places in our
 codebase to name big file-level singletons (e.g. `the_nodelist`,
 `the_evdns_base`, etc.).
 >
 > Please let me know if you still feel like `global_hs_circuitmap` or
 something like that would be better, and I will just rename it.

 If this is a common convention/useful naming to the team, then the_ is
 good with me!

 > > - In the comment for hs_circuits_have_same_token we use introduction
 and rendezvous tokens, but in the function we use first_token and
 second_token. Maybe introduction/rendezvous naming can be consistently
 used.
 >
 > Hmm, that function is abstract on purpose. The HS circuitmap can carry
 both introduction and rendezvous tokens, but that function actually does
 not care about the token type. Do you think the function should be split
 into two functions handling each case separately or something?

 No, that was my mistake- after closer look, this looks good!

 > > - In general, it can be useful to write multiple unit tests per
 function under test if there are branches in the code. For example, it
 looks like we just test one path for generate_establish_intro_cell, but it
 might be worth writing additional unit test to cover error cases.
 >
 > I wrote a unittest for one error case of that function and pushed it to
 the top of the branch. It even found a bug! Please check the top commit
 and if you like it I will squash it along with the rest of the branch so
 that it's clean for Nick's review.

 Yes, looks great.

 > So the bug was there because I was not checking the retval of
 `crypto_hmac_sha3_256()` correctly... Not sure why we have `1` being the
 error retval in `crypto.c` instead of `-1`, perhaps I should address this
 in `crypto_hmac_sha3_256()`?

 Yes, we should probably have consistent retvals for errors. Do you think
 this should be fixed as part of this issue? I see crypto_hmac_sha3_256 is
 added in commit 0cfcc7f3da4a77e1162f1e011ba4954d309070fa

 > > - encode_establish_intro_cell_legacy might be able to be unit tested
 directly. It looks like it is tested indirectly but we could have several
 unit tests to test the different branches of this directly.
 >
 > I think that old function is more unittestable now indeed. However, I'd
 suggest we make a new ticket for unittesting that legacy function.

 Sure, as it looks like the behavior of encode_establish_intro_cell_legacy
 is largely unchanged in this ticket, then opening a new ticket to add
 tests sounds good to me.

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