quiet and fast

Fix AppVeyor on the apache-parser-v2 branch

.codecov.yml

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

.travis.yml

@@ -40,220 +40,6 @@ matrix:
       sudo: required
       services: docker
       <<: *not-on-master
-
-    # This job is always executed, including on master
-    - python: "2.7"
-      env: TOXENV=py27-cover FYI="py27 tests + code coverage"
-
-    - python: "2.7"
-      env: TOXENV=lint
-      <<: *not-on-master
-    - python: "3.4"
-      env: TOXENV=mypy
-      <<: *not-on-master
-    - python: "3.5"
-      env: TOXENV=mypy
-      <<: *not-on-master
-    - python: "2.7"
-      env: TOXENV='py27-{acme,apache,certbot,dns,nginx}-oldest'
-      sudo: required
-      services: docker
-      <<: *not-on-master
-    - python: "3.4"
-      env: TOXENV=py34
-      sudo: required
-      services: docker
-      <<: *not-on-master
-    - python: "3.7"
-      dist: xenial
-      env: TOXENV=py37
-      sudo: required
-      services: docker
-      <<: *not-on-master
-    - sudo: required
-      env: TOXENV=apache_compat
-      services: docker
-      before_install:
-      addons:
-      <<: *not-on-master
-    - sudo: required
-      env: TOXENV=le_auto_xenial
-      services: docker
-      <<: *not-on-master
-    - python: "2.7"
-      env: TOXENV=apacheconftest-with-pebble
-      sudo: required
-      services: docker
-      <<: *not-on-master
-    - python: "2.7"
-      env: TOXENV=nginxroundtrip
-      <<: *not-on-master
-
-    # Extended test suite on cron jobs and pushes to tested branches other than master
-    - sudo: required
-      env: TOXENV=nginx_compat
-      services: docker
-      before_install:
-      addons:
-      <<: *extended-test-suite
-    - python: "2.7"
-      env:
-        - TOXENV=travis-test-farm-apache2
-        - secure: "f+j/Lj9s1lcuKo5sEFrlRd1kIAMnIJI4z0MTI7QF8jl9Fkmbx7KECGzw31TNgzrOSzxSapHbcueFYvNCLKST+kE/8ogMZBbwqXfEDuKpyF6BY3uYoJn+wPVE5pIb8Hhe08xPte8TTDSMIyHI3EyTfcAKrIreauoArePvh/cRvSw="
-      <<: *extended-test-suite
-    - python: "2.7"
-      env:
-        - TOXENV=travis-test-farm-leauto-upgrades
-        - secure: "f+j/Lj9s1lcuKo5sEFrlRd1kIAMnIJI4z0MTI7QF8jl9Fkmbx7KECGzw31TNgzrOSzxSapHbcueFYvNCLKST+kE/8ogMZBbwqXfEDuKpyF6BY3uYoJn+wPVE5pIb8Hhe08xPte8TTDSMIyHI3EyTfcAKrIreauoArePvh/cRvSw="
-      git:
-        depth: false  # This is needed to have the history to checkout old versions of certbot-auto.
-      <<: *extended-test-suite
-    - python: "2.7"
-      env:
-        - TOXENV=travis-test-farm-certonly-standalone
-        - secure: "f+j/Lj9s1lcuKo5sEFrlRd1kIAMnIJI4z0MTI7QF8jl9Fkmbx7KECGzw31TNgzrOSzxSapHbcueFYvNCLKST+kE/8ogMZBbwqXfEDuKpyF6BY3uYoJn+wPVE5pIb8Hhe08xPte8TTDSMIyHI3EyTfcAKrIreauoArePvh/cRvSw="
-      <<: *extended-test-suite
-    - python: "2.7"
-      env:
-        - TOXENV=travis-test-farm-sdists
-        - secure: "f+j/Lj9s1lcuKo5sEFrlRd1kIAMnIJI4z0MTI7QF8jl9Fkmbx7KECGzw31TNgzrOSzxSapHbcueFYvNCLKST+kE/8ogMZBbwqXfEDuKpyF6BY3uYoJn+wPVE5pIb8Hhe08xPte8TTDSMIyHI3EyTfcAKrIreauoArePvh/cRvSw="
-      <<: *extended-test-suite
-    - python: "3.7"
-      dist: xenial
-      env: TOXENV=py37 CERTBOT_NO_PIN=1
-      <<: *extended-test-suite
-    - python: "2.7"
-      env: ACME_SERVER=boulder-v1 TOXENV=integration
-      sudo: required
-      services: docker
-      <<: *extended-test-suite
-    - python: "2.7"
-      env: ACME_SERVER=boulder-v2 TOXENV=integration
-      sudo: required
-      services: docker
-      <<: *extended-test-suite
-    - python: "2.7"
-      env: TOXENV=py27-certbot-oldest
-      <<: *extended-test-suite
-    - python: "2.7"
-      env: TOXENV=py27-nginx-oldest
-      <<: *extended-test-suite
-    - python: "2.7"
-      env: ACME_SERVER=boulder-v1 TOXENV=integration-certbot-oldest
-      sudo: required
-      services: docker
-      <<: *extended-test-suite
-    - python: "2.7"
-      env: ACME_SERVER=boulder-v2 TOXENV=integration-certbot-oldest
-      sudo: required
-      services: docker
-      <<: *extended-test-suite
-    - python: "2.7"
-      env: ACME_SERVER=boulder-v1 TOXENV=integration-nginx-oldest
-      sudo: required
-      services: docker
-      <<: *extended-test-suite
-    - python: "2.7"
-      env: ACME_SERVER=boulder-v2 TOXENV=integration-nginx-oldest
-      sudo: required
-      services: docker
-      <<: *extended-test-suite
-    - python: "3.4"
-      env: TOXENV=py34
-      <<: *extended-test-suite
-    - python: "3.5"
-      env: TOXENV=py35
-      <<: *extended-test-suite
-    - python: "3.6"
-      env: TOXENV=py36
-      <<: *extended-test-suite
-    - python: "3.7"
-      dist: xenial
-      env: TOXENV=py37
-      <<: *extended-test-suite
-    - python: "3.4"
-      env: ACME_SERVER=boulder-v1 TOXENV=integration
-      sudo: required
-      services: docker
-      <<: *extended-test-suite
-    - python: "3.4"
-      env: ACME_SERVER=boulder-v2 TOXENV=integration
-      sudo: required
-      services: docker
-      <<: *extended-test-suite
-    - python: "3.5"
-      env: ACME_SERVER=boulder-v1 TOXENV=integration
-      sudo: required
-      services: docker
-      <<: *extended-test-suite
-    - python: "3.5"
-      env: ACME_SERVER=boulder-v2 TOXENV=integration
-      sudo: required
-      services: docker
-      <<: *extended-test-suite
-    - python: "3.6"
-      env: ACME_SERVER=boulder-v1 TOXENV=integration
-      sudo: required
-      services: docker
-      <<: *extended-test-suite
-    - python: "3.6"
-      env: ACME_SERVER=boulder-v2 TOXENV=integration
-      sudo: required
-      services: docker
-      <<: *extended-test-suite
-    - python: "3.7"
-      dist: xenial
-      env: ACME_SERVER=boulder-v1 TOXENV=integration
-      sudo: required
-      services: docker
-      <<: *extended-test-suite
-    - python: "3.7"
-      dist: xenial
-      env: ACME_SERVER=boulder-v2 TOXENV=integration
-      sudo: required
-      services: docker
-      <<: *extended-test-suite
-    - sudo: required
-      env: TOXENV=le_auto_jessie
-      services: docker
-      <<: *extended-test-suite
-    - sudo: required
-      env: TOXENV=le_auto_centos6
-      services: docker
-      <<: *extended-test-suite
-    - sudo: required
-      env: TOXENV=docker_dev
-      services: docker
-      addons:
-        apt:
-          packages:  # don't install nginx and apache
-            - libaugeas0
-      <<: *extended-test-suite
-    - language: generic
-      env: TOXENV=py27
-      os: osx
-      # Using this osx_image is a workaround for
-      # https://travis-ci.community/t/xcode-8-3-homebrew-outdated-error/3798.
-      osx_image: xcode10.2
-      addons:
-        homebrew:
-          packages:
-            - augeas
-            - python2
-      <<: *extended-test-suite
-    - language: generic
-      env: TOXENV=py3
-      os: osx
-      # Using this osx_image is a workaround for
-      # https://travis-ci.community/t/xcode-8-3-homebrew-outdated-error/3798.
-      osx_image: xcode10.2
-      addons:
-        homebrew:
-          packages:
-            - augeas
-            - python3
-      <<: *extended-test-suite
-
 # container-based infrastructure
 sudo: false
 
@@ -282,13 +68,3 @@ after_success: '[ "$TOXENV" == "py27-cover" ] && codecov -F linux'
 
 notifications:
   email: false
-  irc:
-    channels:
-      # This is set to a secure variable to prevent forks from sending
-      # notifications. This value was created by installing
-      # https://github.com/travis-ci/travis.rb and running
-      # `travis encrypt "chat.freenode.net#certbot-devel"`.
-      - secure: "EWW66E2+KVPZyIPR8ViENZwfcup4Gx3/dlimmAZE0WuLwxDCshBBOd3O8Rf6pBokEoZlXM5eDT6XdyJj8n0DLslgjO62pExdunXpbcMwdY7l1ELxX2/UbnDTE6UnPYa09qVBHNG7156Z6yE0x2lH4M9Ykvp0G0cubjPQHylAwo0="
-    on_cancel: never
-    on_success: never
-    on_failure: always

AUTHORS.md

