1
0
mirror of https://github.com/certbot/certbot.git synced 2025-08-08 04:02:10 +03:00

[Windows] Security model for files permissions - STEP 3d (#6968)

* Implement security.mkdir and security.makedirs

* Fix lint

* Correct mock

* Rename security into filesystem

* Update apache and nginx plugins requirements

* Update certbot/plugins/webroot.py

Co-Authored-By: ohemorange <ebportnoy@gmail.com>

* Reenable pylint here

* Move code

* Reimplement mkdir

* Control errors on eexist, remove superfluous chmod for makedirs

* Add proper skip for windows only tests

* Fix lint

* Fix mypy

* Clean code

* Adapt coverage threshold on Linux with addition of LOC specific to Windows

* Add forbiden functions to tests

* Update certbot/compat/os.py

Co-Authored-By: ohemorange <ebportnoy@gmail.com>

* Simplify code

* Sync _get_current_user with part3c

* Use the simpliest implementation

* Remove exist_ok, simplify code.

* Simplify inline comment

* Update filesystem_test.py

* Update certbot/compat/os.py

Co-Authored-By: ohemorange <ebportnoy@gmail.com>

* Update certbot/plugins/webroot.py

Co-Authored-By: ohemorange <ebportnoy@gmail.com>

* Update certbot/plugins/webroot.py

Co-Authored-By: ohemorange <ebportnoy@gmail.com>

* Add a test to check we set back os.mkdir correctly after filesystem.makedirs is called.

* Fix lint, adapt coverage
This commit is contained in:
Adrien Ferrand
2019-07-04 01:20:43 +02:00
committed by ohemorange
parent 20b595bc9e
commit 7d61e9ea56
26 changed files with 167 additions and 45 deletions

View File

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

View File

@@ -169,8 +169,7 @@ class ApacheHttp01(common.TLSSNI01):
def _set_up_challenges(self):
if not os.path.isdir(self.challenge_dir):
os.makedirs(self.challenge_dir)
filesystem.chmod(self.challenge_dir, 0o755)
filesystem.makedirs(self.challenge_dir, 0o755)
responses = []
for achall in self.achalls:

View File

@@ -1,3 +1,3 @@
# Remember to update setup.py to match the package versions below.
acme[dev]==0.29.0
certbot[dev]==0.34.0
-e .[dev]

View File

@@ -10,7 +10,7 @@ version = '0.36.0.dev0'
# acme/certbot version.
install_requires = [
'acme>=0.29.0',
'certbot>=0.34.0',
'certbot>=0.35.0.dev0',
'mock',
'PyOpenSSL',
'pyparsing>=1.5.5', # Python3 support; perhaps unnecessary?

View File

@@ -106,6 +106,58 @@ def open(file_path, flags, mode=0o777): # pylint: disable=redefined-builtin
return handle
def makedirs(file_path, mode=0o777):
# type: (str, int) -> None
"""
Rewrite of original os.makedirs function, that will ensure on Windows that given mode
is correctly applied.
:param str file_path: The file path to open
:param int mode: POSIX mode to apply on leaf directory when created, Python defaults
will be applied if ``None``
"""
if POSIX_MODE:
return os.makedirs(file_path, mode)
orig_mkdir_fn = os.mkdir
try:
# As we know that os.mkdir is called internally by os.makedirs, we will swap the function in
# os module for the time of makedirs execution on Windows.
os.mkdir = mkdir # type: ignore
return os.makedirs(file_path, mode)
finally:
os.mkdir = orig_mkdir_fn
def mkdir(file_path, mode=0o777):
# type: (str, int) -> None
"""
Rewrite of original os.mkdir function, that will ensure on Windows that given mode
is correctly applied.
:param str file_path: The file path to open
:param int mode: POSIX mode to apply on directory when created, Python defaults
will be applied if ``None``
"""
if POSIX_MODE:
return os.mkdir(file_path, mode)
attributes = win32security.SECURITY_ATTRIBUTES()
security = attributes.SECURITY_DESCRIPTOR
user = _get_current_user()
dacl = _generate_dacl(user, mode)
security.SetSecurityDescriptorDacl(1, dacl, 0)
try:
win32file.CreateDirectory(file_path, attributes)
except pywintypes.error as err:
# Handle native windows error into python error to be consistent with the API
# of os.mkdir in the situation of a directory already existing.
if err.winerror == winerror.ERROR_ALREADY_EXISTS:
raise OSError(errno.EEXIST, err.strerror, file_path, err.winerror)
raise err
return None
def replace(src, dst):
# type: (str, str) -> None
"""

View File

@@ -62,6 +62,25 @@ def open(*unused_args, **unused_kwargs):
'Use certbot.compat.filesystem.open() instead.')
# Very similarly to os.open, os.mkdir has the same effects on Windows and creates an unsecured
# folder. So a similar mitigation to security.chmod is provided on this platform.
def mkdir(*unused_args, **unused_kwargs):
"""Method os.mkdir() is forbidden"""
raise RuntimeError('Usage of os.mkdir() is forbidden. '
'Use certbot.compat.filesystem.mkdir() instead.')
# As said above, os.makedirs would call the original os.mkdir function recursively on Windows,
# creating the same flaws for every actual folder created. This method is modified to ensure
# that our modified os.mkdir is called on Windows, by monkey patching temporarily the mkdir method
# on the original os module, executing the modified logic to correctly protect newly created
# folders, then restoring original mkdir method in the os module.
def makedirs(*unused_args, **unused_kwargs):
"""Method os.makedirs() is forbidden"""
raise RuntimeError('Usage of os.makedirs() is forbidden. '
'Use certbot.compat.filesystem.makedirs() instead.')
# Because of the blocking strategy on file handlers on Windows, rename does not behave as expected
# with POSIX systems: an exception will be raised if dst already exists.
def rename(*unused_args, **unused_kwargs):

View File

@@ -15,6 +15,7 @@ from certbot import achallenges
from certbot import crypto_util
from certbot import errors
from certbot.compat import os
from certbot.compat import filesystem
from certbot.tests import acme_util
from certbot.tests import util as test_util
@@ -95,7 +96,7 @@ class InstallerTest(test_util.ConfigTestCase):
def setUp(self):
super(InstallerTest, self).setUp()
os.mkdir(self.config.config_dir)
filesystem.mkdir(self.config.config_dir)
from certbot.plugins.common import Installer
self.installer = Installer(config=self.config,

View File

@@ -9,6 +9,7 @@ from acme import challenges
from certbot import errors
from certbot.compat import os
from certbot.compat import filesystem
from certbot.tests import acme_util
from certbot.tests import util as test_util
@@ -23,7 +24,7 @@ class AuthenticatorTest(test_util.TempDirTestCase):
self.dns_achall_2 = acme_util.DNS01_A_2
self.achalls = [self.http_achall, self.dns_achall, self.dns_achall_2]
for d in ["config_dir", "work_dir", "in_progress"]:
os.mkdir(os.path.join(self.tempdir, d))
filesystem.mkdir(os.path.join(self.tempdir, d))
# "backup_dir" and "temp_checkpoint_dir" get created in
# certbot.util.make_or_verify_dir() during the Reverter
# initialization.

View File

@@ -6,6 +6,7 @@ import mock
from certbot import errors
from certbot.compat import os
from certbot.compat import filesystem
from certbot.plugins import common
from certbot.tests import util as test_util
@@ -16,7 +17,7 @@ class PluginStorageTest(test_util.ConfigTestCase):
def setUp(self):
super(PluginStorageTest, self).setUp()
self.plugin_cls = common.Installer
os.mkdir(self.config.config_dir)
filesystem.mkdir(self.config.config_dir)
with mock.patch("certbot.reverter.util"):
self.plugin = self.plugin_cls(config=self.config, name="mockplugin")

View File

@@ -19,6 +19,7 @@ from certbot import cli
from certbot import errors
from certbot import interfaces
from certbot.compat import os
from certbot.compat import filesystem
from certbot.display import ops
from certbot.display import util as display_util
from certbot.plugins import common
@@ -173,10 +174,10 @@ to serve all files under specified web root ({0})."""
# as it does not correspond to a folder path ('/' or 'C:')
for prefix in sorted(util.get_prefixes(self.full_roots[name])[:-1], key=len):
try:
# This is coupled with the "umask" call above because
# os.mkdir's "mode" parameter may not always work:
# This is coupled with the "umask" call above because os.mkdir's
# "mode" parameter may not always work under Linux:
# https://docs.python.org/3/library/os.html#os.mkdir
os.mkdir(prefix, 0o0755)
filesystem.mkdir(prefix, 0o0755)
self._created_dirs.append(prefix)
# Set owner as parent directory if possible
try:

View File

@@ -203,7 +203,7 @@ class AuthenticatorTest(unittest.TestCase):
self.assertFalse(os.path.exists(self.partial_root_challenge_path))
def test_perform_cleanup_existing_dirs(self):
os.mkdir(self.partial_root_challenge_path)
filesystem.mkdir(self.partial_root_challenge_path)
self.auth.prepare()
self.auth.perform([self.achall])
self.auth.cleanup([self.achall])
@@ -219,7 +219,7 @@ class AuthenticatorTest(unittest.TestCase):
domain="thing.com", account_key=KEY)
bingo_validation_path = "YmluZ28"
os.mkdir(self.partial_root_challenge_path)
filesystem.mkdir(self.partial_root_challenge_path)
self.auth.prepare()
self.auth.perform([bingo_achall, self.achall])
@@ -235,7 +235,7 @@ class AuthenticatorTest(unittest.TestCase):
self.auth.perform([self.achall])
leftover_path = os.path.join(self.root_challenge_path, 'leftover')
os.mkdir(leftover_path)
filesystem.mkdir(leftover_path)
self.auth.cleanup([self.achall])
self.assertFalse(os.path.exists(self.validation_path))

View File

@@ -984,7 +984,7 @@ class RenewableCert(object):
for i in (cli_config.renewal_configs_dir, cli_config.default_archive_dir,
cli_config.live_dir):
if not os.path.exists(i):
os.makedirs(i, 0o700)
filesystem.makedirs(i, 0o700)
logger.debug("Creating directory %s.", i)
config_file, config_filename = util.unique_lineage_name(
cli_config.renewal_configs_dir, lineagename)
@@ -1006,8 +1006,8 @@ class RenewableCert(object):
config_file.close()
raise errors.CertStorageError(
"live directory exists for " + lineagename)
os.mkdir(archive)
os.mkdir(live_dir)
filesystem.mkdir(archive)
filesystem.mkdir(live_dir)
logger.debug("Archive directory %s and live "
"directory %s created.", archive, live_dir)

View File

@@ -12,6 +12,7 @@ import mock
from certbot import configuration
from certbot import errors
from certbot.compat import os
from certbot.compat import filesystem
from certbot.display import util as display_util
from certbot.storage import ALL_FOUR
from certbot.tests import storage_test
@@ -25,7 +26,7 @@ class BaseCertManagerTest(test_util.ConfigTestCase):
super(BaseCertManagerTest, self).setUp()
self.config.quiet = False
os.makedirs(self.config.renewal_configs_dir)
filesystem.makedirs(self.config.renewal_configs_dir)
self.domains = {
"example.org": None,
@@ -43,14 +44,14 @@ class BaseCertManagerTest(test_util.ConfigTestCase):
def _set_up_config(self, domain, custom_archive):
# TODO: maybe provide NamespaceConfig.make_dirs?
# TODO: main() should create those dirs, c.f. #902
os.makedirs(os.path.join(self.config.live_dir, domain))
filesystem.makedirs(os.path.join(self.config.live_dir, domain))
config_file = configobj.ConfigObj()
if custom_archive is not None:
os.makedirs(custom_archive)
filesystem.makedirs(custom_archive)
config_file["archive_dir"] = custom_archive
else:
os.makedirs(os.path.join(self.config.default_archive_dir, domain))
filesystem.makedirs(os.path.join(self.config.default_archive_dir, domain))
for kind in ALL_FOUR:
config_file[kind] = os.path.join(self.config.live_dir, domain,
@@ -194,7 +195,7 @@ class CertificatesTest(BaseCertManagerTest):
quiet=False
))
os.makedirs(empty_config.renewal_configs_dir)
filesystem.makedirs(empty_config.renewal_configs_dir)
self._certificates(empty_config)
self.assertFalse(mock_logger.warning.called) #pylint: disable=no-member
self.assertTrue(mock_utility.called)

View File

@@ -16,6 +16,7 @@ from certbot import cli
from certbot import constants
from certbot import errors
from certbot.compat import os
from certbot.compat import filesystem
from certbot.plugins import disco
from certbot.tests.util import TempDirTestCase
@@ -318,8 +319,8 @@ class ParseTest(unittest.TestCase): # pylint: disable=too-many-public-methods
self.parse(short_args + ['renew']), False)
account_dir = os.path.join(config_dir, constants.ACCOUNTS_DIR)
os.mkdir(account_dir)
os.mkdir(os.path.join(account_dir, 'fake_account_dir'))
filesystem.mkdir(account_dir)
filesystem.mkdir(os.path.join(account_dir, 'fake_account_dir'))
self._assert_dry_run_flag_worked(self.parse(short_args + ['auth']), True)
self._assert_dry_run_flag_worked(self.parse(short_args + ['renew']), True)

