Protected headers

[dridi]

This all started with one of @nigoroll's comments in his review of #3798:

As req.http.Content-Length can be modified from VCL, I think we should rather either fix the header or fail the request rather than panicking.

My first thought was that we should prevent that, briefly followed by "in VCL". I don't want to prevent VMODs from implementing their own range support and update headers that would otherwise be protected.

We should protect headers that are essential for HTTP fetch and delivery, like content-length in this example. In its current state the pull request is merely a working draft making req.http.content-length read only and lacking a comprehensive set of protected headers.

[bsdphk]

Sorry, but I see no reason for adding this complexity.

[dridi]

bugwash: someone to mention headers that shouldn't be touched in the VCL manual.

[dridi]

Reopening for discussion.

There are some of us who would rather make VCL safer by default. We also understand that others want to keep the ability for critical headers fixups without involving VMODs. Complexity aside, we came up during bugwash today with a middle ground: protect framing headers with a syntax similar to the one in this pull request, that is to say self-documenting since it would originate from the manual like any other variable, with the ability to override the restriction:

sub vcl_recv {
        unsafe {
                set req.http.content-length = "42";
        }
}

This way, one could explicitly opt in to this kind of fiddling without resorting to VMODs, while sending a clear signal that we may be walking a tightrope here.

[bsdphk]

I understand the issue and the motivation for this proposal, but I am still not convinced it is something we should expend source code lines on.

[dridi]

bugwash: @mbgrydeland, @nigoroll and myself are still arguing for unsafe blocks, @bsdphk against. No consensus yet.

Probably needs more discussions.

[dridi]

Would a prototype similar to this one (only one protected header, not a comprehensive list) and an unsafe block implementation help measure the complexity overhead for libvcc?

We already know the syntax overhead for VCL authors: none, unless you are fiddling with framing headers, in which case, not much.

[nigoroll]

Bugwash has gone full circle and now agreed that the proposal herein is the best option. @Dridi was asked to update the code and make Transfer-Enconding read-only by default also.

[dridi]

Rebased against master and completed, ready for bugwash.

[nigoroll]

FTR I ran A diff on the VGC of the builtin VCL for -b 127.0.0.1:

--- /tmp/o	2023-07-31 15:39:17.702525067 +0200
+++ /tmp/n	2023-07-31 15:38:36.966430111 +0200
@@ -1274,6 +1274,8 @@
 
 void VRT_u_req_http(VRT_CTX);
 
+
+
 VCL_BOOL VRT_r_req_is_hitmiss(VRT_CTX);
 
 VCL_BOOL VRT_r_req_is_hitpass(VRT_CTX);
@@ -1334,6 +1336,8 @@
 
 void VRT_u_bereq_http(VRT_CTX);
 
+
+
 VCL_BOOL VRT_r_bereq_is_bgfetch(VRT_CTX);
 
 VCL_BOOL VRT_r_bereq_is_hitmiss(VRT_CTX);
@@ -1393,6 +1397,8 @@
 
 void VRT_u_beresp_http(VRT_CTX);
 
+
+
 VCL_DURATION VRT_r_beresp_keep(VRT_CTX);
 void VRT_l_beresp_keep(VRT_CTX, VCL_DURATION);
 
@@ -1462,6 +1468,8 @@
 
 void VRT_u_resp_http(VRT_CTX);
 
+
+
 VCL_BOOL VRT_r_resp_is_streaming(VRT_CTX);
 
 VCL_STRING VRT_r_resp_proto(VRT_CTX);
@@ -3745,6 +3753,8 @@
  * var       DURATION    0 99 bereq.first_byte_timeout
  * var       BLOB        0 99 bereq.hash
  * none      HEADER      0 99 bereq.http*
+ * var       HEADER      0 99 bereq.http.content-length
+ * var       HEADER      0 99 bereq.http.transfer-encoding
  * var       BOOL        0 99 bereq.is_bgfetch
  * var       BOOL        0 99 bereq.is_hitmiss
  * var       BOOL        0 99 bereq.is_hitpass
@@ -3771,10 +3781,12 @@
  * var       DURATION    0 99 beresp.grace
  * none      HEADER      0 99 beresp.http*
  * var       HEADER     41 41 beresp.http.Cache-Control
+ * var       HEADER      0 99 beresp.http.content-length
  * var       HEADER     41 41 beresp.http.Content-Type
  * var       HEADER     41 41 beresp.http.Retry-After
  * var       HEADER     41 41 beresp.http.Set-Cookie
  * var       HEADER     41 41 beresp.http.Surrogate-control
+ * var       HEADER      0 99 beresp.http.transfer-encoding
  * var       HEADER     41 41 beresp.http.Vary
  * var       DURATION    0 99 beresp.keep
  * var       STRING      0 40 beresp.proto
@@ -3830,8 +3842,10 @@
  * var       BOOL        0 99 req.hash_ignore_vary
  * none      HEADER      0 99 req.http*
  * var       HEADER     41 41 req.http.Authorization
+ * var       HEADER      0 99 req.http.content-length
  * var       HEADER     41 41 req.http.Cookie
  * var       HEADER     41 41 req.http.host
+ * var       HEADER      0 99 req.http.transfer-encoding
  * var       BOOL        0 99 req.is_hitmiss
  * var       BOOL        0 99 req.is_hitpass
  * var       STRING      0 99 req.method

This makes me wonder why we leave out resp.http.(content-length|transfer-encoding). Because they get overwritten anyway?

[dridi]

That's because I used beresp twice! Good catch!

[nigoroll]

oooh, I (sh|c)ould have seen that ;)

[dridi]

I force-pushed a clean patch series on top of current master to keep this pull request actionable.

[dridi]

It turns out two test cases were unsetting resp.http.content-length, hence the additional force push.

[dridi]

bugwash: @bsdphk to review or implicitly approve by Thursday.