This adds proper TLS verification to wget

I think you should add some of the comments you made in the bugzilla here:

---

By enabling the busybox feature: WGET_OPENSSL it means that in builds WITH openssl (ex. sato)
the issue will be completely fixed, and in builds WITHOUT openssl, busybox will fallback
to using the internal (insecure) client which will print out a message
"note: TLS certificate validation not implemented" Note that busybox does not rely in any way on the OpenSSL library
(it just executes the standalone binary, if it is found) so
we shouldn't have linkage issues is CONFIG_FEATURE_WGET_OPENSSL is enabled but OpenSSL is not getting built.

---

Thanks for the explanation.
We could add a RSUGGESTS make the coupling more clear:

http://docs.yoctoproject.org/ref-manual/variables.html?highlight=rrecommends#term-RSUGGESTS

I don't use that feature at all and it's not widely used in oe-core so hopefully someone
opinionated will reply and help us out.

../Randy

 

Signed-off-by: Shachar Menashe <shachar@...>

---

 meta/recipes-core/busybox/busybox.inc | 1 +

 1 file changed, 1 insertion(+)

 

diff --git a/meta/recipes-core/busybox/busybox.inc b/meta/recipes-core/busybox/busybox.inc

index 47fcb59302..8f274bd263 100644

--- a/meta/recipes-core/busybox/busybox.inc

+++ b/meta/recipes-core/busybox/busybox.inc

@@ -77,6 +77,7 @@ def features_to_busybox_settings(d):

     busybox_cfg(bb.utils.contains('DISTRO_FEATURES', 'ipv4', True, False, d), 'CONFIG_FEATURE_IFUPDOWN_IPV4', cnf, rem)

     busybox_cfg(bb.utils.contains('DISTRO_FEATURES', 'ipv6', True, False, d), 'CONFIG_FEATURE_IFUPDOWN_IPV6', cnf, rem)

     busybox_cfg(bb.utils.contains_any('DISTRO_FEATURES', 'bluetooth wifi', True, False, d), 'CONFIG_RFKILL', cnf, rem)

+    busybox_cfg(True, 'CONFIG_FEATURE_WGET_OPENSSL', cnf, rem)

     return "\n".join(cnf), "\n".join(rem)

 

 # X, Y = ${@features_to_busybox_settings(d)}

--

2.17.1

-- 
# Randy MacLeod
# Wind River Linux

On Tue, Apr 20, 2021 at 10:23 AM Randy MacLeod
<randy.macleod@...> wrote:

Add the oe-core list where patches are usually discussed.

On 2021-04-17 10:41 a.m., Shachar Menashe wrote:

This adds proper TLS verification to wget

I think you should add some of the comments you made in the bugzilla here:

---

By enabling the busybox feature: WGET_OPENSSL it means that in builds WITH openssl (ex. sato)
the issue will be completely fixed, and in builds WITHOUT openssl, busybox will fallback
to using the internal (insecure) client which will print out a message
"note: TLS certificate validation not implemented" Note that busybox does not rely in any way on the OpenSSL library
(it just executes the standalone binary, if it is found) so
we shouldn't have linkage issues is CONFIG_FEATURE_WGET_OPENSSL is enabled but OpenSSL is not getting built.

---

Thanks for the explanation.
We could add a RSUGGESTS make the coupling more clear:

http://docs.yoctoproject.org/ref-manual/variables.html?highlight=rrecommends#term-RSUGGESTS

I don't use that feature at all and it's not widely used in oe-core so hopefully someone
opinionated will reply and help us out.

../Randy

Signed-off-by: Shachar Menashe <shachar@...>
---
meta/recipes-core/busybox/busybox.inc | 1 +
1 file changed, 1 insertion(+)

diff --git a/meta/recipes-core/busybox/busybox.inc b/meta/recipes-core/busybox/busybox.inc
index 47fcb59302..8f274bd263 100644
--- a/meta/recipes-core/busybox/busybox.inc
+++ b/meta/recipes-core/busybox/busybox.inc
@@ -77,6 +77,7 @@ def features_to_busybox_settings(d):
busybox_cfg(bb.utils.contains('DISTRO_FEATURES', 'ipv4', True, False, d), 'CONFIG_FEATURE_IFUPDOWN_IPV4', cnf, rem)
busybox_cfg(bb.utils.contains('DISTRO_FEATURES', 'ipv6', True, False, d), 'CONFIG_FEATURE_IFUPDOWN_IPV6', cnf, rem)
busybox_cfg(bb.utils.contains_any('DISTRO_FEATURES', 'bluetooth wifi', True, False, d), 'CONFIG_RFKILL', cnf, rem)
+ busybox_cfg(True, 'CONFIG_FEATURE_WGET_OPENSSL', cnf, rem)
return "\n".join(cnf), "\n".join(rem)

# X, Y = ${@features_to_busybox_settings(d)}
--
2.17.1

