diff mbox series

[Openvpn-devel] using openssl feature wherever possible

Message ID CAFHpkQFzOBMUC+qX7vV8jeHxg2MdP6_DCwiUZ=seX7r3DE=cBg@mail.gmail.com
State Rejected
Headers show
Series [Openvpn-devel] using openssl feature wherever possible | expand

Commit Message

Илья Шипицин March 9, 2021, 12:54 a.m. UTC 
Hello,

if nobody minds, I can send several patches that eliminates comparison of
OPENSSL_VERSION, for example





Ilya

Comments

Gert Doering March 9, 2021, 1:47 a.m. UTC | #1 
Hi,

On Tue, Mar 09, 2021 at 04:54:13PM +0500, ???????? ?????????????? wrote:
> if nobody minds, I can send several patches that eliminates comparison of
> OPENSSL_VERSION, for example

We do mind.  They are coded this way on purpose - so when we drop support
for OpenSSL before 1.1.0, it is clear from the code which sections can
be removed completely.

gert
Илья Шипицин March 9, 2021, 1:52 a.m. UTC | #2 
вт, 9 мар. 2021 г. в 17:47, Gert Doering <gert@greenie.muc.de>:

> Hi,
>
> On Tue, Mar 09, 2021 at 04:54:13PM +0500, ???????? ?????????????? wrote:
> > if nobody minds, I can send several patches that eliminates comparison of
> > OPENSSL_VERSION, for example
>
> We do mind.  They are coded this way on purpose - so when we drop support
> for OpenSSL before 1.1.0, it is clear from the code which sections can
>

it is not much fun to catch things like
https://boringssl.googlesource.com/boringssl/+/49e9f67d8b7cbeb3953b5548ad1009d15947a523%5E%21/include/openssl/base.h

(BoringSSL claims itself as 1.1.0, accidentally decided "I'm 1.1.1 now")



> be removed completely.
>
> gert
> --
> "If was one thing all people took for granted, was conviction that if you
>  feed honest figures into a computer, honest figures come out. Never
> doubted
>  it myself till I met a computer with a sense of humor."
>                              Robert A. Heinlein, The Moon is a Harsh
> Mistress
>
> Gert Doering - Munich, Germany
> gert@greenie.muc.de
>
<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">вт, 9 мар. 2021 г. в 17:47, Gert Doering &lt;<a href="mailto:gert@greenie.muc.de">gert@greenie.muc.de</a>&gt;:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<br>
On Tue, Mar 09, 2021 at 04:54:13PM +0500, ???????? ?????????????? wrote:<br>
&gt; if nobody minds, I can send several patches that eliminates comparison of<br>
&gt; OPENSSL_VERSION, for example<br>
<br>
We do mind.  They are coded this way on purpose - so when we drop support<br>
for OpenSSL before 1.1.0, it is clear from the code which sections can<br></blockquote><div><br></div><div>it is not much fun to catch things like</div><div><a href="https://boringssl.googlesource.com/boringssl/+/49e9f67d8b7cbeb3953b5548ad1009d15947a523%5E%21/include/openssl/base.h">https://boringssl.googlesource.com/boringssl/+/49e9f67d8b7cbeb3953b5548ad1009d15947a523%5E%21/include/openssl/base.h</a></div><div><br></div><div>(BoringSSL claims itself as 1.1.0, accidentally decided &quot;I&#39;m 1.1.1 now&quot;)<br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
be removed completely.<br>
<br>
gert<br>
-- <br>
&quot;If was one thing all people took for granted, was conviction that if you <br>
 feed honest figures into a computer, honest figures come out. Never doubted <br>
 it myself till I met a computer with a sense of humor.&quot;<br>
                             Robert A. Heinlein, The Moon is a Harsh Mistress<br>
<br>
Gert Doering - Munich, Germany                             <a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</a><br>
</blockquote></div></div>
Gert Doering March 9, 2021, 1:56 a.m. UTC | #3 
Hi,

