Opened 9 months ago

Closed 8 months ago

#25241 closed defect (fixed)

effective_family sometimes contains the relay's own fingerprint

Reported by: irl Owned by: iwakeh
Priority: Medium Milestone: Onionoo 1.11.0
Component: Metrics/Onionoo Version:
Severity: Normal Keywords:
Cc: metrics-team Actual Points:
Parent ID: Points:
Reviewer: iwakeh Sponsor:

Description

This was originally reported as a Relay Search bug #25034.

Sometimes, but not always, the effective_family for a relay will include the relay itself. The specification does not explicitly say that a relay's own fingerprint will or won't be included, but from reading it I would assume that it is not included.

Child Tickets

Change History (26)

comment:1 Changed 9 months ago by karsten

Owner: changed from metrics-team to karsten
Status: newaccepted

Hmm, yes, I think it makes more sense to exclude the relay itself from its "effective_family".

It looks like the reason is that some relays include their own fingerprint in the family line of their server descriptor. For example, 03603092cf69a41857aed9131aba531cf4e71f27:

@type server-descriptor 1.0
router sipuliDE01 94.130.181.81 6381 0 0
identity-ed25519
-----BEGIN ED25519 CERT-----
AQQABnCOAQpGlUrHIKxdR7v7BHwzvtyu5vjWJb3q52EWHg6F8npMAQAgBAA1+uBN
NBr4hTuW+Kbo0w4AdV1ZzBWqbP5pzKa1/WtPGBWiu91zkxM0APYsY17lf9/eu0dI
SnmejAv3CB65kb20XbhG8fJzvbbAvKUApYaOVCO6FmJEwRlNBSVA6my6Ugo=
-----END ED25519 CERT-----
master-key-ed25519 NfrgTTQa+IU7lvim6NMOAHVdWcwVqmz+acymtf1rTxg
or-address [2a01:4f8:1c0c:414b::1]:8573
platform Tor 0.2.9.14 on Linux
proto Cons=1-2 Desc=1-2 DirCache=1 HSDir=1 HSIntro=3 HSRend=1-2 Link=1-4 LinkAuth=1 Microdesc=1-2 Relay=1-2
published 2018-02-05 09:04:34
fingerprint 4357 0F86 2FE7 CAF7 B486 6B9B D95D B276 6221 8BBD
uptime 3515
bandwidth 1073741824 2147483647 57535
extra-info-digest C3F79852F13BD8F15EB221F6D0A298D416242EE0 xbEm5P5s0wcubfLRIYV4ms6cFBz0764X24OpNpa0SGI
onion-key
-----BEGIN RSA PUBLIC KEY-----
MIGJAoGBALkbUUVGfsDVS9Vi8BRc0CLeS0PbRKoAh3CICoychg00B6nQmxGTJX0o
okKUGikA+AmTDcii7VQd3M1INVT7XPIjy7PMAiiiBZqogREMMW1r70+AZEK/whJu
2CjesaY70rP0yUZYsT8QNZgOUNR14jQyB5z2ofYSMZWAacQRdYjJAgMBAAE=
-----END RSA PUBLIC KEY-----
signing-key
-----BEGIN RSA PUBLIC KEY-----
MIGJAoGBAOGWq/4J9CsBA06Re32X8U522p97sQeRY1jXNgS+6cQ14rjfGyU7ydq6
jab34lHe2lLdyENhYroOeUSJWFRKX276IrZBJ1CS+UMPVHcIQvdDMSImg4zEESAX
PNHk2tdnESTVlaUIAStVnQU9Nu3wH7VzQDhupEEYpOJY7siVAJObAgMBAAE=
-----END RSA PUBLIC KEY-----
onion-key-crosscert
-----BEGIN CROSSCERT-----
TKINsu+xKXjn+/4IP8pT7VD7BKKBGF7Pywoo3X+YLzcoESIUgZ9oyUdwt1tPQ4Qr
Yar+hxh1dcJZ9ge7uVp+v/m6zXwFfUTka5MUAgaaUPMqryIp+wV+A7W6rv/7cSBB
Six6IsOJPIVkUKuzYDlMspdRjR6/DcBvlhif40v62SQ=
-----END CROSSCERT-----
ntor-onion-key-crosscert 1
-----BEGIN ED25519 CERT-----
AQoABm+aATX64E00GviFO5b4pujTDgB1XVnMFaps/mnMprX9a08YANimuVWhML71
rmBaLJlfawceE7k5P69VKqXZGis3cnSovIsvFK3p4+vELEwdZEsVu66W5Znumyc8
a3Rg5jQrRQk=
-----END ED25519 CERT-----
family $43570F862FE7CAF7B4866B9BD95DB27662218BBD $AEB76F909EE1730926195E92F0D6BBDDD0AC60ED $AFC637373810BE6EC4411F58D575CE5D137326CF $C552FF01540030F675DCF4793F1C32828ACD3FF1
hidden-service-dir
contact O_O <tor at lin dot fi>
ntor-onion-key IWbsv6SwQv06fSdlybpnF3RuXG4q1tA11FbXB+/ELV8=
accept 8.8.8.8:53
accept 8.8.4.4:53
reject *:*
router-sig-ed25519 TbKPy+MSIG9Cv2GyOGtCykcDJRD04bCESaFDHGqoxjLxQrcV12m3HDzKNBqgYWNXuX054kVDqmNQkbkaFfSfDw
router-signature
-----BEGIN SIGNATURE-----
GuoAa7adXOktLBp7V0hvK4r9TT2tfGkLcxWQlfDf2dtv1NNwQ+DvLXAE+tnWaw8B
8foFO7UAMTPNdePk7uMVOkwRlodEoAw1cwuE0U9rnrcM4jUA/xAnMiqs3bVqTh42
L9JD2IfRQGmINm9pKeY/T1rPNF41Vwtdh0Csx0IPbqU=
-----END SIGNATURE-----

