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 <b...@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
signature.asc
Description: PGP signature