On Tue, Mar 09, 2021 at 05:52:12PM +0500, ???????? ?????????????? wrote:
> > On Tue, Mar 09, 2021 at 04:54:13PM +0500, ???????? ?????????????? wrote:
> > > if nobody minds, I can send several patches that eliminates comparison of
> > > OPENSSL_VERSION, for example
> >
> > We do mind.  They are coded this way on purpose - so when we drop support
> > for OpenSSL before 1.1.0, it is clear from the code which sections can
> 
> it is not much fun to catch things like
> https://boringssl.googlesource.com/boringssl/+/49e9f67d8b7cbeb3953b5548ad1009d15947a523%5E%21/include/openssl/base.h
> 
> (BoringSSL claims itself as 1.1.0, accidentally decided "I'm 1.1.1 now")

We do not support BoringSSL.

(Or any other SSL library that claims "I am compatible with OpenSSL 1.1.1"
but isn't, really - which is why the LibreSSL adjustments are always very
minimal)

We sort-of-support LibreSSL because it is the system SSL library on 
OpenBSD.  Otherwise, the answer would be the same as for BoringSSL.

gert
Илья Шипицин March 9, 2021, 2:26 a.m. UTC | #4 
we may keep combo.
both #ifdef EVP_PKEY_TLS1_PRF and comment related to supported openssl
versions (to drop support if we decide)

вт, 9 мар. 2021 г. в 17:56, Gert Doering <gert@greenie.muc.de>:

