[Buildroot] [PATCHv4] package/uacme: requires TLS support in libcurl

[Buildroot] [PATCHv4] package/uacme: requires TLS support in libcurl

[Yann E. MORIN]

From: Baruch Siach <baruch@tkos.co.il>

uacme configure script fails when libcurl does not support TLS. This
means that BR2_PACKAGE_LIBCURL_TLS_NONE is incompatible with uacme.

Add a kconfig knob to libcurl so that no_TLS is not an option. Select
that from uacme.

Fixes:
http://autobuild.buildroot.net/results/4e16f1d958ac3d30e26e7f17bdffc47834b0e2bd/
http://autobuild.buildroot.net/results/4e16f1d958ac3d30e26e7f17bdffc47834b0e2bd/
http://autobuild.buildroot.net/results/25280409b32282b4dd40b1e88127051439380f3d/

Cc: Nicola Di Lieto <nicola.dilieto@gmail.com>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
[yann.morin.1998@free.fr:
  - keep the current forward select
  - add the kconfig knob
]
Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>

---
v4 (Yann E. MORIN)
  Restore forward select
  Add the force-ssl-tls kconfig knob; use it from uacme

v3:
  Move comments up to fix suboption indentation (Yann)
  Add missing MMU comment dependency (Yann)

v2:
  Add dependency on crypto back end for uacme itself (Nicola Di Lieto)
---
 package/libcurl/Config.in | 5 +++++
 package/uacme/Config.in   | 1 +
 2 files changed, 6 insertions(+)

diff --git a/package/libcurl/Config.in b/package/libcurl/Config.in
index 3381decca8..a3148e086d 100644
--- a/package/libcurl/Config.in
+++ b/package/libcurl/Config.in
@@ -45,6 +45,10 @@ config BR2_PACKAGE_LIBCURL_EXTRA_PROTOCOLS_FEATURES
 	  - DICT
 	  - Gopher
 
+# Packages must select that if they require a SSL/TLS-enabled libcurl
+config BR2_PACKAGE_LIBCURL_FORCE_SSL_TLS
+	bool
+
 choice
 	prompt "SSL/TLS library to use"
 
@@ -77,6 +81,7 @@ comment "WolfSSL needs a toolchain w/ dynamic library"
 
 config BR2_PACKAGE_LIBCURL_TLS_NONE
 	bool "None"
+	depends on !BR2_PACKAGE_LIBCURL_FORCE_SSL_TLS
 
 endchoice
 
diff --git a/package/uacme/Config.in b/package/uacme/Config.in
index 58b7c534e7..1458e74d28 100644
--- a/package/uacme/Config.in
+++ b/package/uacme/Config.in
@@ -3,6 +3,7 @@ config BR2_PACKAGE_UACME
 	depends on BR2_USE_MMU # fork()
 	select BR2_PACKAGE_OPENSSL if !(BR2_PACKAGE_GNUTLS || BR2_PACKAGE_MBEDTLS)
 	select BR2_PACKAGE_LIBCURL
+	select BR2_PACKAGE_LIBCURL_FORCE_SSL_TLS
 	help
 	  uacme is a client for the ACMEv2 protocol described in
 	  RFC8555, written in plain C with minimal dependencies
-- 
2.25.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCHv4] package/uacme: requires TLS support in libcurl

[Baruch Siach via buildroot]

Hi Yann,

On Sun, Jul 17 2022, Yann E. MORIN wrote:
> From: Baruch Siach <baruch@tkos.co.il>
>
> uacme configure script fails when libcurl does not support TLS. This
> means that BR2_PACKAGE_LIBCURL_TLS_NONE is incompatible with uacme.
>
> Add a kconfig knob to libcurl so that no_TLS is not an option. Select
> that from uacme.

Looks much more elegant. Thanks.

Some comments below.

> Fixes:
> http://autobuild.buildroot.net/results/4e16f1d958ac3d30e26e7f17bdffc47834b0e2bd/
> http://autobuild.buildroot.net/results/4e16f1d958ac3d30e26e7f17bdffc47834b0e2bd/
> http://autobuild.buildroot.net/results/25280409b32282b4dd40b1e88127051439380f3d/
>
> Cc: Nicola Di Lieto <nicola.dilieto@gmail.com>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> [yann.morin.1998@free.fr:
>   - keep the current forward select
>   - add the kconfig knob
> ]
> Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
>
> ---
> v4 (Yann E. MORIN)
>   Restore forward select
>   Add the force-ssl-tls kconfig knob; use it from uacme
>
> v3:
>   Move comments up to fix suboption indentation (Yann)
>   Add missing MMU comment dependency (Yann)
>
> v2:
>   Add dependency on crypto back end for uacme itself (Nicola Di Lieto)
> ---
>  package/libcurl/Config.in | 5 +++++
>  package/uacme/Config.in   | 1 +
>  2 files changed, 6 insertions(+)
>
> diff --git a/package/libcurl/Config.in b/package/libcurl/Config.in
> index 3381decca8..a3148e086d 100644
> --- a/package/libcurl/Config.in
> +++ b/package/libcurl/Config.in
> @@ -45,6 +45,10 @@ config BR2_PACKAGE_LIBCURL_EXTRA_PROTOCOLS_FEATURES
>  	  - DICT
>  	  - Gopher
>  
> +# Packages must select that if they require a SSL/TLS-enabled libcurl

Said package must also select one of the crypto back ends that libcurl
supports. This part is somewhat fragile as libcurl might remove support
for any given back end like it recently did for NSS.

> +config BR2_PACKAGE_LIBCURL_FORCE_SSL_TLS

[Bikeshed] Why not just BR2_PACKAGE_LIBCURL_FORCE_TLS ?

baruch

> +	bool
> +
>  choice
>  	prompt "SSL/TLS library to use"
>  
> @@ -77,6 +81,7 @@ comment "WolfSSL needs a toolchain w/ dynamic library"
>  
>  config BR2_PACKAGE_LIBCURL_TLS_NONE
>  	bool "None"
> +	depends on !BR2_PACKAGE_LIBCURL_FORCE_SSL_TLS
>  
>  endchoice
>  
> diff --git a/package/uacme/Config.in b/package/uacme/Config.in
> index 58b7c534e7..1458e74d28 100644
> --- a/package/uacme/Config.in
> +++ b/package/uacme/Config.in
> @@ -3,6 +3,7 @@ config BR2_PACKAGE_UACME
>  	depends on BR2_USE_MMU # fork()
>  	select BR2_PACKAGE_OPENSSL if !(BR2_PACKAGE_GNUTLS || BR2_PACKAGE_MBEDTLS)
>  	select BR2_PACKAGE_LIBCURL
> +	select BR2_PACKAGE_LIBCURL_FORCE_SSL_TLS
>  	help
>  	  uacme is a client for the ACMEv2 protocol described in
>  	  RFC8555, written in plain C with minimal dependencies


-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCHv4] package/uacme: requires TLS support in libcurl

[Yann E. MORIN]

Baruch, All,

On 2022-07-17 22:41 +0300, Baruch Siach via buildroot spake thusly:
> On Sun, Jul 17 2022, Yann E. MORIN wrote:
> > From: Baruch Siach <baruch@tkos.co.il>
> >
> > uacme configure script fails when libcurl does not support TLS. This
> > means that BR2_PACKAGE_LIBCURL_TLS_NONE is incompatible with uacme.
> >
> > Add a kconfig knob to libcurl so that no_TLS is not an option. Select
> > that from uacme.
> Looks much more elegant. Thanks.

Cool! If you prefer my patch, I'll let you mark yours as superseded in
patchwork, then. Otherwise, I'll let another maintainer pick their
preferred one. :-)

[--SNIP--]
> > +# Packages must select that if they require a SSL/TLS-enabled libcurl
> Said package must also select one of the crypto back ends that libcurl
> supports.

Absolutely valid point.

> This part is somewhat fragile as libcurl might remove support
> for any given back end like it recently did for NSS.

I guess openssl will always be a safe default, as it has no architecture
dependency. However, that would need further change in libcurl, such as:

    @@ -47,10 +47,11 @@ config BR2_PACKAGE_LIBCURL_EXTRA_PROTOCOLS_FEATURES

     choice
            prompt "SSL/TLS library to use"
    +       default BR2_PACKAGE_LIBCURL_TLS_NONE

     config BR2_PACKAGE_LIBCURL_OPENSSL
            bool "OpenSSL"
    -       depends on BR2_PACKAGE_OPENSSL
    +       select BR2_PACKAGE_OPENSSL
            select BR2_PACKAGE_LIBOPENSSL_ENABLE_DES if BR2_PACKAGE_LIBOPENSSL

     config BR2_PACKAGE_LIBCURL_BEARSSL

which changes the way we handle crypto backends...

Maybe just not default the choice to BR2_PACKAGE_LIBCURL_TLS_NONE...
After all, we want to promote best practices, and enabled TLS in libcurl
is better than not enabling it... Meh...

Not sure what's the best, and it is starting to become more complex than
my quickly whipped-up patch...

> > +config BR2_PACKAGE_LIBCURL_FORCE_SSL_TLS
> [Bikeshed] Why not just BR2_PACKAGE_LIBCURL_FORCE_TLS ?

