Hi,

Attaching updated PoC patch, which also reuses contexts from 'http' level.

> -----Original Message-----
> From: Maxim Dounin <mdounin@mdounin.ru>
> Sent: Friday, May 20, 2022 16:23
>
> Hello!
>
> On Fri, May 20, 2022 at 06:52:54AM +0000, Pavel Pautov via nginx-devel wrote:
>
> > > -----Original Message-----
> > > From: Maxim Dounin <mdounin@mdounin.ru>
> > > Sent: Wednesday, May 18, 2022 11:32
> > [..]
> > > > At very least, ngx_http_proxy_set_ssl() needs to be converted
> > > > into ngx_http_proxy_create_ssl().
> > >
> > > You may want to focus on actually making the code more readable
> > > and abstracting it into ngx_http_proxy_set_ssl() instead.
> > > Something like ngx_http_upstream_hide_headers_hash() might be a
> > > good example on how to do it properly.
> >
> > Do you suggest to make ngx_http_proxy_set_ssl() responsible for
> > merging context related settings? I guess, we can do that, but
> > something like ngx_http_proxy_create_ssl() might be still
> > beneficial.
>
> No, I'm suggesting to use ngx_http_proxy_set_ssl() for what it
> currently does: create plcf->upstream.ssl. And extend it to
> inherit plcf->upstream.ssl when possible.

I went with ngx_http_proxy_create_ssl() for now (see patch), as I need to call it twice in some cases.
However, contexts reuse logic can be moved into separate function, as well. Especially if we are going to reuse it across modules.

> > > Also, it should be a good idea to avoid creating SSL contexts if
> > > there is no SSL proxying configured. Or, at very least, make sure
> > > only one context at the http level is used in such cases, so
> > > configurations with many servers won't suddenly blow up.
> >
>
> > The patch shouldn't cause a blow up to my understanding, expect perhaps for
> very specific configs like:
> > server {
> > proxy_ssl_session_reuse off;
> > location / {
> > proxy_ssl_session_reuse on;
> > proxy_pass https://backend;
> > }
> > }
> > Basically, one have to have "proxy_ssl_*" directives scattered
> > across hierarchy of locations with a single terminal "proxy_pass
> > https". If there are many terminal locations with different
> > proxy_passes, then increase shouldn't be that big. If there is
> > no terminal "proxy_pass https", then why are these proxy_ssl_*
> > options even there?
>
> Ah, it looks like I've misread your patch. With the existing
> approach it is not clear how do you expect it to actually fix the
> original problem as in the ticket, that is, a configuration like:
>
> http {
> proxy_ssl_trusted_certificate /etc/pki/tls/cert.pem;
>
> server {
> ...
> location ... { proxy_pass https://...; }
> location ... { proxy_pass https://...; }
> location ... { proxy_pass https://...; }
>
> }
> }
>
> With your patch, no SSL context will be created for the server{}
> block, so each location will have to create its own SSL context
> for proxying: exactly what we are trying to avoid.

Indeed, somehow I've assumed there is going to be a merge into 'http' from some 'null' location.

>
> [...]
>
> > That being said, I think, we can actually satisfy "no SSL
> > contexts without https" requirement by linking location configs
> > and traversing them backwards in a search for location with
> > "proxy_ssl_*" directives. I'd update patch with that approach in
> > mind.
>
> You may want to re-evaluate your understanding of the configuration
> merging process, and re-read the
> ngx_http_upstream_hide_headers_hash() function I've already
> mentioned above.

I did and I still find above a valid technique for achieving "no contexts without https proxy_pass" (not sure though, if it's still a requirement). There could be several layers of locations between "proxy_ssl_*" and proxy_pass and if we don't create context on the way down, we'll have to go up to create it for other nested locations to see it. Alternatively, I guess, we can use something like "ngx_ssl_t **" to have same placeholder for locations which would reuse context, and init it only if we hit "https proxy_pass".

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