The reason I think auth_request_module should be able to send the auth response body

Davood Falahati 0x0davood at gmail.com
Tue May 9 13:19:53 UTC 2023


Good day Maxim,
thanks for your comprehensive response and sorry for breaking some
conventions in this mailing list because I'm a real novice here.

I would top post your points and try to answer them as far as I can.

> 1. The ctx->subrequest->out is only available when there is a
   NGX_HTTP_SUBREQUEST_IN_MEMORY flag (and implies various
   restrictions).
I am not aware of the side effects and restrictions it might incur. From
the documentation,
<http://nginx.org/en/docs/dev/development_guide.html#http_subrequests> it
reads:

>> NGX_HTTP_SUBREQUEST_IN_MEMORY - Output is not sent to the client, but
rather stored in memory. The flag only affects subrequests which are
processed by one of the proxying modules. After a subrequest is finalized
its output is available in r->out of type ngx_buf_t.


> 2. The auth subrequest is created with the sr->header_only flag
   set, so the will be no response body available in at all.
a rooky fault from my side. I think I have sent to emails in which one of
them had sr->header_only set. In another email, I removed it. Thus, I can
expect response body set in r->out buffer.

> Futher, it might not be a good idea to copy all headers from the
subrequest while not providing various links and pointers from the
r->headers_out structure.  This is going to break various filter
modules, such as charset filter (which uses
r->headers_out.charset, r->headers_out.override_charset,
r->headers_out.content_encoding), sub filter (as testing content
type uses r->headers_out.content_type_len), and many more things.

good point, I wasn't aware of that and I would try to do it differently.

> Note well that "enable" isn't a good name for a field responsible
for an optional feature.  Similarly, "send_auth_body" does not
look self-explanatory.

Agree. Will come up with more clear naming.

> Overall, please also take a look at
http://nginx.org/en/docs/contributing_changes.html for some basic
hints on how to submit patches.

Indeed, sorry for some pedantic mistakes.

> Most notably, it might be a good idea outline the use case for the
feature you are trying to introduce and why existing features are
not enough for this use case.  The design of the module generally
suggests that the custom response body, if needed, can be provided
using the error_page directive, much like with other auth modules.

The reason I ended up in this patch was a problem we had in our company. We
started using  auth_request_module and it was working as expected, but
suddenly, we noticed our frontend which handles the error messages, fails
when the requests are not being successfully authenticated. In other words,
we needed our frontend not to break its error handling routine when it
receives a 401 or 403. For example, a user wants to see a resource but it
has no proper role to do so. We want to show a message expressing the
problem using our existing error handling mechanisms.
Using custom error_page was not the solution for us. Our proxied endpoints
should have changed a lot to set error responses into headers and it was a
hefty, breaking workload for us and we decided not to go with it.
Then I decided to modify the auth_request_module to make it transparent
about the auth service error messages in case the implementers want it and
they take the risk of showing those error messages.
I think adding a flag to let the auth modules be pass-through would be a
nice feature for the great Nginx.

I hope I was informative enough,
Davood Falahati


On Tue, May 9, 2023 at 12:11 PM <nginx-devel-request at nginx.org> wrote:

