Re: [PATCH] MINOR: sample: add ssl_sni_check converter
Hi guys,
On Fri, May 17, 2019 at 09:58:17PM +0200, bjun...@gmail.com wrote:
> Am Fr., 17. Mai 2019 um 21:15 Uhr schrieb Tim Düsterhus :
> >
> > 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, >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
Re: [PATCH] MINOR: sample: add ssl_sni_check converter
Am Fr., 17. Mai 2019 um 21:15 Uhr schrieb Tim Düsterhus :
>
> 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.
> >
>
> This email serves to bump the patch which appears to have slipped
> through the cracks. For the context see the "Re: Host header and sni
> extension differ" thread.
>
> Best regards
> Tim Düsterhus
>
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.
Best regards / Mit freundlichen Grüßen
Bjoern
Re: [PATCH] MINOR: sample: add ssl_sni_check converter
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.
>
This email serves to bump the patch which appears to have slipped
through the cracks. For the context see the "Re: Host header and sni
extension differ" thread.
Best regards
Tim Düsterhus
[PATCH] MINOR: sample: add ssl_sni_check converter
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.
++
--
Moemen MHEDHBI
>From 14ed628ab9badbb06c45bab324eb00f998de49af Mon Sep 17 00:00:00 2001
From: Moemen MHEDHBI
Date: Sun, 23 Dec 2018 20:50:04 +0100
Subject: [PATCH] MINOR: sample: add ssl_sni_check converter
This adds the ssl_sni_check converter. The converter returns
true if the sample input string matches a loaded certificate's CN/SAN.
Lookup can be done through certificates of a specified bind line (by
) otherwise the search will include all bind lines of the current
proxy.
---
doc/configuration.txt | 6 ++
src/ssl_sock.c| 43 +++
2 files changed, 49 insertions(+)
diff --git a/doc/configuration.txt b/doc/configuration.txt
index 6ca63d64a..0be043e73 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -13651,6 +13651,12 @@ sha1
Converts a binary input sample to a SHA1 digest. The result is a binary
sample with length of 20 bytes.
+ssl_sni_check()
+ Returns true if the sample input string matches a loaded certificate's CN/SAN.
+ Otherwise false is returned. When is provided the lookup is done only
+ through the certificates of the bind line named , if not all bind
+ lines of the current frontend will be searched.
+
strcmp()
Compares the contents of with the input value of type string. Returns
the result as a signed integer compatible with strcmp(3): 0 if both strings
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 282b85ddd..b24d78978 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -7276,6 +7276,41 @@ smp_fetch_ssl_c_verify(const struct arg *args, struct sample *smp, const char *k
return 1;
}
+/* boolean, returns true if input string matches a loaded certificate's CN/SAN. */
+/* The lookup is done only for the bind named if the param is prvided. */
+static int smp_conv_ssl_sni_check(const struct arg *args, struct sample *smp, void *private)
+{
+ struct proxy *px = smp->px;
+ struct listener *l;
+ struct ebmb_node *node = NULL;
+ char *wildp = NULL;
+ int i;
+
+ for (i = 0; i < trash.size && i < smp->data.u.str.data; i++) {
+ trash.area[i] = tolower(smp->data.u.str.area[i]);
+ if (!wildp && (trash.area[i] == '.'))
+ wildp = [i];
+ }
+ trash.area[i] = 0;
+
+ list_for_each_entry(l, >conf.listeners, by_fe) {
+ if ( args->type == ARGT_STR && l->name && (strcmp(args->data.str.area, l->name) != 0))
+ continue;
+ /* lookup in full qualified names */
+ node = ebst_lookup(>bind_conf->sni_ctx, trash.area);
+ /* lookup in wildcards names */
+ if (!node && wildp)
+ node = ebst_lookup(>bind_conf->sni_w_ctx, wildp);
+ if (node != NULL)
+ break;
+ }
+
+ smp->data.type = SMP_T_BOOL;
+ smp->data.u.sint = !!node;
+ smp->flags = SMP_F_VOL_TEST;
+ return 1;
+}
+
/* parse the "ca-file" bind keyword */
static int ssl_bind_parse_ca_file(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, char **err)
{
@@ -9047,6 +9082,14 @@ static struct sample_fetch_kw_list sample_fetch_keywords = {ILH, {
INITCALL1(STG_REGISTER, sample_register_fetches, _fetch_keywords);
+/* Note: must not be declared as its list will be overwritten */
+static struct sample_conv_kw_list sample_conv_kws = {ILH, {
+ { "ssl_sni_check", smp_conv_ssl_sni_check, ARG1(0,STR), NULL, SMP_T_STR, SMP_T_BOOL },
+ { /* END */ },
+}};
+
+INITCALL1(STG_REGISTER, sample_register_convs, _conv_kws);
+
/* Note: must not be declared as its list will be overwritten.
* Please take care of keeping this list alphabetically sorted.
*/
--
2.19.2
