PR#2111: sha 256 for GPG signing - copr/copr

		`@@ -209,7 +209,8 @@`
		`# Drop old signatures coming from original repo and re-sign.`
		`unsign_rpms_in_dir(dst_path, opts=self.opts, log=self.log)`
		`if sign:`
		`- sign_rpms_in_dir(data["user"], data["copr"], dst_path, opts=self.opts, log=self.log)`
		`+ sign_rpms_in_dir(data["user"], data["copr"], dst_path,`
		`+ chroot, opts=self.opts, log=self.log)`

		`self.log.info("Forked build %s as %s", src_path, dst_path)`

backend/copr_backend/background_worker_build.py

file modified

		`@@ -596,6 +596,7 @@`
		`self.job.project_owner,`
		`self.job.project_name,`
		`os.path.join(self.job.chroot_dir, self.job.target_dir_name),`
		`+ self.job.chroot,`
		`opts=self.opts,`
		`log=self.log`
		`)`

backend/copr_backend/helpers.py

file modified

		`@@ -387,6 +387,9 @@`

		`opts.prune_days = _get_conf(cp, "backend", "prune_days", None, mode="int")`

		`+ opts.gently_gpg_sha256 = _get_conf(`
		`+ cp, "backend", "gently_gpg_sha256", True, mode="bool")`
		`+`
		`# ssh options`
		`opts.ssh = Munch()`
		`opts.ssh.builder_config = _get_conf(`

backend/copr_backend/sign.py

file modified

