Re: [tor-bugs] #18616 [Core Tor/Tor]: Relay fails to self-test its DirPort with AccountingMax enabled

[Tor Bug Tracker & Wiki]

#18616: Relay fails to self-test its DirPort with AccountingMax enabled
-+-
 Reporter:  toralf   |  Owner:  andrea
 Type:  defect   | Status:
 Priority:  Medium   |  needs_review
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.8.x-final
 Keywords:  regression, must-fix-before-028-rc,  |Version:  Tor:
  TorCoreTeam201605, TorCoreTeam-|  0.2.8.1-alpha
  postponed-201604, review-group-1   | Resolution:
Parent ID:   |  Actual Points:  20
 Reviewer:  arma |  hours
 | Points:  medium
 |Sponsor:
-+-

Comment (by teor):

 Yes, if a relay operator sets DisableNetwork while the user updates the
 config, we shouldn't be testing ORPort reachability. I think it was still
 possible to do that.

--
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] #18616 [Core Tor/Tor]: Relay fails to self-test its DirPort with AccountingMax enabled

[Tor Bug Tracker & Wiki]

#18616: Relay fails to self-test its DirPort with AccountingMax enabled
-+-
 Reporter:  toralf   |  Owner:  andrea
 Type:  defect   | Status:
 Priority:  Medium   |  needs_review
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.8.x-final
 Keywords:  regression, must-fix-before-028-rc,  |Version:  Tor:
  TorCoreTeam201605, TorCoreTeam-|  0.2.8.1-alpha
  postponed-201604, review-group-1   | Resolution:
Parent ID:   |  Actual Points:  20
 Reviewer:  arma |  hours
 | Points:  medium
 |Sponsor:
-+-

Comment (by teor):

 Replying to [comment:31 arma]:
 > Ok, I looked at {{{bug18616-v3-squashed}}}. Looks good to me.
 >
 > I have a further cleanup patch on my side that I'm still working on. But
 I realized I'm confused about what's actually being fixed here.
 >
 > Here's my in-progress new changes file:
 > {{{
 >   o Major bugfixes (directory mirrors):
 > - Fix broken DirPort self-checks. Decide to advertise begindir
 >   support the same way we decide to advertise DirPorts.
 >   Resolves bug 18616; bugfix on 0.2.8.1-alpha. Patch by teor.
 >
 >   o Minor bugfixes:
 > - Add additional config options that might change the content of
 >   a relay's descriptor. Fixes more of bug 12538; bugfix on
 0.2.8.1-alpha.
 > - Avoid checking ORPort reachability when the network is disabled.
 > }}}
 >
 > For the first entry, what exactly was broken about them? I think that's
 this ticket. What was the actual bug?

 DirPort self-checks were failing with error messages. They also weren't
 being scheduled and checked correctly. We fixed some of it in #18632, this
 is the cleanup / remainder.

 >
 > And then for the last entry, was that something that could happen to
 users in practice?

 Yes, if they did a HUP after changing any of those options, it would be 18
 hours before they were reflected in the descriptor. This would be a
 problem if they added a quota and wanted the DirPort to stop working soon.

--
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] #18616 [Core Tor/Tor]: Relay fails to self-test its DirPort with AccountingMax enabled

[Tor Bug Tracker & Wiki]

#18616: Relay fails to self-test its DirPort with AccountingMax enabled
-+-
 Reporter:  toralf   |  Owner:  andrea
 Type:  defect   | Status:
 Priority:  Medium   |  needs_review
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.8.x-final
 Keywords:  regression, must-fix-before-028-rc,  |Version:  Tor:
  TorCoreTeam201605, TorCoreTeam-|  0.2.8.1-alpha
  postponed-201604, review-group-1   | Resolution:
Parent ID:   |  Actual Points:  20
 Reviewer:  arma |  hours
 | Points:  medium
 |Sponsor:
-+-

