dns extension for automatic key management #1085

msehnout · 2018-05-11T10:37:10Z

Motivation
Every time a new repository is added with GPG signed packages, dnf asks the user to accept the verification key, but there seems to be very little amount of users who actually do the key verification instead of simply accepting the key.

Importing GPG key 0x46CD093F:
 Userid     : "msehnout_neovim (None) <msehnout#neovim@copr.fedora.org>"
 Fingerprint: 5E84 DA77 A5C0 121E D09C 60B9 4603 C1E8 46CD 093F
 From       : https://copr-be.cloud.fedora.org/msehnout/neovim/pubkey.gpg
Is this ok [y/N]: y
Key imported successfully

Of course the official keys are available in the local file system:

gpgkey=file:///etc/pki/rpm-gpg/RPM-GPG-KEY-fedora-$releasever-$basearch

and others are downloaded over TLS secured HTTP:

gpgkey=https://dl.google.com/linux/linux_signing_key.pub

but still, there is no need to do the verification manually.

Unfortunately there is no way of automatic key revocation. GPG keys are stored inside of the RPM database and they simply stay there forever.

Implementation
DNS can be used to store any data even GPG keys as defined in RFC 7929:
https://tools.ietf.org/html/rfc7929
These records are then signed using DNSSEC, thus ensuring authenticity.

dnf can use libundound as an implementation of a validating resolver to obtain the key using DNS and verify it using DNSSEC. This way the verification process will be automatic.

Testing
I am currently using custom VM with a simulation of the DNS hierarchy. I could try to turn it into a Vagrantfile, that would run the testing environment. Is it ok for you or would you like to use a different approach to testing?

Pending issues

libunbound configuration file
testing of backwards compatibility (some domains might not have correct DNSSEC setup)

This work is based on my master's thesis, but the text is not finished yet. It will be ready in 2-3 weeks.

m-blaha · 2018-05-31T04:51:18Z

dnf/base.py

@@ -37,6 +37,7 @@
 import dnf.conf
 import dnf.conf.read
 import dnf.crypto
+import dnf.dnssec.dnsseckeyverification as dnssec


Either make from dnssec directory subpackage (using __init__.py and then do from dnf import dnssec), or if you do not plan to add more files into it, you can simply move dnssecverification.py to dnf/dnssec.py.

m-blaha · 2018-05-31T04:56:35Z

dnf/base.py

+                    # and therefore there is no way to know for sure (this is mainly for
+                    # backward compatibility)
+                    if dns_result == dnssec.Validity.VALID or \
+                            dns_result == dnssec.Validity.PROVEN_NONEXISTENCE:


you can shorten this to if dns_result in (dnssec.Validity.VALID, dnssec.Validity.PROVEN_NONEXISTENCE):

m-blaha · 2018-05-31T04:58:10Z

dnf/base.py

+                    if dns_result == dnssec.Validity.VALID or \
+                            dns_result == dnssec.Validity.PROVEN_NONEXISTENCE:
+                        rc = True
+                        logger.info(dnssec.any_msg("The key has been approved."))


All string literals should be translated from dnf.i18n import _, _("The key has been approved.")

m-blaha · 2018-05-31T05:01:27Z

dnf/cli/main.py

@@ -87,6 +89,7 @@ def _main(base, args, cli_class, option_parser):

    # our core object for the cli
    base._logging._presetup()
+    dnssec.RpmImportedKeys.check_imported_keys_validity(logger)


Instead of passing logger as parameter, you can do logger = logging.getLogger('dnf') in dnssec module.

m-blaha · 2018-05-31T05:07:30Z

dnf/dnssec/dnsseckeyverification.py

+        return digest + "." + tag + "." + domain
+    else:
+        # Error
+        return "Error"


This is strange - later you are not able to distinguish whether returned value is a regular location or an error. Consider returning None or raising Exception in case of incorrect input.

m-blaha · 2018-05-31T06:13:03Z

dnf/dnssec/dnsseckeyverification.py

+    # type: Dict[str, Union[str, NoKey]]
+
+    @staticmethod
+    def __cache_hit(key_union, input_key_string):


again, no two leading underscores please.

m-blaha · 2018-05-31T06:16:26Z

dnf/dnssec/dnsseckeyverification.py

+    @staticmethod
+    def __cache_miss(input_key):
+        # type: (KeyInfo) -> Validity
+        RR_TYPE_OPENPGPKEY = 61


