From 3c965b7468d4fa656b0d072fae3c467c2eadb20b Mon Sep 17 00:00:00 2001 From: William Brown Date: Tue, 21 Jun 2022 10:20:50 +1000 Subject: [PATCH 1/8] OBS has a fault where is sends invalid md5s This causes downloads to come from the api, generally on noarch packages. However, in countries like australia, due to OBS' high latency, and poor bandwidth, these faults can cause downloads to take more than an hour, compared to using a local mirror which can take minutes. There is actually nothing wrong with the packages it all, OBS just sends the wrong md5. As a result, ignore the problem and complain about it, because OBS is broken here, not osc, and this wastes a lot of time. --- osc/build.py | 9 ++++++--- osc/fetch.py | 12 ++++++++---- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/osc/build.py b/osc/build.py index 993987c38c..b286c1d0d7 100644 --- a/osc/build.py +++ b/osc/build.py @@ -145,7 +145,10 @@ def __init__(self, filename, apiurl, buildtype = 'spec', localpkgs = [], binaryt else: self.release = None if config['api_host_options'][apiurl]['downloadurl']: - self.enable_cpio = False + # Formerly, this was set to False, but we have to set it to True, because a large + # number of repos in OBS are misconfigured and don't actually have repos setup - they + # are API only. + self.enable_cpio = True self.downloadurl = config['api_host_options'][apiurl]['downloadurl'] + "/repositories" if config['http_debug']: print("⚠️ setting dl_url to %s" % config['api_host_options'][apiurl]['downloadurl']) @@ -1351,8 +1354,8 @@ def __str__(self): print("Error: cannot get hdrmd5 for %s" % i.fullfilename) sys.exit(1) if hdrmd5 != i.hdrmd5: - print("Error: hdrmd5 mismatch for %s: %s != %s" % (i.fullfilename, hdrmd5, i.hdrmd5)) - sys.exit(1) + print("WARNING: OBS BUG hdrmd5 mismatch for %s: %s != %s" % (i.fullfilename, hdrmd5, i.hdrmd5)) + # sys.exit(1) print('Writing build configuration') diff --git a/osc/fetch.py b/osc/fetch.py index 839023d06b..d8f28bbb18 100644 --- a/osc/fetch.py +++ b/osc/fetch.py @@ -261,13 +261,17 @@ def run(self, buildinfo): else: # if the checksum of the downloaded package doesn't match, # delete it and mark it for downloading from the API + # + # wbrown 2022 - is there a reason to keep these md5's at all? md5 is + # broken from a security POV so these aren't a trusted source for validation + # of the file content. They are often incorrect forcing download via the API + # which for anyone outside the EU is excruciating. And when they are ignored + # builds work and progress anyway? So what do they even do? What are they + # for? They should just be removed. hdrmd5 = packagequery.PackageQuery.queryhdrmd5(i.fullfilename) if not hdrmd5 or hdrmd5 != i.hdrmd5: - print('%s/%s: attempting download from api, since the hdrmd5 did not match - %s != %s' + print('%s/%s: allowing invalid file, probably an OBS bug - hdrmd5 did not match - %s != %s' % (i.project, i.name, hdrmd5, i.hdrmd5)) - os.unlink(i.fullfilename) - self.__add_cpio(i) - except KeyboardInterrupt: print('Cancelled by user (ctrl-c)') print('Exiting.') From 46f4546f4903867b2f0a8649526a739fdae8f745 Mon Sep 17 00:00:00 2001 From: Daniel Mach Date: Mon, 20 Feb 2023 14:28:59 +0100 Subject: [PATCH 2/8] build: New option 'disable_hdrmd5_check' to ignore hdrmd5 mismatches --- osc/build.py | 7 +++++-- osc/conf.py | 30 ++++++++++++++++++++++++++++-- osc/fetch.py | 35 ++++++++++++++++++++++------------- 3 files changed, 55 insertions(+), 17 deletions(-) diff --git a/osc/build.py b/osc/build.py index b286c1d0d7..c917523f97 100644 --- a/osc/build.py +++ b/osc/build.py @@ -1354,8 +1354,11 @@ def __str__(self): print("Error: cannot get hdrmd5 for %s" % i.fullfilename) sys.exit(1) if hdrmd5 != i.hdrmd5: - print("WARNING: OBS BUG hdrmd5 mismatch for %s: %s != %s" % (i.fullfilename, hdrmd5, i.hdrmd5)) - # sys.exit(1) + if conf.config["api_host_options"][apiurl]["disable_hdrmd5_check"]: + print(f"Warning: Ignoring a hdrmd5 mismatch for {i.fullfilename}: {hdrmd5} (actual) != {i.hdrmd5} (expected)") + else: + print(f"Error: hdrmd5 mismatch for {i.fullfilename}: {hdrmd5} (actual) != {i.hdrmd5} (expected)") + sys.exit(1) print('Writing build configuration') diff --git a/osc/conf.py b/osc/conf.py index eca1c4af0c..dd02d05647 100644 --- a/osc/conf.py +++ b/osc/conf.py @@ -158,6 +158,28 @@ def _identify_osccookiejar(): 'cookiejar': _identify_osccookiejar(), # fallback for osc build option --no-verify 'no_verify': '0', + + # Disable hdrmd5 checks of downloaded and cached packages in `osc build` + # Recommended value: 0 + # + # OBS builds the noarch packages once per binary arch. + # Such noarch packages are supposed to be nearly identical across all build arches, + # any discrepancy in the payload and dependencies is considered a packaging bug. + # But to guarantee that the local builds work identically to builds in OBS, + # using the arch-specific copy of the noarch package is required. + # Unfortunatelly only one of the noarch packages gets distributed + # and can be downloaded from a local mirror. + # All other noarch packages are available through the OBS API only. + # Since there is currently no information about hdrmd5 checksums of published noarch packages, + # we download them, verify hdrmd5 and re-download the package from OBS API on mismatch. + # + # The same can also happen for architecture depend packages when someone is messing around + # with the source history or the release number handling in a way that it is not increasing. + # + # If you want to save some bandwidth and don't care about the exact rebuilds + # you can turn this option on to disable hdrmd5 checks completely. + 'disable_hdrmd5_check': '0', + # enable project tracking by default 'do_package_tracking': '1', # default for osc build @@ -221,13 +243,13 @@ def _identify_osccookiejar(): boolean_opts = ['debug', 'do_package_tracking', 'http_debug', 'post_mortem', 'traceback', 'check_filelist', 'plaintext_passwd', 'checkout_no_colon', 'checkout_rooted', 'check_for_request_on_action', 'linkcontrol', 'show_download_progress', 'request_show_interactive', - 'request_show_source_buildstatus', 'review_inherit_group', 'use_keyring', 'gnome_keyring', 'no_verify', 'builtin_signature_check', + 'request_show_source_buildstatus', 'review_inherit_group', 'use_keyring', 'gnome_keyring', 'no_verify', 'disable_hdrmd5_check', 'builtin_signature_check', 'http_full_debug', 'include_request_from_project', 'local_service_run', 'buildlog_strip_time', 'no_preinstallimage', 'status_mtime_heuristic', 'print_web_links', 'ccache', 'sccache', 'build-shell-after-fail'] integer_opts = ['build-jobs'] api_host_options = ['user', 'pass', 'passx', 'aliases', 'http_headers', 'realname', 'email', 'sslcertck', 'cafile', 'capath', 'trusted_prj', - 'downloadurl', 'sshkey'] + 'downloadurl', 'sshkey', 'disable_hdrmd5_check'] new_conf_template = """ [general] @@ -1207,6 +1229,10 @@ def get_config(override_conffile=None, if api_host_options[apiurl]['sshkey'] is None: api_host_options[apiurl]['sshkey'] = config['sshkey'] + api_host_options[apiurl]["disable_hdrmd5_check"] = config["disable_hdrmd5_check"] + if cp.has_option(url, "disable_hdrmd5_check"): + api_host_options[apiurl][key] = cp.getboolean(url, "disable_hdrmd5_check") + # add the auth data we collected to the config dict config['api_host_options'] = api_host_options config['apiurl_aliases'] = aliases diff --git a/osc/fetch.py b/osc/fetch.py index d8f28bbb18..0db758c41b 100644 --- a/osc/fetch.py +++ b/osc/fetch.py @@ -206,6 +206,7 @@ def _build_urllist(self, buildinfo, pac): return urllist def run(self, buildinfo): + apiurl = buildinfo.apiurl cached = 0 all = len(buildinfo.deps) for i in buildinfo.deps: @@ -222,12 +223,24 @@ def run(self, buildinfo): cached += 1 if not i.name.startswith('container:') and i.pacsuffix != 'rpm': continue + + hdrmd5_is_valid = True if i.hdrmd5: if i.name.startswith('container:'): hdrmd5 = dgst(i.fullfilename) + if hdrmd5 != i.hdrmd5: + hdrmd5_is_valid = False else: hdrmd5 = packagequery.PackageQuery.queryhdrmd5(i.fullfilename) - if not hdrmd5 or hdrmd5 != i.hdrmd5: + if hdrmd5 != i.hdrmd5: + if conf.config["api_host_options"][apiurl]["disable_hdrmd5_check"]: + print(f"Warning: Ignoring a hdrmd5 mismatch for {i.fullfilename}: {hdrmd5} (actual) != {i.hdrmd5} (expected)") + hdrmd5_is_valid = True + else: + print(f"The file will be redownloaded from the API due to a hdrmd5 mismatch for {i.fullfilename}: {hdrmd5} (actual) != {i.hdrmd5} (expected)") + hdrmd5_is_valid = False + + if not hdrmd5_is_valid: os.unlink(i.fullfilename) cached -= 1 @@ -259,19 +272,15 @@ def run(self, buildinfo): # mark it for downloading from the API self.__add_cpio(i) else: - # if the checksum of the downloaded package doesn't match, - # delete it and mark it for downloading from the API - # - # wbrown 2022 - is there a reason to keep these md5's at all? md5 is - # broken from a security POV so these aren't a trusted source for validation - # of the file content. They are often incorrect forcing download via the API - # which for anyone outside the EU is excruciating. And when they are ignored - # builds work and progress anyway? So what do they even do? What are they - # for? They should just be removed. hdrmd5 = packagequery.PackageQuery.queryhdrmd5(i.fullfilename) - if not hdrmd5 or hdrmd5 != i.hdrmd5: - print('%s/%s: allowing invalid file, probably an OBS bug - hdrmd5 did not match - %s != %s' - % (i.project, i.name, hdrmd5, i.hdrmd5)) + if hdrmd5 != i.hdrmd5: + if conf.config["api_host_options"][apiurl]["disable_hdrmd5_check"]: + print(f"Warning: Ignoring a hdrmd5 mismatch for {i.fullfilename}: {hdrmd5} (actual) != {i.hdrmd5} (expected)") + else: + print(f"The file will be redownloaded from the API due to a hdrmd5 mismatch for {i.fullfilename}: {hdrmd5} (actual) != {i.hdrmd5} (expected)") + os.unlink(i.fullfilename) + self.__add_cpio(i) + except KeyboardInterrupt: print('Cancelled by user (ctrl-c)') print('Exiting.') From 34f7bdb47d1829dc02a68fe8a9ac71f5f81de26f Mon Sep 17 00:00:00 2001 From: Daniel Mach Date: Tue, 9 May 2023 16:12:21 +0200 Subject: [PATCH 3/8] Consider only open requests when listing requests with a given review state --- osc/core.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/osc/core.py b/osc/core.py index 139ff89eba..9f3e2ef888 100644 --- a/osc/core.py +++ b/osc/core.py @@ -4467,9 +4467,11 @@ def build_by(xpath, val): return '' xpath = '' + + # we're interested only in reviews of requests that are still open + xpath = xpath_join(xpath, "(state/@name='new' or state/@name='review' or state/@name='declined')", op="and") + if states == (): - # default: requests which are still open and have reviews in "new" state - xpath = xpath_join('', 'state/@name=\'review\'', op='and') xpath = xpath_join(xpath, 'review/@state=\'new\'', op='and') if byuser: xpath = build_by(xpath, '@by_user=\'%s\'' % byuser) From 7fa326bbbd94d788aa274020630d3e50f8563f96 Mon Sep 17 00:00:00 2001 From: Daniel Mach Date: Wed, 7 Sep 2022 10:09:20 +0200 Subject: [PATCH 4/8] Properly handle missing ssh-keygen and ssh-add A very conservative backport of: commit 2496b3e987d15d394f1e17973e7735799071af9d Author: Daniel Mach Date: Wed Sep 7 10:09:20 2022 +0200 Properly handle missing ssh-keygen and ssh-add --- osc/conf.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/osc/conf.py b/osc/conf.py index dd02d05647..94a3e14f88 100644 --- a/osc/conf.py +++ b/osc/conf.py @@ -605,7 +605,12 @@ def __init__(self, user, sshkey): def list_ssh_agent_keys(self): cmd = ['ssh-add', '-l'] - proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + try: + proc = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + except OSError: + # ssh-add is not available + return [] + stdout, _ = proc.communicate() if proc.returncode == 0 and stdout.strip(): return [self.get_fingerprint(line) for line in stdout.splitlines()] @@ -699,7 +704,10 @@ def ssh_sign(self, data, namespace, keyfile=None): keyfile = os.path.expanduser(keyfile) cmd = ['ssh-keygen', '-Y', 'sign', '-f', keyfile, '-n', namespace, '-q'] - proc = subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE) + try: + proc = subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE) + except OSError: + return None stdout, _ = proc.communicate(data) if proc.returncode: raise oscerr.OscIOError(None, 'ssh-keygen signature creation failed: %d' % proc.returncode) @@ -715,6 +723,9 @@ def get_authorization(self, req, chal): now = int(time.time()) sigdata = "(created): %d" % now signature = self.ssh_sign(sigdata, realm, self.sshkey) + if not signature: + # the signing step failed due to missing ssh-keygen + return None signature = decode_it(base64.b64encode(signature)) return 'keyId="%s",algorithm="ssh",headers="(created)",created=%d,signature="%s"' \ % (self.user, now, signature) From 26ac62f1e9d738742e254b897dd37b76c3d18880 Mon Sep 17 00:00:00 2001 From: Fabian Vogt Date: Fri, 12 May 2023 09:51:06 +0200 Subject: [PATCH 5/8] Add req_states parameter to osc.core.get_review_list Keep the original behaviour by default, but allow other callers to also request reviews on e.g. declined SRs. --- osc/core.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/osc/core.py b/osc/core.py index 9f3e2ef888..3922108763 100644 --- a/osc/core.py +++ b/osc/core.py @@ -4450,7 +4450,7 @@ def change_request_state_template(req, newstate): return '' def get_review_list(apiurl, project='', package='', byuser='', bygroup='', byproject='', bypackage='', states=(), - req_type=''): + req_type='', req_states=("review",)): # this is so ugly... def build_by(xpath, val): if 'all' in states: @@ -4468,8 +4468,11 @@ def build_by(xpath, val): xpath = '' - # we're interested only in reviews of requests that are still open - xpath = xpath_join(xpath, "(state/@name='new' or state/@name='review' or state/@name='declined')", op="and") + # By default we're interested only in reviews of requests that are in state review. + for req_state in req_states: + xpath = xpath_join(xpath, "state/@name='%s'" % req_state, inner=True) + + xpath = "(%s)" % xpath if states == (): xpath = xpath_join(xpath, 'review/@state=\'new\'', op='and') From 409dbdfa27457ff2678ad19979c4bd85daf7047f Mon Sep 17 00:00:00 2001 From: Daniel Mach Date: Fri, 19 May 2023 21:51:25 +0200 Subject: [PATCH 6/8] Change 'review list' command to display open requests (state: new, review, declined) The original behavior was that only requests in the 'review' state were displayed. --- osc/commandline.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osc/commandline.py b/osc/commandline.py index 09a72a5f9c..a086119611 100644 --- a/osc/commandline.py +++ b/osc/commandline.py @@ -2470,7 +2470,7 @@ def do_request(self, subcmd, opts, *args): if subcmd == 'review': # FIXME: do the review list for the user and for all groups he belong to results = get_review_list(apiurl, project, package, who, opts.group, opts.project, opts.package, state_list, - opts.type) + opts.type, req_states=("new", "review", "declined")) else: if opts.involved_projects: who = who or conf.get_apiurl_usr(apiurl) From 675f3d50c9eb379d0d6d62f9c2b0fd089636dd7c Mon Sep 17 00:00:00 2001 From: Daniel Mach Date: Tue, 14 Feb 2023 14:39:33 +0100 Subject: [PATCH 7/8] GHA: Enable global.break-system-packages option for pip --- .github/workflows/unittests.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/unittests.yaml b/.github/workflows/unittests.yaml index 95bdb33674..5f27461150 100644 --- a/.github/workflows/unittests.yaml +++ b/.github/workflows/unittests.yaml @@ -80,6 +80,7 @@ jobs: - name: 'Run unit tests' run: | + pip3 config set global.break-system-packages 1 pip3 install -e . python3 setup.py test @@ -123,5 +124,6 @@ jobs: - name: 'Run unit tests' run: | mkdir -p /usr/local/lib/python2.7/site-packages/ + pip2 config set global.break-system-packages 1 pip2 install -e . python2 setup.py test From 538c72bd8be9681319e8e02acc8fa3e48cd1ea8d Mon Sep 17 00:00:00 2001 From: Daniel Mach Date: Tue, 23 May 2023 15:04:53 +0200 Subject: [PATCH 8/8] GHA: Disable repo 'repo-openh264' during tests We don't use it --- .github/workflows/unittests.yaml | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/.github/workflows/unittests.yaml b/.github/workflows/unittests.yaml index 5f27461150..db66d49139 100644 --- a/.github/workflows/unittests.yaml +++ b/.github/workflows/unittests.yaml @@ -55,10 +55,11 @@ jobs: - name: 'Install packages (OpenSUSE)' if: ${{ startsWith(matrix.container, 'opensuse/') }} run: | - zypper --non-interactive --gpg-auto-import-keys refresh - zypper --non-interactive dist-upgrade - zypper --non-interactive install git-lfs - zypper --non-interactive install diffstat diffutils python3 python3-M2Crypto python3-pip python3-rpm python3-setuptools + zypper -n modifyrepo --disable repo-openh264 || : + zypper -n --gpg-auto-import-keys refresh + zypper -n dist-upgrade + zypper -n install git-lfs + zypper -n install diffstat diffutils python3 python3-M2Crypto python3-pip python3-rpm python3-setuptools - name: 'Install packages (Fedora/CentOS)' if: ${{ startsWith(matrix.container, 'fedora:') || contains(matrix.container, 'centos:') }} @@ -114,10 +115,11 @@ jobs: - name: 'Install packages (OpenSUSE)' if: ${{ startsWith(matrix.container, 'opensuse/') }} run: | - zypper --non-interactive --gpg-auto-import-keys refresh - zypper --non-interactive dist-upgrade - zypper --non-interactive install git-lfs - zypper --non-interactive install diffstat diffutils python python2-M2Crypto python2-pip python2-rpm python2-setuptools + zypper -n modifyrepo --disable repo-openh264 || : + zypper -n --gpg-auto-import-keys refresh + zypper -n dist-upgrade + zypper -n install git-lfs + zypper -n install diffstat diffutils python python2-M2Crypto python2-pip python2-rpm python2-setuptools - uses: actions/checkout@v3