Opened 11 months ago

Closed 4 months ago

#31081 closed task (fixed)

GETCONF allows zero arguments, contrary to spec

Reported by: catalyst Owned by:
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-control, tor-spec
Cc: Actual Points: 0.1
Parent ID: Points: 1
Reviewer: teor Sponsor:

Description

control-spec.txt says GETCONF takes at least one argument. Tor currently allows there to be zero arguments for GETCONF. If there are zero arguments, it replies with 250 OK. A reply with a keyword and no = can mean that a configuration variable has its default value. We've therefore committed to having no configuration variables named OK. I suppose that's OK.

Probably the easiest thing to do is to document the existing behavior.

Child Tickets

Change History (12)

comment:1 Changed 11 months ago by rl1987

Status: newneeds_review

I propose rejecting GETCONF commands with no arguments as they don't make sense semantically. The patch turned out to be trivial.

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

comment:2 in reply to:  1 ; Changed 11 months ago by teor

Replying to rl1987:

I propose rejecting GETCONF commands with no arguments as they don't make sense semantically. The patch turned out to be trivial.

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

We talked about rejecting GETCONF with no arguments, but we decided it was easier to update the spec. We were concerned that we might break controllers that depend on the incorrect behaviour. And it seems like such a trivial issue to risk breaking a controller.

We can talk about it again at our next team meeting, if you'd like.

If we do decide to reject GETCONF with no arguments, we'll also need to change the spec to document the versions that (mistakenly) accepted no arguments.

comment:3 in reply to:  2 Changed 11 months ago by catalyst

Replying to teor:

We talked about rejecting GETCONF with no arguments, but we decided it was easier to update the spec. We were concerned that we might break controllers that depend on the incorrect behaviour. And it seems like such a trivial issue to risk breaking a controller.

We can talk about it again at our next team meeting, if you'd like.

If we do decide to reject GETCONF with no arguments, we'll also need to change the spec to document the versions that (mistakenly) accepted no arguments.

We should figure out which stakeholders we should consult about this behavior change, because I think not all of them regularly attend our team meetings.

comment:4 Changed 10 months ago by dgoulet

Status: needs_reviewneeds_information

comment:5 Changed 10 months ago by catalyst

Status: needs_informationneeds_revision

We talked about it at the 2019-07-08 dev meeting. It's probably safest to document the existing behavior in the spec.

comment:6 Changed 4 months ago by gaba

Sponsor: Sponsor31-can

No more sponsor 31. All this tickets remained open after sponsor 31 ended.

comment:7 Changed 4 months ago by rl1987

Status: needs_revisionneeds_review

comment:8 Changed 4 months ago by teor

Actual Points: 0.1
Milestone: Tor: unspecifiedTor: 0.4.3.x-final
Status: needs_reviewneeds_revision
Type: defecttask

Thanks!

I suggested a tweak to the wording of your spec patch, so that the text and the syntax match. (And the text explicitly allows zero arguments.)

Did you also want to document that zero keywords returns "250 OK", so that controllers don't think it's an option?

comment:9 Changed 4 months ago by rl1987

Status: needs_revisionneeds_review

comment:10 Changed 4 months ago by dgoulet

Reviewer: teor

comment:11 Changed 4 months ago by teor

Status: needs_reviewmerge_ready

Looks good, let's merge.

comment:12 Changed 4 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Looks good to me too; merged to torspec.

Note: See TracTickets for help on using tickets.