diff mbox series

[Openvpn-devel,v6,2/3] crypto_openssl: add initialization to pick up local configuration

Message ID 20200528225920.6983-3-James.Bottomley@HansenPartnership.com
State Accepted
Headers
Series add support for engine keys |

Commit Message

James Bottomley May 28, 2020, 12:59 p.m. UTC 
  The test programme for the new openssl engine code requires overriding
the system default configuration file to point to the location of the
test engine.  Add an initialization stanza that makes this behaviour
universal, so now anyone running openvpn configured with openssl can
specify their own configuration file with the OPENSSL_CONF environment
variable.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 src/openvpn/crypto_openssl.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Arne Schwabe June 5, 2020, 1:41 a.m. UTC | #1 
Am 29.05.20 um 00:59 schrieb James Bottomley:
> The test programme for the new openssl engine code requires overriding
                   ^^^
Unrelated to this commit but I wondered about this spelling and it seems
that the British programme spelling usually is a TV programme while
program is used in computer context. AE just uses program for both.

> the system default configuration file to point to the location of the
> test engine.  Add an initialization stanza that makes this behaviour
> universal, so now anyone running openvpn configured with openssl can
> specify their own configuration file with the OPENSSL_CONF environment
> variable.

Acked-By: Arne Schwabe <arne@rfc2549.org>

I also tested that with a configuration file with an invalid statement
this does not abort OpenVPN (the bug we had a few weeks ago)

Arne
Gert Doering June 6, 2020, 8:13 a.m. UTC | #2 
Your patch has been applied to the master branch.

commit a4071b20115c7d4e808df81169a986e65cec4efa
Author: James Bottomley
Date:   Thu May 28 15:59:19 2020 -0700

     crypto_openssl: add initialization to pick up local configuration

     Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <20200528225920.6983-3-James.Bottomley@HansenPartnership.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19936.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Gert Doering June 7, 2020, 1:11 a.m. UTC | #3 
Hi,

> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index 4ac77fde..fd57edd2 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -149,6 +149,11 @@ crypto_init_lib_engine(const char *engine_name)
>  void
>  crypto_init_lib(void)
>  {
> +#if (OPENSSL_VERSION_NUMBER >= 0x10100000L)
> +    OPENSSL_init_crypto(OPENSSL_INIT_LOAD_CONFIG, NULL);
> +#else
> +    OPENSSL_config(NULL);
> +#endif

This is failing on some of the Travis builds, with the error message

1191 crypto_openssl.c: In function `crypto_init_lib':
1192 crypto_openssl.c:155:5: error: implicit declaration of function `OPENSSL_config'; did you mean `OPENSSL_init'? [-Werror=implicit-function-declaration]
1193     OPENSSL_config(NULL);
1194     ^~~~~~~~~~~~~~

https://travis-ci.org/github/OpenVPN/openvpn/builds/695633823

From what I can see, this is for the builds with "openssl 1.0.1u" and
"openssl 1.0.2u" on Bionic.


I'm not sure if we intend to support these versions, but this needs to
be fixed - either the code needs to be adjusted or the travis build
matrix (for master).

gert
James Bottomley June 7, 2020, 12:10 p.m. UTC | #4 
On Sun, 2020-06-07 at 13:11 +0200, Gert Doering wrote:
> Hi,
> 
> > diff --git a/src/openvpn/crypto_openssl.c
> > b/src/openvpn/crypto_openssl.c
> > index 4ac77fde..fd57edd2 100644
> > --- a/src/openvpn/crypto_openssl.c
> > +++ b/src/openvpn/crypto_openssl.c
> > @@ -149,6 +149,11 @@ crypto_init_lib_engine(const char
> > *engine_name)
> >  void
> >  crypto_init_lib(void)
> >  {
> > +#if (OPENSSL_VERSION_NUMBER >= 0x10100000L)
> > +    OPENSSL_init_crypto(OPENSSL_INIT_LOAD_CONFIG, NULL);
> > +#else
> > +    OPENSSL_config(NULL);
> > +#endif
> 
> This is failing on some of the Travis builds, with the error message
> 
> 1191 crypto_openssl.c: In function `crypto_init_lib':
> 1192 crypto_openssl.c:155:5: error: implicit declaration of function
> `OPENSSL_config'; did you mean `OPENSSL_init'? [-Werror=implicit-
> function-declaration]
> 1193     OPENSSL_config(NULL);
> 1194     ^~~~~~~~~~~~~~
> 
> https://travis-ci.org/github/OpenVPN/openvpn/builds/695633823
> 
> From what I can see, this is for the builds with "openssl 1.0.1u" and
> "openssl 1.0.2u" on Bionic.
> 
> 
> I'm not sure if we intend to support these versions, but this needs
> to be fixed - either the code needs to be adjusted or the travis
> build matrix (for master).

Sorry about that.  Best guess is it's missing an include for
openssl/conf.h.  You don't need that today because pretty much every
other openssl header includes it, but that may not always have been so.

Does the below patch fix it?  If it does, it should probably be folded
into the other patch.  It should be safe because openssl/conf.h has
existed for every version of openssl you support.

James

---8>8>8><8<8<8---
From: James Bottomley <James.Bottomley@HansenPartnership.com>
Subject: [PATCH] crypto_openssl: add include for openssl/conf.h

Fix build failure on older versions of openssl.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 src/openvpn/crypto_openssl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index fd57edd2..94b6d85b 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -43,6 +43,7 @@
 #include "crypto_backend.h"
 #include "openssl_compat.h"
 
+#include <openssl/conf.h>
 #include <openssl/des.h>
 #include <openssl/err.h>
 #include <openssl/evp.h>
Arne Schwabe June 8, 2020, 12:05 a.m. UTC | #5 
> 
> Sorry about that.  Best guess is it's missing an include for
> openssl/conf.h.  You don't need that today because pretty much every
> other openssl header includes it, but that may not always have been so.
> 
> Does the below patch fix it?  If it does, it should probably be folded
> into the other patch.  It should be safe because openssl/conf.h has
> existed for every version of openssl you support.


I am also puzzeld by this. On my local machine the OpenSSL 1.0.2 buildis
fine without this extra include.

But I am ACKing this alone on the basis that including the conf.h header
is the corect thing to do (the man page says it should be included)

It fixes the travis build error
(https://travis-ci.org/github/schwabe/openvpn/builds/695947378) but I do
not really understand why OpenSSL behaves different there.

So

Acked-By: Arne Schwabe <arne@rfc2549.org>

> 
> James
> 
> ---8>8>8><8<8<8---
> From: James Bottomley <James.Bottomley@HansenPartnership.com>
> Subject: [PATCH] crypto_openssl: add include for openssl/conf.h
> 
> Fix build failure on older versions of openssl.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>  src/openvpn/crypto_openssl.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index fd57edd2..94b6d85b 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -43,6 +43,7 @@
>  #include "crypto_backend.h"
>  #include "openssl_compat.h"
>  
> +#include <openssl/conf.h>
>  #include <openssl/des.h>
>  #include <openssl/err.h>
>  #include <openssl/evp.h>
>
Илья Шипицин June 8, 2020, 1:12 a.m. UTC | #6 
пн, 8 июн. 2020 г. в 15:06, Arne Schwabe <arne@rfc2549.org>:

>
> >
> > Sorry about that.  Best guess is it's missing an include for
> > openssl/conf.h.  You don't need that today because pretty much every
> > other openssl header includes it, but that may not always have been so.
> >
> > Does the below patch fix it?  If it does, it should probably be folded
> > into the other patch.  It should be safe because openssl/conf.h has
> > existed for every version of openssl you support.
>
>
> I am also puzzeld by this. On my local machine the OpenSSL 1.0.2 buildis
> fine without this extra include.
>
> But I am ACKing this alone on the basis that including the conf.h header
> is the corect thing to do (the man page says it should be included)
>
> It fixes the travis build error
> (https://travis-ci.org/github/schwabe/openvpn/builds/695947378) but I do
> not really understand why OpenSSL behaves different there.
>

it might happen if includes are mixed from OS openssl and custom openssl.
I'll have a look (what is include path in travis).



>
> So
>
> Acked-By: Arne Schwabe <arne@rfc2549.org>
>
> >
> > James
> >
> > ---8>8>8><8<8<8---
> > From: James Bottomley <James.Bottomley@HansenPartnership.com>
> > Subject: [PATCH] crypto_openssl: add include for openssl/conf.h
> >
> > Fix build failure on older versions of openssl.
> >
> > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> > ---
> >  src/openvpn/crypto_openssl.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> > index fd57edd2..94b6d85b 100644
> > --- a/src/openvpn/crypto_openssl.c
> > +++ b/src/openvpn/crypto_openssl.c
> > @@ -43,6 +43,7 @@
> >  #include "crypto_backend.h"
> >  #include "openssl_compat.h"
> >
> > +#include <openssl/conf.h>
> >  #include <openssl/des.h>
> >  #include <openssl/err.h>
> >  #include <openssl/evp.h>
> >
>
>
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">пн, 8 июн. 2020 г. в 15:06, Arne Schwabe &lt;<a href="mailto:arne@rfc2549.org">arne@rfc2549.org</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"><br>
&gt; <br>
&gt; Sorry about that.  Best guess is it&#39;s missing an include for<br>
&gt; openssl/conf.h.  You don&#39;t need that today because pretty much every<br>
&gt; other openssl header includes it, but that may not always have been so.<br>
&gt; <br>
&gt; Does the below patch fix it?  If it does, it should probably be folded<br>
&gt; into the other patch.  It should be safe because openssl/conf.h has<br>
&gt; existed for every version of openssl you support.<br>
<br>
<br>
I am also puzzeld by this. On my local machine the OpenSSL 1.0.2 buildis<br>
fine without this extra include.<br>
<br>
But I am ACKing this alone on the basis that including the conf.h header<br>
is the corect thing to do (the man page says it should be included)<br>
<br>
It fixes the travis build error<br>
(<a href="https://travis-ci.org/github/schwabe/openvpn/builds/695947378" rel="noreferrer" target="_blank">https://travis-ci.org/github/schwabe/openvpn/builds/695947378</a>) but I do<br>
not really understand why OpenSSL behaves different there.<br></blockquote><div><br></div><div>it might happen if includes are mixed from OS openssl and custom openssl.</div><div>I&#39;ll have a look (what is include path in travis).<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">
<br>
So<br>
<br>
Acked-By: Arne Schwabe &lt;<a href="mailto:arne@rfc2549.org" target="_blank">arne@rfc2549.org</a>&gt;<br>
<br>
&gt; <br>
&gt; James<br>
&gt; <br>
&gt; ---8&gt;8&gt;8&gt;&lt;8&lt;8&lt;8---<br>
&gt; From: James Bottomley &lt;James.Bottomley@HansenPartnership.com&gt;<br>
&gt; Subject: [PATCH] crypto_openssl: add include for openssl/conf.h<br>
&gt; <br>
&gt; Fix build failure on older versions of openssl.<br>
&gt; <br>
&gt; Signed-off-by: James Bottomley &lt;James.Bottomley@HansenPartnership.com&gt;<br>
&gt; ---<br>
&gt;  src/openvpn/crypto_openssl.c | 1 +<br>
&gt;  1 file changed, 1 insertion(+)<br>
&gt; <br>
&gt; diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c<br>
&gt; index fd57edd2..94b6d85b 100644<br>
&gt; --- a/src/openvpn/crypto_openssl.c<br>
&gt; +++ b/src/openvpn/crypto_openssl.c<br>
&gt; @@ -43,6 +43,7 @@<br>
&gt;  #include &quot;crypto_backend.h&quot;<br>
&gt;  #include &quot;openssl_compat.h&quot;<br>
&gt;  <br>
&gt; +#include &lt;openssl/conf.h&gt;<br>
&gt;  #include &lt;openssl/des.h&gt;<br>
&gt;  #include &lt;openssl/err.h&gt;<br>
&gt;  #include &lt;openssl/evp.h&gt;<br>
&gt; <br>
<br>
<br>
_______________________________________________<br>
Openvpn-devel mailing list<br>
<a href="mailto:Openvpn-devel@lists.sourceforge.net" target="_blank">Openvpn-devel@lists.sourceforge.net</a><br>
<a href="https://lists.sourceforge.net/lists/listinfo/openvpn-devel" rel="noreferrer" target="_blank">https://lists.sourceforge.net/lists/listinfo/openvpn-devel</a><br>
</blockquote></div></div>
Gert Doering June 8, 2020, 2 a.m. UTC | #7 
Your patch has been applied to the master branch.

commit 25266ebba97d7f4169f902b8b0d3c38eaa4c43a4
Author: James Bottomley
Date:   Sun Jun 7 15:10:58 2020 -0700

     crypto_openssl: add include for openssl/conf.h

     Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <1591567858.4011.15.camel@HansenPartnership.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19996.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index 4ac77fde..fd57edd2 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -149,6 +149,11 @@  crypto_init_lib_engine(const char *engine_name)
 void
 crypto_init_lib(void)
 {
+#if (OPENSSL_VERSION_NUMBER >= 0x10100000L)
+    OPENSSL_init_crypto(OPENSSL_INIT_LOAD_CONFIG, NULL);
+#else
+    OPENSSL_config(NULL);
+#endif
     /*
      * If you build the OpenSSL library and OpenVPN with
      * CRYPTO_MDEBUG, you will get a listing of OpenSSL