From 8f7b2801061bb36a9b16b4a82193af700574182d Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Thu, 31 Jan 2019 23:53:32 +0100 Subject: [PATCH] [Windows] Fix account paths on Windows when colons are involved (#6711) The account path used to store user credentials is calculated from the domain used to contact the relevant ACME CA server. For instance, if the directory URL is https://my.domain.net/directory, then the account path will be $CONFIG_DIR/accounts/my.domain.net. However, if non standard HTTP/HTTPS port need to be used, colons will be involved. For instance, https://my.domain.net:14000/directory will give $CONFIG_DIR/accounts/my.domain.net:14000. Colons in paths are supported on POSIX systems, but not on Windows (it is reserved for the root drive letter). This PR replaces colons by underscores for account paths on Windows, and leaves them untouched on Linux. * Fix account path on Windows when colons are involved * Protect colon in drive letter * Refactor compat platform specific logic --- certbot/account.py | 2 +- certbot/compat.py | 38 +++++++++++++++++++++++------ certbot/configuration.py | 2 ++ certbot/tests/account_test.py | 4 ++- certbot/tests/configuration_test.py | 5 +++- 5 files changed, 40 insertions(+), 11 deletions(-) diff --git a/certbot/account.py b/certbot/account.py index 76135c3aa..0c653f6dd 100644 --- a/certbot/account.py +++ b/certbot/account.py @@ -142,7 +142,7 @@ class AccountFileStorage(interfaces.AccountStorage): def __init__(self, config): self.config = config util.make_or_verify_dir(config.accounts_dir, 0o700, compat.os_geteuid(), - self.config.strict_permissions) + self.config.strict_permissions) def _account_dir_path(self, account_id): return self._account_dir_path_for_server_path(account_id, self.config.server_path) diff --git a/certbot/compat.py b/certbot/compat.py index 7d936aa9d..3b5d068a6 100644 --- a/certbot/compat.py +++ b/certbot/compat.py @@ -1,12 +1,8 @@ """ Compatibility layer to run certbot both on Linux and Windows. -The approach used here is similar to Modernizr for Web browsers. -We do not check the platform type to determine if a particular logic is supported. -Instead, we apply a logic, and then fallback to another logic if first logic -is not supported at runtime. - -Then logic chains are abstracted into single functions to be exposed to certbot. +This module contains all required platform specific code, +allowing the rest of Certbot codebase to be platform agnostic. """ import os import select @@ -27,6 +23,8 @@ except ImportError: UNPRIVILEGED_SUBCOMMANDS_ALLOWED = [ 'certificates', 'enhance', 'revoke', 'delete', 'register', 'unregister', 'config_changes', 'plugins'] + + def raise_for_non_administrative_windows_rights(subcommand): """ On Windows, raise if current shell does not have the administrative rights. @@ -50,6 +48,7 @@ def raise_for_non_administrative_windows_rights(subcommand): 'Error, "{0}" subcommand must be run on a shell with administrative rights.' .format(subcommand)) + def os_geteuid(): """ Get current user uid @@ -65,6 +64,7 @@ def os_geteuid(): # Windows specific return 0 + def os_rename(src, dst): """ Rename a file to a destination path and handles situations where the destination exists. @@ -117,6 +117,7 @@ def readline_with_timeout(timeout, prompt): # So no timeout on Windows for now. return sys.stdin.readline() + def lock_file(fd): """ Lock the file linked to the specified file descriptor. @@ -131,6 +132,7 @@ def lock_file(fd): # Windows specific msvcrt.locking(fd, msvcrt.LK_NBLCK, 1) + def release_locked_file(fd, path): """ Remove, close, and release a lock file specified by its file descriptor and its path. @@ -164,15 +166,17 @@ def release_locked_file(fd, path): finally: os.close(fd) + def compare_file_modes(mode1, mode2): """Return true if the two modes can be considered as equals for this platform""" - if 'fcntl' in sys.modules: + if os.name != 'nt': # Linux specific: standard compare return oct(stat.S_IMODE(mode1)) == oct(stat.S_IMODE(mode2)) # Windows specific: most of mode bits are ignored on Windows. Only check user R/W rights. return (stat.S_IMODE(mode1) & stat.S_IREAD == stat.S_IMODE(mode2) & stat.S_IREAD and stat.S_IMODE(mode1) & stat.S_IWRITE == stat.S_IMODE(mode2) & stat.S_IWRITE) + WINDOWS_DEFAULT_FOLDERS = { 'config': 'C:\\Certbot', 'work': 'C:\\Certbot\\lib', @@ -184,6 +188,7 @@ LINUX_DEFAULT_FOLDERS = { 'logs': '/var/log/letsencrypt', } + def get_default_folder(folder_type): """ Return the relevant default folder for the current OS @@ -194,8 +199,25 @@ def get_default_folder(folder_type): :rtype: str """ - if 'fcntl' in sys.modules: + if os.name != 'nt': # Linux specific return LINUX_DEFAULT_FOLDERS[folder_type] # Windows specific return WINDOWS_DEFAULT_FOLDERS[folder_type] + + +def underscores_for_unsupported_characters_in_path(path): + # type: (str) -> str + """ + Replace unsupported characters in path for current OS by underscores. + :param str path: the path to normalize + :return: the normalized path + :rtype: str + """ + if os.name != 'nt': + # Linux specific + return path + + # Windows specific + drive, tail = os.path.splitdrive(path) + return drive + tail.replace(':', '_') diff --git a/certbot/configuration.py b/certbot/configuration.py index daf514be8..15f9fa3e0 100644 --- a/certbot/configuration.py +++ b/certbot/configuration.py @@ -5,6 +5,7 @@ import os from six.moves.urllib import parse # pylint: disable=import-error import zope.interface +from certbot import compat from certbot import constants from certbot import errors from certbot import interfaces @@ -69,6 +70,7 @@ class NamespaceConfig(object): def accounts_dir_for_server_path(self, server_path): """Path to accounts directory based on server_path""" + server_path = compat.underscores_for_unsupported_characters_in_path(server_path) return os.path.join( self.namespace.config_dir, constants.ACCOUNTS_DIR, server_path) diff --git a/certbot/tests/account_test.py b/certbot/tests/account_test.py index 278b0c545..b062f437b 100644 --- a/certbot/tests/account_test.py +++ b/certbot/tests/account_test.py @@ -12,6 +12,7 @@ import pytz from acme import messages +from certbot import compat from certbot import errors import certbot.tests.util as test_util @@ -114,7 +115,8 @@ class AccountFileStorageTest(test_util.ConfigTestCase): self.mock_client.directory.new_authz = new_authzr_uri def test_init_creates_dir(self): - self.assertTrue(os.path.isdir(self.config.accounts_dir)) + self.assertTrue(os.path.isdir( + compat.underscores_for_unsupported_characters_in_path(self.config.accounts_dir))) @test_util.broken_on_windows def test_save_and_restore(self): diff --git a/certbot/tests/configuration_test.py b/certbot/tests/configuration_test.py index eb8bcff89..10d9059b3 100644 --- a/certbot/tests/configuration_test.py +++ b/certbot/tests/configuration_test.py @@ -4,6 +4,7 @@ import unittest import mock +from certbot import compat from certbot import constants from certbot import errors @@ -47,9 +48,11 @@ class NamespaceConfigTest(test_util.ConfigTestCase): mock_constants.KEY_DIR = 'keys' mock_constants.TEMP_CHECKPOINT_DIR = 't' + ref_path = compat.underscores_for_unsupported_characters_in_path( + 'acc/acme-server.org:443/new') self.assertEqual( os.path.normpath(self.config.accounts_dir), - os.path.normpath(os.path.join(self.config.config_dir, 'acc/acme-server.org:443/new'))) + os.path.normpath(os.path.join(self.config.config_dir, ref_path))) self.assertEqual( os.path.normpath(self.config.backup_dir), os.path.normpath(os.path.join(self.config.work_dir, 'backups')))