Fix parsing PGP keys for DNS validation #2019
Conversation
A PGP armor message can contain any amount of headers. Up to Fedora 38
there was one:
-----BEGIN PGP PUBLIC KEY BLOCK-----
Version: rpm-4.18.0-beta1
mQINBGIC2cYBEADJye1aE0AR17qwj6wsHWlCQlcihmqkL8s4gbOk1IevBbH4iXJx
[...]
=CHKS
-----END PGP PUBLIC KEY BLOCK-----
Since Fedora 39 there is none:
-----BEGIN PGP PUBLIC KEY BLOCK-----
mQINBGLykg8BEADURjKtgQpQNoluifXia+U3FuqGCTQ1w7iTqx1UvNhLX6tb9Qjy
l/vjl1iXxucrd2JBnrT/21BdtaABhu2hPy7bpcGEkG8MDinAMZBzcyzHcS/JiGHZ
[...]
=CHKS
-----END PGP PUBLIC KEY BLOCK-----
RpmImportedKeys._query_db_for_gpg_keys() assumed exactly one header.
As a result if gpgkey_dns_verification configuration option was true,
DNF reported that Fedora 39 keys was revoked because the key
misextratracted from RPM database did not match a key in DNS:
# dnf-3 upgrade
DNSSEC extension: Testing already imported keys for their validity.
DNSSEC extension: GPG Key [email protected] has been revoked and should be removed immediately
This patch implements skipping all armor headers.
https://bugzilla.redhat.com/show_bug.cgi?id=2249380
ppisar
force-pushed
the
dnssec_fix_parsing_pgp_keys
branch
from
dd1084d
to
74aef76
Compare
If an PGP key is stored in an RPM database without a "packager" RPM
header, or without an e-mail address there, DNS verification crashed
on converting the undefined address into a DNS domain. That was the
case of Fedora 13 key:
# dnf-3 upgrade
Traceback (most recent call last):
File "/usr/bin/dnf-3", line 62, in <module>
main.user_main(sys.argv[1:], exit_code=True)
File "/usr/lib/python3.12/site-packages/dnf/cli/main.py", line 201, in user_main
errcode = main(args)
^^^^^^^^^^
File "/usr/lib/python3.12/site-packages/dnf/cli/main.py", line 67, in main
return _main(base, args, cli_class, option_parser_class)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.12/site-packages/dnf/cli/main.py", line 106, in _main
return cli_run(cli, base)
^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.12/site-packages/dnf/cli/main.py", line 122, in cli_run
cli.run()
File "/usr/lib/python3.12/site-packages/dnf/cli/cli.py", line 1040, in run
self._process_demands()
File "/usr/lib/python3.12/site-packages/dnf/cli/cli.py", line 741, in _process_demands
self.base.fill_sack(
File "/usr/lib/python3.12/site-packages/dnf/base.py", line 403, in fill_sack
dnf.dnssec.RpmImportedKeys.check_imported_keys_validity()
File "/usr/lib/python3.12/site-packages/dnf/dnssec.py", line 286, in check_imported_keys_validity
keys = RpmImportedKeys._query_db_for_gpg_keys()
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.12/site-packages/dnf/dnssec.py", line 276, in _query_db_for_gpg_keys
email = re.search('<(.*@.*)>', packager).group(1)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib64/python3.12/re/__init__.py", line 177, in search
return _compile(pattern, flags).search(string)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: expected string or bytes-like object, got 'NoneType'
This patch defends the crash at two places: In
_query_db_for_gpg_keys() because here we know a NEVRA of the key
and can produce a meaningful message. And in _cache_miss() because
we can get there independenly and called email2location() would also
crash.
ppisar
force-pushed
the
dnssec_fix_parsing_pgp_keys
branch
from
74aef76
to
560807a
Compare
If a user had installed multiple keys for the same e-mail address in
an RPM database, and no records for the address existed in DNS, DNF
validated the first key correctly, but reported that the other key is
revoked:
# rpm -q gpg-pubkey --qf '%{packager} %{nevra}\n' |grep nokey
nokey1 <[email protected]> gpg-pubkey-7460757e-6553a6ab
nokey2 <[email protected]> gpg-pubkey-c8d04ba8-6553a6b1
# dnf-3 upgrade
DNSSEC extension: Testing already imported keys for their validity.
DNSSEC extension: GPG Key [email protected] has been revoked and should be removed immediately
The cause was a wrong test for a cached negative reponse. This patch
fixes it.
https://bugzilla.redhat.com/show_bug.cgi?id=2249380
|
@ppisar Since this is apparently not covered by any CI tests, how about adding them for these 3 mentioned use cases from each commit? If it seems time-consuming and you're out of capacity right now, I suggest creating an issue for that. Also, I believe the QE team can help us with this, as there is proper documentation on how to reproduce these scenarios. |
|
I agree this code would deserve tests. A problem is that one would have to fake a DNS server. Either by deploying a testing DNF server, including a signed zone and that would require injecting a testing DNSSEC root of trust, or by mocking up the Python unbound module. Therefore I conclude that a complexity of the test is inadequate to the this code. That confirms a 5-year-old request for the DNSSEC tests rpm-software-management/ci-dnf-stack#397. I placed a link to this pull request there. |
Thanks for the clarification, I agree with you. |
Fedora 39 slightly changed an armor format of PGP keys and as a result if gpgkey_dns_verification=true, DNF warned that Fedora 39 key is revoked. See https://bugzilla.redhat.com/show_bug.cgi?id=2249380. That's fixed with the first commit.
When I tried to reproduce it, I actually got a crash because an old Fedora 13 I still has imported had no e-mail address. The crash is addressed in the second commit.