diff mbox series

sslapi: Include wolfSSL's options.h for non-PKCS11

Message ID 20200924193818.140725-1-bage@linutronix.de
State Changes Requested
Headers show
Series sslapi: Include wolfSSL's options.h for non-PKCS11 | expand

Commit Message

Bastian Germann Sept. 24, 2020, 7:38 p.m. UTC 
From: Bastian Germann <bage@linutronix.de>

The wolfssl/options.h needs to be loaded even if only the OpenSSL
compatibility layer is loaded.  wolfSSL's David Garske was so nice
to point that out.  ba8c4a8bff("Add missing wolfSSL definitions") is
wrong with the claim that wolfSSL has a memory bug.

Link: https://github.com/wolfSSL/wolfssl/issues/3334
Signed-off-by: Bastian Germann <bage@linutronix.de>
---
 Makefile.flags   | 2 +-
 include/sslapi.h | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

Comments

Stefano Babic Sept. 24, 2020, 7:50 p.m. UTC | #1 
Hi Bastian,

On 24.09.20 21:38, bage@linutronix.de wrote:
> From: Bastian Germann <bage@linutronix.de>
> 
> The wolfssl/options.h needs to be loaded even if only the OpenSSL
> compatibility layer is loaded.  wolfSSL's David Garske was so nice
> to point that out.  ba8c4a8bff("Add missing wolfSSL definitions") is
> wrong with the claim that wolfSSL has a memory bug.
> 
> Link: https://github.com/wolfSSL/wolfssl/issues/3334
> Signed-off-by: Bastian Germann <bage@linutronix.de>
> ---
>   Makefile.flags   | 2 +-
>   include/sslapi.h | 5 ++++-
>   2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile.flags b/Makefile.flags
> index 8b4a95c..b026250 100644
> --- a/Makefile.flags
> +++ b/Makefile.flags
> @@ -150,7 +150,7 @@ LDLIBS += crypto ssl
>   endif
>   
>   ifeq ($(CONFIG_SSL_IMPL_WOLFSSL),y)
> -KBUILD_CPPFLAGS += -I/usr/include/wolfssl -DOPENSSL_ALL -DWOLFSSL_APACHE_HTTPD
> +KBUILD_CPPFLAGS += -I/usr/include/wolfssl -DOPENSSL_ALL

I have not seen this in the previous patch - this is wrong and it must 
be fixed. No hardcoded include path is allowed. How can it work with OE 
/ Buildroot ? And why do we need it ?

$CC has already set sysroots and the include path.

>   LDLIBS += wolfssl
>   else ifeq ($(CONFIG_PKCS11),y)
>   LDLIBS += wolfssl
> diff --git a/include/sslapi.h b/include/sslapi.h
> index 9a6af4f..dfb6b8f 100644
> --- a/include/sslapi.h
> +++ b/include/sslapi.h
> @@ -26,7 +26,10 @@
>   // Exclude p11-kit's pkcs11.h to prevent conflicting with wolfssl's
>   #define PKCS11_H 1
>   #include <p11-kit/uri.h>
> -#endif
> +
> +#elif defined(CONFIG_SSL_IMPL_WOLFSSL)
> +#include <wolfssl/options.h>
> +#endif /* CONFIG_PKCS11 */
>   
>   #if defined(CONFIG_SSL_IMPL_OPENSSL) || defined(CONFIG_SSL_IMPL_WOLFSSL)
>   #include <openssl/bio.h>
> 

Best regards,
Stefano Babic
Bastian Germann Sept. 25, 2020, 9:08 a.m. UTC | #2 
Am 24.09.20 um 21:50 schrieb Stefano Babic:
> Hi Bastian,
> 
> On 24.09.20 21:38, bage@linutronix.de wrote:
>> From: Bastian Germann <bage@linutronix.de>
>>
>> The wolfssl/options.h needs to be loaded even if only the OpenSSL
>> compatibility layer is loaded.  wolfSSL's David Garske was so nice
>> to point that out.  ba8c4a8bff("Add missing wolfSSL definitions") is
>> wrong with the claim that wolfSSL has a memory bug.
>>
>> Link: https://github.com/wolfSSL/wolfssl/issues/3334
>> Signed-off-by: Bastian Germann <bage@linutronix.de>
>> ---
>>   Makefile.flags   | 2 +-
>>   include/sslapi.h | 5 ++++-
>>   2 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/Makefile.flags b/Makefile.flags
>> index 8b4a95c..b026250 100644
>> --- a/Makefile.flags
>> +++ b/Makefile.flags
>> @@ -150,7 +150,7 @@ LDLIBS += crypto ssl
>>   endif
>>     ifeq ($(CONFIG_SSL_IMPL_WOLFSSL),y)
>> -KBUILD_CPPFLAGS += -I/usr/include/wolfssl -DOPENSSL_ALL
>> -DWOLFSSL_APACHE_HTTPD
>> +KBUILD_CPPFLAGS += -I/usr/include/wolfssl -DOPENSSL_ALL
> 
> I have not seen this in the previous patch - this is wrong and it must
> be fixed. No hardcoded include path is allowed. How can it work with OE
> / Buildroot ? And why do we need it ?