The prompt of the choice is "SSL/TLS library to use" so I reflected that
in the symbol name, although I do agree that SSL is something we should
definitely forget about! :-)

If you feel so-inclined, you can grab this patch and adapt it to ensure
a crypto backend is always enabled. Otherwise, I'll try to see what I
can o a bit later...

Thanks for the quick feedback! :-)

Regards,
Yann E. MORIN.

> baruch
> 
> > +	bool
> > +
> >  choice
> >  	prompt "SSL/TLS library to use"
> >  
> > @@ -77,6 +81,7 @@ comment "WolfSSL needs a toolchain w/ dynamic library"
> >  
> >  config BR2_PACKAGE_LIBCURL_TLS_NONE
> >  	bool "None"
> > +	depends on !BR2_PACKAGE_LIBCURL_FORCE_SSL_TLS
> >  
> >  endchoice
> >  
> > diff --git a/package/uacme/Config.in b/package/uacme/Config.in
> > index 58b7c534e7..1458e74d28 100644
> > --- a/package/uacme/Config.in
> > +++ b/package/uacme/Config.in
> > @@ -3,6 +3,7 @@ config BR2_PACKAGE_UACME
> >  	depends on BR2_USE_MMU # fork()
> >  	select BR2_PACKAGE_OPENSSL if !(BR2_PACKAGE_GNUTLS || BR2_PACKAGE_MBEDTLS)
> >  	select BR2_PACKAGE_LIBCURL
> > +	select BR2_PACKAGE_LIBCURL_FORCE_SSL_TLS
> >  	help
> >  	  uacme is a client for the ACMEv2 protocol described in
> >  	  RFC8555, written in plain C with minimal dependencies
> 
> 
> -- 
>                                                      ~. .~   Tk Open Systems
> =}------------------------------------------------ooO--U--Ooo------------{=
>    - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCHv4] package/uacme: requires TLS support in libcurl

[Baruch Siach via buildroot]

Hi Yann,

On Sun, Jul 17 2022, Yann E. MORIN wrote:
> On 2022-07-17 22:41 +0300, Baruch Siach via buildroot spake thusly:
>> On Sun, Jul 17 2022, Yann E. MORIN wrote:
>> > From: Baruch Siach <baruch@tkos.co.il>
>> >
>> > uacme configure script fails when libcurl does not support TLS. This
>> > means that BR2_PACKAGE_LIBCURL_TLS_NONE is incompatible with uacme.
>> >
>> > Add a kconfig knob to libcurl so that no_TLS is not an option. Select
>> > that from uacme.
>> Looks much more elegant. Thanks.
>
> Cool! If you prefer my patch, I'll let you mark yours as superseded in
> patchwork, then. Otherwise, I'll let another maintainer pick their
> preferred one. :-)
>
> [--SNIP--]
>> > +# Packages must select that if they require a SSL/TLS-enabled libcurl
>> Said package must also select one of the crypto back ends that libcurl
>> supports.
>
> Absolutely valid point.
>
>> This part is somewhat fragile as libcurl might remove support
>> for any given back end like it recently did for NSS.
>
> I guess openssl will always be a safe default, as it has no architecture
> dependency. However, that would need further change in libcurl, such as:
>
>     @@ -47,10 +47,11 @@ config BR2_PACKAGE_LIBCURL_EXTRA_PROTOCOLS_FEATURES
>
>      choice
>             prompt "SSL/TLS library to use"
>     +       default BR2_PACKAGE_LIBCURL_TLS_NONE
>
>      config BR2_PACKAGE_LIBCURL_OPENSSL
>             bool "OpenSSL"
>     -       depends on BR2_PACKAGE_OPENSSL
>     +       select BR2_PACKAGE_OPENSSL
>             select BR2_PACKAGE_LIBOPENSSL_ENABLE_DES if BR2_PACKAGE_LIBOPENSSL
>
>      config BR2_PACKAGE_LIBCURL_BEARSSL
>
> which changes the way we handle crypto backends...

Maybe, if we go down this path of 'depends -> select' for all other
libcurl crypto backends, we can solve the original uacme problem with a
simple !BR2_PACKAGE_LIBCURL_TLS_NONE dependency without recursion. Is
that correct?

But I'm not sure what can of recursion worms that would open.

I only meant to say that the comment above should mention that the
package must select a crypto backend. uacme is a special case and it
already selects a crypto backend.  BR2_PACKAGE_LIBCURL_FORCE_SSL_TLS use
is unlikely to become very common in the foreseeable future. So I don't
think we need to optimize of this corner case.

