[PATCH 1/4] openssl-native.bbclass: add bbclass

[PATCH 1/4] openssl-native.bbclass: add bbclass

[Mikko Rapeli]

Using openssl-native shared libraries correctly is hard. A number
of environment variables need to be correctly set or
the errors may be really confusing. openssl can be made
to detect these paths automatically, but upstream has rejected
these ideas. openssl-native provides a wrapper script for 'openssl'
binary, but shared library users like python3-cryptgraphy-native
need to have the shared libraries working directly. Thus follow
example from python3native.bbclass and implement this via
openssl-native.bbclass.

If full certificate checking is needed, then users
also need to DEPEND on ca-certificates-native.

See also:
https://lists.openembedded.org/g/openembedded-core/topic/93651845#170562
https://github.com/openssl/openssl/issues/19242

Signed-off-by: Mikko Rapeli <mikko.rapeli@linaro.org>
---
 documentation/ref-manual/classes.rst               | 11 +++++++++++
 meta/classes/openssl-native.bbclass                |  7 +++++++
 meta/recipes-connectivity/openssl/openssl_3.0.5.bb |  1 +
 3 files changed, 19 insertions(+)
 create mode 100644 meta/classes/openssl-native.bbclass

diff --git a/documentation/ref-manual/classes.rst b/documentation/ref-manual/classes.rst
index 11e0d472e8..5fc30f9233 100644
--- a/documentation/ref-manual/classes.rst
+++ b/documentation/ref-manual/classes.rst
@@ -1672,6 +1672,17 @@ one such example. However, being aware of this class can reduce the
 proliferation of different versions of similar classes across multiple
 layers.
 
+.. _ref-classes-openssl-native:
+
+``openssl-native.bbclass``
+==================
+
+The ``openssl-native`` class supports using the native version of openssl
+built by the build system rather than support of the version provided
+by the build host. It helps setting environment variables for engines,
+modules, certificates etc correctly. For full certificate checking
+users should also add ``ca-certificates-native`` to :term:`DEPENDS`.
+
 .. _ref-classes-overlayfs:
 
 ``overlayfs.bbclass``
diff --git a/meta/classes/openssl-native.bbclass b/meta/classes/openssl-native.bbclass
new file mode 100644
index 0000000000..753f0b0fa4
--- /dev/null
+++ b/meta/classes/openssl-native.bbclass
@@ -0,0 +1,7 @@
+DEPENDS += "openssl-native"
+
+export OPENSSL_MODULES="${STAGING_LIBDIR_NATIVE}/ossl-modules"
+export OPENSSL_ENGINES="${STAGING_LIBDIR_NATIVE}/engines-3"
+export OPENSSL_CONF="${STAGING_LIBDIR_NATIVE}/ssl-3/openssl.cnf"
+export SSL_CERT_DIR="${STAGING_LIBDIR_NATIVE}/ssl-3/certs"
+export SSL_CERT_FILE="${STAGING_LIBDIR_NATIVE}/ssl-3/cert.pem"
diff --git a/meta/recipes-connectivity/openssl/openssl_3.0.5.bb b/meta/recipes-connectivity/openssl/openssl_3.0.5.bb
index 04aff04fab..a0e63d7830 100644
--- a/meta/recipes-connectivity/openssl/openssl_3.0.5.bb
+++ b/meta/recipes-connectivity/openssl/openssl_3.0.5.bb
@@ -163,6 +163,7 @@ do_install () {
 	ln -sf ${@oe.path.relative('${libdir}/ssl-3', '${sysconfdir}/ssl/openssl.cnf')} ${D}${libdir}/ssl-3/openssl.cnf
 }
 
