add option to skip supplementation of a `Date` header #1495

deweerdt · 2017-11-09T23:59:24Z

Current H2O always sets the date: header, both in HTTP/1 and HTTP/2. This might be problematic, for example when a mruby handler or when an upstream sets it.

This PR addresses the issue by not setting the header in the protocol layer, but instead leaving it to the individual handlers to do it. As a result:

proxy never sets it, leaves it as-is if upstream sets it
file always sets it
mruby and fastcgi only set it if isn't set already.

file: always add a date: header

deweerdt · 2017-11-10T00:00:04Z

/cc @i110 this is about the date: header in the response

i110 · 2017-11-10T00:59:55Z

lib/http1.c

@@ -601,20 +601,16 @@ static size_t flatten_headers_estimate_size(h2o_req_t *req, size_t server_name_a
 static size_t flatten_headers(char *buf, h2o_req_t *req, const char *connection)


should tweak flatten_headers_estimate_size too?

done in ed0786d

i110 · 2017-11-10T01:13:41Z

Thank you for the bug fix. One thing I don't understand is, are there any serious reasons that we mustn't append Date header if the upstream doesn't send it?

https://tools.ietf.org/html/rfc7231#section-7.1.1.2

A recipient with a clock that receives a response message without a
   Date header field MUST record the time it was received and append a
   corresponding Date header field to the message's header section if it
   is cached or forwarded downstream.

So in my understanding, the proxy handler also should do set it if isn't set already. If so, the protocol handlers can do the following and other handlers doesn't need to do anything.

    if (h2o_find_header(&req->res.headers, H2O_TOKEN_DATE, 0) < 0)
        h2o_resp_add_date_header(req);

But if there are some use cases that we don't want to fill the Date header, I think it might be good to add a configuration knob to switch the behavior. What do you think?

deweerdt · 2017-11-10T03:04:52Z

One thing I don't understand is, are there any serious reasons that we mustn't append Date header if the upstream doesn't send it?

Some servers give you full control over the headers that you send, and it surprises some customers if they see it appear when we switch to H2O.

Similarly, if you set a specific date in mruby, i would expect the header to take precedence over the one set by the stack?

We could definitely add a knob to handle the proxy case (where the date: header is set unless it's already set by upstream)

i110 · 2017-11-10T03:37:55Z

Similarly, if you set a specific date in mruby, i would expect the header to take precedence over the one set by the stack?

Yes, I understand that the header value which is explicitly set by the upstream server or mruby handler should take precedence over auto-filled value. So I agree that the current implementation has the bug. OTOH, if the date header is missing, I'm afraid the default behavior of h2o should conform HTTP spec, i.e. fill the Date header automatically. So adding a knob is better way to resolve your issue?

BTW I tried nginx and apache proxy modules, they fill the date header if it's missing, and there seems to be no configuration options to suppress that.

deweerdt · 2017-11-10T04:00:38Z

So adding a knob is better way to resolve your issue?

Yes, i think i agree, it would also have the advantage of not changing the current behavior (except when the header is set by upstream, but that's clearly a bug). @kazuho any thoughts?

kazuho · 2017-11-10T06:36:13Z

lib/handler/fastcgi.c

@@ -548,6 +548,10 @@ static int fill_headers(h2o_req_t *req, struct phr_header *headers, size_t num_h
        }
    }

+    /* add date: if it's missing from the response */
+    if (h2o_find_header(&req->res.headers, H2O_TOKEN_DATE, 0) < 0)


Could we use != -1 as a check? It means the same, but it might be a good idea to be consistent with other parts of the code that invoke the same function.

done in 79e5186

kazuho · 2017-11-10T06:41:52Z

So adding a knob is better way to resolve your issue?

Yes, i think i agree, it would also have the advantage of not changing the current behavior (except when the header is set by upstream, but that's clearly a bug). @kazuho any thoughts?

Actually, I prefer having less knobs. Having less makes the code concise and less prone to bugs.

I would argue that we do not need to fix the behavior of a broken HTTP server by the fact that we are forwarding the response.

I would also be a bit worried about the change of the meaning when we add a Date header. Consider the case in which origin sends an Expires header without Date. The client is expected to calculate the difference between it's clock time and the value of the Expires header, and use that as the period for the returned response to be considered fresh. If we add Date header, the meaning changes, since the client would then use the value of the Date header field supplied by H2O to calculate the difference.

i110 · 2017-11-10T12:39:40Z

Hmm, I still feel that it's better to have default behaviors conform to RFC as strict as possible, and changing(?) the meaning of Date header is slight problem for age calculation, because many clients like browsers also use their local time to check the freshness in such case (I believe this approximation works well in most cases) and, above all, it's the spec. Also as an additional bonus, if we can handle Date header in protocol handlers, we can remove Date-header code in each handlers and the fear of forgetting to write that when we create new handlers

But actually I don't have strong motivation to argue it because I think the chance of missing Date header is not so high, so sorry for bothering you!

kazuho · 2017-11-15T03:09:54Z

@i110 I actually might agree with you about making this an opt-in feature. RFC 7231 section 7.1.1.2 states:

An origin server MUST NOT send a Date header field if it does not
have a clock capable of providing a reasonable approximation of the
current instance in Coordinated Universal Time.
(snip)
A recipient with a clock that receives a response message without a
Date header field MUST record the time it was received and append a
corresponding Date header field to the message's header section if it
is cached or forwarded downstream.

So when acting as an intermediary, H2O is required to add a Date header (unless the user wants to opt-out from such behavior).

Also as an additional bonus, if we can handle Date header in protocol handlers, we can remove Date-header code in each handlers and the fear of forgetting to write that when we create new handlers

I am not convinced with this.

In the proxy handler, we need to preserve the Date header sent by upstream (the fact that we do not not do that now is a bug). The fact means that the logic that adds the Date header needs to traverse through the HTTP headers list. I do not want such traversal to happen for every HTTP request H2O handles (e.g. when serving a static file).

That leads to the conclusion that every handler is required to add a Date header.

This is ON by default (which has beem H2O's behavior historically). If set to OFF, H2O won't be setting the `date:` header if it's missing from the upstream response

deweerdt · 2017-11-17T18:53:02Z

245bcb4 adds a proxy.emit-missing-date-header knob, and it defaults to ON, since that's been H2O's behavior historically.

kazuho · 2017-11-24T02:37:18Z

Thank you for working on the issue!

deweerdt added 7 commits November 9, 2017 10:58

http2: don't automatically set the date: header

f36c6b1

http1: don't automatically send the date: header

e645827

An API to add a date: header to a response

10747d2

proxy: Don't drop the date header if present

d511644

fastcgi, mruby: only set a date: header if it's not already present

66c7512

file: always add a date: header

Fix unit test

1c918d8

The test now must check that the response header is preserved

81c074f

i110 reviewed Nov 10, 2017

View reviewed changes

kazuho reviewed Nov 10, 2017

View reviewed changes

http/1: adjust flatten_headers_estimate_size as well

ed0786d

deweerdt force-pushed the only-set-date-header-when-needed branch from cfa38ec to c9991fb Compare November 11, 2017 05:32

deweerdt added 3 commits November 10, 2017 21:32

add test coverage

c9991fb

Follow existing h2o_find_header call checks

79e5186

Address review comments

f20b96b

deweerdt added 2 commits November 17, 2017 10:48

Add a proxy.emit-missing-date-header knob

8cfe555

This is ON by default (which has beem H2O's behavior historically). If set to OFF, H2O won't be setting the `date:` header if it's missing from the upstream response

Generate docs

245bcb4

kazuho merged commit 2886d79 into h2o:master Nov 24, 2017

i110 mentioned this pull request Dec 22, 2017

fix duplicate date headers #1566

Merged

kazuho changed the title ~~Only set date header when needed~~ add option to skip supplementation of a Date header Jan 16, 2018

i110 mentioned this pull request Jun 3, 2018

remove duplicate date headers #1780

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add option to skip supplementation of a `Date` header #1495

add option to skip supplementation of a `Date` header #1495

deweerdt commented Nov 9, 2017

deweerdt commented Nov 10, 2017

i110 Nov 10, 2017

deweerdt Nov 11, 2017

i110 commented Nov 10, 2017 •

edited

deweerdt commented Nov 10, 2017 •

edited

i110 commented Nov 10, 2017 •

edited

deweerdt commented Nov 10, 2017

kazuho Nov 10, 2017

deweerdt Nov 11, 2017

kazuho commented Nov 10, 2017 •

edited

i110 commented Nov 10, 2017 •

edited

kazuho commented Nov 15, 2017 •

edited

deweerdt commented Nov 17, 2017

kazuho commented Nov 24, 2017

		@@ -601,20 +601,16 @@ static size_t flatten_headers_estimate_size(h2o_req_t *req, size_t server_name_a
		static size_t flatten_headers(char buf, h2o_req_t req, const char *connection)

add option to skip supplementation of a Date header #1495

add option to skip supplementation of a Date header #1495

Conversation

deweerdt commented Nov 9, 2017

deweerdt commented Nov 10, 2017

i110 Nov 10, 2017

Choose a reason for hiding this comment

deweerdt Nov 11, 2017

Choose a reason for hiding this comment

i110 commented Nov 10, 2017 • edited

deweerdt commented Nov 10, 2017 • edited

i110 commented Nov 10, 2017 • edited

deweerdt commented Nov 10, 2017

kazuho Nov 10, 2017

Choose a reason for hiding this comment

deweerdt Nov 11, 2017

Choose a reason for hiding this comment

kazuho commented Nov 10, 2017 • edited

i110 commented Nov 10, 2017 • edited

kazuho commented Nov 15, 2017 • edited

deweerdt commented Nov 17, 2017

kazuho commented Nov 24, 2017

add option to skip supplementation of a `Date` header #1495

add option to skip supplementation of a `Date` header #1495

i110 commented Nov 10, 2017 •

edited

deweerdt commented Nov 10, 2017 •

edited

i110 commented Nov 10, 2017 •

edited

kazuho commented Nov 10, 2017 •

edited

i110 commented Nov 10, 2017 •

edited

kazuho commented Nov 15, 2017 •

edited