View File

@@ -229,6 +229,47 @@ class WindowsOpenTest(TempDirTestCase):
raise err
@unittest.skipIf(POSIX_MODE, reason='Test specific to Windows security')
class WindowsMkdirTests(test_util.TempDirTestCase):
"""Unit tests for Windows mkdir + makedirs functions in filesystem module"""
def test_mkdir_correct_permissions(self):
path = os.path.join(self.tempdir, 'dir')
filesystem.mkdir(path, 0o700)
everybody = win32security.ConvertStringSidToSid(EVERYBODY_SID)
dacl = _get_security_dacl(path).GetSecurityDescriptorDacl()
self.assertFalse([dacl.GetAce(index) for index in range(0, dacl.GetAceCount())
if dacl.GetAce(index)[2] == everybody])
def test_makedirs_correct_permissions(self):
path = os.path.join(self.tempdir, 'dir')
subpath = os.path.join(path, 'subpath')
filesystem.makedirs(subpath, 0o700)
everybody = win32security.ConvertStringSidToSid(EVERYBODY_SID)
dacl = _get_security_dacl(subpath).GetSecurityDescriptorDacl()
self.assertFalse([dacl.GetAce(index) for index in range(0, dacl.GetAceCount())
if dacl.GetAce(index)[2] == everybody])
def test_makedirs_switch_os_mkdir(self):
path = os.path.join(self.tempdir, 'dir')
import os as std_os # pylint: disable=os-module-forbidden
original_mkdir = std_os.mkdir
filesystem.makedirs(path)
self.assertEqual(original_mkdir, std_os.mkdir)
try:
filesystem.makedirs(path) # Will fail because path already exists
except OSError:
pass
self.assertEqual(original_mkdir, std_os.mkdir)
class OsReplaceTest(test_util.TempDirTestCase):
"""Test to ensure consistent behavior of rename method"""

