Use trunnel to parse and generate SOCKS wire format in tor-resolve

changed milestone to %Tor: 0.4.0.x-final

added component::core tor/tor milestone::Tor: 0.4.0.x-final owner::rl1987 priority::medium resolution::implemented reviewer::dgoulet security severity::normal socks status::closed tor-resolve trunnel type::enhancement version::tor unspecified wireformat labels

Trac:
Status: new to accepted
Owner: N/A to rl1987

https://github.com/torproject/tor/pull/325

Trac:
Status: accepted to needs_review

Trac:
Milestone: Tor: unspecified to Tor: 0.3.6.x-final

Trac:
Reviewer: N/A to catalyst

Trac:
Reviewer: catalyst to dgoulet

Code lgtm overall. Only issue I found is that when I test tor-resolve with SOCKS5, I get the IP in reverse order :)

$ ./src/tools/tor-resolve -4 torproject.org
95.216.163.36
$ ./src/tools/tor-resolve -5 torproject.org
36.163.216.95

So I'm assuming some place there is a ntohl() missing? :)

Trac:
Status: needs_review to needs_revision

Fixed in 38390604a7e3215483d4a53d7a18b45f93e76828.

Trac:
Status: needs_revision to needs_review

lgtm;

(Needs autosquash)

Trac:
Status: needs_review to merge_ready

Tor 0.3.6.x has been renamed to 0.4.0.x.

Trac:
Milestone: Tor: 0.3.6.x-final to Tor: 0.4.0.x-final

Made a squashed & rebased version as https://github.com/torproject/tor/pull/556 .

So far the only suggestions I have are:

• We should be using strlcpy, not strncpy.
• If we allow socks4_server_reply_version to be zero, then we'll need to explicitly set it to 4 in proto_socks.c so that we're sending the right value from Tor, right?
• Do we have a ticket for adding tests for tor-resolve?

I'll make all the above changes before merging, if the CI passes.

Replying to nickm:

Made a squashed & rebased version as https://github.com/torproject/tor/pull/556 .

So far the only suggestions I have are:

• We should be using strlcpy, not strncpy.

Though actually, tor_strdup() would be fine here, since the trunnel "getstr" functions return nul-terminated strings.

• If we allow socks4_server_reply_version to be zero, then we'll need to explicitly set it to 4 in proto_socks.c so that we're sending the right value from Tor, right?

Oh. apparently we don't do that. That's okay.

• Do we have a ticket for adding tests for tor-resolve?

Yes, #27854 (moved), a child of this ticket.

I'll make all the above changes before merging, if the CI passes.

Done, merged.

Trac:
Resolution: N/A to implemented
Status: merge_ready to closed

closed

mentioned in issue #27854 (moved)

moved to tpo/core/tor#27620 (closed)