Opened 6 weeks ago

Closed 5 weeks ago

#23499 closed defect (fixed)

dir server responses should include a Date: header even when not responding 200

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: clock-skew
Cc: catalyst Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Catalyst noticed that when the Tor client fetches a consensus from moria1 but moria1 responds "302 Not modified", moria1 does not include a Date: header in that response. So a Tor client whose clock is wrong won't be told about it at this stage.

The fix is that all dir responses should include the Date: header, not just 200 responses.

Child Tickets

Change History (7)

comment:1 Changed 5 weeks ago by catalyst

Cc: catalyst added
Keywords: clock-skew added

comment:2 Changed 5 weeks ago by arma

Status: newneeds_review

My bug23499 branch resolves this issue.

The only tricky part is that it doesn't include the Date: header if you're not a relay or a bridge, since maybe we'll end up in this function through some sort of "go away, why are you asking me directory questions" path, and I don't want to tell people my Date then.

comment:3 Changed 5 weeks ago by arma

I am running the new code on moria1, and here is a sample output:

$ telnet 128.31.0.34 9031
Trying 128.31.0.34...
Connected to 128.31.0.34.
Escape character is '^]'.
GET / HTTP/1.0

HTTP/1.0 404 Not found
Date: Thu, 14 Sep 2017 03:26:56 GMT

Hopefully somebody can check whether that's legit http. :)

comment:4 in reply to:  3 Changed 5 weeks ago by yawning

Replying to arma:

Hopefully somebody can check whether that's legit http. :)

That looks fine.

The only tricky part is that it doesn't include the Date: header if you're not a relay or a bridge, since maybe we'll end up in this function through some sort of "go away, why are you asking me directory questions" path, and I don't want to tell people my Date then.

This is fine for HTTP/1.0 which is what is used now. If the daemon ever migrates to HTTP/1.1, the behavior will likely need revisiting (RFC 7231 7.1.1.2).

comment:5 Changed 5 weeks ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: 0.3.2.x-final

dcd0aea85e9315c53a6b70a4a2b62ec65d539896 seems unnecessary. we weren't logging reason_phrase, but instead were logging reason_phrase ? reason_phrase : "OK". Doesn't seem like a terrible problem though. I'm going to turn that conditional into IF_BUG_ONCE.

Also I think we should renamed write_http_status_line(), since it doesn't actually write a line any more.

I've done both of these in bug23499 in my repository, based on top of yours. I think this can go in 0.3.2 if you like. What do you think of the changes?

comment:6 Changed 5 weeks ago by arma

Status: needs_reviewmerge_ready

Your patches look good! Thanks. (check-spaces might have something to say to you.)

(I spent a little while thinking about whether we could refactor the "write the status code and date header" part out to its own thing, since write_http_response_header_impl() essentially do that too, but I didn't hit upon a satisfactory way. If you find one, please feel free. :)

Oh, and as for whether dcd0aea8 is unnecessary, look right below it to where we

  log_debug(LD_DIRSERV,"Wrote status 'HTTP/1.0 %d %s'", status, reason_phrase);

That's the log I meant.

comment:7 Changed 5 weeks ago by nickm

Resolution: fixed
Status: merge_readyclosed

Thanks! And sorry for missing that log. Who runs at debug anyway? ;)

Merging to master.

Note: See TracTickets for help on using tickets.