Opened 9 months ago

Closed 9 months ago

Last modified 9 months ago

#25183 closed defect (fixed)

Implement a way to tell if an IP address is a known relay

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-relay, tor-dos, 029-backport, 031-backport, 032-backport
Cc: Actual Points:
Parent ID: #24902 Points:
Reviewer: dgoulet Sponsor:

Description

For the DoS mitigation subsystem (#24902), we need a way to lookup the connection address and *quickly* know if it is a known relay or not. This is also needed for #2667 which would prevent Exit to reenter the network.

nickm has started a branch to have IP addresses with bloom filter: address_set_029.

Next step is to use that and bridge it with the nodelist so we can lookup an IP address and learn if it is known or not (not get the node_t back, that would require more work and probably lose in performance).

Child Tickets

Change History (10)

comment:1 Changed 9 months ago by nickm

I've added some "is this a known relay" code to that branch too. Still needs tests though; dgoulet has offered to write them.

comment:2 Changed 9 months ago by dgoulet

Status: assignedneeds_review

I've written unit tests for both using address_set_t and the integration with the nodelist.

Quick thing though, I've renamed addressset.c to address_set.c because that triple 's' was kind of intense and difficult to brain-parse on what it is exactly. Furthermore, the namespace for this file is address_set_* so that just makes sense I think.

Branch: ticket25183_029_01

comment:3 Changed 9 months ago by dgoulet

Owner: changed from nickm to dgoulet
Status: needs_reviewaccepted

Got a lgtm by nickm on IRC.

So we'll make this part of #24902 so this is now merged in branch ticket24902_029_05.

Going back in accepted and we'll close it once #24902 is closed and backported.

comment:4 Changed 9 months ago by dgoulet

Resolution: fixed
Reviewer: dgoulet
Status: acceptedclosed

Merged into master. Will be backported through #24902.

comment:5 Changed 9 months ago by teor

Resolution: fixed
Status: closedreopened

There's a comment typo in address_set.c:

since we split the siphash outcome two 32-bit values
since we split the siphash outcome into two 32-bit values

comment:6 Changed 9 months ago by teor

Status: reopenedneeds_revision

comment:7 Changed 9 months ago by teor

Also, if we ever store an AF_UNSPEC address, these lines should probably hash the constant:

  case AF_UNSPEC:
    return 0x4e4d5342;

For consistency, we should hash the constant in both the keyed and unkeyed versions of the function.

comment:8 in reply to:  7 Changed 9 months ago by dgoulet

Replying to teor:

Also, if we ever store an AF_UNSPEC address, these lines should probably hash the constant:

  case AF_UNSPEC:
    return 0x4e4d5342;

For consistency, we should hash the constant in both the keyed and unkeyed versions of the function.

Not sure here... First, I don't think it should ever be called with AF_UNSPEC but apart from that, imo we want to save CPU cycles as much as possible since this is called very very often. Hashing a constant value will just make us be less efficient without any gain. I would probably go for a comment explaining why we do return that value or wrap it in some macro with a semantic that describes it as a "hash value".

And I believe the goal behind having a constant value for the AF_UNSPEC here is that if we ever store one or a billion of those, the function is to return *quickly* "yes we do have an unspecified address". I honestly don't see a use case for that apart from very efficiently ignoring any of them if they ever go through that function.

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

comment:9 Changed 9 months ago by dgoulet

Resolution: fixed
Status: needs_revisionclosed

nickm just fixed both above in nickm/ticket24902_029_05.

I've pushed those two commits in my ticket24902_029_05 branch.

comment:10 Changed 9 months ago by nickm

To give my rationale for making the AF_UNSPEC change: I think it's harmless to have AF_UNSPEC be a constant in the current usage, but it's fragile. When we promise a keyed hash, we ought to implement one, or we might find ourselves surprised later on.

Note: See TracTickets for help on using tickets.