> Send nginx-devel mailing list submissions to
>         nginx-devel at nginx.org
>
> To subscribe or unsubscribe via the World Wide Web, visit
>         https://mailman.nginx.org/mailman/listinfo/nginx-devel
> or, via email, send a message with subject or body 'help' to
>         nginx-devel-request at nginx.org
>
> You can reach the person managing the list at
>         nginx-devel-owner at nginx.org
>
> When replying, please edit your Subject line so it is more specific
> than "Re: Contents of nginx-devel digest..."
>
>
> Today's Topics:
>
>    1. keep response body of the subrequest inside the memory and
>       use it if send_auth_body is set (Davood Falahati)
>    2. Re: enable request_auth module to send auth service error
>       message body when it is allowed (Maxim Dounin)
>    3. Re: keep response body of the subrequest inside the memory
>       and use it if send_auth_body is set (Maxim Dounin)
>    4. Re: [PATCH 1 of 3] QUIC: changed path validation timeout
>       (Sergey Kandaurov)
>
>
> ----------------------------------------------------------------------
>
> Message: 1
> Date: Tue, 9 May 2023 02:45:36 +0200
> From: Davood Falahati <0x0davood at gmail.com>
> To: nginx-devel at nginx.org
> Subject: keep response body of the subrequest inside the memory and
>         use it if send_auth_body is set
> Message-ID:
>         <
> CADTHY-HSfhTynDi3h2ZF6Kx+9LW0Nf9fcG6uuoasWrxuh9H1Aw at mail.gmail.com>
> Content-Type: text/plain; charset="utf-8"
>
> # HG changeset patch
> # User Davood Falahati <0x0davood at gmail.com>
> # Date 1683593026 -7200
> #      Tue May 09 02:43:46 2023 +0200
> # Node ID 1053357966cda6a0902b748a9b4b8a214b36ccd4
> # Parent  b71e69247483631bd8fc79a47cc32b762625b1fb
> keep response body of the subrequest inside the memory and use it if
> send_auth_body is set
>
> diff -r b71e69247483 -r 1053357966cd
> src/http/modules/ngx_http_auth_request_module.c
> --- a/src/http/modules/ngx_http_auth_request_module.c Mon May 01 19:16:05
> 2023 +0400
> +++ b/src/http/modules/ngx_http_auth_request_module.c Tue May 09 02:43:46
> 2023 +0200
> @@ -13,6 +13,7 @@
>  typedef struct {
>      ngx_str_t                 uri;
>      ngx_array_t              *vars;
> +    ngx_flag_t                enable;
>  } ngx_http_auth_request_conf_t;
>
>
> @@ -62,6 +63,12 @@
>        NGX_HTTP_LOC_CONF_OFFSET,
>        0,
>        NULL },
> +    { ngx_string("send_auth_body"),
> +      NGX_HTTP_MAIN_CONF | NGX_HTTP_SRV_CONF | NGX_HTTP_LOC_CONF |
> NGX_CONF_TAKE1,
> +      ngx_conf_set_flag_slot,
> +      NGX_HTTP_LOC_CONF_OFFSET,
> +      offsetof(ngx_http_auth_request_conf_t, enable),
> +      NULL },
>
>        ngx_null_command
>  };
> @@ -106,6 +113,9 @@
>      ngx_http_post_subrequest_t    *ps;
>      ngx_http_auth_request_ctx_t   *ctx;
>      ngx_http_auth_request_conf_t  *arcf;
> +    ngx_list_t *hs;
> +    ngx_buf_t *b;
> +    ngx_chain_t out, *in;
>
>      arcf = ngx_http_get_module_loc_conf(r, ngx_http_auth_request_module);
>
> @@ -141,6 +151,36 @@
>          if (ctx->status == NGX_HTTP_UNAUTHORIZED) {
>              sr = ctx->subrequest;
>
> +            if (arcf->enable) {
> +
> +                r->headers_out.content_type =
> sr->headers_out.content_type;
> +
> +                hs = &sr->headers_out.headers;
> +
> +                r->headers_out.headers = *hs;
> +
> +                b = ngx_calloc_buf(r->pool);
> +                if (b == NULL) {
> +                   return NGX_ERROR;
> +                }
> +
> +                r->headers_out.status = ctx->status;
> +
> +                b->last_buf = 1;
> +                b->last_in_chain = 1;
> +                b->memory = 1;
> +
> +                out.buf = b;
> +                out.next = NULL;
> +
> +                in = sr->out;
> +                in->next = &out;
> +
> +                ngx_http_send_header(r);
> +
> +                return ngx_http_output_filter(r, in);
> +            }
> +
>              h = sr->headers_out.www_authenticate;
>
>              if (!h && sr->upstream) {
> @@ -191,9 +231,12 @@
>
>      ps->handler = ngx_http_auth_request_done;
>      ps->data = ctx;
> -
> +    /*
> +    * response body is being kept in memory and client won't receive it
> +    * use subrequest->out to access the chain buffer
> +    */
>      if (ngx_http_subrequest(r, &arcf->uri, NULL, &sr, ps,
> -                            NGX_HTTP_SUBREQUEST_WAITED)
> +                            NGX_HTTP_SUBREQUEST_IN_MEMORY)
>          != NGX_OK)
>      {
>          return NGX_ERROR;
> @@ -209,8 +252,6 @@
>          return NGX_ERROR;
>      }
>
> -    sr->header_only = 1;
> -
>      ctx->subrequest = sr;
>
>      ngx_http_set_ctx(r, ctx, ngx_http_auth_request_module);
> @@ -323,6 +364,8 @@
>
>      conf->vars = NGX_CONF_UNSET_PTR;
>
> +    conf->enable = NGX_CONF_UNSET;
> +
>      return conf;
>  }
>
> @@ -335,6 +378,7 @@
>
>      ngx_conf_merge_str_value(conf->uri, prev->uri, "");
>      ngx_conf_merge_ptr_value(conf->vars, prev->vars, NULL);
> +    ngx_conf_merge_value(conf->enable, prev->enable, 0);
>
>      return NGX_CONF_OK;
>  }
> -------------- next part --------------
> An HTML attachment was scrubbed...
> URL: <
> http://mailman.nginx.org/pipermail/nginx-devel/attachments/20230509/ea8a5325/attachment-0001.htm
> >
>
> ------------------------------
>
> Message: 2
> Date: Tue, 9 May 2023 03:52:10 +0300
> From: Maxim Dounin <mdounin at mdounin.ru>
> To: nginx-devel at nginx.org
> Subject: Re: enable request_auth module to send auth service error
>         message body when it is allowed
> Message-ID: <ZFmZOlEI44Mtn0Z/@mdounin.ru>
> Content-Type: text/plain; charset=us-ascii
>
> Hello!
>
> On Tue, May 09, 2023 at 01:40:18AM +0200, Davood Falahati wrote:
>
> > # HG changeset patch
> > # User Davood Falahati <0x0davood at gmail.com>
> > # Date 1683588448 -7200
> > #      Tue May 09 01:27:28 2023 +0200
> > # Node ID 0977f155bc2d288eedf006033b9a5094d0e8098f
> > # Parent  b71e69247483631bd8fc79a47cc32b762625b1fb
> > let request_auth_module pass auth body when it is allowed
> >
> > diff -r b71e69247483 -r 0977f155bc2d
> > src/http/modules/ngx_http_auth_request_module.c
> > --- a/src/http/modules/ngx_http_auth_request_module.c Mon May 01 19:16:05
> > 2023 +0400
> > +++ b/src/http/modules/ngx_http_auth_request_module.c Tue May 09 01:27:28
> > 2023 +0200
> > @@ -13,6 +13,7 @@
> >  typedef struct {
> >      ngx_str_t                 uri;
> >      ngx_array_t              *vars;
> > +    ngx_flag_t                enable;
> >  } ngx_http_auth_request_conf_t;
> >
> >
> > @@ -62,6 +63,12 @@
> >        NGX_HTTP_LOC_CONF_OFFSET,
> >        0,
> >        NULL },
> > +    { ngx_string("send_auth_body"),
> > +      NGX_HTTP_MAIN_CONF | NGX_HTTP_SRV_CONF | NGX_HTTP_LOC_CONF |
> > NGX_CONF_TAKE1,
> > +      ngx_conf_set_flag_slot,
> > +      NGX_HTTP_LOC_CONF_OFFSET,
> > +      offsetof(ngx_http_auth_request_conf_t, enable),
> > +      NULL },
> >
> >        ngx_null_command
> >  };
> > @@ -106,6 +113,9 @@
> >      ngx_http_post_subrequest_t    *ps;
> >      ngx_http_auth_request_ctx_t   *ctx;
> >      ngx_http_auth_request_conf_t  *arcf;
> > +    ngx_list_t *hs;
> > +    ngx_buf_t *b;
> > +    ngx_chain_t out, *in;
> >
> >      arcf = ngx_http_get_module_loc_conf(r,
> ngx_http_auth_request_module);
> >
> > @@ -141,6 +151,36 @@
> >          if (ctx->status == NGX_HTTP_UNAUTHORIZED) {
> >              sr = ctx->subrequest;
> >
> > +            if (arcf->enable) {
> > +
> > +                r->headers_out.content_type =
> sr->headers_out.content_type;
> > +
> > +                hs = &sr->headers_out.headers;
> > +
> > +                r->headers_out.headers = *hs;
> > +
> > +                b = ngx_calloc_buf(r->pool);
> > +                if (b == NULL) {
> > +                   return NGX_ERROR;
> > +                }
> > +
> > +                r->headers_out.status = ctx->status;
> > +
> > +                b->last_buf = 1;
> > +                b->last_in_chain = 1;
> > +                b->memory = 1;
> > +
> > +                out.buf = b;
> > +                out.next = NULL;
> > +
> > +                in = ctx->subrequest->out;
> > +                in->next = &out;
> > +
> > +                ngx_http_send_header(r);
> > +
> > +                return ngx_http_output_filter(r, in);
> > +            }
> > +
> >              h = sr->headers_out.www_authenticate;
> >
> >              if (!h && sr->upstream) {
> > @@ -323,6 +363,8 @@
> >
> >      conf->vars = NGX_CONF_UNSET_PTR;
> >
> > +    conf->enable = NGX_CONF_UNSET;
> > +
> >      return conf;
> >  }
> >
> > @@ -335,6 +377,7 @@
> >
> >      ngx_conf_merge_str_value(conf->uri, prev->uri, "");
> >      ngx_conf_merge_ptr_value(conf->vars, prev->vars, NULL);
> > +    ngx_conf_merge_value(conf->enable, prev->enable, 0);
> >
> >      return NGX_CONF_OK;
> >  }
>
> Thanks for the patch.  It is, however, is not going to work for at
> least two reasons:
>
> 1. The ctx->subrequest->out is only available when there is a
>    NGX_HTTP_SUBREQUEST_IN_MEMORY flag (and implies various
>    restrictions).
>
> 2. The auth subrequst is created with the sr->header_only flag
>    set, so the will be no response body available in at all.
>
> Futher, it might not be a good idea to copy all headers from the
> subrequest while not providing various links and pointers from the
> r->headers_out structure.  This is going to break various filter
> modules, such as charset filter (which uses
> r->headers_out.charset, r->headers_out.override_charset,
> r->headers_out.content_encoding), sub filter (as testing content
> type uses r->headers_out.content_type_len), and many more things.
>
> Note well that "enable" isn't a good name for a field responsible
> for an optional feature.  Similarly, "send_auth_body" does not
> look self-explanatory.
>
> Overall, please also take a look at
> http://nginx.org/en/docs/contributing_changes.html for some basic
> hints on how to submit patches.
>
> Most notably, it might be a good idea outline the use case for the
> feature you are trying to introduce and why existing features are
> not enough for this use case.  The design of the module generally
> suggests that the custom response body, if needed, can be provided
> using the error_page directive, much like with other auth modules.
>
> Hope this helps.
>
> --
> Maxim Dounin
> http://mdounin.ru/
>
> ------------------------------
>
> Message: 3
> Date: Tue, 9 May 2023 03:58:56 +0300
> From: Maxim Dounin <mdounin at mdounin.ru>
> To: nginx-devel at nginx.org
> Subject: Re: keep response body of the subrequest inside the memory
>         and use it if send_auth_body is set
> Message-ID: <ZFma0Doug+au39id at mdounin.ru>
> Content-Type: text/plain; charset=us-ascii
>
> Hello!
>
> On Tue, May 09, 2023 at 02:45:36AM +0200, Davood Falahati wrote:
>
> > # HG changeset patch
> > # User Davood Falahati <0x0davood at gmail.com>
> > # Date 1683593026 -7200
> > #      Tue May 09 02:43:46 2023 +0200
> > # Node ID 1053357966cda6a0902b748a9b4b8a214b36ccd4
> > # Parent  b71e69247483631bd8fc79a47cc32b762625b1fb
> > keep response body of the subrequest inside the memory and use it if
> > send_auth_body is set
>
> Please see the response to your previous patch.
>
> Please also note that when sending updated versions of a patch, it
> is usually a good idea to make sure they are properly threaded.
> When using hg email, use "--in-reply-to <msgid>" to ensure correct
> threading.
>
> [...]
>
> --
> Maxim Dounin
> http://mdounin.ru/
>
> ------------------------------
>
> Message: 4
> Date: Tue, 9 May 2023 14:11:43 +0400
> From: Sergey Kandaurov <pluknet at nginx.com>
> To: nginx-devel at nginx.org
> Subject: Re: [PATCH 1 of 3] QUIC: changed path validation timeout
> Message-ID: <0BC9C2FA-22ED-401F-8E12-D95D45F0B2E5 at nginx.com>
> Content-Type: text/plain;       charset=us-ascii
>
>
> > On 3 May 2023, at 17:00, Roman Arutyunyan <arut at nginx.com> wrote:
> >
> > Hi,
> >
> > On Mon, Apr 24, 2023 at 04:15:21PM +0400, Sergey Kandaurov wrote:
> >>
> >>
> >>> On 28 Mar 2023, at 18:51, Roman Arutyunyan <arut at nginx.com> wrote:
> >>>
> >>> # HG changeset patch
> >>> # User Roman Arutyunyan <arut at nginx.com>
> >>> # Date 1679925333 -14400
> >>> #      Mon Mar 27 17:55:33 2023 +0400
> >>> # Branch quic
> >>> # Node ID f76e83412133085a6c82fce2c3e15b2c34a6e959
> >>> # Parent  5fd628b89bb7fb5c95afa1dc914385f7ab79f6a3
> >>> QUIC: changed path validation timeout.
> >>>
> >>> Path validation packets containing PATH_CHALLENGE frames are sent
> separately
> >>> from regular frame queue, because of the need to use a decicated path
> and
> >>> pad the packets.  The packets are also resent separately from the
> regular
> >>> probe/lost detection mechanism.  A path validation packet is resent 3
> times,
> >>> each time after PTO expiration.  Assuming constant PTO, the overall
> maximum
> >>> waiting time is 3 * PTO.  According to RFC 9000, 8.2.4. Failed Path
> Validation,
> >>> the following value is recommended as a validation timeout:
> >>>
> >>> A value of three times the larger of the current PTO
> >>> or the PTO for the new path (using kInitialRtt, as
> >>> defined in [QUIC-RECOVERY]) is RECOMMENDED.
> >>>
> >>> The change adds PTO of the new path to the equation as the lower bound.
> >>> Also, max_ack_delay is now always accounted for, unlike previously,
> when
> >>> it was only used when there are packets in flight.  As mentioned
> before,
> >>> PACH_CHALLENGE is not considered in-flight by nginx since it's
> processed
> >>> separately, but technically it is.
> >>
> >> I don't like an idea to make a separate function to calculate
> >> time for path validation retransmits.  It looks like an existing
> >> function could be reused.
> >>
> >> I tend to think checking for inflight packets in ngx_quic_pto()
> >> isn't correct at the first place.  The condition comes from
> >> the GetPtoTimeAndSpace example in 9002, A.8:
> >>
> >> : GetPtoTimeAndSpace():
> >> :   duration = (smoothed_rtt + max(4 * rttvar, kGranularity))
> >> :       * (2 ^ pto_count)
> >> :   // Anti-deadlock PTO starts from the current time
> >> :   if (no ack-eliciting packets in flight):
> >> :     assert(!PeerCompletedAddressValidation())
> >> :     if (has handshake keys):
> >> :       return (now() + duration), Handshake
> >> :     else:
> >> :       return (now() + duration), Initial
> >> :   <..>
> >> :   return pto_timeout, pto_space
> >>
> >> But PeerCompletedAddressValidation is always true for the server.
> >> The above anti-deadlock measure seems to only make sense for a client
> >> when it has no new data to send, but forced to send something to rise
> >> an anti-amplification limit for the server.  This thought is supported
> >> by commentaries in places of GetPtoTimeAndSpace use.
> >>
> >> Removing the condition from ngx_quic_pto() makes possible to unify
> >> the function to use it for both regular PTO and path validation.
> >>
> >> Next is to make retransmits similar to a new connection establishment.
> >> Per RFC 9000, 8.2.1:
> >> : An endpoint SHOULD NOT probe a new path with packets containing a
> >> : PATH_CHALLENGE frame more frequently than it would send an Initial
> packet.
> >>
> >> I think we can improve path validation to use a separate backoff,
> >> path->tries can be used to base a backoff upon it.
> >>
> >> Since PATH_CHALLENGE are resent separately from the regular probe/lost
> >> detection mechanism, this needs to be moved out from ngx_quic_pto().
> >>
> >> This makes the following series based on your patch.
> >> We could set an overall maximum waiting time of 3 * PTO and test it
> >> in pv handler in addition to the check for NGX_QUIC_PATH_RETRIES.
> >
> > Jftr, discussed all of the above in person.  Agreed to implement that.
> >
> >> [..]
> >> # HG changeset patch
> >> # User Sergey Kandaurov <pluknet at nginx.com>
> >> # Date 1682338151 -14400
> >> #      Mon Apr 24 16:09:11 2023 +0400
> >> # Branch quic
> >> # Node ID 808fe808e276496a9b026690c141201720744ab3
> >> # Parent  f49aba6e3fb54843d3e3bd5df26dbb45f5d3d687
> >> QUIC: separated path validation retransmit backoff.
> >>
> >> Path validation packets containing PATH_CHALLENGE frames are sent
> separately
> >> from regular frame queue, because of the need to use a decicated path
> and pad
> >> the packets.  The packets are sent periodically, separately from the
> regular
> >> probe/lost detection mechanism.  A path validation packet is resent up
> to 3
> >> times, each time after PTO expiration, with increasing per-path PTO
> backoff.
> >>
> >> diff --git a/src/event/quic/ngx_event_quic_ack.c
> b/src/event/quic/ngx_event_quic_ack.c
> >> --- a/src/event/quic/ngx_event_quic_ack.c
> >> +++ b/src/event/quic/ngx_event_quic_ack.c
> >> @@ -736,7 +736,8 @@ ngx_quic_set_lost_timer(ngx_connection_t
> >>
> >>         q = ngx_queue_last(&ctx->sent);
> >>         f = ngx_queue_data(q, ngx_quic_frame_t, queue);
> >> -        w = (ngx_msec_int_t) (f->last + ngx_quic_pto(c, ctx) - now);
> >> +        w = (ngx_msec_int_t) (f->last + (ngx_quic_pto(c, ctx) <<
> qc->pto_count)
> >> +                              - now);
> >>
> >>         if (w < 0) {
> >>             w = 0;
> >> @@ -785,10 +786,9 @@ ngx_quic_pto(ngx_connection_t *c, ngx_qu
> >>
> >>     duration = qc->avg_rtt;
> >>     duration += ngx_max(4 * qc->rttvar, NGX_QUIC_TIME_GRANULARITY);
> >> -    duration <<= qc->pto_count;
> >>
> >>     if (ctx->level == ssl_encryption_application && c->ssl->handshaked)
> {
> >> -        duration += qc->ctp.max_ack_delay << qc->pto_count;
> >> +        duration += qc->ctp.max_ack_delay;
> >>     }
> >>
> >>     return duration;
> >> @@ -846,7 +846,9 @@ ngx_quic_pto_handler(ngx_event_t *ev)
> >>             continue;
> >>         }
> >>
> >> -        if ((ngx_msec_int_t) (f->last + ngx_quic_pto(c, ctx) - now) >
> 0) {
> >> +        if ((ngx_msec_int_t) (f->last + (ngx_quic_pto(c, ctx) <<
> qc->pto_count)
> >> +                              - now) > 0)
> >> +        {
> >>             continue;
> >>         }
> >>
> >> diff --git a/src/event/quic/ngx_event_quic_migration.c
> b/src/event/quic/ngx_event_quic_migration.c
> >> --- a/src/event/quic/ngx_event_quic_migration.c
> >> +++ b/src/event/quic/ngx_event_quic_migration.c
> >> @@ -496,6 +496,7 @@ ngx_quic_validate_path(ngx_connection_t
> >>                    "quic initiated validation of path seq:%uL",
> path->seqnum);
> >>
> >>     path->validating = 1;
> >> +    path->tries = 0;
> >>
> >>     if (RAND_bytes(path->challenge1, 8) != 1) {
> >>         return NGX_ERROR;
> >> @@ -513,7 +514,6 @@ ngx_quic_validate_path(ngx_connection_t
> >>     pto = ngx_quic_pto(c, ctx);
> >>
> >>     path->expires = ngx_current_msec + pto;
> >> -    path->tries = NGX_QUIC_PATH_RETRIES;
> >>
> >>     if (!qc->path_validation.timer_set) {
> >>         ngx_add_timer(&qc->path_validation, pto);
> >> @@ -578,7 +578,6 @@ ngx_quic_path_validation_handler(ngx_eve
> >>     qc = ngx_quic_get_connection(c);
> >>
> >>     ctx = ngx_quic_get_send_ctx(qc, ssl_encryption_application);
> >> -    pto = ngx_quic_pto(c, ctx);
> >>
> >>     next = -1;
> >>     now = ngx_current_msec;
> >> @@ -605,7 +604,9 @@ ngx_quic_path_validation_handler(ngx_eve
> >>             continue;
> >>         }
> >>
> >> -        if (--path->tries) {
> >> +        if (++path->tries < NGX_QUIC_PATH_RETRIES) {
> >> +            pto = ngx_quic_pto(c, ctx) << path->tries;
> >
> > Here we schedule a timer for 2 * PTO or 4 * PTO.  Technically, our path
> > validation code allows several paths to be validated at the same time.
> If that
> > happens, 2 * PTO and 4 * PTO will be too much for the first attempt for
> the new
> > path, where the timeout is just PTO.  As a result, the last path may
> wait 2x
> > or 4x longer than needed.
>
> Agree it needs to be addressed.
>
> Below is the first glance to set the next available pv timer:
> 1) in ngx_quic_validate_path(), a new path may own a shorter timer
> than already established, especially after several timeouts
> Previously, new path validation could be scheduled late.
> 2) in ngx_quic_handle_path_response_frame(), a validated path
> might left a spurious timer, while nothing more to validate
> or remaining paths have more distant timer
> This one is less severe, just extra work added.
>
> Unlike the two above, pv handler has a more complex
> logic I decided not to touch it.
>
> # HG changeset patch
> # User Sergey Kandaurov <pluknet at nginx.com>
> # Date 1683626923 -14400
> #      Tue May 09 14:08:43 2023 +0400
> # Branch quic
> # Node ID 90f3e839532d899b09967cb2db3b3de30484c484
> # Parent  c9a6960c548501b5234ca7f7ff5ae23fad75b023
> QUIC: reschedule path validation on path insertion/removal.
>
> Two issues fixed:
> - new path validation could be scheduled late
> - a validated path could leave a spurious timer
>
> diff --git a/src/event/quic/ngx_event_quic_migration.c
> b/src/event/quic/ngx_event_quic_migration.c
> --- a/src/event/quic/ngx_event_quic_migration.c
> +++ b/src/event/quic/ngx_event_quic_migration.c
> @@ -16,6 +16,7 @@ static ngx_int_t ngx_quic_validate_path(
>      ngx_quic_path_t *path);
>  static ngx_int_t ngx_quic_send_path_challenge(ngx_connection_t *c,
>      ngx_quic_path_t *path);
> +static ngx_msec_int_t ngx_quic_next_pv_timer(ngx_connection_t *c);
>  static ngx_quic_path_t *ngx_quic_get_path(ngx_connection_t *c, ngx_uint_t
> tag);
>
>
> @@ -78,6 +79,7 @@ ngx_quic_handle_path_response_frame(ngx_
>  {
>      ngx_uint_t              rst;
>      ngx_queue_t            *q;
> +    ngx_msec_int_t          next;
>      ngx_quic_path_t        *path, *prev;
>      ngx_quic_connection_t  *qc;
>
> @@ -169,6 +171,17 @@ valid:
>      path->validating = 0;
>      path->limited = 0;
>
> +    /* reschedule if validated path owned next timer */
> +
> +    next = ngx_quic_next_pv_timer(c);
> +
> +    if (next == -1) {
> +        ngx_del_timer(&qc->path_validation);
> +
> +    } else if (next > (ngx_msec_int_t) (path->expires -
> ngx_current_msec)) {
> +        ngx_add_timer(&qc->path_validation, next);
> +    }
> +
>      return NGX_OK;
>  }
>
> @@ -486,7 +499,7 @@ ngx_quic_handle_migration(ngx_connection
>  static ngx_int_t
>  ngx_quic_validate_path(ngx_connection_t *c, ngx_quic_path_t *path)
>  {
> -    ngx_msec_t              pto;
> +    ngx_msec_t              pto, next;
>      ngx_quic_send_ctx_t    *ctx;
>      ngx_quic_connection_t  *qc;
>
> @@ -517,6 +530,15 @@ ngx_quic_validate_path(ngx_connection_t
>
>      if (!qc->path_validation.timer_set) {
>          ngx_add_timer(&qc->path_validation, pto);
> +        return NGX_OK;
> +    }
> +
> +    /* reschedule if new path owns next timer */
> +
> +    next = ngx_quic_next_pv_timer(c);
> +
> +    if (next == pto) {
> +        ngx_add_timer(&qc->path_validation, next);
>      }
>
>      return NGX_OK;
> @@ -563,6 +585,41 @@ ngx_quic_send_path_challenge(ngx_connect
>  }
>
>
> +static ngx_msec_int_t
> +ngx_quic_next_pv_timer(ngx_connection_t *c)
> +{
> +    ngx_msec_t              now;
> +    ngx_queue_t            *q;
> +    ngx_msec_int_t          left, next;
> +    ngx_quic_path_t        *path;
> +    ngx_quic_connection_t  *qc;
> +
> +    qc = ngx_quic_get_connection(c);
> +
> +    now = ngx_current_msec;
> +    next = -1;
> +
> +    for (q = ngx_queue_head(&qc->paths);
> +         q != ngx_queue_sentinel(&qc->paths);
> +         q = ngx_queue_next(q))
> +    {
> +        path = ngx_queue_data(q, ngx_quic_path_t, queue);
> +
> +        if (!path->validating) {
> +            continue;
> +        }
> +
> +        left = path->expires - now;
> +
> +        if (next == -1 || left < next) {
> +            next = left;
> +        }
> +    }
> +
> +    return next;
> +}
> +
> +
>  void
>  ngx_quic_path_validation_handler(ngx_event_t *ev)
>  {
>
>
> >  Luckily, this is not possible, because when we start
> > validation of a path, the validation for the old path is stopped.
>
> As internally discussed, it is still possible on apparent migration,
> where path validation starts on a previous active and now active paths.
>
> >  This
> > allows us to simplify the PATH_RESPONSE and timeout handlers to only
> handle
> > qc->path.  I suggest to add a patch for this.
>
> Hence, this wont work if client validates backup path (which is not
> qc->path).
>
> >  It will also affect PMTUD,
> > since the same handler will be responsible for MTU packets.  Also, it's
> > probably a good idea to suspend PMTUD for the old path when we switch to
> a
> > new one.  This will happend naturally if we support only one path in
> those
> > handlers.
> >
>
> --
> Sergey Kandaurov
>
>
> ------------------------------
>
> Subject: Digest Footer
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel
>
>
> ------------------------------
>
> End of nginx-devel Digest, Vol 153, Issue 16
> ********************************************
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20230509/161c0fb4/attachment-0001.htm>


More information about the nginx-devel mailing list