Opened 3 years ago

Closed 8 weeks ago

#21003 closed enhancement (fixed)

extend_info_describe should list IPv6 address (if present)

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: easy, intro, ipv6, logging, fast-fix, no-backport, asn-merge nickm-merge
Cc: gaba Actual Points: 0.3
Parent ID: #31088 Points: 0.5
Reviewer: dgoulet Sponsor: Sponsor27-can

Description (last modified by teor)

When running an IPv6-only client using:

src/or/tor ClientUseIPv4 0 DataDirectory `mktemp -d` UseMicrodescriptors 0

I get log messages like
[info] add_an_entry_guard(): Chose $906933A309F15D7CF379127078A6791DF9B0C763~Unnamed at 91.121.82.25 as new entry guard

But I'm really contacting them on their IPv6 address. So we should list it along with the IPv4 address.

Child Tickets

Change History (24)

comment:1 Changed 3 years ago by teor

Description: modified (diff)

comment:2 Changed 3 years ago by teor

(This only makes sense for entry nodes: (directory) guards, bridges, fallbacks, authorities - all other relays are referred to by their IPv4 addresses.)

Last edited 3 years ago by teor (previous) (diff)

comment:3 Changed 3 years ago by teor

This is a problem with the address chosen by the calling code: an extend_info only has one tor_addr_t in it.

comment:4 Changed 2 years ago by nickm

Keywords: logging added

comment:5 Changed 20 months ago by valentecaio

Is this ticket still valid?

According to teor comments, only entry nodes should be concerned by it.
I've run an IPv6 client (ClientUseIPv4 0 UseMicrodescriptors 0) and I can't find any log of entry points using an IPv4 address.
However, other nodes are referred to by their IPv4 addresses.

An example:

[debug] onion_extend_cpath(): Chose router $A6EDA578BA3CA94AB207841DBD468B0FF991643B~FalkensteinTor01 at 2a01:4f8:1c17:4216::1 for hop #1 (exit is inwaiting)
[debug] onion_extend_cpath(): Chose router $AC153580D1DE33051AFC975C7108BCD31CBBC9A1~spaghetti2 at 176.31.181.71 for hop #2 (exit is inwaiting)
[debug] onion_extend_cpath(): Chose router $18BFF1DEAA073003108CFDA2C0DE970D8F604478~inwaiting at 178.175.138.98 for hop #3 (exit is inwaiting)

comment:6 Changed 20 months ago by teor

Yes, this ticket is about log messages that look like:

[info] add_an_entry_guard(): Chose %s as new entry guard

where %s is the output of extend_info_describe().

But it is good to see that messages like:

[debug] onion_extend_cpath(): Chose router %s for hop #%d (%s)

work correctly.

comment:7 Changed 20 months ago by meryemz

I can't reproduce this output:

[info] add_an_entry_guard(): Chose %s as new entry guard

I think the function: add_an_entry_guard() has been deleted somewhere between the versions 0.2.3 and 0.2.4 .
Is there any other function with the same bug?

comment:8 Changed 20 months ago by valentecaio

I would add that we have four functions for describing routers and nodes:

const char *router_describe(const routerinfo_t *ri);
const char *node_describe(const node_t *node);
const char *routerstatus_describe(const routerstatus_t *ri);
const char *extend_info_describe(const extend_info_t *ei);

But three of them use IPv4 addresses when describing the node.
The only one that can use IPv6 is extend_info_describe().

comment:9 Changed 20 months ago by teor

Ok, then they should all be modified so they can use IPv6.
In particular, each of the first 3 functions takes a struct with an IPv4 and IPv6 address, and so it should report both, if they are present.

comment:10 Changed 20 months ago by valentecaio

Do you think it would be better to be able to show both addresses?

By now, the buffer used has space for this output format:

"$FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF~xxxxxxxxxxxxxxxxxxx at 
[ffff:ffff:ffff:ffff:ffff:ffff:255.255.255.255]"

But I think the functions only return strings in one of the following formats (please confirm this if you can):

"$FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF~xxxxxxxxxxxxxxxxxxx at ffff:ffff:ffff:ffff:ffff:ffff"
or
"$FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF~xxxxxxxxxxxxxxxxxxx at 255.255.255.255"

We could add a separator and show both addresses, something like (increasing the buffer size by 2):

"$FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF~xxxxxxxxxxxxxxxxxxx at 
[ffff:ffff:ffff:ffff:ffff:ffff | 255.255.255.255]"