> Hi,
>
> On Tue, Mar 09, 2021 at 05:52:12PM +0500, ???????? ?????????????? wrote:
> > > On Tue, Mar 09, 2021 at 04:54:13PM +0500, ???????? ??????????????
> wrote:
> > > > if nobody minds, I can send several patches that eliminates
> comparison of
> > > > OPENSSL_VERSION, for example
> > >
> > > We do mind.  They are coded this way on purpose - so when we drop
> support
> > > for OpenSSL before 1.1.0, it is clear from the code which sections can
> >
> > it is not much fun to catch things like
> >
> https://boringssl.googlesource.com/boringssl/+/49e9f67d8b7cbeb3953b5548ad1009d15947a523%5E%21/include/openssl/base.h
> >
> > (BoringSSL claims itself as 1.1.0, accidentally decided "I'm 1.1.1 now")
>
> We do not support BoringSSL.
>
> (Or any other SSL library that claims "I am compatible with OpenSSL 1.1.1"
> but isn't, really - which is why the LibreSSL adjustments are always very
> minimal)
>
> We sort-of-support LibreSSL because it is the system SSL library on
> OpenBSD.  Otherwise, the answer would be the same as for BoringSSL.
>
> gert
> --
> "If was one thing all people took for granted, was conviction that if you
>  feed honest figures into a computer, honest figures come out. Never
> doubted
>  it myself till I met a computer with a sense of humor."
>                              Robert A. Heinlein, The Moon is a Harsh
> Mistress
>
> Gert Doering - Munich, Germany
> gert@greenie.muc.de
>
<div dir="ltr"><div>we may keep combo.</div><div>both #ifdef EVP_PKEY_TLS1_PRF and comment related to supported openssl versions (to drop support if we decide)<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">вт, 9 мар. 2021 г. в 17:56, Gert Doering &lt;<a href="mailto:gert@greenie.muc.de">gert@greenie.muc.de</a>&gt;:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<br>
On Tue, Mar 09, 2021 at 05:52:12PM +0500, ???????? ?????????????? wrote:<br>
&gt; &gt; On Tue, Mar 09, 2021 at 04:54:13PM +0500, ???????? ?????????????? wrote:<br>
&gt; &gt; &gt; if nobody minds, I can send several patches that eliminates comparison of<br>
&gt; &gt; &gt; OPENSSL_VERSION, for example<br>
&gt; &gt;<br>
&gt; &gt; We do mind.  They are coded this way on purpose - so when we drop support<br>
&gt; &gt; for OpenSSL before 1.1.0, it is clear from the code which sections can<br>
&gt; <br>
&gt; it is not much fun to catch things like<br>
&gt; <a href="https://boringssl.googlesource.com/boringssl/+/49e9f67d8b7cbeb3953b5548ad1009d15947a523%5E%21/include/openssl/base.h" rel="noreferrer" target="_blank">https://boringssl.googlesource.com/boringssl/+/49e9f67d8b7cbeb3953b5548ad1009d15947a523%5E%21/include/openssl/base.h</a><br>
&gt; <br>
&gt; (BoringSSL claims itself as 1.1.0, accidentally decided &quot;I&#39;m 1.1.1 now&quot;)<br>
<br>
We do not support BoringSSL.<br>
<br>
(Or any other SSL library that claims &quot;I am compatible with OpenSSL 1.1.1&quot;<br>
but isn&#39;t, really - which is why the LibreSSL adjustments are always very<br>
minimal)<br>
<br>
We sort-of-support LibreSSL because it is the system SSL library on <br>
OpenBSD.  Otherwise, the answer would be the same as for BoringSSL.<br>
<br>
gert<br>
-- <br>
&quot;If was one thing all people took for granted, was conviction that if you <br>
 feed honest figures into a computer, honest figures come out. Never doubted <br>
 it myself till I met a computer with a sense of humor.&quot;<br>
                             Robert A. Heinlein, The Moon is a Harsh Mistress<br>
<br>
Gert Doering - Munich, Germany                             <a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</a><br>
</blockquote></div>
Gert Doering March 9, 2021, 2:28 a.m. UTC | #5 
Hi,

On Tue, Mar 09, 2021 at 06:26:08PM +0500, ???????? ?????????????? wrote:
> we may keep combo.
> both #ifdef EVP_PKEY_TLS1_PRF and comment related to supported openssl
> versions (to drop support if we decide)

We could, but we won't.

(I can see the benefits, but I'm not the one maintaining these code 
bits - Arne is, and he has stated a clear preference on doing it the
way it is currently done)

gert
David Sommerseth March 9, 2021, 4:05 a.m. UTC | #6 
On 09/03/2021 14:28, Gert Doering wrote:
> Hi,
> 
> On Tue, Mar 09, 2021 at 06:26:08PM +0500, ???????? ?????????????? wrote:
>> we may keep combo.
>> both #ifdef EVP_PKEY_TLS1_PRF and comment related to supported openssl
>> versions (to drop support if we decide)
> 
> We could, but we won't.
> 
> (I can see the benefits, but I'm not the one maintaining these code
> bits - Arne is, and he has stated a clear preference on doing it the
> way it is currently done)
> 

+1 ... This is really the right approach.

Either these "wannabe OpenSSL" libraries gets their act together and 
really do fully support the OpenSSL API they claim to be compliant to - 
and everything will work out just fine.  Otherwise they break with 
OpenVPN.  If they start playing fun games with the version macros 
originally defined by OpenSSL, it will break.  It's that easy.  We won't 
spend (waste) time playing their compatibility games.  If it breaks, 
it's their bug not ours.
Arne Schwabe March 9, 2021, 4:54 a.m. UTC | #7 
Am 09.03.21 um 12:54 schrieb Илья Шипицин:
> Hello,
> 
> if nobody minds, I can send several patches that eliminates comparison
> of OPENSSL_VERSION, for example
> 
> 
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index 49698e4b..316cca6f 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -51,7 +51,8 @@
>  #include <openssl/rand.h>
>  #include <openssl/ssl.h>
>  
> -#if (OPENSSL_VERSION_NUMBER >= 0x10100000L) &&
> !defined(LIBRESSL_VERSION_NUMBER)
> +#ifdef EVP_PKEY_TLS1_PRF
>  #include <openssl/kdf.h>
>  #endif

I do not really see a benefit here other than it a lot harder to drop
support for OpenSSL 1.0.2 and not leaving dead code in the repository.
The macro currently tells me exactly why the code is still there. The
EVP_PKEY_TLS1_PRF is not clear. Is this an optional OpenSSL? Is it for
an old version?

Arne
diff mbox series

Patch

diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index 49698e4b..316cca6f 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -51,7 +51,8 @@ 
 #include <openssl/rand.h>
 #include <openssl/ssl.h>

-#if (OPENSSL_VERSION_NUMBER >= 0x10100000L) &&
!defined(LIBRESSL_VERSION_NUMBER)
+#ifdef EVP_PKEY_TLS1_PRF
 #include <openssl/kdf.h>
 #endif