move constants definition to begining of module please

m-blaha · 2018-05-31T06:17:10Z

dnf/dnssec/dnsseckeyverification.py

+        RR_TYPE_OPENPGPKEY = 61
+        ctx = unbound.ub_ctx()
+        # TODO: is this the right place to put this file?
+        ctx.config("/etc/dnf/libunbound.conf")


No please, it should be possible to use unbound module without any conf file.

m-blaha · 2018-05-31T06:46:29Z

dnf/dnssec/dnsseckeyverification.py

+        p1 = subprocess.Popen(["rpm", "-q", "gpg-pubkey"], stdout=subprocess.PIPE)
+        out = p1.communicate()[0]
+        keys = out.decode().split('\n')
+        return [x for x in keys if x.startswith('gpg-pubkey')]


I'd like you to use rpm module instead. Something like (you will need to get dnf.base.Base instance somehow):

import dnf.rpm.transaction ts = dnf.rpm.transaction.TransactionWrapper(base.conf.installroot) ['{}-{}-{}'.format(hdr['name'], hdr['version'], hdr['release']) for hdr in ts.dbMatch("name", "gpg-pubkey")]

m-blaha · 2018-05-31T07:29:16Z

dnf/dnssec/dnsseckeyverification.py

+    def __pkg_name_into_key(pkg):
+        # type: (str) -> KeyInfo
+        # Load output of the rpm -qi call
+        p1 = subprocess.Popen(["rpm", "-qi", pkg], stdout=subprocess.PIPE)


The same here, use rpm library.

msehnout · 2018-05-31T08:17:43Z

Thanks for the review. I added some comments directly to yours. The rest is obvious.

dnf-bot · 2018-07-02T12:18:43Z

Flake8 detected 18 issues on b1d88ea
Visit https://sider.review/gh/3671909/pull_requests/1085 to review the issues.

msehnout · 2018-07-02T12:26:20Z

I rewrote parts of the code where you requested changes. Missing parts are:

handle exceptions raised in the DNSSEC extension
make this feature optional (this requires a patch for libdnf).
finish testing environment: https://github.com/msehnout/dnf-testing

dnf-bot · 2018-08-14T09:40:47Z

dnf/dnssec/__init__.py

+from dnf.dnssec.dnsseckeyverification import DNSSECKeyVerification, KeyInfo, RpmImportedKeys,\
+    Validity
+# Functions
+from dnf.dnssec.dnsseckeyverification import any_msg, email2location, nice_user_msg


[Flake8]

[F401] 'dnf.dnssec.dnsseckeyverification.any_msg' imported but unused

dnf-bot · 2018-08-14T09:40:48Z

dnf/base.py

+                    # and therefore there is no way to know for sure (this is mainly for
+                    # backward compatibility)
+                    if self.conf.dns_verification:
+                        if dns_result in (dnssec.Validity.VALID, dnssec.Validity.PROVEN_NONEXISTENCE):


[Flake8]

[E501] line too long (102 > 100 characters)

dnf-bot · 2018-08-14T09:40:48Z

dnf/dnssec/__init__.py

+import dnf.exceptions
+
+# Classes
+from dnf.dnssec.dnsseckeyverification import DNSSECKeyVerification, KeyInfo, RpmImportedKeys,\


[Flake8]

[F401] 'dnf.dnssec.dnsseckeyverification.DNSSECKeyVerification' imported but unused

tyll · 2018-08-15T10:17:33Z

I have a few questions:

How does this handle key revocation?
Will this be used to verify keys from the local file system/downloaded via HTTPS additionally via DNS?

msehnout · 2018-08-15T10:47:57Z

How does this handle key revocation?

It creates a list of all keys installed in the RPM database and checks one by one before any transaction.

dnf/dnf/cli/main.py

Line 93 in 9015583

dnssec.RpmImportedKeys.check_imported_keys_validity()

dnf/dnf/dnssec/dnsseckeyverification.py

Line 203 in 9015583

class RpmImportedKeys:

Will this be used to verify keys from the local file system/downloaded via HTTPS additionally via DNS?

Yes, this has no information about the key origin.

Conan-Kudo · 2018-09-17T01:56:11Z

@msehnout This is an interesting idea, but the implementation needs to move to libdnf or otherwise half of the package manager stack won't be able to use it.

msehnout · 2018-09-17T09:49:15Z