Comment (by arma):

 Ok, I looked at {{{bug18616-v3-squashed}}}. Looks good to me.

 I have a further cleanup patch on my side that I'm still working on. But I
 realized I'm confused about what's actually being fixed here.

 Here's my in-progress new changes file:
 {{{
   o Major bugfixes (directory mirrors):
 - Fix broken DirPort self-checks. Decide to advertise begindir
   support the same way we decide to advertise DirPorts.
   Resolves bug 18616; bugfix on 0.2.8.1-alpha. Patch by teor.

   o Minor bugfixes:
 - Add additional config options that might change the content of
   a relay's descriptor. Fixes more of bug 12538; bugfix on
 0.2.8.1-alpha.
 - Avoid checking ORPort reachability when the network is disabled.
 }}}

 For the first entry, what exactly was broken about them? I think that's
 this ticket. What was the actual bug?

 And then for the last entry, was that something that could happen to users
 in practice?

--
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] #18616 [Core Tor/Tor]: Relay fails to self-test its DirPort with AccountingMax enabled

[Tor Bug Tracker & Wiki]

#18616: Relay fails to self-test its DirPort with AccountingMax enabled
-+-
 Reporter:  toralf   |  Owner:  andrea
 Type:  defect   | Status:
 Priority:  Medium   |  needs_review
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.8.x-final
 Keywords:  regression, must-fix-before-028-rc,  |Version:  Tor:
  TorCoreTeam201605, TorCoreTeam-|  0.2.8.1-alpha
  postponed-201604, review-group-1   | Resolution:
Parent ID:   |  Actual Points:  20
 Reviewer:  arma |  hours
 | Points:  medium
 |Sponsor:
-+-
Changes (by dgoulet):

 * reviewer:  dgoulet, arma => arma


--
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] #18616 [Core Tor/Tor]: Relay fails to self-test its DirPort with AccountingMax enabled

[Tor Bug Tracker & Wiki]

#18616: Relay fails to self-test its DirPort with AccountingMax enabled
-+-
 Reporter:  toralf   |  Owner:  andrea
 Type:  defect   | Status:
 Priority:  Medium   |  needs_review
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.8.x-final
 Keywords:  regression, must-fix-before-028-rc,  |Version:  Tor:
  TorCoreTeam201605, TorCoreTeam-|  0.2.8.1-alpha
  postponed-201604   | Resolution:
Parent ID:   |  Actual Points:  20
 Reviewer:  dgoulet, arma|  hours
 | Points:  medium
 |Sponsor:
-+-
Changes (by teor):

 * 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] #18616 [Core Tor/Tor]: Relay fails to self-test its DirPort with AccountingMax enabled

[Tor Bug Tracker & Wiki]

#18616: Relay fails to self-test its DirPort with AccountingMax enabled
-+-
 Reporter:  toralf   |  Owner:  andrea
 Type:  defect   | Status:
 Priority:  Medium   |  assigned
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.8.x-final
 Keywords:  regression, must-fix-before-028-rc,  |Version:  Tor:
  TorCoreTeam201605, TorCoreTeam-|  0.2.8.1-alpha
  postponed-201604   | Resolution:
Parent ID:   |  Actual Points:  20
 Reviewer:  dgoulet, arma|  hours
 | Points:  medium
 |Sponsor:
-+-
Changes (by andrea):

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


--
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] #18616 [Core Tor/Tor]: Relay fails to self-test its DirPort with AccountingMax enabled

[Tor Bug Tracker & Wiki]

#18616: Relay fails to self-test its DirPort with AccountingMax enabled
-+-
 Reporter:  toralf   |  Owner:
 Type:  defect   | Status:
 Priority:  Medium   |  needs_review
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.8.x-final
 Keywords:  regression, must-fix-before-028-rc,  |Version:  Tor:
  TorCoreTeam201604, TorCoreTeam201605   |  0.2.8.1-alpha
Parent ID:   | Resolution:
 Reviewer:  dgoulet, arma|  Actual Points:  20
 |  hours
 | Points:  medium
 |Sponsor:
-+-
Changes (by teor):

 * reviewer:  dgoulet => dgoulet, arma
 * actualpoints:  16 hours => 20 hours


