diff --git a/.travis.yml b/.travis.yml index e1859cc0e..b23164a19 100644 --- a/.travis.yml +++ b/.travis.yml @@ -60,12 +60,7 @@ matrix: env: TOXENV=mypy <<: *not-on-master - python: "2.7" - env: TOXENV='py27-{acme,apache,certbot,nginx,postfix}-oldest' - sudo: required - services: docker - <<: *not-on-master - - python: "2.7" - env: TOXENV='py27-dns-oldest' + env: TOXENV='py27-{acme,apache,certbot,dns,nginx,postfix}-oldest' sudo: required services: docker <<: *not-on-master diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b695786f..cb705e03e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). ### Fixed -* +* certbot-auto no longer writes a check_permissions.py script at the root + of the filesystem. Despite us having broken lockstep, we are continuing to release new versions of all Certbot components during releases for the time being, however, the only diff --git a/certbot/compat/misc.py b/certbot/compat/misc.py index 2f4ba0c6b..4f0e22078 100644 --- a/certbot/compat/misc.py +++ b/certbot/compat/misc.py @@ -31,7 +31,7 @@ def raise_for_non_administrative_windows_rights(subcommand): # Because windll exists only on a Windows runtime, and static code analysis engines # do not like at all non existent objects when run from Linux (even if we handle properly # all the cases in the code). - # So we access windll only by reflection to trick theses engines. + # So we access windll only by reflection to trick these engines. if hasattr(ctypes, 'windll') and subcommand not in UNPRIVILEGED_SUBCOMMANDS_ALLOWED: windll = getattr(ctypes, 'windll') if windll.shell32.IsUserAnAdmin() == 0: @@ -73,7 +73,7 @@ def os_rename(src, dst): raise if not hasattr(os, 'replace'): # pragma: no cover # We should never go on this line. Either we are on Linux and os.rename has succeeded, - # either we are on Windows, and only Python >= 3.4 is supported where os.replace is + # or we are on Windows, and only Python >= 3.4 is supported where os.replace is # available. raise RuntimeError('Error: tried to run os_rename on Python < 3.3. ' 'Certbot supports only Python 3.4 >= on Windows.') diff --git a/certbot/compat/os.py b/certbot/compat/os.py index 0112fbc73..8a139a141 100644 --- a/certbot/compat/os.py +++ b/certbot/compat/os.py @@ -1,6 +1,6 @@ """ This compat modules is a wrapper of the core os module that forbids usage of specific operations -(eg. chown, chmod, getuid) that would be harmful to the Windows file security model of Certbot. +(e.g. chown, chmod, getuid) that would be harmful to the Windows file security model of Certbot. This module is intended to replace standard os module throughout certbot projects (except acme). """ from __future__ import absolute_import diff --git a/letsencrypt-auto-source/letsencrypt-auto b/letsencrypt-auto-source/letsencrypt-auto index 2d3da3962..21a76aaff 100755 --- a/letsencrypt-auto-source/letsencrypt-auto +++ b/letsencrypt-auto-source/letsencrypt-auto @@ -953,6 +953,95 @@ if __name__ == '__main__': UNLIKELY_EOF } +# Check that the given PATH_TO_CHECK has secured permissions. +# Parameters: LE_PYTHON, PATH_TO_CHECK +CheckPathPermissions() { + "$1" - "$2" << "UNLIKELY_EOF" +"""Verifies certbot-auto cannot be modified by unprivileged users. + +This script takes the path to certbot-auto as its only command line +argument. It then checks that the file can only be modified by uid/gid +< 1000 and if other users can modify the file, it prints a warning with +a suggestion on how to solve the problem. + +Permissions on symlinks in the absolute path of certbot-auto are ignored +and only the canonical path to certbot-auto is checked. There could be +permissions problems due to the symlinks that are unreported by this +script, however, issues like this were not caused by our documentation +and are ignored for the sake of simplicity. + +All warnings are printed to stdout rather than stderr so all stderr +output from this script can be suppressed to avoid printing messages if +this script fails for some reason. + +""" +from __future__ import print_function + +import os +import stat +import sys + + +FORUM_POST_URL = 'https://community.letsencrypt.org/t/certbot-auto-deployment-best-practices/91979/' + + +def has_safe_permissions(path): + """Returns True if the given path has secure permissions. + + The permissions are considered safe if the file is only writable by + uid/gid < 1000. + + The reason we allow more IDs than 0 is because on some systems such + as Debian, system users/groups other than uid/gid 0 are used for the + path we recommend in our instructions which is /usr/local/bin. 1000 + was chosen because on Debian 0-999 is reserved for system IDs[1] and + on RHEL either 0-499 or 0-999 is reserved depending on the + version[2][3]. Due to these differences across different OSes, this + detection isn't perfect so we only determine permissions are + insecure when we can be reasonably confident there is a problem + regardless of the underlying OS. + + [1] https://www.debian.org/doc/debian-policy/ch-opersys.html#uid-and-gid-classes + [2] https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/6/html/deployment_guide/ch-managing_users_and_groups + [3] https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/system_administrators_guide/ch-managing_users_and_groups + + :param str path: filesystem path to check + :returns: True if the path has secure permissions, otherwise, False + :rtype: bool + + """ + # os.stat follows symlinks before obtaining information about a file. + stat_result = os.stat(path) + if stat_result.st_mode & stat.S_IWOTH: + return False + if stat_result.st_mode & stat.S_IWGRP and stat_result.st_gid >= 1000: + return False + if stat_result.st_mode & stat.S_IWUSR and stat_result.st_uid >= 1000: + return False + return True + + +def main(certbot_auto_path): + current_path = os.path.realpath(certbot_auto_path) + last_path = None + permissions_ok = True + # This loop makes use of the fact that os.path.dirname('/') == '/'. + while current_path != last_path and permissions_ok: + permissions_ok = has_safe_permissions(current_path) + last_path = current_path + current_path = os.path.dirname(current_path) + + if not permissions_ok: + print('{0} has insecure permissions!'.format(certbot_auto_path)) + print('To learn how to fix them, visit {0}'.format(FORUM_POST_URL)) + + +if __name__ == '__main__': + main(sys.argv[1]) + +UNLIKELY_EOF +} + if [ "$1" = "--le-auto-phase2" ]; then # Phase 2: Create venv, install LE, and run. @@ -1505,96 +1594,10 @@ else # Don't warn about file permissions if the user disabled the check or we # can't find an up-to-date Python. if [ "$PYVER" -ge "$MIN_PYVER" -a "$NO_PERMISSIONS_CHECK" != 1 ]; then - # --------------------------------------------------------------------------- - cat << "UNLIKELY_EOF" > "$TEMP_DIR/check_permissions.py" -"""Verifies certbot-auto cannot be modified by unprivileged users. - -This script takes the path to certbot-auto as its only command line -argument. It then checks that the file can only be modified by uid/gid -< 1000 and if other users can modify the file, it prints a warning with -a suggestion on how to solve the problem. - -Permissions on symlinks in the absolute path of certbot-auto are ignored -and only the canonical path to certbot-auto is checked. There could be -permissions problems due to the symlinks that are unreported by this -script, however, issues like this were not caused by our documentation -and are ignored for the sake of simplicity. - -All warnings are printed to stdout rather than stderr so all stderr -output from this script can be suppressed to avoid printing messages if -this script fails for some reason. - -""" -from __future__ import print_function - -import os -import stat -import sys - - -FORUM_POST_URL = 'https://community.letsencrypt.org/t/certbot-auto-deployment-best-practices/91979/' - - -def has_safe_permissions(path): - """Returns True if the given path has secure permissions. - - The permissions are considered safe if the file is only writable by - uid/gid < 1000. - - The reason we allow more IDs than 0 is because on some systems such - as Debian, system users/groups other than uid/gid 0 are used for the - path we recommend in our instructions which is /usr/local/bin. 1000 - was chosen because on Debian 0-999 is reserved for system IDs[1] and - on RHEL either 0-499 or 0-999 is reserved depending on the - version[2][3]. Due to these differences across different OSes, this - detection isn't perfect so we only determine permissions are - insecure when we can be reasonably confident there is a problem - regardless of the underlying OS. - - [1] https://www.debian.org/doc/debian-policy/ch-opersys.html#uid-and-gid-classes - [2] https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/6/html/deployment_guide/ch-managing_users_and_groups - [3] https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/system_administrators_guide/ch-managing_users_and_groups - - :param str path: filesystem path to check - :returns: True if the path has secure permissions, otherwise, False - :rtype: bool - - """ - # os.stat follows symlinks before obtaining information about a file. - stat_result = os.stat(path) - if stat_result.st_mode & stat.S_IWOTH: - return False - if stat_result.st_mode & stat.S_IWGRP and stat_result.st_gid >= 1000: - return False - if stat_result.st_mode & stat.S_IWUSR and stat_result.st_uid >= 1000: - return False - return True - - -def main(certbot_auto_path): - current_path = os.path.realpath(certbot_auto_path) - last_path = None - permissions_ok = True - # This loop makes use of the fact that os.path.dirname('/') == '/'. - while current_path != last_path and permissions_ok: - permissions_ok = has_safe_permissions(current_path) - last_path = current_path - current_path = os.path.dirname(current_path) - - if not permissions_ok: - print('{0} has insecure permissions!'.format(certbot_auto_path)) - print('To learn how to fix them, visit {0}'.format(FORUM_POST_URL)) - - -if __name__ == '__main__': - main(sys.argv[1]) - -UNLIKELY_EOF - # --------------------------------------------------------------------------- # If the script fails for some reason, don't break certbot-auto. set +e # Suppress unexpected error output. - CHECK_PERM_OUT=$("$LE_PYTHON" "$TEMP_DIR/check_permissions.py" "$0" 2>/dev/null) + CHECK_PERM_OUT=$(CheckPathPermissions "$LE_PYTHON" "$0" 2>/dev/null) CHECK_PERM_STATUS="$?" set -e # Only print output if the script ran successfully and it actually produced diff --git a/letsencrypt-auto-source/letsencrypt-auto.template b/letsencrypt-auto-source/letsencrypt-auto.template index bff4173d4..c064580bd 100755 --- a/letsencrypt-auto-source/letsencrypt-auto.template +++ b/letsencrypt-auto-source/letsencrypt-auto.template @@ -501,6 +501,14 @@ CreateVenv() { UNLIKELY_EOF } +# Check that the given PATH_TO_CHECK has secured permissions. +# Parameters: LE_PYTHON, PATH_TO_CHECK +CheckPathPermissions() { + "$1" - "$2" << "UNLIKELY_EOF" +{{ check_permissions.py }} +UNLIKELY_EOF +} + if [ "$1" = "--le-auto-phase2" ]; then # Phase 2: Create venv, install LE, and run. @@ -663,15 +671,10 @@ else # Don't warn about file permissions if the user disabled the check or we # can't find an up-to-date Python. if [ "$PYVER" -ge "$MIN_PYVER" -a "$NO_PERMISSIONS_CHECK" != 1 ]; then - # --------------------------------------------------------------------------- - cat << "UNLIKELY_EOF" > "$TEMP_DIR/check_permissions.py" -{{ check_permissions.py }} -UNLIKELY_EOF - # --------------------------------------------------------------------------- # If the script fails for some reason, don't break certbot-auto. set +e # Suppress unexpected error output. - CHECK_PERM_OUT=$("$LE_PYTHON" "$TEMP_DIR/check_permissions.py" "$0" 2>/dev/null) + CHECK_PERM_OUT=$(CheckPathPermissions "$LE_PYTHON" "$0" 2>/dev/null) CHECK_PERM_STATUS="$?" set -e # Only print output if the script ran successfully and it actually produced diff --git a/tools/pip_install.py b/tools/pip_install.py index abc7baa91..cf0a7aee5 100755 --- a/tools/pip_install.py +++ b/tools/pip_install.py @@ -99,25 +99,25 @@ def main(args): else: # Otherwise, we merge requirements to build the constraints and pin dependencies requirements = None - reinstall = False if os.environ.get('CERTBOT_OLDEST') == '1': requirements = certbot_oldest_processing(tools_path, args, test_constraints) - # We need to --force-reinstall the tested distribution when using oldest - # requirements because of an error in these tests in particular situations - # described in https://github.com/certbot/certbot/issues/7014. - # However this slows down considerably the oldest tests (5 min -> 10 min), - # so we need to find a better mitigation in the future. - reinstall = True else: certbot_normal_processing(tools_path, test_constraints) merge_requirements(tools_path, requirements, test_constraints, all_constraints) - if requirements: + if requirements: # This branch is executed during the oldest tests + # First step, install the transitive dependencies of oldest requirements + # in respect with oldest constraints. pip_install_with_print('--constraint "{0}" --requirement "{1}"' .format(all_constraints, requirements)) + # Second step, ensure that oldest requirements themselves are effectively + # installed using --force-reinstall, and avoid corner cases like the one described + # in https://github.com/certbot/certbot/issues/7014. + pip_install_with_print('--force-reinstall --no-deps --requirement "{0}"' + .format(requirements)) - pip_install_with_print('--constraint "{0}" {1} {2}'.format( - all_constraints, '--force-reinstall' if reinstall else '', ' '.join(args))) + pip_install_with_print('--constraint "{0}" {1}'.format( + all_constraints, ' '.join(args))) finally: if os.environ.get('TRAVIS'): print('travis_fold:end:install_certbot_deps')