Re: [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:  closed
 Priority:  Medium   |  Milestone:  Tor:
 |  0.3.0.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-hs, prop224, 6.s194, |  implemented
  TorCoreTeam201612, review-group-13 |  Actual Points:
Parent ID:  #17241   | Points:  6
 Reviewer:  asn, dgoulet |Sponsor:
 |  SponsorR-must
-+-
Changes (by nickm):

 * status:  needs_review => closed
 * resolution:   => implemented


Comment:

 merged to master!

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 |  needs_review
 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:
  TorCoreTeam201612, review-group-13 |
Parent ID:  #17241   | Points:  6
 Reviewer:  asn, dgoulet |Sponsor:
 |  SponsorR-must
-+-
Changes (by dgoulet):

 * status:  needs_revision => needs_review


Comment:

 Addressed all comments so back in review mode!

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 |  needs_review
 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:
  TorCoreTeam201612, review-group-13 |
Parent ID:  #17241   | Points:  6
 Reviewer:  asn, dgoulet |Sponsor:
 |  SponsorR-must
-+-

Comment (by nickm):

 comments posted (to both pull requests).

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 |  needs_revision
 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:
  TorCoreTeam201612, review-group-13 |
Parent ID:  #17241   | Points:  6
 Reviewer:  asn, dgoulet |Sponsor:
 |  SponsorR-must
-+-
Changes (by nickm):

 * status:  needs_review => needs_revision


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 |  needs_review
 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:
  TorCoreTeam201612, review-group-13 |
Parent ID:  #17241   | Points:  6
 Reviewer:  asn, dgoulet |Sponsor:
 |  SponsorR-must
-+-
Changes (by dgoulet):

 * status:  needs_revision => needs_review


Comment:

 Taking over this one from asn. Made a pass on nickm's comment in
 https://gitlab.com/asn/tor/merge_requests/7/diffs.

 The _new_ merge request is here containing all the fixups from the review
 in the above request:
 https://gitlab.com/dgoulet/tor/merge_requests/16/commits

 Branch: `ticket19043_030_03`

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 |  needs_revision
 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:
  TorCoreTeam201612, review-group-13 |
Parent ID:  #17241   | Points:  6
 Reviewer:  asn, dgoulet |Sponsor:
 |  SponsorR-must
-+-
Changes (by asn):

 * keywords:  tor-hs, prop224, 6.s194, TorCoreTeam201611, review-group-13 =>
 tor-hs, prop224, 6.s194, TorCoreTeam201612, review-group-13


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 |  needs_revision
 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-13 |
Parent ID:  #17241   | Points:  6
 Reviewer:  asn, dgoulet |Sponsor:
 |  SponsorR-must
-+-

Comment (by nickm):

 I've left a bunch of comments in the gitlab review. Some of the issues
 here are cosmetic, but others are potential stack-smashers!

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 |  needs_revision
 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-13 |
Parent ID:  #17241   | Points:  6
 Reviewer:  asn, dgoulet |Sponsor:
 |  SponsorR-must
-+-
Changes (by nickm):

 * keywords:  tor-hs, prop224, 6.s194, TorCoreTeam201611, review-group-12 =>
 tor-hs, prop224, 6.s194, TorCoreTeam201611, review-group-13
 * status:  merge_ready => needs_revision


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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
-+-
Changes (by asn):

 * status:  needs_revision => merge_ready


Comment:

 OK. After another great round of review by chelsea and david, I revised my
 code and pushed a new branch and merge request. My revisions mainly
 improve the sha3 MAC code and its tests.

 The new branch is `ticket19043_030_03` and the merge request is here:
  https://gitlab.com/asn/tor/merge_requests/7/diffs

 Putting this back to `merge_ready`. Feel free to change ticket status if
 you find more issues.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 |  needs_revision
 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
-+-
Changes (by chelseakomlo):

 * status:  merge_ready => needs_revision


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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
-+-
Changes (by asn):

 * status:  needs_revision => merge_ready


Comment:

 Replying to [comment:36 chelseakomlo]:
 > All of the new modules look great!
 >
 > A couple minor points, feel free to take/leave any:
 >

 Thanks for the review. I force pushed the fixes to the same branch.

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

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

 > - 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?

 > - In commit f4db1ab5fcf3e48c6b4011e9f1cbae6db1aa8c5b it says that
 hs_received_establish_intro is the new entry point, but I think this is
 meant to be hs_intro_received_establish_intro

 Oops. Fixed that commit msg and force-pushed to the same branch name
 (naughty I know).

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

 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()`?

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

 Putting this in `needs_review`, please let me know if it needs any other
 changes!!!

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 |  needs_revision
 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
-+-
Changes (by dgoulet):

 * status:  merge_ready => needs_revision


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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):

 All of the new modules look great!

 A couple minor points, feel free to take/leave any:

 - There are some magic numbers in encode_establish_intro_cell_legacy- some
 of these could be extracted into defines
 - the_hs_circuitmap might be renamed for specificity- the_ is a bit
 unclear.
 - 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.
 - In commit f4db1ab5fcf3e48c6b4011e9f1cbae6db1aa8c5b it says that
 hs_received_establish_intro is the new entry point, but I think this is
 meant to be hs_intro_received_establish_intro
 - 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.
 - 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.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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
-+-
Changes (by nickm):

 * keywords:  tor-hs, prop224, 6.s194, TorCoreTeam201611, review-group-11 =>
 tor-hs, prop224, 6.s194, TorCoreTeam201611, review-group-12


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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-11 |
Parent ID:  #17241   | Points:  6
 Reviewer:  asn, dgoulet |Sponsor:
 |  SponsorR-must
-+-
Changes (by asn):

 * status:  needs_review => merge_ready


Comment:

 OK! I have a branch that is mature enough for Nick to take a look.
 It can be found in my repo at `ticket19043_030_02`.

 It basically allows Tor to parse and handle ESTABLISH_INTRO cells and also
 map them with circuits (using the new HS circuitmap subsystem). It's also
 able to generate ESTABLISH_INTRO cells, but this is just done to generate
 unit test data.

 It's squashed and rebased on git master. When the prop224 HSDir branch
 (#17238) gets merged, this branch will require some minimal edit so that
 we move some stuff to the `hs_common.c` file introduced by #17238 (there
 is an XXX about that). I will do that and rebase it when the time comes.

 I also tested it on chutney today and the current HS functionality seems
 to work just fine. More manual testing is always good.

 WRT unit testing, here is the coverage:
 {{{
 File 'src/or/hs_circuitmap.c'
 Lines executed:99.05% of 105

 File 'src/or/hs_intropoint.c'
 Lines executed:84.52% of 84

 File 'src/or/hs_service.c'
 Lines executed:80.39% of 51
 }}}
 Please note that `hs_service.c` is unused by real code yet (it's just
 there basically to generate unittest vectors).

 Please let me know if you need me to do anything else here.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 |  needs_review
 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-11 |
Parent ID:  #17241   | Points:  6
 Reviewer:  asn, dgoulet |Sponsor:
 |  SponsorR-must
-+-
Changes (by dgoulet):

 * status:  needs_revision => needs_review


Comment:

 Ok, dropping the hs_cell idea, it was way to confusing with the trunnel
 API prefixed `hs_cell_`.

 I've made two extra commits on top. First one is some comments/define
 naming cleanup and second one is some code... let's call that "code
 enhancement/" :).

 Still in `ticket19043_030_01`.

 @asn, if you think this is fine, I think we can move on to `merge_ready`.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 |  needs_revision
 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-11 |
Parent ID:  #17241   | Points:  6
 Reviewer:  asn, dgoulet |Sponsor:
 |  SponsorR-must
-+-
Changes (by dgoulet):

 * keywords:  tor-hs, prop224, 6.s194, TorCoreTeam201610, review-group-11 =>
 tor-hs, prop224, 6.s194, TorCoreTeam201611, review-group-11
 * status:  needs_review => needs_revision
 * points:  3 => 6


Comment:

 I've reviewed and squashed-rebased on master in: `ticket19043_030_01`

 This is merge ready imo but I'll set this one in `needs_revision` for now
 as I'll do an attempt to break it down with the `hs_cell.{c|h}` idea and
 see if we like it as we discussed. If not, we'll move this one in
 `merge_ready` for nickm's review.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 |  needs_review
 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:
  TorCoreTeam201610  |
Parent ID:  #17241   | Points:  3
 Reviewer:  asn, dgoulet |Sponsor:
 |  SponsorR-must
-+-
Changes (by asn):

 * status:  needs_revision => needs_review


Comment:

 Replying to [comment:28 dgoulet]:
 > Ok I've done a round of review on the merge request.

 Thanks for the review.

 I addressed most of your comments and pushed a fixup branch at
 `bug19043_v2_dgoulet_review`. I also replied to the gitlab comments. This
 is just a ticket for you to review the fixes.
 Let me know when you feel OK with them and I can squash all the fixup
 commits back to the original commit structure.

 Also, please let me know if you'd like to get the code mutex for now to do
 miscellaneous improvements (e.g. try out the hs_cells API). Personally, I
 found
 it useful when I took the HSDir code from you at the very end, so maybe
 you also find it useful.

 Putting this in `needs_review` again for now.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 |  needs_revision
 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:
  TorCoreTeam201610  |
Parent ID:  #17241   | Points:  3
 Reviewer:  asn, dgoulet |Sponsor:
 |  SponsorR-must
-+-
Changes (by asn):

 * keywords:  tor-hs, prop224, 6.s194, TorCoreTeam201610, review-group-10 =>
 tor-hs, prop224, 6.s194, TorCoreTeam201610


Comment:

 Taking this out of review-group-10 for now. The branch needs to be revised
 based on david's review.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 |  needs_revision
 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:
  TorCoreTeam201610, review-group-10 |
Parent ID:  #17241   | Points:  3
 Reviewer:  asn, dgoulet |Sponsor:
 |  SponsorR-must
-+-
Changes (by dgoulet):

 * status:  needs_review => needs_revision


Comment:

 Ok I've done a round of review on the merge request.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 |  needs_review
 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:
  TorCoreTeam201610, review-group-10 |
Parent ID:  #17241   | Points:  3
 Reviewer:  asn, dgoulet |Sponsor:
 |  SponsorR-must
-+-

Comment (by asn):

 Note: We should probably use the trunnel feature from #20138 in this
 branch. Will do so along with the first round of reviews.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 |  needs_review
 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:
  TorCoreTeam201610, review-group-10 |
Parent ID:  #17241   | Points:  3
 Reviewer:  asn, dgoulet |Sponsor:
 |  SponsorR-must
-+-
Changes (by asn):

 * keywords:  tor-hs, prop224, 6.s194, TorCoreTeam201609, review-group-10 =>
 tor-hs, prop224, 6.s194, TorCoreTeam201610, review-group-10


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 |  needs_review
 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:
  TorCoreTeam201609, review-group-10 |
Parent ID:  #17241   | Points:  3
 Reviewer:  asn, dgoulet |Sponsor:
 |  SponsorR-must
-+-
Changes (by asn):

 * status:  assigned => needs_review


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 |  assigned
 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:
  TorCoreTeam201609, review-group-10 |
Parent ID:  #17241   | Points:  3
 Reviewer:  asn, dgoulet |Sponsor:
 |  SponsorR-must
-+-
Changes (by asn):

 * status:  needs_review => assigned
 * owner:   => asn


Comment:

 (Claiming ownership)

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 Type:  enhancement  | Status:
 |  needs_review
 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:
  TorCoreTeam201609  |
Parent ID:  #17241   | Points:  3
 Reviewer:  asn, dgoulet |Sponsor:
 |  SponsorR-must
-+-
Changes (by asn):

 * status:  needs_revision => needs_review


Comment:

 Hello there friends,

 I'm happy to push the first revision of code that handles prop224
 ESTABLISH_INTRO cells. I started with MIT Alec's branch as a base, but
 most of
 the code has since been rewritten. All errors mine, etc.

 You can find the code in branch `bug19043_v1` in my repo.
 Here is a gitlab link as well: https://gitlab.com/asn/tor/merge_requests/3

 A few words about the branch:

 First of all, it's based on David's #20004 branch which adds all the
 relevant
 trunnel definitions. It then introduces some functions that will come
 handy
 later when we handle v3 ESTABLISH_INTRO cells.  The biggest addition here
 is
 the HS circuitmap subsystem which maps introduction tokens to circuits.
 The
 branch then proceeds with functions that generate and handle
 ESTABLISH_INTRO
 cells, which is the main contribution.  Finally, it adds tests for
 handling
 both v2 and v3 ESTABLISH_INTRO cells.

 There are some XXXs sprinkled around the code aimed towards early
 reviewers.
 Please browse through them as most of them should be resolved before we
 reach
 the final review stages :)

 WRT unittest LoC coverage:
 {{{
 hs_circuitmap.c : 100.00%
 hs_intropoint.c : 80.95%
 hs_service.c: 80.39%
 }}}
 The numbers here will improve further upon resolving some of the XXXs.

 David, feel free to grab the code mutex here if you feel like it. :)

 Cheers!

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 Type:  enhancement  | Status:
 |  needs_revision
 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:
  TorCoreTeam201609  |
Parent ID:  #17241   | Points:  3
 Reviewer:  asn, dgoulet |Sponsor:
 |  SponsorR-must
-+-
Changes (by dgoulet):

 * keywords:  tor-hs, prop224, 6.s194, 0210-proposed, TorCoreTeam201608 =>
 tor-hs, prop224, 6.s194, TorCoreTeam201609


Comment:

 Moving this to 030 and part of the September core team work.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

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

[Tor Bug Tracker & Wiki]

#19043: prop224: Implementation of ESTABLISH_INTRO cell
-+-
 Reporter:  alec.heif|  Owner:
 Type:  enhancement  | Status:
 |  needs_revision
 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:
  0210-proposed, TorCoreTeam201608   |
Parent ID:  #17241   | Points:  3
 Reviewer:  asn, dgoulet |Sponsor:
 |  SponsorR-must
-+-
Changes (by dgoulet):

 * milestone:  Tor: 0.2.??? => Tor: 0.3.0.x-final


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs