On Tue, Sep 24, 2019 at 12:43:17PM -0400, Tom Lane wrote:
> One thing I'm wondering is if it's safe to assume that TLS_MAX_VERSION
> will be defined whenever these other symbols are. Looking in an
> 0.9.8x install tree, that doesn't seem to define any of them; while
> in 1.0.1e I see
Yeah, I could personally live with the argument of simplicity and just
say that trying to compile v12 with any version older than 0.9.8zc or
any version that does not have those symbols just does not work, and
that one needs to use the top of the released versions.
> ./tls1.h:#define TLS1_1_VERSION 0x0302
> ./tls1.h:#define TLS1_2_VERSION 0x0303
> ./tls1.h:#define TLS_MAX_VERSION TLS1_2_VERSION
>
> So the patch seems okay for these two versions, but I have no data about
> intermediate OpenSSL versions.
More precisely, all those fields have been added by this upstream
commit, so the fields are present since 0.9.8zc:
commit: c6a876473cbff0fd323c8abcaace98ee2d21863d
author: Bodo Moeller <bodo@openssl.org>
date: Wed, 15 Oct 2014 04:18:29 +0200
Support TLS_FALLBACK_SCSV.
> BTW, the spacing in this patch seems rather random.
Indeed.
Now that I think about it, another method would be to rely on the fact
that a given version of OpenSSL does not support TLS 1.1 and 1.2. So
we could also just add checks based on OPENSSL_VERSION_NUMBER and be
done with it. And actually, looking at their tree TLS 1.1 and 2.2 are
not supported in 1.0.0 either. 1.0.1, 1.0.2, 1.1.0 and HEAD do
support them, but not TLS 1.3.
I would still prefer relying on TLS_MAX_VERSION though, as that's more
portable for future decisions, like the introduction of TLS1_3_VERSION
for which we have already some logic in be-secure-openssl.c. And
updating this stuff would very likely get forgotten once OpenSSL adds
support for TLS 1.3...
There is another issue in the patch:
-#ifdef TLS1_3_VERSION
+#if defined(TLS1_3_VERSION) && TLS1_2_VERSION <= TLS_MAX_VERSION
The second part of the if needs to use TLS1_3_VERSION.
I would also add more brackets around the extra conditions for
readability.
--
Michael