We need it for wolfSSL's OpenSSL compatibility layer. It is located in
wolfssl/openssl/... Otherwise we would have to prepend all the openssl
includes with "wolfssl/" in case that layer is used. Or you have to rely
on people setting this themselves. I find it always frustrating if I
cannot build without any buildsystem from scratch without specifying any
additional header/lib stuff. And this one is not obvious when you get
"missing <openssl/...>"

Another approach would be finding the wolfssl include dir via pkgconfig
and adding "/wolfssl" to it.

> 
> $CC has already set sysroots and the include path.
> 
>>   LDLIBS += wolfssl
>>   else ifeq ($(CONFIG_PKCS11),y)
>>   LDLIBS += wolfssl
>> diff --git a/include/sslapi.h b/include/sslapi.h
>> index 9a6af4f..dfb6b8f 100644
>> --- a/include/sslapi.h
>> +++ b/include/sslapi.h
>> @@ -26,7 +26,10 @@
>>   // Exclude p11-kit's pkcs11.h to prevent conflicting with wolfssl's
>>   #define PKCS11_H 1
>>   #include <p11-kit/uri.h>
>> -#endif
>> +
>> +#elif defined(CONFIG_SSL_IMPL_WOLFSSL)
>> +#include <wolfssl/options.h>
>> +#endif /* CONFIG_PKCS11 */
>>     #if defined(CONFIG_SSL_IMPL_OPENSSL) ||
>> defined(CONFIG_SSL_IMPL_WOLFSSL)
>>   #include <openssl/bio.h>
>>
> 
> Best regards,
> Stefano Babic
Stefano Babic Sept. 30, 2020, 10:26 a.m. UTC | #3 
Hi Bastian,

On 25.09.20 11:08, Bastian Germann wrote:
> Am 24.09.20 um 21:50 schrieb Stefano Babic:
>> Hi Bastian,
>>
>> On 24.09.20 21:38, bage@linutronix.de wrote:
>>> From: Bastian Germann <bage@linutronix.de>
>>>
>>> The wolfssl/options.h needs to be loaded even if only the OpenSSL
>>> compatibility layer is loaded.  wolfSSL's David Garske was so nice
>>> to point that out.  ba8c4a8bff("Add missing wolfSSL definitions") is
>>> wrong with the claim that wolfSSL has a memory bug.
>>>
>>> Link: https://github.com/wolfSSL/wolfssl/issues/3334
>>> Signed-off-by: Bastian Germann <bage@linutronix.de>
>>> ---
>>>   Makefile.flags   | 2 +-
>>>   include/sslapi.h | 5 ++++-
>>>   2 files changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Makefile.flags b/Makefile.flags
>>> index 8b4a95c..b026250 100644
>>> --- a/Makefile.flags
>>> +++ b/Makefile.flags
>>> @@ -150,7 +150,7 @@ LDLIBS += crypto ssl
>>>   endif
>>>     ifeq ($(CONFIG_SSL_IMPL_WOLFSSL),y)
>>> -KBUILD_CPPFLAGS += -I/usr/include/wolfssl -DOPENSSL_ALL
>>> -DWOLFSSL_APACHE_HTTPD
>>> +KBUILD_CPPFLAGS += -I/usr/include/wolfssl -DOPENSSL_ALL
>>
>> I have not seen this in the previous patch - this is wrong and it must
>> be fixed. No hardcoded include path is allowed. How can it work with OE
>> / Buildroot ? And why do we need it ?
> 
> We need it for wolfSSL's OpenSSL compatibility layer.

