Hello!

On Fri, May 20, 2022 at 05:51:19PM +0400, Sergey Kandaurov wrote:

> > On 13 May 2022, at 05:57, Maxim Dounin <mdounin@mdounin.ru> wrote:
> >
> > 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).

Yes, you're right, forgot about this check. So it's probably only
proxying then.

> > 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.

Yes, looks like a good example. So, the module doesn't work
correctly with "satisfy any;" now, and it cannot be fixed to work
properly. With the patch, it will be possible to fix the module
to work correctly.

> > 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.

I think you are right and it's not currently possible, see above.
Yet I still tend to think that committing this with other changes
would the right thing to do.

> > # 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.

Yep, sure. And I tend to think it worth committing, so it will be
clear why and how modules adding WWW-Authenticate have to be
modified. Added this to the queue.

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org