@@ -15,6 +15,7 @@ Authors
 * [Alex Gaynor](https://github.com/alex)
 * [Alex Halderman](https://github.com/jhalderm)
 * [Alex Jordan](https://github.com/strugee)
+* [Alex Zorin](https://github.com/alexzorin)
 * [Amjad Mashaal](https://github.com/TheNavigat)
 * [Andrew Murray](https://github.com/radarhere)
 * [Anselm Levskaya](https://github.com/levskaya)

CHANGELOG.md

@@ -6,7 +6,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 
 ### Added
 
-*
+* Turn off session tickets for apache plugin by default
+* acme: Authz deactivation added to `acme` module.
 
 ### Changed
 

acme/acme/client.py

@@ -123,6 +123,21 @@ class ClientBase(object):  # pylint: disable=too-many-instance-attributes
         """
         return self.update_registration(regr, update={'status': 'deactivated'})
 
+    def deactivate_authorization(self, authzr):
+        # type: (messages.AuthorizationResource) -> messages.AuthorizationResource
+        """Deactivate authorization.
+
+        :param messages.AuthorizationResource authzr: The Authorization resource
+            to be deactivated.
+
+        :returns: The Authorization resource that was deactivated.
+        :rtype: `.AuthorizationResource`
+
+        """
+        body = messages.UpdateAuthorization(status='deactivated')
+        response = self._post(authzr.uri, body)
+        return self._authzr_from_response(response)
+
     def _authzr_from_response(self, response, identifier=None, uri=None):
         authzr = messages.AuthorizationResource(
             body=messages.Authorization.from_json(response.json()),

acme/acme/client_test.py

@@ -637,6 +637,14 @@ class ClientTest(ClientTestBase):
             errors.PollError, self.client.poll_and_request_issuance,
             csr, authzrs, mintime=mintime, max_attempts=2)
 
+    def test_deactivate_authorization(self):
+        authzb = self.authzr.body.update(status=messages.STATUS_DEACTIVATED)
+        self.response.json.return_value = authzb.to_json()
+        authzr = self.client.deactivate_authorization(self.authzr)
+        self.assertEqual(authzb, authzr.body)
+        self.assertEqual(self.client.net.post.call_count, 1)
+        self.assertTrue(self.authzr.uri in self.net.post.call_args_list[0][0])
+
     def test_check_cert(self):
         self.response.headers['Location'] = self.certr.uri
         self.response.content = CERT_DER

acme/acme/messages.py

@@ -168,6 +168,7 @@ STATUS_VALID = Status('valid')
 STATUS_INVALID = Status('invalid')
 STATUS_REVOKED = Status('revoked')
 STATUS_READY = Status('ready')
+STATUS_DEACTIVATED = Status('deactivated')
 
 
 class IdentifierType(_Constant):
@@ -471,7 +472,7 @@ class Authorization(ResourceBody):
     :ivar datetime.datetime expires:
 
     """
-    identifier = jose.Field('identifier', decoder=Identifier.from_json)
+    identifier = jose.Field('identifier', decoder=Identifier.from_json, omitempty=True)
     challenges = jose.Field('challenges', omitempty=True)
     combinations = jose.Field('combinations', omitempty=True)
 
@@ -501,6 +502,12 @@ class NewAuthorization(Authorization):
     resource = fields.Resource(resource_type)
 
 
+class UpdateAuthorization(Authorization):
+    """Update authorization."""
+    resource_type = 'authz'
+    resource = fields.Resource(resource_type)
+
+
 class AuthorizationResource(ResourceWithURI):
     """Authorization Resource.
 

certbot-apache/MANIFEST.in

@@ -5,3 +5,4 @@ recursive-include certbot_apache/tests/testdata *
 include certbot_apache/centos-options-ssl-apache.conf
 include certbot_apache/options-ssl-apache.conf
 recursive-include certbot_apache/augeas_lens *.aug
+recursive-include certbot_apache/tls_configs *.conf

certbot-apache/certbot_apache/apache_util.py

@@ -1,6 +1,8 @@
 """ Utility functions for certbot-apache plugin """
 import binascii
 
+import pkg_resources
+
 from certbot import util
 from certbot.compat import os
 
@@ -105,3 +107,15 @@ def parse_define_file(filepath, varname):
 def unique_id():
     """ Returns an unique id to be used as a VirtualHost identifier"""
     return binascii.hexlify(os.urandom(16)).decode("utf-8")
+
+
+def find_ssl_apache_conf(prefix):
+    """
+    Find a TLS Apache config file in the dedicated storage.
+    :param str prefix: prefix of the TLS Apache config file to find
+    :return: the path the TLS Apache config file
+    :rtype: str
+    """
+    return pkg_resources.resource_filename(
+        "certbot_apache",
+        os.path.join("tls_configs", "{0}-options-ssl-apache.conf".format(prefix)))

certbot-apache/certbot_apache/configurator.py

@@ -9,7 +9,6 @@ import time
 
 from collections import defaultdict
 
-import pkg_resources
 import six
 
 import zope.component
@@ -110,14 +109,24 @@ class ApacheConfigurator(common.Installer):
         handle_modules=False,
         handle_sites=False,
         challenge_location="/etc/apache2",
-        MOD_SSL_CONF_SRC=pkg_resources.resource_filename(
-            "certbot_apache", "options-ssl-apache.conf")
     )
 
     def option(self, key):
         """Get a value from options"""
         return self.options.get(key)
 
+    def pick_apache_config(self):
+        """
+        Pick the appropriate TLS Apache configuration file for current version of Apache and OS.
+        :return: the path to the TLS Apache configuration file to use
+        :rtype: str
+        """
+        # Disabling TLS session tickets is supported by Apache 2.4.11+.
+        # So for old versions of Apache we pick a configuration without this option.
+        if self.version < (2, 4, 11):
+            return apache_util.find_ssl_apache_conf("old")
+        return apache_util.find_ssl_apache_conf("current")
+
     def _prepare_options(self):
         """
         Set the values possibly changed by command line parameters to
@@ -2339,8 +2348,9 @@ class ApacheConfigurator(common.Installer):
         # XXX if we ever try to enforce a local privilege boundary (eg, running
         # certbot for unprivileged users via setuid), this function will need
         # to be modified.
-        return common.install_version_controlled_file(options_ssl, options_ssl_digest,
-            self.option("MOD_SSL_CONF_SRC"), constants.ALL_SSL_OPTIONS_HASHES)
+        apache_config_path = self.pick_apache_config()
+        return common.install_version_controlled_file(
+            options_ssl, options_ssl_digest, apache_config_path, constants.ALL_SSL_OPTIONS_HASHES)
 
     def enable_autohsts(self, _unused_lineage, domains):
         """

certbot-apache/certbot_apache/constants.py

@@ -9,6 +9,7 @@ MOD_SSL_CONF_DEST = "options-ssl-apache.conf"
 UPDATED_MOD_SSL_CONF_DIGEST = ".updated-options-ssl-apache-conf-digest.txt"
 """Name of the hash of the updated or informed mod_ssl_conf as saved in `IConfig.config_dir`."""
 
+# NEVER REMOVE A SINGLE HASH FROM THIS LIST UNLESS YOU KNOW EXACTLY WHAT YOU ARE DOING!
 ALL_SSL_OPTIONS_HASHES = [
     '2086bca02db48daf93468332543c60ac6acdb6f0b58c7bfdf578a5d47092f82a',
     '4844d36c9a0f587172d9fa10f4f1c9518e3bcfa1947379f155e16a70a728c21a',
@@ -18,6 +19,10 @@ ALL_SSL_OPTIONS_HASHES = [
     'cfdd7c18d2025836ea3307399f509cfb1ebf2612c87dd600a65da2a8e2f2797b',
     '80720bd171ccdc2e6b917ded340defae66919e4624962396b992b7218a561791',
     'c0c022ea6b8a51ecc8f1003d0a04af6c3f2bc1c3ce506b3c2dfc1f11ef931082',
+    '717b0a89f5e4c39b09a42813ac6e747cfbdeb93439499e73f4f70a1fe1473f20',
+    '0fcdc81280cd179a07ec4d29d3595068b9326b455c488de4b09f585d5dafc137',
+    '86cc09ad5415cd6d5f09a947fe2501a9344328b1e8a8b458107ea903e80baa6c',
+    '06675349e457eae856120cdebb564efe546f0b87399f2264baeb41e442c724c7',
 ]
 """SHA256 hashes of the contents of previous versions of all versions of MOD_SSL_CONF_SRC"""
 

certbot-apache/certbot_apache/override_arch.py

@@ -1,6 +1,4 @@
 """ Distribution specific override class for Arch Linux """
-import pkg_resources
-
 import zope.interface
 
 from certbot import interfaces
@@ -26,6 +24,4 @@ class ArchConfigurator(configurator.ApacheConfigurator):
         handle_modules=False,
         handle_sites=False,
         challenge_location="/etc/httpd/conf",
-        MOD_SSL_CONF_SRC=pkg_resources.resource_filename(
-            "certbot_apache", "options-ssl-apache.conf")
     )

certbot-apache/certbot_apache/override_centos.py

@@ -1,7 +1,6 @@
 """ Distribution specific override class for CentOS family (RHEL, Fedora) """
 import logging
 
-import pkg_resources
 import zope.interface
 
 from certbot import errors
@@ -39,8 +38,6 @@ class CentOSConfigurator(configurator.ApacheConfigurator):
         handle_modules=False,
         handle_sites=False,
         challenge_location="/etc/httpd/conf.d",
-        MOD_SSL_CONF_SRC=pkg_resources.resource_filename(
-            "certbot_apache", "centos-options-ssl-apache.conf")
     )
 
     def config_test(self):
@@ -75,6 +72,18 @@ class CentOSConfigurator(configurator.ApacheConfigurator):
         # Finish with actual config check to see if systemctl restart helped
         super(CentOSConfigurator, self).config_test()
 
+    def pick_apache_config(self):
+        """
+        Pick the appropriate TLS Apache configuration file for current version of Apache and OS.
+        :return: the path to the TLS Apache configuration file to use
+        :rtype: str
+        """
+        # Disabling TLS session tickets is supported by Apache 2.4.11+.
+        # So for old versions of Apache we pick a configuration without this option.
+        if self.version < (2, 4, 11):
+            return apache_util.find_ssl_apache_conf("centos-old")
+        return apache_util.find_ssl_apache_conf("centos-current")
+
     def _prepare_options(self):
         """
         Override the options dictionary initialization in order to support

certbot-apache/certbot_apache/override_darwin.py

@@ -1,6 +1,4 @@
 """ Distribution specific override class for macOS """
-import pkg_resources
-
 import zope.interface
 
 from certbot import interfaces
@@ -26,6 +24,4 @@ class DarwinConfigurator(configurator.ApacheConfigurator):
         handle_modules=False,
         handle_sites=False,
         challenge_location="/etc/apache2/other",
-        MOD_SSL_CONF_SRC=pkg_resources.resource_filename(
-            "certbot_apache", "options-ssl-apache.conf")
     )

certbot-apache/certbot_apache/override_debian.py

@@ -1,7 +1,6 @@
 """ Distribution specific override class for Debian family (Ubuntu/Debian) """
 import logging
 
-import pkg_resources
 import zope.interface
 
 from certbot import errors
@@ -35,8 +34,6 @@ class DebianConfigurator(configurator.ApacheConfigurator):
         handle_modules=True,
         handle_sites=True,
         challenge_location="/etc/apache2",
-        MOD_SSL_CONF_SRC=pkg_resources.resource_filename(
-            "certbot_apache", "options-ssl-apache.conf")
     )
 
     def enable_site(self, vhost):

certbot-apache/certbot_apache/override_fedora.py

@@ -1,5 +1,4 @@
 """ Distribution specific override class for Fedora 29+ """
-import pkg_resources
 import zope.interface
 
 from certbot import errors
@@ -31,9 +30,6 @@ class FedoraConfigurator(configurator.ApacheConfigurator):
         handle_modules=False,
         handle_sites=False,
         challenge_location="/etc/httpd/conf.d",
-        MOD_SSL_CONF_SRC=pkg_resources.resource_filename(
-            # TODO: eventually newest version of Fedora will need their own config
-            "certbot_apache", "centos-options-ssl-apache.conf")
     )
 
     def config_test(self):

certbot-apache/certbot_apache/override_gentoo.py

@@ -1,6 +1,4 @@
 """ Distribution specific override class for Gentoo Linux """
-import pkg_resources
-
 import zope.interface
 
 from certbot import interfaces
@@ -29,8 +27,6 @@ class GentooConfigurator(configurator.ApacheConfigurator):
         handle_modules=False,
         handle_sites=False,
         challenge_location="/etc/apache2/vhosts.d",
-        MOD_SSL_CONF_SRC=pkg_resources.resource_filename(
-            "certbot_apache", "options-ssl-apache.conf")
     )
 
     def _prepare_options(self):

certbot-apache/certbot_apache/override_suse.py

@@ -1,6 +1,4 @@
 """ Distribution specific override class for OpenSUSE """
-import pkg_resources
-
 import zope.interface
 
 from certbot import interfaces
@@ -26,6 +24,4 @@ class OpenSUSEConfigurator(configurator.ApacheConfigurator):
         handle_modules=False,
         handle_sites=False,
         challenge_location="/etc/apache2/vhosts.d",
-        MOD_SSL_CONF_SRC=pkg_resources.resource_filename(
-            "certbot_apache", "options-ssl-apache.conf")
     )

certbot-apache/certbot_apache/tests/centos_test.py

@@ -190,6 +190,13 @@ class MultipleVhostsTestCentOS(util.ApacheTest):
                                        errors.SubprocessError]
         self.assertRaises(errors.MisconfigurationError, self.config.restart)
 
+    def test_pick_correct_tls_config(self):
+        self.config.version = (2, 4, 10)
+        self.assertTrue('centos-old' in self.config.pick_apache_config())
+
+        self.config.version = (2, 4, 11)
+        self.assertTrue('centos-current' in self.config.pick_apache_config())
+
 
 if __name__ == "__main__":
     unittest.main()  # pragma: no cover

certbot-apache/certbot_apache/tests/configurator_test.py

@@ -1706,7 +1706,7 @@ class InstallSslOptionsConfTest(util.ApacheTest):
                                              self.config.updated_mod_ssl_conf_digest)
 
     def _current_ssl_options_hash(self):
-        return crypto_util.sha256sum(self.config.option("MOD_SSL_CONF_SRC"))
+        return crypto_util.sha256sum(self.config.pick_apache_config())
 
     def _assert_current_file(self):
         self.assertTrue(os.path.isfile(self.config.mod_ssl_conf))
@@ -1742,7 +1742,7 @@ class InstallSslOptionsConfTest(util.ApacheTest):
             self.assertFalse(mock_logger.warning.called)
         self.assertTrue(os.path.isfile(self.config.mod_ssl_conf))
         self.assertEqual(crypto_util.sha256sum(
-            self.config.option("MOD_SSL_CONF_SRC")),
+            self.config.pick_apache_config()),
             self._current_ssl_options_hash())
         self.assertNotEqual(crypto_util.sha256sum(self.config.mod_ssl_conf),
             self._current_ssl_options_hash())
@@ -1758,18 +1758,31 @@ class InstallSslOptionsConfTest(util.ApacheTest):
                 "%s has been manually modified; updated file "
                 "saved to %s. We recommend updating %s for security purposes.")
         self.assertEqual(crypto_util.sha256sum(
-            self.config.option("MOD_SSL_CONF_SRC")),
+            self.config.pick_apache_config()),
             self._current_ssl_options_hash())
         # only print warning once
         with mock.patch("certbot.plugins.common.logger") as mock_logger:
             self._call()
             self.assertFalse(mock_logger.warning.called)
 
