Write fascist_firewall_choose_address_ls() and use it in hs_get_extend_info_from_lspecs()
Currently, the address choice logic is:
- if we have an IPv6 address and can reach the ls IPv6 address, and prefer IPv6, use it
- if we have an IPv4 address and can reach the ls IPv4 address, use it
But it needs to be:
- if we have both addresses and can reach both, then use whatever we prefer
- if we have one address and can reach it, use it
This doesn't matter until clients put IPv6 addresses in the link specifier.
Activity
changed milestone to %Tor: 0.4.1.x-final
added 034-removed-20180328 034-triage-20180328 040-unreached-20190109 041-proposed actualpoints::2 asn-merge component::core tor/tor ipv6 milestone::Tor: 0.4.1.x-final nickm-merge owner::teor parent::23493 points::1 priority::low prop224 resolution::fixed reviewer::dgoulet severity::normal single-onion sponsor::27-must status::closed tor-hs type::enhancement v3-onion-service-feature-parity-can version::tor 0.3.2.1-alpha labels
In my commit 20b0e9e07d in my branch bug23820_032 (#23820 (moved)), I ripped out IPv6 support in hs_get_extend_info_from_lspecs(). We'll need to revert that commit, and then fix the issues in the function.
Remove buggy IPv6 support from hs_get_extend_info_from_lspecs() The previous version of this function has the following issues: * it doesn't choose between IPv4 and IPv6 addresses correctly, and * it doesn't fall back to a 3-hop path when the address for a direct connection is unreachable. But we can't fix these things in a bugfix release. Instead, treat IPv6 addresses like any other unrecognised link specifier and ignore them. If there is no IPv4 address, return NULL.Trac:
Status: new to assigned
Cc: N/A to neel@neelc.org
Owner: N/A to neelIn
ed25519_cert.h,link_specifier_t/link_specifier_stlooks like this:struct link_specifier_st { uint8_t ls_type; uint8_t ls_len; uint32_t un_ipv4_addr; uint16_t un_ipv4_port; uint8_t un_ipv6_addr[16]; uint16_t un_ipv6_port; uint8_t un_legacy_id[20]; uint8_t un_ed25519_id[32]; TRUNNEL_DYNARRAY_HEAD(, uint8_t) un_unrecognized; uint8_t trunnel_error_code_; };But the
fascist_firewall_choose_address_*family of functions defines DirPorts in the function, like this:static void fascist_firewall_choose_address_base(const tor_addr_t *ipv4_addr, uint16_t ipv4_orport, uint16_t ipv4_dirport, const tor_addr_t *ipv6_addr, uint16_t ipv6_orport, uint16_t ipv6_dirport, firewall_connection_t fw_connection, int pref_only, int pref_ipv6, tor_addr_port_t* ap)Should I then assume the
ipv4_dirportandipv6_dirportis0?Replying to neel:
In
ed25519_cert.h,link_specifier_t/link_specifier_stlooks like this:{{{ struct link_specifier_st { uint8_t ls_type; uint8_t ls_len; uint32_t un_ipv4_addr; uint16_t un_ipv4_port; uint8_t un_ipv6_addr[16]; uint16_t un_ipv6_port; uint8_t un_legacy_id[20]; uint8_t un_ed25519_id[32]; TRUNNEL_DYNARRAY_HEAD(, uint8_t) un_unrecognized; uint8_t trunnel_error_code_; }; }}}
But the
fascist_firewall_choose_address_*family of functions defines DirPorts in the function, like this:{{{ static void fascist_firewall_choose_address_base(const tor_addr_t *ipv4_addr, uint16_t ipv4_orport, uint16_t ipv4_dirport, const tor_addr_t ipv6_addr, uint16_t ipv6_orport, uint16_t ipv6_dirport, firewall_connection_t fw_connection, int pref_only, int pref_ipv6, tor_addr_port_t ap) }}}
Should I then assume the
ipv4_dirportandipv6_dirportis0?Yes, link specifiers are only defined for ORPorts. Also, #24732 (moved) will remove the IPv6 DirPort code, because it's unused.
My PR is here: https://github.com/torproject/tor/pull/252
Replying to neel:
My PR is here: https://github.com/torproject/tor/pull/252
Thanks for this patch!
The unit tests fail in the new function: https://travis-ci.org/torproject/tor/jobs/409667005#L3572
Please add new commits to the branch to make them pass.
You can run the unit tests before you push your branch using:
make checkYou might be able to see the CI status on: https://github.com/torproject/tor/pull/252 https://travis-ci.org/torproject/tor https://ci.appveyor.com/project/torproject/tor
If you can't, let us know, and we'll work out how to get you access.
Trac:
Milestone: Tor: unspecified to Tor: 0.3.5.x-final
Status: assigned to needs_revisionThe Appveyor Windows CI gives a good error:
bash.exe : In file included from ../src/core/or/or.h:51:0, At line:2 char:5 + & $commandPath $args 2>&1 + ~~~~~~~~~~~~~~~~~~~~~~~~~ + CategoryInfo : NotSpecified: (In file include...e/or/or.h:51:0,:String) [], RemoteException + FullyQualifiedErrorId : NativeCommandError from ../src/feature/hs/hs_common.c:14: ../src/feature/hs/hs_common.c: In function 'hs_get_extend_info_from_lspecs': ../src/lib/net/address.h:308:49: error: null pointer dereference [-Werror=null-dereference] tor_addr_port_is_valid(&(ap)->addr, (ap)->port, (for_listening)) ^ ../src/lib/net/address.h:305:29: note: in definition of macro 'tor_addr_port_is_valid' tor_port_is_valid((port), (for_listening))) ^~~~ ../src/feature/hs/hs_common.c:1720:8: note: in expansion of macro 'tor_addr_port_is_valid_ap' if (!tor_addr_port_is_valid_ap(ap, 0) || !have_legacy_id) { ^~~~~~~~~~~~~~~~~~~~~~~~~ ../src/feature/hs/hs_common.c:1729:64: error: null pointer dereference [-Werror=null-dereference] fascist_firewall_allows_address_addr(&ap->addr, ap->port, ~~^~~~~~ In file included from ../src/lib/crypt_ops/crypto_rsa.h:21:0, from ../src/lib/crypt_ops/crypto.h:20, from ../src/core/or/or.h:30, from ../src/feature/hs/hs_common.c:14: ../src/feature/hs/hs_common.c:1748:48: error: null pointer dereference [-Werror=null-dereference] "it: %s:%u", fmt_addr(&ap->addr), ap->port); ~~^~~ ../src/lib/log/log.h:250:51: note: in definition of macro 'log_fn' log_fn_(severity, domain, __FUNCTION__, args, ##__VA_ARGS__) ^~~~~~~~~~~ ../src/feature/hs/hs_common.c:1755:50: error: null pointer dereference [-Werror=null-dereference] onion_key, &ap->addr, ap->port); ~~^~~~~~ cc1.exe: all warnings being treated as errors make: *** [Makefile:8173: src/feature/hs/hs_common.o] Error 1 make: *** Waiting for unfinished jobs....https://ci.appveyor.com/project/torproject/tor/build/job/7isiro2d4mryqtr0#L1825
I have fixed these issues. The same PR is used for this new patch: https://github.com/torproject/tor/pull/252
Appveyor builds on the latest master are failing due to #26986 (moved), so I merged the fix to b23588 and pushed to https://github.com/teor2345/tor.git
I'll review your branch once the CI succeeds.
Trac:
Reviewer: N/A to teor
Status: needs_revision to needs_reviewHi Neel, thanks for this pull request.
I did a review on the pull request on github: https://github.com/torproject/tor/pull/252
Trac:
Status: needs_review to needs_revisionNew PR is here: https://github.com/torproject/tor/pull/256