This was discussed on the list last year. The conclusion was that
FEATURE_WGET_HTTPS should be disabled by default (ie giving anyone who
needs to fetch from https URLs to clear hint that that should be using
full featured wget or curl) rather than enabling a hacky solution to
have busybox call out to the openssl command line tool. Has something
changed since then?

Last time we talked about this I thought we would need to change something in openssl build settings to make the openssl binary get built just for this solution, and that was what got rejected.
But actually now I see (or perhaps it got changed) that the openssl binary is built anyways, in any build that already relies on openssl.
So my suggestion is to enable this feature. Like I said in builds with openssl it will make everything more secure in a transparent manner, and in builds without openssl it will display a warning just like today.
I wouldn't consider it a hacky solution since this is the official solution for this issue.
This is also exacerbated due to the fact that there are no other alternatives for secure download from CLI (ex. the sato build doesn't contain the "curl" standalone binary)

On Tue, Apr 20, 2021 at 1:46 PM Shachar Menashe <shachar@...> wrote:

Last time we talked about this I thought we would need to change something in openssl build settings to make the openssl binary get built just for this solution, and that was what got rejected.
But actually now I see (or perhaps it got changed) that the openssl binary is built anyways, in any build that already relies on openssl.
So my suggestion is to enable this feature. Like I said in builds with openssl it will make everything more secure in a transparent manner, and in builds without openssl it will display a warning just like today.

How much does busybox size grow with this? I think we will have to add
openssl dependency on it, or else default wget behavious will be less
than ideal. right now perhaps using gnu wget is a standalone solution
but I do understand that it may not be usable in some cases.

I wouldn't consider it a hacky solution since this is the official solution for this issue.
This is also exacerbated due to the fact that there are no other alternatives for secure download from CLI (ex. the sato build doesn't contain the "curl" standalone binary)

certainly, add curl to default reference images would be fine.

On Tue, Apr 20, 2021 at 1:46 PM Shachar Menashe <shachar@...> wrote:

Last time we talked about this I thought we would need to change something in openssl build settings to make the openssl binary get built just for this solution, and that was what got rejected.
But actually now I see (or perhaps it got changed) that the openssl binary is built anyways, in any build that already relies on openssl.
So my suggestion is to enable this feature. Like I said in builds with openssl it will make everything more secure in a transparent manner, and in builds without openssl it will display a warning just like today.
I wouldn't consider it a hacky solution since this is the official solution for this issue.

It's very clearly a hack. Maybe it's the "official solution" for
supporting https with busybox wget, but OE has a wider scope - we're
not limited to busybox wget if a better overall solution is available.

This is also exacerbated due to the fact that there are no other alternatives for secure download from CLI (ex. the sato build doesn't contain the "curl" standalone binary)

I don't see an issue with adding curl to any OE reference image which
needs an https client.

On Tue, Apr 20, 2021 at 1:46 PM Shachar Menashe <shachar@...> wrote:

Last time we talked about this I thought we would need to change something in openssl build settings to make the openssl binary get built just for this solution, and that was what got rejected.
But actually now I see (or perhaps it got changed) that the openssl binary is built anyways, in any build that already relies on openssl.
So my suggestion is to enable this feature. Like I said in builds with openssl it will make everything more secure in a transparent manner, and in builds without openssl it will display a warning just like today.
I wouldn't consider it a hacky solution since this is the official solution for this issue.

It's very clearly a hack. Maybe it's the "official solution" for
supporting https with busybox wget, but OE has a wider scope - we're
not limited to busybox wget if a better overall solution is available.

This is also exacerbated due to the fact that there are no other alternatives for secure download from CLI (ex. the sato build doesn't contain the "curl" standalone binary)

I don't see an issue with adding curl to any OE reference image which
needs an https client.

OK, so do you suggest adding curl and removing wget? (that would be a patch with a configuration change to busybox)

On Wed, Apr 21, 2021 at 2:22 AM Shachar Menashe <shachar@...> wrote:

On Tue, Apr 20, 2021 at 1:46 PM Shachar Menashe <shachar@...> wrote:

Last time we talked about this I thought we would need to change something in openssl build settings to make the openssl binary get built just for this solution, and that was what got rejected.
But actually now I see (or perhaps it got changed) that the openssl binary is built anyways, in any build that already relies on openssl.
So my suggestion is to enable this feature. Like I said in builds with openssl it will make everything more secure in a transparent manner, and in builds without openssl it will display a warning just like today.
I wouldn't consider it a hacky solution since this is the official solution for this issue.

It's very clearly a hack. Maybe it's the "official solution" for
supporting https with busybox wget, but OE has a wider scope - we're
not limited to busybox wget if a better overall solution is available.

This is also exacerbated due to the fact that there are no other alternatives for secure download from CLI (ex. the sato build doesn't contain the "curl" standalone binary)

I don't see an issue with adding curl to any OE reference image which
needs an https client.

OK, so do you suggest adding curl and removing wget? (that would be a patch with a configuration change to busybox)

Yes, sounds good to me.