-    def test_current_file_hash_in_all_hashes(self):
+    def test_ssl_config_files_hash_in_all_hashes(self):
+        """
+        It is really critical that all TLS Apache config files have their SHA256 hash registered in
+        constants.ALL_SSL_OPTIONS_HASHES. Otherwise Certbot will mistakenly assume that the config
+        file has been manually edited by the user, and will refuse to update it.
+        This test ensures that all necessary hashes are present.
+        """
         from certbot_apache.constants import ALL_SSL_OPTIONS_HASHES
-        self.assertTrue(self._current_ssl_options_hash() in ALL_SSL_OPTIONS_HASHES,
-            "Constants.ALL_SSL_OPTIONS_HASHES must be appended"
-            " with the sha256 hash of self.config.mod_ssl_conf when it is updated.")
+        import pkg_resources
+        tls_configs_dir = pkg_resources.resource_filename("certbot_apache", "tls_configs")
+        all_files = [os.path.join(tls_configs_dir, name) for name in os.listdir(tls_configs_dir)
+                     if name.endswith('options-ssl-apache.conf')]
+        self.assertTrue(all_files)
+        for one_file in all_files:
+            file_hash = crypto_util.sha256sum(one_file)
+            self.assertTrue(file_hash in ALL_SSL_OPTIONS_HASHES,
+                            "Constants.ALL_SSL_OPTIONS_HASHES must be appended with the sha256 "
+                            "hash of {0} when it is updated.".format(one_file))
 
 
 if __name__ == "__main__":

certbot-apache/certbot_apache/tls_configs/centos-current-options-ssl-apache.conf

@@ -10,16 +10,10 @@ SSLEngine on
 SSLProtocol             all -SSLv2 -SSLv3
 SSLCipherSuite          ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES256-SHA384:ECDHE-ECDSA-AES256-SHA:ECDHE-RSA-AES256-SHA:DHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA:DHE-RSA-AES256-SHA256:DHE-RSA-AES256-SHA:ECDHE-ECDSA-DES-CBC3-SHA:ECDHE-RSA-DES-CBC3-SHA:EDH-RSA-DES-CBC3-SHA:AES128-GCM-SHA256:AES256-GCM-SHA384:AES128-SHA256:AES256-SHA256:AES128-SHA:AES256-SHA:DES-CBC3-SHA:!DSS
 SSLHonorCipherOrder     on
+SSLSessionTickets       off
 
 SSLOptions +StrictRequire
 
 # Add vhost name to log entries:
 LogFormat "%h %l %u %t \"%r\" %>s %b \"%{Referer}i\" \"%{User-agent}i\"" vhost_combined
 LogFormat "%v %h %l %u %t \"%r\" %>s %b" vhost_common
-
-#CustomLog /var/log/apache2/access.log vhost_combined
-#LogLevel warn
-#ErrorLog /var/log/apache2/error.log
-
-# Always ensure Cookies have "Secure" set (JAH 2012/1)
-#Header edit Set-Cookie (?i)^(.*)(;\s*secure)??((\s*;)?(.*)) "$1; Secure$3$4"

certbot-apache/certbot_apache/tls_configs/centos-old-options-ssl-apache.conf

@@ -0,0 +1,18 @@
+# This file contains important security parameters. If you modify this file
+# manually, Certbot will be unable to automatically provide future security
+# updates. Instead, Certbot will print and log an error message with a path to
+# the up-to-date file that you will need to refer to when manually updating
+# this file.
+
+SSLEngine on
+
+# Intermediate configuration, tweak to your needs
+SSLProtocol             all -SSLv2 -SSLv3
+SSLCipherSuite          ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES256-SHA384:ECDHE-ECDSA-AES256-SHA:ECDHE-RSA-AES256-SHA:DHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA:DHE-RSA-AES256-SHA256:DHE-RSA-AES256-SHA:ECDHE-ECDSA-DES-CBC3-SHA:ECDHE-RSA-DES-CBC3-SHA:EDH-RSA-DES-CBC3-SHA:AES128-GCM-SHA256:AES256-GCM-SHA384:AES128-SHA256:AES256-SHA256:AES128-SHA:AES256-SHA:DES-CBC3-SHA:!DSS
+SSLHonorCipherOrder     on
+
+SSLOptions +StrictRequire
+
+# Add vhost name to log entries:
+LogFormat "%h %l %u %t \"%r\" %>s %b \"%{Referer}i\" \"%{User-agent}i\"" vhost_combined
+LogFormat "%v %h %l %u %t \"%r\" %>s %b" vhost_common

certbot-apache/certbot_apache/tls_configs/current-options-ssl-apache.conf

@@ -11,16 +11,10 @@ SSLProtocol             all -SSLv2 -SSLv3
 SSLCipherSuite          ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES256-SHA384:ECDHE-ECDSA-AES256-SHA:ECDHE-RSA-AES256-SHA:DHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA:DHE-RSA-AES256-SHA256:DHE-RSA-AES256-SHA:ECDHE-ECDSA-DES-CBC3-SHA:ECDHE-RSA-DES-CBC3-SHA:EDH-RSA-DES-CBC3-SHA:AES128-GCM-SHA256:AES256-GCM-SHA384:AES128-SHA256:AES256-SHA256:AES128-SHA:AES256-SHA:DES-CBC3-SHA:!DSS
 SSLHonorCipherOrder     on
 SSLCompression          off
+SSLSessionTickets       off
 
 SSLOptions +StrictRequire
 
 # Add vhost name to log entries:
 LogFormat "%h %l %u %t \"%r\" %>s %b \"%{Referer}i\" \"%{User-agent}i\"" vhost_combined
 LogFormat "%v %h %l %u %t \"%r\" %>s %b" vhost_common
-
-#CustomLog /var/log/apache2/access.log vhost_combined
-#LogLevel warn
-#ErrorLog /var/log/apache2/error.log
-
-# Always ensure Cookies have "Secure" set (JAH 2012/1)
-#Header edit Set-Cookie (?i)^(.*)(;\s*secure)??((\s*;)?(.*)) "$1; Secure$3$4"

certbot-apache/certbot_apache/tls_configs/old-options-ssl-apache.conf

@@ -0,0 +1,19 @@
+# This file contains important security parameters. If you modify this file
+# manually, Certbot will be unable to automatically provide future security
+# updates. Instead, Certbot will print and log an error message with a path to
+# the up-to-date file that you will need to refer to when manually updating
+# this file.
+
+SSLEngine on
+
+# Intermediate configuration, tweak to your needs
+SSLProtocol             all -SSLv2 -SSLv3
+SSLCipherSuite          ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES256-SHA384:ECDHE-ECDSA-AES256-SHA:ECDHE-RSA-AES256-SHA:DHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA:DHE-RSA-AES256-SHA256:DHE-RSA-AES256-SHA:ECDHE-ECDSA-DES-CBC3-SHA:ECDHE-RSA-DES-CBC3-SHA:EDH-RSA-DES-CBC3-SHA:AES128-GCM-SHA256:AES256-GCM-SHA384:AES128-SHA256:AES256-SHA256:AES128-SHA:AES256-SHA:DES-CBC3-SHA:!DSS
+SSLHonorCipherOrder     on
+SSLCompression          off
+
+SSLOptions +StrictRequire
+
+# Add vhost name to log entries:
+LogFormat "%h %l %u %t \"%r\" %>s %b \"%{Referer}i\" \"%{User-agent}i\"" vhost_combined
+LogFormat "%v %h %l %u %t \"%r\" %>s %b" vhost_common

certbot-ci/certbot_integration_tests/utils/acme_server.py

@@ -159,7 +159,7 @@ class ACMEServer(object):
 
         # Wait for the ACME CA server to be up.
         print('=> Waiting for boulder instance to respond...')