Comment:

 Replying to [comment:22 arma]:
 > 1)
 > {{{
 > \ No newline at end of file
 > }}}
 > for the changes file will produce surprising results when somebody cats
 all the changes files together.

 A1: 2c9b85d fixup! Changes file for #18616
 Also fixes a typo in the changes file name, and rewords one of the changes
 to make it clearer.

 > 2)
 > {{{
 > +static int router_reachability_checks_disabled(void)
 >  {
 >const or_options_t *options = get_options();
 > }}}
 > As a static helper function, it might be a bit cleaner to pass in
 {{{options}}} to it, especially since one of the two callers already has
 an 'options' hanging out ready to be passed in. We're certainly not
 consistent about this, but I think it's the righter thing to do.

 It also makes it slightly easier to unit test the function.

 A2: a12df41 Refactor common code out of reachability checks

 > 3) I like 004e1c2704 but it looks like it does two things -- first is
 the refactor, and second is the change where
 check_whether_orport_reachable() considers net_is_disabled(). "Refactor"
 commits are easiest to review when they don't also change behavior. This
 could become two commits, the first adding the net_is_disabled() to
 check_whether_orport_reachable(), and the second adding the modular
 function along with a note "no actual changes in behavior" to make it
 clear that it's just a refactor.

 A3:
 0a4ac3e Reverts original commit 004e1c2, so I can split it up without a
 force push
 00bb4d7 Avoid checking ORPort reachability when the network is disabled
 a12df41 Refactor common code out of reachability checks

 > 3b) Does the behavior change from 004e1c2704 warrant a changelog entry?
 I think it probably does.

 A3b: 00bb4d7 Avoid checking ORPort reachability when the network is
 disabled

 > 3c) in control.c:
 > {{{
 > } else if (!strcmp(question, "status/reachability-succeeded/or")) {
 >   *answer = tor_strdup(check_whether_orport_reachable() ? "1" :
 "0");
 > } else if (!strcmp(question, "status/reachability-succeeded/dir")) {
 >   *answer = tor_strdup(check_whether_dirport_reachable() ? "1" :
 "0");
 > } else if (!strcmp(question, "status/reachability-succeeded")) {
 >   tor_asprintf(answer, "OR=%d DIR=%d",
 >check_whether_orport_reachable() ? 1 : 0,
 >check_whether_dirport_reachable() ? 1 : 0);
 > }}}
 > are documented as
 > {{{
 > "status/reachability-succeeded/or"
 >   0 or 1, depending on whether we've found our ORPort reachable.
 > "status/reachability-succeeded/dir"
 >   0 or 1, depending on whether we've found our DirPort reachable.
 > "status/reachability-succeeded"
 > }}}
 > I think the "we aren't trying to test reachability on it, so return 1"
 behavior might be surprising here. On the other hand, the reachability did
 *succeed*, in that we're not unhappy with the current state of things.
 Yuck. No need to fix it here but maybe we want to clean this up in the
 future?

 #18851 contains a branch with a control spec patch to clarify this
 behaviour

 > 4) In commit 4dda75fc0 we end up with a new function
 decide_to_advertise_begindir(), that looks like it shares a lot of code
 with the place you cut-and-pasted it from (decide_to_advertise_dirport())?
 Leaving these two functions in place together is begging for bugs in the
 future where one gets out of sync from the other.

 A4: 4e0e555 Refactor DirPort & begindir descriptor checks

 The logic is slightly more complex now, but I think it's better than the
 alternatives:
 * duplicate code,
 * a macro that expands to the function body
 * a third variable in the refactored function indicating either a DirPort
 or begindir check

 > 5)
 > {{{
 > +  /* redundant - if DirPort is unreachable, we don't publish a
 descriptor */
 >if (!check_whether_dirport_reachable())
 > 

Re: [tor-bugs] #18616 [Core Tor/Tor]: Relay fails to self-test its DirPort with AccountingMax enabled

[Tor Bug Tracker & Wiki]

#18616: Relay fails to self-test its DirPort with AccountingMax enabled
-+-
 Reporter:  toralf   |  Owner:
 Type:  defect   | Status:
 Priority:  Medium   |  needs_review
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.8.x-final
 Keywords:  regression, must-fix-before-028-rc,  |Version:  Tor:
  TorCoreTeam201604  |  0.2.8.1-alpha
Parent ID:   | Resolution:
 Reviewer:  dgoulet  |  Actual Points:  16
 |  hours
 | Points:  medium
 |Sponsor:
-+-

Comment (by arma):

 1)
 {{{
 \ No newline at end of file
 }}}
 for the changes file will produce surprising results when somebody cats
 all the changes files together.

 2)
 {{{
 +static int router_reachability_checks_disabled(void)
  {
const or_options_t *options = get_options();
 }}}
 As a static helper function, it might be a bit cleaner to pass in
 {{{options}}} to it, especially since one of the two callers already has
 an 'options' hanging out ready to be passed in. We're certainly not
 consistent about this, but I think it's the righter thing to do.

 3) I like 004e1c2704 but it looks like it does two things -- first is the
 refactor, and second is the change where check_whether_orport_reachable()
 considers net_is_disabled(). "Refactor" commits are easiest to review when
 they don't also change behavior. This could become two commits, the first
 adding the net_is_disabled() to check_whether_orport_reachable(), and the
 second adding the modular function along with a note "no actual changes in
 behavior" to make it clear that it's just a refactor.

 3b) Does the behavior change from 004e1c2704 warrant a changelog entry? I
 think it probably does.

 Also, I looked through the calls to check_whether_orport_reachable() and I
 don't think that behavior change will surprise us. Good.

 3c) in control.c:
 {{{
 } else if (!strcmp(question, "status/reachability-succeeded/or")) {
   *answer = tor_strdup(check_whether_orport_reachable() ? "1" : "0");
 } else if (!strcmp(question, "status/reachability-succeeded/dir")) {
   *answer = tor_strdup(check_whether_dirport_reachable() ? "1" : "0");
 } else if (!strcmp(question, "status/reachability-succeeded")) {
   tor_asprintf(answer, "OR=%d DIR=%d",
check_whether_orport_reachable() ? 1 : 0,
check_whether_dirport_reachable() ? 1 : 0);
 }}}
 are documented as
 {{{
 "status/reachability-succeeded/or"
   0 or 1, depending on whether we've found our ORPort reachable.
 "status/reachability-succeeded/dir"
   0 or 1, depending on whether we've found our DirPort reachable.
 "status/reachability-succeeded"
 }}}
 I think the "we aren't trying to test reachability on it, so return 1"
 behavior might be surprising here. On the other hand, the reachability did
 *succeed*, in that we're not unhappy with the current state of things.
 Yuck. No need to fix it here but maybe we want to clean this up in the
 future?

 4) In commit 4dda75fc0 we end up with a new function
 decide_to_advertise_begindir(), that looks like it shares a lot of code
 with the place you cut-and-pasted it from (decide_to_advertise_dirport())?
 Leaving these two functions in place together is begging for bugs in the
 future where one gets out of sync from the other.

 5)
 {{{
 +  /* redundant - if DirPort is unreachable, we don't publish a descriptor
 */
if (!check_whether_dirport_reachable())
 }}}
 I agree. Let's add a commit to take that check out?

 6) It seems we still have a complicated mess of similar sounding
 functions, directory-permits-this, dirport-set-that, etc. Not something
 that needs to be fixed for this ticket, but certainly something worth
 working on for the future!

