[Buildroot] [PATCH v4 1/1] package/libssh2: Add selectable crypto libraries

[Buildroot] [PATCH v4 1/1] package/libssh2: Add selectable crypto libraries

[Sam Voss]

Currently, the selection of the backend is based on a priority order,
which is not always desirable: not all features are available for all
backends, as reported upstream:
    https://github.com/libssh2/libssh2/issues/213

As such, allow a user to select the backend most appropriate to their
use-case.

Signed-off-by: Sam Voss <sam.voss@rockwellcollins.com>

--

[v3->v4]
 - Update configuration for "type->depends->select" ordering
 - Update patch message to be more descriptive

[v2->v3]
 - Fix comment about favoring mbedtls

[v1->v2]
 - Do not have comments when crypo is not selected, select it instead.
 - Do not select OpenSSL by default when libssh2 is selected if no
   others are chosen
---
 package/libssh2/Config.in  | 24 +++++++++++++++++++++++-
 package/libssh2/libssh2.mk |  8 ++++----
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/package/libssh2/Config.in b/package/libssh2/Config.in
index 9b60823..f2d32a9 100644
--- a/package/libssh2/Config.in
+++ b/package/libssh2/Config.in
@@ -1,6 +1,5 @@
 config BR2_PACKAGE_LIBSSH2
 	bool "libssh2"
-	select BR2_PACKAGE_OPENSSL if !(BR2_PACKAGE_MBEDTLS || BR2_PACKAGE_LIBGCRYPT)
 	help
 	  libssh2 is a client-side C library implementing the SSH2
 	  protocol as defined by Internet Drafts: SECSH-TRANS(22),
@@ -8,3 +7,26 @@ config BR2_PACKAGE_LIBSSH2
 	  SECSH-FILEXFER(06)*, SECSH-DHGEX(04), and SECSH-NUMBERS(10)
 
 	  http://www.libssh2.org/
+
+if BR2_PACKAGE_LIBSSH2
+
+choice
+	prompt "Crypto Backend"
+	help
+	  Select crypto library to be used in libssh2.
+
+config BR2_PACKAGE_LIBSSH2_MBEDTLS
+	bool "mbedtls"
+	select BR2_PACKAGE_MBEDTLS
+
+config BR2_PACKAGE_LIBSSH2_LIBGCRYPT
+	bool "gcrypt"
+	depends on BR2_PACKAGE_LIBGPG_ERROR_ARCH_SUPPORTS # libgcrypt -> libgpg-error
+	select BR2_PACKAGE_LIBGCRYPT
+
+config BR2_PACKAGE_LIBSSH2_OPENSSL
+	bool "openssl"
+	select BR2_PACKAGE_OPENSSL
+
+endchoice
+endif
diff --git a/package/libssh2/libssh2.mk b/package/libssh2/libssh2.mk
index d40e844..befac92 100644
--- a/package/libssh2/libssh2.mk
+++ b/package/libssh2/libssh2.mk
@@ -15,19 +15,19 @@ LIBSSH2_CONF_OPTS = --disable-examples-build
 LIBSSH2_AUTORECONF = YES
 
 # Dependency is one of mbedtls, libgcrypt or openssl, guaranteed in
-# Config.in. Favour mbedtls.
-ifeq ($(BR2_PACKAGE_MBEDTLS),y)
+# Config.in.
+ifeq ($(BR2_PACKAGE_LIBSSH2_MBEDTLS),y)
 LIBSSH2_DEPENDENCIES += mbedtls
 LIBSSH2_CONF_OPTS += --with-libmbedcrypto-prefix=$(STAGING_DIR)/usr \
 	--with-crypto=mbedtls
-else ifeq ($(BR2_PACKAGE_LIBGCRYPT),y)
+else ifeq ($(BR2_PACKAGE_LIBSSH2_LIBGCRYPT),y)
 LIBSSH2_DEPENDENCIES += libgcrypt
 LIBSSH2_CONF_OPTS += --with-libgcrypt-prefix=$(STAGING_DIR)/usr \
 	--with-crypto=libgcrypt
 # configure.ac forgets to link to dependent libraries of gcrypt breaking static
 # linking
 LIBSSH2_CONF_ENV += LIBS="`$(STAGING_DIR)/usr/bin/libgcrypt-config --libs`"