-        misc.check_until_timeout(self.acme_xdist['directory_url'])
+        misc.check_until_timeout(self.acme_xdist['directory_url'], attempts=240)
 
         # Configure challtestsrv to answer any A record request with ip of the docker host.
         response = requests.post('http://localhost:{0}/set-default-ipv4'.format(CHALLTESTSRV_PORT),

certbot-ci/certbot_integration_tests/utils/misc.py

@@ -28,12 +28,13 @@ RSA_KEY_TYPE = 'rsa'
 ECDSA_KEY_TYPE = 'ecdsa'
 
 
-def check_until_timeout(url):
+def check_until_timeout(url, attempts=30):
     """
     Wait and block until given url responds with status 200, or raise an exception
-    after 150 attempts.
+    after the specified number of attempts.
     :param str url: the URL to test
-    :raise ValueError: exception raised after 150 unsuccessful attempts to reach the URL
+    :param int attempts: the number of times to try to connect to the URL
+    :raise ValueError: exception raised if unable to reach the URL
     """
     try:
         import urllib3
@@ -43,7 +44,7 @@ def check_until_timeout(url):
         from requests.packages.urllib3.exceptions import InsecureRequestWarning
         requests.packages.urllib3.disable_warnings(InsecureRequestWarning)
 
-    for _ in range(0, 150):
+    for _ in range(attempts):
         time.sleep(1)
         try:
             if requests.get(url, verify=False).status_code == 200:
@@ -51,7 +52,7 @@ def check_until_timeout(url):
         except requests.exceptions.ConnectionError:
             pass
 
-    raise ValueError('Error, url did not respond after 150 attempts: {0}'.format(url))
+    raise ValueError('Error, url did not respond after {0} attempts: {1}'.format(attempts, url))
 
 
 class GracefulTCPServer(socketserver.TCPServer):

certbot-dns-cloudflare/certbot_dns_cloudflare/__init__.py

@@ -22,7 +22,9 @@ Credentials
 
 Use of this plugin requires a configuration file containing Cloudflare API
 credentials, obtained from your Cloudflare
-`account page <https://www.cloudflare.com/a/account/my-account>`_.
+`account page <https://www.cloudflare.com/a/account/my-account>`_. This plugin
+does not currently support Cloudflare's "API Tokens", so please ensure you use
+the "Global API Key" for authentication.
 
 .. code-block:: ini
    :name: credentials.ini

certbot-nginx/certbot_nginx/configurator.py

@@ -20,7 +20,6 @@ from certbot import crypto_util
 from certbot import errors
 from certbot import interfaces
 from certbot import util
-from certbot.compat import misc
 from certbot.compat import os
 from certbot.plugins import common
 
@@ -903,13 +902,9 @@ class NginxConfigurator(common.Installer):
         have permissions of root.
 
         """
-        uid = misc.os_geteuid()
-        util.make_or_verify_dir(
-            self.config.work_dir, core_constants.CONFIG_DIRS_MODE, uid)
-        util.make_or_verify_dir(
-            self.config.backup_dir, core_constants.CONFIG_DIRS_MODE, uid)
-        util.make_or_verify_dir(
-            self.config.config_dir, core_constants.CONFIG_DIRS_MODE, uid)
+        util.make_or_verify_dir(self.config.work_dir, core_constants.CONFIG_DIRS_MODE)
+        util.make_or_verify_dir(self.config.backup_dir, core_constants.CONFIG_DIRS_MODE)
+        util.make_or_verify_dir(self.config.config_dir, core_constants.CONFIG_DIRS_MODE)
 
     def get_version(self):
         """Return version of Nginx Server.

certbot/account.py

@@ -20,7 +20,6 @@ from certbot import constants
 from certbot import errors
 from certbot import interfaces
 from certbot import util
-from certbot.compat import misc
 from certbot.compat import os
 
 logger = logging.getLogger(__name__)
@@ -139,8 +138,7 @@ class AccountFileStorage(interfaces.AccountStorage):
     """
     def __init__(self, config):
         self.config = config
-        util.make_or_verify_dir(config.accounts_dir, 0o700, misc.os_geteuid(),
-                                self.config.strict_permissions)
+        util.make_or_verify_dir(config.accounts_dir, 0o700, 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)
@@ -322,8 +320,7 @@ class AccountFileStorage(interfaces.AccountStorage):
 
     def _save(self, account, acme, regr_only):
         account_dir_path = self._account_dir_path(account.id)
-        util.make_or_verify_dir(account_dir_path, 0o700, misc.os_geteuid(),
-                                self.config.strict_permissions)
+        util.make_or_verify_dir(account_dir_path, 0o700, self.config.strict_permissions)
         try:
             with open(self._regr_path(account_dir_path), "w") as regr_file:
                 regr = account.regr

certbot/cert_manager.py

@@ -15,7 +15,6 @@ from certbot import interfaces
 from certbot import ocsp
 from certbot import storage
 from certbot import util
-from certbot.compat import misc
 from certbot.compat import os
 from certbot.display import util as display_util
 
@@ -106,7 +105,7 @@ def lineage_for_certname(cli_config, certname):
     """Find a lineage object with name certname."""
     configs_dir = cli_config.renewal_configs_dir
     # Verify the directory is there
-    util.make_or_verify_dir(configs_dir, mode=0o755, uid=misc.os_geteuid())
+    util.make_or_verify_dir(configs_dir, mode=0o755)
     try:
         renewal_file = storage.renewal_file_for_certname(cli_config, certname)
     except errors.CertStorageError:
@@ -375,7 +374,7 @@ def _search_lineages(cli_config, func, initial_rv, *args):
     """
     configs_dir = cli_config.renewal_configs_dir
     # Verify the directory is there
-    util.make_or_verify_dir(configs_dir, mode=0o755, uid=misc.os_geteuid())
+    util.make_or_verify_dir(configs_dir, mode=0o755)
 
     rv = initial_rv
     for renewal_file in storage.renewal_conf_files(cli_config):

certbot/client.py

@@ -30,7 +30,6 @@ from certbot import interfaces
 from certbot import reverter
 from certbot import storage
 from certbot import util
-from certbot.compat import misc
 from certbot.compat import os
 from certbot.display import enhancements
 from certbot.display import ops as display_ops
@@ -459,9 +458,7 @@ class Client(object):
 
         """
         for path in cert_path, chain_path, fullchain_path:
-            util.make_or_verify_dir(
-                os.path.dirname(path), 0o755, misc.os_geteuid(),
-                self.config.strict_permissions)
+            util.make_or_verify_dir(os.path.dirname(path), 0o755, self.config.strict_permissions)
 
 
         cert_file, abs_cert_path = _open_pem_file('cert_path', cert_path)

certbot/compat/filesystem.py

@@ -77,6 +77,54 @@ def copy_ownership_and_apply_mode(src, dst, mode, copy_user, copy_group):
     chmod(dst, mode)
 
 
+def check_mode(file_path, mode):
+    # type: (str, int) -> bool
+    """
+    Check if the given mode matches the permissions of the given file.
+    On Linux, will make a direct comparison, on Windows, mode will be compared against
+    the security model.
+    :param str file_path: Path of the file
+    :param int mode: POSIX mode to test
+    :rtype: bool
+    :return: True if the POSIX mode matches the file permissions
+    """
+    if POSIX_MODE:
+        return stat.S_IMODE(os.stat(file_path).st_mode) == mode
+
+    return _check_win_mode(file_path, mode)
+
+
+def check_owner(file_path):
+    # type: (str) -> bool
+    """
+    Check if given file is owned by current user.
+    :param str file_path: File path to check
+    :rtype: bool
+    :return: True if given file is owned by current user, False otherwise.
+    """
+    if POSIX_MODE:
+        return os.stat(file_path).st_uid == os.getuid()
+
+    # Get owner sid of the file
+    security = win32security.GetFileSecurity(file_path, win32security.OWNER_SECURITY_INFORMATION)
+    user = security.GetSecurityDescriptorOwner()
+
+    # Compare sids
+    return _get_current_user() == user
+
+
+def check_permissions(file_path, mode):
+    # type: (str, int) -> bool
+    """
+    Check if given file has the given mode and is owned by current user.
+    :param str file_path: File path to check
+    :param int mode: POSIX mode to check
+    :rtype: bool
+    :return: True if file has correct mode and owner, False otherwise.
+    """
+    return check_owner(file_path) and check_mode(file_path, mode)
+
+
 def open(file_path, flags, mode=0o777):  # pylint: disable=redefined-builtin
     # type: (str, int, int) -> int
     """
@@ -107,6 +155,10 @@ def open(file_path, flags, mode=0o777):  # pylint: disable=redefined-builtin
         security = attributes.SECURITY_DESCRIPTOR
         user = _get_current_user()
         dacl = _generate_dacl(user, mode)
+        # We set second parameter to 0 (`False`) to say that this security descriptor is
+        # NOT constructed from a default mechanism, but is explicitly set by the user.
+        # See https://docs.microsoft.com/en-us/windows/desktop/api/securitybaseapi/nf-securitybaseapi-setsecuritydescriptorowner  # pylint: disable=line-too-long
+        security.SetSecurityDescriptorOwner(user, 0)
         # We set first parameter to 1 (`True`) to say that this security descriptor contains
         # a DACL. Otherwise second and third parameters are ignored.
         # We set third parameter to 0 (`False`) to say that this security descriptor is
@@ -177,6 +229,7 @@ def mkdir(file_path, mode=0o777):
     security = attributes.SECURITY_DESCRIPTOR
     user = _get_current_user()
     dacl = _generate_dacl(user, mode)
+    security.SetSecurityDescriptorOwner(user, False)
     security.SetSecurityDescriptorDacl(1, dacl, 0)
 
     try:
@@ -236,6 +289,42 @@ def realpath(file_path):
     return os.path.abspath(file_path)
 
 
+# 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
+# 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
+# requires to be run under a privileged shell, so the user will always benefit
+# from the highest (privileged one) set of permissions on a given file.
+def is_executable(path):
+    """
+    Is path an executable file?
+    :param str path: path to test
+    :returns: True if path is an executable file
+    :rtype: bool
+    """
+    if POSIX_MODE:
+        return os.path.isfile(path) and os.access(path, os.X_OK)
+
+    return _win_is_executable(path)
+
+
+def _win_is_executable(path):
+    if not os.path.isfile(path):
+        return False
+
+    security = win32security.GetFileSecurity(path, win32security.DACL_SECURITY_INFORMATION)
+    dacl = security.GetSecurityDescriptorDacl()
+
+    mode = dacl.GetEffectiveRightsFromAcl({
+        'TrusteeForm': win32security.TRUSTEE_IS_SID,
+        'TrusteeType': win32security.TRUSTEE_IS_USER,
+        'Identifier': _get_current_user(),
+    })
+
+    return mode & ntsecuritycon.FILE_GENERIC_EXECUTE == ntsecuritycon.FILE_GENERIC_EXECUTE
+
+
 def _apply_win_mode(file_path, mode):
     """
     This function converts the given POSIX mode into a Windows ACL list, and applies it to the
@@ -353,6 +442,28 @@ def _generate_windows_flags(rights_desc):
     return flag
 
 
+def _check_win_mode(file_path, mode):
+    # Resolve symbolic links
+    file_path = realpath(file_path)
+    # Get current dacl file
+    security = win32security.GetFileSecurity(file_path, win32security.OWNER_SECURITY_INFORMATION
+                                             | win32security.DACL_SECURITY_INFORMATION)
+    dacl = security.GetSecurityDescriptorDacl()
+
+    # Get current file owner sid
+    user = security.GetSecurityDescriptorOwner()
+
+    if not dacl:
+        # No DACL means full control to everyone
+        # This is not a deterministic permissions set.
+        return False
+
+    # Calculate the target dacl
+    ref_dacl = _generate_dacl(user, mode)
+
+    return _compare_dacls(dacl, ref_dacl)
+
+
 def _compare_dacls(dacl1, dacl2):
     """
     This method compare the two given DACLs to check if they are identical.

certbot/compat/misc.py

@@ -30,6 +30,10 @@ else:
     MASK_FOR_PRIVATE_KEY_PERMISSIONS = 0
 
 
+# For Linux: define OS specific standard binary directories
+STANDARD_BINARY_DIRS = ["/usr/sbin", "/usr/local/bin", "/usr/local/sbin"] if POSIX_MODE else []
+
+
 def raise_for_non_administrative_windows_rights():
     # type: () -> None
     """
@@ -42,22 +46,6 @@ def raise_for_non_administrative_windows_rights():
         raise errors.Error('Error, certbot must be run on a shell with administrative rights.')
 
 
-def os_geteuid():
-    """
-    Get current user uid
-
-    :returns: The current user uid.
-    :rtype: int
-
-    """
-    try:
-        # Linux specific
-        return os.geteuid()
-    except AttributeError:
-        # Windows specific
-        return 0
-
-
 def readline_with_timeout(timeout, prompt):
     # type: (float, str) -> str
     """
@@ -88,16 +76,6 @@ def readline_with_timeout(timeout, prompt):
         return sys.stdin.readline()
 
 
-def compare_file_modes(mode1, mode2):
-    """Return true if the two modes can be considered as equals for this platform"""
-    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',

certbot/compat/os.py

@@ -107,3 +107,12 @@ def replace(*unused_args, **unused_kwargs):
     """Method os.replace() is forbidden"""
     raise RuntimeError('Usage of os.replace() is forbidden. '
                        'Use certbot.compat.filesystem.replace() instead.')
+
+
+# Results given by os.access are inconsistent or partial on Windows, because this platform is not
+# following the POSIX approach.
+def access(*unused_args, **unused_kwargs):
+    """Method os.access() is forbidden"""
+    raise RuntimeError('Usage of os.access() is forbidden. '
+                       'Use certbot.compat.filesystem.check_mode() or '
+                       'certbot.compat.filesystem.is_executable() instead.')

certbot/constants.py

@@ -169,9 +169,10 @@ ACCOUNTS_DIR = "accounts"
 """Directory where all accounts are saved."""
 
 LE_REUSE_SERVERS = {
-    'acme-v02.api.letsencrypt.org/directory': 'acme-v01.api.letsencrypt.org/directory',
-    'acme-staging-v02.api.letsencrypt.org/directory':
-        'acme-staging.api.letsencrypt.org/directory'
+    os.path.normpath('acme-v02.api.letsencrypt.org/directory'):
+        os.path.normpath('acme-v01.api.letsencrypt.org/directory'),
+    os.path.normpath('acme-staging-v02.api.letsencrypt.org/directory'):
+        os.path.normpath('acme-staging.api.letsencrypt.org/directory')
 }
 """Servers that can reuse accounts from other servers."""
 

certbot/crypto_util.py

@@ -28,7 +28,6 @@ from acme.magic_typing import IO  # pylint: disable=unused-import, no-name-in-mo
 from certbot import errors
 from certbot import interfaces
 from certbot import util
-from certbot.compat import misc
 from certbot.compat import os
 
 logger = logging.getLogger(__name__)
@@ -61,8 +60,7 @@ def init_save_key(key_size, key_dir, keyname="key-certbot.pem"):
 
     config = zope.component.getUtility(interfaces.IConfig)
     # Save file
-    util.make_or_verify_dir(key_dir, 0o700, misc.os_geteuid(),
-                            config.strict_permissions)
+    util.make_or_verify_dir(key_dir, 0o700, config.strict_permissions)
     key_f, key_path = util.unique_file(
         os.path.join(key_dir, keyname), 0o600, "wb")
     with key_f:
@@ -92,8 +90,7 @@ def init_save_csr(privkey, names, path):
         privkey.pem, names, must_staple=config.must_staple)
 
     # Save CSR
-    util.make_or_verify_dir(path, 0o755, misc.os_geteuid(),
-                            config.strict_permissions)
+    util.make_or_verify_dir(path, 0o755, config.strict_permissions)
     csr_f, csr_filename = util.unique_file(
         os.path.join(path, "csr-certbot.pem"), 0o644, "wb")
     with csr_f:

certbot/hooks.py

@@ -8,6 +8,7 @@ from acme.magic_typing import Set, List  # pylint: disable=unused-import, no-nam
 
 from certbot import errors
 from certbot import util
+from certbot.compat import filesystem
 from certbot.compat import os
 from certbot.plugins import util as plug_util
 
@@ -254,7 +255,7 @@ def execute(cmd_name, shell_cmd):
                      cmd_name, shell_cmd, cmd.returncode)
     if err:
         logger.error('Error output from %s command %s:\n%s', cmd_name, base_cmd, err)
-    return (err, out)
+    return err, out
 
 
 def list_hooks(dir_path):
