Opened 5 years ago

Last modified 4 months ago

#7646 new defect

fix/enhance getinfo ns/id/* commands

Reported by: mr-4 Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: 0.2.4.7-alpha
Severity: Keywords: tor-client, needs-spec, tor-control
Cc: atagar Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

  1. Fix getinfo ns/id/*

See the last few comments on #7059 with regards to the ns/id/* getinfo command.

Currently, it only returns information about tor nodes which have microdescriptors and ignores those that use "normal" descriptors, in other words, it behaves exactly like the md/id/node command.

I can understand the rationale behind having desc/id/node to only show information about tor nodes with "normal" descriptors and I can understand the reasons for md/id/node to do the same for tor nodes with microdescriptors, but ns/id/node should, in my view, be implemented in such a way to return information about all tor nodes, regardless of whether or not they use microdescriptors, particularly so because by definition of this command in control-specs.txt it is supposed to return V2 directory info on all nodes without making such distinctions.

  1. Enhance ns/* - just an idea (not a bug!): currently, there is no way I could get information on a tor node by specifying its ip address.

I would love to be able to get that information by executing something like "getinfo ns/ip/<ip_address>" (note the "ip" in the middle!). This should return information on all tor nodes with the specified ip address (and/or netmask), regardless of whether they use microdescriptors or not.

Child Tickets

Attachments (1)

0001-GETINFO-ns-id-should-return-a-networkstatus-flavoure.patch (3.0 KB) - added by rl1987 4 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 5 years ago by mr-4

Version: Tor: 0.2.4.7-alpha

comment:2 Changed 5 years ago by nickm

Milestone: Tor: 0.2.5.x-final

Adding a milestone so this doesn't get lost.

comment:3 Changed 5 years ago by nickm

Keywords: tor-client added

comment:4 Changed 5 years ago by nickm

Keywords: controller added

comment:5 Changed 4 years ago by rl1987

I tried fixing GETINFO ns/id/$DIGEST by searching the network status smartlist only and ignoring the microdescriptors smartlist. Is this approach valid? When I add UseMicrodescriptors 1 line to my torrc, GETINFO ns/id/$DIGEST doesn't return anything since Tor instance refrain from downloading network statuses when this configuration option is set. Is this okay? Does this bug even apply to modern Tor codebase, provided that it uses V3 directory protocol now?

comment:6 Changed 4 years ago by nickm

Cc: atagar added
Status: newneeds_review

Damian, would this mess anything up? I wonder if we shouldn't just redesign the interface we *want*, and then implement it, rather than trying to reverse engineer what we meant when we designed the old thing.

comment:7 Changed 4 years ago by atagar

Currently, it only returns information about tor nodes which have microdescriptors and ignores those that use "normal" descriptors, in other words, it behaves exactly like the md/id/node command.

I'm not clear - (a) do you mean that it provides router status entries if and only if microdescriptors are available, or (b) it provides actual microdescriptors like md/id/node?

... this command in control-specs.txt it is supposed to return V2 directory info on all nodes without making such distinctions.

This was fixed in the spec a while back.

When I add UseMicrodescriptors 1 line to my torrc, GETINFO ns/id/$DIGEST doesn't return anything since Tor instance refrain from downloading network statuses when this configuration option is set.

... huh? UseMicrodescriptors 1 should mean that we download microdescriptors and router status entries, while UseMicrodescriptors 0 means that we download server descriptors and router status entries. Are you saying that we opt to only download microdescriptors by default nowadays?

Damian, would this mess anything up? I wonder if we shouldn't just redesign the interface we *want*, and then implement it, rather than trying to reverse engineer what we meant when we designed the old thing.

If mr-4 meant (a) then that sounds like a bug and fixing it will make controllers happy (... though I'm not sure why the relays you have microdescriptors for wouldn't match the router status entires). If he meant (b) then that sounds like a horrible bug and I'm not sure how it's possible that went undetected (... Stem has integ tests that should definitely complain about that).

Tor's "GETINFO ns/id/*" options are pretty nice as-is and used by Stem. I'm not sure what type of re-implementation you have in mind.

Cheers! -Damian

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

Replying to atagar:

Currently, it only returns information about tor nodes which have microdescriptors and ignores those that use "normal" descriptors, in other words, it behaves exactly like the md/id/node command.

I'm not clear - (a) do you mean that it provides router status entries if and only if microdescriptors are available, or (b) it provides actual microdescriptors like md/id/node?

The actual behavior is that it looks up the routerstatus item in the current consensus. If we're using a microdescriptor consensus, it looks at that. If we're using a networkstatus consensus, it looks at that.

It works even if we haven't fetched the microdescriptors themselves -- the issue is that it might return results from a FLAV_NS consensus.

... this command in control-specs.txt it is supposed to return V2 directory info on all nodes without making such distinctions.

This was fixed in the spec a while back.

When I add UseMicrodescriptors 1 line to my torrc, GETINFO ns/id/$DIGEST doesn't return anything since Tor instance refrain from downloading network statuses when this configuration option is set.

... huh? UseMicrodescriptors 1 should mean that we download microdescriptors and router status entries, while UseMicrodescriptors 0 means that we download server descriptors and router status entries. Are you saying that we opt to only download microdescriptors by default nowadays?

"NS" is the flavor of the old-style, pre-microdescriptor networkstatus consensus documents. I think that mr-4 is calling those networkstatuses. We don't download FLAV_NS networkstatuses by default any more when UseMicrodescriptors 0 is set; we download FLAV_MICRODESC networkstatuses instead.

Damian, would this mess anything up? I wonder if we shouldn't just redesign the interface we *want*, and then implement it, rather than trying to reverse engineer what we meant when we designed the old thing.

If mr-4 meant (a) then that sounds like a bug and fixing it will make controllers happy (... though I'm not sure why the relays you have microdescriptors for wouldn't match the router status entires). If he meant (b) then that sounds like a horrible bug and I'm not sure how it's possible that went undetected (... Stem has integ tests that should definitely complain about that).

Tor's "GETINFO ns/id/*" options are pretty nice as-is and used by Stem. I'm not sure what type of re-implementation you have in mind.

Hm. This suggests that the right thing to do here might be "no change in 0.2.5, just document the current behavior", and turn this into a needs-proposal thing.

comment:9 Changed 4 years ago by atagar

Oooh, I see. Ewww, that is unpleasant.

This is a bug then that is indeed impacting Stem. Presently it assumes that the ns methods are giving us RouterStatusEntryV3 instances, but sometimes it's RouterStatusEntryMicroV3 instead. That would actually explain some confusing behavior I saw a while back.

For now I'll have Stem check UseMicrodescriptors so we can start providing the write instance types. Thanks for bringing this to my attention!

comment:10 Changed 4 years ago by atagar

Pushed a fix to Stem to account for this.

Honestly I'm not quite sure what the right way forward is on Tor's side. If Tor only has microdescriptor router status entries available then that's clearly all it can provide controllers. Long term there's several changes I'd like in Tor's descriptors, but this isn't the place for that.

comment:11 Changed 3 years ago by nickm

Actually... ug.

If I'm analyzing this right, the current behavior is just plain ugly. It takes the router status information, not from the node_t (where "our current opinion about the router" should be), but from the networkstatus_t (the thing we downloaded, which used to get modified with our opinions of node status, but which now is treated as immutable since we did the node_t refactor). And then it writes it in the old, pre-microdescriptor format...whether it's microdescriptor data or not, so the microdescriptor digest gets written out in the place that the descriptor digest should go, with its last 12 bytes omitted.

Yuck yuck yuck.

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

comment:12 Changed 3 years ago by nickm

Status: needs_reviewneeds_revision

comment:13 Changed 3 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final

tentatively moving these out of 0.2.5.

comment:14 Changed 3 years ago by nickm

Keywords: needs-spec added
Milestone: Tor: 0.2.6.x-finalTor: 0.2.???
Status: needs_revisionnew

comment:15 Changed 10 months ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:16 Changed 9 months ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:17 Changed 4 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:18 Changed 4 months ago by dgoulet

Unify controller keyword to "tor-control".

comment:19 Changed 4 months ago by dgoulet

Keywords: tor-control added; controller removed

Unify "controller" keyword to "tor-control".

Note: See TracTickets for help on using tickets.