-else
+else ifeq ($(BR2_PACKAGE_LIBSSH2_OPENSSL),y)
 LIBSSH2_DEPENDENCIES += openssl
 LIBSSH2_CONF_OPTS += --with-libssl-prefix=$(STAGING_DIR)/usr \
 	--with-crypto=openssl
-- 
1.9.1

[Buildroot] [PATCH v4 1/1] package/libssh2: Add selectable crypto libraries

[Arnout Vandecappelle]

On 01-11-17 17:22, Sam Voss wrote:
> Currently, the selection of the backend is based on a priority order,
> which is not always desirable: not all features are available for all
> backends, as reported upstream:
>     https://github.com/libssh2/libssh2/issues/213

 OK, so finally an indirect explanation for why you need this. For people who
don't want to follow the link, here it is (Sam, correct me if I misinterpreted
the issue): apparently, libgcrypt is not able to use a password-encrypted
certificate generated by openssl. So really, it's a shortcoming of libgcrypt. If
you need to use such a certificate with libssh2, you have no choice but to use
the openssl backend.

 And I can imagine that there will be other situations where one of the backends
is missing some feature - not so much in the crypto supported, but rather in the
fringe aspects like reading certificates or whatnot.

 So yes, it makes sense to make this selectable.

> 
> As such, allow a user to select the backend most appropriate to their
> use-case.
> 
> Signed-off-by: Sam Voss <sam.voss@rockwellcollins.com>
> 
> --
> 
> [v3->v4]
>  - Update configuration for "type->depends->select" ordering
>  - Update patch message to be more descriptive
> 
> [v2->v3]
>  - Fix comment about favoring mbedtls
> 
> [v1->v2]
>  - Do not have comments when crypo is not selected, select it instead.
>  - Do not select OpenSSL by default when libssh2 is selected if no
>    others are chosen
> ---
>  package/libssh2/Config.in  | 24 +++++++++++++++++++++++-
>  package/libssh2/libssh2.mk |  8 ++++----
>  2 files changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/package/libssh2/Config.in b/package/libssh2/Config.in
> index 9b60823..f2d32a9 100644
> --- a/package/libssh2/Config.in
> +++ b/package/libssh2/Config.in
> @@ -1,6 +1,5 @@
>  config BR2_PACKAGE_LIBSSH2
>  	bool "libssh2"
> -	select BR2_PACKAGE_OPENSSL if !(BR2_PACKAGE_MBEDTLS || BR2_PACKAGE_LIBGCRYPT)
>  	help
>  	  libssh2 is a client-side C library implementing the SSH2
>  	  protocol as defined by Internet Drafts: SECSH-TRANS(22),
> @@ -8,3 +7,26 @@ config BR2_PACKAGE_LIBSSH2
>  	  SECSH-FILEXFER(06)*, SECSH-DHGEX(04), and SECSH-NUMBERS(10)
>  
>  	  http://www.libssh2.org/
> +
> +if BR2_PACKAGE_LIBSSH2
> +
> +choice
> +	prompt "Crypto Backend"
> +	help
> +	  Select crypto library to be used in libssh2.
> +
> +config BR2_PACKAGE_LIBSSH2_MBEDTLS
> +	bool "mbedtls"
> +	select BR2_PACKAGE_MBEDTLS

 Note that this changes the defaults we had previously. If openssl was already
selected and you select libssh2, then openssl would be used as a backend. Now,
the default is mbedtls, so if you don't take any action, it will be mbedtls.

 This does affect people updating Buildroot, so it needs to be mentioned in CHANGES.

> +
> +config BR2_PACKAGE_LIBSSH2_LIBGCRYPT
> +	bool "gcrypt"
> +	depends on BR2_PACKAGE_LIBGPG_ERROR_ARCH_SUPPORTS # libgcrypt -> libgpg-error
> +	select BR2_PACKAGE_LIBGCRYPT
> +
> +config BR2_PACKAGE_LIBSSH2_OPENSSL
> +	bool "openssl"
> +	select BR2_PACKAGE_OPENSSL
> +
> +endchoice
> +endif
> diff --git a/package/libssh2/libssh2.mk b/package/libssh2/libssh2.mk
> index d40e844..befac92 100644
> --- a/package/libssh2/libssh2.mk
> +++ b/package/libssh2/libssh2.mk
> @@ -15,19 +15,19 @@ LIBSSH2_CONF_OPTS = --disable-examples-build
>  LIBSSH2_AUTORECONF = YES
>  
>  # Dependency is one of mbedtls, libgcrypt or openssl, guaranteed in
> -# Config.in. Favour mbedtls.

 This comment has now become useless - since the different options come from a