--
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] #18616 [Core Tor/Tor]: Relay fails to self-test its DirPort with AccountingMax enabled

[Tor Bug Tracker & Wiki]

#18616: Relay fails to self-test its DirPort with AccountingMax enabled
-+-
 Reporter:  toralf   |  Owner:
 Type:  defect   | Status:
 Priority:  Medium   |  needs_review
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.8.x-final
 Keywords:  regression, must-fix-before-028-rc,  |Version:  Tor:
  TorCoreTeam201604  |  0.2.8.1-alpha
Parent ID:   | Resolution:
 Reviewer:  dgoulet  |  Actual Points:  16
 |  hours
 | Points:  medium
 |Sponsor:
-+-

Comment (by teor):

 Oops, see my branch bug18616-v3 on https://github.com/teor2345/tor.git

--
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] #18616 [Core Tor/Tor]: Relay fails to self-test its DirPort with AccountingMax enabled

[Tor Bug Tracker & Wiki]

#18616: Relay fails to self-test its DirPort with AccountingMax enabled
-+-
 Reporter:  toralf   |  Owner:
 Type:  defect   | Status:
 Priority:  Medium   |  needs_review
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.8.x-final
 Keywords:  regression, must-fix-before-028-rc,  |Version:  Tor:
  TorCoreTeam201604  |  0.2.8.1-alpha
