Merge remote-tracking branch 'origin/master' into more-ua

Dockerfile-dev

@@ -20,10 +20,10 @@ WORKDIR /opt/certbot/src
 # If <dest> doesn't exist, it is created along with all missing
 # directories in its path.
 
-# TODO: Install non-default Python versions for tox.
 # TODO: Install Apache/Nginx for plugin development.
 COPY letsencrypt-auto-source/letsencrypt-auto /opt/certbot/src/letsencrypt-auto-source/letsencrypt-auto
 RUN /opt/certbot/src/letsencrypt-auto-source/letsencrypt-auto --os-packages-only && \
+    apt-get install python3-dev -y && \
     apt-get clean && \
     rm -rf /var/lib/apt/lists/* \
            /tmp/* \
@@ -51,6 +51,8 @@ COPY certbot-compatibility-test /opt/certbot/src/certbot-compatibility-test/
 COPY tests /opt/certbot/src/tests/
 
 RUN virtualenv --no-site-packages -p python2 /opt/certbot/venv && \
+    /opt/certbot/venv/bin/pip install -U pip && \
+    /opt/certbot/venv/bin/pip install -U setuptools && \
     /opt/certbot/venv/bin/pip install \
     -e /opt/certbot/src/acme \
     -e /opt/certbot/src \
@@ -58,8 +60,7 @@ RUN virtualenv --no-site-packages -p python2 /opt/certbot/venv && \
     -e /opt/certbot/src/certbot-nginx \
     -e /opt/certbot/src/letshelp-certbot \
     -e /opt/certbot/src/certbot-compatibility-test \
-    -e /opt/certbot/src[dev,docs] && \
-    /opt/certbot/venv/bin/pip install -U setuptools
+    -e /opt/certbot/src[dev,docs]
 
 # install in editable mode (-e) to save space: it's not possible to
 # "rm -rf /opt/certbot/src" (it's stays in the underlaying image);

README.rst

@@ -129,19 +129,7 @@ email to client-dev+subscribe@letsencrypt.org)
 System Requirements
 ===================
 
-The Let's Encrypt Client presently only runs on Unix-ish OSes that include
-Python 2.6 or 2.7; Python 3.x support will hopefully be added in the future. The
-client requires root access in order to write to ``/etc/letsencrypt``,
-``/var/log/letsencrypt``, ``/var/lib/letsencrypt``; to bind to ports 80 and 443
-(if you use the ``standalone`` plugin) and to read and modify webserver
-configurations (if you use the ``apache`` or ``nginx`` plugins).  If none of
-these apply to you, it is theoretically possible to run without root privileges,
-but for most users who want to avoid running an ACME client as root, either
-`letsencrypt-nosudo <https://github.com/diafygi/letsencrypt-nosudo>`_ or
-`simp_le <https://github.com/kuba/simp_le>`_ are more appropriate choices.
-
-The Apache plugin currently requires a Debian-based OS with augeas version
-1.0; this includes Ubuntu 12.04+ and Debian 7+.
+See https://certbot.eff.org/docs/install.html#system-requirements.
 
 .. Do not modify this comment unless you know what you're doing. tag:intro-end
 

acme/acme/challenges.py

@@ -5,7 +5,7 @@ import hashlib
 import logging
 import socket
 
-from cryptography.hazmat.primitives import hashes
+from cryptography.hazmat.primitives import hashes  # type: ignore
 import OpenSSL
 import requests
 
@@ -23,7 +23,7 @@ logger = logging.getLogger(__name__)
 class Challenge(jose.TypedJSONObjectWithFields):
     # _fields_to_partial_json | pylint: disable=abstract-method
     """ACME challenge."""
-    TYPES = {}
+    TYPES = {}  # type: dict
 
     @classmethod
     def from_json(cls, jobj):
@@ -37,7 +37,7 @@ class Challenge(jose.TypedJSONObjectWithFields):
 class ChallengeResponse(jose.TypedJSONObjectWithFields):
     # _fields_to_partial_json | pylint: disable=abstract-method
     """ACME challenge response."""
-    TYPES = {}
+    TYPES = {}  # type: dict
     resource_type = 'challenge'
     resource = fields.Resource(resource_type)
 

acme/acme/client.py

@@ -28,11 +28,13 @@ logger = logging.getLogger(__name__)
 # https://urllib3.readthedocs.org/en/latest/security.html#insecureplatformwarning
 if sys.version_info < (2, 7, 9):  # pragma: no cover
     try:
-        requests.packages.urllib3.contrib.pyopenssl.inject_into_urllib3()
+        requests.packages.urllib3.contrib.pyopenssl.inject_into_urllib3()  # type: ignore
     except AttributeError:
         import urllib3.contrib.pyopenssl  # pylint: disable=import-error
         urllib3.contrib.pyopenssl.inject_into_urllib3()
 
+DEFAULT_NETWORK_TIMEOUT = 45
+
 DER_CONTENT_TYPE = 'application/pkix-cert'
 
 
@@ -503,13 +505,14 @@ class ClientNetwork(object):  # pylint: disable=too-many-instance-attributes
     REPLAY_NONCE_HEADER = 'Replay-Nonce'
 
     def __init__(self, key, alg=jose.RS256, verify_ssl=True,
-                 user_agent='acme-python'):
+                 user_agent='acme-python', timeout=DEFAULT_NETWORK_TIMEOUT):
         self.key = key
         self.alg = alg
         self.verify_ssl = verify_ssl
         self._nonces = set()
         self.user_agent = user_agent
         self.session = requests.Session()
+        self._default_timeout = timeout
 
     def __del__(self):
         self.session.close()
@@ -608,7 +611,7 @@ class ClientNetwork(object):  # pylint: disable=too-many-instance-attributes
         kwargs['verify'] = self.verify_ssl
         kwargs.setdefault('headers', {})
         kwargs['headers'].setdefault('User-Agent', self.user_agent)
-        kwargs.setdefault('timeout', 45) # timeout after 45 seconds
+        kwargs.setdefault('timeout', self._default_timeout)
         response = self.session.request(method, url, *args, **kwargs)
         # If content is DER, log the base64 of it instead of raw bytes, to keep
         # binary data out of the logs.

acme/acme/crypto_util.py

@@ -23,7 +23,7 @@ logger = logging.getLogger(__name__)
 # https://www.openssl.org/docs/ssl/SSLv23_method.html). _serve_sni
 # should be changed to use "set_options" to disable SSLv2 and SSLv3,
 # in case it's used for things other than probing/serving!
-_DEFAULT_TLSSNI01_SSL_METHOD = OpenSSL.SSL.SSLv23_METHOD
+_DEFAULT_TLSSNI01_SSL_METHOD = OpenSSL.SSL.SSLv23_METHOD  # type: ignore
 
 
 class SSLSocket(object):  # pylint: disable=too-few-public-methods

acme/acme/crypto_util_test.py

@@ -6,7 +6,7 @@ import time
 import unittest
 
 import six
-from six.moves import socketserver  # pylint: disable=import-error
+from six.moves import socketserver  #type: ignore  # pylint: disable=import-error
 
 import OpenSSL
 

acme/acme/jose/jwa.py

@@ -9,9 +9,9 @@ import logging
 
 import cryptography.exceptions
 from cryptography.hazmat.backends import default_backend
-from cryptography.hazmat.primitives import hashes
-from cryptography.hazmat.primitives import hmac
-from cryptography.hazmat.primitives.asymmetric import padding
+from cryptography.hazmat.primitives import hashes  # type: ignore
+from cryptography.hazmat.primitives import hmac  # type: ignore
+from cryptography.hazmat.primitives.asymmetric import padding  # type: ignore
 
 from acme.jose import errors
 from acme.jose import interfaces
@@ -28,9 +28,9 @@ class JWA(interfaces.JSONDeSerializable):  # pylint: disable=abstract-method
     """JSON Web Algorithm."""
 
 
-class JWASignature(JWA, collections.Hashable):
+class JWASignature(JWA, collections.Hashable):  # type: ignore
     """JSON Web Signature Algorithm."""
-    SIGNATURES = {}
+    SIGNATURES = {}  # type: dict
 
     def __init__(self, name):
         self.name = name

acme/acme/jose/jwk.py

@@ -6,9 +6,9 @@ import logging
 
 import cryptography.exceptions
 from cryptography.hazmat.backends import default_backend
-from cryptography.hazmat.primitives import hashes
+from cryptography.hazmat.primitives import hashes  # type: ignore
 from cryptography.hazmat.primitives import serialization
-from cryptography.hazmat.primitives.asymmetric import ec
+from cryptography.hazmat.primitives.asymmetric import ec  # type: ignore
 from cryptography.hazmat.primitives.asymmetric import rsa
 
 import six
@@ -25,8 +25,8 @@ class JWK(json_util.TypedJSONObjectWithFields):
     # pylint: disable=too-few-public-methods
     """JSON Web Key."""
     type_field_name = 'kty'
-    TYPES = {}
-    cryptography_key_types = ()
+    TYPES = {}  # type: dict
+    cryptography_key_types = ()  # type: tuple
     """Subclasses should override."""
 
     required = NotImplemented

acme/acme/jose/jws.py

@@ -121,12 +121,12 @@ class Header(json_util.JSONObjectWithFields):
 
     # x5c does NOT use JOSE Base64 (4.1.6)
 
-    @x5c.encoder
+    @x5c.encoder  # type: ignore
     def x5c(value):  # pylint: disable=missing-docstring,no-self-argument
         return [base64.b64encode(OpenSSL.crypto.dump_certificate(
             OpenSSL.crypto.FILETYPE_ASN1, cert.wrapped)) for cert in value]
 
-    @x5c.decoder
+    @x5c.decoder  # type: ignore
     def x5c(value):  # pylint: disable=missing-docstring,no-self-argument
         try:
             return tuple(util.ComparableX509(OpenSSL.crypto.load_certificate(
@@ -157,12 +157,12 @@ class Signature(json_util.JSONObjectWithFields):
         'signature', decoder=json_util.decode_b64jose,
         encoder=json_util.encode_b64jose)
 
-    @protected.encoder
+    @protected.encoder  # type: ignore
     def protected(value):  # pylint: disable=missing-docstring,no-self-argument
         # wrong type guess (Signature, not bytes) | pylint: disable=no-member
         return json_util.encode_b64jose(value.encode('utf-8'))
 
-    @protected.decoder
+    @protected.decoder  # type: ignore
     def protected(value):  # pylint: disable=missing-docstring,no-self-argument
         return json_util.decode_b64jose(value).decode('utf-8')
 

acme/acme/jose/util.py

@@ -134,7 +134,7 @@ class ComparableRSAKey(ComparableKey):  # pylint: disable=too-few-public-methods
             return hash((self.__class__, pub.n, pub.e))
 
 
-class ImmutableMap(collections.Mapping, collections.Hashable):
+class ImmutableMap(collections.Mapping, collections.Hashable):  # type: ignore
     # pylint: disable=too-few-public-methods
     """Immutable key to value mapping with attribute access."""
 
@@ -180,7 +180,7 @@ class ImmutableMap(collections.Mapping, collections.Hashable):
             for key, value in six.iteritems(self)))
 
 
-class frozendict(collections.Mapping, collections.Hashable):
+class frozendict(collections.Mapping, collections.Hashable):  # type: ignore
     # pylint: disable=invalid-name,too-few-public-methods
     """Frozen dictionary."""
     __slots__ = ('_items', '_keys')

acme/acme/messages.py

@@ -98,7 +98,7 @@ class Error(jose.JSONObjectWithFields, errors.Error):
             if part is not None)
 
 
-class _Constant(jose.JSONDeSerializable, collections.Hashable):
+class _Constant(jose.JSONDeSerializable, collections.Hashable):  # type: ignore
     """ACME constant."""
     __slots__ = ('name',)
     POSSIBLE_NAMES = NotImplemented
@@ -132,7 +132,7 @@ class _Constant(jose.JSONDeSerializable, collections.Hashable):
 
 class Status(_Constant):
     """ACME "status" field."""
-    POSSIBLE_NAMES = {}
+    POSSIBLE_NAMES = {}  # type: dict
 STATUS_UNKNOWN = Status('unknown')
 STATUS_PENDING = Status('pending')
 STATUS_PROCESSING = Status('processing')
@@ -143,7 +143,7 @@ STATUS_REVOKED = Status('revoked')
 
 class IdentifierType(_Constant):
     """ACME identifier type."""
-    POSSIBLE_NAMES = {}
+    POSSIBLE_NAMES = {}  # type: dict
 IDENTIFIER_FQDN = IdentifierType('dns')  # IdentifierDNS in Boulder
 
 
@@ -161,7 +161,7 @@ class Identifier(jose.JSONObjectWithFields):
 class Directory(jose.JSONDeSerializable):
     """Directory."""
 
-    _REGISTERED_TYPES = {}
+    _REGISTERED_TYPES = {}  # type: dict
 
     class Meta(jose.JSONObjectWithFields):
         """Directory Meta."""

acme/acme/standalone.py

@@ -6,9 +6,9 @@ import logging
 import os
 import sys
 
-from six.moves import BaseHTTPServer  # pylint: disable=import-error
+from six.moves import BaseHTTPServer  # type: ignore  # pylint: disable=import-error
 from six.moves import http_client  # pylint: disable=import-error
-from six.moves import socketserver  # pylint: disable=import-error
+from six.moves import socketserver  # type: ignore  # pylint: disable=import-error
 
 import OpenSSL
 

acme/acme/standalone_test.py

@@ -7,7 +7,7 @@ import time
 import unittest
 
 from six.moves import http_client  # pylint: disable=import-error
-from six.moves import socketserver  # pylint: disable=import-error
+from six.moves import socketserver  # type: ignore  # pylint: disable=import-error
 
 import requests
 

certbot-apache/certbot_apache/configurator.py

@@ -174,6 +174,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         # Set Version
         if self.version is None:
             self.version = self.get_version()
+            logger.debug('Apache version is %s',
+                         '.'.join(str(i) for i in self.version))
         if self.version < (2, 2):
             raise errors.NotSupportedError(
                 "Apache Version %s not supported.", str(self.version))
@@ -254,9 +256,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             raise errors.PluginError(
                 "Unable to find cert and/or key directives")
 
-        logger.info("Deploying Certificate to VirtualHost %s", vhost.filep)
-        logger.debug("Apache version is %s",
-                     ".".join(str(i) for i in self.version))
+        logger.info("Deploying Certificate for %s to VirtualHost %s", domain, vhost.filep)
 
         if self.version < (2, 4, 8) or (chain_path and not fullchain_path):
             # install SSLCertificateFile, SSLCertificateKeyFile,

certbot-apache/certbot_apache/parser.py

@@ -136,7 +136,8 @@ class ApacheParser(object):
             proc = subprocess.Popen(
                 constants.os_constant("define_cmd"),
                 stdout=subprocess.PIPE,
-                stderr=subprocess.PIPE)
+                stderr=subprocess.PIPE,
+                universal_newlines=True)
             stdout, stderr = proc.communicate()
 
         except (OSError, ValueError):

certbot-apache/setup.py

@@ -11,7 +11,7 @@ install_requires = [
     'acme=={0}'.format(version),
     'certbot=={0}'.format(version),
     'mock',
-    'python-augeas',
+    'python-augeas<=0.5.0',
     # For pkg_resources. >=1.0 so pip resolves it to a version cryptography
     # will tolerate; see #2599:
     'setuptools>=1.0',

certbot-compatibility-test/certbot_compatibility_test/interfaces.py

@@ -20,20 +20,20 @@ class IPluginProxy(zope.interface.Interface):
     def __init__(args):
         """Initializes the plugin with the given command line args"""
 
-    def cleanup_from_tests():
+    def cleanup_from_tests():  # type: ignore
         """Performs any necessary cleanup from running plugin tests.
 
         This is guaranteed to be called before the program exits.
 
         """
 
-    def has_more_configs():
+    def has_more_configs():  # type: ignore
         """Returns True if there are more configs to test"""
 
-    def load_config():
+    def load_config():  # type: ignore
         """Loads the next config and returns its name"""
 
-    def get_testable_domain_names():
+    def get_testable_domain_names():  # type: ignore
         """Returns the domain names that can be used in testing"""
 
 
@@ -44,7 +44,7 @@ class IAuthenticatorProxy(IPluginProxy, certbot.interfaces.IAuthenticator):
 class IInstallerProxy(IPluginProxy, certbot.interfaces.IInstaller):
     """Wraps a Certbot installer"""
 
-    def get_all_names_answer():
+    def get_all_names_answer():  # type: ignore
         """Returns all names that should be found by the installer"""
 
 

certbot-compatibility-test/nginx/nginx-roundtrip-testdata/simplepythonfcgi/weird-spacing.conf

@@ -1,16 +1,16 @@
  # static files
  location ~ ^/(images|javascript|js|css|flash|media|static)/ {
-   root ${PROJECTBASE}/${PROJECTNAME}/static;
+   root "${PROJECTBASE}/${PROJECTNAME}/static";
  }
 
  location = /favicon.ico {
-   root ${PROJECTBASE}/${PROJECTNAME}/static/images;
+   root "${PROJECTBASE}/${PROJECTNAME}/static/images";
  }
 
  # pass all requests to FastCGI TG server listening on ${HOST}:${PORT}
  #
  location / {
-   fastcgi_pass ${HOST}:${PORT};
+   fastcgi_pass "${HOST}:${PORT}";
    fastcgi_index index;
    fastcgi_param SCRIPT_FILENAME /scripts$fastcgi_script_name;
    include conf/fastcgi_params;

certbot-nginx/certbot_nginx/configurator.py

@@ -5,9 +5,11 @@ import re
 import shutil
 import socket
 import subprocess
+import tempfile
 import time
 
 import OpenSSL
+import six
 import zope.interface
 
 from acme import challenges
@@ -30,16 +32,16 @@ from certbot_nginx import parser
 logger = logging.getLogger(__name__)
 
 REDIRECT_BLOCK = [[
-    ['\n    ', 'if', ' ', '($scheme != "https") '],
-    [['\n        ', 'return', ' ', '301 https://$host$request_uri'],
+    ['\n    ', 'if', ' ', '($scheme', ' ', '!=', ' ', '"https") '],
+    [['\n        ', 'return', ' ', '301', ' ', 'https://$host$request_uri'],
      '\n    ']
 ], ['\n']]
 
 TEST_REDIRECT_BLOCK = [
     [
-        ['if', '($scheme != "https")'],
+        ['if', '($scheme', '!=', '"https")'],
         [
-            ['return', '301 https://$host$request_uri']
+            ['return', '301', 'https://$host$request_uri']
         ]
     ],
     ['#', ' managed by Certbot']
@@ -262,7 +264,7 @@ class NginxConfigurator(common.Plugin):
         """
         if not matches:
             return None
-        elif matches[0]['rank'] in xrange(2, 6):
+        elif matches[0]['rank'] in six.moves.range(2, 6):
             # Wildcard match - need to find the longest one
             rank = matches[0]['rank']
             wildcards = [x for x in matches if x['rank'] == rank]
@@ -818,7 +820,7 @@ class NginxConfigurator(common.Plugin):
             self.restart()
 
 
-def nginx_restart(nginx_ctl, nginx_conf="/etc/nginx.conf"):
+def nginx_restart(nginx_ctl, nginx_conf):
     """Restarts the Nginx Server.
 
     .. todo:: Nginx restart is fatal if the configuration references
@@ -829,22 +831,22 @@ def nginx_restart(nginx_ctl, nginx_conf="/etc/nginx.conf"):
 
     """
     try:
-        proc = subprocess.Popen([nginx_ctl, "-c", nginx_conf, "-s", "reload"],
-                                stdout=subprocess.PIPE,
-                                stderr=subprocess.PIPE)
-        stdout, stderr = proc.communicate()
+        proc = subprocess.Popen([nginx_ctl, "-c", nginx_conf, "-s", "reload"])
+        proc.communicate()
 
         if proc.returncode != 0:
             # Maybe Nginx isn't running
-            nginx_proc = subprocess.Popen([nginx_ctl, "-c", nginx_conf],
-                                          stdout=subprocess.PIPE,
-                                          stderr=subprocess.PIPE)
-            stdout, stderr = nginx_proc.communicate()
-
-            if nginx_proc.returncode != 0:
-                # Enter recovery routine...
-                raise errors.MisconfigurationError(
-                    "nginx restart failed:\n%s\n%s" % (stdout, stderr))
+            # Write to temporary files instead of piping because of communication issues on Arch
+            # https://github.com/certbot/certbot/issues/4324
+            with tempfile.TemporaryFile() as out:
+                with tempfile.TemporaryFile() as err:
+                    nginx_proc = subprocess.Popen([nginx_ctl, "-c", nginx_conf],
+                        stdout=out, stderr=err)
+                    nginx_proc.communicate()
+                    if nginx_proc.returncode != 0:
+                        # Enter recovery routine...
+                        raise errors.MisconfigurationError(
+                            "nginx restart failed:\n%s\n%s" % (out.read(), err.read()))
 
     except (OSError, ValueError):
         raise errors.MisconfigurationError("nginx restart failed")

certbot-nginx/certbot_nginx/nginxparser.py

@@ -2,11 +2,9 @@
 # Forked from https://github.com/fatiherikli/nginxparser (MIT Licensed)
 import copy
 import logging
-import string
 
 from pyparsing import (
-    Literal, White, Word, alphanums, CharsNotIn, Combine, Forward, Group,
-    Optional, OneOrMore, Regex, ZeroOrMore)
+    Literal, White, Forward, Group, Optional, OneOrMore, QuotedString, Regex, ZeroOrMore, Combine)
 from pyparsing import stringEnd
 from pyparsing import restOfLine
 
@@ -14,73 +12,42 @@ logger = logging.getLogger(__name__)
 
 class RawNginxParser(object):
     # pylint: disable=expression-not-assigned
+    # pylint: disable=pointless-statement
     """A class that parses nginx configuration with pyparsing."""
 
     # constants
-    space = Optional(White())
-    nonspace = Regex(r"\S+")
+    space = Optional(White()).leaveWhitespace()
+    required_space = White().leaveWhitespace()
+
     left_bracket = Literal("{").suppress()
-    right_bracket = space.leaveWhitespace() + Literal("}").suppress()
+    right_bracket = space + Literal("}").suppress()
     semicolon = Literal(";").suppress()
-    key = Word(alphanums + "_/+-.")
-    dollar_var = Combine(Literal('$') + Regex(r"[^\{\};,\s]+"))
-    condition = Regex(r"\(.+\)")
-    # Matches anything that is not a special character, and ${SHELL_VARS}, AND
-    # any chars in single or double quotes
-    # All of these COULD be upgraded to something like
-    # https://stackoverflow.com/a/16130746
-    dquoted = Regex(r'(\".*\")')
-    squoted = Regex(r"(\'.*\')")
-    nonspecial = Regex(r"[^\{\};,]")
-    varsub = Regex(r"(\$\{\w+\})")
-    # nonspecial nibbles one character at a time, but the other objects take
-    # precedence.  We use ZeroOrMore to allow entries like "break ;" to be
-    # parsed as assignments
-    value = Combine(ZeroOrMore(dquoted | squoted | varsub | nonspecial))
+    dquoted = QuotedString('"', multiline=True, unquoteResults=False, escChar='\\')
+    squoted = QuotedString("'", multiline=True, unquoteResults=False, escChar='\\')
+    quoted = dquoted | squoted
+    head_tokenchars = Regex(r"[^{};\s'\"]") # if (last_space)
+    tail_tokenchars = Regex(r"(\$\{)|[^{;\s]") # else
+    tokenchars = Combine(head_tokenchars + ZeroOrMore(tail_tokenchars))
+    paren_quote_extend = Combine(quoted + Literal(')') + ZeroOrMore(tail_tokenchars))
+    # note: ')' allows extension, but then we fall into else, not last_space.
 
-    location = CharsNotIn("{};," + string.whitespace)
-    # modifier for location uri [ = | ~ | ~* | ^~ ]
-    modifier = Literal("=") | Literal("~*") | Literal("~") | Literal("^~")
+    token = paren_quote_extend | tokenchars | quoted
+
+    whitespace_token_group = space + token + ZeroOrMore(required_space + token) + space
+    assignment = whitespace_token_group + semicolon
 
-    # rules
     comment = space + Literal('#') + restOfLine
 
-    assignment = space + key + Optional(space + value, default=None) + semicolon
-    location_statement = space + Optional(modifier) + Optional(space + location + space)
-    if_statement = space + Literal("if") + space + condition + space
-    charset_map_statement = space + Literal("charset_map") + space + value + space + value
-
-    map_statement = space + Literal("map") + space + nonspace + space + dollar_var + space
-    # This is NOT an accurate way to parse nginx map entries; it's almost
-    # certainly too permissive and may be wrong in other ways, but it should
-    # preserve things correctly in mmmmost or all cases.
-    #
-    #    - I can neither prove nor disprove that it is correct wrt all escaped
-    #      semicolon situations
-    # Addresses https://github.com/fatiherikli/nginxparser/issues/19
-    map_pattern = Regex(r'".*"') | Regex(r"'.*'") | nonspace
-    map_entry = space + map_pattern + space + value + space + semicolon
-    map_block = Group(
-        Group(map_statement).leaveWhitespace() +
-        left_bracket +
-        Group(ZeroOrMore(Group(comment | map_entry)) + space).leaveWhitespace() +
-        right_bracket)
-
     block = Forward()
 
-    # key could for instance be "server" or "http", or "location" (in which case
-    # location_statement needs to have a non-empty location)
+    # order matters! see issue 518, and also http { # server { \n}
+    contents = Group(comment) | Group(block) | Group(assignment)
 
-    block_begin = (Group(space + key + location_statement) ^
-                   Group(if_statement) ^
-                   Group(charset_map_statement)).leaveWhitespace()
+    block_begin = Group(whitespace_token_group)
+    block_innards = Group(ZeroOrMore(contents) + space).leaveWhitespace()
+    block << block_begin + left_bracket + block_innards + right_bracket
 
-    block_innards = Group(ZeroOrMore(Group(comment | assignment) | block | map_block)
-                          + space).leaveWhitespace()
-
-    block << Group(block_begin + left_bracket + block_innards + right_bracket)
-
-    script = OneOrMore(Group(comment | assignment) ^ block ^ map_block) + space + stringEnd
+    script = OneOrMore(contents) + space + stringEnd
     script.parseWithTabs().leaveWhitespace()
 
     def __init__(self, source):
@@ -107,30 +74,23 @@ class RawNginxDumper(object):
             if isinstance(b0, str):
                 yield b0
                 continue
-            b = copy.deepcopy(b0)
-            if spacey(b[0]):
-                yield b.pop(0) # indentation
-                if not b:
+            item = copy.deepcopy(b0)
+            if spacey(item[0]):
+                yield item.pop(0) # indentation
+                if not item:
                     continue
-            key, values = b.pop(0), b.pop(0)
 
-            if isinstance(key, list):
-                yield "".join(key) + '{'
-                for parameter in values:
+            if isinstance(item[0], list): # block
+                yield "".join(item.pop(0)) + '{'
+                for parameter in item.pop(0):
                     for line in self.__iter__([parameter]): # negate "for b0 in blocks"
                         yield line
                 yield '}'
-            else:
-                if isinstance(key, str) and key.strip() == '#':  # comment
-                    yield key + values
-                else:                                            # assignment
-                    gap = ""
-                    # Sometimes the parser has stuck some gap whitespace in here;
-                    # if so rotate it into gap
-                    if values and spacey(values):
-                        gap = values
-                        values = b.pop(0)
-                    yield key + gap + values + ';'
+            else: # not a block - list of strings
+                semicolon = ";"
+                if isinstance(item[0], str) and item[0].strip() == '#': # comment
+                    semicolon = ""
+                yield "".join(item) + semicolon
 
     def __str__(self):
         """Return the parsed block as a string."""

certbot-nginx/certbot_nginx/obj.py

@@ -1,6 +1,8 @@
 """Module contains classes used by the Nginx Configurator."""
 import re
 
+import six
+
 from certbot.plugins import common
 
 REDIRECT_DIRECTIVES = ['return', 'rewrite']
@@ -97,6 +99,11 @@ class Addr(common.Addr):
     def __repr__(self):
         return "Addr(" + self.__str__() + ")"
 
+    def __hash__(self):
+        # Python 3 requires explicit overridden for __hash__
+        # See certbot-apache/certbot_apache/obj.py for more information
+        return super(Addr, self).__hash__()
+
     def super_eq(self, other):
         """Check ip/port equality, with IPv6 support.
         """
@@ -147,13 +154,15 @@ class VirtualHost(object):  # pylint: disable=too-few-public-methods
         self.path = path
 
     def __str__(self):
-        addr_str = ", ".join(str(addr) for addr in self.addrs)
+        addr_str = ", ".join(str(addr) for addr in sorted(self.addrs, key=str))
+        # names might be a set, and it has different representations in Python
+        # 2 and 3. Force it to be a list here for consistent outputs
         return ("file: %s\n"
                 "addrs: %s\n"
                 "names: %s\n"
                 "ssl: %s\n"
                 "enabled: %s" % (self.filep, addr_str,
-                                 self.names, self.ssl, self.enabled))
+                                 list(self.names), self.ssl, self.enabled))
 
     def __repr__(self):
         return "VirtualHost(" + self.__str__().replace("\n", ", ") + ")\n"
@@ -161,7 +170,7 @@ class VirtualHost(object):  # pylint: disable=too-few-public-methods
     def __eq__(self, other):
         if isinstance(other, self.__class__):
             return (self.filep == other.filep and
-                    list(self.addrs) == list(other.addrs) and
+                    sorted(self.addrs, key=str) == sorted(other.addrs, key=str) and
                     self.names == other.names and
                     self.ssl == other.ssl and
                     self.enabled == other.enabled and
@@ -181,7 +190,7 @@ class VirtualHost(object):  # pylint: disable=too-few-public-methods
     def contains_list(self, test):
         """Determine if raw server block contains test list at top level
         """
-        for i in xrange(0, len(self.raw) - len(test)):
+        for i in six.moves.range(0, len(self.raw) - len(test)):
             if self.raw[i:i + len(test)] == test:
                 return True
         return False

certbot-nginx/certbot_nginx/options-ssl-nginx.conf

@@ -4,4 +4,4 @@ ssl_session_timeout 1440m;
 ssl_protocols TLSv1 TLSv1.1 TLSv1.2;
 ssl_prefer_server_ciphers on;
 
-ssl_ciphers "ECDHE-ECDSA-AES128-GCM-SHA256 ECDHE-ECDSA-AES256-GCM-SHA384 ECDHE-ECDSA-AES128-SHA ECDHE-ECDSA-AES256-SHA ECDHE-ECDSA-AES128-SHA256 ECDHE-ECDSA-AES256-SHA384 ECDHE-RSA-AES128-GCM-SHA256 ECDHE-RSA-AES256-GCM-SHA384 ECDHE-RSA-AES128-SHA ECDHE-RSA-AES128-SHA256 ECDHE-RSA-AES256-SHA384 DHE-RSA-AES128-GCM-SHA256 DHE-RSA-AES256-GCM-SHA384 DHE-RSA-AES128-SHA DHE-RSA-AES256-SHA DHE-RSA-AES128-SHA256 DHE-RSA-AES256-SHA256 EDH-RSA-DES-CBC3-SHA";
+ssl_ciphers "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";

certbot-nginx/certbot_nginx/parser.py

@@ -298,9 +298,9 @@ class NginxParser(object):
         """
         server = vhost.raw
         for directive in server:
-            if not directive or len(directive) < 2:
+            if not directive:
                 continue
-            elif directive[0] == 'ssl' and directive[1] == 'on':
+            elif _is_ssl_on_directive(directive):
                 return True
 
         return False
@@ -342,7 +342,7 @@ class NginxParser(object):
             vhost.names = parsed_server['names']
             vhost.raw = new_server
         except errors.MisconfigurationError as err:
-            raise errors.MisconfigurationError("Problem in %s: %s" % (filename, err.message))
+            raise errors.MisconfigurationError("Problem in %s: %s" % (filename, str(err)))
 
 
 def _do_for_subarray(entry, condition, func, path=None):
@@ -468,17 +468,17 @@ def _is_include_directive(entry):
             len(entry) == 2 and entry[0] == 'include' and
             isinstance(entry[1], str))
 
+def _is_ssl_on_directive(entry):
+    """Checks if an nginx parsed entry is an 'ssl on' directive.
 
-def _get_servernames(names):
-    """Turns a server_name string into a list of server names
-
-    :param str names: server names
-    :rtype: list
+    :param list entry: the parsed entry
+    :returns: Whether it's an 'ssl on' directive
+    :rtype: bool
 
     """
-    whitespace_re = re.compile(r'\s+')
-    names = re.sub(whitespace_re, ' ', names)
-    return names.split(' ')
+    return (isinstance(entry, list) and
+            len(entry) == 2 and entry[0] == 'ssl' and
+            entry[1] == 'on')
 
 def _add_directives(block, directives, replace):
     """Adds or replaces directives in a config block.
@@ -550,12 +550,11 @@ def _add_directive(block, directive, replace):
         # and there is already a copy of that directive with a different value
         # in the config file.
         directive_name = directive[0]
-        directive_value = directive[1]
         if location is None or (isinstance(directive_name, str) and
                                 directive_name in REPEATABLE_DIRECTIVES):
             block.append(directive)
             _comment_directive(block, len(block) - 1)
-        elif block[location][1] != directive_value:
+        elif block[location] != directive:
             raise errors.MisconfigurationError(
                 'tried to insert directive "{0}" but found '
                 'conflicting "{1}".'.format(directive, block[location]))
@@ -585,15 +584,14 @@ def _parse_server_raw(server):
         if not directive:
             continue
         if directive[0] == 'listen':
-            addr = obj.Addr.fromstring(directive[1])
+            addr = obj.Addr.fromstring(" ".join(directive[1:]))
             if addr:
                 parsed_server['addrs'].add(addr)
                 if addr.ssl:
                     parsed_server['ssl'] = True
         elif directive[0] == 'server_name':
-            parsed_server['names'].update(
-                _get_servernames(directive[1]))
-        elif directive[0] == 'ssl' and directive[1] == 'on':
+            parsed_server['names'].update(directive[1:])
+        elif _is_ssl_on_directive(directive):
             parsed_server['ssl'] = True
             apply_ssl_to_all_addrs = True
 

certbot-nginx/certbot_nginx/tests/configurator_test.py

@@ -27,12 +27,13 @@ class NginxConfiguratorTest(util.NginxTest):
         super(NginxConfiguratorTest, self).setUp()
 
         self.config = util.get_nginx_configurator(
-            self.config_path, self.config_dir, self.work_dir)
+            self.config_path, self.config_dir, self.work_dir, self.logs_dir)
 
     def tearDown(self):
         shutil.rmtree(self.temp_dir)
         shutil.rmtree(self.config_dir)
         shutil.rmtree(self.work_dir)
+        shutil.rmtree(self.logs_dir)
 
     @mock.patch("certbot_nginx.configurator.util.exe_exists")
     def test_prepare_no_install(self, mock_exe_exists):
@@ -93,7 +94,7 @@ class NginxConfiguratorTest(util.NginxTest):
                                      None, [0])
         self.config.parser.add_server_directives(
             mock_vhost,
-            [['listen', ' ', '5001 ssl']],
+            [['listen', ' ', '5001', ' ', 'ssl']],
             replace=False)
         self.config.save()
 
@@ -104,7 +105,7 @@ class NginxConfiguratorTest(util.NginxTest):
                             ['listen', '127.0.0.1'],
                             ['server_name', '.example.com'],
                             ['server_name', 'example.*'],
-                            ['listen', '5001 ssl'],
+                            ['listen', '5001', 'ssl'],
                             ['#', parser.COMMENT]]]],
                          parsed[0])
 
@@ -204,13 +205,13 @@ class NginxConfiguratorTest(util.NginxTest):
                             ['server_name', '.example.com'],
                             ['server_name', 'example.*'],
 
-                            ['listen', '5001 ssl'],
+                            ['listen', '5001', 'ssl'],
                             ['ssl_certificate', 'example/fullchain.pem'],
                             ['ssl_certificate_key', 'example/key.pem']] +
                             util.filter_comments(self.config.parser.loc["ssl_options"])
                             ]],
                          parsed_example_conf)
-        self.assertEqual([['server_name', 'somename  alias  another.alias']],
+        self.assertEqual([['server_name', 'somename', 'alias', 'another.alias']],
                          parsed_server_conf)
         self.assertTrue(util.contains_at_depth(
             parsed_nginx_conf,
@@ -221,8 +222,8 @@ class NginxConfiguratorTest(util.NginxTest):
               ['include', 'server.conf'],
               [['location', '/'],
                [['root', 'html'],
-                ['index', 'index.html index.htm']]],
-              ['listen', '5001 ssl'],
+                ['index', 'index.html', 'index.htm']]],
+              ['listen', '5001', 'ssl'],
               ['ssl_certificate', '/etc/nginx/fullchain.pem'],
               ['ssl_certificate_key', '/etc/nginx/key.pem']] +
              util.filter_comments(self.config.parser.loc["ssl_options"])
@@ -246,7 +247,7 @@ class NginxConfiguratorTest(util.NginxTest):
                            ['server_name', 'summer.com'],
 
                            ['listen', '80'],
-                           ['listen', '5001 ssl'],
+                           ['listen', '5001', 'ssl'],
                            ['ssl_certificate', 'summer/fullchain.pem'],
                            ['ssl_certificate_key', 'summer/key.pem']] +
                            util.filter_comments(self.config.parser.loc["ssl_options"])
@@ -261,13 +262,13 @@ class NginxConfiguratorTest(util.NginxTest):
         # Note: As more challenges are offered this will have to be expanded
         achall1 = achallenges.KeyAuthorizationAnnotatedChallenge(
             challb=messages.ChallengeBody(
-                chall=challenges.TLSSNI01(token="kNdwjwOeX0I_A8DXt9Msmg"),
+                chall=challenges.TLSSNI01(token=b"kNdwjwOeX0I_A8DXt9Msmg"),
                 uri="https://ca.org/chall0_uri",
                 status=messages.Status("pending"),
             ), domain="localhost", account_key=self.rsa512jwk)
         achall2 = achallenges.KeyAuthorizationAnnotatedChallenge(
             challb=messages.ChallengeBody(
-                chall=challenges.TLSSNI01(token="m8TdO1qik4JVFtgPPurJmg"),
+                chall=challenges.TLSSNI01(token=b"m8TdO1qik4JVFtgPPurJmg"),
                 uri="https://ca.org/chall1_uri",
                 status=messages.Status("pending"),
             ), domain="example.com", account_key=self.rsa512jwk)
@@ -407,8 +408,8 @@ class NginxConfiguratorTest(util.NginxTest):
         # Test that we successfully add a redirect when there is
         # a listen directive
         expected = [
-            ['if', '($scheme != "https") '],
-            [['return', '301 https://$host$request_uri']]
+            ['if', '($scheme', '!=', '"https") '],
+            [['return', '301', 'https://$host$request_uri']]
         ]
 
         example_conf = self.config.parser.abs_path('sites-enabled/example.com')

certbot-nginx/certbot_nginx/tests/nginxparser_test.py

@@ -25,15 +25,15 @@ class TestRawNginxParser(unittest.TestCase):
 
     def test_blocks(self):
         parsed = RawNginxParser.block.parseString('foo {}').asList()
-        self.assertEqual(parsed, [[['foo', ' '], []]])
+        self.assertEqual(parsed, [['foo', ' '], []])
         parsed = RawNginxParser.block.parseString('location /foo{}').asList()
-        self.assertEqual(parsed, [[['location', ' ', '/foo'], []]])
+        self.assertEqual(parsed, [['location', ' ', '/foo'], []])
         parsed = RawNginxParser.block.parseString('foo { bar foo ; }').asList()
-        self.assertEqual(parsed, [[['foo', ' '], [[' ', 'bar', ' ', 'foo '], ' ']]])
+        self.assertEqual(parsed, [['foo', ' '], [[' ', 'bar', ' ', 'foo', ' '], ' ']])
 
     def test_nested_blocks(self):
         parsed = RawNginxParser.block.parseString('foo { bar {} }').asList()
-        block, content = FIRST(parsed)
+        block, content = parsed
         self.assertEqual(FIRST(content), [[' ', 'bar', ' '], []])
         self.assertEqual(FIRST(block), 'foo')
 
@@ -72,8 +72,8 @@ class TestRawNginxParser(unittest.TestCase):
             [['user', 'www-data'],
              [['http'],
               [[['server'], [
-                  ['listen', '*:80 default_server ssl'],
-                  ['server_name', '*.www.foo.com *.www.example.com'],
+                  ['listen', '*:80', 'default_server', 'ssl'],
+                  ['server_name', '*.www.foo.com', '*.www.example.com'],
                   ['root', '/home/ubuntu/sites/foo/'],
                   [['location', '/status'], [
                       [['types'], [['image/jpeg', 'jpg']]],
@@ -97,17 +97,35 @@ class TestRawNginxParser(unittest.TestCase):
              [['server'],
               [['server_name', 'with.if'],
                [['location', '~', '^/services/.+$'],
-                [[['if', '($request_filename ~* \\.(ttf|woff)$)'],
-                  [['add_header', 'Access-Control-Allow-Origin "*"']]]]]]],
+                [[['if', '($request_filename', '~*', '\\.(ttf|woff)$)'],
+                  [['add_header', 'Access-Control-Allow-Origin', '"*"']]]]]]],
              [['server'],
               [['server_name', 'with.complicated.headers'],
                [['location', '~*', '\\.(?:gif|jpe?g|png)$'],
-                [['add_header', 'Pragma public'],
+                [['add_header', 'Pragma', 'public'],
                  ['add_header',
-                  'Cache-Control  \'public, must-revalidate, proxy-revalidate\''
-                  ' "test,;{}" foo'],
+                  'Cache-Control', '\'public, must-revalidate, proxy-revalidate\'',
+                  '"test,;{}"', 'foo'],
                  ['blah', '"hello;world"'],
-                 ['try_files', '$uri @rewrites']]]]]])
+                 ['try_files', '$uri', '@rewrites']]]]]])
+
+    def test_parse_from_file3(self):
+        with open(util.get_data_filename('multiline_quotes.conf')) as handle:
+            parsed = util.filter_comments(load(handle))
+        self.assertEqual(
+            parsed,
+            [[['http'],
+                [[['server'],
+                    [['listen', '*:443'],
+                    [['location', '/'],
+                        [['body_filter_by_lua',
+                          '\'ngx.ctx.buffered = (ngx.ctx.buffered or "")'
+                          ' .. string.sub(ngx.arg[1], 1, 1000)\n'
+                          '                            '
+                          'if ngx.arg[2] then\n'
+                          '                              '
+                          'ngx.var.resp_body = ngx.ctx.buffered\n'
+                          '                            end\'']]]]]]]])
 
     def test_abort_on_parse_failure(self):
         with open(util.get_data_filename('broken.conf')) as handle:
@@ -117,7 +135,7 @@ class TestRawNginxParser(unittest.TestCase):
         with open(util.get_data_filename('nginx.conf')) as handle:
             parsed = load(handle)
         parsed[-1][-1].append(UnspacedList([['server'],
-                               [['listen', ' ', '443 ssl'],
+                               [['listen', ' ', '443', ' ', 'ssl'],
                                 ['server_name', ' ', 'localhost'],
                                 ['ssl_certificate', ' ', 'cert.pem'],
                                 ['ssl_certificate_key', ' ', 'cert.key'],
@@ -126,9 +144,9 @@ class TestRawNginxParser(unittest.TestCase):
                                 ['ssl_ciphers', ' ', 'HIGH:!aNULL:!MD5'],
                                 [['location', ' ', '/'],
                                  [['root', ' ', 'html'],
-                                  ['index', ' ', 'index.html index.htm']]]]]))
+                                  ['index', ' ', 'index.html', ' ', 'index.htm']]]]]))
 
-        with tempfile.TemporaryFile() as f:
+        with tempfile.TemporaryFile(mode='w+t') as f:
             dump(parsed, f)
             f.seek(0)
             parsed_new = load(f)
@@ -138,7 +156,7 @@ class TestRawNginxParser(unittest.TestCase):
         with open(util.get_data_filename('minimalistic_comments.conf')) as handle:
             parsed = load(handle)
 
-        with tempfile.TemporaryFile() as f:
+        with tempfile.TemporaryFile(mode='w+t') as f:
             dump(parsed, f)
             f.seek(0)
             parsed_new = load(f)
@@ -161,10 +179,177 @@ class TestRawNginxParser(unittest.TestCase):
         parsed = loads('if ($http_accept ~* "webp") { set $webp "true"; }')
 
         self.assertEqual(parsed, [
-            [['if', '($http_accept ~* "webp")'],
-             [['set', '$webp "true"']]]
+            [['if', '($http_accept', '~*', '"webp")'],
+             [['set', '$webp', '"true"']]]
         ])
 
+    def test_comment_in_block(self):
+        parsed = loads("""http {
+          # server{
+          }""")
+
+        self.assertEqual(parsed, [
+            [['http'],
+             [['#', ' server{']]]
+        ])
+
+    def test_access_log(self):
+        # see issue #3798
+        parsed = loads('access_log syslog:server=unix:/dev/log,facility=auth,'
+            'tag=nginx_post,severity=info custom;')
+
+        self.assertEqual(parsed, [
+            ['access_log',
+             'syslog:server=unix:/dev/log,facility=auth,tag=nginx_post,severity=info',
+             'custom']
+        ])
+
+    def test_add_header(self):
+        # see issue #3798
+        parsed = loads('add_header Cache-Control no-cache,no-store,must-revalidate,max-age=0;')
+
+        self.assertEqual(parsed, [
+            ['add_header', 'Cache-Control', 'no-cache,no-store,must-revalidate,max-age=0']
+        ])
+
+    def test_map_then_assignment_in_block(self):
+        # see issue #3798
+        test_str = """http {
+            map $http_upgrade $connection_upgrade {
+              default upgrade;
+              ''      close;
+              "~Opera Mini" 1;
+              *.example.com 1;
+            }
+            one;
+        }"""
+        parsed = loads(test_str)
+        self.assertEqual(parsed, [
+            [['http'], [
+                [['map', '$http_upgrade', '$connection_upgrade'], [
+                    ['default', 'upgrade'],
+                    ["''", 'close'],
+                    ['"~Opera Mini"', '1'],
+                    ['*.example.com', '1']
+                ]],
+                ['one']
+            ]]
+        ])
+
+    def test_variable_name(self):
+        parsed = loads('try_files /typo3temp/tx_ncstaticfilecache/'
+            '$host${request_uri}index.html @nocache;')
+
+        self.assertEqual(parsed, [
+            ['try_files',
+             '/typo3temp/tx_ncstaticfilecache/$host${request_uri}index.html',
+             '@nocache']
+        ])
+
+    def test_weird_blocks(self):
+        test = r"""
+            if ($http_user_agent ~ MSIE) {
+                rewrite ^(.*)$ /msie/$1 break;
+            }
+
+            if ($http_cookie ~* "id=([^;]+)(?:;|$)") {
+               set $id $1;
+            }
+
+            if ($request_method = POST) {
+               return 405;
+            }
+
+            if ($request_method) {
+               return 403;
+            }
+
+            if ($args ~ post=140){
+              rewrite ^ http://example.com/;
+            }
+
+            location ~ ^/users/(.+\.(?:gif|jpe?g|png))$ {
+              alias /data/w3/images/$1;
+            }
+        """
+        parsed = loads(test)
+        self.assertEqual(parsed, [[['if', '($http_user_agent', '~', 'MSIE)'],
+            [['rewrite', '^(.*)$', '/msie/$1', 'break']]],
+            [['if', '($http_cookie', '~*', '"id=([^;]+)(?:;|$)")'], [['set', '$id', '$1']]],
+            [['if', '($request_method', '=', 'POST)'], [['return', '405']]],
+            [['if', '($request_method)'],
+            [['return', '403']]], [['if', '($args', '~', 'post=140)'],
+            [['rewrite', '^', 'http://example.com/']]],
+            [['location', '~', '^/users/(.+\\.(?:gif|jpe?g|png))$'],
+            [['alias', '/data/w3/images/$1']]]]
+        )
+
+    def test_edge_cases(self):
+        # quotes
+        parsed = loads(r'"hello\""; # blah "heh heh"')
+        self.assertEqual(parsed, [['"hello\\""'], ['#', ' blah "heh heh"']])
+
+        # empty var as block
+        parsed = loads(r"${}")
+        self.assertEqual(parsed, [[['$'], []]])
+
+        # if with comment
+        parsed = loads("""if ($http_cookie ~* "id=([^;]+)(?:;|$)") { # blah )
+            }""")
+        self.assertEqual(parsed, [[['if', '($http_cookie', '~*', '"id=([^;]+)(?:;|$)")'],
+            [['#', ' blah )']]]])
+
+        # end paren
+        test = """
+            one"test";
+            ("two");
+            "test")red;
+            "test")"blue";
+            "test")"three;
+            (one"test")one;
+            one";
+            one"test;
+            one"test"one;
+        """
+        parsed = loads(test)
+        self.assertEqual(parsed, [
+            ['one"test"'],
+            ['("two")'],
+            ['"test")red'],
+            ['"test")"blue"'],
+            ['"test")"three'],
+            ['(one"test")one'],
+            ['one"'],
+            ['one"test'],
+            ['one"test"one']
+        ])
+        self.assertRaises(ParseException, loads, r'"test"one;') # fails
+        self.assertRaises(ParseException, loads, r'"test;') # fails
+
+        # newlines
+        test = """
+            server_name foo.example.com bar.example.com \
+                        baz.example.com qux.example.com;
+            server_name foo.example.com bar.example.com
+                        baz.example.com qux.example.com;
+        """
+        parsed = loads(test)
+        self.assertEqual(parsed, [
+            ['server_name', 'foo.example.com', 'bar.example.com',
+                'baz.example.com', 'qux.example.com'],
+            ['server_name', 'foo.example.com', 'bar.example.com',
+                'baz.example.com', 'qux.example.com']
+        ])
+
+        # variable weirdness
+        parsed = loads("directive $var;")
+        self.assertEqual(parsed, [['directive', '$var']])
+        self.assertRaises(ParseException, loads, "server {server_name test.com};")
+        self.assertRaises(ParseException, loads, "directive ${var};")
+        self.assertEqual(loads("blag${dfgdfg};"), [['blag${dfgdfg}']])
+        self.assertRaises(ParseException, loads, "blag${dfgdf{g};")
+
+
 class TestUnspacedList(unittest.TestCase):
     """Test the UnspacedList data structure"""
     def setUp(self):
@@ -219,18 +404,18 @@ class TestUnspacedList(unittest.TestCase):
                 ['\n    ', 'listen', '       ', '127.0.0.1'],
                 ['\n    ', 'server_name', ' ', '.example.com'],
                 ['\n    ', 'server_name', ' ', 'example.*'], '\n',
-                ['listen', ' ', '5001 ssl']])
+                ['listen', ' ', '5001', ' ', 'ssl']])
         x.insert(5, "FROGZ")
         self.assertEqual(x,
             [['listen', '69.50.225.155:9000'], ['listen', '127.0.0.1'],
             ['server_name', '.example.com'], ['server_name', 'example.*'],
-            ['listen', '5001 ssl'], 'FROGZ'])
+            ['listen', '5001', 'ssl'], 'FROGZ'])
         self.assertEqual(x.spaced,
             [['\n    ', 'listen', '       ', '69.50.225.155:9000'],
             ['\n    ', 'listen', '       ', '127.0.0.1'],
             ['\n    ', 'server_name', ' ', '.example.com'],
             ['\n    ', 'server_name', ' ', 'example.*'], '\n',
-            ['listen', ' ', '5001 ssl'],
+            ['listen', ' ', '5001', ' ', 'ssl'],
             'FROGZ'])
 
     def test_rawlists(self):

certbot-nginx/certbot_nginx/tests/obj_test.py

@@ -108,8 +108,8 @@ class VirtualHostTest(unittest.TestCase):
         from certbot_nginx.obj import Addr
         raw1 = [
             ['listen', '69.50.225.155:9000'],
-            [['if', '($scheme != "https") '],
-                [['return', '301 https://$host$request_uri']]
+            [['if', '($scheme', '!=', '"https") '],
+                [['return', '301', 'https://$host$request_uri']]
             ],
             ['#', ' managed by Certbot']
         ]
@@ -119,8 +119,8 @@ class VirtualHostTest(unittest.TestCase):
             set(['localhost']), raw1, [])
         raw2 = [
             ['listen', '69.50.225.155:9000'],
-            [['if', '($scheme != "https") '],
-                [['return', '301 https://$host$request_uri']]
+            [['if', '($scheme', '!=', '"https") '],
+                [['return', '301', 'https://$host$request_uri']]
             ]
         ]
         self.vhost2 = VirtualHost(
@@ -129,7 +129,7 @@ class VirtualHostTest(unittest.TestCase):
             set(['localhost']), raw2, [])
         raw3 = [
             ['listen', '69.50.225.155:9000'],
-            ['rewrite', '^(.*)$ $scheme://www.domain.com$1 permanent;']
+            ['rewrite', '^(.*)$', '$scheme://www.domain.com$1', 'permanent']
         ]
         self.vhost3 = VirtualHost(
             "filep",
@@ -158,7 +158,7 @@ class VirtualHostTest(unittest.TestCase):
 
     def test_str(self):
         stringified = '\n'.join(['file: filep', 'addrs: localhost',
-                                 "names: set(['localhost'])", 'ssl: False',
+                                 "names: ['localhost']", 'ssl: False',
                                  'enabled: False'])
         self.assertEqual(stringified, str(self.vhost1))
 
@@ -181,7 +181,9 @@ class VirtualHostTest(unittest.TestCase):
             ['#', ' managed by Certbot'],
             ['ssl_certificate_key', '/etc/letsencrypt/live/two.functorkitten.xyz/privkey.pem'],
             ['#', ' managed by Certbot'],
-            [['if', '($scheme != "https")'], [['return', '301 https://$host$request_uri']]],
+            [['if', '($scheme', '!=', '"https")'],
+             [['return', '301', 'https://$host$request_uri']]
+            ],
             ['#', ' managed by Certbot'], []]
         vhost_haystack = VirtualHost(
             "filp",
@@ -195,7 +197,9 @@ class VirtualHostTest(unittest.TestCase):
             ['#', ' managed by Certbot'],
             ['ssl_certificate_key', '/etc/letsencrypt/live/two.functorkitten.xyz/privkey.pem'],
             ['#', ' managed by Certbot'],
-            [['if', '($scheme != "https")'], [['return', '302 https://$host$request_uri']]],
+            [['if', '($scheme', '!=', '"https")'],
+             [['return', '302', 'https://$host$request_uri']]
+            ],
             ['#', ' managed by Certbot'], []]
         vhost_bad_haystack = VirtualHost(
             "filp",

certbot-nginx/certbot_nginx/tests/parser_test.py

@@ -52,7 +52,7 @@ class NginxParserTest(util.NginxTest):
                                'sites-enabled/sslon.com',
                                'sites-enabled/globalssl.com']]),
                          set(nparser.parsed.keys()))
-        self.assertEqual([['server_name', 'somename  alias  another.alias']],
+        self.assertEqual([['server_name', 'somename', 'alias', 'another.alias']],
                          nparser.parsed[nparser.abs_path('server.conf')])
         self.assertEqual([[['server'], [['listen', '69.50.225.155:9000'],
                                         ['listen', '127.0.0.1'],
@@ -168,16 +168,16 @@ class NginxParserTest(util.NginxTest):
                [['location', '/'], [['root', 'html'], ['index', 'index.html index.htm']]]
                ], None)
         self.assertFalse(nparser.has_ssl_on_directive(mock_vhost))
-        mock_vhost.raw = [['listen', '*:80 default_server ssl'],
-                          ['server_name', '*.www.foo.com *.www.example.com'],
+        mock_vhost.raw = [['listen', '*:80', 'default_server', 'ssl'],
+                          ['server_name', '*.www.foo.com', '*.www.example.com'],
                           ['root', '/home/ubuntu/sites/foo/']]
         self.assertFalse(nparser.has_ssl_on_directive(mock_vhost))
         mock_vhost.raw = [['listen', '80 ssl'],
-                          ['server_name', '*.www.foo.com *.www.example.com']]
+                          ['server_name', '*.www.foo.com', '*.www.example.com']]
         self.assertFalse(nparser.has_ssl_on_directive(mock_vhost))
         mock_vhost.raw = [['listen', '80'],
                           ['ssl', 'on'],
-                          ['server_name', '*.www.foo.com *.www.example.com']]
+                          ['server_name', '*.www.foo.com', '*.www.example.com']]
         self.assertTrue(nparser.has_ssl_on_directive(mock_vhost))
 
     def test_add_server_directives(self):
@@ -309,7 +309,7 @@ class NginxParserTest(util.NginxTest):
         self.assertFalse(server['ssl'])
 
         server = parser._parse_server_raw([ #pylint: disable=protected-access
-            ['listen', '443 ssl']
+            ['listen', '443', 'ssl']
         ])
         self.assertTrue(server['ssl'])
 
@@ -341,16 +341,21 @@ class NginxParserTest(util.NginxTest):
         self.assertEqual(nginxparser.UnspacedList(nparser.loc["ssl_options"]),
                          [['ssl_session_cache', 'shared:le_nginx_SSL:1m'],
                           ['ssl_session_timeout', '1440m'],
-                          ['ssl_protocols', 'TLSv1 TLSv1.1 TLSv1.2'],
+                          ['ssl_protocols', 'TLSv1', 'TLSv1.1', 'TLSv1.2'],
                           ['ssl_prefer_server_ciphers', 'on'],
-                          ['ssl_ciphers', '"ECDHE-ECDSA-AES128-GCM-SHA256 ECDHE-ECDSA-'+
-                           'AES256-GCM-SHA384 ECDHE-ECDSA-AES128-SHA ECDHE-ECDSA-AES256'+
-                           '-SHA ECDHE-ECDSA-AES128-SHA256 ECDHE-ECDSA-AES256-SHA384'+
-                           ' ECDHE-RSA-AES128-GCM-SHA256 ECDHE-RSA-AES256-GCM-SHA384'+
-                           ' ECDHE-RSA-AES128-SHA ECDHE-RSA-AES128-SHA256 ECDHE-RSA-'+
-                           'AES256-SHA384 DHE-RSA-AES128-GCM-SHA256 DHE-RSA-AES256-GCM'+
-                           '-SHA384 DHE-RSA-AES128-SHA DHE-RSA-AES256-SHA DHE-RSA-'+
-                           'AES128-SHA256 DHE-RSA-AES256-SHA256 EDH-RSA-DES-CBC3-SHA"']
+                          ['ssl_ciphers', '"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"']
                          ])
 
 if __name__ == "__main__":

certbot-nginx/certbot_nginx/tests/testdata/etc_nginx/multiline_quotes.conf

@@ -0,0 +1,16 @@
+# Test nginx configuration file with multiline quoted strings.
+# Good example of usage for multilined quoted values is when
+# using Openresty's Lua directives and you wish to keep the
+# inline Lua code readable.
+http {
+  server {
+      listen *:443; # because there should be no other port open.
+
+      location / {
+        body_filter_by_lua 'ngx.ctx.buffered = (ngx.ctx.buffered or "") .. string.sub(ngx.arg[1], 1, 1000)
+                            if ngx.arg[2] then
+                              ngx.var.resp_body = ngx.ctx.buffered
+                            end';
+      }
+  }
+}

certbot-nginx/certbot_nginx/tests/tls_sni_01_test.py

@@ -3,6 +3,7 @@ import unittest
 import shutil
 
 import mock
+import six
 
 from acme import challenges
 
@@ -23,25 +24,25 @@ class TlsSniPerformTest(util.NginxTest):
     achalls = [
         achallenges.KeyAuthorizationAnnotatedChallenge(
             challb=acme_util.chall_to_challb(
-                challenges.TLSSNI01(token="kNdwjwOeX0I_A8DXt9Msmg"), "pending"),
+                challenges.TLSSNI01(token=b"kNdwjwOeX0I_A8DXt9Msmg"), "pending"),
             domain="www.example.com", account_key=account_key),
         achallenges.KeyAuthorizationAnnotatedChallenge(
             challb=acme_util.chall_to_challb(
                 challenges.TLSSNI01(
-                    token="\xba\xa9\xda?<m\xaewmx\xea\xad\xadv\xf4\x02\xc9y"
-                          "\x80\xe2_X\t\xe7\xc7\xa4\t\xca\xf7&\x945"
+                    token=b"\xba\xa9\xda?<m\xaewmx\xea\xad\xadv\xf4\x02\xc9y"
+                          b"\x80\xe2_X\t\xe7\xc7\xa4\t\xca\xf7&\x945"
                 ), "pending"),
             domain="another.alias", account_key=account_key),
         achallenges.KeyAuthorizationAnnotatedChallenge(
             challb=acme_util.chall_to_challb(
                 challenges.TLSSNI01(
-                    token="\x8c\x8a\xbf_-f\\cw\xee\xd6\xf8/\xa5\xe3\xfd"
-                          "\xeb9\xf1\xf5\xb9\xefVM\xc9w\xa4u\x9c\xe1\x87\xb4"
+                    token=b"\x8c\x8a\xbf_-f\\cw\xee\xd6\xf8/\xa5\xe3\xfd"
+                          b"\xeb9\xf1\xf5\xb9\xefVM\xc9w\xa4u\x9c\xe1\x87\xb4"
                 ), "pending"),
             domain="www.example.org", account_key=account_key),
         achallenges.KeyAuthorizationAnnotatedChallenge(
             challb=acme_util.chall_to_challb(
-                challenges.TLSSNI01(token="kNdwjxOeX0I_A8DXt9Msmg"), "pending"),
+                challenges.TLSSNI01(token=b"kNdwjxOeX0I_A8DXt9Msmg"), "pending"),
             domain="sslon.com", account_key=account_key),
     ]
 
@@ -49,7 +50,7 @@ class TlsSniPerformTest(util.NginxTest):
         super(TlsSniPerformTest, self).setUp()
 
         config = util.get_nginx_configurator(
-            self.config_path, self.config_dir, self.work_dir)
+            self.config_path, self.config_dir, self.work_dir, self.logs_dir)
 
         from certbot_nginx import tls_sni_01
         self.sni = tls_sni_01.NginxTlsSni01(config)
@@ -117,7 +118,7 @@ class TlsSniPerformTest(util.NginxTest):
             util.contains_at_depth(http, ['server_name', 'another.alias'], 3))
 
         self.assertEqual(len(sni_responses), 4)
-        for i in xrange(4):
+        for i in six.moves.range(4):
             self.assertEqual(sni_responses[i], acme_responses[i])
 
     def test_mod_config(self):
@@ -148,7 +149,7 @@ class TlsSniPerformTest(util.NginxTest):
             else:
                 response = self.achalls[2].response(self.account_key)
                 self.assertEqual(vhost.addrs, set(v_addr2_print))
-            self.assertEqual(vhost.names, set([response.z_domain]))
+            self.assertEqual(vhost.names, set([response.z_domain.decode('ascii')]))
 
         self.assertEqual(len(vhs), 2)
 

certbot-nginx/certbot_nginx/tests/util.py

@@ -2,6 +2,7 @@
 import copy
 import os
 import pkg_resources
+import tempfile
 import unittest
 
 import mock
@@ -27,6 +28,7 @@ class NginxTest(unittest.TestCase):  # pylint: disable=too-few-public-methods
 
         self.temp_dir, self.config_dir, self.work_dir = common.dir_setup(
             "etc_nginx", "certbot_nginx.tests")
+        self.logs_dir = tempfile.mkdtemp('logs')
 
         self.ssl_options = common.setup_ssl_options(
             self.config_dir, constants.MOD_SSL_CONF_SRC,
@@ -46,7 +48,7 @@ def get_data_filename(filename):
 
 
 def get_nginx_configurator(
-        config_path, config_dir, work_dir, version=(1, 6, 2)):
+        config_path, config_dir, work_dir, logs_dir, version=(1, 6, 2)):
     """Create an Nginx Configurator with the specified options."""
 
     backups = os.path.join(work_dir, "backups")
@@ -62,11 +64,13 @@ def get_nginx_configurator(
                     le_vhost_ext="-le-ssl.conf",
                     config_dir=config_dir,
                     work_dir=work_dir,
+                    logs_dir=logs_dir,
                     backup_dir=backups,
                     temp_checkpoint_dir=os.path.join(work_dir, "temp_checkpoints"),
                     in_progress_dir=os.path.join(backups, "IN_PROGRESS"),
                     server="https://acme-server.org:443/new",
                     tls_sni_01_port=5001,
+                    dry_run=False,
                 ),
                 name="nginx",
                 version=version)
@@ -111,7 +115,7 @@ def contains_at_depth(haystack, needle, n):
     """
     # Specifically use hasattr rather than isinstance(..., collections.Iterable)
     # because we want to include lists but reject strings.
-    if not hasattr(haystack, '__iter__'):
+    if not hasattr(haystack, '__iter__') or hasattr(haystack, 'strip'):
         return False
     if n == 0:
         return needle in haystack

certbot-nginx/certbot_nginx/tls_sni_01.py

@@ -1,9 +1,10 @@
 """A class that performs TLS-SNI-01 challenges for Nginx"""
 
-import itertools
 import logging
 import os
 
+import six
+
 from certbot import errors
 from certbot.plugins import common
 
@@ -95,11 +96,12 @@ class NginxTlsSni01(common.TLSSNI01):
         bucket_directive = ['\n', 'server_names_hash_bucket_size', ' ', '128']
 
         main = self.configurator.parser.parsed[root]
-        for key, body in main:
-            if key == ['http']:
+        for line in main:
+            if line[0] == ['http']:
+                body = line[1]
                 found_bucket = False
-                for k, _ in body:
-                    if k == bucket_directive[1]:
+                for inner_line in body:
+                    if inner_line[0] == bucket_directive[1]:
                         found_bucket = True
                 if not found_bucket:
                     body.insert(0, bucket_directive)
@@ -113,7 +115,7 @@ class NginxTlsSni01(common.TLSSNI01):
                 'TLS-SNI-01 challenges in %s.' % root)
 
         config = [self._make_server_block(pair[0], pair[1])
-                  for pair in itertools.izip(self.achalls, ll_addrs)]
+                  for pair in six.moves.zip(self.achalls, ll_addrs)]
         config = nginxparser.UnspacedList(config)
 
         self.configurator.reverter.register_file_creation(
@@ -142,7 +144,7 @@ class NginxTlsSni01(common.TLSSNI01):
         block = [['listen', ' ', addr.to_string(include_default=False)] for addr in addrs]
 
         block.extend([['server_name', ' ',
-                       achall.response(achall.account_key).z_domain],
+                       achall.response(achall.account_key).z_domain.decode('ascii')],
                       # access and error logs necessary for
                       # integration testing (non-root)
                       ['access_log', ' ', os.path.join(

certbot/auth_handler.py

@@ -66,11 +66,16 @@ class AuthHandler(object):
             self.authzr[domain] = self.acme.request_domain_challenges(domain)
 
         self._choose_challenges(domains)
+        config = zope.component.getUtility(interfaces.IConfig)
+        notify = zope.component.getUtility(interfaces.IDisplay).notification
 
         # While there are still challenges remaining...
         while self.achalls:
             resp = self._solve_challenges()
             logger.info("Waiting for verification...")
+            if config.debug_challenges:
+                notify('Challenges loaded. Press continue to submit to CA. '
+                       'Pass "-v" for more info about challenges.', pause=True)
 
             # Send all Responses - this modifies achalls
             self._respond(resp, best_effort)

certbot/cert_manager.py

@@ -13,6 +13,7 @@ from certbot import storage
 from certbot import util
 
 from certbot.display import util as display_util
+from certbot.plugins import disco as plugins_disco
 
 logger = logging.getLogger(__name__)
 
@@ -46,6 +47,7 @@ def rename_lineage(config):
 
     certname = _get_certname(config, "rename")
 
+    # what is the new name we want to use?
     new_certname = config.new_certname
     if not new_certname:
         code, new_certname = disp.input(
@@ -54,13 +56,34 @@ def rename_lineage(config):
         if code != display_util.OK or not new_certname:
             raise errors.Error("User ended interaction.")
 
-    lineage = lineage_for_certname(config, certname)
-    if not lineage:
-        raise errors.ConfigurationError("No existing certificate with name "
-            "{0} found.".format(certname))
-    storage.rename_renewal_config(certname, new_certname, config)
-    disp.notification("Successfully renamed {0} to {1}."
-        .format(certname, new_certname), pause=False)
+    try:
+        # copy files to new name
+        new_lineage = storage.duplicate_lineage(config, certname, new_certname)
+
+        # install the new name's files
+        config.certname = new_certname
+        plugins = plugins_disco.PluginsRegistry.find_all()
+        from certbot.main import install
+        install(config, plugins, new_lineage, False)
+    except (errors.CertStorageError, errors.ConfigurationError, IOError, OSError) as e:
+        # delete the new files
+        config.certname = new_certname
+        # we might not have created anything to delete
+        try:
+            storage.delete_files(config, new_certname)
+        except errors.CertStorageError:
+            pass
+        reporter = zope.component.getUtility(interfaces.IReporter)
+        reporter.add_message("Unable to rename certificate", reporter.HIGH_PRIORITY)
+        raise e
+    else:
+        # delete old files
+        config.certname = certname
+        storage.delete_files(config, certname)
+        disp.notification("Renamed files for {0} to {1}."
+            .format(certname, new_certname), pause=False)
+    finally:
+        config.certname = certname
 
 def certificates(config):
     """Display information about certs configured with Certbot

certbot/cli.py

@@ -81,6 +81,7 @@ obtain, install, and renew certificates:
 manage certificates:
     certificates    Display information about certs you have from Certbot
     revoke          Revoke a certificate (supply --cert-path)
+    rename          Rename a certificate
     delete          Delete a certificate
 
 manage your account with Let's Encrypt:
@@ -207,7 +208,7 @@ def set_by_cli(var):
 
     return False
 # static housekeeping var
-set_by_cli.detector = None
+set_by_cli.detector = None  # type: ignore
 
 
 def has_default_value(option, value):
@@ -363,6 +364,10 @@ VERB_HELP = [
         "opts": "Options for revocation of certs",
         "usage": "\n\n  certbot revoke --cert-path /path/to/fullchain.pem [options]\n\n"
     }),
+    ("rename", {
+        "short": "Change a certificate's name (for management purposes)",
+        "opts": "Options for changing certificate names"
+    }),
     ("register", {
         "short": "Register for account with Let's Encrypt / other ACME server",
         "opts": "Options for account registration & modification"
@@ -420,6 +425,7 @@ class HelpfulArgumentParser(object):
             "plugins": main.plugins_cmd,
             "register": main.register,
             "unregister": main.unregister,
+            "rename": main.rename,
             "renew": main.renew,
             "revoke": main.revoke,
             "rollback": main.rollback,
@@ -791,7 +797,7 @@ def _add_all_groups(helpful):
     helpful.add_group("paths", description="Arguments changing execution paths & servers")
     helpful.add_group("manage",
         description="Various subcommands and flags are available for managing your certificates:",
-        verbs=["certificates", "delete", "renew", "revoke", "update_symlinks"])
+        verbs=["certificates", "delete", "rename", "renew", "revoke", "update_symlinks"])
 
     # VERBS
     for verb, docs in VERB_HELP:
@@ -844,12 +850,17 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False):  # pylint: dis
              "multiple -d flags or enter a comma separated list of domains "
              "as a parameter. (default: Ask)")
     helpful.add(
-        [None, "run", "certonly", "manage", "delete", "certificates"],
+        [None, "run", "certonly", "manage", "rename", "delete", "certificates"],
         "--cert-name", dest="certname",
         metavar="CERTNAME", default=None,
         help="Certificate name to apply. Only one certificate name can be used "
              "per Certbot run. To see certificate names, run 'certbot certificates'. "
              "When creating a new certificate, specifies the new certificate's name.")
+    helpful.add(
+        ["rename", "manage"],
+        "--updated-cert-name", dest="new_certname",
+        metavar="NEW_CERTNAME", default=None,
+        help="New name for the certificate. Must be a valid filename.")
     helpful.add(
         [None, "testing", "renew", "certonly"],
         "--dry-run", action="store_true", dest="dry_run",
@@ -956,6 +967,11 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False):  # pylint: dis
         "testing", "--debug", action="store_true",
         help="Show tracebacks in case of errors, and allow certbot-auto "
              "execution on experimental platforms")
+    helpful.add(
+        [None, "certonly", "renew", "run"], "--debug-challenges", action="store_true",
+        default=flag_default("debug_challenges"),
+        help="After setting up challenges, wait for user input before "
+             "submitting to CA")
     helpful.add(
         "testing", "--no-verify-ssl", action="store_true",
         help=config_help("no_verify_ssl"),
@@ -1152,7 +1168,7 @@ def _paths_parser(helpful):
     default_cp = None
     if verb == "certonly":
         default_cp = flag_default("auth_chain_path")
-    add(["install", "paths"], "--fullchain-path", default=default_cp, type=os.path.abspath,
+    add(["paths", "install"], "--fullchain-path", default=default_cp, type=os.path.abspath,
         help="Accompanying path to a full certificate chain (cert plus chain).")
     add("paths", "--chain-path", default=default_cp, type=os.path.abspath,
         help="Accompanying path to a certificate chain.")

certbot/client.py

@@ -9,6 +9,7 @@ import OpenSSL
 import zope.component
 
 from acme import client as acme_client
+from acme import errors as acme_errors
 from acme import jose
 from acme import messages
 
@@ -126,7 +127,7 @@ def register(config, account_storage, tos_cb=None):
             logger.warning(msg)
             raise errors.Error(msg)
         if not config.dry_run:
-            logger.warning("Registering without email!")
+            logger.info("Registering without email!")
 
     # Each new registration shall use a fresh new key
     key = jose.JWKRSA(key=jose.ComparableRSAKey(
@@ -175,7 +176,7 @@ def perform_registration(acme, config):
                        "registration again." % config.email)
                 raise errors.Error(msg)
             else:
-                config.namespace.email = display_ops.get_email(invalid=True)
+                config.email = display_ops.get_email(invalid=True)
                 return perform_registration(acme, config)
         else:
             raise
@@ -251,7 +252,28 @@ class Client(object):
             jose.ComparableX509(
                 OpenSSL.crypto.load_certificate_request(typ, csr.data)),
                 authzr)
-        return certr, self.acme.fetch_chain(certr)
+
+        notify = zope.component.getUtility(interfaces.IDisplay).notification
+        retries = 0
+        chain = None
+
+        while retries <= 1:
+            if retries:
+                notify('Failed to fetch chain, please check your network '
+                       'and continue', pause=True)
+            try:
+                chain = self.acme.fetch_chain(certr)
+                break
+            except acme_errors.Error:
+                logger.debug('Failed to fetch chain', exc_info=True)
+                retries += 1
+
+        if chain is None:
+            raise acme_errors.Error(
+                'Failed to fetch chain. You should not deploy the generated '
+                'certificate, please rerun the command for a new one.')
+
+        return certr, chain
 
     def obtain_certificate(self, domains):
         """Obtains a certificate from the ACME server.
@@ -278,10 +300,12 @@ class Client(object):
         key = crypto_util.init_save_key(
             self.config.rsa_key_size, self.config.key_dir)
         csr = crypto_util.init_save_csr(key, domains, self.config.csr_dir)
+        certr, chain = self.obtain_certificate_from_csr(
+            domains, csr, authzr=authzr)
 
-        return (self.obtain_certificate_from_csr(domains, csr, authzr=authzr)
-                                                                + (key, csr))
+        return certr, chain, key, csr
 
+    # pylint: disable=no-member
     def obtain_and_enroll_certificate(self, domains, certname):
         """Obtain and enroll certificate.
 

certbot/colored_logging.py

@@ -1,45 +0,0 @@
-"""A formatter and StreamHandler for colorizing logging output."""
-import logging
-import sys
-
-from certbot import util
-
-
-class StreamHandler(logging.StreamHandler):
-    """Sends colored logging output to a stream.
-
-    If the specified stream is not a tty, the class works like the
-    standard logging.StreamHandler. Default red_level is logging.WARNING.
-
-    :ivar bool colored: True if output should be colored
-    :ivar bool red_level: The level at which to output
-
-    """
-
-    def __init__(self, stream=None):
-        if sys.version_info < (2, 7):
-            # pragma: no cover
-            # pylint: disable=non-parent-init-called
-            logging.StreamHandler.__init__(self, stream)
-        else:
-            super(StreamHandler, self).__init__(stream)
-        self.colored = (sys.stderr.isatty() if stream is None else
-                        stream.isatty())
-        self.red_level = logging.WARNING
-
-    def format(self, record):
-        """Formats the string representation of record.
-
-        :param logging.LogRecord record: Record to be formatted
-
-        :returns: Formatted, string representation of record
-        :rtype: str
-
-        """
-        out = (logging.StreamHandler.format(self, record)
-               if sys.version_info < (2, 7)
-               else super(StreamHandler, self).format(record))
-        if self.colored and record.levelno >= self.red_level:
-            return ''.join((util.ANSI_SGR_RED, out, util.ANSI_SGR_RESET))
-        else:
-            return out

certbot/configuration.py

@@ -42,7 +42,7 @@ class NamespaceConfig(object):
     """
 
     def __init__(self, namespace):
-        self.namespace = namespace
+        object.__setattr__(self, 'namespace', namespace)
 
         self.namespace.config_dir = os.path.abspath(self.namespace.config_dir)
         self.namespace.work_dir = os.path.abspath(self.namespace.work_dir)
@@ -54,6 +54,9 @@ class NamespaceConfig(object):
     def __getattr__(self, name):
         return getattr(self.namespace, name)
 
+    def __setattr__(self, name, value):
+        setattr(self.namespace, name, value)
+
     @property
     def server_path(self):
         """File path based on ``server``."""

certbot/constants.py

@@ -18,6 +18,7 @@ CLI_DEFAULTS = dict(
         os.path.join(os.environ.get("XDG_CONFIG_HOME", "~/.config"),
                      "letsencrypt", "cli.ini"),
     ],
+    dry_run=False,
     verbose_count=-int(logging.INFO / 10),
     server="https://acme-v01.api.letsencrypt.org/directory",
     rsa_key_size=2048,
@@ -32,6 +33,7 @@ CLI_DEFAULTS = dict(
     auth_cert_path="./cert.pem",
     auth_chain_path="./chain.pem",
     strict_permissions=False,
+    debug_challenges=False,
 )
 STAGING_URI = "https://acme-staging.api.letsencrypt.org/directory"
 

certbot/crypto_util.py

@@ -53,12 +53,15 @@ def init_save_key(key_size, key_dir, keyname="key-certbot.pem"):
     # Save file
     util.make_or_verify_dir(key_dir, 0o700, os.geteuid(),
                             config.strict_permissions)
-    key_f, key_path = util.unique_file(
-        os.path.join(key_dir, keyname), 0o600, "wb")
-    with key_f:
-        key_f.write(key_pem)
-
-    logger.info("Generating key (%d bits): %s", key_size, key_path)
+    if config.dry_run:
+        key_path = None
+        logger.info("Generating key (%d bits), not saving to file", key_size)
+    else:
+        key_f, key_path = util.unique_file(
+            os.path.join(key_dir, keyname), 0o600, "wb")
+        with key_f:
+            key_f.write(key_pem)
+        logger.info("Generating key (%d bits): %s", key_size, key_path)
 
     return util.Key(key_path, key_pem)
 
@@ -85,12 +88,15 @@ def init_save_csr(privkey, names, path, csrname="csr-certbot.pem"):
     # Save CSR
     util.make_or_verify_dir(path, 0o755, os.geteuid(),
                                config.strict_permissions)
-    csr_f, csr_filename = util.unique_file(
-        os.path.join(path, csrname), 0o644, "wb")
-    csr_f.write(csr_pem)
-    csr_f.close()
-
-    logger.info("Creating CSR: %s", csr_filename)
+    if config.dry_run:
+        csr_filename = None
+        logger.info("Creating CSR: not saving to file")
+    else:
+        csr_f, csr_filename = util.unique_file(
+            os.path.join(path, csrname), 0o644, "wb")
+        with csr_f:
+            csr_f.write(csr_pem)
+        logger.info("Creating CSR: %s", csr_filename)
 
     return util.CSR(csr_filename, csr_der, "der")
 

certbot/display/completer.py

@@ -4,7 +4,7 @@ import glob
 try:
     import readline
 except ImportError:
-    import certbot.display.dummy_readline as readline
+    import certbot.display.dummy_readline as readline  # type: ignore
 
 
 class Completer(object):

certbot/hooks.py

@@ -13,12 +13,14 @@ from certbot.plugins import util as plug_util
 
 logger = logging.getLogger(__name__)
 
+
 def validate_hooks(config):
     """Check hook commands are executable."""
     validate_hook(config.pre_hook, "pre")
     validate_hook(config.post_hook, "post")
     validate_hook(config.renew_hook, "renew")
 
+
 def _prog(shell_cmd):
     """Extract the program run by a shell command.
 
@@ -52,6 +54,7 @@ def validate_hook(shell_cmd, hook_name):
 
             raise errors.HookCommandNotFound(msg)
 
+
 def pre_hook(config):
     "Run pre-hook if it's defined and hasn't been run."
     cmd = config.pre_hook
@@ -62,7 +65,7 @@ def pre_hook(config):
     elif cmd:
         logger.info("Pre-hook command already run, skipping: %s", cmd)
 
-pre_hook.already = set()
+pre_hook.already = set()  # type: ignore
 
 
 def post_hook(config):
@@ -82,7 +85,8 @@ def post_hook(config):
         logger.info("Running post-hook command: %s", cmd)
         _run_hook(cmd)
 
-post_hook.eventually = []
+post_hook.eventually = []  # type: ignore
+
 
 def run_saved_post_hooks():
     """Run any post hooks that were saved up in the course of the 'renew' verb"""

certbot/interfaces.py

@@ -99,7 +99,7 @@ class IPluginFactory(zope.interface.Interface):
 class IPlugin(zope.interface.Interface):
     """Certbot plugin."""
 
-    def prepare():
+    def prepare():  # type: ignore
         """Prepare the plugin.
 
         Finish up any additional initialization.
@@ -118,7 +118,7 @@ class IPlugin(zope.interface.Interface):
 
         """
 
-    def more_info():
+    def more_info():  # type: ignore
         """Human-readable string to help the user.
 
         Should describe the steps taken and any relevant info to help the user
@@ -251,7 +251,7 @@ class IInstaller(IPlugin):
 
     """
 
-    def get_all_names():
+    def get_all_names():  # type: ignore
         """Returns all names that may be authenticated.
 
         :rtype: `collections.Iterable` of `str`
@@ -288,7 +288,7 @@ class IInstaller(IPlugin):
 
         """
 
-    def supported_enhancements():
+    def supported_enhancements():  # type: ignore
         """Returns a `collections.Iterable` of supported enhancements.
 
         :returns: supported enhancements which should be a subset of
@@ -326,7 +326,7 @@ class IInstaller(IPlugin):
 
         """
 
-    def recovery_routine():
+    def recovery_routine():  # type: ignore
         """Revert configuration to most recent finalized checkpoint.
 
         Remove all changes (temporary and permanent) that have not been
@@ -337,21 +337,21 @@ class IInstaller(IPlugin):
 
         """
 
-    def view_config_changes():
+    def view_config_changes():  # type: ignore
         """Display all of the LE config changes.
 
         :raises .PluginError: when config changes cannot be parsed
 
         """
 
-    def config_test():
+    def config_test():  # type: ignore
         """Make sure the configuration is valid.
 
         :raises .MisconfigurationError: when the config is not in a usable state
 
         """
 
-    def restart():
+    def restart():  # type: ignore
         """Restart or refresh the server content.
 
         :raises .PluginError: when server cannot be restarted

certbot/log.py

@@ -0,0 +1,180 @@
+"""Logging utilities for Certbot."""
+from __future__ import print_function
+import functools
+import logging
+import os
+import sys
+import time
+import traceback
+
+from acme import messages
+
+from certbot import cli
+from certbot import constants
+from certbot import errors
+from certbot import util
+
+# Logging format
+CLI_FMT = "%(message)s"
+FILE_FMT = "%(asctime)s:%(levelname)s:%(name)s:%(message)s"
+
+
+logger = logging.getLogger(__name__)
+
+
+def pre_arg_parse_setup():
+    """Ensures fatal exceptions are logged and reported to the user."""
+    sys.excepthook = functools.partial(except_hook, config=None)
+
+
+def post_arg_parse_setup(config):
+    """Setup logging after command line arguments are parsed.
+
+    :param certbot.interface.IConfig config: Configuration object
+
+    """
+    file_handler, file_path = setup_log_file_handler(
+        config, 'letsencrypt.log', FILE_FMT)
+
+    if config.quiet:
+        level = constants.QUIET_LOGGING_LEVEL
+    else:
+        level = -config.verbose_count * 10
+    stderr_handler = ColoredStreamHandler()
+    stderr_handler.setFormatter(logging.Formatter(CLI_FMT))
+    stderr_handler.setLevel(level)
+
+    root_logger = logging.getLogger()
+    root_logger.setLevel(logging.DEBUG)  # send all records to handlers
+    root_logger.addHandler(stderr_handler)
+    root_logger.addHandler(file_handler)
+
+    logger.debug('Root logging level set at %d', level)
+    logger.info('Saving debug log to %s', file_path)
+
+    sys.excepthook = functools.partial(except_hook, config=config)
+
+
+def setup_log_file_handler(config, logfile, fmt):
+    """Setup file debug logging.
+
+    :param certbot.interface.IConfig config: Configuration object
+    :param str logfile: basename for the log file
+    :param str fmt: logging format string
+
+    :returns: file handler and absolute path to the log file
+    :rtype: tuple
+
+    """
+    # TODO: logs might contain sensitive data such as contents of the
+    # private key! #525
+    util.make_or_verify_core_dir(
+        config.logs_dir, 0o700, os.geteuid(), config.strict_permissions)
+    log_file_path = os.path.join(config.logs_dir, logfile)
+    try:
+        handler = logging.handlers.RotatingFileHandler(
+            log_file_path, maxBytes=2 ** 20, backupCount=1000)
+    except IOError as error:
+        raise errors.Error(util.PERM_ERR_FMT.format(error))
+    # rotate on each invocation, rollover only possible when maxBytes
+    # is nonzero and backupCount is nonzero, so we set maxBytes as big
+    # as possible not to overrun in single CLI invocation (1MB).
+    handler.doRollover()  # TODO: creates empty letsencrypt.log.1 file
+    handler.setLevel(logging.DEBUG)
+    handler_formatter = logging.Formatter(fmt=fmt)
+    handler_formatter.converter = time.gmtime  # don't use localtime
+    handler.setFormatter(handler_formatter)
+    return handler, log_file_path
+
+
+class ColoredStreamHandler(logging.StreamHandler):
+    """Sends colored logging output to a stream.
+
+    If the specified stream is not a tty, the class works like the
+    standard logging.StreamHandler. Default red_level is logging.WARNING.
+
+    :ivar bool colored: True if output should be colored
+    :ivar bool red_level: The level at which to output
+
+    """
+
+    def __init__(self, stream=None):
+        if sys.version_info < (2, 7):
+            # pragma: no cover
+            # pylint: disable=non-parent-init-called
+            logging.StreamHandler.__init__(self, stream)
+        else:
+            super(ColoredStreamHandler, self).__init__(stream)
+        self.colored = (sys.stderr.isatty() if stream is None else
+                        stream.isatty())
+        self.red_level = logging.WARNING
+
+    def format(self, record):
+        """Formats the string representation of record.
+
+        :param logging.LogRecord record: Record to be formatted
+
+        :returns: Formatted, string representation of record
+        :rtype: str
+
+        """
+        out = (logging.StreamHandler.format(self, record)
+               if sys.version_info < (2, 7)
+               else super(ColoredStreamHandler, self).format(record))
+        if self.colored and record.levelno >= self.red_level:
+            return ''.join((util.ANSI_SGR_RED, out, util.ANSI_SGR_RESET))
+        else:
+            return out
+
+
+def except_hook(exc_type, exc_value, trace, config):
+    """Logs exceptions and reports them to the user.
+
+    Config is used to determine how to display exceptions to the user. In
+    general, if config.debug is True, then the full exception and traceback is
+    shown to the user, otherwise it is suppressed. If config itself is None,
+    then the traceback and exception is attempted to be written to a logfile.
+    If this is successful, the traceback is suppressed, otherwise it is shown
+    to the user. sys.exit is always called with a nonzero status.
+
+    """
+    tb_str = "".join(traceback.format_exception(exc_type, exc_value, trace))
+    logger.debug("Exiting abnormally:%s%s", os.linesep, tb_str)
+
+    if issubclass(exc_type, Exception) and (config is None or not config.debug):
+        if config is None:
+            logfile = "certbot.log"
+            try:
+                with open(logfile, "w") as logfd:
+                    traceback.print_exception(
+                        exc_type, exc_value, trace, file=logfd)
+                assert "--debug" not in sys.argv  # config is None if this explodes
+            except:  # pylint: disable=bare-except
+                sys.exit(tb_str)
+            if "--debug" in sys.argv:
+                sys.exit(tb_str)
+
+        if issubclass(exc_type, errors.Error):
+            sys.exit(exc_value)
+        else:
+            # Here we're passing a client or ACME error out to the client at the shell
+            # Tell the user a bit about what happened, without overwhelming
+            # them with a full traceback
+            err = traceback.format_exception_only(exc_type, exc_value)[0]
+            # Typical error from the ACME module:
+            # acme.messages.Error: urn:ietf:params:acme:error:malformed :: The
+            # request message was malformed :: Error creating new registration
+            # :: Validation of contact mailto:none@longrandomstring.biz failed:
+            # Server failure at resolver
+            if (messages.is_acme_error(err) and ":: " in err and
+                 config.verbose_count <= cli.flag_default("verbose_count")):
+                # prune ACME error code, we have a human description
+                _code, _sep, err = err.partition(":: ")
+            msg = "An unexpected error occurred:\n" + err + "Please see the "
+            if config is None:
+                msg += "logfile '{0}' for more details.".format(logfile)
+            else:
+                msg += "logfiles in {0} for more details.".format(config.logs_dir)
+            sys.exit(msg)
+    else:
+        sys.exit(tb_str)

certbot/main.py

@@ -1,47 +1,37 @@
 """Certbot main entry point."""
 from __future__ import print_function
-import atexit
-import functools
 import logging.handlers
 import os
 import sys
-import time
-import traceback
 
 import zope.component
 
 from acme import jose
-from acme import messages
 from acme import errors as acme_errors
 
 import certbot
 
 from certbot import account
 from certbot import cert_manager
-from certbot import client
 from certbot import cli
-from certbot import crypto_util
-from certbot import colored_logging
+from certbot import client
 from certbot import configuration
 from certbot import constants
+from certbot import crypto_util
 from certbot import eff
 from certbot import errors
 from certbot import hooks
 from certbot import interfaces
-from certbot import util
-from certbot import reporter
+from certbot import log
 from certbot import renewal
+from certbot import reporter
+from certbot import util
 
 from certbot.display import util as display_util, ops as display_ops
 from certbot.plugins import disco as plugins_disco
 from certbot.plugins import selection as plug_sel
 
 
-_PERM_ERR_FMT = os.linesep.join((
-    "The following error was encountered:", "{0}",
-    "If running as non-root, set --config-dir, "
-    "--work-dir, and --logs-dir to writeable paths."))
-
 USER_CANCELLED = ("User chose to cancel the operation and may "
                   "reinvoke the client.")
 
@@ -363,7 +353,7 @@ def _determine_account(config):
             acc = accounts[0]
         else:  # no account registered yet
             if config.email is None and not config.register_unsafely_without_email:
-                config.namespace.email = display_ops.get_email()
+                config.email = display_ops.get_email()
 
             def _tos_cb(regr):
                 if config.tos:
@@ -386,7 +376,7 @@ def _determine_account(config):
                 raise errors.Error(
                     "Unable to register an account with ACME server")
 
-    config.namespace.account = acc.id
+    config.account = acc.id
     return acc, acme
 
 
@@ -463,7 +453,7 @@ def register(config, unused_plugins):
             return ("--register-unsafely-without-email provided, however, a "
                     "new e-mail address must\ncurrently be provided when "
                     "updating a registration.")
-        config.namespace.email = display_ops.get_email(optional=False)
+        config.email = display_ops.get_email(optional=False)
 
     acc, acme = _determine_account(config)
     acme_client = client.Client(config, acc, None, None, acme=acme)
@@ -474,15 +464,16 @@ def register(config, unused_plugins):
     eff.handle_subscription(config)
     add_msg("Your e-mail address was updated to {0}.".format(config.email))
 
-def _install_cert(config, le_client, domains, lineage=None):
+def _install_cert(config, le_client, domains, lineage=None, enhance=True):
     path_provider = lineage if lineage else config
     assert path_provider.cert_path is not None
 
     le_client.deploy_certificate(domains, path_provider.key_path,
         path_provider.cert_path, path_provider.chain_path, path_provider.fullchain_path)
-    le_client.enhance_config(domains, path_provider.chain_path)
+    if enhance:
+        le_client.enhance_config(domains, path_provider.chain_path)
 
-def install(config, plugins):
+def install(config, plugins, lineage=None, enhance=True):
     """Install a previously obtained cert in a server."""
     # XXX: Update for renewer/RenewableCert
     # FIXME: be consistent about whether errors are raised or returned from
@@ -491,11 +482,11 @@ def install(config, plugins):
     try:
         installer, _ = plug_sel.choose_configurator_plugins(config, plugins, "install")
     except errors.PluginSelectionError as e:
-        return e.message
+        return str(e)
 
     domains, _ = _find_domains_or_certname(config, installer)
     le_client = _init_le_client(config, authenticator=None, installer=installer)
-    _install_cert(config, le_client, domains)
+    _install_cert(config, le_client, domains, lineage, enhance)
 
 
 def plugins_cmd(config, plugins):  # TODO: Use IDisplay rather than print
@@ -569,7 +560,7 @@ def certificates(config, unused_plugins):
 def revoke(config, unused_plugins):  # TODO: coop with renewal config
     """Revoke a previously obtained certificate."""
     # For user-agent construction
-    config.namespace.installer = config.namespace.authenticator = "None"
+    config.installer = config.authenticator = "None"
     if config.key_path is not None:  # revocation by cert key
         logger.debug("Revoking %s using cert key %s",
                      config.cert_path[0], config.key_path[0])
@@ -710,138 +701,11 @@ def renew(config, unused_plugins):
         hooks.run_saved_post_hooks()
 
 
-def setup_log_file_handler(config, logfile, fmt):
-    """Setup file debug logging."""
-    log_file_path = os.path.join(config.logs_dir, logfile)
-    try:
-        handler = logging.handlers.RotatingFileHandler(
-            log_file_path, maxBytes=2 ** 20, backupCount=1000)
-    except IOError as error:
-        raise errors.Error(_PERM_ERR_FMT.format(error))
-    # rotate on each invocation, rollover only possible when maxBytes
-    # is nonzero and backupCount is nonzero, so we set maxBytes as big
-    # as possible not to overrun in single CLI invocation (1MB).
-    handler.doRollover()  # TODO: creates empty letsencrypt.log.1 file
-    handler.setLevel(logging.DEBUG)
-    handler_formatter = logging.Formatter(fmt=fmt)
-    handler_formatter.converter = time.gmtime  # don't use localtime
-    handler.setFormatter(handler_formatter)
-    return handler, log_file_path
-
-
-def _cli_log_handler(level, fmt):
-    handler = colored_logging.StreamHandler()
-    handler.setFormatter(logging.Formatter(fmt))
-    handler.setLevel(level)
-    return handler
-
-
-def setup_logging(config):
-    """Sets up logging to logfiles and the terminal.
-
-    :param certbot.interface.IConfig config: Configuration object
-
-    """
-    cli_fmt = "%(message)s"
-    file_fmt = "%(asctime)s:%(levelname)s:%(name)s:%(message)s"
-    logfile = "letsencrypt.log"
-    if config.quiet:
-        level = constants.QUIET_LOGGING_LEVEL
-    else:
-        level = -config.verbose_count * 10
-    file_handler, log_file_path = setup_log_file_handler(
-        config, logfile=logfile, fmt=file_fmt)
-    cli_handler = _cli_log_handler(level, cli_fmt)
-
-    # TODO: use fileConfig?
-
-    root_logger = logging.getLogger()
-    root_logger.setLevel(logging.DEBUG)  # send all records to handlers
-    root_logger.addHandler(cli_handler)
-    root_logger.addHandler(file_handler)
-
-    logger.debug("Root logging level set at %d", level)
-    logger.info("Saving debug log to %s", log_file_path)
-
-
-def _handle_exception(exc_type, exc_value, trace, config):
-    """Logs exceptions and reports them to the user.
-
-    Config is used to determine how to display exceptions to the user. In
-    general, if config.debug is True, then the full exception and traceback is
-    shown to the user, otherwise it is suppressed. If config itself is None,
-    then the traceback and exception is attempted to be written to a logfile.
-    If this is successful, the traceback is suppressed, otherwise it is shown
-    to the user. sys.exit is always called with a nonzero status.
-
-    """
-    tb_str = "".join(traceback.format_exception(exc_type, exc_value, trace))
-    logger.debug("Exiting abnormally:%s%s", os.linesep, tb_str)
-
-    if issubclass(exc_type, Exception) and (config is None or not config.debug):
-        if config is None:
-            logfile = "certbot.log"
-            try:
-                with open(logfile, "w") as logfd:
-                    traceback.print_exception(
-                        exc_type, exc_value, trace, file=logfd)
-                assert "--debug" not in sys.argv  # config is None if this explodes
-            except:  # pylint: disable=bare-except
-                sys.exit(tb_str)
-            if "--debug" in sys.argv:
-                sys.exit(tb_str)
-
-        if issubclass(exc_type, errors.Error):
-            sys.exit(exc_value)
-        else:
-            # Here we're passing a client or ACME error out to the client at the shell
-            # Tell the user a bit about what happened, without overwhelming
-            # them with a full traceback
-            err = traceback.format_exception_only(exc_type, exc_value)[0]
-            # Typical error from the ACME module:
-            # acme.messages.Error: urn:ietf:params:acme:error:malformed :: The
-            # request message was malformed :: Error creating new registration
-            # :: Validation of contact mailto:none@longrandomstring.biz failed:
-            # Server failure at resolver
-            if (messages.is_acme_error(err) and ":: " in err and
-                 config.verbose_count <= cli.flag_default("verbose_count")):
-                # prune ACME error code, we have a human description
-                _code, _sep, err = err.partition(":: ")
-            msg = "An unexpected error occurred:\n" + err + "Please see the "
-            if config is None:
-                msg += "logfile '{0}' for more details.".format(logfile)
-            else:
-                msg += "logfiles in {0} for more details.".format(config.logs_dir)
-            sys.exit(msg)
-    else:
-        sys.exit(tb_str)
-
-
-def make_or_verify_core_dir(directory, mode, uid, strict):
-    """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 the directory cannot be made or verified
-
-    """
-    try:
-        util.make_or_verify_dir(directory, mode, uid, strict)
-    except OSError as error:
-        raise errors.Error(_PERM_ERR_FMT.format(error))
-
 def make_or_verify_needed_dirs(config):
-    """Create or verify existence of config, work, or logs directories"""
-    make_or_verify_core_dir(config.config_dir, constants.CONFIG_DIRS_MODE,
+    """Create or verify existence of config and work directories"""
+    util.make_or_verify_core_dir(config.config_dir, constants.CONFIG_DIRS_MODE,
                             os.geteuid(), config.strict_permissions)
-    make_or_verify_core_dir(config.work_dir, constants.CONFIG_DIRS_MODE,
-                            os.geteuid(), config.strict_permissions)
-    # TODO: logs might contain sensitive data such as contents of the
-    # private key! #525
-    make_or_verify_core_dir(config.logs_dir, 0o700,
+    util.make_or_verify_core_dir(config.work_dir, constants.CONFIG_DIRS_MODE,
                             os.geteuid(), config.strict_permissions)
 
 
@@ -874,7 +738,7 @@ def _post_logging_setup(config, plugins, cli_args):
 
 def main(cli_args=sys.argv[1:]):
     """Command line argument parsing and main script execution."""
-    sys.excepthook = functools.partial(_handle_exception, config=None)
+    log.pre_arg_parse_setup()
     plugins = plugins_disco.PluginsRegistry.find_all()
 
     # note: arg parser internally handles --help (and exits afterwards)
@@ -884,20 +748,16 @@ def main(cli_args=sys.argv[1:]):
 
     make_or_verify_needed_dirs(config)
 
-    # Setup logging ASAP, otherwise "No handlers could be found for
-    # logger ..." TODO: this should be done before plugins discovery
-    setup_logging(config)
+    log.post_arg_parse_setup(config)
 
     _post_logging_setup(config, plugins, cli_args)
 
-    sys.excepthook = functools.partial(_handle_exception, config=config)
-
     set_displayer(config)
 
     # Reporter
     report = reporter.Reporter(config)
     zope.component.provideUtility(report)
-    atexit.register(report.atexit_print_messages)
+    util.atexit_register(report.print_messages)
 
     return config.func(config, plugins)
 

certbot/plugins/disco.py

@@ -27,7 +27,7 @@ class PluginEntryPoint(object):
     """Distributions for which prefix will be omitted."""
 
     # this object is mutable, don't allow it to be hashed!
-    __hash__ = None
+    __hash__ = None  # type: ignore
 
     def __init__(self, entry_point):
         self.name = self.entry_point_to_plugin_name(entry_point)

certbot/plugins/selection.py

@@ -137,9 +137,8 @@ noninstaller_plugins = ["webroot", "manual", "standalone"]
 
 def record_chosen_plugins(config, plugins, auth, inst):
     "Update the config entries to reflect the plugins we actually selected."
-    cn = config.namespace
-    cn.authenticator = plugins.find_init(auth).name if auth else "None"
-    cn.installer = plugins.find_init(inst).name if inst else "None"
+    config.authenticator = plugins.find_init(auth).name if auth else "None"
+    config.installer = plugins.find_init(inst).name if inst else "None"
 
 
 def choose_configurator_plugins(config, plugins, verb):

certbot/renewal.py

@@ -103,13 +103,13 @@ def _restore_webroot_config(config, renewalparams):
     """
     if "webroot_map" in renewalparams:
         if not cli.set_by_cli("webroot_map"):
-            config.namespace.webroot_map = renewalparams["webroot_map"]
+            config.webroot_map = renewalparams["webroot_map"]
     elif "webroot_path" in renewalparams:
         logger.debug("Ancient renewal conf file without webroot-map, restoring webroot-path")
         wp = renewalparams["webroot_path"]
         if isinstance(wp, str):  # prior to 0.1.0, webroot_path was a string
             wp = [wp]
-        config.namespace.webroot_path = wp
+        config.webroot_path = wp
 
 
 def _restore_plugin_configs(config, renewalparams):
@@ -148,10 +148,10 @@ def _restore_plugin_configs(config, renewalparams):
                 if config_value in ("None", "True", "False"):
                     # bool("False") == True
                     # pylint: disable=eval-used
-                    setattr(config.namespace, config_item, eval(config_value))
+                    setattr(config, config_item, eval(config_value))
                 else:
                     cast = cli.argparse_type(config_item)
-                    setattr(config.namespace, config_item, cast(config_value))
+                    setattr(config, config_item, cast(config_value))
 
 
 def restore_required_config_elements(config, renewalparams):
@@ -172,7 +172,7 @@ def restore_required_config_elements(config, renewalparams):
     for item_name, restore_func in required_items:
         if item_name in renewalparams and not cli.set_by_cli(item_name):
             value = restore_func(item_name, renewalparams[item_name])
-            setattr(config.namespace, item_name, value)
+            setattr(config, item_name, value)
 
 
 def _restore_pref_challs(unused_name, value):

certbot/reporter.py

@@ -3,11 +3,10 @@ from __future__ import print_function
 
 import collections
 import logging
-import os
 import sys
 import textwrap
 
-from six.moves import queue  # pylint: disable=import-error
+from six.moves import queue  # type: ignore  # pylint: disable=import-error
 import zope.interface
 
 from certbot import interfaces
@@ -16,11 +15,6 @@ from certbot import util
 
 logger = logging.getLogger(__name__)
 
-# Store the pid of the process that first imported this module so that
-# atexit_print_messages side-effects such as error reporting can be limited to
-# this process and not any fork()'d children.
-INITIAL_PID = os.getpid()
-
 
 @zope.interface.implementer(interfaces.IReporter)
 class Reporter(object):
@@ -60,19 +54,6 @@ class Reporter(object):
         self.messages.put(self._msg_type(priority, msg, on_crash))
         logger.debug("Reporting to user: %s", msg)
 
-    def atexit_print_messages(self, pid=None):
-        """Function to be registered with atexit to print messages.
-
-        :param int pid: Process ID
-
-        """
-        if pid is None:
-            pid = INITIAL_PID
-        # This ensures that messages are only printed from the process that
-        # created the Reporter.
-        if pid == os.getpid():
-            self.print_messages()
-
     def print_messages(self):
         """Prints messages to the user and clears the message queue.
 

certbot/storage.py

@@ -19,6 +19,9 @@ from certbot import errors
 from certbot import error_handler
 from certbot import util
 
+from certbot.plugins import common as plugins_common
+from certbot.plugins import disco as plugins_disco
+
 logger = logging.getLogger(__name__)
 
 ALL_FOUR = ("cert", "privkey", "chain", "fullchain")
@@ -31,7 +34,9 @@ def renewal_conf_files(config):
     return glob.glob(os.path.join(config.renewal_configs_dir, "*.conf"))
 
 def renewal_file_for_certname(config, certname):
-    """Return /path/to/certname.conf in the renewal conf directory"""
+    """Return /path/to/certname.conf in the renewal conf directory
+    :raises .CertStorageError: if file is missing
+    """
     path = os.path.join(config.renewal_configs_dir, "{0}.conf".format(certname))
     if not os.path.exists(path):
         raise errors.CertStorageError("No certificate found with name {0} (expected "
@@ -127,6 +132,8 @@ def rename_renewal_config(prev_name, new_name, cli_config):
     except OSError:
         raise errors.ConfigurationError("Please specify a valid filename "
             "for the new certificate name.")
+    else:
+        return new_filename
 
 
 def update_configuration(lineagename, archive_dir, target, cli_config):
@@ -143,19 +150,25 @@ def update_configuration(lineagename, archive_dir, target, cli_config):
 
     """
     config_filename = renewal_filename_for_lineagename(cli_config, lineagename)
-    temp_filename = config_filename + ".new"
+
+    def _save_renewal_values(unused_config, temp_filename):
+        # Save only the config items that are relevant to renewal
+        values = relevant_values(vars(cli_config.namespace))
+        write_renewal_config(config_filename, temp_filename, archive_dir, target, values)
+    _modify_config_with_tempfile(config_filename, _save_renewal_values)
+
+    return configobj.ConfigObj(config_filename)
+
+def _modify_config_with_tempfile(filename, function):
+    temp_filename = filename + ".new"
 
     # If an existing tempfile exists, delete it
     if os.path.exists(temp_filename):
         os.unlink(temp_filename)
 
-    # Save only the config items that are relevant to renewal
-    values = relevant_values(vars(cli_config.namespace))
-    write_renewal_config(config_filename, temp_filename, archive_dir, target, values)
-    os.rename(temp_filename, config_filename)
-
-    return configobj.ConfigObj(config_filename)
-
+    config = configobj.ConfigObj(filename)
+    function(config, temp_filename)
+    os.rename(temp_filename, filename)
 
 def get_link_target(link):
     """Get an absolute path to the target of link.
@@ -179,13 +192,12 @@ def _relevant(option):
 
     :rtype: bool
     """
-    # The list() here produces a list of the plugin names as strings.
     from certbot import renewal
-    from certbot.plugins import disco as plugins_disco
-    plugins = list(plugins_disco.PluginsRegistry.find_all())
+    plugins = plugins_disco.PluginsRegistry.find_all()
+    namespaces = [plugins_common.dest_namespace(plugin) for plugin in plugins]
 
     return (option in renewal.CONFIG_ITEMS or
-            any(option.startswith(x + "_") for x in plugins))
+            any(option.startswith(namespace) for namespace in namespaces))
 
 
 def relevant_values(all_values):
@@ -241,6 +253,7 @@ def delete_files(config, certname):
     """Delete all files related to the certificate.
 
     If some files are not found, ignore them and continue.
+    :raises .CertStorageError: if lineage is missing
     """
     renewal_filename = renewal_file_for_certname(config, certname)
     # file exists
@@ -302,6 +315,79 @@ def delete_files(config, certname):
     except OSError:
         logger.debug("Unable to remove %s", archive_path)
 
+def duplicate_lineage(config, certname, new_certname):
+    """Create a duplicate of certname with name new_certname
+
+    :raises .CertStorageError: for storage errors
+    :raises .ConfigurationError: for cli and renewal configuration errors
+    :raises IOError: for filename errors
+    :raises OSError: for OS errors
+    """
+
+    # copy renewal config file
+    prev_filename = renewal_filename_for_lineagename(config, certname)
+    new_filename = renewal_filename_for_lineagename(config, new_certname)
+    if os.path.exists(new_filename):
+        raise errors.ConfigurationError("The new certificate name "
+            "is already in use.")
+    try:
+        shutil.copy2(prev_filename, new_filename)
+    except (OSError, IOError):
+        raise errors.ConfigurationError("Please specify a valid filename "
+            "for the new certificate name.")
+    logger.debug("Copied %s to %s", prev_filename, new_filename)
+
+    # load config file
+    try:
+        renewal_config = configobj.ConfigObj(new_filename)
+    except configobj.ConfigObjError:
+        # config is corrupted
+        logger.warning("Could not parse %s. Only the certificate has been renamed.",
+            new_filename)
+        raise errors.CertStorageError(
+            "error parsing {0}".format(new_filename))
+
+    def copy_to_new_dir(prev_dir):
+        """Replace certname with new_certname in prev_dir"""
+        new_dir = prev_dir.replace(certname, new_certname)
+        # make dir iff it doesn't exist
+        shutil.copytree(prev_dir, new_dir, symlinks=True)
+        logger.debug("Copied %s to %s", prev_dir, new_dir)
+        return new_dir
+
+    # archive dir
+    prev_archive_dir = _full_archive_path(renewal_config, config, certname)
+    new_archive_dir = prev_archive_dir
+    if not certname in prev_archive_dir:
+        raise errors.CertStorageError("Archive directory does not conform to defaults: "
+            "{0} not in {1}", certname, prev_archive_dir)
+    else:
+        new_archive_dir = copy_to_new_dir(prev_archive_dir)
+
+    # live dir
+    # if things aren't in their default places, don't try to change things.
+    prev_live_dir = _full_live_path(config, certname)
+    prev_links = dict((kind, renewal_config.get(kind)) for kind in ALL_FOUR)
+    if (certname not in prev_live_dir or
+            len(set(os.path.dirname(renewal_config.get(kind)) for kind in ALL_FOUR)) != 1):
+        raise errors.CertStorageError("Live directory does not conform to defaults.")
+    else:
+        copy_to_new_dir(prev_live_dir)
+        new_links = dict((k, prev_links[k].replace(certname, new_certname)) for k in prev_links)
+
+    # Update renewal config file
+    def _update_and_write(renewal_config, temp_filename):
+        renewal_config["archive_dir"] = new_archive_dir
+        renewal_config["version"] = certbot.__version__
+        for kind in ALL_FOUR:
+            renewal_config[kind] = new_links[kind]
+        with open(temp_filename, "wb") as f:
+            renewal_config.write(outfile=f)
+    _modify_config_with_tempfile(new_filename, _update_and_write)
+
+    # Update symlinks
+    return RenewableCert(new_filename, config, update_symlinks=True)
+
 
 class RenewableCert(object):
     # pylint: disable=too-many-instance-attributes,too-many-public-methods

certbot/tests/account_test.py

@@ -4,7 +4,6 @@ import json
 import os
 import shutil
 import stat
-import tempfile
 import unittest
 
 import mock
@@ -17,6 +16,8 @@ from certbot import errors
 
 from certbot.tests import util
 
+from certbot.tests.util import TempDirTestCase
+
 
 KEY = jose.JWKRSA.load(util.load_vector("rsa512_key_2.pem"))
 
@@ -98,13 +99,14 @@ class AccountMemoryStorageTest(unittest.TestCase):
         self.assertEqual([account], self.storage.find_all())
 
 
-class AccountFileStorageTest(unittest.TestCase):
+class AccountFileStorageTest(TempDirTestCase):
     """Tests for certbot.account.AccountFileStorage."""
 
     def setUp(self):
-        self.tmp = tempfile.mkdtemp()
+        super(AccountFileStorageTest, self).setUp()
+
         self.config = mock.MagicMock(
-            accounts_dir=os.path.join(self.tmp, "accounts"))
+            accounts_dir=os.path.join(self.tempdir, "accounts"))
         from certbot.account import AccountFileStorage
         self.storage = AccountFileStorage(self.config)
 
@@ -118,9 +120,6 @@ class AccountFileStorageTest(unittest.TestCase):
         self.mock_client = mock.MagicMock()
         self.mock_client.directory.new_authz = new_authzr_uri
 
-    def tearDown(self):
-        shutil.rmtree(self.tmp)
-
     def test_init_creates_dir(self):
         self.assertTrue(os.path.isdir(self.config.accounts_dir))
 

certbot/tests/auth_handler_test.py

@@ -5,6 +5,7 @@ import unittest
 
 import mock
 import six
+import zope.component
 
 from acme import challenges
 from acme import client as acme_client
@@ -12,6 +13,7 @@ from acme import messages
 
 from certbot import achallenges
 from certbot import errors
+from certbot import interfaces
 from certbot import util
 
 from certbot.tests import acme_util
@@ -65,6 +67,12 @@ class GetAuthorizationsTest(unittest.TestCase):
     def setUp(self):
         from certbot.auth_handler import AuthHandler
 
+        self.mock_display = mock.Mock()
+        zope.component.provideUtility(
+            self.mock_display, interfaces.IDisplay)
+        zope.component.provideUtility(
+            mock.Mock(debug_challenges=False), interfaces.IConfig)
+
         self.mock_auth = mock.MagicMock(name="ApacheConfigurator")
 
         self.mock_auth.get_chall_pref.return_value = [challenges.TLSSNI01]
@@ -157,6 +165,20 @@ class GetAuthorizationsTest(unittest.TestCase):
 
         self.assertEqual(len(authzr), 3)
 
+    @mock.patch("certbot.auth_handler.AuthHandler._poll_challenges")
+    def test_debug_challenges(self, mock_poll):
+        zope.component.provideUtility(
+            mock.Mock(debug_challenges=True), interfaces.IConfig)
+        self.mock_net.request_domain_challenges.side_effect = functools.partial(
+            gen_dom_authzr, challs=acme_util.CHALLENGES)
+
+        mock_poll.side_effect = self._validate_all
+
+        self.handler.get_authorizations(["0"])
+
+        self.assertEqual(self.mock_net.answer_challenge.call_count, 1)
+        self.assertEqual(self.mock_display.notification.call_count, 1)
+
     def test_perform_failure(self):
         self.mock_net.request_domain_challenges.side_effect = functools.partial(
             gen_dom_authzr, challs=acme_util.CHALLENGES)

certbot/tests/cert_manager_test.py

@@ -18,11 +18,14 @@ from certbot.storage import ALL_FOUR
 from certbot.tests import storage_test
 from certbot.tests import util as test_util
 
-class BaseCertManagerTest(unittest.TestCase):
+from certbot.tests.util import TempDirTestCase
+
+
+class BaseCertManagerTest(TempDirTestCase):
     """Base class for setting up Cert Manager tests.
     """
     def setUp(self):
-        self.tempdir = tempfile.mkdtemp()
+        super(BaseCertManagerTest, self).setUp()
 
         os.makedirs(os.path.join(self.tempdir, "renewal"))
 
@@ -68,9 +71,6 @@ class BaseCertManagerTest(unittest.TestCase):
         config.write()
         return config
 
-    def tearDown(self):
-        shutil.rmtree(self.tempdir)
-
 
 class UpdateLiveSymlinksTest(BaseCertManagerTest):
     """Tests for certbot.cert_manager.update_live_symlinks
@@ -384,14 +384,19 @@ class RenameLineageTest(BaseCertManagerTest):
     @test_util.patch_get_utility()
     @mock.patch('certbot.cert_manager.lineage_for_certname')
     def test_no_existing_certname(self, mock_lineage_for_certname, unused_get_utility):
-        mock_config = mock.Mock(certname="one", new_certname="two")
+        mock_config = mock.Mock(certname="one", new_certname="two",
+            renewal_configs_dir="/tmp/etc/letsencrypt/renewal/")
         mock_lineage_for_certname.return_value = None
-        self.assertRaises(errors.ConfigurationError,
-            self._call, mock_config)
+        self.assertRaises(errors.ConfigurationError, self._call, mock_config)
 
+    @mock.patch("certbot.storage.RenewableCert._update_symlinks")
     @test_util.patch_get_utility()
     @mock.patch("certbot.storage.RenewableCert._check_symlinks")
-    def test_rename_cert(self, mock_check, unused_get_utility):
+    @mock.patch("certbot.storage.relevant_values")
+    def test_rename_cert(self, mock_rv, mock_check, unused_get_utility, unused_update_symlinks):
+        # Mock relevant_values() to claim that all values are relevant here
+        # (to avoid instantiating parser)
+        mock_rv.side_effect = lambda x: x
         mock_check.return_value = True
         mock_config = self.mock_config
         self._call(mock_config)
@@ -400,9 +405,15 @@ class RenameLineageTest(BaseCertManagerTest):
         self.assertTrue(updated_lineage is not None)
         self.assertEqual(updated_lineage.lineagename, mock_config.new_certname)
 
+    @mock.patch("certbot.storage.RenewableCert._update_symlinks")
     @test_util.patch_get_utility()
     @mock.patch("certbot.storage.RenewableCert._check_symlinks")
-    def test_rename_cert_interactive_certname(self, mock_check, mock_get_utility):
+    @mock.patch("certbot.storage.relevant_values")
+    def test_rename_cert_interactive_certname(self, mock_rv, mock_check, mock_get_utility,
+        unused_update_symlinks):
+        # python 3.4 and 3.5 order things differently, so remove other.com for this test
+        os.remove(self.configs["other.com"].filename)
+        mock_rv.side_effect = lambda x: x
         mock_check.return_value = True
         mock_config = self.mock_config
         mock_config.certname = None
@@ -437,9 +448,6 @@ class DuplicativeCertsTest(storage_test.BaseRenewableCertTest):
         self.config.write()
         self._write_out_ex_kinds()
 
-    def tearDown(self):
-        shutil.rmtree(self.tempdir)
-
     @mock.patch('certbot.util.make_or_verify_dir')
     def test_find_duplicative_names(self, unused_makedir):
         from certbot.cert_manager import find_duplicative_certs

certbot/tests/cli_test.py

@@ -15,17 +15,18 @@ from certbot import constants
 from certbot import errors
 from certbot.plugins import disco
 
+from certbot.tests.util import TempDirTestCase
+
 PLUGINS = disco.PluginsRegistry.find_all()
 
 
-class TestReadFile(unittest.TestCase):
+class TestReadFile(TempDirTestCase):
     '''Test cli.read_file'''
 
     _multiprocess_can_split_ = True
 
     def test_read_file(self):
-        tmp_dir = tempfile.mkdtemp()
-        rel_test_path = os.path.relpath(os.path.join(tmp_dir, 'foo'))
+        rel_test_path = os.path.relpath(os.path.join(self.tempdir, 'foo'))
         self.assertRaises(
             argparse.ArgumentTypeError, cli.read_file, rel_test_path)
 

certbot/tests/client_test.py

@@ -7,6 +7,7 @@ import unittest
 import OpenSSL
 import mock
 
+from acme import errors as acme_errors
 from acme import jose
 
 from certbot import account
@@ -101,7 +102,7 @@ class RegisterTest(unittest.TestCase):
                     self.config.register_unsafely_without_email = True
                     self.config.dry_run = False
                     self._call()
-                    mock_logger.warning.assert_called_once_with(mock.ANY)
+                    mock_logger.info.assert_called_once_with(mock.ANY)
                     self.assertTrue(mock_handle.called)
 
     def test_unsupported_error(self):
@@ -170,7 +171,9 @@ class ClientTest(ClientTestCommon):
         self.acme.fetch_chain.assert_called_once_with(mock.sentinel.certr)
 
     @mock.patch("certbot.client.logger")
-    def test_obtain_certificate_from_csr(self, mock_logger):
+    @test_util.patch_get_utility()
+    def test_obtain_certificate_from_csr(self, unused_mock_get_utility,
+                                         mock_logger):
         self._mock_obtain_certificate()
         test_csr = util.CSR(form="der", file=None, data=CSR_SAN)
         auth_handler = self.client.auth_handler
@@ -203,8 +206,44 @@ class ClientTest(ClientTestCommon):
             test_csr)
         mock_logger.warning.assert_called_once_with(mock.ANY)
 
+    @test_util.patch_get_utility()
+    def test_obtain_certificate_from_csr_retry_succeeded(
+            self, mock_get_utility):
+        self._mock_obtain_certificate()
+        self.acme.fetch_chain.side_effect = [acme_errors.Error,
+                                             mock.sentinel.chain]
+        test_csr = util.CSR(form="der", file=None, data=CSR_SAN)
+        auth_handler = self.client.auth_handler
+
+        authzr = auth_handler.get_authorizations(self.eg_domains, False)
+        self.assertEqual(
+            (mock.sentinel.certr, mock.sentinel.chain),
+            self.client.obtain_certificate_from_csr(
+                self.eg_domains,
+                test_csr,
+                authzr=authzr))
+        self.assertEqual(1, mock_get_utility().notification.call_count)
+
+    @test_util.patch_get_utility()
+    def test_obtain_certificate_from_csr_retry_failed(self, mock_get_utility):
+        self._mock_obtain_certificate()
+        self.acme.fetch_chain.side_effect = acme_errors.Error
+        test_csr = util.CSR(form="der", file=None, data=CSR_SAN)
+        auth_handler = self.client.auth_handler
+
+        authzr = auth_handler.get_authorizations(self.eg_domains, False)
+        self.assertRaises(
+            acme_errors.Error,
+            self.client.obtain_certificate_from_csr,
+            self.eg_domains,
+            test_csr,
+            authzr=authzr)
+        self.assertEqual(1, mock_get_utility().notification.call_count)
+
     @mock.patch("certbot.client.crypto_util")
-    def test_obtain_certificate(self, mock_crypto_util):
+    @test_util.patch_get_utility()
+    def test_obtain_certificate(self, unused_mock_get_utility,
+                                mock_crypto_util):
         self._mock_obtain_certificate()
 
         csr = util.CSR(form="der", file=None, data=CSR_SAN)

certbot/tests/colored_logging_test.py

@@ -1,41 +0,0 @@
-"""Tests for certbot.colored_logging."""
-import logging
-import unittest
-
-import six
-
-from certbot import util
-
-
-class StreamHandlerTest(unittest.TestCase):
-    """Tests for certbot.colored_logging."""
-
-    def setUp(self):
-        from certbot import colored_logging
-
-        self.stream = six.StringIO()
-        self.stream.isatty = lambda: True
-        self.handler = colored_logging.StreamHandler(self.stream)
-
-        self.logger = logging.getLogger()
-        self.logger.setLevel(logging.DEBUG)
-        self.logger.addHandler(self.handler)
-
-    def test_format(self):
-        msg = 'I did a thing'
-        self.logger.debug(msg)
-        self.assertEqual(self.stream.getvalue(), '{0}\n'.format(msg))
-
-    def test_format_and_red_level(self):
-        msg = 'I did another thing'
-        self.handler.red_level = logging.DEBUG
-        self.logger.debug(msg)
-
-        self.assertEqual(self.stream.getvalue(),
-                         '{0}{1}{2}\n'.format(util.ANSI_SGR_RED,
-                                              msg,
-                                              util.ANSI_SGR_RESET))
-
-
-if __name__ == "__main__":
-    unittest.main()  # pragma: no cover

certbot/tests/configuration_test.py

@@ -120,6 +120,12 @@ class NamespaceConfigTest(unittest.TestCase):
         self.assertTrue(os.path.isabs(config.live_dir))
         self.assertTrue(os.path.isabs(config.renewal_configs_dir))
 
+    def test_get_and_set_attr(self):
+        self.config.foo = 42
+        self.assertEqual(self.config.namespace.foo, 42)
+        self.config.namespace.bar = 1337
+        self.assertEqual(self.config.bar, 1337)
+
 
 if __name__ == '__main__':
     unittest.main()  # pragma: no cover

certbot/tests/crypto_util_test.py

@@ -1,7 +1,6 @@
 """Tests for certbot.crypto_util."""
 import logging
-import shutil
-import tempfile
+import os
 import unittest
 
 import OpenSSL
@@ -21,17 +20,20 @@ CERT = test_util.load_vector('cert.pem')
 SAN_CERT = test_util.load_vector('cert-san.pem')
 
 
-class InitSaveKeyTest(unittest.TestCase):
+class InitSaveKeyTest(test_util.TempDirTestCase):
     """Tests for certbot.crypto_util.init_save_key."""
     def setUp(self):
+        super(InitSaveKeyTest, self).setUp()
+
         logging.disable(logging.CRITICAL)
         zope.component.provideUtility(
-            mock.Mock(strict_permissions=True), interfaces.IConfig)
-        self.key_dir = tempfile.mkdtemp('key_dir')
+            mock.Mock(strict_permissions=True, dry_run=False),
+            interfaces.IConfig)
 
     def tearDown(self):
+        super(InitSaveKeyTest, self).tearDown()
+
         logging.disable(logging.NOTSET)
-        shutil.rmtree(self.key_dir)
 
     @classmethod
     def _call(cls, key_size, key_dir):
@@ -41,41 +43,68 @@ class InitSaveKeyTest(unittest.TestCase):
     @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.key_dir)
+        key = self._call(1024, self.tempdir)
         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)))
+
+    @mock.patch('certbot.crypto_util.make_key')
+    def test_success_dry_run(self, mock_make):
+        zope.component.provideUtility(
+            mock.Mock(strict_permissions=True, dry_run=True),
+            interfaces.IConfig)
+        mock_make.return_value = b'key_pem'
+        key = self._call(1024, self.tempdir)
+        self.assertEqual(key.pem, b'key_pem')
+        self.assertTrue(key.file is None)
 
     @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.key_dir)
+        self.assertRaises(ValueError, self._call, 431, self.tempdir)
 
 
-class InitSaveCSRTest(unittest.TestCase):
+class InitSaveCSRTest(test_util.TempDirTestCase):
     """Tests for certbot.crypto_util.init_save_csr."""
 
     def setUp(self):
-        zope.component.provideUtility(
-            mock.Mock(strict_permissions=True), interfaces.IConfig)
-        self.csr_dir = tempfile.mkdtemp('csr_dir')
+        super(InitSaveCSRTest, self).setUp()
 
-    def tearDown(self):
-        shutil.rmtree(self.csr_dir)
+        zope.component.provideUtility(
+            mock.Mock(strict_permissions=True, dry_run=False),
+            interfaces.IConfig)
 
     @mock.patch('certbot.crypto_util.make_csr')
     @mock.patch('certbot.crypto_util.util.make_or_verify_dir')
-    def test_it(self, unused_mock_verify, mock_csr):
+    def test_success(self, unused_mock_verify, mock_csr):
         from certbot.crypto_util import init_save_csr
 
         mock_csr.return_value = (b'csr_pem', b'csr_der')
 
         csr = init_save_csr(
-            mock.Mock(pem='dummy_key'), 'example.com', self.csr_dir,
+            mock.Mock(pem='dummy_key'), 'example.com', self.tempdir,
             'csr-certbot.pem')
 
         self.assertEqual(csr.data, b'csr_der')
         self.assertTrue('csr-certbot.pem' in csr.file)
 
+    @mock.patch('certbot.crypto_util.make_csr')
+    @mock.patch('certbot.crypto_util.util.make_or_verify_dir')
+    def test_success_dry_run(self, unused_mock_verify, mock_csr):
+        from certbot.crypto_util import init_save_csr
+
+        zope.component.provideUtility(
+            mock.Mock(strict_permissions=True, dry_run=True),
+            interfaces.IConfig)
+        mock_csr.return_value = (b'csr_pem', b'csr_der')
+
+        csr = init_save_csr(
+            mock.Mock(pem='dummy_key'), 'example.com', self.tempdir,
+            'csr-certbot.pem')
+
+        self.assertEqual(csr.data, b'csr_der')
+        self.assertTrue(csr.file is None)
+
 
 class MakeCSRTest(unittest.TestCase):
     """Tests for certbot.crypto_util.make_csr."""

certbot/tests/display/completer_test.py

@@ -1,31 +1,30 @@
 """Test certbot.display.completer."""
 import os
 import readline
-import shutil
 import string
 import sys
-import tempfile
 import unittest
 
 import mock
 from six.moves import reload_module  # pylint: disable=import-error
 
+from certbot.tests.util import TempDirTestCase
 
-class CompleterTest(unittest.TestCase):
+class CompleterTest(TempDirTestCase):
     """Test certbot.display.completer.Completer."""
 
     def setUp(self):
-        self.temp_dir = tempfile.mkdtemp()
+        super(CompleterTest, self).setUp()
 
         # directories must end with os.sep for completer to
         # search inside the directory for possible completions
-        if self.temp_dir[-1] != os.sep:
-            self.temp_dir += os.sep
+        if self.tempdir[-1] != os.sep:
+            self.tempdir += os.sep
 
         self.paths = []
         # create some files and directories in temp_dir
         for c in string.ascii_lowercase:
-            path = os.path.join(self.temp_dir, c)
+            path = os.path.join(self.tempdir, c)
             self.paths.append(path)
             if ord(c) % 2:
                 os.mkdir(path)
@@ -33,21 +32,18 @@ class CompleterTest(unittest.TestCase):
                 with open(path, 'w'):
                     pass
 
-    def tearDown(self):
-        shutil.rmtree(self.temp_dir)
-
     def test_complete(self):
         from certbot.display import completer
         my_completer = completer.Completer()
         num_paths = len(self.paths)
 
         for i in range(num_paths):
-            completion = my_completer.complete(self.temp_dir, i)
+            completion = my_completer.complete(self.tempdir, i)
             self.assertTrue(completion in self.paths)
             self.paths.remove(completion)
 
         self.assertFalse(self.paths)
-        completion = my_completer.complete(self.temp_dir, num_paths)
+        completion = my_completer.complete(self.tempdir, num_paths)
         self.assertEqual(completion, None)
 
     def test_import_error(self):

certbot/tests/display/ops_test.py

@@ -2,7 +2,6 @@
 """Test certbot.display.ops."""
 import os
 import sys
-import tempfile
 import unittest
 
 import mock
@@ -87,18 +86,19 @@ class GetEmailTest(unittest.TestCase):
             self.assertTrue(invalid_txt in mock_input.call_args[0][0])
 
 
-class ChooseAccountTest(unittest.TestCase):
+class ChooseAccountTest(test_util.TempDirTestCase):
     """Tests for certbot.display.ops.choose_account."""
     def setUp(self):
+        super(ChooseAccountTest, self).setUp()
+
         zope.component.provideUtility(display_util.FileDisplay(sys.stdout,
                                                                False))
 
-        self.accounts_dir = tempfile.mkdtemp("accounts")
-        self.account_keys_dir = os.path.join(self.accounts_dir, "keys")
+        self.account_keys_dir = os.path.join(self.tempdir, "keys")
         os.makedirs(self.account_keys_dir, 0o700)
 
         self.config = mock.MagicMock(
-            accounts_dir=self.accounts_dir,
+            accounts_dir=self.tempdir,
             account_keys_dir=self.account_keys_dir,
             server="certbot-demo.org")
         self.key = KEY

certbot/tests/log_test.py

@@ -0,0 +1,199 @@
+"""Tests for certbot.log."""
+import logging
+import traceback
+import logging.handlers
+import os
+import sys
+import time
+import unittest
+
+import mock
+import six
+
+from acme import messages
+
+from certbot import constants
+from certbot import errors
+from certbot import util
+from certbot.tests import util as test_util
+
+
+class PreArgParseSetupTest(unittest.TestCase):
+    """Tests for certbot.log.pre_arg_parse_setup."""
+
+    @classmethod
+    def _call(cls, *args, **kwargs):
+        from certbot.log import pre_arg_parse_setup
+        return pre_arg_parse_setup(*args, **kwargs)
+
+    def test_it(self):
+        with mock.patch('certbot.log.except_hook') as mock_except_hook:
+            with mock.patch('certbot.log.sys') as mock_sys:
+                self._call()
+
+        mock_sys.excepthook(1, 2, 3)
+        mock_except_hook.assert_called_once_with(1, 2, 3, config=None)
+
+
+class PostArgParseSetupTest(test_util.TempDirTestCase):
+    """Tests for certbot.log.post_arg_parse_setup."""
+
+    @classmethod
+    def _call(cls, *args, **kwargs):
+        from certbot.log import post_arg_parse_setup
+        return post_arg_parse_setup(*args, **kwargs)
+
+    def setUp(self):
+        super(PostArgParseSetupTest, self).setUp()
+        self.config = mock.MagicMock(
+            logs_dir=self.tempdir, quiet=False,
+            verbose_count=constants.CLI_DEFAULTS['verbose_count'])
+        self.root_logger = mock.MagicMock()
+
+    def test_common(self):
+        with mock.patch('certbot.log.logging.getLogger') as mock_get_logger:
+            mock_get_logger.return_value = self.root_logger
+            with mock.patch('certbot.log.except_hook') as mock_except_hook:
+                with mock.patch('certbot.log.sys') as mock_sys:
+                    mock_sys.version_info = sys.version_info
+                    self._call(self.config)
+
+        self.assertEqual(self.root_logger.addHandler.call_count, 2)
+        self.assertTrue(os.path.exists(os.path.join(
+            self.config.logs_dir, 'letsencrypt.log')))
+        mock_sys.excepthook(1, 2, 3)
+        mock_except_hook.assert_called_once_with(1, 2, 3, config=self.config)
+
+        stderr_handler = self.root_logger.addHandler.call_args_list[0][0][0]
+        level = stderr_handler.level
+        if self.config.quiet:
+            self.assertEqual(level, constants.QUIET_LOGGING_LEVEL)
+        else:
+            self.assertEqual(level, -self.config.verbose_count * 10)
+
+    def test_quiet(self):
+        self.config.quiet = True
+        self.test_common()
+
+
+class SetupLogFileHandlerTest(test_util.TempDirTestCase):
+    """Tests for certbot.log.setup_log_file_handler."""
+
+    @classmethod
+    def _call(cls, *args, **kwargs):
+        from certbot.log import setup_log_file_handler
+        return setup_log_file_handler(*args, **kwargs)
+
+    def setUp(self):
+        super(SetupLogFileHandlerTest, self).setUp()
+        self.config = mock.MagicMock(logs_dir=self.tempdir)
+
+    @mock.patch('certbot.main.logging.handlers.RotatingFileHandler')
+    def test_failure(self, mock_handler):
+        mock_handler.side_effect = IOError
+
+        try:
+            self._call(self.config, 'test.log', '%(message)s')
+        except errors.Error as err:
+            self.assertTrue('--logs-dir' in str(err))
+        else:  # pragma: no cover
+            self.fail('Error not raised.')
+
+    def test_success(self):
+        log_file = 'test.log'
+        handler, log_path = self._call(self.config, log_file, '%(message)s')
+        self.assertEqual(handler.level, logging.DEBUG)
+        self.assertEqual(handler.formatter.converter, time.gmtime)
+
+        expected_path = os.path.join(self.config.logs_dir, log_file)
+        self.assertEqual(log_path, expected_path)
+
+
+class ColoredStreamHandlerTest(unittest.TestCase):
+    """Tests for certbot.log."""
+
+    def setUp(self):
+        from certbot import log
+
+        self.stream = six.StringIO()
+        self.stream.isatty = lambda: True
+        self.handler = log.ColoredStreamHandler(self.stream)
+
+        self.logger = logging.getLogger()
+        self.logger.setLevel(logging.DEBUG)
+        self.logger.addHandler(self.handler)
+
+    def test_format(self):
+        msg = 'I did a thing'
+        self.logger.debug(msg)
+        self.assertEqual(self.stream.getvalue(), '{0}\n'.format(msg))
+
+    def test_format_and_red_level(self):
+        msg = 'I did another thing'
+        self.handler.red_level = logging.DEBUG
+        self.logger.debug(msg)
+
+        self.assertEqual(self.stream.getvalue(),
+                         '{0}{1}{2}\n'.format(util.ANSI_SGR_RED,
+                                              msg,
+                                              util.ANSI_SGR_RESET))
+
+
+class ExceptHookTest(unittest.TestCase):
+    """Tests for certbot.log.except_hook."""
+    @classmethod
+    def _call(cls, *args, **kwargs):
+        from certbot.log import except_hook
+        return except_hook(*args, **kwargs)
+
+    @mock.patch('certbot.log.sys')
+    def test_except_hook(self, mock_sys):
+        config = mock.MagicMock()
+        mock_open = mock.mock_open()
+
+        with mock.patch('certbot.log.open', mock_open, create=True):
+            exception = Exception('detail')
+            config.verbose_count = 1
+            self._call(
+                Exception, exc_value=exception, trace=None, config=None)
+            mock_open().write.assert_any_call(''.join(
+                traceback.format_exception_only(Exception, exception)))
+            error_msg = mock_sys.exit.call_args_list[0][0][0]
+            self.assertTrue('unexpected error' in error_msg)
+
+        with mock.patch('certbot.log.open', mock_open, create=True):
+            mock_open.side_effect = [KeyboardInterrupt]
+            error = errors.Error('detail')
+            self._call(
+                errors.Error, exc_value=error, trace=None, config=None)
+            # assert_any_call used because sys.exit doesn't exit in cli.py
+            mock_sys.exit.assert_any_call(''.join(
+                traceback.format_exception_only(errors.Error, error)))
+
+        bad_typ = messages.ERROR_PREFIX + 'triffid'
+        exception = messages.Error(detail='alpha', typ=bad_typ, title='beta')
+        config = mock.MagicMock(debug=False, verbose_count=-3)
+        self._call(
+            messages.Error, exc_value=exception, trace=None, config=config)
+        error_msg = mock_sys.exit.call_args_list[-1][0][0]
+        self.assertTrue('unexpected error' in error_msg)
+        self.assertTrue('acme:error' not in error_msg)
+        self.assertTrue('alpha' in error_msg)
+        self.assertTrue('beta' in error_msg)
+        config = mock.MagicMock(debug=False, verbose_count=1)
+        self._call(
+            messages.Error, exc_value=exception, trace=None, config=config)
+        error_msg = mock_sys.exit.call_args_list[-1][0][0]
+        self.assertTrue('unexpected error' in error_msg)
+        self.assertTrue('acme:error' in error_msg)
+        self.assertTrue('alpha' in error_msg)
+
+        interrupt = KeyboardInterrupt('detail')
+        self._call(
+            KeyboardInterrupt, exc_value=interrupt, trace=None, config=None)
+        mock_sys.exit.assert_called_with(''.join(
+            traceback.format_exception_only(KeyboardInterrupt, interrupt)))
+
+
+if __name__ == "__main__":
+    unittest.main()  # pragma: no cover

certbot/tests/main_test.py

@@ -6,7 +6,6 @@ import itertools
 import mock
 import os
 import shutil
-import tempfile
 import traceback
 import unittest
 import datetime
@@ -19,7 +18,6 @@ from acme import jose
 
 from certbot import account
 from certbot import cli
-from certbot import colored_logging
 from certbot import constants
 from certbot import configuration
 from certbot import crypto_util
@@ -219,13 +217,14 @@ class FindDomainsOrCertnameTest(unittest.TestCase):
             (["one.com", "two.com"], "one.com"))
 
 
-class RevokeTest(unittest.TestCase):
+class RevokeTest(test_util.TempDirTestCase):
     """Tests for certbot.main.revoke."""
 
     def setUp(self):
-        self.tempdir_path = tempfile.mkdtemp()
-        shutil.copy(CERT_PATH, self.tempdir_path)
-        self.tmp_cert_path = os.path.abspath(os.path.join(self.tempdir_path,
+        super(RevokeTest, self).setUp()
+
+        shutil.copy(CERT_PATH, self.tempdir)
+        self.tmp_cert_path = os.path.abspath(os.path.join(self.tempdir,
             'cert.pem'))
 
         self.patches = [
@@ -251,7 +250,8 @@ class RevokeTest(unittest.TestCase):
         self.mock_determine_account.return_value = (self.acc, None)
 
     def tearDown(self):
-        shutil.rmtree(self.tempdir_path)
+        super(RevokeTest, self).tearDown()
+
         for patch in self.patches:
             patch.stop()
 
@@ -287,89 +287,6 @@ class RevokeTest(unittest.TestCase):
         self.mock_success_revoke.assert_not_called()
 
 
-class SetupLogFileHandlerTest(unittest.TestCase):
-    """Tests for certbot.main.setup_log_file_handler."""
-
-    def setUp(self):
-        self.config = mock.Mock(spec_set=['logs_dir'],
-                                logs_dir=tempfile.mkdtemp())
-
-    def tearDown(self):
-        shutil.rmtree(self.config.logs_dir)
-
-    def _call(self, *args, **kwargs):
-        from certbot.main import setup_log_file_handler
-        return setup_log_file_handler(*args, **kwargs)
-
-    @mock.patch('certbot.main.logging.handlers.RotatingFileHandler')
-    def test_ioerror(self, mock_handler):
-        mock_handler.side_effect = IOError
-        self.assertRaises(errors.Error, self._call,
-                          self.config, "test.log", "%s")
-
-
-class SetupLoggingTest(unittest.TestCase):
-    """Tests for certbot.main.setup_logging."""
-
-    def setUp(self):
-        self.config = mock.Mock(
-            logs_dir=tempfile.mkdtemp(),
-            noninteractive_mode=False, quiet=False,
-            verbose_count=constants.CLI_DEFAULTS['verbose_count'])
-
-    def tearDown(self):
-        shutil.rmtree(self.config.logs_dir)
-
-    @classmethod
-    def _call(cls, *args, **kwargs):
-        from certbot.main import setup_logging
-        return setup_logging(*args, **kwargs)
-
-    @mock.patch('certbot.main.logging.getLogger')
-    def test_defaults(self, mock_get_logger):
-        self._call(self.config)
-
-        cli_handler = mock_get_logger().addHandler.call_args_list[0][0][0]
-        self.assertEqual(cli_handler.level, -self.config.verbose_count * 10)
-        self.assertTrue(
-            isinstance(cli_handler, colored_logging.StreamHandler))
-
-    @mock.patch('certbot.main.logging.getLogger')
-    def test_quiet_mode(self, mock_get_logger):
-        self.config.quiet = self.config.noninteractive_mode = True
-        self._call(self.config)
-
-        cli_handler = mock_get_logger().addHandler.call_args_list[0][0][0]
-        self.assertEqual(cli_handler.level, constants.QUIET_LOGGING_LEVEL)
-        self.assertTrue(
-            isinstance(cli_handler, colored_logging.StreamHandler))
-
-
-class MakeOrVerifyCoreDirTest(unittest.TestCase):
-    """Tests for certbot.main.make_or_verify_core_dir."""
-
-    def setUp(self):
-        self.dir = tempfile.mkdtemp()
-
-    def tearDown(self):
-        shutil.rmtree(self.dir)
-
-    def _call(self, *args, **kwargs):
-        from certbot.main import make_or_verify_core_dir
-        return make_or_verify_core_dir(*args, **kwargs)
-
-    def test_success(self):
-        new_dir = os.path.join(self.dir, 'new')
-        self._call(new_dir, 0o700, os.geteuid(), False)
-        self.assertTrue(os.path.exists(new_dir))
-
-    @mock.patch('certbot.main.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.dir, 0o700, os.geteuid(), False)
-
-
 class DetermineAccountTest(unittest.TestCase):
     """Tests for certbot.main._determine_account."""
 
@@ -440,14 +357,15 @@ class DetermineAccountTest(unittest.TestCase):
         self.assertEqual('other email', self.config.email)
 
 
-class MainTest(unittest.TestCase):  # pylint: disable=too-many-public-methods
+class MainTest(test_util.TempDirTestCase):  # pylint: disable=too-many-public-methods
     """Tests for different commands."""
 
     def setUp(self):
-        self.tmp_dir = tempfile.mkdtemp()
-        self.config_dir = os.path.join(self.tmp_dir, 'config')
-        self.work_dir = os.path.join(self.tmp_dir, 'work')
-        self.logs_dir = os.path.join(self.tmp_dir, 'logs')
+        super(MainTest, self).setUp()
+
+        self.config_dir = os.path.join(self.tempdir, 'config')
+        self.work_dir = os.path.join(self.tempdir, 'work')
+        self.logs_dir = os.path.join(self.tempdir, 'logs')
         os.mkdir(self.logs_dir)
         self.standard_args = ['--config-dir', self.config_dir,
                               '--work-dir', self.work_dir,
@@ -456,7 +374,8 @@ class MainTest(unittest.TestCase):  # pylint: disable=too-many-public-methods
     def tearDown(self):
         # Reset globals in cli
         reload_module(cli)
-        shutil.rmtree(self.tmp_dir)
+
+        super(MainTest, self).tearDown()
 
     def _call(self, args, stdout=None):
         "Run the cli with output streams and actual client mocked out"
@@ -1254,59 +1173,5 @@ class UnregisterTest(unittest.TestCase):
         self.assertFalse(acme_client.acme.deactivate_registration.called)
 
 
-class TestHandleException(unittest.TestCase):
-    """Test main._handle_exception"""
-    @mock.patch('certbot.main.sys')
-    def test_handle_exception(self, mock_sys):
-        # pylint: disable=protected-access
-        from acme import messages
-
-        config = mock.MagicMock()
-        mock_open = mock.mock_open()
-
-        with mock.patch('certbot.main.open', mock_open, create=True):
-            exception = Exception('detail')
-            config.verbose_count = 1
-            main._handle_exception(
-                Exception, exc_value=exception, trace=None, config=None)
-            mock_open().write.assert_any_call(''.join(
-                traceback.format_exception_only(Exception, exception)))
-            error_msg = mock_sys.exit.call_args_list[0][0][0]
-            self.assertTrue('unexpected error' in error_msg)
-
-        with mock.patch('certbot.main.open', mock_open, create=True):
-            mock_open.side_effect = [KeyboardInterrupt]
-            error = errors.Error('detail')
-            main._handle_exception(
-                errors.Error, exc_value=error, trace=None, config=None)
-            # assert_any_call used because sys.exit doesn't exit in cli.py
-            mock_sys.exit.assert_any_call(''.join(
-                traceback.format_exception_only(errors.Error, error)))
-
-        bad_typ = messages.ERROR_PREFIX + 'triffid'
-        exception = messages.Error(detail='alpha', typ=bad_typ, title='beta')
-        config = mock.MagicMock(debug=False, verbose_count=-3)
-        main._handle_exception(
-            messages.Error, exc_value=exception, trace=None, config=config)
-        error_msg = mock_sys.exit.call_args_list[-1][0][0]
-        self.assertTrue('unexpected error' in error_msg)
-        self.assertTrue('acme:error' not in error_msg)
-        self.assertTrue('alpha' in error_msg)
-        self.assertTrue('beta' in error_msg)
-        config = mock.MagicMock(debug=False, verbose_count=1)
-        main._handle_exception(
-            messages.Error, exc_value=exception, trace=None, config=config)
-        error_msg = mock_sys.exit.call_args_list[-1][0][0]
-        self.assertTrue('unexpected error' in error_msg)
-        self.assertTrue('acme:error' in error_msg)
-        self.assertTrue('alpha' in error_msg)
-
-        interrupt = KeyboardInterrupt('detail')
-        main._handle_exception(
-            KeyboardInterrupt, exc_value=interrupt, trace=None, config=None)
-        mock_sys.exit.assert_called_with(''.join(
-            traceback.format_exception_only(KeyboardInterrupt, interrupt)))
-
-
 if __name__ == '__main__':
     unittest.main()  # pragma: no cover

certbot/tests/renewal_test.py

@@ -2,8 +2,6 @@
 import os
 import mock
 import unittest
-import shutil
-import tempfile
 
 from acme import challenges
 
@@ -14,13 +12,11 @@ from certbot import storage
 from certbot.tests import util
 
 
-class RenewalTest(unittest.TestCase):
+class RenewalTest(util.TempDirTestCase):
     def setUp(self):
-        self.tmp_dir = tempfile.mkdtemp()
-        self.config_dir = os.path.join(self.tmp_dir, 'config')
+        super(RenewalTest, self).setUp()
 
-    def tearDown(self):
-        shutil.rmtree(self.tmp_dir)
+        self.config_dir = os.path.join(self.tempdir, 'config')
 
     @mock.patch('certbot.cli.set_by_cli')
     def test_ancient_webroot_renewal_conf(self, mock_set_by_cli):
@@ -52,7 +48,7 @@ class RestoreRequiredConfigElementsTest(unittest.TestCase):
     def test_allow_subset_of_names_success(self, mock_set_by_cli):
         mock_set_by_cli.return_value = False
         self._call(self.config, {'allow_subset_of_names': 'True'})
-        self.assertTrue(self.config.namespace.allow_subset_of_names is True)
+        self.assertTrue(self.config.allow_subset_of_names is True)
 
     @mock.patch('certbot.renewal.cli.set_by_cli')
     def test_allow_subset_of_names_failure(self, mock_set_by_cli):
@@ -68,7 +64,7 @@ class RestoreRequiredConfigElementsTest(unittest.TestCase):
         self._call(self.config, renewalparams)
         expected = [challenges.TLSSNI01.typ,
                     challenges.HTTP01.typ, challenges.DNS01.typ]
-        self.assertEqual(self.config.namespace.pref_challs, expected)
+        self.assertEqual(self.config.pref_challs, expected)
 
     @mock.patch('certbot.renewal.cli.set_by_cli')
     def test_pref_challs_str(self, mock_set_by_cli):
@@ -76,7 +72,7 @@ class RestoreRequiredConfigElementsTest(unittest.TestCase):
         renewalparams = {'pref_challs': 'dns'}
         self._call(self.config, renewalparams)
         expected = [challenges.DNS01.typ]
-        self.assertEqual(self.config.namespace.pref_challs, expected)
+        self.assertEqual(self.config.pref_challs, expected)
 
     @mock.patch('certbot.renewal.cli.set_by_cli')
     def test_pref_challs_failure(self, mock_set_by_cli):
@@ -88,7 +84,7 @@ class RestoreRequiredConfigElementsTest(unittest.TestCase):
     def test_must_staple_success(self, mock_set_by_cli):
         mock_set_by_cli.return_value = False
         self._call(self.config, {'must_staple': 'True'})
-        self.assertTrue(self.config.namespace.must_staple is True)
+        self.assertTrue(self.config.must_staple is True)
 
     @mock.patch('certbot.renewal.cli.set_by_cli')
     def test_must_staple_failure(self, mock_set_by_cli):

certbot/tests/reporter_test.py

@@ -38,18 +38,6 @@ class ReporterTest(unittest.TestCase):
             self.reporter.print_messages()
         self.assertEqual(sys.stdout.getvalue(), "")
 
-    @mock.patch('certbot.reporter.os.getpid')
-    def test_atexit_print_messages(self, mock_getpid):
-        self._add_messages()
-        mock_getpid.return_value = 42
-        with mock.patch('certbot.reporter.INITIAL_PID', 42):
-            self.reporter.atexit_print_messages()
-        output = sys.stdout.getvalue()
-        self.assertTrue("IMPORTANT NOTES:" in output)
-        self.assertTrue("High" in output)
-        self.assertTrue("Med" in output)
-        self.assertTrue("Low" in output)
-
     def test_tty_successful_exit(self):
         sys.stdout.isatty = lambda: True
         self._successful_exit_common()

certbot/tests/storage_test.py

@@ -3,7 +3,6 @@
 import datetime
 import os
 import shutil
-import tempfile
 import unittest
 
 import configobj
@@ -36,7 +35,7 @@ def fill_with_sample_data(rc_object):
             f.write(kind)
 
 
-class BaseRenewableCertTest(unittest.TestCase):
+class BaseRenewableCertTest(util.TempDirTestCase):
     """Base class for setting up Renewable Cert tests.
 
     .. note:: It may be required to write out self.config for
@@ -47,7 +46,8 @@ class BaseRenewableCertTest(unittest.TestCase):
 
     def setUp(self):
         from certbot import storage
-        self.tempdir = tempfile.mkdtemp()
+
+        super(BaseRenewableCertTest, self).setUp()
 
         self.cli_config = configuration.NamespaceConfig(
             namespace=mock.MagicMock(
@@ -91,9 +91,6 @@ class BaseRenewableCertTest(unittest.TestCase):
             check.return_value = True
             self.test_rc = storage.RenewableCert(config.filename, self.cli_config)
 
-    def tearDown(self):
-        shutil.rmtree(self.tempdir)
-
     def _write_out_kind(self, kind, ver, value=None):
         link = getattr(self.test_rc, kind)
         if os.path.lexists(link):
@@ -581,6 +578,15 @@ class RenewableCertTests(BaseRenewableCertTest):
         self.assertEqual(
             self._test_relevant_values_common(values), values)
 
+    @mock.patch("certbot.cli.set_by_cli")
+    @mock.patch("certbot.plugins.disco.PluginsRegistry.find_all")
+    def test_relevant_values_namespace(self, mock_find_all, mock_set_by_cli):
+        mock_set_by_cli.return_value = True
+        mock_find_all.return_value = ["certbot-foo:bar"]
+        values = {"certbot_foo:bar_baz": 42}
+        self.assertEqual(
+            self._test_relevant_values_common(values), values)
+
     @mock.patch("certbot.storage.relevant_values")
     def test_new_lineage(self, mock_rv):
         """Test for new_lineage() class method."""
@@ -789,6 +795,7 @@ class DeleteFilesTest(BaseRenewableCertTest):
     """Tests for certbot.storage.delete_files"""
     def setUp(self):
         super(DeleteFilesTest, self).setUp()
+
         for kind in ALL_FOUR:
             kind_path = os.path.join(self.tempdir, "live", "example.org",
                                         kind + ".pem")

certbot/tests/util.py

@@ -6,6 +6,7 @@
 import os
 import pkg_resources
 import shutil
+import tempfile
 import unittest
 
 from cryptography.hazmat.backends import default_backend
@@ -230,3 +231,13 @@ def _assert_valid_call(*args, **kwargs):
 
     # pylint: disable=star-args
     display_util.assert_valid_call(*assert_args, **assert_kwargs)
+
+
+class TempDirTestCase(unittest.TestCase):
+    """Base test class which sets up and tears down a temporary directory"""
+
+    def setUp(self):
+        self.tempdir = tempfile.mkdtemp()
+
+    def tearDown(self):
+        shutil.rmtree(self.tempdir)

certbot/tests/util_test.py

@@ -2,9 +2,7 @@
 import argparse
 import errno
 import os
-import shutil
 import stat
-import tempfile
 import unittest
 
 import mock
@@ -75,7 +73,26 @@ class ExeExistsTest(unittest.TestCase):
         self.assertFalse(self._call("exe"))
 
 
-class MakeOrVerifyDirTest(unittest.TestCase):
+class MakeOrVerifyCoreDirTest(test_util.TempDirTestCase):
+    """Tests for certbot.util.make_or_verify_core_dir."""
+
+    def _call(self, *args, **kwargs):
+        from certbot.util import make_or_verify_core_dir
+        return make_or_verify_core_dir(*args, **kwargs)
+
+    def test_success(self):
+        new_dir = os.path.join(self.tempdir, 'new')
+        self._call(new_dir, 0o700, os.geteuid(), False)
+        self.assertTrue(os.path.exists(new_dir))
+
+    @mock.patch('certbot.main.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, os.geteuid(), False)
+
+
+class MakeOrVerifyDirTest(test_util.TempDirTestCase):
     """Tests for certbot.util.make_or_verify_dir.
 
     Note that it is not possible to test for a wrong directory owner,
@@ -84,21 +101,19 @@ class MakeOrVerifyDirTest(unittest.TestCase):
     """
 
     def setUp(self):
-        self.root_path = tempfile.mkdtemp()
-        self.path = os.path.join(self.root_path, "foo")
+        super(MakeOrVerifyDirTest, self).setUp()
+
+        self.path = os.path.join(self.tempdir, "foo")
         os.mkdir(self.path, 0o400)
 
         self.uid = os.getuid()
 
-    def tearDown(self):
-        shutil.rmtree(self.root_path, ignore_errors=True)
-
     def _call(self, directory, mode):
         from certbot.util import make_or_verify_dir
         return make_or_verify_dir(directory, mode, self.uid, strict=True)
 
     def test_creates_dir_when_missing(self):
-        path = os.path.join(self.root_path, "bar")
+        path = os.path.join(self.tempdir, "bar")
         self._call(path, 0o650)
         self.assertTrue(os.path.isdir(path))
         self.assertEqual(stat.S_IMODE(os.stat(path).st_mode), 0o650)
@@ -116,7 +131,7 @@ class MakeOrVerifyDirTest(unittest.TestCase):
             self.assertRaises(OSError, self._call, "bar", 12312312)
 
 
-class CheckPermissionsTest(unittest.TestCase):
+class CheckPermissionsTest(test_util.TempDirTestCase):
     """Tests for certbot.util.check_permissions.
 
     Note that it is not possible to test for a wrong file owner,
@@ -125,34 +140,30 @@ class CheckPermissionsTest(unittest.TestCase):
     """
 
     def setUp(self):
-        _, self.path = tempfile.mkstemp()
-        self.uid = os.getuid()
+        super(CheckPermissionsTest, self).setUp()
 
-    def tearDown(self):
-        os.remove(self.path)
+        self.uid = os.getuid()
 
     def _call(self, mode):
         from certbot.util import check_permissions
-        return check_permissions(self.path, mode, self.uid)
+        return check_permissions(self.tempdir, mode, self.uid)
 
     def test_ok_mode(self):
-        os.chmod(self.path, 0o600)
+        os.chmod(self.tempdir, 0o600)
         self.assertTrue(self._call(0o600))
 
     def test_wrong_mode(self):
-        os.chmod(self.path, 0o400)
+        os.chmod(self.tempdir, 0o400)
         self.assertFalse(self._call(0o600))
 
 
-class UniqueFileTest(unittest.TestCase):
+class UniqueFileTest(test_util.TempDirTestCase):
     """Tests for certbot.util.unique_file."""
 
     def setUp(self):
-        self.root_path = tempfile.mkdtemp()
-        self.default_name = os.path.join(self.root_path, "foo.txt")
+        super(UniqueFileTest, self).setUp()
 
-    def tearDown(self):
-        shutil.rmtree(self.root_path, ignore_errors=True)
+        self.default_name = os.path.join(self.tempdir, "foo.txt")
 
     def _call(self, mode=0o600):
         from certbot.util import unique_file
@@ -177,9 +188,9 @@ class UniqueFileTest(unittest.TestCase):
         self.assertNotEqual(name1, name3)
         self.assertNotEqual(name2, name3)
 
-        self.assertEqual(os.path.dirname(name1), self.root_path)
-        self.assertEqual(os.path.dirname(name2), self.root_path)
-        self.assertEqual(os.path.dirname(name3), self.root_path)
+        self.assertEqual(os.path.dirname(name1), self.tempdir)
+        self.assertEqual(os.path.dirname(name2), self.tempdir)
+        self.assertEqual(os.path.dirname(name3), self.tempdir)
 
         basename1 = os.path.basename(name2)
         self.assertTrue(basename1.endswith("foo.txt"))
@@ -193,26 +204,20 @@ try:
     file_type = file
 except NameError:
     import io
-    file_type = io.TextIOWrapper
+    file_type = io.TextIOWrapper  # type: ignore
 
 
-class UniqueLineageNameTest(unittest.TestCase):
+class UniqueLineageNameTest(test_util.TempDirTestCase):
     """Tests for certbot.util.unique_lineage_name."""
 
-    def setUp(self):
-        self.root_path = tempfile.mkdtemp()
-
-    def tearDown(self):
-        shutil.rmtree(self.root_path, ignore_errors=True)
-
     def _call(self, filename, mode=0o777):
         from certbot.util import unique_lineage_name
-        return unique_lineage_name(self.root_path, filename, mode)
+        return unique_lineage_name(self.tempdir, filename, mode)
 
     def test_basic(self):
         f, path = self._call("wow")
         self.assertTrue(isinstance(f, file_type))
-        self.assertEqual(os.path.join(self.root_path, "wow.conf"), path)
+        self.assertEqual(os.path.join(self.tempdir, "wow.conf"), path)
 
     def test_multiple(self):
         for _ in six.moves.range(10):
@@ -237,15 +242,13 @@ class UniqueLineageNameTest(unittest.TestCase):
         self.assertRaises(OSError, self._call, "wow")
 
 
-class SafelyRemoveTest(unittest.TestCase):
+class SafelyRemoveTest(test_util.TempDirTestCase):
     """Tests for certbot.util.safely_remove."""
 
     def setUp(self):
-        self.tmp = tempfile.mkdtemp()
-        self.path = os.path.join(self.tmp, "foo")
+        super(SafelyRemoveTest, self).setUp()
 
-    def tearDown(self):
-        shutil.rmtree(self.tmp)
+        self.path = os.path.join(self.tempdir, "foo")
 
     def _call(self):
         from certbot.util import safely_remove
@@ -489,5 +492,37 @@ class OsInfoTest(unittest.TestCase):
                                      ("windows", "95"))
 
 
+class AtexitRegisterTest(unittest.TestCase):
+    """Tests for certbot.util.atexit_register."""
+    def setUp(self):
+        self.func = mock.MagicMock()
+        self.args = ('hi',)
+        self.kwargs = {'answer': 42}
+
+    @classmethod
+    def _call(cls, *args, **kwargs):
+        from certbot.util import atexit_register
+        return atexit_register(*args, **kwargs)
+
+    def test_called(self):
+        self._test_common(os.getpid())
+        self.func.assert_called_with(*self.args, **self.kwargs)
+
+    def test_not_called(self):
+        self._test_common(initial_pid=-1)
+        self.assertFalse(self.func.called)
+
+    def _test_common(self, initial_pid):
+        with mock.patch('certbot.util._INITIAL_PID', initial_pid):
+            with mock.patch('certbot.util.atexit') as mock_atexit:
+                self._call(self.func, *self.args, **self.kwargs)
+
+            # _INITAL_PID must be mocked when calling atexit_func
+            self.assertTrue(mock_atexit.register.called)
+            args, kwargs = mock_atexit.register.call_args
+            atexit_func = args[0]
+            atexit_func(*args[1:], **kwargs)  # pylint: disable=star-args
+
+
 if __name__ == "__main__":
     unittest.main()  # pragma: no cover

certbot/util.py

@@ -1,5 +1,6 @@
 """Utilities for all Certbot."""
 import argparse
+import atexit
 import collections
 # distutils.version under virtualenv confuses pylint
 # For more info, see: https://github.com/PyCQA/pylint/issues/73
@@ -38,6 +39,16 @@ ANSI_SGR_RED = "\033[31m"
 ANSI_SGR_RESET = "\033[0m"
 
 
+PERM_ERR_FMT = os.linesep.join((
+    "The following error was encountered:", "{0}",
+    "If running as non-root, set --config-dir, "
+    "--work-dir, and --logs-dir to writeable paths."))
+
+
+# Stores importing process ID to be used by atexit_register()
+_INITIAL_PID = os.getpid()
+
+
 def run_script(params, log=logger.error):
     """Run the script with the given params.
 
@@ -92,6 +103,23 @@ def exe_exists(exe):
     return False
 
 
+def make_or_verify_core_dir(directory, mode, uid, strict):
+    """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 the directory cannot be made or verified
+
+    """
+    try:
+        make_or_verify_dir(directory, mode, uid, strict)
+    except OSError as error:
+        raise errors.Error(PERM_ERR_FMT.format(error))
+
+
 def make_or_verify_dir(directory, mode=0o755, uid=0, strict=False):
     """Make sure directory exists with proper permissions.
 
@@ -533,3 +561,20 @@ def is_staging(srv):
     :rtype bool:
     """
     return srv == constants.STAGING_URI or "staging" in srv
+
+
+def atexit_register(func, *args, **kwargs):
+    """Sets func to be called before the program exits.
+
+    Special care is taken to ensure func is only called when the process
+    that first imports this module exits rather than any child processes.
+
+    :param function func: function to be called in case of an error
+
+    """
+    atexit.register(_atexit_call, func, *args, **kwargs)
+
+
+def _atexit_call(func, *args, **kwargs):
+    if _INITIAL_PID == os.getpid():
+        func(*args, **kwargs)

docs/contributing.rst

@@ -145,13 +145,15 @@ different webservers, other TLS servers, and operating systems.
 The interfaces available for plugins to implement are defined in
 `interfaces.py`_ and `plugins/common.py`_.
 
-The most common kind of plugin is a "Configurator", which is likely to
-implement the `~certbot.interfaces.IAuthenticator` and
-`~certbot.interfaces.IInstaller` interfaces (though some
-Configurators may implement just one of those).
+The main two plugin interfaces are `~certbot.interfaces.IAuthenticator`, which
+implements various ways of proving domain control to a certificate authority,
+and `~certbot.interfaces.IInstaller`, which configures a server to use a
+certificate once it is issued. Some plugins, like the built-in Apache and Nginx
+plugins, implement both interfaces and perform both tasks. Others, like the
+built-in Standalone authenticator, implement just one interface.
 
 There are also `~certbot.interfaces.IDisplay` plugins,
-which implement bindings to alternative UI libraries.
+which can change how prompts are displayed to a user.
 
 .. _interfaces.py: https://github.com/certbot/certbot/blob/master/certbot/interfaces.py
 .. _plugins/common.py: https://github.com/certbot/certbot/blob/master/certbot/plugins/common.py#L34
@@ -160,27 +162,20 @@ which implement bindings to alternative UI libraries.
 Authenticators
 --------------
 
-Authenticators are plugins designed to prove that this client deserves a
-certificate for some domain name by solving challenges received from
-the ACME server. From the protocol, there are essentially two
-different types of challenges. Challenges that must be solved by
-individual plugins in order to satisfy domain validation (subclasses
-of `~.DVChallenge`, i.e. `~.challenges.TLSSNI01`,
-`~.challenges.HTTP01`, `~.challenges.DNS`) and continuity specific
-challenges (subclasses of `~.ContinuityChallenge`,
-i.e. `~.challenges.RecoveryToken`, `~.challenges.RecoveryContact`,
-`~.challenges.ProofOfPossession`). Continuity challenges are
-always handled by the `~.ContinuityAuthenticator`, while plugins are
-expected to handle `~.DVChallenge` types.
-Right now, we have two authenticator plugins, the `~.ApacheConfigurator`
-and the `~.StandaloneAuthenticator`. The Standalone and Apache
-authenticators only solve the `~.challenges.TLSSNI01` challenge currently.
-(You can set which challenges your authenticator can handle through the
-:meth:`~.IAuthenticator.get_chall_pref`.
+Authenticators are plugins that prove control of a domain name by solving a
+challenge provided by the ACME server. ACME currently defines three types of
+challenges: HTTP, TLS-SNI, and DNS, represented by classes in `acme.challenges`.
+An authenticator plugin should implement support for at least one challenge type.
 
-(FYI: We also have a partial implementation for a `~.DNSAuthenticator`
-in a separate branch).
+An Authenticator indicates which challenges it supports by implementing
+get_chall_pref(domain) to return a sorted list of challenge types in preference
+order.
 
+An Authenticator must also implement `perform(achalls)`, which "performs" a list
+of challenges by, for instance, provisioning a file on an HTTP server, or
+setting a TXT record in DNS. Once all challenges have succeeded or failed,
+Certbot will call the plugin's `cleanup(achalls)` method to remove any files or
+DNS records that were needed only during authentication.
 
 Installer
 ---------
@@ -218,16 +213,10 @@ Augeas may still find the `~.Reverter` class helpful in handling
 configuration checkpoints and rollback.
 
 
-Display
-~~~~~~~
-
-We currently only offer a "text" mode for displays. Display plugins
-implement the `~certbot.interfaces.IDisplay` interface.
-
 .. _dev-plugin:
 
 Writing your own plugin
-=======================
+~~~~~~~~~~~~~~~~~~~~~~~
 
 Certbot client supports dynamic discovery of plugins through the
 `setuptools entry points`_. This way you can, for example, create a
@@ -236,6 +225,26 @@ the `~certbot.interfaces.IInstaller` without having to merge it
 with the core upstream source code. An example is provided in
 ``examples/plugins/`` directory.
 
+While developing, you can install your plugin into a Certbot development
+virtualenv like this:
+
+.. code-block:: shell
+  . venv/bin/activate
+  . tests/integration/_common.sh
+  pip install -e examples/plugins/
+  certbot_test plugins
+
+Your plugin should show up in the output of the last command. If not,
+it was not installed properly.
+
+Once you've finished your plugin and published it, you can have your
+users install it system-wide with `pip install`. Note that this will
+only work for users who have Certbot installed from OS packages or via
+pip. Users who run `certbot-auto` are currently unable to use third-party
+plugins. It's technically possible to install third-party plugins into
+the virtualenv used by `certbot-auto`, but they will be wiped away when
+`certbot-auto` upgrades.
+
 .. warning:: Please be aware though that as this client is still in a
    developer-preview stage, the API may undergo a few changes. If you
    believe the plugin will be beneficial to the community, please

docs/install.rst

@@ -22,9 +22,8 @@ your system.
 System Requirements
 ===================
 
-The Let's Encrypt Client presently only runs on Unix-ish OSes that include
-Python 2.6 or 2.7; Python 3.x support will hopefully be added in the future. The
-client requires root access in order to write to ``/etc/letsencrypt``,
+Certbot currently requires Python 2.6 or 2.7. By default, it requires root
+access in order to write to ``/etc/letsencrypt``,
 ``/var/log/letsencrypt``, ``/var/lib/letsencrypt``; to bind to ports 80 and 443
 (if you use the ``standalone`` plugin) and to read and modify webserver
 configurations (if you use the ``apache`` or ``nginx`` plugins).  If none of
@@ -33,11 +32,16 @@ but for most users who want to avoid running an ACME client as root, either
 `letsencrypt-nosudo <https://github.com/diafygi/letsencrypt-nosudo>`_ or
 `simp_le <https://github.com/kuba/simp_le>`_ are more appropriate choices.
 
-The Apache plugin currently requires OS with augeas version 1.0; currently `it
+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.
 
+Installing with ``certbot-auto`` requires 512MB of RAM in order to build some
+of the dependencies. Installing from pre-built OS packages avoids this
+requirement. You can also temporarily set a swap file. See "Problems with
+Python virtual environment" below for details.
+
 Alternate installation methods
 ================================
 
@@ -76,7 +80,7 @@ For full command line help, you can type::
 Problems with Python virtual environment
 ----------------------------------------
 
-On a low memory system such as VPS with only 256MB of RAM, the required dependencies of Certbot will failed to build.
+On a low memory system such as VPS with less than 512MB of RAM, the required dependencies of Certbot will failed to build.
 This can be identified if the pip outputs contains something like ``internal compiler error: Killed (program cc1)``.
 You can workaround this restriction by creating a temporary swapfile::
 
@@ -120,7 +124,7 @@ to, `install Docker`_, then issue the following command:
    sudo docker run -it --rm -p 443:443 -p 80:80 --name certbot \
                -v "/etc/letsencrypt:/etc/letsencrypt" \
                -v "/var/lib/letsencrypt:/var/lib/letsencrypt" \
-               quay.io/letsencrypt/letsencrypt:latest certonly
+               certbot/certbot certonly
 
 Running Certbot with the ``certonly`` command will obtain a certificate and place it in the directory
 ``/etc/letsencrypt/live`` on your system. Because Certonly cannot install the certificate from

docs/packaging.rst

@@ -16,21 +16,19 @@ The following scripts are used in the process:
 
 - https://github.com/letsencrypt/letsencrypt/blob/master/tools/release.sh
 
-We currently version with the following scheme:
+We use git tags to identify releases, using `Semantic Versioning`_. For
+example: `v0.11.1`.
 
-- ``0.1.0``
-- ``0.2.0dev`` for developement in ``master``
-- ``0.2.0`` (only temporarily in ``master``)
-- ...
+.. _`Semantic Versioning`: http://semver.org/
 
 Notes for package maintainers
 =============================
 
-0. Please use our releases, not ``master``!
+0. Please use our tagged releases, not ``master``!
 
 1. Do not package ``certbot-compatibility-test`` or ``letshelp-certbot`` - it's only used internally.
 
-2. If you'd like to include automated renewal in your package ``certbot renew -q`` should be added to crontab or systemd timer.
+2. If you'd like to include automated renewal in your package ``certbot renew -q`` should be added to crontab or systemd timer. Additionally you should include a random per-machine time offset to avoid having a large number of your clients hit Let's Encrypt's servers simultaneously.
 
 3. ``jws`` is an internal script for ``acme`` module and it doesn't have to be packaged - it's mostly for debugging: you can use it as ``echo foo | jws sign | jws verify``.
 
@@ -44,34 +42,43 @@ Arch
 ----
 
 From our official releases:
+
 - https://www.archlinux.org/packages/community/any/python2-acme
 - https://www.archlinux.org/packages/community/any/certbot
 - https://www.archlinux.org/packages/community/any/certbot-apache
 - https://www.archlinux.org/packages/community/any/certbot-nginx
-- https://www.archlinux.org/packages/community/any/letshelp-certbot
 
 From ``master``: https://aur.archlinux.org/packages/certbot-git
 
 Debian (and its derivatives, including Ubuntu)
 ------
 
-https://packages.debian.org/sid/certbot
-https://packages.debian.org/sid/python-certbot
-https://packages.debian.org/sid/python-certbot-apache
+- https://packages.debian.org/sid/certbot
+- https://packages.debian.org/sid/python-certbot
+- https://packages.debian.org/sid/python-certbot-apache
 
 Fedora
 ------
 
 In Fedora 23+.
 
-- https://admin.fedoraproject.org/pkgdb/package/letsencrypt/
 - https://admin.fedoraproject.org/pkgdb/package/certbot/
 - https://admin.fedoraproject.org/pkgdb/package/python-acme/
 
 FreeBSD
 -------
 
-https://svnweb.freebsd.org/ports/head/security/py-certbot/
+- https://svnweb.freebsd.org/ports/head/security/py-certbot/
+
+Gentoo
+------
+
+Currently, all ``certbot`` related packages are in the testing branch:
+
+- https://packages.gentoo.org/packages/app-crypt/certbot
+- https://packages.gentoo.org/packages/app-crypt/certbot-apache
+- https://packages.gentoo.org/packages/app-crypt/certbot-nginx
+- https://packages.gentoo.org/packages/app-crypt/acme
 
 GNU Guix
 --------

docs/using.rst

@@ -175,15 +175,15 @@ the UI, you can use the plugin to obtain a cert by specifying
 to copy and paste commands into another terminal session, which may
 be on a different computer.
 
-The manual plugin can use either the ``http`` or the ``dns`` challenge. You 
-can use the ``--preferred-challenges`` option to chose the challenge of your 
+The manual plugin can use either the ``http`` or the ``dns`` challenge. You
+can use the ``--preferred-challenges`` option to choose the challenge of your
 preference.
-The ``http`` challenge will ask you to place a file with a specific name and 
-specific content in the ``/.well-known/acme-challenge/`` directory directly 
-in the top-level directory (“web root”) containing the files served by your 
+The ``http`` challenge will ask you to place a file with a specific name and
+specific content in the ``/.well-known/acme-challenge/`` directory directly
+in the top-level directory (“web root”) containing the files served by your
 webserver. In essence it's the same as the webroot_ plugin, but not automated.
-When using the ``dns`` plugin, ``certbot`` will ask you to place a TXT DNS 
-record with specific contents under the domain name consisting of the hostname 
+When using the ``dns`` plugin, ``certbot`` will ask you to place a TXT DNS
+record with specific contents under the domain name consisting of the hostname
 for which you want a certificate issued, prepended by ``_acme-challenge``.
 
 For example, for the domain ``example.com``, a zone file entry would look like:

examples/cli.ini

@@ -1,6 +1,11 @@
 # This is an example of the kind of things you can do in a configuration file.
 # All flags used by the client can be configured here. Run Certbot with
 # "--help" to learn more about the available options.
+#
+# Note that these options apply automatically to all use of Certbot for
+# obtaining or renewing certificates, so options specific to a single
+# certificate on a system with several certificates should not be placed
+# here.
 
 # Use a 4096 bit RSA key instead of 2048
 rsa-key-size = 4096
@@ -8,13 +13,6 @@ rsa-key-size = 4096
 # Uncomment and update to register with the specified e-mail address
 # email = foo@example.com
 
-# Uncomment and update to generate certificates for the specified
-# domains.
-# domains = example.com, www.example.com
-
-# Uncomment to use a text interface instead of ncurses
-# text = True
-
 # Uncomment to use the standalone authenticator on port 443
 # authenticator = standalone
 # standalone-supported-challenges = tls-sni-01

letshelp-certbot/letshelp_certbot/apache.py

@@ -15,6 +15,7 @@ import tarfile
 import tempfile
 import textwrap
 
+import six
 
 _DESCRIPTION = """
 Let's Help is a simple script you can run to help out the Certbot
@@ -69,7 +70,7 @@ def make_and_verify_selection(server_root, temp_dir):
 
     sys.stdout.write("\nIs it safe to submit these files? ")
     while True:
-        ans = raw_input("(Y)es/(N)o: ").lower()
+        ans = six.moves.input("(Y)es/(N)o: ").lower()
         if ans.startswith("y"):
             return
         elif ans.startswith("n"):
@@ -144,7 +145,8 @@ def safe_config_file(config_file):
         return False
 
     proc = subprocess.Popen(["file", config_file],
-                            stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+                            stdout=subprocess.PIPE, stderr=subprocess.PIPE,
+                            universal_newlines=True)
     file_output, _ = proc.communicate()
 
     if "ASCII" in file_output:

letshelp-certbot/letshelp_certbot/apache_test.py

@@ -9,6 +9,8 @@ import tempfile
 import unittest
 
 import mock
+# six is used in mock.patch()
+import six  # pylint: disable=unused-import
 
 import letshelp_certbot.apache as letshelp_le_apache
 
@@ -63,7 +65,7 @@ class LetsHelpApacheTest(unittest.TestCase):
     def test_make_and_verify_selection(self, mock_copy_config):
         mock_copy_config.return_value = (["apache2.conf"], ["apache2"])
 
-        with mock.patch("__builtin__.raw_input") as mock_input:
+        with mock.patch("six.moves.input") as mock_input:
             with mock.patch(_MODULE_NAME + ".sys.stdout"):
                 mock_input.side_effect = ["Yes", "No"]
                 letshelp_le_apache.make_and_verify_selection("root", "temp")

tests/boulder-fetch.sh

@@ -12,5 +12,7 @@ fi
 
 cd ${BOULDERPATH}
 FAKE_DNS=$(ifconfig docker0 | grep "inet addr:" | cut -d: -f2 | awk '{ print $1}')
+[ -z "$FAKE_DNS" ] && FAKE_DNS=$(ifconfig docker0 | grep "inet " | xargs | cut -d ' ' -f 2)
+[ -z "$FAKE_DNS" ] && echo Unable to find the IP for docker0 && exit 1
 sed -i "s/FAKE_DNS: .*/FAKE_DNS: ${FAKE_DNS}/" docker-compose.yml
 docker-compose up -d