+# Please keep openssl-native.bbclass in sync with this wrapper
 do_install:append:class-native () {
 	create_wrapper ${D}${bindir}/openssl \
 	    OPENSSL_CONF=${libdir}/ssl-3/openssl.cnf \
-- 
2.17.1

Re: [docs] [PATCH 1/4] openssl-native.bbclass: add bbclass

[Richard Purdie]

Hi Mikko,

Thanks for working on this, particularly with upstream.

On Tue, 2022-10-04 at 13:10 +0300, Mikko Rapeli wrote:
> Using openssl-native shared libraries correctly is hard. A number
> of environment variables need to be correctly set or
> the errors may be really confusing. openssl can be made
> to detect these paths automatically, but upstream has rejected
> these ideas. openssl-native provides a wrapper script for 'openssl'
> binary, but shared library users like python3-cryptgraphy-native
> need to have the shared libraries working directly. Thus follow
> example from python3native.bbclass and implement this via
> openssl-native.bbclass.
> 
> If full certificate checking is needed, then users
> also need to DEPEND on ca-certificates-native.
> 
> See also:
> https://lists.openembedded.org/g/openembedded-core/topic/93651845#170562
> https://github.com/openssl/openssl/issues/19242
> 
> Signed-off-by: Mikko Rapeli <mikko.rapeli@linaro.org>
> ---
>  documentation/ref-manual/classes.rst               | 11 +++++++++++
>  meta/classes/openssl-native.bbclass                |  7 +++++++
>  meta/recipes-connectivity/openssl/openssl_3.0.5.bb |  1 +
>  3 files changed, 19 insertions(+)

Docs are in a different repo to OE-Core so this patch would need to be
split.

I think it is worth referencing this too:

https://github.com/openssl/openssl/pull/19260

I noticed there that the patches have thrown some compiler warnings:

crypto/conf/conf_mod.c:667:20: error: passing 'const char *(int)' to parameter of type 'const void *' converts between void pointer and function pointer [-Werror,-Wpedantic]
        if (dladdr(OpenSSL_version, &info)) {
crypto/conf/conf_mod.c: In function 'CONF_get1_default_config_file':
crypto/conf/conf_mod.c:667:20: error: ISO C forbids passing argument 1 of 'dladdr' between function pointer and 'void *' [-Werror=pedantic]
  667 |         if (dladdr(OpenSSL_version, &info)) {
      |                    ^~~~~~~~~~~~~~~
In file included from /usr/aarch64-linux-gnu/include/link.h:25,
                 from crypto/conf/conf_mod.c:34:
/usr/aarch64-linux-gnu/include/dlfcn.h:98:32: note: expected 'const void *' but argument is of type 'const char * (*)(int)'
   98 | extern int dladdr (const void *__address, Dl_info *__info)


It may be worth fixing those just in case they consider the patch.

I'm wondering whether we should just carry those patches? They don't
look so invasive and it would save complicating things elsewhere in our
codebase...

Cheers,

Richard

Re: [docs] [PATCH 1/4] openssl-native.bbclass: add bbclass

[Mikko Rapeli]

Hi,

On Tue, Oct 04, 2022 at 12:09:18PM +0100, Richard Purdie wrote:
> Hi Mikko,
> 
> Thanks for working on this, particularly with upstream.
> 
> On Tue, 2022-10-04 at 13:10 +0300, Mikko Rapeli wrote:
> > Using openssl-native shared libraries correctly is hard. A number
> > of environment variables need to be correctly set or
> > the errors may be really confusing. openssl can be made
> > to detect these paths automatically, but upstream has rejected
> > these ideas. openssl-native provides a wrapper script for 'openssl'
> > binary, but shared library users like python3-cryptgraphy-native
> > need to have the shared libraries working directly. Thus follow
> > example from python3native.bbclass and implement this via
> > openssl-native.bbclass.
> > 
> > If full certificate checking is needed, then users
> > also need to DEPEND on ca-certificates-native.
> > 
> > See also:
> > https://lists.openembedded.org/g/openembedded-core/topic/93651845#170562
> > https://github.com/openssl/openssl/issues/19242
> > 
> > Signed-off-by: Mikko Rapeli <mikko.rapeli@linaro.org>
> > ---
> >  documentation/ref-manual/classes.rst               | 11 +++++++++++
> >  meta/classes/openssl-native.bbclass                |  7 +++++++
> >  meta/recipes-connectivity/openssl/openssl_3.0.5.bb |  1 +
> >  3 files changed, 19 insertions(+)
> 
> Docs are in a different repo to OE-Core so this patch would need to be
> split.

Ok, will split them.

> I think it is worth referencing this too:
> 
> https://github.com/openssl/openssl/pull/19260

Ok, will add.

> I noticed there that the patches have thrown some compiler warnings:
> 
> crypto/conf/conf_mod.c:667:20: error: passing 'const char *(int)' to parameter of type 'const void *' converts between void pointer and function pointer [-Werror,-Wpedantic]
>         if (dladdr(OpenSSL_version, &info)) {
> crypto/conf/conf_mod.c: In function 'CONF_get1_default_config_file':
> crypto/conf/conf_mod.c:667:20: error: ISO C forbids passing argument 1 of 'dladdr' between function pointer and 'void *' [-Werror=pedantic]
>   667 |         if (dladdr(OpenSSL_version, &info)) {
>       |                    ^~~~~~~~~~~~~~~
> In file included from /usr/aarch64-linux-gnu/include/link.h:25,
>                  from crypto/conf/conf_mod.c:34:
> /usr/aarch64-linux-gnu/include/dlfcn.h:98:32: note: expected 'const void *' but argument is of type 'const char * (*)(int)'
>    98 | extern int dladdr (const void *__address, Dl_info *__info)
> 
> 
> It may be worth fixing those just in case they consider the patch.

Yes, but the general design of using dladdr(OpenSSL_version,...) did not
get any positive comments in the bug report so I think this is wasted
effort.

> I'm wondering whether we should just carry those patches? They don't
> look so invasive and it would save complicating things elsewhere in our
> codebase...

I don't think so. For every variable the patch is annoyingly different
and there are merge/rebase conflicts with newer upstream releases
already.

I think pyton3native.bbclass sets the example. Variables need to be set
for openssl to work correctly after relocation to paths not known at
compile time. Same applies to any code which needs to dlopen() something
or which has custom locations for modules, data or configuration files.

Cheers,

-Mikko

Re: [docs] [PATCH 1/4] openssl-native.bbclass: add bbclass

[Richard Purdie]

On Tue, 2022-10-04 at 14:38 +0300, Mikko Rapeli wrote:
> On Tue, Oct 04, 2022 at 12:09:18PM +0100, Richard Purdie wrote:
> 
> > I noticed there that the patches have thrown some compiler warnings:
> > 
> > crypto/conf/conf_mod.c:667:20: error: passing 'const char *(int)' to parameter of type 'const void *' converts between void pointer and function pointer [-Werror,-Wpedantic]
> >         if (dladdr(OpenSSL_version, &info)) {
> > crypto/conf/conf_mod.c: In function 'CONF_get1_default_config_file':
> > crypto/conf/conf_mod.c:667:20: error: ISO C forbids passing argument 1 of 'dladdr' between function pointer and 'void *' [-Werror=pedantic]
> >   667 |         if (dladdr(OpenSSL_version, &info)) {
> >       |                    ^~~~~~~~~~~~~~~
> > In file included from /usr/aarch64-linux-gnu/include/link.h:25,
> >                  from crypto/conf/conf_mod.c:34:
> > /usr/aarch64-linux-gnu/include/dlfcn.h:98:32: note: expected 'const void *' but argument is of type 'const char * (*)(int)'
> >    98 | extern int dladdr (const void *__address, Dl_info *__info)
> > 
> > 
> > It may be worth fixing those just in case they consider the patch.
> 
> Yes, but the general design of using dladdr(OpenSSL_version,...) did not
> get any positive comments in the bug report so I think this is wasted
> effort.

There isn't any feedback there saying dladdr is rejected, just that
they're not sure about the general use case. Getting changes accepted
by upstreams does usually require a bit of work so I'd not quite give
up yet! I can understand it from the maintainers side too, if you're
being asked to accept and maintain something, you do need there to be a
compelling reason for it.

> > I'm wondering whether we should just carry those patches? They don't
> > look so invasive and it would save complicating things elsewhere in our
> > codebase...
> 
> I don't think so. For every variable the patch is annoyingly different
> and there are merge/rebase conflicts with newer upstream releases
> already.

I didn't look in detail but at least the injected code blocks do look
relatively self contained which does make maintenance easier. The fact
they're had rebase conflicts already is sad/worrying though :(.

> I think pyton3native.bbclass sets the example. Variables need to be set
> for openssl to work correctly after relocation to paths not known at
> compile time. Same applies to any code which needs to dlopen() something
> or which has custom locations for modules, data or configuration files.

I'm not sure I'd hold python3native as a good example. It was what we
needed to do to get things to work and was the least worst thing :/.
Limitations to a single scripting language are much easier than a
general elf shared library.

Cheers,

Richard

Re: [docs] [PATCH 1/4] openssl-native.bbclass: add bbclass

[Mikko Rapeli]

Hi,

On Tue, Oct 04, 2022 at 01:19:41PM +0100, Richard Purdie wrote:
> On Tue, 2022-10-04 at 14:38 +0300, Mikko Rapeli wrote:
> > On Tue, Oct 04, 2022 at 12:09:18PM +0100, Richard Purdie wrote:
> > 
> > > I noticed there that the patches have thrown some compiler warnings:
> > > 
> > > crypto/conf/conf_mod.c:667:20: error: passing 'const char *(int)' to parameter of type 'const void *' converts between void pointer and function pointer [-Werror,-Wpedantic]
> > >         if (dladdr(OpenSSL_version, &info)) {
> > > crypto/conf/conf_mod.c: In function 'CONF_get1_default_config_file':
> > > crypto/conf/conf_mod.c:667:20: error: ISO C forbids passing argument 1 of 'dladdr' between function pointer and 'void *' [-Werror=pedantic]
> > >   667 |         if (dladdr(OpenSSL_version, &info)) {
> > >       |                    ^~~~~~~~~~~~~~~
> > > In file included from /usr/aarch64-linux-gnu/include/link.h:25,
> > >                  from crypto/conf/conf_mod.c:34:
> > > /usr/aarch64-linux-gnu/include/dlfcn.h:98:32: note: expected 'const void *' but argument is of type 'const char * (*)(int)'
> > >    98 | extern int dladdr (const void *__address, Dl_info *__info)
> > > 
> > > 
> > > It may be worth fixing those just in case they consider the patch.
> > 
> > Yes, but the general design of using dladdr(OpenSSL_version,...) did not
> > get any positive comments in the bug report so I think this is wasted
> > effort.
> 
> There isn't any feedback there saying dladdr is rejected, just that
> they're not sure about the general use case. Getting changes accepted
> by upstreams does usually require a bit of work so I'd not quite give
> up yet! I can understand it from the maintainers side too, if you're
> being asked to accept and maintain something, you do need there to be a
> compelling reason for it.

openssl has been using these environment variables for decades. I can
understand that they hesitate to change any of that. Also because some
of the code is obviously trying to avoid any posix dependencies including
stat().

https://github.com/openssl/openssl/issues/19242

"t8m commented 15 days ago
I am afraid this is potentially asking for security issues. It would
have to be implemented very carefully."

"beldmit commented 15 days ago
I don't like this approach as a whole. IMHO, we should have some defines
to find installation-specific values for a specific installation."

"levitte commented 14 days ago
It's possible that it would be better if util/wrap.pl became a public
tool. Not necessarily exactly as it works now, but something with a
similar intent."

"levitte commented 14 days ago
This isn't just an OpenSSL problem, is it? There are other libraries
that are plugable (and essentially, providers are exactly that,
plugins), and I imagine that they also have their own custom default
location for plugins.
Otherwise, the obvious answer would be that you should install things
that belong with OpenSSL into its default locations... and that's
answered with openssl version -a as said above, or with the openssl info
command in later OpenSSL versions."

So wrapper it is then.

> > > I'm wondering whether we should just carry those patches? They don't
> > > look so invasive and it would save complicating things elsewhere in our
> > > codebase...
> > 
> > I don't think so. For every variable the patch is annoyingly different
> > and there are merge/rebase conflicts with newer upstream releases
> > already.
> 
> I didn't look in detail but at least the injected code blocks do look
> relatively self contained which does make maintenance easier. The fact
> they're had rebase conflicts already is sad/worrying though :(.

Exactly.

> > I think pyton3native.bbclass sets the example. Variables need to be set
> > for openssl to work correctly after relocation to paths not known at
> > compile time. Same applies to any code which needs to dlopen() something
> > or which has custom locations for modules, data or configuration files.
> 
> I'm not sure I'd hold python3native as a good example. It was what we
> needed to do to get things to work and was the least worst thing :/.
> Limitations to a single scripting language are much easier than a
> general elf shared library.

Well, the openssl-native recipes 'openssl' wrapper script seems to be fine.

This openssl-native.bbclass extends the same approach for those who just load
the shared libraries. python3-cryptography-native would be one clear
user since currently it's completely broken and even fails to load the
python3 module.

Cheers,

-Mikko

Re: [docs] [PATCH 1/4] openssl-native.bbclass: add bbclass

[Richard Purdie]

On Tue, 2022-10-04 at 15:54 +0300, Mikko Rapeli wrote:
> On Tue, Oct 04, 2022 at 01:19:41PM +0100, Richard Purdie wrote:
> > On Tue, 2022-10-04 at 14:38 +0300, Mikko Rapeli wrote:
> > > On Tue, Oct 04, 2022 at 12:09:18PM +0100, Richard Purdie wrote:
> > > 
> > > > I noticed there that the patches have thrown some compiler warnings:
> > > > 
> > > > crypto/conf/conf_mod.c:667:20: error: passing 'const char *(int)' to parameter of type 'const void *' converts between void pointer and function pointer [-Werror,-Wpedantic]
> > > >         if (dladdr(OpenSSL_version, &info)) {
> > > > crypto/conf/conf_mod.c: In function 'CONF_get1_default_config_file':
> > > > crypto/conf/conf_mod.c:667:20: error: ISO C forbids passing argument 1 of 'dladdr' between function pointer and 'void *' [-Werror=pedantic]
> > > >   667 |         if (dladdr(OpenSSL_version, &info)) {
> > > >       |                    ^~~~~~~~~~~~~~~
> > > > In file included from /usr/aarch64-linux-gnu/include/link.h:25,
> > > >                  from crypto/conf/conf_mod.c:34:
> > > > /usr/aarch64-linux-gnu/include/dlfcn.h:98:32: note: expected 'const void *' but argument is of type 'const char * (*)(int)'
> > > >    98 | extern int dladdr (const void *__address, Dl_info *__info)
> > > > 
> > > > 
> > > > It may be worth fixing those just in case they consider the patch.
> > > 
> > > Yes, but the general design of using dladdr(OpenSSL_version,...) did not
> > > get any positive comments in the bug report so I think this is wasted
> > > effort.
> > 
> > There isn't any feedback there saying dladdr is rejected, just that
> > they're not sure about the general use case. Getting changes accepted
> > by upstreams does usually require a bit of work so I'd not quite give
> > up yet! I can understand it from the maintainers side too, if you're
> > being asked to accept and maintain something, you do need there to be a
> > compelling reason for it.
> 
> openssl has been using these environment variables for decades. I can
> understand that they hesitate to change any of that. Also because some
> of the code is obviously trying to avoid any posix dependencies including
> stat().
> 
> https://github.com/openssl/openssl/issues/19242
> 
> "t8m commented 15 days ago
> I am afraid this is potentially asking for security issues. It would
> have to be implemented very carefully."
> 
> "beldmit commented 15 days ago
> I don't like this approach as a whole. IMHO, we should have some defines
> to find installation-specific values for a specific installation."
> 
> "levitte commented 14 days ago
> It's possible that it would be better if util/wrap.pl became a public
> tool. Not necessarily exactly as it works now, but something with a
> similar intent."
> 
> "levitte commented 14 days ago
> This isn't just an OpenSSL problem, is it? There are other libraries
> that are plugable (and essentially, providers are exactly that,
> plugins), and I imagine that they also have their own custom default
> location for plugins.
> Otherwise, the obvious answer would be that you should install things
> that belong with OpenSSL into its default locations... and that's
> answered with openssl version -a as said above, or with the openssl info
> command in later OpenSSL versions."
> 
> So wrapper it is then.

I was going to write a reply to some of that, I still might, but as I
was doing it, another idea did just come to mind.

Somewhere I'm guessing openssl has some common init function?

Perhaps in that we patch in a hook, which looks at the current path of
the library, compares that to the default install location, then sets
the magic envvars accordingly if the location has changed?

That code should be isolatable in that we only have one entry point to
patch in and as you say, the envvars have been around forever, we'd
just need to watch for new ones.

The advantage to this is that the openssl library should then "just
work", we don't need to worry about the environment being present
everywhere?

This would also give us a new way to avoid some of these kinds of
issues with a new example to follow which doesn't involve manually
ensuring all users are tweaked. I really don't like the environment
variable approach for libraries.

Cheers,

Richard

Re: [docs] [PATCH 1/4] openssl-native.bbclass: add bbclass

[Mikko Rapeli]

Hi,

On Tue, Oct 04, 2022 at 02:09:45PM +0100, Richard Purdie wrote:
> On Tue, 2022-10-04 at 15:54 +0300, Mikko Rapeli wrote:
> > On Tue, Oct 04, 2022 at 01:19:41PM +0100, Richard Purdie wrote:
> > > On Tue, 2022-10-04 at 14:38 +0300, Mikko Rapeli wrote:
> > > > On Tue, Oct 04, 2022 at 12:09:18PM +0100, Richard Purdie wrote:
> > > > 
> > > > > I noticed there that the patches have thrown some compiler warnings:
> > > > > 
> > > > > crypto/conf/conf_mod.c:667:20: error: passing 'const char *(int)' to parameter of type 'const void *' converts between void pointer and function pointer [-Werror,-Wpedantic]
> > > > >         if (dladdr(OpenSSL_version, &info)) {
> > > > > crypto/conf/conf_mod.c: In function 'CONF_get1_default_config_file':
> > > > > crypto/conf/conf_mod.c:667:20: error: ISO C forbids passing argument 1 of 'dladdr' between function pointer and 'void *' [-Werror=pedantic]
> > > > >   667 |         if (dladdr(OpenSSL_version, &info)) {
> > > > >       |                    ^~~~~~~~~~~~~~~
> > > > > In file included from /usr/aarch64-linux-gnu/include/link.h:25,
> > > > >                  from crypto/conf/conf_mod.c:34:
> > > > > /usr/aarch64-linux-gnu/include/dlfcn.h:98:32: note: expected 'const void *' but argument is of type 'const char * (*)(int)'
> > > > >    98 | extern int dladdr (const void *__address, Dl_info *__info)
> > > > > 
> > > > > 
> > > > > It may be worth fixing those just in case they consider the patch.
> > > > 
> > > > Yes, but the general design of using dladdr(OpenSSL_version,...) did not
> > > > get any positive comments in the bug report so I think this is wasted
> > > > effort.
> > > 
> > > There isn't any feedback there saying dladdr is rejected, just that
> > > they're not sure about the general use case. Getting changes accepted
> > > by upstreams does usually require a bit of work so I'd not quite give
> > > up yet! I can understand it from the maintainers side too, if you're
> > > being asked to accept and maintain something, you do need there to be a
> > > compelling reason for it.
> > 
> > openssl has been using these environment variables for decades. I can
> > understand that they hesitate to change any of that. Also because some
> > of the code is obviously trying to avoid any posix dependencies including
> > stat().
> > 
> > https://github.com/openssl/openssl/issues/19242
> > 
> > "t8m commented 15 days ago
> > I am afraid this is potentially asking for security issues. It would
> > have to be implemented very carefully."
> > 
> > "beldmit commented 15 days ago
> > I don't like this approach as a whole. IMHO, we should have some defines
> > to find installation-specific values for a specific installation."
> > 
> > "levitte commented 14 days ago
> > It's possible that it would be better if util/wrap.pl became a public
> > tool. Not necessarily exactly as it works now, but something with a
> > similar intent."
> > 
> > "levitte commented 14 days ago
> > This isn't just an OpenSSL problem, is it? There are other libraries
> > that are plugable (and essentially, providers are exactly that,
> > plugins), and I imagine that they also have their own custom default
> > location for plugins.
> > Otherwise, the obvious answer would be that you should install things
> > that belong with OpenSSL into its default locations... and that's
> > answered with openssl version -a as said above, or with the openssl info
> > command in later OpenSSL versions."
> > 
> > So wrapper it is then.
> 
> I was going to write a reply to some of that, I still might, but as I
> was doing it, another idea did just come to mind.
> 
> Somewhere I'm guessing openssl has some common init function?

Sadly, there isn't. Not even a common header file. Or at least
I did not find any. The engines, modules, config files and certs
are quite different things compared to the plain shared libraries
which are already found from LD_LIBRARY_PATH.

Cheers,

-Mikko