Parent ID:   | Resolution:
 Reviewer:  dgoulet  |  Actual Points:  16
 |  hours
 | Points:  medium
 |Sponsor:
-+-
Changes (by teor):

 * status:  needs_revision => needs_review
 * reviewer:   => dgoulet
 * points:   => medium
 * actualpoints:   => 16 hours


Comment:

 I think part of this bug was due to #18623, and the logging is being
 tracked in #18849.

 This leaves the fixes for #12538 and #18050:

 Replying to [comment:18 teor]:
 > I think I can update the patch so it cleanly answers the following
 questions:
 > * do we have a dirport open?

 get_options()->DirPort_set needs no changes.

 >   * if we have a dirport open, is the DirPort reachable?

 check_whether_dirport_reachable() returns 0 when we need to do a
 reachability check,
 and 1 if we've successfully done a reachability check, or if there's no
 reason to do a reachability check. It needs changes for clarity.

 004e1c27 refactors common parts of the ORPort and DirPort
 check_*_reachable functions into router_reachability_checks_disabled(),
 and updates the function comments. It disables OR reahcability checks when
 net_is_disabled(), for consistency with Dir reachability checks.

 #18851 updates the control-spec to clarify this behaviour.

 > * if the DirPort is reachable, do we want to advertise it?

 decide_to_advertise_dirport() needs no changes.

  * do we have an orport open?

 get_options()->ORPort_set needs no changes.

 > * would we answer begindir requests?

 directory_permits_begindir_requests() needs no changes, but should be used
 to set supports_tunnelled_dir_requests. (See 4dda75fc below.)

 This resolves a nasty edge case with bridges, which should always support
 begindir.

 >   * ~~if we would answer begindir requests,~~ is the ORPort reachable?

 check_whether_orport_reachable() is refactored in 004e1c27, see notes
 above

 > * if the ORPort is reachable, do we want to advertise begindir
 support?

 4dda75fc creates decide_to_advertise_begindir(), like
 decide_to_advertise_dirport(), which should clean up a lot of nasty edge
 cases:
 * directory authorities should always support and advertise begindir,
 * ORs with disabled networks shouldn't advertise support for begindir
 (possibly not an issue, but done for consistency with DirPort),
 * ORPort reachability is required for advertisement (possibly not an
 issue, but done for consistency with DirPort),
 * we now call router_should_be_directory_server() once per descriptor
 upload, which I hope satisfies nickm's request to call it infrequently
 (NM1).

 > As nickm notes in comment 11, I need to make sure we do the reachability
 and advertisability checks, store the results, and then use the results to
 answer these questions.

 NM1: This now happens once per descriptor upload, which means tor only
 logs when actually making the decision for each descriptor.

 > As dgoulet notes in 13 & 15, I should also remove redundant function
 calls.

 DG1: I don't believe there are any redundant function calls in this patch.

 Also, a28d98f4 adds the options used by these functions to the list of
 options we check to see if we need to rebuild our descriptor.

 Finally, we need to update our descriptor when we change our mind about
 advertising the dirport or begindir support due to accounting max. We
 didn't do this in the past for DirPort, and we do it every 18 hours
 anyway, so I'm splitting it off into #18852 in case we want to defer it to
 0.2.9 or later.

--
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] #18616 [Core Tor/Tor]: Relay fails to self-test its DirPort with AccountingMax enabled (was: Bug: Node search initiated by. Stack trace:)

[Tor Bug Tracker & Wiki]

#18616: Relay fails to self-test its DirPort with AccountingMax enabled
-+-
 Reporter:  toralf   |  Owner:
 Type:  defect   | Status:
 Priority:  Medium   |  needs_revision
Component:  Core Tor/Tor |  Milestone:  Tor:
 Severity:  Normal   |  0.2.8.x-final
 Keywords:  regression, must-fix-before-028-rc,  |Version:  Tor:
  TorCoreTeam201604  |  0.2.8.1-alpha
Parent ID:   | Resolution:
 Reviewer:   |  Actual Points:
 | Points:
 |Sponsor:
-+-

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