Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Tor Trac #23588]: Write fascist_firewall_choose_address_ls() and use it in hs_get_extend_info_from_lspecs() #252

Closed
wants to merge 3 commits into from
Closed

[Tor Trac #23588]: Write fascist_firewall_choose_address_ls() and use it in hs_get_extend_info_from_lspecs() #252

wants to merge 3 commits into from

Conversation

Reviewers

@teor2345 teor2345

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@neelchauhan @teor2345 @coveralls
@neelchauhan
Copy link
Contributor

@neelchauhan neelchauhan commented Jul 30, 2018

For the bug here.
@teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jul 30, 2018

Please make the unit tests pass:
https://travis-ci.org/torproject/tor/jobs/409667005#L3572

@neelchauhan neelchauhan force-pushed the b23588 branch 3 times, most recently from 25061c5 to 0b50239 Jul 30, 2018
teor2345
teor2345 suggested changes Jul 31, 2018
View changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Thanks for this pull request!

Relays won't extend via IPv6, so we need to choose an IPv4 address if direct_connect is false. See my comments for details.

There are also a few minor fixes to code and comments, see my comments for details.

src/core/or/policies.c Outdated
*/
void
fascist_firewall_choose_address_ls(const smartlist_t *lspecs,
firewall_connection_t fw_connection,
Copy link
Contributor

@teor2345 teor2345 Jul 31, 2018

Link specifiers are only used for ORPorts, so fw_connection is always FIREWALL_OR_CONNECTION.

src/core/or/policies.c Outdated
ap->port = 0;

/* Here, don't check for IPv6 DirPorts when checking whether we prefer IPv6
* as they're not used. */
Copy link
Contributor

@teor2345 teor2345 Jul 31, 2018

Link specifiers are only used for ORPorts, so this function should not check for any DirPorts.

src/core/or/policies.c Outdated
/* Assume that the DirPorts are zero as link specifiers only use ORPorts. */
fascist_firewall_choose_address_base(&addr_v4, port_v4, 0,
&addr_v6, port_v6, 0,
fw_connection,
Copy link
Contributor

@teor2345 teor2345 Jul 31, 2018

Link specifiers are only used for ORPorts, so fw_connection is always FIREWALL_OR_CONNECTION.

src/core/or/policies.c Outdated
@@ -990,6 +991,60 @@ fascist_firewall_choose_address_rs(const routerstatus_t *rs,
}
}

/** Like fascist_firewall_choose_address_base(), but takes in a smartlist
* <b>lspecs</b> consisting of a link specifier.
Copy link
Contributor

@teor2345 teor2345 Jul 31, 2018

There can be more than one link specifier in the list, so say: "one or more link specifiers".

src/feature/hs/hs_common.c Outdated
@@ -1723,45 +1714,45 @@ hs_get_extend_info_from_lspecs(const smartlist_t *lspecs,
}
} SMARTLIST_FOREACH_END(ls);

fascist_firewall_choose_address_ls(lspecs, FIREWALL_OR_CONNECTION, 0, &ap);

/* Legacy ID is mandatory, and we require IPv4. */
Copy link
Contributor

@teor2345 teor2345 Jul 31, 2018

fascist_firewall_choose_address_ls() can return an IPv6 address, so we require an IP address.
(We only require IPv4 for connections that are not direct.)

src/feature/hs/hs_common.c Outdated
if (!direct_conn) {
/* All clients can extend to any IPv4 via a 3-hop path. */
/* All clients can extend to any IP via a 3-hop path. */
Copy link
Contributor

@teor2345 teor2345 Jul 31, 2018

Relays won't extend via IPv6, we can only connect via IPv6 when we are connecting directly.

src/feature/hs/hs_common.c Outdated
goto validate;
} else if (direct_conn &&
fascist_firewall_allows_address_addr(&addr_v4, port_v4,
fascist_firewall_allows_address_addr(&ap.addr, ap.port,
Copy link
Contributor

@teor2345 teor2345 Jul 31, 2018

This check is redundant, because fascist_firewall_choose_address_ls() has already chosen an allowed address.

src/feature/hs/hs_common.c Outdated
@@ -1723,45 +1714,45 @@ hs_get_extend_info_from_lspecs(const smartlist_t *lspecs,
}
} SMARTLIST_FOREACH_END(ls);

fascist_firewall_choose_address_ls(lspecs, FIREWALL_OR_CONNECTION, 0, &ap);
Copy link
Contributor

@teor2345 teor2345 Jul 31, 2018

If direct_conn is not true, we need to choose an IPv4 address, because relays only extend to IPv4 addresses.
See the similar code in:

tor/src/core/or/circuitbuild.c

Lines 2874 to 2880 in 8b0920b

/* Choose a preferred address first, but fall back to an allowed address. */
if (for_direct_connect)
fascist_firewall_choose_address_node(node, FIREWALL_OR_CONNECTION, 0, &ap);
else {
node_get_prim_orport(node, &ap);
}
valid_addr = tor_addr_port_is_valid_ap(&ap, 0);

(node_get_prim_orport() always returns the IPv4 address.)

neelchauhan added 3 commits Jul 31, 2018
@neelchauhan
Add function fascist_firewall_choose_address_ls()
8a0b6fc
@neelchauhan
Use fascist_firewall_choose_address_ls() in hs_get_extend_info_from_l…
e47dcf6 
…specs()
@neelchauhan
Add changes file for Bug #23588
f9852f6
@neelchauhan
Copy link
Contributor Author

@neelchauhan neelchauhan commented Jul 31, 2018

New PR is here.

@teor2345 teor2345 reopened this Aug 7, 2018
@teor2345
Copy link
Contributor

@teor2345 teor2345 commented Aug 7, 2018

I forced-pushed b23588a from #256 to this branch b23588, so I could see these comments on b23588a.

@coveralls
Copy link

@coveralls coveralls commented Aug 7, 2018

Coverage Status

Coverage increased (+0.03%) to 59.559% when pulling f9852f6 on neelchauhan:b23588 into abf88af on torproject:master.

@teor2345
Copy link
Contributor

@teor2345 teor2345 commented Aug 7, 2018

I wrote comments on #256.

@teor2345 teor2345 closed this Aug 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment