Configuration: made large_header_buffer_size a valid setting.

[ac000]

@JanMikes and @tagur87 on GitHub both reported issues with long URLs that were exceeding the 8192 byte large_header_buffer_size setting, which resulted in a HTTP 431 error (Request Header Fields Too Large).

This can be resolved in the code by updating the following line in src/nxt_router.c::nxt_router_conf_create()

skcf->large_header_buffer_size = 8192;

However, requiring users to modify unit and install custom versions is less than ideal. We could increase the value, but to what?

This commit takes the option of allowing the user to set this option in their config.

large_header_buffer_size is already set by the configuration system in nxt_router.c it just isn't set as a valid config option in nxt_conf_validation.c

With this change users can set this option in their config if required by the following

"settings": {
    "http": {
        "large_header_buffer_size": 16384
    }
},

It retains its default value of 8192 if this is not set.

With this commit, without the above setting or too low a value, with a long URL you get a 431 error. With the above setting set to a large enough value, the request is successful.

Closes: https://github.com/nginx/unit/issues/521 Signed-off-by: Andrew Clayton [email protected]

[lcrilly]

I'd like to consider the name of the configurable. I think we can provide a more concise name for users.

• It's not helpful to present the term "buffer" - this is internal detail.
• "large" is relative and we should not expect the user to know what various RFCs say about it
• Existing settings include header_read_timeout and max_body_size

I propose max_header_size which is already compatible with the default value.

[ac000]

Can do, haven't tried yet, but I think the mapping would happen in the nxt_router_http_conf[] array in src/nxt_router.c, e.g it would become

{                                                                           
    nxt_string("max_header_size"),                                 
    NXT_CONF_MAP_SIZE,                                                      
    offsetof(nxt_socket_conf_t, large_header_buffer_size),                  
},

Do we mind that the setting name and the internal structure member name no longer match? or do we want to change that as well?

[alejandro-colomar]

Can do, haven't tried yet, but I think the mapping would happen in the nxt_router_http_conf[] array in src/nxt_router.c, e.g it would become

{                                                                           
    nxt_string("max_header_size"),                                 
    NXT_CONF_MAP_SIZE,                                                      
    offsetof(nxt_socket_conf_t, large_header_buffer_size),                  
},

Do we mind that the setting name and the internal structure member name no longer match? or do we want to change that as well?

I prefer if they match. Do you think reasonable changing the other side as well?

[ac000]

There's less than half a dozen references to large_header_buffer_size so it's not a big deal to change.

[hongzhidao]

Hi, Take a look at the structure.

typedef struct {
    size_t                 header_buffer_size;
    size_t                 large_header_buffer_size;
    size_t                 large_header_buffers;
    size_t                 body_buffer_size;
    size_t                 max_body_size;
} nxt_socket_conf_t;

We need to think about them together, especially for large_header_buffer_size and large_header_buffers. They should have the same prefix as large_header_. And the source code is for developers, but the configuration is for users, they can't always be the same names.

In my feelings, the current way is better than the below one.

    size_t                 header_buffer_size;
    size_t                 max_headers_size;
    size_t                 large_header_buffers;
    size_t                 body_buffer_size;
    size_t                 max_body_size;

And now it introduced a new setting to make a single header larger. If users want to make the whole request header be large enough, they still can't do it without changing large_header_buffers. Just my two cents.

[alejandro-colomar]

Hi, Take a look at the structure.

[...]

We need to think about them together, especially for large_header_buffer_size and large_header_buffers. They should have the same prefix as large_header_. And the source code is for developers, but the configuration is for users, they can't always be the same names.

In my feelings, the current way is better than the below one.

[...]

That makes sense. Thanks @hongzhidao !

[arjenvri]

Nice work on this one! I am facing a similar issue, can confirm this fix is working on my local custom build. Anything that can be done to merge this one?

[ac000]

@hongzhidao

Hi Zhidao, if you review this and use the above 'Compare' button, ignore the changes to the first couple of files, they are from a different commit and not part of this (the result of me fighting with GitHub). Alternatively look at https://github.com/nginx/unit/pull/771/commits/346a036d724bd6358566b251ca0d4d2ae6a06f91

[ac000]

Changes :-

• Add a changelog entry.

[hongzhidao]

@ac000

1. It's better to keep the order: header, body, etc. Move the related "header" in front of "body" options.

    }, {
        .name       = nxt_string("max_headers_size"),
        .type       = NXT_CONF_VLDT_INTEGER,
    }, {

2. Is "max_header_size" a typo? Not sure which one you prefer, since the above is "max_headers_size".
3. It seems that related test cases are required.

btw, if the patch is ready to review, will you post it into RB?

[ac000]

1. It's better to keep the order: header, body, etc. Move the related "header" in front of "body" options.

Sure. I'll stick it before 'body_buffer_size'.

2. Is "max_header_size" a typo? Not sure which one you prefer, since the above is "max_headers_size".

Ah, in src/nxt_router.c, yes good spot.

3. It seems that related test cases are required.

Hmm, I'll see if I can cook some up.

btw, if the patch is ready to review, will you post it into RB?

I'd prefer to keep it on GH, then everyone can follow along. No need to take it private...

[hongzhidao]

3. It seems that related test cases are required. Hmm, I'll see if I can cook some up.

Btw, it's fine to separate tests into an individual commit, both ways are ok.

[ac000]

Changes :-

• Fixed typo in nxt_router.c
• Re-ordered config setting
• Added a couple of tests

[ac000]

Added review requests for

@andrey-zelenkov to check over the tests. @artemkonev as this will require documentation updates.

[ac000]

Changes :-

• Tweak to the changelog

[ac000]

@andrey-zelenkov When you get a chance, if you could take a quick look over the tests, they're pretty simple. Cheers.

[ac000]

Reverted the first commit back to using 'large_header_buffer_size' as the config option.

Added a new commit that adds 'large_header_buffers' as a valid config option.

[ac000]

Changes :-

• Tweaked the changelog entry for the large_header_buffer_size patch
• Removed the large_header_buffer_size tests (@andrey-zelenkov is doing a separate tests patch)