choice, it's obvious that only one of them would be true.


 I've applied with the above changes, and I've also pushed an update to the
CHANGES file.

 Regards,
 Arnout

> -ifeq ($(BR2_PACKAGE_MBEDTLS),y)
> +# Config.in.
> +ifeq ($(BR2_PACKAGE_LIBSSH2_MBEDTLS),y)
>  LIBSSH2_DEPENDENCIES += mbedtls
>  LIBSSH2_CONF_OPTS += --with-libmbedcrypto-prefix=$(STAGING_DIR)/usr \
>  	--with-crypto=mbedtls
> -else ifeq ($(BR2_PACKAGE_LIBGCRYPT),y)
> +else ifeq ($(BR2_PACKAGE_LIBSSH2_LIBGCRYPT),y)
>  LIBSSH2_DEPENDENCIES += libgcrypt
>  LIBSSH2_CONF_OPTS += --with-libgcrypt-prefix=$(STAGING_DIR)/usr \
>  	--with-crypto=libgcrypt
>  # configure.ac forgets to link to dependent libraries of gcrypt breaking static
>  # linking
>  LIBSSH2_CONF_ENV += LIBS="`$(STAGING_DIR)/usr/bin/libgcrypt-config --libs`"
> -else
> +else ifeq ($(BR2_PACKAGE_LIBSSH2_OPENSSL),y)
>  LIBSSH2_DEPENDENCIES += openssl
>  LIBSSH2_CONF_OPTS += --with-libssl-prefix=$(STAGING_DIR)/usr \
>  	--with-crypto=openssl
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH v4 1/1] package/libssh2: Add selectable crypto libraries

[Peter Korsgaard]

>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:

 > On 01-11-17 17:22, Sam Voss wrote:
 >> Currently, the selection of the backend is based on a priority order,
 >> which is not always desirable: not all features are available for all
 >> backends, as reported upstream:
 >> https://github.com/libssh2/libssh2/issues/213

 >  OK, so finally an indirect explanation for why you need this. For people who
 > don't want to follow the link, here it is (Sam, correct me if I misinterpreted
 > the issue): apparently, libgcrypt is not able to use a password-encrypted
 > certificate generated by openssl. So really, it's a shortcoming of libgcrypt. If
 > you need to use such a certificate with libssh2, you have no choice but to use
 > the openssl backend.

 >  And I can imagine that there will be other situations where one of the backends
 > is missing some feature - not so much in the crypto supported, but rather in the
 > fringe aspects like reading certificates or whatnot.

Ahh, yes - That also wasn't clear to me.

>> +++ b/package/libssh2/Config.in
 >> @@ -1,6 +1,5 @@
 >> config BR2_PACKAGE_LIBSSH2
 >> bool "libssh2"
 >> -	select BR2_PACKAGE_OPENSSL if !(BR2_PACKAGE_MBEDTLS || BR2_PACKAGE_LIBGCRYPT)
 >> help
 >> libssh2 is a client-side C library implementing the SSH2
 >> protocol as defined by Internet Drafts: SECSH-TRANS(22),
 >> @@ -8,3 +7,26 @@ config BR2_PACKAGE_LIBSSH2
 >> SECSH-FILEXFER(06)*, SECSH-DHGEX(04), and SECSH-NUMBERS(10)
 >> 
 >> http://www.libssh2.org/
 >> +
 >> +if BR2_PACKAGE_LIBSSH2
 >> +
 >> +choice
 >> +	prompt "Crypto Backend"
 >> +	help
 >> +	  Select crypto library to be used in libssh2.
 >> +
 >> +config BR2_PACKAGE_LIBSSH2_MBEDTLS
 >> +	bool "mbedtls"
 >> +	select BR2_PACKAGE_MBEDTLS

 >  Note that this changes the defaults we had previously. If openssl was already
 > selected and you select libssh2, then openssl would be used as a backend. Now,
 > the default is mbedtls, so if you don't take any action, it will be mbedtls.

 >  This does affect people updating Buildroot, so it needs to be mentioned in CHANGES.

Why don't we just keep the old logic instead? E.G. leave the select
openssl if !(mbedtls || libgcrypt) and then change the selects to
depends on in the choice:

choice

