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.

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

[...]

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

[...]

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