tests/boulder-integration.sh

@@ -1,4 +1,4 @@
-#!/bin/sh -xe
+#!/bin/bash
 # Simple integration test. Make sure to activate virtualenv beforehand
 # (source venv/bin/activate) and that you are running Boulder test
 # instance (see ./boulder-fetch.sh).
@@ -8,12 +8,11 @@
 #
 # Note: this script is called by Boulder integration test suite!
 
+set -eux
+
 . ./tests/integration/_common.sh
 export PATH="$PATH:/usr/sbin"  # /usr/sbin/nginx
 
-export GOPATH="${GOPATH:-/tmp/go}"
-export PATH="$GOPATH/bin:$PATH"
-
 if [ `uname` = "Darwin" ];then
   readlink="greadlink"
 else
@@ -27,6 +26,14 @@ cleanup_and_exit() {
         echo Kill server subprocess, left running by abnormal exit
         kill $SERVER_STILL_RUNNING
     fi
+    # Dump boulder logs in case they contain useful debugging information.
+    : "------------------ ------------------ ------------------"
+    : "------------------ begin boulder logs ------------------"
+    : "------------------ ------------------ ------------------"
+    docker logs boulder_boulder_1
+    : "------------------ ------------------ ------------------"
+    : "------------------  end boulder logs  ------------------"
+    : "------------------ ------------------ ------------------"
     exit $EXIT_STATUS
 }
 

