| Message ID | 1509553376-43168-1-git-send-email-sam.voss@rockwellcollins.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v4,1/1] package/libssh2: Add selectable crypto libraries | expand |
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" == 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?
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@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
>>>>> "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?
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, 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" == 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.
>>>>> "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!
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
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(-)