And we could let the caller decide what address it want as output (only IPv4, only IPv6 or both), letting "only IPv4" as default.

What do you think about this?

Last edited 20 months ago by valentecaio (previous) (diff)

comment:11 in reply to:  10 Changed 20 months ago by teor

Replying to valentecaio:

Do you think it would be better to be able to show both addresses?

By now, the buffer used has space for this output format:

"$FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF~xxxxxxxxxxxxxxxxxxx at 
[ffff:ffff:ffff:ffff:ffff:ffff:255.255.255.255]"

This legacy format prints the last 4 bytes of the IPv6 address like an IPv4 address, it is never used in Tor.

But I think the functions only return strings in one of the following formats (please confirm this if you can):

"$FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF~xxxxxxxxxxxxxxxxxxx at ffff:ffff:ffff:ffff:ffff:ffff"
or
"$FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF~xxxxxxxxxxxxxxxxxxx at 255.255.255.255"

An IPv6 address has 128 bits, yours is 32 bits short.
We are trying to always print IPv6 addresses between brackets.

We could add a separator and show both addresses, something like (increasing the buffer size by 2):

"$FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF~xxxxxxxxxxxxxxxxxxx at 
[ffff:ffff:ffff:ffff:ffff:ffff | 255.255.255.255]"

The IPv6 address should be last, and in brackets.
The IPv4 address should not be in brackets.
I suggest using "or", to match "at".

And we could let the caller decide what address it want as output (only IPv4, only IPv6 or both), letting "only IPv4" as default.

We should always print all the addresses that are present.

What do you think about this?

Let's make it happen

Edit: spelling

Last edited 20 months ago by teor (previous) (diff)

comment:12 Changed 9 months ago by teor

Owner: set to teor
Status: newassigned

My commit c83e48e82 has a draft version of this change. I need to fix the format, and add tests.

comment:13 Changed 8 months ago by teor

Status: assignedneeds_revision

comment:14 Changed 8 months ago by teor

Keywords: fast-fix added

comment:15 Changed 3 months ago by teor

Keywords: teor-backlog-revise added
Owner: teor deleted
Status: needs_revisionassigned

These items are not on our roadmap, so I won't be revising them any time soon.

comment:16 Changed 3 months ago by teor

Status: assignedneeds_revision

comment:17 Changed 2 months ago by teor

Actual Points: 0.3
Milestone: Tor: unspecifiedTor: 0.4.2.x-final
Parent ID: #31088
Status: needs_revisionneeds_review
Type: defectenhancement

We needed this feature to test #31088.

I made a pull request on master for this feature:
https://github.com/torproject/tor/pull/1234

I still need to do some more unit tests, and a changes file.
But I would like CI and an initial review.

I am not sure if we should backport this feature, so older versions get better diagnostics. Probably not.

Last edited 2 months ago by teor (previous) (diff)

comment:18 Changed 2 months ago by teor

Keywords: no-backport added; teor-backlog-revise removed

comment:19 Changed 2 months ago by dgoulet

Reviewer: dgoulet

comment:20 in reply to:  17 Changed 2 months ago by teor

Replying to teor:

I made a pull request on master for this feature:
https://github.com/torproject/tor/pull/1234

I still need to do some more unit tests, and a changes file.
But I would like CI and an initial review.

I did the unit tests and changes file, and did a force-push to the PR branch.

comment:21 Changed 2 months ago by teor

Cc: gaba added

Hi Gaba, this ticket is also useful for debugging #23818 and #23507.
Can we make it Sponsor27-can?

comment:22 Changed 8 weeks ago by dgoulet

Keywords: asn-merge nickm-merge added
Sponsor: Sponsor27-can
Status: needs_reviewmerge_ready

Looks good!

One of my worry was that we were removing one side of the LongName format from the control spec with this:

-   buf[1+HEX_DIGEST_LEN] = is_named ? '=' : '~';
+   buf[1+HEX_DIGEST_LEN] = '~';

where LongName from the control spec is:

LongName   = Fingerprint [ ( "=" / "~" ) Nickname ]

I went over the control subsystem and we appear to be OK there.

comment:23 Changed 8 weeks ago by teor

The Named flag is obsolete, so I think it is ok to remove code that uses is_named.

comment:24 Changed 8 weeks ago by asn

Resolution: fixed
Status: merge_readyclosed

merged!

Note: See TracTickets for help on using tickets.