[PATCH 18 of 20] Upstream: multiple WWW-Authenticate headers (ticket #485)

Maxim Dounin mdounin at mdounin.ru
Fri May 20 21:07:27 UTC 2022


Hello!

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

> > On 13 May 2022, at 05:57, Maxim Dounin <mdounin at 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 at 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 at 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/



More information about the nginx-devel mailing list