@@ -267,5 +268,5 @@ def list_hooks(dir_path):
 
     """
     allpaths = (os.path.join(dir_path, f) for f in os.listdir(dir_path))
-    hooks = [path for path in allpaths if util.is_exe(path) and not path.endswith('~')]
+    hooks = [path for path in allpaths if filesystem.is_executable(path) and not path.endswith('~')]
     return sorted(hooks)

certbot/log.py

@@ -17,6 +17,7 @@ from __future__ import print_function
 import functools
 import logging
 import logging.handlers
+import shutil
 import sys
 import tempfile
 import traceback
@@ -26,7 +27,6 @@ from acme import messages
 from certbot import constants
 from certbot import errors
 from certbot import util
-from certbot.compat import misc
 from certbot.compat import os
 
 # Logging format
@@ -134,8 +134,7 @@ def setup_log_file_handler(config, logfile, fmt):
     """
     # TODO: logs might contain sensitive data such as contents of the
     # private key! #525
-    util.set_up_core_dir(
-        config.logs_dir, 0o700, misc.os_geteuid(), config.strict_permissions)
+    util.set_up_core_dir(config.logs_dir, 0o700, config.strict_permissions)
     log_file_path = os.path.join(config.logs_dir, logfile)
     try:
         handler = logging.handlers.RotatingFileHandler(
@@ -240,9 +239,10 @@ class TempHandler(logging.StreamHandler):
 
     """
     def __init__(self):
-        stream = tempfile.NamedTemporaryFile('w', delete=False)
+        self._workdir = tempfile.mkdtemp()
+        self.path = os.path.join(self._workdir, 'log')
+        stream = util.safe_open(self.path, mode='w', chmod=0o600)
         super(TempHandler, self).__init__(stream)
-        self.path = stream.name
         self._delete = True
 
     def emit(self, record):
@@ -266,7 +266,7 @@ class TempHandler(logging.StreamHandler):
             # stream like stderr to be used
             self.stream.close()
             if self._delete:
-                os.remove(self.path)
+                shutil.rmtree(self._workdir)
             self._delete = False
             super(TempHandler, self).close()
         finally:

certbot/main.py

@@ -1299,18 +1299,14 @@ def make_or_verify_needed_dirs(config):
     :rtype: None
 
     """
-    util.set_up_core_dir(config.config_dir, constants.CONFIG_DIRS_MODE,
-                         misc.os_geteuid(), config.strict_permissions)
-    util.set_up_core_dir(config.work_dir, constants.CONFIG_DIRS_MODE,
-                         misc.os_geteuid(), config.strict_permissions)
+    util.set_up_core_dir(config.config_dir, constants.CONFIG_DIRS_MODE, config.strict_permissions)
+    util.set_up_core_dir(config.work_dir, constants.CONFIG_DIRS_MODE, config.strict_permissions)
 
     hook_dirs = (config.renewal_pre_hooks_dir,
                  config.renewal_deploy_hooks_dir,
                  config.renewal_post_hooks_dir,)
     for hook_dir in hook_dirs:
-        util.make_or_verify_dir(hook_dir,
-                                uid=misc.os_geteuid(),
-                                strict=config.strict_permissions)
+        util.make_or_verify_dir(hook_dir, strict=config.strict_permissions)
 
 
 def set_displayer(config):

certbot/plugins/util.py

@@ -3,9 +3,11 @@ import logging
 
 from certbot import util
 from certbot.compat import os
+from certbot.compat.misc import STANDARD_BINARY_DIRS
 
 logger = logging.getLogger(__name__)
 
+
 def get_prefixes(path):
     """Retrieves all possible path prefixes of a path, in descending order
     of length. For instance,
@@ -26,6 +28,7 @@ def get_prefixes(path):
             break
     return prefixes
 
+
 def path_surgery(cmd):
     """Attempt to perform PATH surgery to find cmd
 
@@ -35,10 +38,9 @@ def path_surgery(cmd):
 
     :returns: True if the operation succeeded, False otherwise
     """
-    dirs = ("/usr/sbin", "/usr/local/bin", "/usr/local/sbin")
     path = os.environ["PATH"]
     added = []
-    for d in dirs:
+    for d in STANDARD_BINARY_DIRS:
         if d not in path:
             path += os.pathsep + d
             added.append(d)

certbot/plugins/util_test.py

@@ -16,6 +16,7 @@ class GetPrefixTest(unittest.TestCase):
         self.assertEqual(get_prefixes('/'), [os.path.normpath('/')])
         self.assertEqual(get_prefixes('a'), ['a'])
 
+
 class PathSurgeryTest(unittest.TestCase):
     """Tests for certbot.plugins.path_surgery."""
 
@@ -29,13 +30,15 @@ class PathSurgeryTest(unittest.TestCase):
                 self.assertEqual(path_surgery("eg"), True)
                 self.assertEqual(mock_debug.call_count, 0)
                 self.assertEqual(os.environ["PATH"], all_path["PATH"])
-        no_path = {"PATH": "/tmp/"}
-        with mock.patch.dict('os.environ', no_path):
-            path_surgery("thingy")
-            self.assertEqual(mock_debug.call_count, 2)
-            self.assertTrue("Failed to find" in mock_debug.call_args[0][0])
-            self.assertTrue("/usr/local/bin" in os.environ["PATH"])
-            self.assertTrue("/tmp" in os.environ["PATH"])
+        if os.name != 'nt':
+            # This part is specific to Linux since on Windows no PATH surgery is ever done.
+            no_path = {"PATH": "/tmp/"}
+            with mock.patch.dict('os.environ', no_path):
+                path_surgery("thingy")
+                self.assertEqual(mock_debug.call_count, 2 if os.name != 'nt' else 1)
+                self.assertTrue("Failed to find" in mock_debug.call_args[0][0])
+                self.assertTrue("/usr/local/bin" in os.environ["PATH"])
+                self.assertTrue("/tmp" in os.environ["PATH"])
 
 
 if __name__ == "__main__":

certbot/plugins/webroot.py

@@ -24,6 +24,7 @@ from certbot.display import ops
 from certbot.display import util as display_util
 from certbot.plugins import common
 from certbot.plugins import util
+from certbot.util import safe_open
 
 logger = logging.getLogger(__name__)
 
@@ -207,7 +208,7 @@ to serve all files under specified web root ({0})."""
         old_umask = os.umask(0o022)
 
         try:
-            with open(validation_path, "wb") as validation_file:
+            with safe_open(validation_path, mode="wb", chmod=0o644) as validation_file:
                 validation_file.write(validation.encode())
         finally:
             os.umask(old_umask)

certbot/plugins/webroot_test.py

@@ -17,7 +17,6 @@ from acme import challenges
 
 from certbot import achallenges
 from certbot import errors
-from certbot.compat import misc
 from certbot.compat import os
 from certbot.compat import filesystem
 from certbot.display import util as display_util
@@ -168,14 +167,14 @@ class AuthenticatorTest(unittest.TestCase):
         # Remove exec bit from permission check, so that it
         # matches the file
         self.auth.perform([self.achall])
-        self.assertTrue(misc.compare_file_modes(os.stat(self.validation_path).st_mode, 0o644))
+        self.assertTrue(filesystem.check_mode(self.validation_path, 0o644))
 
         # Check permissions of the directories
 
         for dirpath, dirnames, _ in os.walk(self.path):
             for directory in dirnames:
                 full_path = os.path.join(dirpath, directory)
-                self.assertTrue(misc.compare_file_modes(os.stat(full_path).st_mode, 0o755))
+                self.assertTrue(filesystem.check_mode(full_path, 0o755))
 
         parent_gid = os.stat(self.path).st_gid
         parent_uid = os.stat(self.path).st_uid

certbot/reverter.py

@@ -15,7 +15,6 @@ from certbot import constants
 from certbot import errors
 from certbot import interfaces
 from certbot import util
-from certbot.compat import misc
 from certbot.compat import os
 from certbot.compat import filesystem
 
@@ -68,8 +67,7 @@ class Reverter(object):
         self.config = config
 
         util.make_or_verify_dir(
-            config.backup_dir, constants.CONFIG_DIRS_MODE, misc.os_geteuid(),
-            self.config.strict_permissions)
+            config.backup_dir, constants.CONFIG_DIRS_MODE, self.config.strict_permissions)
 
     def revert_temporary_config(self):
         """Reload users original configuration files after a temporary save.
@@ -225,8 +223,7 @@ class Reverter(object):
 
         """
         util.make_or_verify_dir(
-            cp_dir, constants.CONFIG_DIRS_MODE, misc.os_geteuid(),
-            self.config.strict_permissions)
+            cp_dir, constants.CONFIG_DIRS_MODE, self.config.strict_permissions)
 
         op_fd, existing_filepaths = self._read_and_append(
             os.path.join(cp_dir, "FILEPATHS"))
@@ -445,8 +442,7 @@ class Reverter(object):
             cp_dir = self.config.in_progress_dir
 
         util.make_or_verify_dir(
-            cp_dir, constants.CONFIG_DIRS_MODE, misc.os_geteuid(),
-            self.config.strict_permissions)
+            cp_dir, constants.CONFIG_DIRS_MODE, self.config.strict_permissions)
 
         return cp_dir
 

certbot/tests/account_test.py

@@ -2,7 +2,6 @@
 import datetime
 import json
 import shutil
-import stat
 import unittest
 
 import josepy as jose
@@ -13,6 +12,7 @@ from acme import messages
 
 import certbot.tests.util as test_util
 from certbot import errors
+from certbot.compat import filesystem
 from certbot.compat import misc
 from certbot.compat import os
 
@@ -116,7 +116,6 @@ class AccountFileStorageTest(test_util.ConfigTestCase):
         self.assertTrue(os.path.isdir(
             misc.underscores_for_unsupported_characters_in_path(self.config.accounts_dir)))
 
-    @test_util.broken_on_windows
     def test_save_and_restore(self):
         self.storage.save(self.acc, self.mock_client)
         account_path = os.path.join(self.config.accounts_dir, self.acc.id)
@@ -124,8 +123,8 @@ class AccountFileStorageTest(test_util.ConfigTestCase):
         for file_name in "regr.json", "meta.json", "private_key.json":
             self.assertTrue(os.path.exists(
                 os.path.join(account_path, file_name)))
-        self.assertTrue(oct(os.stat(os.path.join(
-            account_path, "private_key.json"))[stat.ST_MODE] & 0o777) in ("0400", "0o400"))
+        self.assertTrue(
+            filesystem.check_mode(os.path.join(account_path, "private_key.json"), 0o400))
 
         # restore
         loaded = self.storage.load(self.acc.id)
@@ -219,14 +218,12 @@ class AccountFileStorageTest(test_util.ConfigTestCase):
         self._set_server('https://acme-staging.api.letsencrypt.org/directory')
         self.assertEqual([], self.storage.find_all())
 
-    @test_util.broken_on_windows
     def test_upgrade_version_staging(self):
         self._set_server('https://acme-staging.api.letsencrypt.org/directory')
         self.storage.save(self.acc, self.mock_client)
         self._set_server('https://acme-staging-v02.api.letsencrypt.org/directory')
         self.assertEqual([self.acc], self.storage.find_all())
 
-    @test_util.broken_on_windows
     def test_upgrade_version_production(self):
         self._set_server('https://acme-v01.api.letsencrypt.org/directory')
         self.storage.save(self.acc, self.mock_client)
@@ -244,7 +241,6 @@ class AccountFileStorageTest(test_util.ConfigTestCase):
         self._set_server('https://acme-staging-v02.api.letsencrypt.org/directory')
         self.assertEqual([], self.storage.find_all())
 
-    @test_util.broken_on_windows
     def test_upgrade_load(self):
         self._set_server('https://acme-staging.api.letsencrypt.org/directory')
         self.storage.save(self.acc, self.mock_client)
@@ -253,7 +249,6 @@ class AccountFileStorageTest(test_util.ConfigTestCase):
         account = self.storage.load(self.acc.id)
         self.assertEqual(prev_account, account)
 
-    @test_util.broken_on_windows
     def test_upgrade_load_single_account(self):
         self._set_server('https://acme-staging.api.letsencrypt.org/directory')
         self.storage.save(self.acc, self.mock_client)
@@ -278,7 +273,6 @@ class AccountFileStorageTest(test_util.ConfigTestCase):
                 errors.AccountStorageError, self.storage.save,
                     self.acc, self.mock_client)
 
-    @test_util.broken_on_windows
     def test_delete(self):
         self.storage.save(self.acc, self.mock_client)
         self.storage.delete(self.acc.id)
@@ -313,12 +307,10 @@ class AccountFileStorageTest(test_util.ConfigTestCase):
         self._set_server('https://acme-staging-v02.api.letsencrypt.org/directory')
         self.assertRaises(errors.AccountNotFound, self.storage.load, self.acc.id)
 
-    @test_util.broken_on_windows
     def test_delete_folders_up(self):
         self._test_delete_folders('https://acme-staging.api.letsencrypt.org/directory')
         self._assert_symlinked_account_removed()
 
