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

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.

changed milestone to %Tor: 0.3.2.x-final

added clock-skew component::core tor/tor milestone::Tor: 0.3.2.x-final priority::medium resolution::fixed severity::normal status::closed type::defect labels

Trac:
Cc: N/A to catalyst
Keywords: N/A deleted, clock-skew added

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.

Trac:
Status: new to needs_review

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. :)

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).

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?

Trac:
Milestone: Tor: 0.3.3.x-final to Tor: 0.3.2.x-final

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.

Trac:
Status: needs_review to merge_ready

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

Merging to master.

Trac:
Status: merge_ready to closed
Resolution: N/A to fixed

closed

mentioned in issue #23908 (moved)

moved to tpo/core/tor#23499 (closed)