@Conan-Kudo, can you please elaborate more on what exact components of the stack won't be able to use it?

Conan-Kudo · 2018-09-17T10:17:33Z

@msehnout PackageKit, RPM-OSTree, and microdnf use libdnf. Stuff only implemented in DNF won't work for those tools.

msehnout · 2018-09-17T10:26:22Z

@Conan-Kudo, well that is unfortunate. But, I'd go with this implementation for now. We still need to cooperate with Fedora Infra and Copr to make it useful at least in dnf. Support for libdnf and all the tools that require it can be implemented later on.

Conan-Kudo · 2018-09-17T10:28:07Z

Unfortunately, that's not going to be true for much longer. A big part of what's been going on in DNF development has been to move the core logic from Python to C++ to improve performance and consistency in behavior. @m-blaha should be able to help with implementing this in libdnf.

msehnout · 2018-09-17T10:32:35Z

Still, this PR can be merged and replaced with different implementation later on.

msehnout · 2018-09-18T07:44:17Z

@m-blaha any update?

dmach · 2018-09-19T11:00:07Z

dnf/cli/main.py

@@ -42,6 +42,8 @@
 import os.path
 import sys

+import dnf.dnssec.dnsseckeyverification as dnssec


Use absolute import:
import dnf.dnssec

dmach · 2018-09-19T11:00:18Z

dnf/base.py

@@ -47,6 +47,7 @@
 import dnf.conf
 import dnf.conf.read
 import dnf.crypto
+import dnf.dnssec as dnssec


Use absolute import:
import dnf.dnssec

dmach · 2018-09-19T11:02:00Z

dnf/dnssec/__init__.py

+import dnf.exceptions
+
+# Classes
+from dnf.dnssec.dnsseckeyverification import DNSSECKeyVerification, KeyInfo, RpmImportedKeys,\


merge these into this file and rename it to dnf/dnssec.py

dmach · 2018-09-19T11:02:34Z

dnf/CMakeLists.txt

@@ -10,3 +10,4 @@ ADD_SUBDIRECTORY (module)
 ADD_SUBDIRECTORY (rpm)
 ADD_SUBDIRECTORY (yum)
 ADD_SUBDIRECTORY (db)
+ADD_SUBDIRECTORY (dnssec)


not needed after making other changes

dmach · 2018-09-19T11:06:16Z

dnf/base.py

@@ -2298,14 +2299,33 @@ def _prov_key_data(msg):
                    logger.info(msg, keyurl, info.short_id)
                    continue

+                # DNS Extension: create a key object, pass it to the verification class
+                # and print its result as an advice to the user.
+                if self.conf.dns_verification:


rename to gpgkey_dns_verification

dmach · 2018-09-19T11:08:25Z

dnf/base.py

+                    # and therefore there is no way to know for sure (this is mainly for
+                    # backward compatibility)
+                    if self.conf.dns_verification:
+                        if dns_result in (dnssec.Validity.VALID, dnssec.Validity.PROVEN_NONEXISTENCE):


add a comment what PROVEN_NONEXISTENCE means
it doesn't make sense for someone who doesn't understand dnssec

dmach · 2018-09-19T11:17:20Z

dnf/cli/main.py

@@ -87,6 +89,8 @@ def _main(base, args, cli_class, option_parser):

    # our core object for the cli
    base._logging._presetup()
+    # TODO: move somewhere I can check configuration options
+    dnssec.RpmImportedKeys.check_imported_keys_validity()


move to base.py - fill_sack(), probably somewhere under "if load_available_repos:"
we want dnssec checking only if we load remote repositories and work with them
revoking keys makes sense only before downloading and installing something from repos,
having revoked keys on system is ok for remove, local queries, etc.

dmach · 2018-09-19T11:18:17Z

dnf/dnssec/dnsseckeyverification.py

+import hashlib
+import logging
+import re
+import unbound


fix import ordering: system, newline, 3rd party, newline, project

also move unbound import to _cache_miss
if unbound import fails with ImportError -> raise RuntimeError explaining that gpgkey_dns_verification requires unbound.

dmach · 2018-09-19T11:20:10Z

dnf/dnssec/dnsseckeyverification.py

+            .lower()
+        return digest + "." + tag + "." + domain
+    else:
+        raise DnssecError


raise DnssecError()

dmach · 2018-09-19T11:21:12Z

dnf/dnssec/dnsseckeyverification.py

