Opened 5 years ago

Closed 4 months ago

#5847 closed defect (implemented)

Better error message on GETINFO desc/* when you only have MDs.

Reported by: mickeyc Owned by:
Priority: Low Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version: Tor: 0.2.3.15-alpha
Severity: Normal Keywords: easy, tor-client, tor-control, review-group-18
Cc: atagar Actual Points:
Parent ID: Points: small
Reviewer: nickm Sponsor:

Description

The "GETINFO desc/id/OR identity" and "GETINFO desc/name/OR nick" options don't seem to work on the Tor control port:

GETINFO desc/id/DD397148A4AB4D43E5E6CB9C5F45E922872CC2D3
552 Unrecognized key "desc/id/DD397148A4AB4D43E5E6CB9C5F45E922872CC2D3"

If I replace "desc" with "md" I get the expected response, and they're both described in the spec as taking the same arguments...

Child Tickets

Change History (24)

comment:1 Changed 5 years ago by nickm

Status: newneeds_information

Sounds a lot like your client is fetching microdescriptors and not full descriptors. If that's the case, the behavior you're seeing would be expected: you'd have the microdesc for the given ID, but not the full descriptor.

Is that what's happening?

comment:2 Changed 5 years ago by mickeyc

You are correct. I just added "UseMicrodescriptors 0" to my torrc and now it is working. I wonder if it would be possible to give a more accurate error message than "Unrecognized key" in this scenario?

comment:3 Changed 5 years ago by atagar

Status: needs_informationnew

Agreed, a note in the spec about these being disabled by default nowadays and a better error message would be nice. I was bitten by this too a while back and puzzled for a few minutes before remembering about microdescriptors.

comment:4 Changed 5 years ago by nickm

Milestone: Tor: unspecified

seems reasonable. It would require a change to the GETINFO infrastructure, though: right now, every 552 error for a key not being present has the message "Unrecognized key." Setting the error message would turn the code into a 551. Since it's fairly minor, I'll defer to 0.2.4.x or later. I'd take a spec clarification patch any time though.

comment:5 Changed 5 years ago by nickm

Keywords: documentation spec easy added

comment:6 Changed 5 years ago by maker

Status: newneeds_review

comment:7 Changed 5 years ago by maker

Status: needs_reviewnew

comment:8 Changed 5 years ago by nickm

Keywords: tor-client added

comment:9 Changed 5 years ago by nickm

Component: Tor ClientTor

comment:10 Changed 5 years ago by nickm

Keywords: controller added; documentation spec removed
Priority: majorminor

I've added the note to the spec in f811e9eb07e672c1d58f86f7c3470574c3b53715.

Adding a better error message is still worth doing, so I'll leave this open.

comment:11 Changed 4 years ago by Ry

Need some clarification, is it only a better error message that's wanted but still returning 552, or are we ok with a 551?

I'd imagine 554 (Invalid descriptor) is what we really want to respond?

Also I'm pretty sure I'm reading the correct spec addition you mention (just by looking at the spec) but I cannot find the commit you're referencing with that digest in either the main tor repo or the torspec repo. Where can I find that?

comment:12 Changed 4 years ago by nickm

I think that 552 with a more accurate error message is fine.

I worry about 554, since controllers won't be expecting it from a GETINFO (I think).

f811e9eb07e672c1d58f8 was added to master torspec repository last november. You can also find it at https://gitweb.torproject.org/torspec.git/commitdiff/f811e9eb07e672c1d58f86f7c3470574c3b53715

comment:13 Changed 4 years ago by Ry

Apparently reply by email isn't supported? :(

https://github.com/Ryman/tor/tree/bug5847

comment:14 Changed 4 years ago by Ry

Status: newneeds_review

comment:15 Changed 4 years ago by Ry

I think we're also leaking errmsg if we return a 551 or if we get an answer but errmsg was also set (shouldn't happen). So I think we need 2 additional frees of errmsg after 2293 and 2302, but that could just be 4am talking. (I think someone was trying to use msg instead of errmsg at one point, I deleted it as it wasn't actually being used)

comment:16 Changed 21 months ago by nickm

Points: small
Severity: Normal
Summary: GETINFO "desc/id/OR identity" doesn't workBetter error message on GETINFO desc/* when you only have MDs.

comment:17 Changed 5 months ago by dgoulet

Keywords: easy tor-client controllereasy, tor-client, controller

Unify controller keyword to "tor-control".

comment:18 Changed 5 months ago by dgoulet

Keywords: tor-control added; controller removed

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

comment:19 Changed 5 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.2.x-final

we should pick this up again and review it; it's pretty small.

comment:20 Changed 4 months ago by nickm

Keywords: review-group-18 added

comment:21 Changed 4 months ago by nickm

Cc: atagar added
Status: needs_reviewmerge_ready

Looks good. I rebased and tweaked this as Ryman-bug5847. Damian, will it break anything in stem if we merge this?

comment:22 Changed 4 months ago by nickm

Reviewer: nickm

comment:23 Changed 4 months ago by atagar

Hi Nick. Stem should be fine. We explicitly check 'GETCONF UseMicrodescriptors' rather than relying on the error message so you can change that without impacting us.

comment:24 Changed 4 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

great; merged Ryman-bug5847-squashed, and opened #22684 to talk about a better alternative to GETCONF UseMicrodescriptors.

Note: See TracTickets for help on using tickets.