Hi guys,
On Fri, May 17, 2019 at 09:58:17PM +0200, [email protected] wrote:
> Am Fr., 17. Mai 2019 um 21:15 Uhr schrieb Tim Düsterhus <[email protected]>:
> >
> > Willy,
> >
> > Am 23.12.18 um 21:20 schrieb Moemen MHEDHBI:
> > > Hi,
> > >
> > > The attached patch adds the ssl_sni_check converter which returns true
> > > if the sample input string matches a loaded certificate's CN/SAN.
> > >
> > > This can be useful to check for example if a host header matches a
> > > loaded certificate CN/SAN before doing a redirect:
> > >
> > > frontent fe_main
> > > bind 127.0.0.1:80
> > > bind 127.0.0.1:443 ssl crt /etc/haproxy/ssl/
> > > http-request redirect scheme https if !{ ssl_fc } {
> > > hdr(host),ssl_sni_check() }
> > > This converter may be even more useful when certificates will be
> > > added/removed at runtime.
(...)
> Definitely thumbs up for this converter. I've implemented on-the-fly
> certificate generation for HAProxy with the help of Lua. The converter
> would help me to reduce or simplify parts of the code and could
> possible improve performance.
I vaguely remember having had a quickly look at it by then, and switched
to something else and forgot to respond as usual :-/
So I had another look at it, and there is something slightly wrong in
the concept but actually, fixing it will simplify the patch. What is
wrong is the lookup on a specific listener or any of them. We should
*only* check the listener that was used to accept the connection,
otherwise it opens a security hole. Example :
frontend web
bind 192.0.2.1:443 ssl crt /etc/haproxy/ssl/
bind 127.0.0.1:443 ssl crt /etc/haproxy/insecure-wildcard
You just use a set of dummy wildcard certs on 127.0.0.1 for use with
local tools but you use the real certs on the public access. This
converter will happily match incoming public connections against the
certs available on the other one which is unrelated. The right way to
do it is to compare the certs of the listener which accepted the
connection. Fortunately we do have it :-)
Instead of the loop doing this :
list_for_each_entry(l, &px->conf.listeners, by_fe) {
if ( args->type == ARGT_STR && l->name &&
(strcmp(args->data.str.area, l->name) != 0))
continue;
You simply have to do this and remove the "name" argument :
l = smp->sess->l;
if (!l)
return 0;
Another point is that I don't find the name "ssl_sni_check" very explicit
about the fact that it compares strings with listeners. In fact "ssl_sni_"
tends to make me think it does something with the SNI advertised on the
SSL connection. Shouldn't we use something like "crt-lookup" for example,
since it's really what it does ?
And to go a bit further, is it really valid to lookup a name into a bunch
of certificates which were not involved in the connection ? I don't think
so. If you use a crt-list with some names requiring client authentication
and others not, this will happily validate a host name sent on an
unauthenticated connection because another unrelated cert does support
this name.
I'm starting to think that instead the mechanism should involve the SSL
CTX used on the front connection, and figure from this CTX what names
are permitted. Then the converter should be named like the other ones to
indicate it is related to the front SSL connection. We could have for
example "ssl_fc_name_lookup" or "ssl_fc_sni_lookup". We seem to have
the SSL_CTX available in conn->xprt_ctx->xprt_ctx but I don't know how
we are supposed to retrieve the list of supported names from an SSL_CTX,
we'll probably need some help from Emeric or Manu who have worked much
more on this.
At least we need to be extremely careful here, because doing the wrong
test is much worse than not having the check as it will make users feel
their config is secure while it is not.
Willy