tests/integration/_common.sh

@@ -1,12 +1,7 @@
-#!/bin/sh
-
-if [ "xxx$root" = "xxx" ];
-then
-    # The -t is required on macOS. It provides a template file path for
-    # the kernel to use.
-    root="$(mktemp -d -t leitXXXX)"
-    echo "Root integration tests directory: $root"
-fi
+# The -t is required on macOS. It provides a template file path for
+# the kernel to use.
+root=${root:-$(mktemp -d -t leitXXXX)}
+echo "Root integration tests directory: $root"
 store_flags="--config-dir $root/conf --work-dir $root/work"
 store_flags="$store_flags --logs-dir $root/logs"
 tls_sni_01_port=5001

tools/release.sh

@@ -109,7 +109,7 @@ do
   echo "Signing ($pkg_dir)"
   for x in dist/*.tar.gz dist/*.whl
   do
-      gpg -u "$RELEASE_GPG_KEY" --detach-sign --armor --sign $x
+      gpg2 -u "$RELEASE_GPG_KEY" --detach-sign --armor --sign --digest-algo sha256 $x
   done
 
   cd -
@@ -194,7 +194,7 @@ while ! openssl dgst -sha256 -verify $RELEASE_OPENSSL_PUBKEY -signature \
 done
 
 # This signature is not quite as strong, but easier for people to verify out of band
-gpg -u "$RELEASE_GPG_KEY" --detach-sign --armor --sign letsencrypt-auto-source/letsencrypt-auto
+gpg2 -u "$RELEASE_GPG_KEY" --detach-sign --armor --sign --digest-algo sha256 letsencrypt-auto-source/letsencrypt-auto
 # We can't rename the openssl letsencrypt-auto.sig for compatibility reasons,
 # but we can use the right name for certbot-auto.asc from day one
 mv letsencrypt-auto-source/letsencrypt-auto.asc letsencrypt-auto-source/certbot-auto.asc
@@ -214,7 +214,7 @@ name=${root_without_le%.*}
 ext="${root_without_le##*.}"
 rev="$(git rev-parse --short HEAD)"
 echo tar cJvf $name.$rev.tar.xz $name.$rev
-echo gpg -U $RELEASE_GPG_KEY --detach-sign --armor $name.$rev.tar.xz
+echo gpg2 -U $RELEASE_GPG_KEY --detach-sign --armor $name.$rev.tar.xz
 cd ~-
 
 echo "New root: $root"

tox.ini

@@ -40,42 +40,6 @@ deps =
     py{26,27}-oldest: PyOpenSSL==0.13
     py{26,27}-oldest: requests<=2.11.1
 
-[testenv:py33]
-commands =
-    pip install -e acme[dev]
-    nosetests -v acme --processes=-1
-    pip install -e .[dev]
-    nosetests -v certbot --processes=-1 --process-timeout=100
-    pip install -e certbot-apache
-    nosetests -v certbot_apache --processes=-1 --process-timeout=80
-
-[testenv:py34]
-commands =
-    pip install -e acme[dev]
-    nosetests -v acme --processes=-1
-    pip install -e .[dev]
-    nosetests -v certbot --processes=-1 --process-timeout=100
-    pip install -e certbot-apache
-    nosetests -v certbot_apache --processes=-1 --process-timeout=80
-
-[testenv:py35]
-commands =
-    pip install -e acme[dev]
-    nosetests -v acme --processes=-1
-    pip install -e .[dev]
-    nosetests -v certbot --processes=-1 --process-timeout=100
-    pip install -e certbot-apache
-    nosetests -v certbot_apache --processes=-1 --process-timeout=80
-
-[testenv:py36]
-commands =
-    pip install -e acme[dev]
-    nosetests -v acme --processes=-1
-    pip install -e .[dev]
-    nosetests -v certbot --processes=-1 --process-timeout=100
-    pip install -e certbot-apache
-    nosetests -v certbot_apache --processes=-1 --process-timeout=80
-
 [testenv:py27_install]
 basepython = python2.7
 commands =
@@ -97,6 +61,13 @@ commands =
     pip install -q -e acme[dev] -e .[dev] -e certbot-apache -e certbot-nginx -e certbot-compatibility-test -e letshelp-certbot
     pylint --reports=n --rcfile=.pylintrc acme/acme certbot certbot-apache/certbot_apache certbot-nginx/certbot_nginx certbot-compatibility-test/certbot_compatibility_test letshelp-certbot/letshelp_certbot
 
+[testenv:mypy]
+basepython = python3.4
+commands =
+    pip install mypy
+    pip install -q -e acme[dev] -e .[dev] -e certbot-apache -e certbot-nginx -e certbot-compatibility-test -e letshelp-certbot
+    mypy --py2 --ignore-missing-imports acme/acme certbot certbot-apache/certbot_apache certbot-nginx/certbot_nginx certbot-compatibility-test/certbot_compatibility_test letshelp-certbot/letshelp_certbot
+
 [testenv:apacheconftest]
 #basepython = python2.7
 commands =