I see this and I understand the reason, it is quite a "trick". What
happens if both openSSL and WolfSSL are installed on the same system ?
The order of the include path rules.

Wolfssl compatibility layer sets the right include, because they search
then includes in wolfssl/openssl/. But depending on on the order is not
the best.

> It is located in
> wolfssl/openssl/... Otherwise we would have to prepend all the openssl
> includes with "wolfssl/" in case that layer is used.

This is an option, we need to explicitely set them in sslapi.h with
#ifdef. Not very nice, but it is at least just in one file I suppose.

> Or you have to rely
> on people setting this themselves.

ok, and the the ML list is overflood by people asking "why cannot build
? ...". No, thanks !

> I find it always frustrating if I
> cannot build without any buildsystem from scratch without specifying any
> additional header/lib stuff. And this one is not obvious when you get
> "missing <openssl/...>"

Then remains the option before. I can still manage in OE by adding an
EXTRA_OEMAKE with the path, but it is exactly the same "frustrating"
topic you mention. It is a way used by several recipes, I see many of them.

Anyway, "/usr/include" is wrong for crosscompiling on any system, and it
works just well with native building (ok, Debian is not affected !).

The same issue is for the pkcs11:

ifeq ($(CONFIG_PKCS11),y)
 KBUILD_CPPFLAGS += -I/usr/include/p11-kit-1
 LDLIBS += p11-kit
 endif


But this seems superflous to me. Headers are already  including
<p11-kit/..>. Do we need this ? Can we drop it ?

> 
> Another approach would be finding the wolfssl include dir via pkgconfig
> and adding "/wolfssl" to it.

Still working well in distro, it should be ok in OE (I have to check for
Wolfssl recipe), I do not know in Buildroot.

Regards,
Stefano

> 
>>
>> $CC has already set sysroots and the include path.
>>
>>>   LDLIBS += wolfssl
>>>   else ifeq ($(CONFIG_PKCS11),y)
>>>   LDLIBS += wolfssl
>>> diff --git a/include/sslapi.h b/include/sslapi.h
>>> index 9a6af4f..dfb6b8f 100644
>>> --- a/include/sslapi.h
>>> +++ b/include/sslapi.h
>>> @@ -26,7 +26,10 @@
>>>   // Exclude p11-kit's pkcs11.h to prevent conflicting with wolfssl's
>>>   #define PKCS11_H 1
>>>   #include <p11-kit/uri.h>
>>> -#endif
>>> +
>>> +#elif defined(CONFIG_SSL_IMPL_WOLFSSL)
>>> +#include <wolfssl/options.h>
>>> +#endif /* CONFIG_PKCS11 */
>>>     #if defined(CONFIG_SSL_IMPL_OPENSSL) ||
>>> defined(CONFIG_SSL_IMPL_WOLFSSL)
>>>   #include <openssl/bio.h>
>>>
>>
>> Best regards,
>> Stefano Babic
diff mbox series

Patch

diff --git a/Makefile.flags b/Makefile.flags
index 8b4a95c..b026250 100644
--- a/Makefile.flags
+++ b/Makefile.flags
@@ -150,7 +150,7 @@  LDLIBS += crypto ssl
 endif
 
 ifeq ($(CONFIG_SSL_IMPL_WOLFSSL),y)
-KBUILD_CPPFLAGS += -I/usr/include/wolfssl -DOPENSSL_ALL -DWOLFSSL_APACHE_HTTPD
+KBUILD_CPPFLAGS += -I/usr/include/wolfssl -DOPENSSL_ALL
 LDLIBS += wolfssl
 else ifeq ($(CONFIG_PKCS11),y)
 LDLIBS += wolfssl
diff --git a/include/sslapi.h b/include/sslapi.h
index 9a6af4f..dfb6b8f 100644
--- a/include/sslapi.h
+++ b/include/sslapi.h
@@ -26,7 +26,10 @@ 
 // Exclude p11-kit's pkcs11.h to prevent conflicting with wolfssl's
 #define PKCS11_H 1
 #include <p11-kit/uri.h>
-#endif
+
+#elif defined(CONFIG_SSL_IMPL_WOLFSSL)
+#include <wolfssl/options.h>
+#endif /* CONFIG_PKCS11 */
 
 #if defined(CONFIG_SSL_IMPL_OPENSSL) || defined(CONFIG_SSL_IMPL_WOLFSSL)
 #include <openssl/bio.h>