> Maybe just not default the choice to BR2_PACKAGE_LIBCURL_TLS_NONE...
> After all, we want to promote best practices, and enabled TLS in libcurl
> is better than not enabling it... Meh...

I think this is too heavy handed. TLS takes much more than a crypto
backend after all. It should be an explicit user choice. And if we do
default to crypto enabled, then a more lightweight option is probably
better.

>
> Not sure what's the best, and it is starting to become more complex than
> my quickly whipped-up patch...
>
>> > +config BR2_PACKAGE_LIBCURL_FORCE_SSL_TLS
>> [Bikeshed] Why not just BR2_PACKAGE_LIBCURL_FORCE_TLS ?
>
> The prompt of the choice is "SSL/TLS library to use" so I reflected that
> in the symbol name, although I do agree that SSL is something we should
> definitely forget about! :-)
>
> If you feel so-inclined, you can grab this patch and adapt it to ensure
> a crypto backend is always enabled. Otherwise, I'll try to see what I
> can o a bit later...

I'm fine with the patch as is.

baruch

> Thanks for the quick feedback! :-)
>
> Regards,
> Yann E. MORIN.
>
>> baruch
>> 
>> > +	bool
>> > +
>> >  choice
>> >  	prompt "SSL/TLS library to use"
>> >  
>> > @@ -77,6 +81,7 @@ comment "WolfSSL needs a toolchain w/ dynamic library"
>> >  
>> >  config BR2_PACKAGE_LIBCURL_TLS_NONE
>> >  	bool "None"
>> > +	depends on !BR2_PACKAGE_LIBCURL_FORCE_SSL_TLS
>> >  
>> >  endchoice
>> >  
>> > diff --git a/package/uacme/Config.in b/package/uacme/Config.in
>> > index 58b7c534e7..1458e74d28 100644
>> > --- a/package/uacme/Config.in
>> > +++ b/package/uacme/Config.in
>> > @@ -3,6 +3,7 @@ config BR2_PACKAGE_UACME
>> >  	depends on BR2_USE_MMU # fork()
>> >  	select BR2_PACKAGE_OPENSSL if !(BR2_PACKAGE_GNUTLS || BR2_PACKAGE_MBEDTLS)
>> >  	select BR2_PACKAGE_LIBCURL
>> > +	select BR2_PACKAGE_LIBCURL_FORCE_SSL_TLS
>> >  	help
>> >  	  uacme is a client for the ACMEv2 protocol described in
>> >  	  RFC8555, written in plain C with minimal dependencies

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCHv4] package/uacme: requires TLS support in libcurl

[Yann E. MORIN]

Baruch, All,

On 2022-07-18 06:38 +0300, Baruch Siach via buildroot spake thusly:
> On Sun, Jul 17 2022, Yann E. MORIN wrote:
> > On 2022-07-17 22:41 +0300, Baruch Siach via buildroot spake thusly:
[--SNIP--]
> >> This part is somewhat fragile as libcurl might remove support
> >> for any given back end like it recently did for NSS.
> > I guess openssl will always be a safe default, as it has no architecture
> > dependency. However, that would need further change in libcurl, such as:
[--SNIP--]
> Maybe, if we go down this path of 'depends -> select' for all other
> libcurl crypto backends, we can solve the original uacme problem with a
> simple !BR2_PACKAGE_LIBCURL_TLS_NONE dependency without recursion. Is
> that correct?

Probably. I was wondering why the current choice was using depends on
rathwer than select, as that would have been the most logical solution,
but the commit message does not explain it.

Probably this was done to avoid propagating all the reverse
dependencies...

> But I'm not sure what can of recursion worms that would open.

I was also wondering the same...

> I only meant to say that the comment above should mention that the
> package must select a crypto backend.

Yes, this could also do the trick, but it is not nice that a pakcage
that does not have to use crypto for itself be in charge of selecting a
crypto backend for libcurl... This does not look nice.

> uacme is a special case and it
> already selects a crypto backend.

Yes, and I indeed leverage that condition to introduce _FORCE_SSL_TLS

> BR2_PACKAGE_LIBCURL_FORCE_SSL_TLS use
> is unlikely to become very common in the foreseeable future. So I don't
> think we need to optimize of this corner case.

Valid.

[--SNIP--]
> > If you feel so-inclined, you can grab this patch and adapt it to ensure
> > a crypto backend is always enabled. Otherwise, I'll try to see what I
> > can o a bit later...
> I'm fine with the patch as is.

Ok, thanks for the feedback! :-)

I'll send an updated patch that tweaks the comment to also note that a
crupto backend needs to be selected.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot