Opened 5 years ago

Last modified 4 weeks 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 3 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 4 years ago by mr-4

  • Version set to Tor: 0.2.4.7-alpha

comment:2 Changed 4 years ago by nickm

  • Milestone set to Tor: 0.2.5.x-final

Adding a milestone so this doesn't get lost.

comment:3 Changed 4 years ago by nickm

  • Keywords tor-client added

comment:4 Changed 4 years ago by nickm

  • Keywords controller added

comment:5 Changed 3 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 3 years ago by nickm

  • Cc atagar added
  • Status changed from new to needs_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 follow-up: Changed 3 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 3 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 3 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 3 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 changed from needs_review to needs_revision

comment:13 Changed 3 years ago by nickm

  • Milestone changed from Tor: 0.2.5.x-final to Tor: 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 changed from Tor: 0.2.6.x-final to Tor: 0.2.???
  • Status changed from needs_revision to new

comment:15 Changed 7 months ago by teor

  • Milestone changed from Tor: 0.2.??? to Tor: 0.3.???

Milestone renamed

comment:16 Changed 6 months ago by nickm

  • Keywords tor-03-unspecified-201612 added
  • Milestone changed from Tor: 0.3.??? to Tor: unspecified

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

comment:17 Changed 4 weeks ago by nickm

  • Keywords tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:18 Changed 4 weeks ago by dgoulet

Unify controller keyword to "tor-control".

comment:19 Changed 4 weeks ago by dgoulet

  • Keywords tor-control added; controller removed

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

Note: See TracTickets for help on using tickets.