From: Claudio Jeker Subject: Re: httpd: add custom HTTP header support #2 To: Rafael Sadowski Cc: tech@openbsd.org, kirill@openbsd.org Date: Thu, 16 Jul 2026 08:50:21 +0200 On Thu, Jul 16, 2026 at 07:42:36AM +0200, Rafael Sadowski wrote: > On Wed Jul 15, 2026 at 08:56:27AM +0200, Claudio Jeker wrote: > > On Wed, Jul 15, 2026 at 08:08:51AM +0200, Rafael Sadowski wrote: > > > On Mon Jul 13, 2026 at 02:16:58AM +0200, Kirill A. Korinsky wrote: > > > > On Sat, 11 Jul 2026 16:12:46 +0200, > > > > Rafael Sadowski wrote: > > > > > > > > > > I think this is a solid first version for that feature. We'll certainly > > > > > find out about more edge cases through user feedback ... or maybe not ;) > > > > > > > > > > > > > Here first pass for review. I haven't spent much time on this. I may be wrong. > > > > > > > > > + TAILQ_FOREACH(hdr, &parent_srv->srv_conf.headers, entry) { > > > > > + if (header_exists(srv_conf, hdr->name)) { > > > > > + DPRINTF("%s: skipping header \"%s\" from parent " > > > > > + "\"%s\", overridden in location \"%s\"", > > > > > + __func__, hdr->name, > > > > > + parent_srv->srv_conf.name, srv_conf->location); > > > > > + continue; > > > > > + } > > > > > > > > What happens with duplicated headers? Multiple Set-Cookie is good example. > > > > > > Good catch! > > > > > > Fixed by collecting the inherited headers in a temporary list and > > > TAILQ_CONCAT()ing them afterwards, so config_header_exists() only ever sees > > > headers defined in the location itself. > > > > > > The semantics are unchanged otherwise: if a location mentions a header name at > > > all, it takes over that name completely and none of the parent's headers > > > with that name are inherited. So a "header add "Set-Cookie"" in a > > > location replaces the whole parent group rather than adding to it. The > > > users can use remove if they want to drop individual inherited headers. > > > Documented in httpd.conf(5). > > > > > > > > > > > > @@ -1050,6 +1063,7 @@ server_abort_http(struct client *clt, unsigned int code, const char *msg) > > > > > free(hstsheader); > > > > > free(clenheader); > > > > > free(bannerheader); > > > > > + free(customheaders); > > > > > > > > I think this code is reached only on error, and it makes customheaders leaks > > > > on sucess. > > > > > > > > And next things which I not yet sure. I think that roughly 25 directives > > > > exceed the 16K imsg limit because every one is fixed 664 byte structure nad > > > > it is packed into one message. > > > > > > So true! That's what happens when you follow the code from fastcgi. > > > FastCGI has the same problem. It's actually even worse: > > > > With imsg_set_maxsize(3) you can alter the limit. e.g. bgpd uses a 128k > > imsg size. You can go quite large but keep in mind that the full message > > needs to a) fit in memory and b) needs to be transferred before other > > messages can be processed (the imsgq is a fifo). > > > > > #define HTTPD_FCGI_NAME_MAX 511 > > > #define HTTPD_FCGI_VAL_MAX 511 > > > > > > I fixed the problem by sending the headers one after the other. > > > > Isn't 511 bytes for a header a bit short? e.g. some cookies and JWT tokens > > can be rather large. > > Yes that's true. I upper the limits for header to 256/8192 and not to > touch the IMSG limit for the time being. > > > > > The imsg passing in config_setserver_headers() sends the tailq pointers > > which breaks the fork/exec ASLR protection. I would suggest to use encode > > the headers differently and not use the struct for passing. With that also > > the fixed size string buffers can be changed to a length + str value > > encoding. > > > > You can probably shoehorn this into proc_composev, on the receive side > > just use imsg_get_buf() and imsg_get_strbuf() to pull the data out. > > > > Ohh! Thanks, fixed like this: > > config_setserver_headers() now sends a small fixed struct header_imsg > (id, flags, and the name/value lengths) followed by the raw name and > value bytes via proc_composev() > > As you suggested, the length encoding also let me drop the fixed > name[]/value[] buffers in custom_header. > > If you only interested in review this part, you can find it here > https://rsadowski.gothub.org/?action=diff&commit=daa3667f40735c225a6ba94ebd6bb7bd90a115bc&headref=custom-http-header-support&path=httpd.git > > If we can agree on that, I can and will implement ("fix") it for FastCGI > as well. > > Here is the full diff and another iteration with the following changes: > > httpd: apply "add ... always" headers to internal error responses > > Bring back get_always_custom_headers() to append "add ... always" > headers to those responses. Only "add" is handled here. "set" and > "remove" are ignored for now, so "set ... always" is dropped from the > grammar. Framing headers are already rejected at parse time, so a raw > append cannot desync the response, only produce a duplicate header. > > This is a stopgap. A later change should fold this into > server_custom_headers() so all header operations work the same way on > every response. > > That should be "everything", Looks good to me. Two minor comments. - why is always only supported for add? Why can't I remove or set headers in error conditions? - I would use simple #define for header_flags instead of the enum > +enum header_flags { > + HEADER_REMOVE = 0x01, > + HEADER_ADD = 0x02, > + HEADER_SET = 0x04, > + HEADER_ALWAYS = 0x08 > +}; The enum is never used and using enum values as flags feels wrong to me. -- :wq Claudio