+    :return:
+    """
+    split = email_address.split("@")
+    if len(split) == 2:


suggestion:
if len(split) != 2:
raise DnssecError()
and then save 4 spaces of indentation

dmach · 2018-09-19T11:33:33Z

We have decided to wait for requested changes to be made and then we'll merge this in.
It's good enough to be tested by dnf users.
Later on, we'll move the code into libdnf.

dnf-bot · 2018-09-20T10:14:38Z

dnf/dnssec.py

+                logger.info(any_msg("GPG Key {} is valid".format(key.email)))
+            elif result == Validity.PROVEN_NONEXISTENCE:
+                logger.info(any_msg("GPG Key {} does not support DNS verification"
+                                .format(key.email)))


[Flake8]

[E128] continuation line under-indented for visual indent

dnf-bot · 2018-09-20T10:14:39Z

dnf/dnssec.py

+        for key in keys:
+            result = DNSSECKeyVerification.verify(key)
+            # TODO: remove revoked keys automatically
+            #logger.info(any_msg("Key associated with identity " + key.email +


[Flake8]

[E265] block comment should start with '# '

dnf-bot · 2018-09-20T10:14:39Z

dnf/dnssec.py

+                logger.info(any_msg("GPG Key {} has been revoked and should be removed immediately"
+                                .format(key.email)))
+            else:
+                logger.info(any_msg("GPG Key {} could not be tested".format(key.email)))


[Flake8]

[W292] no newline at end of file

dnf-bot · 2018-09-20T10:14:40Z

dnf/dnssec.py

+                                .format(key.email)))
+            elif result == Validity.REVOKED:
+                logger.info(any_msg("GPG Key {} has been revoked and should be removed immediately"
+                                .format(key.email)))


[Flake8]

[E128] continuation line under-indented for visual indent

dnf-bot · 2018-09-20T10:14:40Z

dnf/dnssec.py

+                logger.info(any_msg("GPG Key {} could not be verified, because DNSSEC signatures are\
+                                bogus. Possible causes: wrong configuration of the DNS server, \
+                                MITM attack"
+                                .format(key.email)))


[Flake8]

[E128] continuation line under-indented for visual indent

dnf/dnssec.py

msehnout · 2018-09-24T11:03:52Z

I wrote an additional test for copr builds. It is available here:
https://github.com/msehnout/dnf-testing/blob/master/tests/happy-path-from-copr-build.sh

m-blaha · 2018-09-25T13:18:54Z

@rh-atomic-bot r+

rh-atomic-bot · 2018-09-25T13:18:55Z

📌 Commit bbc78f0 has been approved by m-blaha

rh-atomic-bot · 2018-09-25T13:19:01Z

⌛ Testing commit bbc78f0 with merge 04c11bf...

Closes: #1085 Approved by: m-blaha

THe Python API for Unbound is required for the DNSSEC extension to work, but it is optional. The second dependency is python2-enum34 which backports support for enumerations into Python2. An enumeration is used for expressing validity in a typed way as opposed to plain integer constants. This type is then used in type hints, which are written in Python2 compatible way (comments under function definitions). Closes: #1085 Approved by: m-blaha

Closes: #1085 Approved by: m-blaha

rh-atomic-bot · 2018-09-25T23:28:57Z

💥 Test timed out

m-blaha · 2018-09-27T06:12:12Z

@rh-atomic-bot retry

rh-atomic-bot · 2018-09-27T06:12:20Z

⌛ Testing commit bbc78f0 with merge b4a91a2...

Closes: #1085 Approved by: m-blaha

THe Python API for Unbound is required for the DNSSEC extension to work, but it is optional. The second dependency is python2-enum34 which backports support for enumerations into Python2. An enumeration is used for expressing validity in a typed way as opposed to plain integer constants. This type is then used in type hints, which are written in Python2 compatible way (comments under function definitions). Closes: #1085 Approved by: m-blaha

Closes: #1085 Approved by: m-blaha

rh-atomic-bot · 2018-09-29T02:20:34Z

💥 Test timed out

THe Python API for Unbound is required for the DNSSEC extension to work, but it is optional. The second dependency is python2-enum34 which backports support for enumerations into Python2. An enumeration is used for expressing validity in a typed way as opposed to plain integer constants. This type is then used in type hints, which are written in Python2 compatible way (comments under function definitions).

m-blaha self-assigned this May 30, 2018

m-blaha requested changes May 31, 2018

View reviewed changes

msehnout force-pushed the master branch from 353bd79 to b1d88ea Compare July 2, 2018 12:18

msehnout mentioned this pull request Aug 2, 2018

Config option for automatic key validation using DNS rpm-software-management/libdnf#526

Closed

msehnout force-pushed the master branch from b1d88ea to 9015583 Compare August 14, 2018 09:39

dnf-bot reviewed Aug 14, 2018

View reviewed changes

dmach requested changes Sep 19, 2018

View reviewed changes

msehnout force-pushed the master branch from 9015583 to 4104e87 Compare September 20, 2018 10:13

dnf-bot reviewed Sep 20, 2018

View reviewed changes

msehnout force-pushed the master branch from 4104e87 to b4ab2ed Compare September 20, 2018 11:05

dnf-bot reviewed Sep 20, 2018

View reviewed changes

dnf/dnssec.py Outdated Show resolved Hide resolved

msehnout mentioned this pull request Sep 24, 2018

testing suite for dnssec extension rpm-software-management/ci-dnf-stack#397

Open

rh-atomic-bot pushed a commit that referenced this pull request Sep 25, 2018

dnssec extension

effa3b9

Closes: #1085 Approved by: m-blaha

rh-atomic-bot pushed a commit that referenced this pull request Sep 25, 2018

documentation for the dnssec extension

2dc7ce2

Closes: #1085 Approved by: m-blaha

rh-atomic-bot pushed a commit that referenced this pull request Sep 25, 2018

copyright note and future imports

fdba732

Closes: #1085 Approved by: m-blaha

rh-atomic-bot pushed a commit that referenced this pull request Sep 25, 2018

fix testing data for DNSSEC

c5aa174

Closes: #1085 Approved by: m-blaha

rh-atomic-bot pushed a commit that referenced this pull request Sep 25, 2018

use conditional name for python enum package

04c11bf

Closes: #1085 Approved by: m-blaha

rh-atomic-bot pushed a commit that referenced this pull request Sep 27, 2018

dnssec extension

930d6b0

Closes: #1085 Approved by: m-blaha

rh-atomic-bot pushed a commit that referenced this pull request Sep 27, 2018

documentation for the dnssec extension

8394bab

Closes: #1085 Approved by: m-blaha

rh-atomic-bot pushed a commit that referenced this pull request Sep 27, 2018

copyright note and future imports

8979c56

Closes: #1085 Approved by: m-blaha

rh-atomic-bot pushed a commit that referenced this pull request Sep 27, 2018

fix testing data for DNSSEC

d61cba9

Closes: #1085 Approved by: m-blaha

rh-atomic-bot pushed a commit that referenced this pull request Sep 27, 2018

use conditional name for python enum package

b4a91a2

Closes: #1085 Approved by: m-blaha

m-blaha approved these changes Oct 2, 2018

View reviewed changes

Martin Sehnoutka added 6 commits October 2, 2018 15:07

dnssec extension

8fc74d9

documentation for the dnssec extension

696c0a0

copyright note and future imports

8a930b7

fix testing data for DNSSEC

b3220dc

use conditional name for python enum package

54eec8e

msehnout force-pushed the master branch from bbc78f0 to 54eec8e Compare October 2, 2018 13:14

m-blaha merged commit c90c00c into rpm-software-management:master Oct 3, 2018

dns extension for automatic key management #1085

dns extension for automatic key management #1085

Conversation

msehnout commented May 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msehnout commented May 31, 2018

dnf-bot commented Jul 2, 2018

msehnout commented Jul 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyll commented Aug 15, 2018

msehnout commented Aug 15, 2018

Conan-Kudo commented Sep 17, 2018

msehnout commented Sep 17, 2018

Conan-Kudo commented Sep 17, 2018

msehnout commented Sep 17, 2018

Conan-Kudo commented Sep 17, 2018

msehnout commented Sep 17, 2018

msehnout commented Sep 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmach commented Sep 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msehnout commented Sep 24, 2018

m-blaha commented Sep 25, 2018

rh-atomic-bot commented Sep 25, 2018

rh-atomic-bot commented Sep 25, 2018

rh-atomic-bot commented Sep 25, 2018

m-blaha commented Sep 27, 2018

rh-atomic-bot commented Sep 27, 2018

rh-atomic-bot commented Sep 29, 2018