Opened 4 years ago

Closed 4 months ago

#17065 closed enhancement (wontfix)

Tor should leave its own fingerprint out of its family line

Reported by: teor Owned by:
Priority: Low Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: 0.2.7
Severity: Minor Keywords: regression, prop298
Cc: Actual Points:
Parent ID: #28266 Points: small
Reviewer: Sponsor:

Description

"Tor should notice when it's about to list its
own fingerprint in its family line [in its descriptor], and just quietly leave it out?" -- Roger

https://lists.torproject.org/pipermail/tor-dev/2015-September/009513.html

Child Tickets

Change History (16)

comment:1 Changed 4 years ago by virgilgriffith

In favor!

comment:2 Changed 3 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.???

Move a few tickets out of 0.2.8. I would take a good patch for most of these if somebody writes one. (If you do, please make the ticket needs_review and move it back into maint-0.2.8 milestone. :) )

comment:3 Changed 3 years ago by teor

Severity: Normal

How to write this patch:

When processing the list of MyFamily fingerprints, tor should exclude any digests where router_digest_is_me(digest) is true. It might be a good idea to log a message at LOG_DEBUG when excluding a fingerprint like this.

comment:4 Changed 3 years ago by teor

The code already:

  • skips MyFamily nicknames that match the router's nickname
  • skips MyFamily nicknames that lookup to a hex digest that matches the router's hex digest

But it needs to be fixed to:

  • skip MyFamily hex digest that match the router's hex digest

comment:5 Changed 3 years ago by teor

Version: Tor: unspecifiedTor: 0.2.7

This issue is a regression:
I can convince a relay running 0.2.8.0-dev to include its own fingerprint in its family line, but I can't convince a relay running 0.2.6.9 to do so.

comment:6 Changed 3 years ago by feverDream

I would be happy to work on this. After asking around on the tor-dev channel, user "qwerty1" suggested enabling debug logs and performing git-bisect to find why its not working on 0.2.6.9.

I will try to do that in the next couple of days. Any pointers?

comment:7 in reply to:  6 Changed 3 years ago by teor

Keywords: regression added

Replying to feverDream:

I would be happy to work on this. After asking around on the tor-dev channel, user "qwerty1" suggested enabling debug logs and performing git-bisect to find why its not working on 0.2.6.9.

I will try to do that in the next couple of days. Any pointers?

This is a regression: the feature worked in 0.2.6.9, it doesn't work on 0.2.8.0-alpha-dev.

It will help to write a unit test for router_build_fresh_descriptor, and then bisect based on that.

We've checked the code, and:

  • if (!strcasecmp(name, options->Nickname)) correctly removes the nickname of this relay from MyFamily
  • } else if (router_digest_is_me(member->identity)) { doesn't remove the fingerprint of this relay from MyFamily (when a relay is launched using chutney, and I looked at its first descriptor)
    • member = node_get_by_nickname(name, 1); could be failing to find our relay, because it's not in the consensus yet. You could try checking router_digest_is_me(nickname) after if (!strcasecmp(name, options->Nickname)), and skipping the fingerprint as well.
      • (We can't fix node_get_by_nickname in this case, because we can't return a node_t for ourselves when we're not in the consensus.)

comment:8 Changed 3 years ago by teor

Now that I think about it, relays may have only ever been able to exclude their fingerprint from MyFamily once they had a consensus with their own descriptor in it.

So the first descriptor from a relay would always contain its fingerprint in MyFamily, then subsequent descriptors wouldn't, unless the relay was offline.

If this is the case, then you won't find anything using git bisect. Try modifying router_build_fresh_descriptor like I suggested in the previous comment, and writing a unit test (you might find it easier to write a unit test if you refactor the code that sets the family line into its own helper function), and see if that works.

comment:9 Changed 3 years ago by nickm

Points: small

comment:10 Changed 2 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:11 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:12 Changed 22 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:13 Changed 21 months ago by nickm

Keywords: tor-client added
Severity: NormalMinor

comment:14 Changed 4 months ago by nickm

Keywords: prop298 added; easy tor-client removed

We should close this as wontfix if we do prop298

comment:15 Changed 4 months ago by nickm

Parent ID: #28266

comment:16 Changed 4 months ago by nickm

Resolution: wontfix
Status: newclosed

Wontfix in favor of prop298.

Note: See TracTickets for help on using tickets.