| Message ID | 1509054225-19795-1-git-send-email-sam.voss@rockwellcollins.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2,1/2] package/libssh2: Add selectable crypto libraries | expand |
Sam, All, On 2017-10-26 16:43 -0500, Sam Voss spake thusly: > Add functionality to allow crypto libraries for libssh2 to be selectable > by a choice instead of a fallback that may not work in all cases. > Previous fallback is maintained from within the "defaults" of the choice > menu, but can be overridden by making a choice. > > This fixes issues where two crypto libraries are present on the system, > but the fallback order picks the wrong one. > > Signed-off-by: Sam Voss <sam.voss@rockwellcollins.com> > --- > package/libssh2/Config.in | 34 ++++++++++++++++++++++++++++++++++ > package/libssh2/libssh2.mk | 8 ++++---- > 2 files changed, 38 insertions(+), 4 deletions(-) > > diff --git a/package/libssh2/Config.in b/package/libssh2/Config.in > index 9b60823..61be054 100644 > --- a/package/libssh2/Config.in > +++ b/package/libssh2/Config.in > @@ -8,3 +8,37 @@ 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 Library" > + default BR2_PACKAGE_LIBSSH2_MBEDTLS if BR2_PACKAGE_MBEDTLS > + default BR2_PACKAGE_LIBSSH2_LIBGCRYPT if BR2_PACKAGE_LIBGCRYPT > + default BR2_PACKAGE_OPENSSL All those 'default' lines are useless. A choice always defaults to the first option which dependencies are fulfilled. > + help > + Select crypto library to be used in libssh2. > + > +config BR2_PACKAGE_LIBSSH2_MBEDTLS > + depends on BR2_PACKAGE_MBEDTLS > + bool "mbedtls" > + > +comment "mbedtls not selected" > + depends on !BR2_PACKAGE_MBEDTLS Don't add those comments. > +config BR2_PACKAGE_LIBSSH2_LIBGCRYPT > + depends on BR2_PACKAGE_LIBGCRYPT > + bool "gcrypt" > + > +comment "libgcrypt not selected" > + depends on !BR2_PACKAGE_LIBGCRYPT So this patch does two things: 1- make the backend selectable; 2- add libgcrypt as a backend. This should be two patches. Regards, Yann E. MORIN. > +config BR2_PACKAGE_LIBSSH2_OPENSSL > + depends on BR2_PACKAGE_OPENSSL > + bool "openssl" > + > +comment "openssl not selected" > + depends on !BR2_PACKAGE_OPENSSL > + > +endchoice > +endif > diff --git a/package/libssh2/libssh2.mk b/package/libssh2/libssh2.mk > index dedb890..3978698 100644 > --- a/package/libssh2/libssh2.mk > +++ b/package/libssh2/libssh2.mk > @@ -12,20 +12,20 @@ LIBSSH2_INSTALL_STAGING = YES > LIBSSH2_CONF_OPTS = --disable-examples-build > > # 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-mbedtls=$(STAGING_DIR)/usr \ > --without-openssl --without-libgcrypt > LIBSSH2_CONF_ENV += ac_cv_libgcrypt=no > -else ifeq ($(BR2_PACKAGE_LIBGCRYPT),y) > +else ifeq ($(BR2_PACKAGE_LIBSSH2_LIBGCRYPT),y) > LIBSSH2_DEPENDENCIES += libgcrypt > LIBSSH2_CONF_OPTS += --with-libgcrypt=$(STAGING_DIR)/usr \ > --without-openssl --without-mbedtls > # 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-openssl \ > --with-libssl-prefix=$(STAGING_DIR)/usr \ > -- > 1.9.1 > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
Hi Sam, On Thu, Oct 26, 2017 at 04:43:44PM -0500, Sam Voss wrote: > Add functionality to allow crypto libraries for libssh2 to be selectable > by a choice instead of a fallback that may not work in all cases. > Previous fallback is maintained from within the "defaults" of the choice > menu, but can be overridden by making a choice. > > This fixes issues where two crypto libraries are present on the system, > but the fallback order picks the wrong one. > > Signed-off-by: Sam Voss <sam.voss@rockwellcollins.com> > --- [...] > diff --git a/package/libssh2/libssh2.mk b/package/libssh2/libssh2.mk > index dedb890..3978698 100644 > --- a/package/libssh2/libssh2.mk > +++ b/package/libssh2/libssh2.mk > @@ -12,20 +12,20 @@ LIBSSH2_INSTALL_STAGING = YES > LIBSSH2_CONF_OPTS = --disable-examples-build > > # 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-mbedtls=$(STAGING_DIR)/usr \ > --without-openssl --without-libgcrypt > LIBSSH2_CONF_ENV += ac_cv_libgcrypt=no > -else ifeq ($(BR2_PACKAGE_LIBGCRYPT),y) > +else ifeq ($(BR2_PACKAGE_LIBSSH2_LIBGCRYPT),y) > LIBSSH2_DEPENDENCIES += libgcrypt > LIBSSH2_CONF_OPTS += --with-libgcrypt=$(STAGING_DIR)/usr \ > --without-openssl --without-mbedtls > # 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-openssl \ > --with-libssl-prefix=$(STAGING_DIR)/usr \ Have you tested combination of options after the version bump in the next patch? The master libssh2 branch changes the configure crypto selection options to --with-crypto=auto|openssl|libgcrypt|mbedtls. You should either take this into account in the version bump patch, or (preferably, IMO) reverse the patches order, change to --with-crypto in the version bump patch, and add selectable crypto backend in the second. baruch
Yann, All, (sorry for repeat Yann, miss clicked reply instead of all). On Sat, Oct 28, 2017 at 9:16 AM, Yann E. MORIN <yann.morin.1998@free.fr> wrote: > Sam, All, > > On 2017-10-26 16:43 -0500, Sam Voss spake thusly: [..] >> + >> +choice >> + prompt "Crypto Library" >> + default BR2_PACKAGE_LIBSSH2_MBEDTLS if BR2_PACKAGE_MBEDTLS >> + default BR2_PACKAGE_LIBSSH2_LIBGCRYPT if BR2_PACKAGE_LIBGCRYPT >> + default BR2_PACKAGE_OPENSSL > > All those 'default' lines are useless. A choice always defaults to the > first option which dependencies are fulfilled. Interesting, I didn't know that. Great, saves some redundancy, I'll get those removed. >> +comment "mbedtls not selected" >> + depends on !BR2_PACKAGE_MBEDTLS > > Don't add those comments. I had done this based off of other packages doing something similar when requirements aren't met (to show availability). Do we only show comments on main packages, and not sub choices? Just so I know next time. >>> +config BR2_PACKAGE_LIBSSH2_LIBGCRYPT >> + depends on BR2_PACKAGE_LIBGCRYPT >> + bool "gcrypt" >> + >> +comment "libgcrypt not selected" >> + depends on !BR2_PACKAGE_LIBGCRYPT > > So this patch does two things: > 1- make the backend selectable; > 2- add libgcrypt as a backend. > > This should be two patches. Libgcrypt was always an available crypto, and was not added in this patchset.
Baruch, All, On Mon, Oct 30, 2017 at 12:19 AM, Baruch Siach <baruch@tkos.co.il> wrote: [...] > Have you tested combination of options after the version bump in the next > patch? Could you elaborate on this? I don't understand what you mean by combination of options. > > The master libssh2 branch changes the configure crypto selection options to > --with-crypto=auto|openssl|libgcrypt|mbedtls. You should either take this into > account in the version bump patch, or (preferably, IMO) reverse the patches > order, change to --with-crypto in the version bump patch, and add selectable > crypto backend in the second. Good catch, I didn't catch this in the diffs. I will look into these options. Sam
Hi Sam, On Mon, Oct 30, 2017 at 09:27:17AM -0500, Sam Voss wrote: > On Mon, Oct 30, 2017 at 12:19 AM, Baruch Siach <baruch@tkos.co.il> wrote: > [...] > > Have you tested combination of options after the version bump in the next > > patch? > > Could you elaborate on this? I don't understand what you mean by > combination of options. When you have for example both openssl and mbedtls enabled, the --with-[backend] options will no longer work after the version bump. So the crypto backend selection logic will have no effect, so you might end up with the wrong backend being used. When testing these patches try enabling multiple crypto packages, and make sure that only the backend you selected for libssh2 is the one that is actually used. > > The master libssh2 branch changes the configure crypto selection options to > > --with-crypto=auto|openssl|libgcrypt|mbedtls. You should either take this into > > account in the version bump patch, or (preferably, IMO) reverse the patches > > order, change to --with-crypto in the version bump patch, and add selectable > > crypto backend in the second. > > Good catch, I didn't catch this in the diffs. I will look into these options. baruch
diff --git a/package/libssh2/Config.in b/package/libssh2/Config.in index 9b60823..61be054 100644 --- a/package/libssh2/Config.in +++ b/package/libssh2/Config.in @@ -8,3 +8,37 @@ 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 Library" + default BR2_PACKAGE_LIBSSH2_MBEDTLS if BR2_PACKAGE_MBEDTLS + default BR2_PACKAGE_LIBSSH2_LIBGCRYPT if BR2_PACKAGE_LIBGCRYPT + default BR2_PACKAGE_OPENSSL + help + Select crypto library to be used in libssh2. + +config BR2_PACKAGE_LIBSSH2_MBEDTLS + depends on BR2_PACKAGE_MBEDTLS + bool "mbedtls" + +comment "mbedtls not selected" + depends on !BR2_PACKAGE_MBEDTLS + +config BR2_PACKAGE_LIBSSH2_LIBGCRYPT + depends on BR2_PACKAGE_LIBGCRYPT + bool "gcrypt" + +comment "libgcrypt not selected" + depends on !BR2_PACKAGE_LIBGCRYPT + +config BR2_PACKAGE_LIBSSH2_OPENSSL + depends on BR2_PACKAGE_OPENSSL + bool "openssl" + +comment "openssl not selected" + depends on !BR2_PACKAGE_OPENSSL + +endchoice +endif diff --git a/package/libssh2/libssh2.mk b/package/libssh2/libssh2.mk index dedb890..3978698 100644 --- a/package/libssh2/libssh2.mk +++ b/package/libssh2/libssh2.mk @@ -12,20 +12,20 @@ LIBSSH2_INSTALL_STAGING = YES LIBSSH2_CONF_OPTS = --disable-examples-build # 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-mbedtls=$(STAGING_DIR)/usr \ --without-openssl --without-libgcrypt LIBSSH2_CONF_ENV += ac_cv_libgcrypt=no -else ifeq ($(BR2_PACKAGE_LIBGCRYPT),y) +else ifeq ($(BR2_PACKAGE_LIBSSH2_LIBGCRYPT),y) LIBSSH2_DEPENDENCIES += libgcrypt LIBSSH2_CONF_OPTS += --with-libgcrypt=$(STAGING_DIR)/usr \ --without-openssl --without-mbedtls # 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-openssl \ --with-libssl-prefix=$(STAGING_DIR)/usr \
Add functionality to allow crypto libraries for libssh2 to be selectable by a choice instead of a fallback that may not work in all cases. Previous fallback is maintained from within the "defaults" of the choice menu, but can be overridden by making a choice. This fixes issues where two crypto libraries are present on the system, but the fallback order picks the wrong one. Signed-off-by: Sam Voss <sam.voss@rockwellcollins.com> --- package/libssh2/Config.in | 34 ++++++++++++++++++++++++++++++++++ package/libssh2/libssh2.mk | 8 ++++---- 2 files changed, 38 insertions(+), 4 deletions(-)