+46 -4

		`@@ -10,6 +10,8 @@`
		`import os`
		`from requests import request`

		`+ from packaging import version`
		`+`
		`from .exceptions import CoprSignError, CoprSignNoKeyError, \`
		`CoprKeygenRequestError`

		`@@ -81,8 +83,8 @@`
		`return stdout`


		`- def _sign_one(path, email, log):`
		`- cmd = [SIGN_BINARY, "-u", email, "-r", path]`
		`+ def _sign_one(path, email, hashtype, log):`
		`+ cmd = [SIGN_BINARY, "-h", hashtype, "-u", email, "-r", path]`
		`returncode, stdout, stderr = call_sign_bin(cmd, log)`
		`if returncode != 0:`
		`raise CoprSignError(`
		`@@ -92,7 +94,43 @@`
		`return stdout, stderr`


		`- def sign_rpms_in_dir(username, projectname, path, opts, log):`
		`+ def gpg_hashtype_for_chroot(chroot, opts):`
		`+ """`
		`+ Given the chroot name (in "mock format", like "fedora-rawhide-x86_64")`
		`+ return the expected GPG hash type.`
		`+ """`
		`+`
		`+ el_chroots = ["rhel", "epel", "centos", "oraclelinux"]`
		`+`
		`+ parts = chroot.split("-")`
		`+`
		`+ if opts.gently_gpg_sha256:`
		`+ # For a few weeks we would use the sha256 hash type only for EL8+.`
		`+ # This is a safety belt, in case of any failure we'll just re-sign`
		`+ # epel-8+ and not _all_ the package data on backend.`
		`+ if parts[0] in el_chroots:`
		`+ el_version = parts[1]`
		`+ if el_version == "stream":`
		`+ el_version = parts[2]`
		`+ if version.parse(el_version) > version.parse("7"):`
		`+ return "sha256"`
		`+ return "sha1"`
		`+`
		`+ if parts[0] in el_chroots:`
		`+ chroot_version = parts[1]`
		`+ if chroot_version in ["rawhide", "stream"]:`
		`+ return "sha256"`
		`+ if version.parse(chroot_version) <= version.parse("7"):`
		`+ return "sha1"`
		`+ if parts[0] == "fedora" and parts[1].isnumeric():`
		`+ # Fedora 27 moved to RPM v2.14 with the OpenSSL backend.`
		`+ if int(parts[1]) < 27:`
		`+ return "sha1"`
		`+ # fallback to sha256`
		`+ return "sha256"`
		`+`
		`+`
		`+ def sign_rpms_in_dir(username, projectname, path, chroot, opts, log):`
		`"""`
		`Signs rpms using obs-signd.`

		`@@ -102,6 +140,7 @@`
		`:param username: copr username`
		`:param projectname: copr projectname`
		`:param path: directory with rpms to be signed`
		`+ :param chroot: chroot name where we sign packages, affects the hash type`
		`:param Munch opts: backend config`

		`:type log: logging.Logger`
		`@@ -117,6 +156,8 @@`
		`if not rpm_list:`
		`return`

		`+ hashtype = gpg_hashtype_for_chroot(chroot, opts)`
		`+`
		`try:`
		`get_pubkey(username, projectname, log)`
		`except CoprSignNoKeyError:`
		`@@ -125,7 +166,8 @@`
		`errors = [] # tuples (rpm_filepath, exception)`
		`for rpm in rpm_list:`
		`try:`
		`- _sign_one(rpm, create_gpg_email(username, projectname), log)`
		`+ _sign_one(rpm, create_gpg_email(username, projectname),`
		`+ hashtype, log)`
		`log.info("signed rpm: {}".format(rpm))`

		`except CoprSignError as e:`

backend/run/copr_fix_gpg.py

file modified

+40 -11

		`@@ -1,5 +1,6 @@`
		`#!/usr/bin/python3`

		`+ import re`
		`import sys`
		`import os`
		`import logging`
		`@@ -27,45 +28,73 @@`
		`""")`

		`parser.add_argument('coprs_file_path', action='store', help='Path to the text file with copr full names (owner/coprname) to be fixed.')`
		`+ parser.add_argument("--chroot", action="append", default=[],`
		`+ help=("Only re-sign CHROOT. CHROOT can be a full chroot "`
		`+ "or a shortened chroot name without architecture ("`
		`+ "e.g. fedora-rawhide-x86_64 or just fedora-rawhide)."`
		`+ " (can be specified multiple times)"))`
		`args = parser.parse_args()`


		`def fix_copr(opts, copr_full_name):`
		`- log.info('Going to fix {}:'.format(copr_full_name))`
		`+ log.info("Going to fix %s", copr_full_name)`

		`owner, coprname = tuple(copr_full_name.split('/'))`
		`copr_path = os.path.abspath(os.path.join(opts.destdir, owner, coprname))`

		`if not os.path.isdir(copr_path):`
		`- log.info('Ignoring {}. Directory does not exist.'.format(copr_path))`
		`+ log.info('Ignoring %s. Directory does not exist.', copr_path)`
		`return`

		`- log.info('> Generate key-pair on copr-keygen (if not generated) for email {}.'.format(create_gpg_email(owner, coprname)))`
		`+ log.info("Generate key-pair on copr-keygen (if not generated) for email %s",`
		`+ create_gpg_email(owner, coprname))`
		`create_user_keys(owner, coprname, opts)`

		`- log.info('> Regenerate pubkey.gpg in copr {}.'.format(copr_path))`
		`+ log.info("Regenerate pubkey.gpg in copr %s", copr_path)`
		`get_pubkey(owner, coprname, log, os.path.join(copr_path, 'pubkey.gpg'))`

		`- log.info('> Re-sign rpms and call createrepo in copr\'s chroots:')`
		`- for dir_name in os.listdir(copr_path):`
		`- dir_path = os.path.join(copr_path, dir_name)`
		`+ # Match the "00001231-anycharacer" directory names. Compile once, use many.`
		`+ builddir_matcher = re.compile(r"\d{8,}-")`
		`+`
		`+ log.info("Re-sign rpms and call createrepo in copr's chroots")`
		`+`
		`+ for chroot in os.listdir(copr_path):`
		`+ dir_path = os.path.join(copr_path, chroot)`
		`if not os.path.isdir(dir_path):`
		`- log.info('> > Ignoring {}'.format(dir_path))`
		`+ log.debug("Ignoring %s, not a directory", dir_path)`
		`+ continue`
		`+`
		`+ if chroot in ["srpm-builds", "modules", "repodata"]:`
		`+ log.debug("Ignoring %s, not a chroot", chroot)`
		`continue`

		`+ if args.chroot:`
		`+ parts = chroot.split("-")`
		`+ parts.pop()`
		`+ chroot_without_arch = "-".join(parts)`
		`+ if not (chroot in args.chroot or chroot_without_arch in args.chroot):`
		`+ log.info("Skipping %s, not included by --chroot (%s)", chroot,`
		`+ ", ".join(args.chroot))`
		`+ continue`
		`+`
		`+ log.info("Signing in %s chroot", chroot)`
		`for builddir_name in os.listdir(dir_path):`
		`builddir_path = os.path.join(dir_path, builddir_name)`
		`if not os.path.isdir(builddir_path):`
		`continue`
		`- log.info('> > Processing rpms in builddir {}:'.format(builddir_path))`
		`+ if not builddir_matcher.match(builddir_name):`
		`+ log.debug("Skipping %s, not a build dir", builddir_name)`
		`+ continue`
		`+`
		`+ log.info("Processing rpms in builddir %s", builddir_path)`
		`try:`
		`unsign_rpms_in_dir(builddir_path, opts, log) # first we need to unsign by using rpm-sign before we sign with obs-sign`
		`- sign_rpms_in_dir(owner, coprname, builddir_path, opts, log)`
		`+ sign_rpms_in_dir(owner, coprname, builddir_path, chroot, opts, log)`
		`except Exception as e:`
		`log.exception(str(e))`
		`continue`

		`- log.info("> > Running add_appdata for %s", dir_path)`
		`+ log.info("Running add_appdata for %s", dir_path)`
		`call_copr_repo(dir_path, logger=log)`

backend/tests/test_background_worker_build.py

file modified

+3 -2

		`@@ -228,6 +228,7 @@`
		`with open(config.be_config_file, "a+") as fdconfig:`
		`fdconfig.write("do_sign=true\n")`
		`fdconfig.write("keygen_host=keygen.example.com\n")`
		`+ fdconfig.write("gently_gpg_sha256=false\n")`
		`config.bw = _reset_build_worker()`
		`return config`

		`@@ -338,8 +339,8 @@`
		`srpm = os.path.join(worker.job.results_dir,`
		`"example-1.0.14-1.fc30.src.rpm")`
		`expected_calls = [`
		`- mock.call(rpm, mail, mc_sign_one.call_args_list[0][0][2]),`
		`- mock.call(srpm, mail, mc_sign_one.call_args_list[1][0][2]),`
		`+ mock.call(rpm, mail, "sha256", mc_sign_one.call_args_list[0][0][3]),`
		`+ mock.call(srpm, mail, "sha256", mc_sign_one.call_args_list[1][0][3]),`
		`]`
		`for call in expected_calls:`
		`assert call in mc_sign_one.call_args_list`

backend/tests/test_sign.py

file modified

+74 -16

		`@@ -2,17 +2,17 @@`
		`import tempfile`
		`import shutil`
		`import time`
		`+ from unittest import mock`
		`+ from unittest.mock import MagicMock`

		`from munch import Munch`
		`import pytest`

		`from copr_backend.exceptions import CoprSignError, CoprSignNoKeyError, CoprKeygenRequestError`
		`-`
		`- from unittest import mock`
		`- from unittest.mock import MagicMock`
		`-`
		`- from copr_backend.sign import get_pubkey, _sign_one, sign_rpms_in_dir, create_user_keys`
		`-`
		`+ from copr_backend.sign import (`
		`+ get_pubkey, _sign_one, sign_rpms_in_dir, create_user_keys,`
		`+ gpg_hashtype_for_chroot,`
		`+ )`

		`STDOUT = "stdout"`
		`STDERR = "stderr"`
		`@@ -29,6 +29,7 @@`
		`self.tmp_dir_path = None`

		`self.opts = Munch(keygen_host="example.com")`
		`+ self.opts.gently_gpg_sha256 = False`

		`def teardown_method(self, method):`
		`if self.tmp_dir_path:`
		`@@ -118,10 +119,11 @@`
		`mc_popen.return_value = mc_handle`

		`fake_path = "/tmp/pkg.rpm"`
		`- result = _sign_one(fake_path, self.usermail, MagicMock())`
		`+ result = _sign_one(fake_path, self.usermail, "sha1", MagicMock())`
		`assert STDOUT, STDERR == result`

		`- expected_cmd = ['/bin/sign', '-u', self.usermail, '-r', fake_path]`
		`+ expected_cmd = ['/bin/sign', "-h", "sha1", "-u", self.usermail,`
		`+ "-r", fake_path]`
		`assert mc_popen.call_args[0][0] == expected_cmd`

		`@mock.patch("copr_backend.sign.Popen")`
		`@@ -130,7 +132,7 @@`

		`fake_path = "/tmp/pkg.rpm"`
		`with pytest.raises(CoprSignError):`
		`- _sign_one(fake_path, self.usermail, MagicMock())`
		`+ _sign_one(fake_path, self.usermail, "sha256", MagicMock())`

		`@mock.patch("copr_backend.sign.Popen")`
		`def test_sign_one_cmd_erro(self, mc_popen):`
		`@@ -141,7 +143,7 @@`

		`fake_path = "/tmp/pkg.rpm"`
		`with pytest.raises(CoprSignError):`
		`- _sign_one(fake_path, self.usermail, MagicMock())`
		`+ _sign_one(fake_path, self.usermail, "sha256", MagicMock())`

		`@mock.patch("copr_backend.sign.request")`
		`def test_create_user_keys(self, mc_request):`
		`@@ -182,7 +184,8 @@`
		`tmp_dir):`
		`# empty target dir doesn't produce error`
		`sign_rpms_in_dir(self.username, self.projectname,`
		`- self.tmp_dir_path, self.opts, log=MagicMock())`
		`+ self.tmp_dir_path, "epel-8-x86_64", self.opts,`
		`+ log=MagicMock())`

		`assert not mc_gp.called`
		`assert not mc_cuk.called`
		`@@ -195,7 +198,8 @@`
		`tmp_dir, tmp_files):`

		`sign_rpms_in_dir(self.username, self.projectname,`
		`- self.tmp_dir_path, self.opts, log=MagicMock())`
		`+ self.tmp_dir_path, "fedora-rawhide-x86_64",`
		`+ self.opts, log=MagicMock())`

		`assert mc_gp.called`
		`assert not mc_cuk.called`
		`@@ -218,7 +222,8 @@`
		`mc_gp.side_effect = CoprSignError("foobar")`
		`with pytest.raises(CoprSignError):`
		`sign_rpms_in_dir(self.username, self.projectname,`
		`- self.tmp_dir_path, self.opts, log=MagicMock())`
		`+ self.tmp_dir_path, "epel-7-x86_64", self.opts,`
		`+ log=MagicMock())`

		`assert mc_gp.called`
		`assert not mc_cuk.called`
		`@@ -233,7 +238,8 @@`
		`mc_gp.side_effect = CoprSignNoKeyError("foobar")`

		`sign_rpms_in_dir(self.username, self.projectname,`
		`- self.tmp_dir_path, self.opts, log=MagicMock())`
		`+ self.tmp_dir_path, "rhel-7-x86_64", self.opts,`
		`+ log=MagicMock())`

		`assert mc_gp.called`
		`assert mc_cuk.called`
		`@@ -250,7 +256,8 @@`
		`]`
		`with pytest.raises(CoprSignError):`
		`sign_rpms_in_dir(self.username, self.projectname,`
		`- self.tmp_dir_path, self.opts, log=MagicMock())`
		`+ self.tmp_dir_path, "fedora-36-x86_64", self.opts,`
		`+ log=MagicMock())`

		`assert mc_gp.called`
		`assert not mc_cuk.called`
		`@@ -266,9 +273,60 @@`
		`mc_so.side_effect = CoprSignError("foobar")`
		`with pytest.raises(CoprSignError):`
		`sign_rpms_in_dir(self.username, self.projectname,`
		`- self.tmp_dir_path, self.opts, log=MagicMock())`
		`+ self.tmp_dir_path, "fedora-36-i386", self.opts,`
		`+ log=MagicMock())`

		`assert mc_gp.called`
		`assert not mc_cuk.called`

		`assert mc_so.called`
		`+`
		`+`
		`+ def test_chroot_gpg_hashes():`
		`+ chroots = [`
		`+ ("fedora-26-x86_64", "sha1"),`
		`+ ("fedora-27-s390x", "sha256"),`
		`+ ("fedora-eln-x86_64", "sha256"),`
		`+ ("fedora-rawhide-x86_64", "sha256"),`
		`+ ("mageia-8-x86_64", "sha256"),`
		`+ ("opensuse-tumbleweed-aarch64", "sha256"),`
		`+ ("epel-7-ppc64", "sha1"),`
		`+ ("centos-7.dev-aarch64", "sha1"),`
		`+ ("epel-8-aarch64", "sha256"),`
		`+ ("rhel-8.dev-ppc64le", "sha256"),`
		`+ ("oraclelinux-9-s390x", "sha256"),`
		`+ ("centos-stream-8-s390x", "sha256"),`
		`+ ("centos-stream-9-s390x", "sha256"),`
		`+ ("rhel-rawhide-s390x", "sha256"),`
		`+ # we don't expect stream 7 will ever exist, otherwise we'll have to`
		`+ # check for sha1 here`
		`+ ("centos-stream-7-aarch64", "sha256"),`
		`+ ("srpm-builds", "sha256"),`
		`+ ]`
		`+`
		`+ opts = Munch()`
		`+ opts.gently_gpg_sha256 = False`
		`+`
		`+ for chroot, exp_type in chroots:`
		`+ assert (chroot, exp_type) == (chroot, gpg_hashtype_for_chroot(chroot, opts))`
		`+`
		`+ opts.gently_gpg_sha256 = True`
		`+ chroots = [`
		`+ ("fedora-26-x86_64", "sha1"),`
		`+ ("fedora-27-s390x", "sha1"),`
		`+ ("fedora-eln-x86_64", "sha1"),`
		`+ ("fedora-rawhide-x86_64", "sha1"),`
		`+ ("mageia-8-x86_64", "sha1"),`
		`+ ("opensuse-tumbleweed-aarch64", "sha1"),`
		`+ ("epel-7-ppc64", "sha1"),`
		`+ ("centos-7.dev-aarch64", "sha1"),`
		`+ ("epel-8-aarch64", "sha256"),`
		`+ ("rhel-8.dev-ppc64le", "sha256"),`
		`+ ("oraclelinux-9-s390x", "sha256"),`
		`+ ("centos-stream-8-s390x", "sha256"),`
		`+ ("centos-stream-9-s390x", "sha256"),`
		`+ ("rhel-rawhide-s390x", "sha1"),`
frostyx commented 2 years ago I don't know what rhel-rawhide is but it sounds like it belongs to "start signing with sha256 for EL8+" ?
praiskup commented 2 years ago It's not a real thing nowadays, but it used to be in Red Hat Copr in the past ... (fedora-eln is basically the replacement). but note sh1 is here with "gently_gpg_sha256 = True" (the initial phase). Once we flip to False (very soon), it will be sha256 everywhere.
		`+ ("srpm-builds", "sha1"),`
		`+ ]`
		`+ for chroot, exp_type in chroots:`
		`+ assert (chroot, exp_type) == (chroot, gpg_hashtype_for_chroot(chroot, opts))`

no initial comment

Metadata Update from @praiskup:
- Pull-request tagged with: wip

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 2m 08s

rebased onto 25b3f1db6ad542294d4a896854a5c11da0d674d3

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 1m 53s

2 new commits added

backend: copr_fix_gpg: cleanup logging PyLint issues
backend: copr_fix_gpg safety/optimization

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 2m 09s

1 new commit added

backend: copr_fix_gpg: add --chroot option

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 1m 55s

rebased onto 94aef85fbd15219ef504642c13db01e01144cc50

2 years ago

rebased onto 827043c106f0991b39e13c3202780a8aa5bbbeb5

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 2m 16s

Metadata Update from @praiskup:
- Pull-request untagged with: wip

2 years ago

rebased onto 1d8a064fdc3fe73fc89708e5eb1b67a228cf1c89

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 2m 14s

praiskup commented 2 years ago

This seems to work (deployed to the stage). Please note issue #2112 to be able to fully reproduce the original problem.

The example:
https://copr.stg.fedoraproject.org/coprs/praiskup/test-signing-before-update/builds/
(two builds, dummy-pkg signed with sha1 before update, signed-with-256 package is signed after update with the sha256).

praiskup commented 2 years ago

Note the approach in this PR is to
- use sha256 everywhere for the new packages, except for old chroots (EL7 or older)
- re-sign all the existing builds in EL9 chroots (keep others with sha1)

This proves that the old "pub" keys can be used with both "sha1" and "sha256"
signatures. This also proves that "sha1" and "sha256" packages can coexist
in one repository, and be installed (try epel-8-x86_64 chroot above).

rebased onto 5a8ceddcb515633a3798c4789a4da139a23fe9a7

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 1m 50s

5 new commits added

backend: use sha256 temporarily only for EL8+
backend: copr_fix_gpg: add --chroot option
backend: copr_fix_gpg: cleanup logging PyLint issues
backend: copr_fix_gpg safety/optimization
backend: add support for sha256 signatures

2 years ago

rebased onto 1eab2ffbdcd173a97417fcc5cb0c01a84c63d379

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 3m 05s

rebased onto 2502c157e615399b360a5227226070e1c03c8585

2 years ago

praiskup commented 2 years ago

I added yet another commit that added gently_gpg_sha256 option (default=True).
So the default behavior will be hash sha1 for everything except for EL8+ where
will be sha256. This way we'll test on EL8+ that everything works fine.
We also have a request from LEAPP team (cc @pstodulk) that they need
sha256 urgently also for the el8 chroots.

So the plan is to:
- start signing with sha256 for EL8+
- re-sign old EL9 packages with sha256 (but keep old EL8 on sha1)
- monitor that everything is fine (mixed sha1/sha256 in el8, sha256 on el9)
- after a month or so disable gently_gpg_256 (see my previous comment)

In case of any problem occurred, we would have to "only" re-sign EL8+ chroots
(resigning ~1.5TB of the package data, instead of all the 13TB).

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 1m 57s

praiskup commented 2 years ago

I believe this is ready for review.

rebased onto 1a6ac8909a7393c4076d003f58a7408570c59b2b

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 2m 30s

rebased onto c856f7f

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 1m 56s

frostyx commented on line 172 of backend/tests/test_sign.py 2 years ago

I don't know what rhel-rawhide is but it sounds like it belongs to "start signing with sha256 for EL8+" ?

frostyx commented on line 44 of backend/run/copr_fix_gpg.py 2 years ago

Can you please add a comment with some example of a directory that it is matching? I know it is for those <build_id>-<package_name> directories but regexes are always hard to understand after a while.

praiskup commented on line 172 of backend/tests/test_sign.py 2 years ago

It's not a real thing nowadays, but it used to be in Red Hat Copr in the past ... (fedora-eln is basically the replacement).
but note sh1 is here with "gently_gpg_sha256 = True" (the initial phase). Once we flip to False (very soon), it will be sha256 everywhere.

praiskup commented on line 44 of backend/run/copr_fix_gpg.py 2 years ago

Sure, done.

5 new commits added