config BR2_PACKAGE_LIBSSH2_MBEDTLS
       bool "mbedtls"
       depends on BR2_PACKAGE_MBEDTLS

...

Doesn't that give us the best of both worlds? Same behaviour as before
by default and still the option to explicitly chose when multiple
backends are available?

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v4 1/1] package/libssh2: Add selectable crypto libraries

[Yann E. MORIN]

Peter, All,

On 2017-11-05 09:01 +0100, Peter Korsgaard spake thusly:
> >>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:
>  > On 01-11-17 17:22, Sam Voss wrote:
[--SNIP--]
> >> +++ b/package/libssh2/Config.in
>  >> @@ -1,6 +1,5 @@
>  >> config BR2_PACKAGE_LIBSSH2
>  >> bool "libssh2"
>  >> -	select BR2_PACKAGE_OPENSSL if !(BR2_PACKAGE_MBEDTLS || BR2_PACKAGE_LIBGCRYPT)
>  >> help
>  >> libssh2 is a client-side C library implementing the SSH2
>  >> protocol as defined by Internet Drafts: SECSH-TRANS(22),
>  >> @@ -8,3 +7,26 @@ config BR2_PACKAGE_LIBSSH2
>  >> SECSH-FILEXFER(06)*, SECSH-DHGEX(04), and SECSH-NUMBERS(10)
>  >> 
>  >> http://www.libssh2.org/
>  >> +
>  >> +if BR2_PACKAGE_LIBSSH2
>  >> +
>  >> +choice
>  >> +	prompt "Crypto Backend"
>  >> +	help
>  >> +	  Select crypto library to be used in libssh2.
>  >> +
>  >> +config BR2_PACKAGE_LIBSSH2_MBEDTLS
>  >> +	bool "mbedtls"
>  >> +	select BR2_PACKAGE_MBEDTLS
> 
>  >  Note that this changes the defaults we had previously. If openssl was already
>  > selected and you select libssh2, then openssl would be used as a backend. Now,
>  > the default is mbedtls, so if you don't take any action, it will be mbedtls.
> 
>  >  This does affect people updating Buildroot, so it needs to be mentioned in CHANGES.
> 
> Why don't we just keep the old logic instead? E.G. leave the select
> openssl if !(mbedtls || libgcrypt) and then change the selects to
> depends on in the choice:
> 
> choice
> 
> config BR2_PACKAGE_LIBSSH2_MBEDTLS
>        bool "mbedtls"
>        depends on BR2_PACKAGE_MBEDTLS
> 
> ...
> 
> Doesn't that give us the best of both worlds? Same behaviour as before
> by default and still the option to explicitly chose when multiple
> backends are available?

That's what Sam did in the initial iterations of his patchset, but
Arnout and Thomas (IIRC) both requested he replaces that with a select
instead.

Regards,
Yann E. MORIN.

> -- 
> Bye, Peter Korsgaard
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH v4 1/1] package/libssh2: Add selectable crypto libraries

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

Hi,

 >> >  This does affect people updating Buildroot, so it needs to be mentioned in CHANGES.
 >> 
 >> Why don't we just keep the old logic instead? E.G. leave the select
 >> openssl if !(mbedtls || libgcrypt) and then change the selects to
 >> depends on in the choice:
 >> 
 >> choice
 >> 
 >> config BR2_PACKAGE_LIBSSH2_MBEDTLS
 >> bool "mbedtls"
 >> depends on BR2_PACKAGE_MBEDTLS
 >> 
 >> ...
 >> 
 >> Doesn't that give us the best of both worlds? Same behaviour as before
 >> by default and still the option to explicitly chose when multiple
 >> backends are available?

 > That's what Sam did in the initial iterations of his patchset, but
 > Arnout and Thomas (IIRC) both requested he replaces that with a select
 > instead.

Hmm, I could only find Arnouts mail asking to use select, but he didn't
write why. Arnout, why not use depends on here?

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v4 1/1] package/libssh2: Add selectable crypto libraries

[Arnout Vandecappelle]

