print additional information when verifying GPG key using DNS #1726
Conversation
dnf/crypto.py
Outdated
| if dns_result == dnf.dnssec.Validity.VALID: | ||
| logger.critical(_('Verified using DNS record with DNSSEC signature.')) | ||
| elif dns_result == dnf.dnssec.Validity.PROVEN_NONEXISTENCE: | ||
| logger.critical(_('Verified using DNS record. But NOT signed using DNSSEC.')) |
There was a problem hiding this comment.
This comment feels misleading to me. The state of "proven nonexistence" means that the GPG key is not in DNS system and DNSSEC verified the fact that it is not present. My suggestion would be either: "The domain supports DNSSEC, but does not contain DNS record for this key." But that might be too complicated, so simplified version could be to also use "NOT verified using DNS record". (any suggestions @pemensik? )
There was a problem hiding this comment.
This sentence should user help whether they should allow or deny the import of the key. It is not important to explain all technical details to the user.
If it is not helpful to the user, then we can even skip it (not print). But I have to admit, that I am puzzled here - because in "-y" mode we automatically import the gpg key when result is PROVEN_NONEXISTENCE.
There was a problem hiding this comment.
My idea might have been to preserve current behavior because in the default settings, dnf accepts the GPG key when running with -y and proven nonexistence means that we are sure the domain is not supposed to contain the key which is the case for most of the domains running RPM repos except for the official Fedora.
There was a problem hiding this comment.
Sure, after quick glance on dnf.dnssec code, it seems to me elif log would match dnf.dnssec.Validity.RESULT_NOT_SECURE. PROVEN_NONEXISTENCE means we have cryptographic proof no such key exists. I think the only outcome for user is to verify it manually (some other way). Do we even have to handle unsigned keys? They may lead to false assurance this is the correct key, when anyone may easily forge it. Just not provide any hints unless VALID.
dnf/crypto.py
Outdated
| if dns_result == dnf.dnssec.Validity.VALID: | ||
| logger.critical(_('Verified using DNS record with DNSSEC signature.')) | ||
| elif dns_result == dnf.dnssec.Validity.PROVEN_NONEXISTENCE: | ||
| logger.critical(_('Verified using DNS record. But NOT signed using DNSSEC.')) |
There was a problem hiding this comment.
Sure, after quick glance on dnf.dnssec code, it seems to me elif log would match dnf.dnssec.Validity.RESULT_NOT_SECURE. PROVEN_NONEXISTENCE means we have cryptographic proof no such key exists. I think the only outcome for user is to verify it manually (some other way). Do we even have to handle unsigned keys? They may lead to false assurance this is the correct key, when anyone may easily forge it. Just not provide any hints unless VALID.
dnf/crypto.py
Outdated
| if dns_result == dnf.dnssec.Validity.VALID: | ||
| logger.critical(_('Verified using DNS record with DNSSEC signature.')) | ||
| elif dns_result == dnf.dnssec.Validity.PROVEN_NONEXISTENCE: | ||
| logger.critical(_('Verified using DNS record. But NOT signed using DNSSEC.')) |
There was a problem hiding this comment.
Just remove the elif: please. Unsigned key is not proof for anything, just say not verified in else.
There was a problem hiding this comment.
I agree after reading the explanation. Thank you.
|
Updated according to the comments. |
|
Thanks guys for the patch and review! |
|
bors try |
tryBuild succeeded: |
4 participants
No description provided.