> On 13 May 2022, at 05:57, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> Hello!
>
> On Thu, May 12, 2022 at 01:03:37AM +0400, Sergey Kandaurov wrote:
>
>> On Thu, Apr 21, 2022 at 01:18:58AM +0300, Maxim Dounin wrote:
>>> # HG changeset patch
>>> # User Maxim Dounin <mdounin@mdounin.ru>
>>> # Date 1650492341 -10800
>>> # Thu Apr 21 01:05:41 2022 +0300
>>> # Node ID fa9751ffe7723a11159c158078e454671e81cb87
>>> # Parent 2027b85971d4b8a7e33c018548468057cb57eaf7
>>> Upstream: multiple WWW-Authenticate headers (ticket #485).
>>>
>>> When using proxy_intercept_errors and an error page for error 401
>>> (Unauthorized), multiple WWW-Authenticate headers from the upstream server
>>> response are now properly copied to the response.
>>>
>>> diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
>>> --- a/src/http/ngx_http_upstream.c
>>> +++ b/src/http/ngx_http_upstream.c
>>> @@ -2647,7 +2647,7 @@ ngx_http_upstream_intercept_errors(ngx_h
>>> {
>>> ngx_int_t status;
>>> ngx_uint_t i;
>>> - ngx_table_elt_t *h;
>>> + ngx_table_elt_t *h, *ho, **ph;
>>> ngx_http_err_page_t *err_page;
>>> ngx_http_core_loc_conf_t *clcf;
>>>
>>> @@ -2676,18 +2676,26 @@ ngx_http_upstream_intercept_errors(ngx_h
>>> if (status == NGX_HTTP_UNAUTHORIZED
>>> && u->headers_in.www_authenticate)
>>> {
>>> - h = ngx_list_push(&r->headers_out.headers);
>>> -
>>> - if (h == NULL) {
>>> - ngx_http_upstream_finalize_request(r, u,
>>> + h = u->headers_in.www_authenticate;
>>> + ph = &r->headers_out.www_authenticate;
>>> +
>>> + while (h) {
>>> + ho = ngx_list_push(&r->headers_out.headers);
>>> +
>>> + if (ho == NULL) {
>>> + ngx_http_upstream_finalize_request(r, u,
>>> NGX_HTTP_INTERNAL_SERVER_ERROR);
>>> - return NGX_OK;
>>> + return NGX_OK;
>>> + }
>>> +
>>> + *ho = *h;
>>> + ho->next = NULL;
>>> +
>>> + *ph = ho;
>>> + ph = &ho->next;
>>> +
>>> + h = h->next;
>>> }
>>> -
>>> - *h = *u->headers_in.www_authenticate;
>>> - h->next = NULL;
>>> -
>>> - r->headers_out.www_authenticate = h;
>>> }
>>>
>>> #if (NGX_HTTP_CACHE)
>>>
>>
>> While the patch certainly looks correct,
>> I'm not sure about usefulness of r->headers_out.www_authenticate.
>> Is the header accessed directly through this pointer ever?
>> For read purposes I mean. Header filters seem not.
>>
>> The only place is in the auth_request module,
>> but it doesn't seem to make sense in subrequest.
>
> The auth request module uses it to copy the WWW-Authenticate
> headers from the subrequest response to the main request. For
> example, in a configuration like:
>
> location / { auth_request /auth; ... }
> location /auth { auth_basic foo; ... }
>
> While such configurations are unlikely, as using auth_basic
> directly should be easier, these are certainly possible.

I doubt access phase handlers are executed in subrequests at all,
see the "r != r->main" check in ngx_http_core_access_phase().
So it doesn't look feasible with auth_basic (or another auth module).

>
> Similarly, r->headers_out.www_authenticate can be used with
> proxy_intercept_errors + error_page 401 in the subrequest (since
> r->upstream can be cleared by the error page).

That's true, tnx for the point.
Indeed, with such setup, after internal redirect while in subrequest,
headers are already there in (s)r->headers_out.www_authenticate.

>
> BTW, looking at this once again, I tend to think we also need
> ngx_http_core_access_phase() changes for proper handling of
> multiple WWW-Authenticate headers. It might not be a good idea to
> commit it right now though, see commit log.

auth_spnego is such an outstanding example.
As said, with the patch applied, when it comes to clear headers with
"satisfy any;", this leads to segfault on h->next.
Even with h->next initialized, and since the module allows to insert
two headers at once, the first one gets lost tracking to be cleared
(which is also true now).
So, the module needs more work to gain from properly linked headers.

>
> On the other hand, breaking it all at once might be less painful
> (after all, the auth_request change will already made such modules
> to segfault when used with auth_request; though this is unlikely,
> since such configurations hardly make sense).

Still not clear how's that possible in subrequests, see above.

>
> # HG changeset patch
> # User Maxim Dounin <mdounin@mdounin.ru>
> # Date 1652405911 -10800
> # Fri May 13 04:38:31 2022 +0300
> # Node ID 0c2cf997bde390c9be27e0685ee2a00024c57e4e
> # Parent c53cb6f31b9855020d85476fa98ef1cdca809aed
> Multiple WWW-Authenticate headers with "satisfy any;".
>
> If a module adds multiple WWW-Authenticate headers (ticket #485) to the
> response, linked in r->headers_out.www_authenticate, all headers are now
> cleared if another module later allows access.
>
> This change is a nop for standard modules, since the only access module which
> can add multiple WWW-Authenticate headers is the auth request module, and
> it is checked after other standard access modules. Though this might
> affect some third party access modules.
>
> Note that if a 3rd party module adds a single WWW-Authenticate header
> and not yet modified to set the header's next pointer to NULL, attempt to
> clear such a header with this change will result in a segmentation fault.
>
> diff --git a/src/http/ngx_http_core_module.c b/src/http/ngx_http_core_module.c
> --- a/src/http/ngx_http_core_module.c
> +++ b/src/http/ngx_http_core_module.c
> @@ -1088,6 +1088,7 @@ ngx_int_t
> ngx_http_core_access_phase(ngx_http_request_t *r, ngx_http_phase_handler_t *ph)
> {
> ngx_int_t rc;
> + ngx_table_elt_t *h;
> ngx_http_core_loc_conf_t *clcf;
>
> if (r != r->main) {
> @@ -1122,8 +1123,8 @@ ngx_http_core_access_phase(ngx_http_requ
> if (rc == NGX_OK) {
> r->access_code = 0;
>
> - if (r->headers_out.www_authenticate) {
> - r->headers_out.www_authenticate->hash = 0;
> + for (h = r->headers_out.www_authenticate; h; h = h->next) {
> + h->hash = 0;
> }
>
> r->phase_handler = ph->next;
>

I think it is good from an architectural point of view.

--
Sergey Kandaurov

_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org