View File

@@ -7,7 +7,7 @@ from certbot.compat import os
class OsTest(unittest.TestCase):
"""Unit tests for os module."""
def test_forbidden_methods(self):
for method in ['chmod', 'open', 'rename', 'replace']:
for method in ['chmod', 'open', 'mkdir', 'makedirs', 'rename', 'replace']:
self.assertRaises(RuntimeError, getattr(os, method))

View File

@@ -13,6 +13,7 @@ from six.moves import reload_module # pylint: disable=import-error
from acme.magic_typing import List # pylint: disable=unused-import,no-name-in-module
from certbot.compat import os # pylint: disable=ungrouped-imports
from certbot.compat import filesystem # pylint: disable=ungrouped-imports
import certbot.tests.util as test_util # pylint: disable=ungrouped-imports
@@ -33,7 +34,7 @@ class CompleterTest(test_util.TempDirTestCase):
path = os.path.join(self.tempdir, c)
self.paths.append(path)
if ord(c) % 2:
os.mkdir(path)
filesystem.mkdir(path)
else:
with open(path, 'w'):
pass

View File

@@ -13,6 +13,7 @@ import certbot.tests.util as test_util
from certbot import account
from certbot import errors
from certbot.compat import os
from certbot.compat import filesystem
from certbot.display import ops
from certbot.display import util as display_util
@@ -93,7 +94,7 @@ class ChooseAccountTest(test_util.TempDirTestCase):
False))
self.account_keys_dir = os.path.join(self.tempdir, "keys")
os.makedirs(self.account_keys_dir, 0o700)
filesystem.makedirs(self.account_keys_dir, 0o700)
self.config = mock.MagicMock(
accounts_dir=self.tempdir,

View File

@@ -96,7 +96,7 @@ class PreHookTest(HookTest):
super(PreHookTest, self).setUp()
self.config.pre_hook = "foo"
os.makedirs(self.config.renewal_pre_hooks_dir)
filesystem.makedirs(self.config.renewal_pre_hooks_dir)
self.dir_hook = os.path.join(self.config.renewal_pre_hooks_dir, "bar")
create_hook(self.dir_hook)
@@ -174,7 +174,7 @@ class PostHookTest(HookTest):
super(PostHookTest, self).setUp()
self.config.post_hook = "bar"
os.makedirs(self.config.renewal_post_hooks_dir)
filesystem.makedirs(self.config.renewal_post_hooks_dir)
self.dir_hook = os.path.join(self.config.renewal_post_hooks_dir, "foo")
create_hook(self.dir_hook)
@@ -376,7 +376,7 @@ class RenewHookTest(RenewalHookTest):
super(RenewHookTest, self).setUp()
self.config.renew_hook = "foo"
os.makedirs(self.config.renewal_deploy_hooks_dir)
filesystem.makedirs(self.config.renewal_deploy_hooks_dir)
self.dir_hook = os.path.join(self.config.renewal_deploy_hooks_dir,
"bar")
create_hook(self.dir_hook)

View File

@@ -33,6 +33,7 @@ from certbot import updater
from certbot import util
from certbot.compat import misc
from certbot.compat import os
from certbot.compat import filesystem
from certbot.plugins import disco
from certbot.plugins import enhancements
from certbot.plugins import manual
@@ -513,7 +514,7 @@ class MainTest(test_util.ConfigTestCase): # pylint: disable=too-many-public-met
def setUp(self):
super(MainTest, self).setUp()
os.mkdir(self.config.logs_dir)
filesystem.mkdir(self.config.logs_dir)
self.standard_args = ['--config-dir', self.config.config_dir,
'--work-dir', self.config.work_dir,
'--logs-dir', self.config.logs_dir, '--text']
@@ -1161,7 +1162,7 @@ class MainTest(test_util.ConfigTestCase): # pylint: disable=too-many-public-met
def test_renew_verb_empty_config(self):
rd = os.path.join(self.config.config_dir, 'renewal')
if not os.path.exists(rd):
os.makedirs(rd)
filesystem.makedirs(rd)
with open(os.path.join(rd, 'empty.conf'), 'w'):
pass # leave the file empty
args = ["renew", "--dry-run", "-tvv"]
@@ -1179,7 +1180,7 @@ class MainTest(test_util.ConfigTestCase): # pylint: disable=too-many-public-met
def _make_dummy_renewal_config(self):
renewer_configs_dir = os.path.join(self.config.config_dir, 'renewal')
os.makedirs(renewer_configs_dir)
filesystem.makedirs(renewer_configs_dir)
with open(os.path.join(renewer_configs_dir, 'test.conf'), 'w') as f:
f.write("My contents don't matter")

View File

@@ -10,6 +10,7 @@ import six
from certbot import errors
from certbot.compat import os
from certbot.compat import filesystem
from certbot.tests import util as test_util
@@ -395,7 +396,7 @@ class TestFullCheckpointsReverter(test_util.ConfigTestCase):
def test_view_config_changes_bad_backups_dir(self):
# There shouldn't be any "in progress directories when this is called
# It must just be clean checkpoints
os.makedirs(os.path.join(self.config.backup_dir, "in_progress"))
filesystem.makedirs(os.path.join(self.config.backup_dir, "in_progress"))
self.assertRaises(
errors.ReverterError, self.reverter.view_config_changes)

View File

@@ -92,10 +92,10 @@ class BaseRenewableCertTest(test_util.ConfigTestCase):
# TODO: maybe provide NamespaceConfig.make_dirs?
# TODO: main() should create those dirs, c.f. #902
os.makedirs(os.path.join(self.config.config_dir, "live", "example.org"))
filesystem.makedirs(os.path.join(self.config.config_dir, "live", "example.org"))
archive_path = os.path.join(self.config.config_dir, "archive", "example.org")
os.makedirs(archive_path)
os.makedirs(os.path.join(self.config.config_dir, "renewal"))
filesystem.makedirs(archive_path)
filesystem.makedirs(os.path.join(self.config.config_dir, "renewal"))
config_file = configobj.ConfigObj()
for kind in ALL_FOUR:
@@ -634,12 +634,12 @@ class RenewableCertTests(BaseRenewableCertTest):
self.assertTrue(os.path.exists(os.path.join(
self.config.live_dir, "the-lineage.com-0001", "README")))
# Now trigger the detection of already existing files
os.mkdir(os.path.join(
filesystem.mkdir(os.path.join(
self.config.live_dir, "the-lineage.com-0002"))
self.assertRaises(errors.CertStorageError,
storage.RenewableCert.new_lineage, "the-lineage.com",
b"cert3", b"privkey3", b"chain3", self.config)
os.mkdir(os.path.join(self.config.default_archive_dir, "other-example.com"))
filesystem.mkdir(os.path.join(self.config.default_archive_dir, "other-example.com"))
self.assertRaises(errors.CertStorageError,
storage.RenewableCert.new_lineage,
"other-example.com", b"cert4",

View File

@@ -139,7 +139,7 @@ def make_lineage(config_dir, testfile):
for directory in (archive_dir, conf_dir, live_dir,):
if not os.path.exists(directory):
os.makedirs(directory)
filesystem.makedirs(directory)
sample_archive = vector_path('sample-archive')
for kind in os.listdir(sample_archive):

View File

@@ -92,7 +92,7 @@ class LockDirUntilExit(test_util.TempDirTestCase):
@mock.patch('certbot.util.atexit_register')
def test_it(self, mock_register, mock_logger):
subdir = os.path.join(self.tempdir, 'subdir')
os.mkdir(subdir)
filesystem.mkdir(subdir)
self._call(self.tempdir)
self._call(subdir)
self._call(subdir)
@@ -143,7 +143,7 @@ class MakeOrVerifyDirTest(test_util.TempDirTestCase):
super(MakeOrVerifyDirTest, self).setUp()
self.path = os.path.join(self.tempdir, "foo")
os.mkdir(self.path, 0o600)
filesystem.mkdir(self.path, 0o600)
self.uid = misc.os_geteuid()
@@ -166,7 +166,7 @@ class MakeOrVerifyDirTest(test_util.TempDirTestCase):
self.assertRaises(errors.Error, self._call, self.path, 0o400)
def test_reraises_os_error(self):
with mock.patch.object(os, "makedirs") as makedirs:
with mock.patch.object(filesystem, "makedirs") as makedirs:
makedirs.side_effect = OSError()
self.assertRaises(OSError, self._call, "bar", 12312312)

View File

@@ -181,7 +181,7 @@ def make_or_verify_dir(directory, mode=0o755, uid=0, strict=False):
"""
try:
os.makedirs(directory, mode)
filesystem.makedirs(directory, mode)
except OSError as exception:
if exception.errno == errno.EEXIST:
if strict and not check_permissions(directory, mode, uid):

View File

@@ -21,6 +21,7 @@ from cryptography.hazmat.primitives.asymmetric import rsa
from certbot import lock
from certbot import util
from certbot.compat import filesystem
from certbot.tests import util as test_util
@@ -92,7 +93,7 @@ def set_up_dirs():
nginx_dir = os.path.join(temp_dir, 'nginx')
for directory in (config_dir, logs_dir, work_dir, nginx_dir,):
os.mkdir(directory)
filesystem.mkdir(directory)
test_util.make_lineage(config_dir, 'sample-renewal.conf')
set_up_nginx_dir(nginx_dir)