Onionoo returns the following details document for this relay:

{
  "version": "5.0",
  "build_revision": "a66bfd1",
  "relays_published": "2018-02-14 10:00:00",
  "relays": [
    {
      "nickname": "sipuliDE01",
      "fingerprint": "43570F862FE7CAF7B4866B9BD95DB27662218BBD",
      "or_addresses": [
        "94.130.181.81:6381",
        "[2a01:4f8:1c0c:414b::1]:8573"
      ],
      "last_seen": "2018-02-12 05:00:00",
      "last_changed_address_or_port": "2018-01-24 22:00:00",
      "first_seen": "2018-01-23 14:00:00",
      "running": false,
      "flags": [
        "Fast",
        "Running",
        "Stable",
        "Valid"
      ],
      "country": "de",
      "country_name": "Germany",
      "latitude": 51.2993,
      "longitude": 9.491,
      "as_number": "AS24940",
      "as_name": "Hetzner Online GmbH",
      "consensus_weight": 66900,
      "host_name": "static.81.181.130.94.clients.your-server.de",
      "last_restarted": "2018-02-11 18:31:40",
      "bandwidth_rate": 1073741824,
      "bandwidth_burst": 2147483647,
      "observed_bandwidth": 0,
      "advertised_bandwidth": 0,
      "exit_policy": [
        "accept 8.8.8.8:53",
        "accept 8.8.4.4:53",
        "reject *:*"
      ],
      "exit_policy_summary": {
        "reject": [
          "1-65535"
        ]
      },
      "contact": "O_O <tor at lin dot fi>",
      "platform": "Tor 0.2.9.14 on Linux",
      "version": "0.2.9.14",
      "effective_family": [
        "43570F862FE7CAF7B4866B9BD95DB27662218BBD",
        "AEB76F909EE1730926195E92F0D6BBDDD0AC60ED",
        "AFC637373810BE6EC4411F58D575CE5D137326CF",
        "C552FF01540030F675DCF4793F1C32828ACD3FF1"
      ],
      "recommended_version": true,
      "hibernating": true,
      "measured": true
    }
  ],
  "bridges_published": "2018-02-14 10:04:06",
  "bridges": []
}

I wrote a quick fix that I'm currently testing. Grabbing this ticket and posting a branch as soon as it's tested.

comment:2 Changed 9 months ago by karsten

Just in case we need more examples, here are all details documents that I found that have their "fingerprint" as one element in their "effective_family" field:

8FF0B5BD7AE8A20DD2033E06053EE7B8BA609FAA
23AA8C61EC325C1F9B2443A64F5721C8D1074E0C
43570F862FE7CAF7B4866B9BD95DB27662218BBD
95C4EF3787069A3A1A3B5FD4057A34CA97C171C3
695DBEE606CA6CD99CD925CAA07DDF5CD8C9190B
CF9831F5CE1068F083D0092F30CD77C4847CFC56
13557448AD1632BF080757C2FF7769BB23DF1DF3
0820262CA0468D56A20C3ABC62F58CC5F73CB53D
1B37A891E0BCB1911F1E442ADE9A3C5CC7542692
940850BDE514895F68B30D05C99637A0D27D07A9

comment:3 Changed 9 months ago by irl

This is fixed for Relay Search (#25034) so that there's no incorrect family counts showing up there (just deployed), but this should still be fixed in Onionoo too. Thanks for the example relays, handy for testing. (:

comment:4 Changed 9 months ago by karsten

Status: acceptedneeds_review

Glad to hear that the fingerprints helped and that Relay Search is already fixed.

Please somebody (iwakeh or irl) review commit 5da2d85 in my task-25241 branch.

comment:5 Changed 9 months ago by karsten

Cc: metrics-team added
Reviewer: iwakeh

iwakeh, would you want to take a look? If not, feel free to give it back.

comment:6 Changed 9 months ago by iwakeh

Status: needs_reviewneeds_information

Wondering, if his shouldn't go in a different direction. I would expect the family to contain the listing relay (the 'self'). Isn't it a use case to compare families of relays? Then I would need to somewhat artificially add the 'self's. And, the field's name also yields that the entire family is listed incl. 'self'. So, Onionoo should rather add 'self' always (, which is also a slightly simpler patch).
I couldn't find any reasons for removing the 'self' in the other tickets referenced here.
Thoughts?

Last edited 9 months ago by iwakeh (previous) (diff)

comment:7 Changed 9 months ago by cypherpunks

Iirc I'm the reason that effective_family has been added to onionoo and I'm post-processing onionoo data to always add self to all effective_family sets to have uniform effective_family sets for relays that are in the same effective_family. (That said I'm happy with either, remove it always or add it always)

comment:8 Changed 9 months ago by iwakeh

Status: needs_informationneeds_review

Please find two commits on top of the above branch.

(Pending question still in comment:6)

comment:9 Changed 9 months ago by karsten

Hmm. Interesting idea. Some thoughts/questions:

  • What would the "effective_family" field contain if a relay is not in an effective family with any other relay?
  • The suggestion is to include the relay itself to "effective_family" only, and not to "alleged_family" or "indirect_family", right?

I'm open to this idea, especially if it makes processing easier for Onionoo clients. It's a bit redundant to include the same fingerprint twice in a document, but we made similar decisions in the past where some redundancy is acceptable if that makes it easier to process results.

I'll look closer at the branch as soon as we have made a decision here.

comment:10 in reply to:  9 Changed 8 months ago by iwakeh

Replying to karsten:

Hmm. Interesting idea. Some thoughts/questions:

  • What would the "effective_family" field contain if a relay is not in an effective family with any other relay?

A family consists of that relay then, or we decide that it needs more than one to be a family, but that seems counter intuitive.

  • The suggestion is to include the relay itself to "effective_family" only, and not to "alleged_family" or "indirect_family", right?

The protocol states clearly that the effective family relays are not contained therein. So, actually, the 'self' needs to be in the effective family to make these correct.

I'm open to this idea, especially if it makes processing easier for Onionoo clients. It's a bit redundant to include the same fingerprint twice in a document, but we made similar decisions in the past where some redundancy is acceptable if that makes it easier to process results.

It follows the protocol more closely, I think.

I'll look closer at the branch as soon as we have made a decision here.

Even the other way wouldn't need big changes to the patch, only replacing 'remove' with 'add' and test adaption.

Last edited 8 months ago by iwakeh (previous) (diff)

comment:11 Changed 8 months ago by irl

If we do include a relay in its own effective family, Relay Search will need to be updated to not perform that step as it currently does.

comment:12 in reply to:  11 Changed 8 months ago by karsten

Replying to irl:

If we do include a relay in its own effective family, Relay Search will need to be updated to not perform that step as it currently does.

Not sure I understand. The spec seems vague right now, so how about Relay Search just either adds or removes a relay's own fingerprint to ensure it's contained or not contained, whichever state it is that Relay Search expects? That is, what prevents us from updating Relay Search now and making the Onionoo change at a later time?

comment:13 Changed 8 months ago by irl

Currently Relay Search removes the relay. This means it has to iterate over 2000 (worst case) lists to remove fingerprints for the search view as effective family count is shown in the results table. When I added the code to filter the fingerprints, I added a comment to say this was temporary and should be removed after this ticket was resolved.

https://gitweb.torproject.org/atlas.git/commit/?id=409e410b5c10666c202cd56b2ff42db0c0a8f766

The important thing to achieve in this ticket is consistency. My comment was not very clear, it's a different change that should be made in Relay Search depending on which way we choose to go making it consistent.

If we always add the relay to its family:

  • Stop filtering the relay's own fingerprint
  • Stop adding 1 to the effective family count

If we always remove the relay from its family:

  • Stop filtering the relay's own fingerprint
  • Continue to add 1 to the effective family count

comment:14 Changed 8 months ago by iwakeh

Open steps:

  • decide for adding 'self' always to the effective family, because clients (like RS) interpret it like that (and had to work around the inconsistent situation so far) or against it because it would add too much to current documents
  • explicitly state whether the relay belongs to its effective family or not in the protocol (if not it should also explicitly be added that the 'self' doesn't show up in the other two family types either)
  • make & merge the appropriate changes (everywhere)

comment:15 in reply to:  14 ; Changed 8 months ago by karsten

Replying to iwakeh:

Open steps:

  • decide for adding 'self' always to the effective family, because clients (like RS) interpret it like that (and had to work around the inconsistent situation so far) or against it because it would add too much to current documents

I'm fine with this. From comments above it sounds like there are no concerns. Let's do it.

  • explicitly state whether the relay belongs to its effective family or not in the protocol (if not it should also explicitly be added that the 'self' doesn't show up in the other two family types either)
  • make & merge the appropriate changes (everywhere)

Do you want to do this? I just reviewed your task-25241 branch until commit df11f4d. Looks good! Do you want to change it to always add a relays own fingerprint to its declared family? And I guess we'll squash those commits in the end, to avoid making a change and undoing it right afterwards.

If you're already juggling too many tickets, I can also take this one. Let me know.

comment:16 Changed 8 months ago by karsten

Status: needs_reviewneeds_revision

comment:17 Changed 8 months ago by karsten

Milestone: Onionoo 1.11.0

Let's try to do this as part of the next milestone.

comment:18 in reply to:  15 Changed 8 months ago by iwakeh

Replying to karsten:

Replying to iwakeh:

Open steps:

  • decide for adding 'self' always to the effective family, because clients (like RS) interpret it like that (and had to work around the inconsistent situation so far) or against it because it would add too much to current documents

I'm fine with this. From comments above it sounds like there are no concerns. Let's do it.

  • explicitly state whether the relay belongs to its effective family or not in the protocol (if not it should also explicitly be added that the 'self' doesn't show up in the other two family types either)
  • make & merge the appropriate changes (everywhere)

Do you want to do this? I just reviewed your task-25241 branch until commit df11f4d. Looks good! Do you want to change it to always add a relays own fingerprint to its declared family? And I guess we'll squash those commits in the end, to avoid making a change and undoing it right afterwards.

If you're already juggling too many tickets, I can also take this one. Let me know.

You might be more efficient on this task because the tests need to be adapted; and you reviewed my suggested branch already, in which only small simple changes will be necessary.

comment:19 Changed 8 months ago by iwakeh

Owner: changed from karsten to iwakeh
Status: needs_revisionaccepted

Unless I here any stop call, I'll start working on this within the next hour.

comment:20 Changed 8 months ago by karsten

Feel free to start working on this!

comment:21 Changed 8 months ago by iwakeh

Status: acceptedneeds_review

Please review another commit that implements the changes discussed above.

comment:22 Changed 8 months ago by karsten

Looks good. Here's a minor tweak in commit 2817936 of my task-25241 branch. If that looks good, I'll squash and merge.

comment:23 Changed 8 months ago by iwakeh

Status: needs_reviewmerge_ready

Yes, the variable names should be adapted, too. Thanks!

comment:24 Changed 8 months ago by karsten

Squashed and merged!

What remains to be done? Update the spec? Anything else?

comment:25 Changed 8 months ago by iwakeh

Only the spec update, I think.

comment:26 Changed 8 months ago by karsten

Resolution: fixed
Status: merge_readyclosed

Moving the specification part to #25476 and closing this ticket.

Note: See TracTickets for help on using tickets.