On 05-11-17 11:40, Peter Korsgaard wrote:
>>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:
> 
> Hi,
> 
>  >> >  This does affect people updating Buildroot, so it needs to be mentioned in CHANGES.
>  >> 
>  >> Why don't we just keep the old logic instead? E.G. leave the select
>  >> openssl if !(mbedtls || libgcrypt) and then change the selects to
>  >> depends on in the choice:
>  >> 
>  >> choice
>  >> 
>  >> config BR2_PACKAGE_LIBSSH2_MBEDTLS
>  >> bool "mbedtls"
>  >> depends on BR2_PACKAGE_MBEDTLS
>  >> 
>  >> ...
>  >> 
>  >> Doesn't that give us the best of both worlds? Same behaviour as before
>  >> by default and still the option to explicitly chose when multiple
>  >> backends are available?
> 
>  > That's what Sam did in the initial iterations of his patchset, but
>  > Arnout and Thomas (IIRC) both requested he replaces that with a select
>  > instead.
> 
> Hmm, I could only find Arnouts mail asking to use select, but he didn't
> write why. Arnout, why not use depends on here?

 I was just applying the default principle that you should use select rather
than depends unless there is a good reason to use depends. I hadn't thought of
backward compatibility back then, and I didn't think of using depends now.

 I've already committed, can you fix it up?

 Regards,
 Arnout

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH v4 1/1] package/libssh2: Add selectable crypto libraries

[Yann E. MORIN]

Arnout, All,

On 2017-11-05 13:46 +0100, Arnout Vandecappelle spake thusly:
> On 05-11-17 11:40, Peter Korsgaard wrote:
> >>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:
> >  >> >  This does affect people updating Buildroot, so it needs to be mentioned in CHANGES.
> >  >> 
> >  >> Why don't we just keep the old logic instead? E.G. leave the select
> >  >> openssl if !(mbedtls || libgcrypt) and then change the selects to
> >  >> depends on in the choice:
> >  >> 
> >  >> choice
> >  >> 
> >  >> config BR2_PACKAGE_LIBSSH2_MBEDTLS
> >  >> bool "mbedtls"
> >  >> depends on BR2_PACKAGE_MBEDTLS
> >  >> 
> >  >> ...
> >  >> 
> >  >> Doesn't that give us the best of both worlds? Same behaviour as before
> >  >> by default and still the option to explicitly chose when multiple
> >  >> backends are available?
> > 
> >  > That's what Sam did in the initial iterations of his patchset, but
> >  > Arnout and Thomas (IIRC) both requested he replaces that with a select
> >  > instead.
> > 
> > Hmm, I could only find Arnouts mail asking to use select, but he didn't
> > write why. Arnout, why not use depends on here?
> 
>  I was just applying the default principle that you should use select rather
> than depends unless there is a good reason to use depends. I hadn't thought of
> backward compatibility back then, and I didn't think of using depends now.
> 
>  I've already committed, can you fix it up?

I think that using select is better, so we should keep it as-is, even
though that means backward incompatibility.

After all, a user should review their configuration pon updating
Buildroot.

Note: this exposes a flaw in the way we handle legacy: it can only take
care of options that were removed, and can not account for new options
like this case. There is nothing we can do about this, though... :-/

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH v4 1/1] package/libssh2: Add selectable crypto libraries

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 >> > Hmm, I could only find Arnouts mail asking to use select, but he didn't
 >> > write why. Arnout, why not use depends on here?
 >> 
 >> I was just applying the default principle that you should use select rather
 >> than depends unless there is a good reason to use depends. I hadn't thought of
 >> backward compatibility back then, and I didn't think of using depends now.
 >> 
 >> I've already committed, can you fix it up?

 > I think that using select is better, so we should keep it as-is, even
 > though that means backward incompatibility.

Really? I do think the new behaviour is less nice than what we
had. E.G. imagine that you have openssl enabled (which is very likely,
given the number of packages using it) and you enable some package that
pulls in libssh2. In the past libssh2 used the openssl backend and all
was fine, but now you end up with mbedtls as well and libssh2 using it
instead of the "better" openssl backend.

I don't really think that is progress.

 > After all, a user should review their configuration pon updating
 > Buildroot.

It is not just existing users, it is also any future libssh2 users.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v4 1/1] package/libssh2: Add selectable crypto libraries

[Peter Korsgaard]

>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:

Hi,

 >> Hmm, I could only find Arnouts mail asking to use select, but he didn't
 >> write why. Arnout, why not use depends on here?

 >  I was just applying the default principle that you should use select rather
 > than depends unless there is a good reason to use depends. I hadn't thought of
 > backward compatibility back then, and I didn't think of using depends now.

 >  I've already committed, can you fix it up?

Sure!

-- 
Bye, Peter Korsgaard