[PR PATCH] update-check: no partial response and not trust insecure certificate

[PR PATCH] update-check: no partial response and not trust insecure certificate

[Dieken]

[-- Attachment #1: Type: text/plain, Size: 1827 bytes --]

There is a new pull request by Dieken against master on the void-packages repository

https://github.com/Dieken/void-packages stricter-update-check
https://github.com/void-linux/void-packages/pull/43192

update-check: no partial response and not trust insecure certificate
1. My network is slow, `./xbps-src update-check xorg-server` takes 50s and `./xbps-src update-check xorg-server-xwayland` takes 11s, `curl --max-time 10` returns partial response but no error.  So I remove `--max-time 10` and add `-f` to forbid partial response. It's better to call `set -eo pipefail` for the whole script but I'm not sure whether it will break the script.  The CI job or interactive execution can have their own timeout.

2. `-k` means `--insecure`, it's bad, we should always validate HTTPS server certificate.

<!-- Uncomment relevant sections and delete options which are not applicable -->

#### Testing the changes
- I tested the changes in this PR: **YES**

<!--
#### New package
- This new package conforms to the [package requirements](https://github.com/void-linux/void-packages/blob/master/CONTRIBUTING.md#package-requirements): **YES**|**NO**
-->

<!-- Note: If the build is likely to take more than 2 hours, please add ci skip tag as described in
https://github.com/void-linux/void-packages/blob/master/CONTRIBUTING.md#continuous-integration
and test at least one native build and, if supported, at least one cross build.
Ignore this section if this PR is not skipping CI.
-->
<!--
#### Local build testing
- I built this PR locally for my native architecture, (ARCH-LIBC)
- I built this PR locally for these architectures (if supported. mark crossbuilds):
  - aarch64-musl
  - armv7l
  - armv6l-musl
-->


A patch file from https://github.com/void-linux/void-packages/pull/43192.patch is attached

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: github-pr-stricter-update-check-43192.patch --]
[-- Type: text/x-diff, Size: 2226 bytes --]

From 1c6a56334ed9724fca11c7e1bfb29bf641dbf989 Mon Sep 17 00:00:00 2001
From: Yubao Liu <yubao.liu@gmail.com>
Date: Sun, 2 Apr 2023 18:36:33 +0800
Subject: [PATCH] update-check: no partial response and not trust insecure
 certificate

1. My network is slow, `./xbps-src update-check xorg-server` takes 50s
   and `./xbps-src update-check xorg-server-xwayland` takes 11s, `curl
   --max-time 10` returns partial response but no error.  So I remove
   `--max-time 10` and add `-f` to forbid partial response. It's better
   to call `set -eo pipefail` for the whole script but I'm not sure
   whether it will break the script.  The CI job or interactive execution
   can have their own timeout.

2. `-k` means `--insecure`, it's bad, we should always validate HTTPS
   server certificate.
---
 common/xbps-src/shutils/update_check.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/common/xbps-src/shutils/update_check.sh b/common/xbps-src/shutils/update_check.sh
index cd3bb369bdb3..cd5c6ad7ff96 100644
--- a/common/xbps-src/shutils/update_check.sh
+++ b/common/xbps-src/shutils/update_check.sh
@@ -94,7 +94,7 @@ update_check() {
                 echo "(folder) fetching $urlpfx and scanning with $rx" 1>&2
             fi
             skipdirs=
-            curl -A "xbps-src-update-check/$XBPS_SRC_VERSION" --max-time 10 -Lsk "$urlpfx" |
+            curl -A "xbps-src-update-check/$XBPS_SRC_VERSION" -Lsf "$urlpfx" |
                 grep -Po -i "$rx" |
                 # sort -V places 1.1/ before 1/, but 1A/ before 1.1A/
                 sed -e 's:$:A:' -e 's:/A$:A/:' | sort -Vru | sed -e 's:A/$:/A:' -e 's:A$::' |
@@ -200,7 +200,7 @@ update_check() {
         if [ -n "$XBPS_UPDATE_CHECK_VERBOSE" ]; then
             echo "fetching $url and scanning with $rx" 1>&2
         fi
-        curl -H 'Accept: text/html,application/xhtml+xml,application/xml,text/plain,application/rss+xml' -A "xbps-src-update-check/$XBPS_SRC_VERSION" --max-time 10 -Lsk "$url" |
+        curl -H 'Accept: text/html,application/xhtml+xml,application/xml,text/plain,application/rss+xml' -A "xbps-src-update-check/$XBPS_SRC_VERSION" -Lsf "$url" |
             grep -Po -i "$rx"
         fetchedurls[$url]=yes
     done |

Re: update-check: no partial response and not trust insecure certificate

[classabbyamp]

[-- Attachment #1: Type: text/plain, Size: 210 bytes --]

New comment by classabbyamp on void-packages repository

https://github.com/void-linux/void-packages/pull/43192#issuecomment-1493432813

Comment:
I don't see not validating the cert as an issue for this script

Re: update-check: no partial response and not trust insecure certificate

[Dieken]

[-- Attachment #1: Type: text/plain, Size: 325 bytes --]

New comment by Dieken on void-packages repository

https://github.com/void-linux/void-packages/pull/43192#issuecomment-1493462484

Comment:
Is certificate validation an issue? Any package depend on self-signed certificate or outdated/wrong certificate? It’s weird to see an OS isn’t serious about security best practice.

Re: update-check: no partial response and not trust insecure certificate

[classabbyamp]

[-- Attachment #1: Type: text/plain, Size: 404 bytes --]

New comment by classabbyamp on void-packages repository

https://github.com/void-linux/void-packages/pull/43192#issuecomment-1493464006

Comment:
this is only for downloading a list of possible versions, not for downloading package distfiles or something where we care about the received data more than running some regexes over it. the most someone could do is make us think a new version is available.

Re: update-check: no partial response and not trust insecure certificate

[classabbyamp]

[-- Attachment #1: Type: text/plain, Size: 445 bytes --]

New comment by classabbyamp on void-packages repository

https://github.com/void-linux/void-packages/pull/43192#issuecomment-1493464006

Comment:
this is only for downloading a list of possible versions, not for downloading package distfiles or something where we care about the received data more than running some regexes over it. the most someone could do is make us think a new version is available when there isn't, or something like that.

Re: update-check: no partial response and not trust insecure certificate

[Dieken]

[-- Attachment #1: Type: text/plain, Size: 392 bytes --]

New comment by Dieken on void-packages repository

https://github.com/void-linux/void-packages/pull/43192#issuecomment-1493466846

Comment:
The daily void-updates job may be fooled there is no new version.

Could you explain why validate certificate IS an issue? Any package depend on insecure certificate? Why insecure by default? That’s unbelievable attitude for a famous distribution.

Re: update-check: no partial response and not trust insecure certificate

[Duncaen]

[-- Attachment #1: Type: text/plain, Size: 965 bytes --]

New comment by Duncaen on void-packages repository

https://github.com/void-linux/void-packages/pull/43192#issuecomment-1493470004

Comment:
> The daily void-updates job may be fooled there is no new version.

Sure, but so can a man in the middle simply dropping the connection or the server being unreachable simply by chance.

> Could you explain why validate certificate IS an issue? Any package depend on insecure certificate? Why insecure by default? 

This was added years ago without much explanation https://github.com/void-linux/void-packages/commit/ebbb33b51996c381bc8a0d2023e0e048614190.
Might have been the bad state of availability of ssl certificates at the time.
Its probably fine to remove that flag, and maybe add a mechanism to allow self signed certificates through the `srcpkg/*/update` file if required at some point.

> That’s unbelievable attitude for a famous distribution.

This comes a bit of as being hostile for no reason.

Re: update-check: no partial response and not trust insecure certificate

[classabbyamp]

[-- Attachment #1: Type: text/plain, Size: 233 bytes --]

New comment by classabbyamp on void-packages repository

https://github.com/void-linux/void-packages/pull/43192#issuecomment-1493470070

Comment:
Some packages' homepages have invalid or self-signed certificates, I've seen it before

Re: update-check: no partial response and not trust insecure certificate

[Duncaen]

[-- Attachment #1: Type: text/plain, Size: 433 bytes --]

New comment by Duncaen on void-packages repository

https://github.com/void-linux/void-packages/pull/43192#issuecomment-1493472600

Comment:
I don't think its a good idea to complete remove the timeout, we are requesting thousands urls, I think generally 10 seconds for a reply is a pretty good default, otherwise we'll end up with the void-updates job running a lot longer.
A way to configure the timeout might make sense instead.

Re: update-check: no partial response and not trust insecure certificate

[Duncaen]

[-- Attachment #1: Type: text/plain, Size: 434 bytes --]

New comment by Duncaen on void-packages repository

https://github.com/void-linux/void-packages/pull/43192#issuecomment-1493472600

Comment:
I don't think its a good idea to completey remove the timeout, we are requesting thousands urls, I think generally 10 seconds for a reply is a pretty good default, otherwise we'll end up with the void-updates job running a lot longer.
A way to configure the timeout might make sense instead.

Re: update-check: no partial response and not trust insecure certificate

[Dieken]

[-- Attachment #1: Type: text/plain, Size: 510 bytes --]

New comment by Dieken on void-packages repository

https://github.com/void-linux/void-packages/pull/43192#issuecomment-1493481055

Comment:
I apologize if my surprise sounds like offensive, I like simplicity and solidity of Void Linux.

I meant to increase that timeout but not sure about proper default value, then I feel it shouldn’t have timeout because if timeout happens it always get wrong result, and if no timeout the void-updates job won’t run longer :-)

Anyway I’m ok to a larger timeout.

Re: update-check: no partial response and not trust insecure certificate

[Dieken]

[-- Attachment #1: Type: text/plain, Size: 487 bytes --]

New comment by Dieken on void-packages repository

https://github.com/void-linux/void-packages/pull/43192#issuecomment-1493482399

Comment:
> Some packages' homepages have invalid or self-signed certificates, I've seen it before

Get it, thanks for the explanation, I’m new to Void Linux.

IMHO, the public official package repository should forbid insecure certificate, at least by default, I always feel nervous about skipping certificate validation when I review peer’s code.

Re: update-check: no partial response and not trust insecure certificate

[github-actions]

[-- Attachment #1: Type: text/plain, Size: 305 bytes --]

New comment by github-actions[bot] on void-packages repository

https://github.com/void-linux/void-packages/pull/43192#issuecomment-1616272470

Comment:
Pull Requests become stale 90 days after last activity and are closed 14 days after that.  If this pull request is still relevant bump it or assign it.

Re: [PR PATCH] [Closed]: update-check: no partial response and not trust insecure certificate

[github-actions]

[-- Attachment #1: Type: text/plain, Size: 1664 bytes --]

There's a closed pull request on the void-packages repository

update-check: no partial response and not trust insecure certificate
https://github.com/void-linux/void-packages/pull/43192

Description:
1. My network is slow, `./xbps-src update-check xorg-server` takes 50s and `./xbps-src update-check xorg-server-xwayland` takes 11s, `curl --max-time 10` returns partial response but no error.  So I remove `--max-time 10` and add `-f` to forbid partial response. It's better to call `set -eo pipefail` for the whole script but I'm not sure whether it will break the script.  The CI job or interactive execution can have their own timeout.

2. `-k` means `--insecure`, it's bad, we should always validate HTTPS server certificate.

<!-- Uncomment relevant sections and delete options which are not applicable -->

#### Testing the changes
- I tested the changes in this PR: **YES**

<!--
#### New package
- This new package conforms to the [package requirements](https://github.com/void-linux/void-packages/blob/master/CONTRIBUTING.md#package-requirements): **YES**|**NO**
-->

<!-- Note: If the build is likely to take more than 2 hours, please add ci skip tag as described in
https://github.com/void-linux/void-packages/blob/master/CONTRIBUTING.md#continuous-integration
and test at least one native build and, if supported, at least one cross build.
Ignore this section if this PR is not skipping CI.
-->
<!--
#### Local build testing
- I built this PR locally for my native architecture, (ARCH-LIBC)
- I built this PR locally for these architectures (if supported. mark crossbuilds):
  - aarch64-musl
  - armv7l
  - armv6l-musl
-->