Opened 5 years ago

Closed 5 years ago

#6124 closed defect (fixed)

Flash proxy facilitator should not give IPv6 addresses to IPv4 proxies

Reported by: dcf Owned by: jct
Priority: Medium Milestone:
Component: Archived/Flashproxy Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The facilitator understands IPv4 and IPv6 client addresses. It should avoid giving an IPv6 client address to an IPv4-only proxy. Perhaps a reasonable heuristic is to only give IPv6 client addresses to proxies that talk to the facilitator over IPv6. It may be okay to assume that IPv6 proxies also support IPv4 (but also may not buy us much as long as IPv4 far outnumbers IPv6).

Child Tickets

Attachments (3)

patch_ticket_6124.patch (5.5 KB) - added by jct 5 years ago.
A patch with the candidate code in order to solve the ticket.
ticket_6124.patch (22.0 KB) - added by jct 5 years ago.
A candidate patch in order to resolve the ticket.
6124-twopools.patch (6.2 KB) - added by dcf 5 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 5 years ago by dcf

For the moment I'm taking the facilitator's IPv6 address out of DNS to make it less likely that clients will try to register IPv6 addresses.

Changed 5 years ago by jct

Attachment: patch_ticket_6124.patch added

A patch with the candidate code in order to solve the ticket.

comment:2 Changed 5 years ago by jct

The attached patch is doing the following:

  • Guessing the IP version of the Proxy
  • Guessing the IP version of the Client:
    • The Client's address in the list of registered clients (RegSet) has a different encoding that the Proxy's address
  • Giving to the Proxy a Client from RegSet with the same IP version:
    • It is easy to change it in order to give IPv4 clients to IPv6 Proxies

The patch contains an automatic test: ticket-6124-test.py

comment:3 in reply to:  2 Changed 5 years ago by dcf

Status: newneeds_revision

Replying to jct:

The attached patch is doing the following:

  • Guessing the IP version of the Proxy
  • Guessing the IP version of the Client:
    • The Client's address in the list of registered clients (RegSet) has a different encoding that the Proxy's address
  • Giving to the Proxy a Client from RegSet with the same IP version:
    • It is easy to change it in order to give IPv4 clients to IPv6 Proxies

The patch contains an automatic test: ticket-6124-test.py

Use two RegSets for IPv4 and IPv6, instead of searching the set for a reg of the appropriate family. fetch should remain a constant-time operation.

Functions like ip_ver that don't depend on a class instance (that don't use the self variable) should be ordinary functions, not part of a class. Put them in fac.py.

It looks like you want to factor out the address family-detection code from fac.parse_addr_spec into a new function (and have fac.parse_addr_spec call that function). That, or create a new function that can be run on the first element of an addr tuple (i.e., a numeric address string, not including brackets for IPv6), and returns the address family.

In general, use socket.AF_INET and socket.AF_INET6, not the numbers 4 and 6.

In the test code, change comments like "# IPv6 client and IPv4 proxy" into docstrings so they appear in the unittest output. Thank you for making new tests!

comment:4 Changed 5 years ago by dcf

Owner: changed from dcf to jct
Status: needs_revisionassigned

comment:5 Changed 5 years ago by jct

Working on that!

Changed 5 years ago by jct

Attachment: ticket_6124.patch added

A candidate patch in order to resolve the ticket.

comment:6 Changed 5 years ago by jct

The new attached patch is doing the suggested previously.

The included suite test (ticket-6124-test.py) is testing the new function ip_ver, the modified parse_addr_spec and the Facilitator behaviour with IPv4/IPv6 proxies and clients.

comment:7 in reply to:  6 ; Changed 5 years ago by jct

Replying to jct:

The new attached patch is doing the suggested previously.

The included suite test (ticket-6124-test.py) is testing the new function ip_ver, the modified parse_addr_spec and the Facilitator behaviour with IPv4/IPv6 proxies and clients.

There is also another change: when the proxy has IPv6 support, the Facilitator tries first to give it a IPv6 client, but if there isn't one, then tries to give it a IPv4 client.

comment:8 in reply to:  7 Changed 5 years ago by dcf

Replying to jct:

Replying to jct:

The new attached patch is doing the suggested previously.

The included suite test (ticket-6124-test.py) is testing the new function ip_ver, the modified parse_addr_spec and the Facilitator behaviour with IPv4/IPv6 proxies and clients.

There is also another change: when the proxy has IPv6 support, the Facilitator tries first to give it a IPv6 client, but if there isn't one, then tries to give it a IPv4 client.

I don't want this. IPv4 proxies get only IPv4 clients. IPv6 proxies get only IPv6 clients.

comment:9 in reply to:  6 ; Changed 5 years ago by dcf

Status: assignedneeds_revision

Replying to jct:

The new attached patch is doing the suggested previously.

The included suite test (ticket-6124-test.py) is testing the new function ip_ver, the modified parse_addr_spec and the Facilitator behaviour with IPv4/IPv6 proxies and clients.

I am not quite happy with this patch. It rewrites parse_addr_spec in a less straightforward way, and changes its behavior in ways that are not obvious.

I added some of the test cases to master; these are the only new tests I want. I don't want an ip_ver that is complicated enough to need lots of tests.

I'll attach a patch that is more in line with how I want this to be implemented. Please base the remaining work off of it. (Apply it with git am.)

Changed 5 years ago by dcf

Attachment: 6124-twopools.patch added

comment:10 in reply to:  9 ; Changed 5 years ago by jct

Replying to dcf:

Replying to jct:

The new attached patch is doing the suggested previously.

The included suite test (ticket-6124-test.py) is testing the new function ip_ver, the modified parse_addr_spec and the Facilitator behaviour with IPv4/IPv6 proxies and clients.

I am not quite happy with this patch. It rewrites parse_addr_spec in a less straightforward way, and changes its behavior in ways that are not obvious.

I added some of the test cases to master; these are the only new tests I want. I don't want an ip_ver that is complicated enough to need lots of tests.

I'll attach a patch that is more in line with how I want this to be implemented. Please base the remaining work off of it. (Apply it with git am.)

Done at: https://github.com/uyjco0/flashproxy/tree/6124

comment:11 in reply to:  10 Changed 5 years ago by dcf

Resolution: fixed
Status: needs_revisionclosed

Replying to jct:

Done at: https://github.com/uyjco0/flashproxy/tree/6124

Thanks, now merged.

I've reenabled the AAAA record for tor-facilitator.bamsoftware.com.

Note: See TracTickets for help on using tickets.