only run relevant tests

The method `os.readlink()` has a significant behavior change with Python 3.8+ on Windows. 

Starting with this version, it will return the resolved path in its "extended-style" form unconditionally, a form which allows to use more than 259 characters in a Windows path, and its string representation is prepended with "\\\\?\\".

See https://docs.microsoft.com/fr-fr/windows/win32/fileio/naming-a-file?redirectedfrom=MSDN#maximum-path-length-limitation

Problem is that `os.readlink()` does it for any path, including paths that could be represented with the normal form. As a consequence, any string comparison with a path provided in the normal form will fail even if it represents the same path. This makes Certbot partially break on Windows with Python 3.8.

My proposition in this PR is to forbid `os.readlink()`, and provide `certbot.compat.filesystem.readlink()` which serves the same purpose at resolving the pointed path of a link, and has a consistent behavior over supported Python versions.

* Forbid os.readlink()

* Use readlink

* Raise error with long paths on Windows

* Add unit tests

* Update certbot/certbot/compat/filesystem.py

Co-authored-by: Brad Warren <bmw@users.noreply.github.com>

.azure-pipelines/templates/jobs/packaging-jobs.yml

@@ -1,62 +1,11 @@
 jobs:
-  - job: docker_build
-    pool:
-      vmImage: ubuntu-18.04
-    strategy:
-      matrix:
-        amd64:
-          DOCKER_ARCH: amd64
-        # Do not run the heavy non-amd64 builds for test branches
-        ${{ if not(startsWith(variables['Build.SourceBranchName'], 'test-')) }}:
-          arm32v6:
-            DOCKER_ARCH: arm32v6
-          arm64v8:
-            DOCKER_ARCH: arm64v8
-    steps:
-      - bash: set -e && tools/docker/build.sh $(dockerTag) $DOCKER_ARCH
-        displayName: Build the Docker images
-      # We don't filter for the Docker Hub organization to continue to allow
-      # easy testing of these scripts on forks.
-      - bash: |
-          set -e
-          DOCKER_IMAGES=$(docker images --filter reference='*/certbot' --filter reference='*/dns-*' --format '{{.Repository}}')
-          docker save --output images.tar $DOCKER_IMAGES
-        displayName: Save the Docker images
-      # If the name of the tar file or artifact changes, the deploy stage will
-      # also need to be updated.
-      - bash: set -e && mv images.tar $(Build.ArtifactStagingDirectory)
-        displayName: Prepare Docker artifact
-      - task: PublishPipelineArtifact@1
-        inputs:
-          path: $(Build.ArtifactStagingDirectory)
-          artifact: docker_$(DOCKER_ARCH)
-        displayName: Store Docker artifact
-  - job: docker_run
-    dependsOn: docker_build
-    pool:
-      vmImage: ubuntu-18.04
-    steps:
-      - task: DownloadPipelineArtifact@2
-        inputs:
-          artifact: docker_amd64
-          path: $(Build.SourcesDirectory)
-        displayName: Retrieve Docker images
-      - bash: set -e && docker load --input $(Build.SourcesDirectory)/images.tar
-        displayName: Load Docker images
-      - bash: |
-          set -ex
-          DOCKER_IMAGES=$(docker images --filter reference='*/certbot' --filter reference='*/dns-*' --format '{{.Repository}}:{{.Tag}}')
-          for DOCKER_IMAGE in ${DOCKER_IMAGES}
-            do docker run --rm "${DOCKER_IMAGE}" plugins --prepare
-          done
-        displayName: Run integration tests for Docker images
   - job: installer_build
     pool:
       vmImage: vs2017-win2016
     steps:
       - task: UsePythonVersion@0
         inputs:
-          versionSpec: 3.7
+          versionSpec: 3.8
           architecture: x86
           addToPath: true
       - script: python windows-installer/construct.py
@@ -113,105 +62,3 @@ jobs:
           set PATH=%ProgramFiles(x86)%\Certbot\bin;%PATH%
           venv\Scripts\python -m pytest certbot-ci\certbot_integration_tests\certbot_tests -n 4
         displayName: Run certbot integration tests