-    @test_util.broken_on_windows
     def test_delete_folders_down(self):
         self._test_delete_folders('https://acme-staging-v02.api.letsencrypt.org/directory')
         self._assert_symlinked_account_removed()
@@ -328,15 +320,14 @@ class AccountFileStorageTest(test_util.ConfigTestCase):
         with open(os.path.join(self.config.accounts_dir, 'foo'), 'w') as f:
             f.write('bar')
 
-    @test_util.broken_on_windows
     def test_delete_shared_account_up(self):
         self._set_server_and_stop_symlink('https://acme-staging-v02.api.letsencrypt.org/directory')
         self._test_delete_folders('https://acme-staging.api.letsencrypt.org/directory')
 
-    @test_util.broken_on_windows
     def test_delete_shared_account_down(self):
         self._set_server_and_stop_symlink('https://acme-staging-v02.api.letsencrypt.org/directory')
         self._test_delete_folders('https://acme-staging-v02.api.letsencrypt.org/directory')
 
+
 if __name__ == "__main__":
     unittest.main()  # pragma: no cover

certbot/tests/cert_manager_test.py

@@ -277,13 +277,12 @@ class SearchLineagesTest(BaseCertManagerTest):
     @mock.patch('certbot.storage.renewal_conf_files')
     @mock.patch('certbot.storage.RenewableCert')
     def test_cert_storage_error(self, mock_renewable_cert, mock_renewal_conf_files,
-        mock_make_or_verify_dir):
+                                mock_make_or_verify_dir):
         mock_renewal_conf_files.return_value = ["badfile"]
         mock_renewable_cert.side_effect = errors.CertStorageError
         from certbot import cert_manager
         # pylint: disable=protected-access
-        self.assertEqual(cert_manager._search_lineages(self.config, lambda x: x, "check"),
-            "check")
+        self.assertEqual(cert_manager._search_lineages(self.config, lambda x: x, "check"), "check")
         self.assertTrue(mock_make_or_verify_dir.called)
 
 
@@ -294,33 +293,28 @@ class LineageForCertnameTest(BaseCertManagerTest):
     @mock.patch('certbot.storage.renewal_file_for_certname')
     @mock.patch('certbot.storage.RenewableCert')
     def test_found_match(self, mock_renewable_cert, mock_renewal_conf_file,
-        mock_make_or_verify_dir):
+                         mock_make_or_verify_dir):
         mock_renewal_conf_file.return_value = "somefile.conf"
         mock_match = mock.Mock(lineagename="example.com")
         mock_renewable_cert.return_value = mock_match
         from certbot import cert_manager
-        self.assertEqual(cert_manager.lineage_for_certname(self.config, "example.com"),
-            mock_match)
+        self.assertEqual(cert_manager.lineage_for_certname(self.config, "example.com"), mock_match)
         self.assertTrue(mock_make_or_verify_dir.called)
 
     @mock.patch('certbot.util.make_or_verify_dir')
     @mock.patch('certbot.storage.renewal_file_for_certname')
-    def test_no_match(self, mock_renewal_conf_file,
-        mock_make_or_verify_dir):
+    def test_no_match(self, mock_renewal_conf_file, mock_make_or_verify_dir):
         mock_renewal_conf_file.return_value = "other.com.conf"
         from certbot import cert_manager
-        self.assertEqual(cert_manager.lineage_for_certname(self.config, "example.com"),
-            None)
+        self.assertEqual(cert_manager.lineage_for_certname(self.config, "example.com"), None)
         self.assertTrue(mock_make_or_verify_dir.called)
 
     @mock.patch('certbot.util.make_or_verify_dir')
     @mock.patch('certbot.storage.renewal_file_for_certname')
-    def test_no_renewal_file(self, mock_renewal_conf_file,
-        mock_make_or_verify_dir):
+    def test_no_renewal_file(self, mock_renewal_conf_file, mock_make_or_verify_dir):
         mock_renewal_conf_file.side_effect = errors.CertStorageError()
         from certbot import cert_manager
-        self.assertEqual(cert_manager.lineage_for_certname(self.config, "example.com"),
-            None)
+        self.assertEqual(cert_manager.lineage_for_certname(self.config, "example.com"), None)
         self.assertTrue(mock_make_or_verify_dir.called)
 
 
@@ -331,7 +325,7 @@ class DomainsForCertnameTest(BaseCertManagerTest):
     @mock.patch('certbot.storage.renewal_file_for_certname')
     @mock.patch('certbot.storage.RenewableCert')
     def test_found_match(self, mock_renewable_cert, mock_renewal_conf_file,
-        mock_make_or_verify_dir):
+                         mock_make_or_verify_dir):
         mock_renewal_conf_file.return_value = "somefile.conf"
         mock_match = mock.Mock(lineagename="example.com")
         domains = ["example.com", "example.org"]
@@ -344,12 +338,10 @@ class DomainsForCertnameTest(BaseCertManagerTest):
 
     @mock.patch('certbot.util.make_or_verify_dir')
     @mock.patch('certbot.storage.renewal_file_for_certname')
-    def test_no_match(self, mock_renewal_conf_file,
-        mock_make_or_verify_dir):
+    def test_no_match(self, mock_renewal_conf_file, mock_make_or_verify_dir):
         mock_renewal_conf_file.return_value = "somefile.conf"
         from certbot import cert_manager
-        self.assertEqual(cert_manager.domains_for_certname(self.config, "other.com"),
-            None)
+        self.assertEqual(cert_manager.domains_for_certname(self.config, "other.com"), None)
         self.assertTrue(mock_make_or_verify_dir.called)
 
 

certbot/tests/compat/filesystem_test.py

@@ -1,4 +1,5 @@
 """Tests for certbot.compat.filesystem"""
+import contextlib
 import errno
 import unittest
 
@@ -16,6 +17,7 @@ except ImportError:
 
 import certbot.tests.util as test_util
 from certbot import lock
+from certbot import util
 from certbot.compat import os
 from certbot.compat import filesystem
 from certbot.tests.util import TempDirTestCase
@@ -306,6 +308,52 @@ class CopyOwnershipTest(test_util.TempDirTestCase):
         mock_chmod.assert_called_once_with(self.probe_path, 0o700)
 
 
+class CheckPermissionsTest(test_util.TempDirTestCase):
+    def setUp(self):
+        super(CheckPermissionsTest, self).setUp()
+        self.probe_path = _create_probe(self.tempdir)
+
+    def test_check_mode(self):
+        self.assertTrue(filesystem.check_mode(self.probe_path, 0o744))
+
+        filesystem.chmod(self.probe_path, 0o700)
+        self.assertFalse(filesystem.check_mode(self.probe_path, 0o744))
+
+    @unittest.skipIf(POSIX_MODE, reason='Test specific to Windows security')
+    def test_check_owner_windows(self):
+        self.assertTrue(filesystem.check_owner(self.probe_path))
+
+        system = win32security.ConvertStringSidToSid(SYSTEM_SID)
+        security = win32security.SECURITY_ATTRIBUTES().SECURITY_DESCRIPTOR
+        security.SetSecurityDescriptorOwner(system, False)
+
+        with mock.patch('win32security.GetFileSecurity') as mock_get:
+            mock_get.return_value = security
+            self.assertFalse(filesystem.check_owner(self.probe_path))
+
+    @unittest.skipUnless(POSIX_MODE, reason='Test specific to Linux security')
+    def test_check_owner_linux(self):
+        self.assertTrue(filesystem.check_owner(self.probe_path))
+
+        import os as std_os  # pylint: disable=os-module-forbidden
+        uid = std_os.getuid()
+
+        with mock.patch('os.getuid') as mock_uid:
+            mock_uid.return_value = uid + 1
+            self.assertFalse(filesystem.check_owner(self.probe_path))
+
+    def test_check_permissions(self):
+        self.assertTrue(filesystem.check_permissions(self.probe_path, 0o744))
+
+        with mock.patch('certbot.compat.filesystem.check_mode') as mock_mode:
+            mock_mode.return_value = False
+            self.assertFalse(filesystem.check_permissions(self.probe_path, 0o744))
+
+        with mock.patch('certbot.compat.filesystem.check_owner') as mock_owner:
+            mock_owner.return_value = False
+            self.assertFalse(filesystem.check_permissions(self.probe_path, 0o744))
+
+
 class OsReplaceTest(test_util.TempDirTestCase):
     """Test to ensure consistent behavior of rename method"""
     def test_os_replace_to_existing_file(self):
@@ -329,6 +377,8 @@ class RealpathTest(test_util.TempDirTestCase):
         self.probe_path = _create_probe(self.tempdir)
 
     def test_symlink_resolution(self):
+        # Remove any symlinks already in probe_path
+        self.probe_path = filesystem.realpath(self.probe_path)
         # Absolute resolution
         link_path = os.path.join(self.tempdir, 'link_abs')
         os.symlink(self.probe_path, link_path)
@@ -362,6 +412,70 @@ class RealpathTest(test_util.TempDirTestCase):
         self.assertTrue('link1 is a loop!' in str(error.exception))
 
 
+class IsExecutableTest(test_util.TempDirTestCase):
+    """Tests for is_executable method"""
+    def test_not_executable(self):
+        file_path = os.path.join(self.tempdir, "foo")
+
+        # On Windows a file created within Certbot will always have all permissions to the
+        # Administrators group set. Since the unit tests are typically executed under elevated
+        # privileges, it means that current user will always have effective execute rights on the
+        # hook script, and so the test will fail. To prevent that and represent a file created
+        # outside Certbot as typically a hook file is, we mock the _generate_dacl function in
+        # certbot.compat.filesystem to give rights only to the current user. This implies removing
+        # all ACEs except the first one from the DACL created by original _generate_dacl function.
+
+        from certbot.compat.filesystem import _generate_dacl
+
+        def _execute_mock(user_sid, mode):
+            dacl = _generate_dacl(user_sid, mode)
+            for _ in range(1, dacl.GetAceCount()):
+                dacl.DeleteAce(1)  # DeleteAce dynamically updates the internal index mapping.
+            return dacl
+
+        # create a non-executable file
+        with mock.patch("certbot.compat.filesystem._generate_dacl", side_effect=_execute_mock):
+            os.close(filesystem.open(file_path, os.O_CREAT | os.O_WRONLY, 0o666))
+
+        self.assertFalse(filesystem.is_executable(file_path))
+
+    @mock.patch("certbot.compat.filesystem.os.path.isfile")
+    @mock.patch("certbot.compat.filesystem.os.access")
+    def test_full_path(self, mock_access, mock_isfile):
+        with _fix_windows_runtime():
+            mock_access.return_value = True
+            mock_isfile.return_value = True
+            self.assertTrue(filesystem.is_executable("/path/to/exe"))
+
+    @mock.patch("certbot.compat.filesystem.os.path.isfile")
+    @mock.patch("certbot.compat.filesystem.os.access")
+    def test_rel_path(self, mock_access, mock_isfile):
+        with _fix_windows_runtime():
+            mock_access.return_value = True
+            mock_isfile.return_value = True
+            self.assertTrue(filesystem.is_executable("exe"))
+
+    @mock.patch("certbot.compat.filesystem.os.path.isfile")
+    @mock.patch("certbot.compat.filesystem.os.access")
+    def test_not_found(self, mock_access, mock_isfile):
+        with _fix_windows_runtime():
+            mock_access.return_value = True
+            mock_isfile.return_value = False
+            self.assertFalse(filesystem.is_executable("exe"))
+
+
+@contextlib.contextmanager
+def _fix_windows_runtime():
+    if os.name != 'nt':
+        yield
+    else:
+        with mock.patch('win32security.GetFileSecurity') as mock_get:
+            dacl_mock = mock_get.return_value.GetSecurityDescriptorDacl
+            mode_mock = dacl_mock.return_value.GetEffectiveRightsFromAcl
+            mode_mock.return_value = ntsecuritycon.FILE_GENERIC_EXECUTE
+            yield
+
+
 def _get_security_dacl(target):
     return win32security.GetFileSecurity(target, win32security.DACL_SECURITY_INFORMATION)
 
@@ -379,7 +493,7 @@ def _set_owner(target, security_owner, user):
 def _create_probe(tempdir):
     filesystem.chmod(tempdir, 0o744)
     probe_path = os.path.join(tempdir, 'probe')
-    open(probe_path, 'w').close()
+    util.safe_open(probe_path, 'w', chmod=0o744).close()
     return probe_path
 
 

certbot/tests/compat/os_test.py

