Use distro library for all OS version detection (#7467)

This pull request ensures that we use distro package in all the distribution version detection. It also replaces the custom systemd /etc/os-release parsing and adds a few version fingerprints to Apache override selection.

Fixes: #7405

* Revert "Try to use platform.linux_distribution() before distro equivalent (#7403)"

This reverts commit ca3077d034.

* Use distro for all os detection code

* Address review comments

* Add changelog entry

* Added tests

* Fix tests to return a consistent os name

* Do not crash on non-linux systems

* Minor fixes to distro compatibility checks

* Make the tests OS independent

* Update certbot/util.py

Co-Authored-By: Brad Warren <bmw@users.noreply.github.com>

* Skip linux specific tests on other platforms

* Test fixes

* Better test state handling

* Lower the coverage target for Windows tests

.codecov.yml

@@ -13,6 +13,6 @@ coverage:
         flags: windows
         # Fixed target instead of auto set by #7173, can
         # be removed when flags in Codecov are added back.
-        target: 97.7
+        target: 97.4
         threshold: 0.1
         base: auto

CHANGELOG.md

@@ -20,6 +20,7 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
   staging server instead of the live server when `--dry-run` is used.
 * Updated certbot-dns-google to depend on newer versions of
   google-api-python-client and oauth2client.
+* The OS detection logic again uses distro library for Linux OSes
 * certbot.plugins.common.TLSSNI01 has been deprecated and will be removed in a
   future release.
 * CLI flags --tls-sni-01-port and --tls-sni-01-address have been removed.

certbot-apache/certbot_apache/entrypoint.py

@@ -16,6 +16,7 @@ from certbot_apache import override_suse
 
 OVERRIDE_CLASSES = {
     "arch": override_arch.ArchConfigurator,
+    "cloudlinux": override_centos.CentOSConfigurator,
     "darwin": override_darwin.DarwinConfigurator,
     "debian": override_debian.DebianConfigurator,
     "ubuntu": override_debian.DebianConfigurator,
@@ -23,7 +24,9 @@ OVERRIDE_CLASSES = {
     "centos linux": override_centos.CentOSConfigurator,
     "fedora_old": override_centos.CentOSConfigurator,
     "fedora": override_fedora.FedoraConfigurator,
+    "linuxmint": override_debian.DebianConfigurator,
     "ol": override_centos.CentOSConfigurator,
+    "oracle": override_centos.CentOSConfigurator,
     "redhatenterpriseserver": override_centos.CentOSConfigurator,
     "red hat enterprise linux server": override_centos.CentOSConfigurator,
     "rhel": override_centos.CentOSConfigurator,
@@ -32,6 +35,7 @@ OVERRIDE_CLASSES = {
     "gentoo base system": override_gentoo.GentooConfigurator,
     "opensuse": override_suse.OpenSUSEConfigurator,
     "suse": override_suse.OpenSUSEConfigurator,
+    "sles": override_suse.OpenSUSEConfigurator,
     "scientific": override_centos.CentOSConfigurator,
     "scientific linux": override_centos.CentOSConfigurator,
 }

certbot/tests/util_test.py

@@ -1,6 +1,7 @@
 """Tests for certbot.util."""
 import argparse
 import errno
+import sys
 import unittest
 
 import mock
@@ -473,74 +474,92 @@ class IsWildcardDomainTest(unittest.TestCase):
 class OsInfoTest(unittest.TestCase):
     """Test OS / distribution detection"""
 
-    def test_systemd_os_release(self):
+    @mock.patch("certbot.util.distro")
-        from certbot.util import (get_os_info, get_systemd_os_info,
+    @unittest.skipUnless(sys.platform.startswith("linux"), "requires Linux")
-                                  get_os_info_ua)
+    def test_systemd_os_release_like(self, m_distro):
+        import certbot.util as cbutil
+        m_distro.like.return_value = "first debian third"
+        id_likes = cbutil.get_systemd_os_like()
+        self.assertEqual(len(id_likes), 3)
+        self.assertTrue("debian" in id_likes)
 
-        with mock.patch('certbot.compat.os.path.isfile', return_value=True):
+    @mock.patch("certbot.util.distro")
-            self.assertEqual(get_os_info(
+    @unittest.skipUnless(sys.platform.startswith("linux"), "requires Linux")
-                test_util.vector_path("os-release"))[0], 'systemdos')
+    def test_get_os_info_ua(self, m_distro):
-            self.assertEqual(get_os_info(
+        import certbot.util as cbutil
-                test_util.vector_path("os-release"))[1], '42')
+        with mock.patch('platform.system_alias',
-            self.assertEqual(get_systemd_os_info(os.devnull), ("", ""))
+                        return_value=('linux', '42', '42')):
-            self.assertEqual(get_os_info_ua(
+            m_distro.name.return_value = ""
-                test_util.vector_path("os-release")), "SystemdOS")
+            m_distro.linux_distribution.return_value = ("something", "1.0", "codename")
-        with mock.patch('certbot.compat.os.path.isfile', return_value=False):
+            cbutil.get_python_os_info(pretty=True)
-            self.assertEqual(get_systemd_os_info(), ("", ""))
+            self.assertEqual(cbutil.get_os_info_ua(),
+                            " ".join(cbutil.get_python_os_info(pretty=True)))
 
-    def test_systemd_os_release_like(self):
+        m_distro.name.return_value = "whatever"
-        from certbot.util import get_systemd_os_like
+        self.assertEqual(cbutil.get_os_info_ua(), "whatever")
 
-        with mock.patch('certbot.compat.os.path.isfile', return_value=True):
+    @mock.patch("certbot.util.distro")
-            id_likes = get_systemd_os_like(test_util.vector_path(
+    @unittest.skipUnless(sys.platform.startswith("linux"), "requires Linux")
-                "os-release"))
+    def test_get_os_info(self, m_distro):
-            self.assertEqual(len(id_likes), 3)
+        import certbot.util as cbutil
-            self.assertTrue("debian" in id_likes)
+        with mock.patch("platform.system") as mock_platform:
+            m_distro.linux_distribution.return_value = ("name", "version", 'x')
+            mock_platform.return_value = "linux"
+            self.assertEqual(cbutil.get_os_info(), ("name", "version"))
+
+            m_distro.linux_distribution.return_value = ("something", "else")
+            self.assertEqual(cbutil.get_os_info(), ("something", "else"))
+
+    @mock.patch("warnings.warn")
+    @mock.patch("certbot.util.distro")
+    @unittest.skipUnless(sys.platform.startswith("linux"), "requires Linux")
+    def test_get_systemd_os_info_deprecation(self, _, mock_warn):
+        import certbot.util as cbutil
+        cbutil.get_systemd_os_info()
+        self.assertTrue(mock_warn.called)
 
     @mock.patch("certbot.util.subprocess.Popen")
     def test_non_systemd_os_info(self, popen_mock):
-        from certbot.util import (get_os_info, get_python_os_info,
+        import certbot.util as cbutil
-                                     get_os_info_ua)
+        with mock.patch('certbot.util._USE_DISTRO', False):
-        with mock.patch('certbot.compat.os.path.isfile', return_value=False):
             with mock.patch('platform.system_alias',
                             return_value=('NonSystemD', '42', '42')):
-                self.assertEqual(get_os_info()[0], 'nonsystemd')
+                self.assertEqual(cbutil.get_python_os_info()[0], 'nonsystemd')
-                self.assertEqual(get_os_info_ua(),
-                                 " ".join(get_python_os_info()))
 
             with mock.patch('platform.system_alias',
                             return_value=('darwin', '', '')):
                 comm_mock = mock.Mock()
                 comm_attrs = {'communicate.return_value':
-                              ('42.42.42', 'error')}
+                            ('42.42.42', 'error')}
                 comm_mock.configure_mock(**comm_attrs)
                 popen_mock.return_value = comm_mock
-                self.assertEqual(get_os_info()[0], 'darwin')
+                self.assertEqual(cbutil.get_python_os_info()[0], 'darwin')
-                self.assertEqual(get_os_info()[1], '42.42.42')
+                self.assertEqual(cbutil.get_python_os_info()[1], '42.42.42')
-
-            with mock.patch('platform.system_alias',
-                            return_value=('linux', '', '')):
-                with mock.patch('platform.linux_distribution',
-                                side_effect=AttributeError,
-                                create=True):
-                    with mock.patch('distro.linux_distribution',
-                                    return_value=('', '', '')):
-                        self.assertEqual(get_python_os_info(), ("linux", ""))
-
-                    with mock.patch('distro.linux_distribution',
-                                    return_value=('testdist', '42', '')):
-                        self.assertEqual(get_python_os_info(), ("testdist", "42"))
 
             with mock.patch('platform.system_alias',
                             return_value=('freebsd', '9.3-RC3-p1', '')):
-                self.assertEqual(get_python_os_info(), ("freebsd", "9"))
+                self.assertEqual(cbutil.get_python_os_info(), ("freebsd", "9"))
 
             with mock.patch('platform.system_alias',
                             return_value=('windows', '', '')):
                 with mock.patch('platform.win32_ver',
                                 return_value=('4242', '95', '2', '')):
-                    self.assertEqual(get_python_os_info(),
+                    self.assertEqual(cbutil.get_python_os_info(),
-                                     ("windows", "95"))
+                                    ("windows", "95"))
+
+    @mock.patch("certbot.util.distro")
+    @unittest.skipUnless(sys.platform.startswith("linux"), "requires Linux")
+    def test_python_os_info_notfound(self, m_distro):
+        import certbot.util as cbutil
+        m_distro.linux_distribution.return_value = ('', '', '')
+        self.assertEqual(cbutil.get_python_os_info()[0], "linux")
+
+    @mock.patch("certbot.util.distro")
+    @unittest.skipUnless(sys.platform.startswith("linux"), "requires Linux")
+    def test_python_os_info_custom(self, m_distro):
+        import certbot.util as cbutil
+        m_distro.linux_distribution.return_value = ('testdist', '42', '')
+        self.assertEqual(cbutil.get_python_os_info(), ("testdist", "42"))
 
 
 class AtexitRegisterTest(unittest.TestCase):

certbot/util.py

@@ -12,9 +12,10 @@ import platform
 import re
 import socket
 import subprocess
+import sys
+import warnings
 
 import configargparse
-import distro
 import six
 
 from acme.magic_typing import Tuple, Union  # pylint: disable=unused-import, no-name-in-module
@@ -25,6 +26,12 @@ from certbot import lock
 from certbot.compat import os
 from certbot.compat import filesystem
 
+if sys.platform.startswith('linux'):
+    import distro
+    _USE_DISTRO = True
+else:
+    _USE_DISTRO = False
+
 logger = logging.getLogger(__name__)
 
 
@@ -277,77 +284,59 @@ def get_filtered_names(all_names):
             logger.debug('Not suggesting name "%s"', name, exc_info=True)
     return filtered_names
 
-
+def get_os_info():
-def get_os_info(filepath="/etc/os-release"):
     """
     Get OS name and version
 
-    :param str filepath: File path of os-release file
     :returns: (os_name, os_version)
     :rtype: `tuple` of `str`
     """
 
-    if os.path.isfile(filepath):
+    return get_python_os_info(pretty=False)
-        # Systemd os-release parsing might be viable
-        os_name, os_version = get_systemd_os_info(filepath=filepath)
-        if os_name:
-            return os_name, os_version
 
-    # Fallback to platform module
+def get_os_info_ua():
-    return get_python_os_info()
-
-
-def get_os_info_ua(filepath="/etc/os-release"):
     """
     Get OS name and version string for User Agent
 
-    :param str filepath: File path of os-release file
     :returns: os_ua
     :rtype: `str`
     """
+    if _USE_DISTRO:
+        os_info = distro.name(pretty=True)
 
-    if os.path.isfile(filepath):
+    if not _USE_DISTRO or not os_info:
-        os_ua = get_var_from_file("PRETTY_NAME", filepath=filepath)
+        return " ".join(get_python_os_info(pretty=True))
-        if not os_ua:
+    return os_info
-            os_ua = get_var_from_file("NAME", filepath=filepath)
-        if os_ua:
-            return os_ua
 
-    # Fallback
+def get_systemd_os_info():
-    return " ".join(get_python_os_info())
-
-
-def get_systemd_os_info(filepath="/etc/os-release"):
     """
     Parse systemd /etc/os-release for distribution information
 
-    :param str filepath: File path of os-release file
     :returns: (os_name, os_version)
     :rtype: `tuple` of `str`
     """
 
-    os_name = get_var_from_file("ID", filepath=filepath)
+    warnings.warn(
-    os_version = get_var_from_file("VERSION_ID", filepath=filepath)
+        "The get_sytemd_os_like() function is deprecated and will be removed in "
+        "a future release.", DeprecationWarning, stacklevel=2)
+    return get_os_info()[:2]
 
-    return (os_name, os_version)
+def get_systemd_os_like():
-
-
-def get_systemd_os_like(filepath="/etc/os-release"):
     """
     Get a list of strings that indicate the distribution likeness to
     other distributions.
 
-    :param str filepath: File path of os-release file
     :returns: List of distribution acronyms
     :rtype: `list` of `str`
     """
 
-    return get_var_from_file("ID_LIKE", filepath).split(" ")
+    if _USE_DISTRO:
-
+        return distro.like().split(" ")
+    return []
 
 def get_var_from_file(varname, filepath="/etc/os-release"):
     """
-    Get single value from systemd /etc/os-release
+    Get single value from a file formatted like systemd /etc/os-release
 
     :param str varname: Name of variable to fetch
     :param str filepath: File path of os-release file
@@ -367,7 +356,6 @@ def get_var_from_file(varname, filepath="/etc/os-release"):
             return _normalize_string(line.strip()[len(var_string):])
     return ""
 
-
 def _normalize_string(orig):
     """
     Helper function for get_var_from_file() to remove quotes
@@ -375,12 +363,13 @@ def _normalize_string(orig):
     """
     return orig.replace('"', '').replace("'", "").strip()
 
-
+def get_python_os_info(pretty=False):
-def get_python_os_info():
     """
     Get Operating System type/distribution and major version
     using python platform module
 
+    :param bool pretty: If the returned OS name should be in longer (pretty) form
+
     :returns: (os_name, os_version)
     :rtype: `tuple` of `str`
     """
@@ -391,8 +380,8 @@ def get_python_os_info():
     )
     os_type, os_ver, _ = info
     os_type = os_type.lower()
-    if os_type.startswith('linux'):
+    if os_type.startswith('linux') and _USE_DISTRO:
-        info = _get_linux_distribution()
+        info = distro.linux_distribution(pretty)
         # On arch, distro.linux_distribution() is reportedly ('','',''),
         # so handle it defensively
         if info[0]:
@@ -424,14 +413,6 @@ def get_python_os_info():
         os_ver = ''
     return os_type, os_ver
 
-def _get_linux_distribution():
-    """Gets the linux distribution name from the underlying OS"""
-
-    try:
-        return platform.linux_distribution()
-    except AttributeError:
-        return distro.linux_distribution()
-
 # Just make sure we don't get pwned... Make sure that it also doesn't
 # start with a period or have two consecutive periods <- this needs to
 # be done in addition to the regex

pytest.ini

@@ -18,3 +18,4 @@ filterwarnings =
     ignore:.*collections\.abc:DeprecationWarning
     ignore:The `color_scheme` argument is deprecated:DeprecationWarning:IPython.*
     ignore:.*view_config_changes:DeprecationWarning
+    ignore:.*get_systemd_os_info:DeprecationWarning