-  - job: snaps_build
-    pool:
-      vmImage: ubuntu-18.04
-    timeoutInMinutes: 0
-    variables:
-      # Do not run the heavy non-amd64 builds for test branches
-      ${{ if not(startsWith(variables['Build.SourceBranchName'], 'test-')) }}:
-        ARCHS: amd64 arm64 armhf
-      ${{ if startsWith(variables['Build.SourceBranchName'], 'test-') }}:
-        ARCHS: amd64
-    steps:
-      - script: |
-          set -e
-          sudo apt-get update
-          sudo apt-get install -y --no-install-recommends snapd
-          sudo snap install --classic snapcraft
-        displayName: Install dependencies
-      - task: UsePythonVersion@0
-        inputs:
-          versionSpec: 3.8
-          addToPath: true
-      - task: DownloadSecureFile@1
-        name: credentials
-        inputs:
-          secureFile: launchpad-credentials
-      - script: |
-          set -e
-          git config --global user.email "$(Build.RequestedForEmail)"
-          git config --global user.name "$(Build.RequestedFor)"
-          mkdir -p ~/.local/share/snapcraft/provider/launchpad
-          cp $(credentials.secureFilePath) ~/.local/share/snapcraft/provider/launchpad/credentials
-          python3 tools/snap/build_remote.py ALL --archs ${ARCHS} --timeout 19800
-        displayName: Build snaps
-      - script: |
-          set -e
-          mv *.snap $(Build.ArtifactStagingDirectory)
-          mv certbot-dns-*/*.snap $(Build.ArtifactStagingDirectory)
-        displayName: Prepare artifacts
-      - task: PublishPipelineArtifact@1
-        inputs:
-          path: $(Build.ArtifactStagingDirectory)
-          artifact: snaps
-        displayName: Store snaps artifacts
-  - job: snap_run
-    dependsOn: snaps_build
-    pool:
-      vmImage: ubuntu-18.04
-    steps:
-      - task: UsePythonVersion@0
-        inputs:
-          versionSpec: 3.8
-          addToPath: true
-      - script: |
-          set -e
-          sudo apt-get update
-          sudo apt-get install -y --no-install-recommends nginx-light snapd
-          python3 -m venv venv
-          venv/bin/python tools/pipstrap.py
-          venv/bin/python tools/pip_install.py -U tox
-        displayName: Install dependencies
-      - task: DownloadPipelineArtifact@2
-        inputs:
-          artifact: snaps
-          path: $(Build.SourcesDirectory)/snap
-        displayName: Retrieve Certbot snaps
-      - script: |
-          set -e
-          sudo snap install --dangerous --classic snap/certbot_*_amd64.snap
-        displayName: Install Certbot snap
-      - script: |
-          set -e
-          venv/bin/python -m tox -e integration-external,apacheconftest-external-with-pebble
-        displayName: Run tox
-  - job: snap_dns_run
-    dependsOn: snaps_build
-    pool:
-      vmImage: ubuntu-18.04
-    steps:
-      - script: |
-          set -e
-          sudo apt-get update
-          sudo apt-get install -y --no-install-recommends snapd
-        displayName: Install dependencies
-      - task: UsePythonVersion@0
-        inputs:
-          versionSpec: 3.8
-          addToPath: true
-      - task: DownloadPipelineArtifact@2
-        inputs:
-          artifact: snaps
-          path: $(Build.SourcesDirectory)/snap
-        displayName: Retrieve Certbot snaps
-      - script: |
-          set -e
-          python3 -m venv venv
-          venv/bin/python tools/pipstrap.py
-          venv/bin/python tools/pip_install.py -e certbot-ci
-        displayName: Prepare Certbot-CI
-      - script: |
-          set -e
-          sudo -E venv/bin/pytest certbot-ci/snap_integration_tests/dns_tests --allow-persistent-changes --snap-folder $(Build.SourcesDirectory)/snap --snap-arch amd64
-        displayName: Test DNS plugins snaps

.azure-pipelines/templates/jobs/standard-tests-jobs.yml

@@ -16,13 +16,13 @@ jobs:
           IMAGE_NAME: vs2017-win2016
           PYTHON_VERSION: 3.6
           TOXENV: py36
-        windows-py37-cover:
+        windows-py38-cover:
           IMAGE_NAME: vs2017-win2016
-          PYTHON_VERSION: 3.7
-          TOXENV: py37-cover
+          PYTHON_VERSION: 3.8
+          TOXENV: py38-cover
         windows-integration-certbot:
           IMAGE_NAME: vs2017-win2016
-          PYTHON_VERSION: 3.7
+          PYTHON_VERSION: 3.8
           TOXENV: integration-certbot
         linux-oldest-tests-1:
           IMAGE_NAME: ubuntu-18.04

.azure-pipelines/templates/stages/test-and-package-stage.yml

@@ -2,5 +2,4 @@ stages:
   - stage: TestAndPackage
     jobs:
       - template: ../jobs/standard-tests-jobs.yml
-      - template: ../jobs/extended-tests-jobs.yml
       - template: ../jobs/packaging-jobs.yml

certbot/certbot/_internal/account.py

@@ -20,6 +20,7 @@ from certbot import interfaces
 from certbot import util
 from certbot._internal import constants
 from certbot.compat import os
+from certbot.compat import filesystem
 
 logger = logging.getLogger(__name__)
 
@@ -324,7 +325,7 @@ class AccountFileStorage(interfaces.AccountStorage):
             if server_path in reused_servers:
                 next_server_path = reused_servers[server_path]
                 next_dir_path = link_func(next_server_path)
-                if os.path.islink(next_dir_path) and os.readlink(next_dir_path) == dir_path:
+                if os.path.islink(next_dir_path) and filesystem.readlink(next_dir_path) == dir_path:
                     possible_next_link = True
                     server_path = next_server_path
                     dir_path = next_dir_path
@@ -332,7 +333,7 @@ class AccountFileStorage(interfaces.AccountStorage):
         # if there's not a next one up to delete, then delete me
         # and whatever I link to
         while os.path.islink(dir_path):
-            target = os.readlink(dir_path)
+            target = filesystem.readlink(dir_path)
             os.unlink(dir_path)
             dir_path = target
 

certbot/certbot/_internal/storage.py

@@ -214,7 +214,7 @@ def get_link_target(link):
 
     """
     try:
-        target = os.readlink(link)
+        target = filesystem.readlink(link)
     except OSError:
         raise errors.CertStorageError(
             "Expected {0} to be a symlink".format(link))
@@ -223,6 +223,7 @@ def get_link_target(link):
         target = os.path.join(os.path.dirname(link), target)
     return os.path.abspath(target)
 
+
 def _write_live_readme_to(readme_path, is_base_dir=False):
     prefix = ""
     if is_base_dir:
@@ -665,7 +666,7 @@ class RenewableCert(interfaces.RenewableCert):
                 current_link = getattr(self, kind)
                 if os.path.lexists(current_link):
                     os.unlink(current_link)
-                os.symlink(os.readlink(previous_link), current_link)
+                os.symlink(filesystem.readlink(previous_link), current_link)
 
         for _, link in previous_symlinks:
             if os.path.exists(link):
@@ -846,7 +847,7 @@ class RenewableCert(interfaces.RenewableCert):
         link = getattr(self, kind)
         filename = "{0}{1}.pem".format(kind, version)
         # Relative rather than absolute target directory
-        target_directory = os.path.dirname(os.readlink(link))
+        target_directory = os.path.dirname(filesystem.readlink(link))
         # TODO: it could be safer to make the link first under a temporary
         #       filename, then unlink the old link, then rename the new link
         #       to the old link; this ensures that this process is able to
@@ -1121,7 +1122,7 @@ class RenewableCert(interfaces.RenewableCert):
             # The behavior below keeps the prior key by creating a new
             # symlink to the old key or the target of the old key symlink.
             if os.path.islink(old_privkey):
-                old_privkey = os.readlink(old_privkey)
+                old_privkey = filesystem.readlink(old_privkey)
             else:
                 old_privkey = "privkey{0}.pem".format(prior_version)
             logger.debug("Writing symlink to old private key, %s.", old_privkey)

certbot/certbot/compat/filesystem.py

@@ -4,6 +4,7 @@ from __future__ import absolute_import
 import errno
 import os  # pylint: disable=os-module-forbidden
 import stat
+import sys
 
 from acme.magic_typing import List
 
@@ -361,7 +362,8 @@ def realpath(file_path):
     """
     original_path = file_path
 
-    if POSIX_MODE:
+    # Since Python 3.8, os.path.realpath also resolves symlinks on Windows.
+    if POSIX_MODE or sys.version_info >= (3, 8):
         path = os.path.realpath(file_path)
         if os.path.islink(path):
             # If path returned by realpath is still a link, it means that it failed to
@@ -383,8 +385,36 @@ def realpath(file_path):
     return os.path.abspath(file_path)
 
 
+def readlink(link_path):
+    # type: (str) -> str
+    """
+    Return a string representing the path to which the symbolic link points.
+
+    :param str link_path: The symlink path to resolve
+    :return: The path the symlink points to
+    :returns: str
+    :raise: ValueError if a long path (260> characters) is encountered on Windows
+    """
+    path = os.readlink(link_path)
+
+    if POSIX_MODE or not path.startswith('\\\\?\\'):
+        return path
+
+    # At this point, we know we are on Windows and that the path returned uses
+    # the extended form which is done for all paths in Python 3.8+
+
+    # Max length of a normal path is 260 characters on Windows, including the non printable
+    # termination character "<NUL>". The termination character is not included in Python
+    # strings, giving a max length of 259 characters, + 4 characters for the extended form
+    # prefix, to an effective max length 263 characters on a string representing a normal path.
+    if len(path) < 264:
+        return path[4:]
+
+    raise ValueError("Long paths are not supported by Certbot on Windows.")
+
+
 # On Windows is_executable run from an unprivileged shell may claim that a path is
-# executable when it is excutable only if run from a privileged shell. This result
+# executable when it is executable only if run from a privileged shell. This result
 # is due to the fact that GetEffectiveRightsFromAcl calculate effective rights
 # without taking into consideration if the target user has currently required the
 # elevated privileges or not. However this is not a problem since certbot always

certbot/certbot/compat/os.py

@@ -152,3 +152,14 @@ def fstat(*unused_args, **unused_kwargs):
     raise RuntimeError('Usage of os.fstat() is forbidden. '
                        'Use certbot.compat.filesystem functions instead '
                        '(eg. has_min_permissions, has_same_ownership).')
+
+
+# Method os.readlink has a significant behavior change with Python 3.8+. Starting
+# with this version, it will return the resolved path in its "extended-style" form
+# unconditionally, which allows to use more than 259 characters, and its string
+# representation is prepended with "\\?\". Problem is that it does it for any path,
+# and will make equality comparison fail with paths that will use the simple form.
+def readlink(*unused_args, **unused_kwargs):
+    """Method os.readlink() is forbidden"""
+    raise RuntimeError('Usage of os.readlink() is forbidden. '
+                       'Use certbot.compat.filesystem.realpath() instead.')

certbot/tests/cert_manager_test.py

@@ -99,7 +99,7 @@ class UpdateLiveSymlinksTest(BaseCertManagerTest):
                 for kind in ALL_FOUR:
                     os.chdir(os.path.dirname(self.config_files[domain][kind]))
                     self.assertEqual(
-                        filesystem.realpath(os.readlink(self.config_files[domain][kind])),
+                        filesystem.realpath(filesystem.readlink(self.config_files[domain][kind])),
                         filesystem.realpath(archive_paths[domain][kind]))
         finally:
             os.chdir(prev_dir)

certbot/tests/compat/filesystem_test.py

@@ -597,6 +597,32 @@ class IsExecutableTest(test_util.TempDirTestCase):
             self.assertFalse(filesystem.is_executable("exe"))
 
 
+class ReadlinkTest(unittest.TestCase):
+    @unittest.skipUnless(POSIX_MODE, reason='Tests specific to Linux')
+    @mock.patch("certbot.compat.filesystem.os.readlink")
+    def test_path_posix(self, mock_readlink):
+        mock_readlink.return_value = "/normal/path"
+        self.assertEqual(filesystem.readlink("dummy"), "/normal/path")
+
+    @unittest.skipIf(POSIX_MODE, reason='Tests specific to Windows')
+    @mock.patch("certbot.compat.filesystem.os.readlink")
+    def test_normal_path_windows(self, mock_readlink):
+        # Python <3.8
+        mock_readlink.return_value = "C:\\short\\path"
+        self.assertEqual(filesystem.readlink("dummy"), "C:\\short\\path")
+
+        # Python >=3.8 (os.readlink always returns the extended form)
+        mock_readlink.return_value = "\\\\?\\C:\\short\\path"
+        self.assertEqual(filesystem.readlink("dummy"), "C:\\short\\path")
+
+    @unittest.skipIf(POSIX_MODE, reason='Tests specific to Windows')
+    @mock.patch("certbot.compat.filesystem.os.readlink")
+    def test_extended_path_windows(self, mock_readlink):
+        # Following path is largely over the 260 characters permitted in the normal form.
+        mock_readlink.return_value = "\\\\?\\C:\\long" + 1000 * "\\path"
+        with self.assertRaises(ValueError):
+            filesystem.readlink("dummy")
+
 @contextlib.contextmanager
 def _fix_windows_runtime():
     if os.name != 'nt':

certbot/tests/storage_test.py

@@ -330,7 +330,7 @@ class RenewableCertTests(BaseRenewableCertTest):
         self.test_rc._update_link_to("chain", 3000)
         # However, current_version doesn't allow querying the resulting
         # version (because it's a broken link).
-        self.assertEqual(os.path.basename(os.readlink(self.test_rc.chain)),
+        self.assertEqual(os.path.basename(filesystem.readlink(self.test_rc.chain)),
                          "chain3000.pem")
 
     def test_version(self):
@@ -514,7 +514,7 @@ class RenewableCertTests(BaseRenewableCertTest):
         # privkey.
         for i in (6, 7, 8):
             self.assertTrue(os.path.islink(self.test_rc.version("privkey", i)))
-            self.assertEqual("privkey3.pem", os.path.basename(os.readlink(
+            self.assertEqual("privkey3.pem", os.path.basename(filesystem.readlink(
                 self.test_rc.version("privkey", i))))
 
         for kind in ALL_FOUR:

windows-installer/construct.py

@@ -9,7 +9,7 @@ import sys
 import tempfile
 import time
 
-PYTHON_VERSION = (3, 7, 4)
+PYTHON_VERSION = (3, 8, 6)
 PYTHON_BITNESS = 32
 PYWIN32_VERSION = 227  # do not forget to edit pywin32 dependency accordingly in setup.py
 NSIS_VERSION = '3.04'