@@ -8,7 +8,8 @@ class OsTest(unittest.TestCase):
     """Unit tests for os module."""
     def test_forbidden_methods(self):
         # Checks for os module
-        for method in ['chmod', 'chown', 'open', 'mkdir', 'makedirs', 'rename', 'replace']:
+        for method in ['chmod', 'chown', 'open', 'mkdir',
+                       'makedirs', 'rename', 'replace', 'access']:
             self.assertRaises(RuntimeError, getattr(os, method))
         # Checks for os.path module
         for method in ['realpath']:

certbot/tests/crypto_util_test.py

@@ -11,6 +11,7 @@ from certbot import errors
 from certbot import interfaces
 from certbot import util
 from certbot.compat import os
+from certbot.compat import filesystem
 
 RSA256_KEY = test_util.load_vector('rsa256_key.pem')
 RSA256_KEY_PATH = test_util.vector_path('rsa256_key.pem')
@@ -29,6 +30,9 @@ class InitSaveKeyTest(test_util.TempDirTestCase):
     def setUp(self):
         super(InitSaveKeyTest, self).setUp()
 
+        self.workdir = os.path.join(self.tempdir, 'workdir')
+        filesystem.mkdir(self.workdir, mode=0o700)
+
         logging.disable(logging.CRITICAL)
         zope.component.provideUtility(
             mock.Mock(strict_permissions=True), interfaces.IConfig)
@@ -46,15 +50,15 @@ class InitSaveKeyTest(test_util.TempDirTestCase):
     @mock.patch('certbot.crypto_util.make_key')
     def test_success(self, mock_make):
         mock_make.return_value = b'key_pem'
-        key = self._call(1024, self.tempdir)
+        key = self._call(1024, self.workdir)
         self.assertEqual(key.pem, b'key_pem')
         self.assertTrue('key-certbot.pem' in key.file)
-        self.assertTrue(os.path.exists(os.path.join(self.tempdir, key.file)))
+        self.assertTrue(os.path.exists(os.path.join(self.workdir, key.file)))
 
     @mock.patch('certbot.crypto_util.make_key')
     def test_key_failure(self, mock_make):
         mock_make.side_effect = ValueError
-        self.assertRaises(ValueError, self._call, 431, self.tempdir)
+        self.assertRaises(ValueError, self._call, 431, self.workdir)
 
 
 class InitSaveCSRTest(test_util.TempDirTestCase):

certbot/tests/hook_test.py

@@ -1,14 +1,14 @@
 """Tests for certbot.hooks."""
-import stat
 import unittest
 
 import mock
 from acme.magic_typing import List  # pylint: disable=unused-import, no-name-in-module
 
 from certbot import errors
+from certbot import util
 from certbot.compat import os
 from certbot.compat import filesystem
-from certbot.tests import util
+from certbot.tests import util as test_util
 
 
 class ValidateHooksTest(unittest.TestCase):
@@ -30,7 +30,7 @@ class ValidateHooksTest(unittest.TestCase):
         self.assertEqual("renew", types[-1])
 
 
-class ValidateHookTest(util.TempDirTestCase):
+class ValidateHookTest(test_util.TempDirTestCase):
     """Tests for certbot.hooks.validate_hook."""
 
     @classmethod
@@ -38,22 +38,20 @@ class ValidateHookTest(util.TempDirTestCase):
         from certbot.hooks import validate_hook
         return validate_hook(*args, **kwargs)
 
-    @util.broken_on_windows
-    def test_not_executable(self):
-        file_path = os.path.join(self.tempdir, "foo")
-        # create a non-executable file
-        os.close(filesystem.open(file_path, os.O_CREAT | os.O_WRONLY, 0o666))
+    def test_hook_not_executable(self):
         # prevent unnecessary modifications to PATH
         with mock.patch("certbot.hooks.plug_util.path_surgery"):
-            self.assertRaises(errors.HookCommandNotFound,
-                              self._call, file_path, "foo")
+            # We just mock out filesystem.is_executable since on Windows, it is difficult
+            # to get a fully working test around executable permissions. See
+            # certbot.tests.compat.filesystem::NotExecutableTest for more in-depth tests.
+            with mock.patch("certbot.hooks.filesystem.is_executable", return_value=False):
+                self.assertRaises(errors.HookCommandNotFound, self._call, 'dummy', "foo")
 
     @mock.patch("certbot.hooks.util.exe_exists")
     def test_not_found(self, mock_exe_exists):
         mock_exe_exists.return_value = False
         with mock.patch("certbot.hooks.plug_util.path_surgery") as mock_ps:
-            self.assertRaises(errors.HookCommandNotFound,
-                              self._call, "foo", "bar")
+            self.assertRaises(errors.HookCommandNotFound, self._call, "foo", "bar")
         self.assertTrue(mock_ps.called)
 
     @mock.patch("certbot.hooks._prog")
@@ -62,7 +60,7 @@ class ValidateHookTest(util.TempDirTestCase):
         self.assertFalse(mock_prog.called)
 
 
-class HookTest(util.ConfigTestCase):
+class HookTest(test_util.ConfigTestCase):
     """Common base class for hook tests."""
 
     @classmethod
@@ -454,7 +452,7 @@ class ExecuteTest(unittest.TestCase):
             self.assertTrue(mock_logger.error.called)
 
 
-class ListHooksTest(util.TempDirTestCase):
+class ListHooksTest(test_util.TempDirTestCase):
     """Tests for certbot.hooks.list_hooks."""
 
     @classmethod
@@ -494,8 +492,7 @@ def create_hook(file_path):
     :param str file_path: path to create the file at
 
     """
-    open(file_path, "w").close()
-    filesystem.chmod(file_path, os.stat(file_path).st_mode | stat.S_IXUSR)
+    util.safe_open(file_path, mode="w", chmod=0o744).close()
 
 
 if __name__ == '__main__':

certbot/tests/log_test.py

@@ -14,7 +14,7 @@ from acme.magic_typing import Optional  # pylint: disable=unused-import, no-name
 from certbot import constants
 from certbot import errors
 from certbot import util
-from certbot.compat import misc
+from certbot.compat import filesystem
 from certbot.compat import os
 from certbot.tests import util as test_util
 
@@ -260,8 +260,7 @@ class TempHandlerTest(unittest.TestCase):
         self.handler.close()
 
     def test_permissions(self):
-        self.assertTrue(
-            util.check_permissions(self.handler.path, 0o600, misc.os_geteuid()))
+        self.assertTrue(filesystem.check_permissions(self.handler.path, 0o600))
 
     def test_delete(self):
         self.handler.close()

certbot/tests/main_test.py

@@ -31,7 +31,6 @@ from certbot import interfaces  # pylint: disable=unused-import
 from certbot import main
 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
@@ -809,9 +808,9 @@ class MainTest(test_util.ConfigTestCase):  # pylint: disable=too-many-public-met
         ifaces = []  # type: List[interfaces.IPlugin]
         plugins = mock_disco.PluginsRegistry.find_all()
 
-        def throw_error(directory, mode, uid, strict):
+        def throw_error(directory, mode, strict):
             """Raises error.Error."""
-            _, _, _, _ = directory, mode, uid, strict
+            _, _, _ = directory, mode, strict
             raise errors.Error()
 
         stdout = six.StringIO()
@@ -1593,7 +1592,7 @@ class MakeOrVerifyNeededDirs(test_util.ConfigTestCase):
         for core_dir in (self.config.config_dir, self.config.work_dir,):
             mock_util.set_up_core_dir.assert_any_call(
                 core_dir, constants.CONFIG_DIRS_MODE,
-                misc.os_geteuid(), self.config.strict_permissions
+                self.config.strict_permissions
             )
 
         hook_dirs = (self.config.renewal_pre_hooks_dir,
@@ -1602,8 +1601,7 @@ class MakeOrVerifyNeededDirs(test_util.ConfigTestCase):
         for hook_dir in hook_dirs:
             # default mode of 755 is used
             mock_util.make_or_verify_dir.assert_any_call(
-                hook_dir, uid=misc.os_geteuid(),
-                strict=self.config.strict_permissions)
+                hook_dir, strict=self.config.strict_permissions)
 
 
 class EnhanceTest(test_util.ConfigTestCase):

certbot/tests/storage_test.py

@@ -13,7 +13,6 @@ import six
 import certbot
 import certbot.tests.util as test_util
 from certbot import errors
-from certbot.compat import misc
 from certbot.compat import os
 from certbot.compat import filesystem
 from certbot.storage import ALL_FOUR
@@ -21,7 +20,6 @@ from certbot.storage import ALL_FOUR
 CERT = test_util.load_cert('cert_512.pem')
 
 
-
 def unlink_all(rc_object):
     """Unlink all four items associated with this RenewableCert."""
     for kind in ALL_FOUR:
@@ -498,7 +496,6 @@ class RenewableCertTests(BaseRenewableCertTest):
         self.assertTrue(self.test_rc.should_autorenew())
         mock_ocsp.return_value = False
 
-    @test_util.broken_on_windows
     @mock.patch("certbot.storage.relevant_values")
     def test_save_successor(self, mock_rv):
         # Mock relevant_values() to claim that all values are relevant here
@@ -562,7 +559,7 @@ class RenewableCertTests(BaseRenewableCertTest):
         self.assertFalse(os.path.islink(self.test_rc.version("privkey", 10)))
         self.assertFalse(os.path.exists(temp_config_file))
 
-    @test_util.broken_on_windows
+    @test_util.skip_on_windows('Group/everybody permissions are not maintained on Windows.')
     @mock.patch("certbot.storage.relevant_values")
     def test_save_successor_maintains_group_mode(self, mock_rv):
         # Mock relevant_values() to claim that all values are relevant here
@@ -571,22 +568,18 @@ class RenewableCertTests(BaseRenewableCertTest):
         for kind in ALL_FOUR:
             self._write_out_kind(kind, 1)
         self.test_rc.update_all_links_to(1)
-        self.assertTrue(misc.compare_file_modes(
-            os.stat(self.test_rc.version("privkey", 1)).st_mode, 0o600))
+        self.assertTrue(filesystem.check_mode(self.test_rc.version("privkey", 1), 0o600))
         filesystem.chmod(self.test_rc.version("privkey", 1), 0o444)
         # If no new key, permissions should be the same (we didn't write any keys)
         self.test_rc.save_successor(1, b"newcert", None, b"new chain", self.config)
-        self.assertTrue(misc.compare_file_modes(
-            os.stat(self.test_rc.version("privkey", 2)).st_mode, 0o444))
+        self.assertTrue(filesystem.check_mode(self.test_rc.version("privkey", 2), 0o444))
         # If new key, permissions should be kept as 644
         self.test_rc.save_successor(2, b"newcert", b"new_privkey", b"new chain", self.config)
-        self.assertTrue(misc.compare_file_modes(
-            os.stat(self.test_rc.version("privkey", 3)).st_mode, 0o644))
+        self.assertTrue(filesystem.check_mode(self.test_rc.version("privkey", 3), 0o644))
         # If permissions reverted, next renewal will also revert permissions of new key
         filesystem.chmod(self.test_rc.version("privkey", 3), 0o400)
         self.test_rc.save_successor(3, b"newcert", b"new_privkey", b"new chain", self.config)
-        self.assertTrue(misc.compare_file_modes(
-            os.stat(self.test_rc.version("privkey", 4)).st_mode, 0o600))
+        self.assertTrue(filesystem.check_mode(self.test_rc.version("privkey", 4), 0o600))
 
     @mock.patch("certbot.storage.relevant_values")
     @mock.patch("certbot.storage.filesystem.copy_ownership_and_apply_mode")
@@ -622,7 +615,7 @@ class RenewableCertTests(BaseRenewableCertTest):
             self.config.live_dir, "README")))
         self.assertTrue(os.path.exists(os.path.join(
             self.config.live_dir, "the-lineage.com", "README")))
-        self.assertTrue(misc.compare_file_modes(os.stat(result.key_path).st_mode, 0o600))
+        self.assertTrue(filesystem.check_mode(result.key_path, 0o600))
         with open(result.fullchain, "rb") as f:
             self.assertEqual(f.read(), b"cert" + b"chain")
         # Let's do it again and make sure it makes a different lineage

certbot/tests/util.py

@@ -421,15 +421,6 @@ def skip_on_windows(reason):
     return wrapper
 
 
-def broken_on_windows(function):
-    """Decorator to skip temporarily a broken test on Windows."""
-    reason = 'Test is broken and ignored on windows but should be fixed.'
-    return unittest.skipIf(
-        sys.platform == 'win32'
-        and os.environ.get('SKIP_BROKEN_TESTS_ON_WINDOWS', 'true') == 'true',
-        reason)(function)
-
-
 def temp_join(path):
     """
     Return the given path joined to the tempdir path for the current platform

