Opened 7 years ago

Closed 7 years ago

#6876 closed enhancement (fixed)

Add config option OutboundBindAddressIPv6

Reported by: ln5 Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: ipv6 tor-relay
Cc: arma Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We need a way to specify an outbound IPv6 address.

Either we add OutboundBindAddressIPv6 or we start parsing
OutboundBindAddress when we read configuration and store two outbound
addresses, one for each supported address family.

I have implemented the OutboundBindAddressIPv6 alternative (because it
was easy). Let me know if we prefer the other one.

Child Tickets

Change History (16)

comment:1 Changed 7 years ago by nickm

I think the OutboundBindAddressIPv6 idea is fine.

comment:2 Changed 7 years ago by ln5

Branch enh6876 implements this.

It lacks documentation and a changes file, will be squashed later.

comment:3 Changed 7 years ago by ln5

Is it too rude not to explicitly point out what the error is when the
poor user puts an IPv4 address in OutboundBindAddressIPv6 or an IPv6
address in OutboundBindAddress?

comment:4 Changed 7 years ago by nickm

The practical reason we have error messages is to help people get Tor relays up and avoid support requests. If you think that the time that we will save in confused users and busy support people is greater than that time it would take to add such an error message, we should add the error message.

comment:5 Changed 7 years ago by ln5

Status: newneeds_review

Please review branch enh6876_squashed in my public repo.

comment:6 Changed 7 years ago by nickm

Hmm. It feels ouchy to go through this whole business every time we want to bind an outbound socket. Can't we just parse the addresses at configuration time, verify that they match the right address family then, and simplify the code that does the binding?

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

Replying to nickm:

Hmm. It feels ouchy to go through this whole business every time we want to bind an outbound socket. Can't we just parse the addresses at configuration time, verify that they match the right address family then, and simplify the code that does the binding?

That's the second option in my initial comment:

Either we add OutboundBindAddressIPv6 or we start parsing
OutboundBindAddress when we read configuration and store two outbound
addresses, one for each supported address family.

If we do that, wouldn't OutboundBindAddress accepting both IPv4 and
IPv6 addresses be the better solution? See branch enh6876_2 for an
implementation of that.

Should we go even further and store struct sockaddr_in*'s and
socklen_t's in or_options_t? Or maybe struct sockaddr_storage's and
lenghts?

comment:8 in reply to:  7 ; Changed 7 years ago by nickm

Status: needs_reviewneeds_revision

Replying to ln5:

Replying to nickm:

Hmm. It feels ouchy to go through this whole business every time we want to bind an outbound socket. Can't we just parse the addresses at configuration time, verify that they match the right address family then, and simplify the code that does the binding?

That's the second option in my initial comment:

Either we add OutboundBindAddressIPv6 or we start parsing
OutboundBindAddress when we read configuration and store two outbound
addresses, one for each supported address family.

If we do that, wouldn't OutboundBindAddress accepting both IPv4 and
IPv6 addresses be the better solution? See branch enh6876_2 for an
implementation of that.

Looks mostly okay. I think the memset() is wrong: there should be a tor_addr_make_null() function instead. I'll write one of those.

The "if (ext_addr)" test in connection_connect() will always be true: it should probably use "tor_addr_is_null()" instead?

Should we go even further and store struct sockaddr_in*'s and
socklen_t's in or_options_t? Or maybe struct sockaddr_storage's and
lenghts?

We could if we want, but no need to do that here.

comment:9 in reply to:  8 Changed 7 years ago by ln5

Status: needs_revisionneeds_review

Replying to nickm:

Looks mostly okay. I think the memset() is wrong: there should be a tor_addr_make_null() function instead. I'll write one of those.

I think the memset() is OK thanks to

/* tor_addr_is_null() and maybe other functions rely on AF_UNSPEC being 0 to
 * work correctly. Bail out here if we've found a platform where AF_UNSPEC
 * isn't 0. */
#if AF_UNSPEC != 0
#error We rely on AF_UNSPEC being 0. Let us know about your platform, please!
#endif

That said, I actually looked for a tor_addr_make_null() before
settling for memset(). I guess I should've written one.

The "if (ext_addr)" test in connection_connect() will always be true: it should probably use "tor_addr_is_null()" instead?

No, ext_addr will be NULL in some cases.

    const tor_addr_t *ext_addr = NULL;
    if (protocol_family == AF_INET &&
        !tor_addr_is_null(&options->_OutboundBindAddressIPv4))
      ext_addr = &options->_OutboundBindAddressIPv4;
    else if (protocol_family == AF_INET6 &&
             !tor_addr_is_null(&options->_OutboundBindAddressIPv6))
      ext_addr = &options->_OutboundBindAddressIPv6;

I can move the code from the "if (ext_addr)" block into a separate
function and call that from the if clauses above if you think that'd
be better.

comment:10 Changed 7 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Merged; thanks! I think it's ok as it is for now.

comment:11 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:12 Changed 7 years ago by nickm

Component: Tor RelayTor

comment:13 Changed 7 years ago by ln5

Resolution: implemented
Status: closedreopened

The changes file got lost somehow. (Its content was wrong and the
commit implementing this ticket failed to mention this ticket number
correctly so I'm not surprised.)

Please merge branch enh6876_changes_file in my public repo.

comment:14 Changed 7 years ago by nickm

Cc: arma added

Hm. The release with this code in it (0.2.4.4-alpha) already came out. I don't know how arma feels about editting changelogs for existing releases. Either way, this needs special treatment, so I can't just merge it as-is. Roger, please suggest what to do?

comment:15 Changed 7 years ago by arma

We should try to make changelogs accurate when possible. So, editing the ChangeLog directly makes sense here.

comment:16 Changed 7 years ago by nickm

Resolution: fixed
Status: reopenedclosed

Okay; I've rolled this into the changelog for 0.2.4.4-alpha

Note: See TracTickets for help on using tickets.