Opened 6 years ago

Last modified 3 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: Normal 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

TicketStatusOwnerSummaryComponent
#4391new`GETINFO ns/all` doesn't return 'p' lines -- make something that does!Core Tor/Tor
#24110newdocument control protocol router status format surprises when using microdescriptorsCore Tor/Tor
#28676newTor versions of Tor nodes should be accessible through ControlPortCore Tor/Tor
#28982acceptedrl1987Refactor GETINFO dir/ so that new tor/ URLs automatically become GETINFOsCore Tor/Tor

Attachments (1)

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

Download all attachments as: .zip

Change History (26)

comment:1 Changed 6 years ago by mr-4

Version: Tor: 0.2.4.7-alpha

comment:2 Changed 6 years ago by nickm

Milestone: Tor: 0.2.5.x-final

Adding a milestone so this doesn't get lost.

comment:3 Changed 6 years ago by nickm

Keywords: tor-client added

comment:4 Changed 6 years ago by nickm

Keywords: controller added

comment:5 Changed 5 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 5 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 5 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 5 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 5 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 5 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 5 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 5 years ago by nickm (previous) (diff)

comment:12 Changed 5 years ago by nickm

Status: needs_reviewneeds_revision

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

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

comment:15 Changed 2 years ago by teor

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

Milestone renamed

comment:16 Changed 2 years 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 22 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:18 Changed 22 months ago by dgoulet

Unify controller keyword to "tor-control".

comment:19 Changed 22 months ago by dgoulet

Keywords: tor-control added; controller removed

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

comment:20 Changed 16 months ago by teor

Severity: Normal

Set all open tickets without a severity to "Normal"

comment:21 Changed 3 months ago by wagon

The following may be relevant for this ticket. I don't think we have to have full duplicates for ControlPort commands because it confuses users. Now GETINFO commands dir/server/all and desc/all-recent return identical outputs if UseMicrodescriptors is set to 0. Which if these commands should be preferred if needed?

comment:22 Changed 3 months ago by wagon

Now GETINFO dir/status-vote/current/consensus returns 6392 relays while GETINFO desc/all-recent returns 7294 relays. Why these numbers are different? Is it so, because descriptors are distributed in parallel to consensus and are not trusted until they enter consensus?

comment:23 in reply to:  21 Changed 3 months ago by teor

Replying to wagon:

The following may be relevant for this ticket. I don't think we have to have full duplicates for ControlPort commands because it confuses users.

But removing duplicates breaks old code, so we need to be careful when we do that. If you want to remove this duplicate, please open a separate ticket.

Now GETINFO commands dir/server/all and desc/all-recent return identical outputs if UseMicrodescriptors is set to 0. Which if these commands should be preferred if needed?

I don't know. If they return identical outputs for all Tor options, then it literally doesn't matter which one you use. If their outputs change when Tor's options change, then use the one that works for your code when the options change.

They are implemented differently, so dir/server/all might be more memory efficient in some cases.

You can also use md/all if you want microdescriptors, but only on Tor 0.3.5.1-alpha and later.

comment:24 in reply to:  22 Changed 3 months ago by teor

Replying to wagon:

Now GETINFO dir/status-vote/current/consensus returns 6392 relays while GETINFO desc/all-recent returns 7294 relays. Why these numbers are different? Is it so, because descriptors are distributed in parallel to consensus and are not trusted until they enter consensus?

Yes. You can check your answers to these questions by reading the specs:
https://gitweb.torproject.org/torspec.git/tree/control-spec.txt#n637

https://gitweb.torproject.org/torspec.git/tree/control-spec.txt#n807
https://gitweb.torproject.org/torspec.git/tree/dir-spec.txt#n3866

comment:25 Changed 3 months ago by wagon

But removing duplicates breaks old code, so we need to be careful when we do that. If you want to remove this duplicate, please open a separate ticket.

Well, I thought that if we have two options which do the same, one of them is deprecated and will cease to exist in some moment in future. However, you say that both of them are not deprecated now.

I asked this question because I'm not sure whether it is a bug or feature. So, if you say it is a feature, I shouldn't create a new ticket.

dir/server/all might be more memory efficient in some cases

Thanks!

Note: See TracTickets for help on using tickets.