certbot/tests/util_test.py

@@ -9,7 +9,6 @@ from six.moves import reload_module  # pylint: disable=import-error
 
 import certbot.tests.util as test_util
 from certbot import errors
-from certbot.compat import misc
 from certbot.compat import os
 from certbot.compat import filesystem
 
@@ -53,26 +52,13 @@ class ExeExistsTest(unittest.TestCase):
         from certbot.util import exe_exists
         return exe_exists(exe)
 
-    @mock.patch("certbot.util.os.path.isfile")
-    @mock.patch("certbot.util.os.access")
-    def test_full_path(self, mock_access, mock_isfile):
-        mock_access.return_value = True
-        mock_isfile.return_value = True
-        self.assertTrue(self._call("/path/to/exe"))
+    def test_exe_exists(self):
+        with mock.patch("certbot.util.filesystem.is_executable", return_value=True):
+            self.assertTrue(self._call("/path/to/exe"))
 
-    @mock.patch("certbot.util.os.path.isfile")
-    @mock.patch("certbot.util.os.access")
-    def test_on_path(self, mock_access, mock_isfile):
-        mock_access.return_value = True
-        mock_isfile.return_value = True
-        self.assertTrue(self._call("exe"))
-
-    @mock.patch("certbot.util.os.path.isfile")
-    @mock.patch("certbot.util.os.access")
-    def test_not_found(self, mock_access, mock_isfile):
-        mock_access.return_value = False
-        mock_isfile.return_value = True
-        self.assertFalse(self._call("exe"))
+    def test_exe_not_exists(self):
+        with mock.patch("certbot.util.filesystem.is_executable", return_value=False):
+            self.assertFalse(self._call("/path/to/exe"))
 
 
 class LockDirUntilExit(test_util.TempDirTestCase):
@@ -120,15 +106,14 @@ class SetUpCoreDirTest(test_util.TempDirTestCase):
     @mock.patch('certbot.util.lock_dir_until_exit')
     def test_success(self, mock_lock):
         new_dir = os.path.join(self.tempdir, 'new')
-        self._call(new_dir, 0o700, misc.os_geteuid(), False)
+        self._call(new_dir, 0o700, False)
         self.assertTrue(os.path.exists(new_dir))
         self.assertEqual(mock_lock.call_count, 1)
 
     @mock.patch('certbot.util.make_or_verify_dir')
     def test_failure(self, mock_make_or_verify):
         mock_make_or_verify.side_effect = OSError
-        self.assertRaises(errors.Error, self._call,
-                          self.tempdir, 0o700, misc.os_geteuid(), False)
+        self.assertRaises(errors.Error, self._call, self.tempdir, 0o700, False)
 
 
 class MakeOrVerifyDirTest(test_util.TempDirTestCase):
@@ -145,23 +130,20 @@ class MakeOrVerifyDirTest(test_util.TempDirTestCase):
         self.path = os.path.join(self.tempdir, "foo")
         filesystem.mkdir(self.path, 0o600)
 
-        self.uid = misc.os_geteuid()
-
     def _call(self, directory, mode):
         from certbot.util import make_or_verify_dir
-        return make_or_verify_dir(directory, mode, self.uid, strict=True)
+        return make_or_verify_dir(directory, mode, strict=True)
 
     def test_creates_dir_when_missing(self):
         path = os.path.join(self.tempdir, "bar")
         self._call(path, 0o650)
         self.assertTrue(os.path.isdir(path))
-        self.assertTrue(misc.compare_file_modes(os.stat(path).st_mode, 0o650))
+        self.assertTrue(filesystem.check_mode(path, 0o650))
 
     def test_existing_correct_mode_does_not_fail(self):
         self._call(self.path, 0o600)
-        self.assertTrue(misc.compare_file_modes(os.stat(self.path).st_mode, 0o600))
+        self.assertTrue(filesystem.check_mode(self.path, 0o600))
 
-    @test_util.skip_on_windows('Umask modes are mostly ignored on Windows.')
     def test_existing_wrong_mode_fails(self):
         self.assertRaises(errors.Error, self._call, self.path, 0o400)
 
@@ -171,39 +153,6 @@ class MakeOrVerifyDirTest(test_util.TempDirTestCase):
             self.assertRaises(OSError, self._call, "bar", 12312312)
 
 
-class CheckPermissionsTest(test_util.TempDirTestCase):
-    """Tests for certbot.util.check_permissions.
-
-    Note that it is not possible to test for a wrong file owner,
-    as this testing script would have to be run as root.
-
-    """
-
-    def setUp(self):
-        super(CheckPermissionsTest, self).setUp()
-
-        self.uid = misc.os_geteuid()
-
-    def _call(self, mode):
-        from certbot.util import check_permissions
-        return check_permissions(self.tempdir, mode, self.uid)
-
-    def test_ok_mode(self):
-        filesystem.chmod(self.tempdir, 0o600)
-        self.assertTrue(self._call(0o600))
-
-    # TODO: reactivate the test when all logic from windows file permissions is merged.
-    @test_util.broken_on_windows
-    def test_wrong_mode(self):
-        filesystem.chmod(self.tempdir, 0o400)
-        try:
-            self.assertFalse(self._call(0o600))
-        finally:
-            # Without proper write permissions, Windows is unable to delete a folder,
-            # even with admin permissions. Write access must be explicitly set first.
-            filesystem.chmod(self.tempdir, 0o700)
-
-
 class UniqueFileTest(test_util.TempDirTestCase):
     """Tests for certbot.util.unique_file."""
 
@@ -226,8 +175,8 @@ class UniqueFileTest(test_util.TempDirTestCase):
     def test_right_mode(self):
         fd1, name1 = self._call(0o700)
         fd2, name2 = self._call(0o600)
-        self.assertTrue(misc.compare_file_modes(0o700, os.stat(name1).st_mode))
-        self.assertTrue(misc.compare_file_modes(0o600, os.stat(name2).st_mode))
+        self.assertTrue(filesystem.check_mode(name1, 0o700))
+        self.assertTrue(filesystem.check_mode(name2, 0o600))
         fd1.close()
         fd2.close()
 

certbot/util.py

@@ -21,7 +21,6 @@ from acme.magic_typing import Tuple, Union  # pylint: disable=unused-import, no-
 from certbot import constants
 from certbot import errors
 from certbot import lock
-from certbot.compat import misc
 from certbot.compat import os
 from certbot.compat import filesystem
 
@@ -87,18 +86,6 @@ def run_script(params, log=logger.error):
     return stdout, stderr
 
 
-def is_exe(path):
-    """Is path an executable file?
-
-    :param str path: path to test
-
-    :returns: True iff path is an executable file
-    :rtype: bool
-
-    """
-    return os.path.isfile(path) and os.access(path, os.X_OK)
-
-
 def exe_exists(exe):
     """Determine whether path/name refers to an executable.
 
@@ -110,10 +97,10 @@ def exe_exists(exe):
     """
     path, _ = os.path.split(exe)
     if path:
-        return is_exe(exe)
+        return filesystem.is_executable(exe)
     else:
         for path in os.environ["PATH"].split(os.pathsep):
-            if is_exe(os.path.join(path, exe)):
+            if filesystem.is_executable(os.path.join(path, exe)):
                 return True
 
     return False
@@ -144,12 +131,11 @@ def _release_locks():
     _LOCKS.clear()
 
 
-def set_up_core_dir(directory, mode, uid, strict):
+def set_up_core_dir(directory, mode, strict):
     """Ensure directory exists with proper permissions and is locked.
 
     :param str directory: Path to a directory.
     :param int mode: Directory mode.
-    :param int uid: Directory owner.
     :param bool strict: require directory to be owned by current user
 
     :raises .errors.LockError: if the directory cannot be locked
@@ -157,19 +143,18 @@ def set_up_core_dir(directory, mode, uid, strict):
 
     """
     try:
-        make_or_verify_dir(directory, mode, uid, strict)
+        make_or_verify_dir(directory, mode, strict)
         lock_dir_until_exit(directory)
     except OSError as error:
         logger.debug("Exception was:", exc_info=True)
         raise errors.Error(PERM_ERR_FMT.format(error))
 
 
-def make_or_verify_dir(directory, mode=0o755, uid=0, strict=False):
+def make_or_verify_dir(directory, mode=0o755, strict=False):
     """Make sure directory exists with proper permissions.
 
     :param str directory: Path to a directory.
     :param int mode: Directory mode.
-    :param int uid: Directory owner.
     :param bool strict: require directory to be owned by current user
 
     :raises .errors.Error: if a directory already exists,
@@ -184,29 +169,14 @@ def make_or_verify_dir(directory, mode=0o755, uid=0, strict=False):
         filesystem.makedirs(directory, mode)
     except OSError as exception:
         if exception.errno == errno.EEXIST:
-            if strict and not check_permissions(directory, mode, uid):
+            if strict and not filesystem.check_permissions(directory, mode):
                 raise errors.Error(
-                    "%s exists, but it should be owned by user %d with"
-                    "permissions %s" % (directory, uid, oct(mode)))
+                    "%s exists, but it should be owned by current user with"
+                    " permissions %s" % (directory, oct(mode)))
         else:
             raise
 
 
-def check_permissions(filepath, mode, uid=0):
-    """Check file or directory permissions.
-
-    :param str filepath: Path to the tested file (or directory).
-    :param int mode: Expected file mode.
-    :param int uid: Expected file owner.
-
-    :returns: True if `mode` and `uid` match, False otherwise.
-    :rtype: bool
-
-    """
-    file_stat = os.stat(filepath)
-    return misc.compare_file_modes(file_stat.st_mode, mode) and file_stat.st_uid == uid
-
-
 def safe_open(path, mode="w", chmod=None):
     """Safely open a file.
 

docs/contributing.rst

@@ -176,7 +176,7 @@ that can be used once the virtual environment is activated:
 
 .. code-block:: shell
 
-    certbot_tests [ARGS...]
+    certbot_test [ARGS...]
 
 - Execute certbot with the provided arguments and other arguments useful for testing purposes,
   such as: verbose output, full tracebacks in case Certbot crashes, *etc.*

docs/install.rst

@@ -42,7 +42,7 @@ client as root, either `letsencrypt-nosudo
 The Apache plugin currently requires an OS with augeas version 1.0; currently `it
 supports
 <https://github.com/certbot/certbot/blob/master/certbot-apache/certbot_apache/constants.py>`_
-modern OSes based on Debian, Fedora, SUSE, Gentoo and Darwin.
+modern OSes based on Debian, Ubuntu, Fedora, SUSE, Gentoo and Darwin.
 
 
 Additional integrity verification of certbot-auto script can be done by verifying its digital signature.
@@ -218,6 +218,31 @@ repo, if you have not already done so. Then run:
 
    sudo apt-get install certbot python-certbot-apache -t jessie-backports
 
+**Ubuntu**
+
+If you run Ubuntu Trusty, Xenial, or Bionic, certbot is available through the official PPA,
+that can be installed as followed:
+
+.. code-block:: shell
+
+   sudo apt-get update
+   sudo apt-get install software-properties-common
+   sudo add-apt-repository universe
+   sudo add-apt-repository ppa:certbot/certbot
+   sudo apt-get update
+
+Then, certbot can be installed using:
+
+.. code-block:: shell
+
+   sudo apt-get install certbot
+
+Optionally to install the Certbot Apache plugin, you can use:
+
+.. code-block:: shell
+
+   sudo apt-get install python-certbot-apache
+
 **Fedora**
 
 .. code-block:: shell

tests/letstest/multitester.py

@@ -375,7 +375,7 @@ def cleanup(cl_args, instances, targetlist):
     # If lengths of instances and targetlist aren't equal, instances failed to
     # start before running tests so leaving instances running for debugging
     # isn't very useful. Let's cleanup after ourselves instead.
-    if len(instances) == len(targetlist) or not cl_args.saveinstances:
+    if len(instances) != len(targetlist) or not cl_args.saveinstances:
         print('Terminating EC2 Instances')
         if cl_args.killboulder:
             boulder_server.terminate()

tools/dev_constraints.txt

@@ -82,6 +82,6 @@ twine==1.11.0
 typed-ast==1.1.0
 typing==3.6.4
 uritemplate==0.6
-virtualenv==16.6.1
+virtualenv==16.6.2
 wcwidth==0.1.7
 wrapt==1.11.1