Opened 6 months ago

Closed 4 months ago

#20824 closed defect (fixed)

[prop271, controller] DROPGUARDS support for new guard backend

Reported by: nickm Owned by: nickm
Priority: High Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-guard, regression, TorCoreTeam201612, review-group-15
Cc: Actual Points: .1
Parent ID: #20822 Points: .5
Reviewer: Sponsor:

Description

Our new guard backend doesn't support DROPGUARDS. Personally, I think DROPGUARDS is a bad idea, and we ought to provide a method way for users who don't want their guards to follow them around. But we should keep this one working at least until there's a better option.

Child Tickets

Change History (15)

comment:1 Changed 6 months ago by nickm

#20825 [user-named guard selections] is one such attempt at a better option, if we wind up doing it.

comment:2 Changed 6 months ago by dgoulet

  • Keywords tor-guard added

comment:3 Changed 6 months ago by nickm

  • Keywords regression added
  • Priority changed from Medium to High

comment:4 Changed 6 months ago by nickm

  • Owner set to nickm
  • Status changed from new to accepted

comment:5 Changed 6 months ago by nickm

  • Keywords TorCoreTeam201612 added
  • Points set to .5

comment:6 Changed 6 months ago by nickm

  • Actual Points set to .1 so far
  • Status changed from accepted to needs_review

I've done a minimal version of this in a branch called "bug20824". It's a branch on top of prop271_030_v1. It tries to shove the new guards into the GETINFO API that the old guards supported.

I think there should also be a non-legacy controller GETINFO API that exposes this information more sensibly.

Whoops, wrong ticket.

Last edited 6 months ago by nickm (previous) (diff)

comment:7 Changed 6 months ago by nickm

  • Actual Points .1 so far deleted
  • Status changed from needs_review to accepted

comment:8 Changed 6 months ago by nickm

  • Actual Points set to .1
  • Status changed from accepted to needs_review

bug20824 is an implementation of this function for the new API; it's on top of my prop271_030_v1 branch.

comment:9 Changed 5 months ago by chelseakomlo

Overall looks good, and tests pass!

One question: It looks like we have this case in remove_all_entry_guards_for_guard_selection:

#ifdef ENABLE_LEGACY_GUARD_ALGORITHM                                                
  SMARTLIST_FOREACH(gs->chosen_entry_guards, entry_guard_t *, entry, {              
    control_event_guard(entry->nickname, entry->identity, "DROPPED");               
  });                                                                               
#endif                                                                              
  SMARTLIST_FOREACH(gs->sampled_entry_guards, entry_guard_t *, entry, {             
    control_event_guard(entry->nickname, entry->identity, "DROPPED");               
  }); 

Just to confirm: if we are running the legacy algorithm, a guard_selection will use both chosen_entry_guards as well as sampled_entry_guards?

Finally, it may be helpful to add a unit test for this, as we add (some) new functionality to remove_all_entry_guards_for_guard_selection with guard context.

comment:10 Changed 5 months ago by chelseakomlo

  • Status changed from needs_review to needs_revision

comment:11 Changed 5 months ago by nickm

If we have the legacy algorithm enabled, legacy guard_selections will use chosen_entry_guards, and others will use sampled_entry_guards. If we don't, then every guard selection will use sampled_entry_guards.

Finally, it may be helpful to add a unit test for this, as we add (some) new functionality to remove_all_entry_guards_for_guard_selection with guard context.

Hm. ok.

comment:12 Changed 5 months ago by nickm

  • Status changed from needs_revision to needs_review

You were right about this needing unit tests; I found a use-after-free error. :(

Rebased onto master, tests and fixup commits added, in branch bug20824_v2 .

comment:13 Changed 4 months ago by nickm

  • Keywords review-group-15 added

comment:14 Changed 4 months ago by asn

  • Status changed from needs_review to merge_ready

Code and feature seems good to me. I tested it and it works fine.

This branch should probably be rebased on top of #20830 (remove legacy code).

comment:15 Changed 4 months ago by nickm

  • Resolution set to fixed
  • Status changed from merge_ready to closed

rebased on top of master, with some manual cleanup, as bug20824_v4. Merged that. Thanks!

Note: See TracTickets for help on using tickets.