Merge remote-tracking branch 'certbot/master'

acme/acme/errors.py

@@ -49,7 +49,7 @@ class MissingNonce(NonceError):
 
     def __str__(self):
         return ('Server {0} response did not include a replay '
-                'nonce, headers: {1}'.format(
+                'nonce, headers: {1} (This may be a service outage)'.format(
                     self.response.request.method, self.response.headers))
 
 

certbot-apache/certbot_apache/augeas_lens/httpd.aug

@@ -52,11 +52,14 @@ let sep_eq              = del /[ \t]*=[ \t]*/ "="
 let nmtoken             = /[a-zA-Z:_][a-zA-Z0-9:_.-]*/
 let word                = /[a-z][a-z0-9._-]*/i
 
-let comment             = Util.comment
 let eol                 = Util.doseol
 let empty               = Util.empty_dos
 let indent              = Util.indent
 
+let comment_val_re      = /([^ \t\r\n](.|\\\\\r?\n)*[^ \\\t\r\n]|[^ \t\r\n])/
+let comment             = [ label "#comment" . del /[ \t]*#[ \t]*/ "# "
+                          . store comment_val_re . eol ]
+
 (* borrowed from shellvars.aug *)
 let char_arg_dir  = /([^\\ '"{\t\r\n]|[^ '"{\t\r\n]+[^\\ \t\r\n])|\\\\"|\\\\'|\\\\ /
 let char_arg_sec  = /([^\\ '"\t\r\n>]|[^ '"\t\r\n>]+[^\\ \t\r\n>])|\\\\"|\\\\'|\\\\ /

certbot-apache/certbot_apache/configurator.py

@@ -18,6 +18,7 @@ from certbot import interfaces
 from certbot import util
 
 from certbot.plugins import common
+from certbot.plugins.util import path_surgery
 
 from certbot_apache import augeas_configurator
 from certbot_apache import constants
@@ -141,6 +142,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         return os.path.join(self.config.config_dir,
                             constants.MOD_SSL_CONF_DEST)
 
+
     def prepare(self):
         """Prepare the authenticator/installer.
 
@@ -157,8 +159,11 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             raise errors.NoInstallationError("Problem in Augeas installation")
 
         # Verify Apache is installed
-        if not util.exe_exists(constants.os_constant("restart_cmd")[0]):
-            raise errors.NoInstallationError
+        restart_cmd = constants.os_constant("restart_cmd")[0]
+        if not util.exe_exists(restart_cmd):
+            if not path_surgery(restart_cmd):
+                raise errors.NoInstallationError(
+                    'Cannot find Apache control command {0}'.format(restart_cmd))
 
         # Make sure configuration is valid
         self.config_test()
@@ -327,9 +332,12 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         vhost = display_ops.select_vhost(target_name, self.vhosts)
         if vhost is None:
             logger.error(
-                "No vhost exists with servername or alias of: %s. "
-                "No vhost was selected. Please specify servernames "
-                "in the Apache config", target_name)
+                "No vhost exists with servername or alias of: %s "
+                "(or it's in a file with multiple vhosts, which Certbot "
+                "can't parse yet). "
+                "No vhost was selected. Please specify ServerName or ServerAlias "
+                "in the Apache config, or split vhosts into separate files.",
+                target_name)
             raise errors.PluginError("No vhost selected")
         elif temp:
             return vhost
@@ -625,50 +633,93 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
 
         """
 
+        # If nonstandard port, add service definition for matching
+        if port != "443":
+            port_service = "%s %s" % (port, "https")
+        else:
+            port_service = port
+
         self.prepare_https_modules(temp)
         # Check for Listen <port>
         # Note: This could be made to also look for ip:443 combo
         listens = [self.parser.get_arg(x).split()[0] for
                    x in self.parser.find_dir("Listen")]
+
         # In case no Listens are set (which really is a broken apache config)
         if not listens:
             listens = ["80"]
-        if port in listens:
+
+        # Listen already in place
+        if self._has_port_already(listens, port):
             return
+
+        listen_dirs = set(listens)
+
         for listen in listens:
             # For any listen statement, check if the machine also listens on
             # Port 443. If not, add such a listen statement.
             if len(listen.split(":")) == 1:
                 # Its listening to all interfaces
-                if port not in listens:
-                    if port == "443":
-                        args = [port]
-                    else:
-                        # Non-standard ports should specify https protocol
-                        args = [port, "https"]
-                    self.parser.add_dir_to_ifmodssl(
-                        parser.get_aug_path(
-                            self.parser.loc["listen"]), "Listen", args)
-                    self.save_notes += "Added Listen %s directive to %s\n" % (
-                        port, self.parser.loc["listen"])
-                    listens.append(port)
+                if port not in listen_dirs and port_service not in listen_dirs:
+                    listen_dirs.add(port_service)
             else:
                 # The Listen statement specifies an ip
                 _, ip = listen[::-1].split(":", 1)
                 ip = ip[::-1]
-                if "%s:%s" % (ip, port) not in listens:
-                    if port == "443":
-                        args = ["%s:%s" % (ip, port)]
-                    else:
-                        # Non-standard ports should specify https protocol
-                        args = ["%s:%s" % (ip, port), "https"]
-                    self.parser.add_dir_to_ifmodssl(
-                        parser.get_aug_path(
-                            self.parser.loc["listen"]), "Listen", args)
-                    self.save_notes += ("Added Listen %s:%s directive to "
-                                        "%s\n") % (ip, port,
-                                                   self.parser.loc["listen"])
-                    listens.append("%s:%s" % (ip, port))
+                if "%s:%s" % (ip, port_service) not in listen_dirs and (
+                   "%s:%s" % (ip, port_service) not in listen_dirs):
+                    listen_dirs.add("%s:%s" % (ip, port_service))
+        self._add_listens(listen_dirs, listens, port)
+
+    def _add_listens(self, listens, listens_orig, port):
+        """Helper method for prepare_server_https to figure out which new
+        listen statements need adding
+
+        :param set listens: Set of all needed Listen statements
+        :param list listens_orig: List of existing listen statements
+        :param string port: Port number we're adding
+        """
+
+        # Add service definition for non-standard ports
+        if port != "443":
+            port_service = "%s %s" % (port, "https")
+        else:
+            port_service = port
+
+        new_listens = listens.difference(listens_orig)
+
+        if port in new_listens or port_service in new_listens:
+            # We have wildcard, skip the rest
+            self.parser.add_dir_to_ifmodssl(
+                parser.get_aug_path(self.parser.loc["listen"]),
+                "Listen", port_service.split(" "))
+            self.save_notes += "Added Listen %s directive to %s\n" % (
+                port_service, self.parser.loc["listen"])
+        else:
+            for listen in new_listens:
+                self.parser.add_dir_to_ifmodssl(
+                    parser.get_aug_path(self.parser.loc["listen"]),
+                    "Listen", listen.split(" "))
+                self.save_notes += ("Added Listen %s directive to "
+                                    "%s\n") % (listen,
+                                               self.parser.loc["listen"])
+
+    def _has_port_already(self, listens, port):
+        """Helper method for prepare_server_https to find out if user
+        already has an active Listen statement for the port we need
+
+        :param list listens: List of listen variables
+        :param string port: Port in question
+        """
+
+        if port in listens:
+            return True
+        # Check if Apache is already listening on a specific IP
+        for listen in listens:
+            if len(listen.split(":")) > 1:
+                # Ugly but takes care of protocol def, eg: 1.1.1.1:443 https
+                if listen.split(":")[-1].split(" ")[0] == port:
+                    return True
 
     def prepare_https_modules(self, temp):
         """Helper method for prepare_server_https, taking care of enabling
@@ -773,7 +824,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         else:
             return non_ssl_vh_fp + self.conf("le_vhost_ext")
 
-    def _sift_line(self, line):
+    def _sift_rewrite_rule(self, line):
         """Decides whether a line should be copied to a SSL vhost.
 
         A canonical example of when sifting a line is required:
@@ -824,18 +875,62 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             with open(avail_fp, "r") as orig_file:
                 with open(ssl_fp, "w") as new_file:
                     new_file.write("<IfModule mod_ssl.c>\n")
+
+                    comment = ("# Some rewrite rules in this file were "
+                              "disabled on your HTTPS site,\n"
+                              "# because they have the potential to create "
+                              "redirection loops.\n")
+
                     for line in orig_file:
-                        if self._sift_line(line):
+                        A = line.lstrip().startswith("RewriteCond")
+                        B = line.lstrip().startswith("RewriteRule")
+
+                        if not (A or B):
+                            new_file.write(line)
+                            continue
+
+                        # A RewriteRule that doesn't need filtering
+                        if B and not self._sift_rewrite_rule(line):
+                            new_file.write(line)
+                            continue
+
+                        # A RewriteRule that does need filtering
+                        if B and self._sift_rewrite_rule(line):
                             if not sift:
-                                new_file.write(
-                                    "# Some rewrite rules in this file were "
-                                    "were disabled on your HTTPS site,\n"
-                                    "# because they have the potential to "
-                                    "create redirection loops.\n")
+                                new_file.write(comment)
                                 sift = True
                             new_file.write("# " + line)
-                        else:
-                            new_file.write(line)
+                            continue
+
+                        # We save RewriteCond(s) and their corresponding
+                        # RewriteRule in 'chunk'.
+                        # We then decide whether we comment out the entire
+                        # chunk based on its RewriteRule.
+                        chunk = []
+                        if A:
+                            chunk.append(line)
+                            line = next(orig_file)
+
+                            # RewriteCond(s) must be followed by one RewriteRule
+                            while not line.lstrip().startswith("RewriteRule"):
+                                chunk.append(line)
+                                line = next(orig_file)
+
+                            # Now, current line must start with a RewriteRule
+                            chunk.append(line)
+
+                            if self._sift_rewrite_rule(line):
+                                if not sift:
+                                    new_file.write(comment)
+                                    sift = True
+
+                                new_file.write(''.join(
+                                    ['# ' + l for l in chunk]))
+                                continue
+                            else:
+                                new_file.write(''.join(chunk))
+                                continue
+
                     new_file.write("</IfModule>\n")
         except IOError:
             logger.fatal("Error writing/reading to file in make_vhost_ssl")

certbot-apache/certbot_apache/display_ops.py

@@ -86,11 +86,12 @@ def _vhost_menu(domain, vhosts):
             "like to choose?\n(note: conf files with multiple "
             "vhosts are not yet supported)".format(domain, os.linesep),
             choices, help_label="More Info", ok_label="Select")
-    except errors.MissingCommandlineFlag as e:
-        msg = ("Failed to run Apache plugin non-interactively{1}{0}{1}"
-               "(The best solution is to add ServerName or ServerAlias "
-               "entries to the VirtualHost directives of your apache "
-               "configuration files.)".format(e, os.linesep))
+    except errors.MissingCommandlineFlag:
+        msg = ("Encountered vhost ambiguity but unable to ask for user guidance in "
+               "non-interactive mode. Currently Certbot needs each vhost to be "
+               "in its own conf file, and may need vhosts to be explicitly "
+               "labelled with ServerName or ServerAlias directories.")
+        logger.warn(msg)
         raise errors.MissingCommandlineFlag(msg)
 
     return code, tag

certbot-apache/certbot_apache/obj.py

@@ -6,6 +6,7 @@ from certbot.plugins import common
 
 class Addr(common.Addr):
     """Represents an Apache address."""
+
     def __eq__(self, other):
         """This is defined as equalivalent within Apache.
 
@@ -21,6 +22,9 @@ class Addr(common.Addr):
     def __ne__(self, other):
         return not self.__eq__(other)
 
+    def __repr__(self):
+        return "certbot_apache.obj.Addr(" + repr(self.tup) + ")"
+
     def _addr_less_specific(self, addr):
         """Returns if addr.get_addr() is more specific than self.get_addr()."""
         # pylint: disable=protected-access

certbot-apache/certbot_apache/tests/apache-conf-files/passing/comment-continuations-2050.conf

@@ -0,0 +1,428 @@
+# ---------------------------------------------------------------
+# Core ModSecurity Rule Set ver.2.2.6
+# Copyright (C) 2006-2012 Trustwave All rights reserved.
+#
+# The OWASP ModSecurity Core Rule Set is distributed under 
+# Apache Software License (ASL) version 2
+# Please see the enclosed LICENCE file for full details.
+# ---------------------------------------------------------------
+
+
+#
+# -- [[ Recommended Base Configuration ]] ------------------------------------------------- 
+#
+# The configuration directives/settings in this file are used to control
+# the OWASP ModSecurity CRS. These settings do **NOT** configure the main
+# ModSecurity settings such as:
+# 
+# - SecRuleEngine
+# - SecRequestBodyAccess
+# - SecAuditEngine
+# - SecDebugLog
+#
+# You should use the modsecurity.conf-recommended file that comes with the
+# ModSecurity source code archive.
+#
+# Ref: http://mod-security.svn.sourceforge.net/viewvc/mod-security/m2/trunk/modsecurity.conf-recommended
+#
+
+
+#
+# -- [[ Rule Version ]] -------------------------------------------------------------------
+#
+# Rule version data is added to the "Producer" line of Section H of the Audit log:
+#
+# - Producer: ModSecurity for Apache/2.7.0-rc1 (http://www.modsecurity.org/); OWASP_CRS/2.2.4.
+#
+# Ref: https://sourceforge.net/apps/mediawiki/mod-security/index.php?title=Reference_Manual#SecComponentSignature
+#
+#SecComponentSignature "OWASP_CRS/2.2.6"
+
+
+#
+# -- [[ Modes of Operation: Self-Contained vs. Collaborative Detection ]] -----------------
+#
+# Each detection rule uses the "block" action which will inherit the SecDefaultAction
+# specified below.  Your settings here will determine which mode of operation you use.
+#
+# -- [[ Self-Contained Mode ]] --
+# Rules inherit the "deny" disruptive action.  The first rule that matches will block.
+#
+# -- [[ Collaborative Detection Mode ]] --
+# This is a "delayed blocking" mode of operation where each matching rule will inherit
+# the "pass" action and will only contribute to anomaly scores.  Transactional blocking
+# can be applied 
+#
+# -- [[ Alert Logging Control ]] --
+# You have three options -
+#
+# - To log to both the Apache error_log and ModSecurity audit_log file use: "log"
+# - To log *only* to the ModSecurity audit_log file use: "nolog,auditlog"
+# - To log *only* to the Apache error_log file use: "log,noauditlog"
+#
+# Ref: http://blog.spiderlabs.com/2010/11/advanced-topic-of-the-week-traditional-vs-anomaly-scoring-detection-modes.html
+# Ref: https://sourceforge.net/apps/mediawiki/mod-security/index.php?title=Reference_Manual#SecDefaultAction
+#
+#SecDefaultAction "phase:1,deny,log"
+
+
+#
+# -- [[ Collaborative Detection Severity Levels ]] ----------------------------------------
+#
+# These are the default scoring points for each severity level.  You may
+# adjust these to you liking.  These settings will be used in macro expansion
+# in the rules to increment the anomaly scores when rules match.
+#
+# These are the default Severity ratings (with anomaly scores) of the individual rules -
+#
+#    - 2: Critical - Anomaly Score of 5.
+#         Is the highest severity level possible without correlation.  It is
+#         normally generated by the web attack rules (40 level files).
+#    - 3: Error - Anomaly Score of 4.
+#         Is generated mostly from outbound leakage rules (50 level files).
+#    - 4: Warning - Anomaly Score of 3.
+#         Is generated by malicious client rules (35 level files).
+#    - 5: Notice - Anomaly Score of 2.
+#         Is generated by the Protocol policy and anomaly files.
+#
+#SecAction \
+  "id:'900001', \
+  phase:1, \
+  t:none, \
+  setvar:tx.critical_anomaly_score=5, \
+  setvar:tx.error_anomaly_score=4, \
+  setvar:tx.warning_anomaly_score=3, \
+  setvar:tx.notice_anomaly_score=2, \
+  nolog, \
+  pass"
+
+
+#
+# -- [[ Collaborative Detection Scoring Threshold Levels ]] ------------------------------
+#
+# These variables are used in macro expansion in the 49 inbound blocking and 59
+# outbound blocking files.
+#
+# **MUST HAVE** ModSecurity v2.5.12 or higher to use macro expansion in numeric
+# operators.  If you have an earlier version, edit the 49/59 files directly to
+# set the appropriate anomaly score levels.
+#
+# You should set the score to the proper threshold you would prefer. If set to "5"
+# it will work similarly to previous Mod CRS rules and will create an event in the error_log
+# file if there are any rules that match.  If you would like to lessen the number of events
+# generated in the error_log file, you should increase the anomaly score threshold to
+# something like "20".  This would only generate an event in the error_log file if
+# there are multiple lower severity rule matches or if any 1 higher severity item matches.
+#
+#SecAction \
+  "id:'900002', \
+  phase:1, \
+  t:none, \
+  setvar:tx.inbound_anomaly_score_level=5, \
+  nolog, \
+  pass"
+
+
+#SecAction \
+  "id:'900003', \
+  phase:1, \
+  t:none, \
+  setvar:tx.outbound_anomaly_score_level=4, \
+  nolog, \
+  pass"
+
+
+# 
+# -- [[ Collaborative Detection Blocking ]] -----------------------------------------------
+#
+# This is a collaborative detection mode where each rule will increment an overall
+# anomaly score for the transaction. The scores are then evaluated in the following files:
+#
+# Inbound anomaly score - checked in the modsecurity_crs_49_inbound_blocking.conf file
+# Outbound anomaly score - checked in the modsecurity_crs_59_outbound_blocking.conf file
+#
+# If you want to use anomaly scoring mode, then uncomment this line.
+#
+#SecAction \
+  "id:'900004', \
+  phase:1, \
+  t:none, \
+  setvar:tx.anomaly_score_blocking=on, \
+  nolog, \
+  pass"
+
+
+#
+# -- [[ GeoIP Database ]] -----------------------------------------------------------------
+#
+# There are some rulesets that need to inspect the GEO data of the REMOTE_ADDR data.
+# 
+# You must first download the MaxMind GeoIP Lite City DB -
+#
+#       http://geolite.maxmind.com/download/geoip/database/GeoLiteCity.dat.gz
+#
+# You then need to define the proper path for the SecGeoLookupDb directive
+#
+# Ref: http://blog.spiderlabs.com/2010/10/detecting-malice-with-modsecurity-geolocation-data.html
+# Ref: http://blog.spiderlabs.com/2010/11/detecting-malice-with-modsecurity-ip-forensics.html
+#
+#SecGeoLookupDb /opt/modsecurity/lib/GeoLiteCity.dat
+
+#
+# -- [[ Regression Testing Mode ]] --------------------------------------------------------
+#
+# If you are going to run the regression testing mode, you should uncomment the
+# following rule. It will enable DetectionOnly mode for the SecRuleEngine and
+# will enable Response Header tagging so that the client testing script can see
+# which rule IDs have matched.
+#
+# You must specify the your source IP address where you will be running the tests
+# from.
+#
+#SecRule REMOTE_ADDR "@ipMatch 192.168.1.100" \
+  "id:'900005', \
+  phase:1, \
+  t:none, \
+  ctl:ruleEngine=DetectionOnly, \
+  setvar:tx.regression_testing=1, \
+  nolog, \
+  pass"
+
+
+#
+# -- [[ HTTP Policy Settings ]] ----------------------------------------------------------
+#
+# Set the following policy settings here and they will be propagated to the 23 rules
+# file (modsecurity_common_23_request_limits.conf) by using macro expansion.  
+# If you run into false positives, you can adjust the settings here.
+#
+# Only the max number of args is uncommented by default as there are a high rate
+# of false positives.  Uncomment the items you wish to set.
+# 
+#
+# -- Maximum number of arguments in request limited
+#SecAction \
+  "id:'900006', \
+  phase:1, \
+  t:none, \
+  setvar:tx.max_num_args=255, \
+  nolog, \
+  pass"
+
+#
+# -- Limit argument name length
+#SecAction \
+  "id:'900007', \
+  phase:1, \
+  t:none, \
+  setvar:tx.arg_name_length=100, \
+  nolog, \
+  pass"
+
+#
+# -- Limit value name length
+#SecAction \
+  "id:'900008', \
+  phase:1, \
+  t:none, \
+  setvar:tx.arg_length=400, \
+  nolog, \
+  pass"
+
+#
+# -- Limit arguments total length
+#SecAction \
+  "id:'900009', \
+  phase:1, \
+  t:none, \
+  setvar:tx.total_arg_length=64000, \
+  nolog, \
+  pass"
+
+#
+# -- Individual file size is limited
+#SecAction \
+  "id:'900010', \
+  phase:1, \
+  t:none, \
+  setvar:tx.max_file_size=1048576, \
+  nolog, \
+  pass"
+
+#
+# -- Combined file size is limited
+#SecAction \
+  "id:'900011', \
+  phase:1, \
+  t:none, \
+  setvar:tx.combined_file_sizes=1048576, \
+  nolog, \
+  pass"
+
+
+#
+# Set the following policy settings here and they will be propagated to the 30 rules
+# file (modsecurity_crs_30_http_policy.conf) by using macro expansion.  
+# If you run into false positves, you can adjust the settings here.
+#
+#SecAction \
+  "id:'900012', \
+  phase:1, \
+  t:none, \
+  setvar:'tx.allowed_methods=GET HEAD POST OPTIONS', \
+  setvar:'tx.allowed_request_content_type=application/x-www-form-urlencoded|multipart/form-data|text/xml|application/xml|application/x-amf', \
+  setvar:'tx.allowed_http_versions=HTTP/0.9 HTTP/1.0 HTTP/1.1', \
+  setvar:'tx.restricted_extensions=.asa/ .asax/ .ascx/ .axd/ .backup/ .bak/ .bat/ .cdx/ .cer/ .cfg/ .cmd/ .com/ .config/ .conf/ .cs/ .csproj/ .csr/ .dat/ .db/ .dbf/ .dll/ .dos/ .htr/ .htw/ .ida/ .idc/ .idq/ .inc/ .ini/ .key/ .licx/ .lnk/ .log/ .mdb/ .old/ .pass/ .pdb/ .pol/ .printer/ .pwd/ .resources/ .resx/ .sql/ .sys/ .vb/ .vbs/ .vbproj/ .vsdisco/ .webinfo/ .xsd/ .xsx/', \
+  setvar:'tx.restricted_headers=/Proxy-Connection/ /Lock-Token/ /Content-Range/ /Translate/ /via/ /if/', \
+  nolog, \
+  pass"
+
+
+#
+# -- [[ Content Security Policy (CSP) Settings ]] -----------------------------------------
+#
+# The purpose of these settings is to send CSP response headers to
+# Mozilla FireFox users so that you can enforce how dynamic content
+# is used. CSP usage helps to prevent XSS attacks against your users.
+#  
+# Reference Link:
+#
+#	https://developer.mozilla.org/en/Security/CSP
+#
+# Uncomment this SecAction line if you want use CSP enforcement.
+# You need to set the appropriate directives and settings for your site/domain and
+# and activate the CSP file in the experimental_rules directory.
+# 
+# Ref: http://blog.spiderlabs.com/2011/04/modsecurity-advanced-topic-of-the-week-integrating-content-security-policy-csp.html
+#
+#SecAction \
+  "id:'900013', \
+  phase:1, \
+  t:none, \
+  setvar:tx.csp_report_only=1, \
+  setvar:tx.csp_report_uri=/csp_violation_report, \
+  setenv:'csp_policy=allow \'self\'; img-src *.yoursite.com; media-src *.yoursite.com; style-src *.yoursite.com; frame-ancestors *.yoursite.com; script-src *.yoursite.com; report-uri %{tx.csp_report_uri}', \
+  nolog, \
+  pass"
+
+
+#
+# -- [[ Brute Force Protection ]] ---------------------------------------------------------
+#
+# If you are using the Brute Force Protection rule set, then uncomment the following
+# lines and set the following variables:
+# - Protected URLs: resources to protect (e.g. login pages) - set to your login page
+# - Burst Time Slice Interval: time interval window to monitor for bursts
+# - Request Threshold: request # threshold to trigger a burst
+# - Block Period: temporary block timeout
+#
+#SecAction \
+  "id:'900014', \
+  phase:1, \
+  t:none, \
+  setvar:'tx.brute_force_protected_urls=/login.jsp /partner_login.php', \
+  setvar:'tx.brute_force_burst_time_slice=60', \
+  setvar:'tx.brute_force_counter_threshold=10', \
+  setvar:'tx.brute_force_block_timeout=300', \
+  nolog, \
+  pass"
+
+
+#
+# -- [[ DoS Protection ]] ----------------------------------------------------------------
+#
+# If you are using the DoS Protection rule set, then uncomment the following
+# lines and set the following variables:
+# - Burst Time Slice Interval: time interval window to monitor for bursts
+# - Request Threshold: request # threshold to trigger a burst
+# - Block Period: temporary block timeout
+#
+#SecAction \
+  "id:'900015', \
+  phase:1, \
+  t:none, \
+  setvar:'tx.dos_burst_time_slice=60', \
+  setvar:'tx.dos_counter_threshold=100', \
+  setvar:'tx.dos_block_timeout=600', \
+  nolog, \
+  pass"
+
+
+#
+# -- [[ Check UTF enconding ]] -----------------------------------------------------------
+#
+# We only want to apply this check if UTF-8 encoding is actually used by the site, otherwise
+# it will result in false positives.
+#
+# Uncomment this line if your site uses UTF8 encoding
+#SecAction \
+  "id:'900016', \
+  phase:1, \
+  t:none, \
+  setvar:tx.crs_validate_utf8_encoding=1, \
+  nolog, \
+  pass"
+
+
+#
+# -- [[ Enable XML Body Parsing ]] -------------------------------------------------------
+#
+# The rules in this file will trigger the XML parser upon an XML request
+#
+# Initiate XML Processor in case of xml content-type
+#
+#SecRule REQUEST_HEADERS:Content-Type "text/xml" \
+  "id:'900017', \
+  phase:1, \
+  t:none,t:lowercase, \
+  nolog, \
+  pass, \
+  chain"
+	#SecRule REQBODY_PROCESSOR "!@streq XML" \
+	  "ctl:requestBodyProcessor=XML"
+
+
+#
+# -- [[ Global and IP Collections ]] -----------------------------------------------------
+#
+# Create both Global and IP collections for rules to use
+# There are some CRS rules that assume that these two collections
+# have already been initiated.
+#
+#SecRule REQUEST_HEADERS:User-Agent "^(.*)$" \
+  "id:'900018', \
+  phase:1, \
+  t:none,t:sha1,t:hexEncode, \
+  setvar:tx.ua_hash=%{matched_var}, \
+  nolog, \
+  pass"
+
+
+#SecRule REQUEST_HEADERS:x-forwarded-for "^\b(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})\b" \
+  "id:'900019', \
+  phase:1, \
+  t:none, \
+  capture, \
+  setvar:tx.real_ip=%{tx.1}, \
+  nolog, \
+  pass"
+
+
+#SecRule &TX:REAL_IP "!@eq 0" \
+  "id:'900020', \
+  phase:1, \
+  t:none, \
+  initcol:global=global, \
+  initcol:ip=%{tx.real_ip}_%{tx.ua_hash}, \
+  nolog, \
+  pass"
+
+
+#SecRule &TX:REAL_IP "@eq 0" \
+  "id:'900021', \
+  phase:1, \
+  t:none, \
+  initcol:global=global, \
+  initcol:ip=%{remote_addr}_%{tx.ua_hash}, \
+  nolog, \
+  pass"

certbot-apache/certbot_apache/tests/configurator_test.py

@@ -49,11 +49,14 @@ class MultipleVhostsTest(util.ApacheTest):
         shutil.rmtree(self.config_dir)
         shutil.rmtree(self.work_dir)
 
-    @mock.patch("certbot_apache.configurator.util.exe_exists")
-    def test_prepare_no_install(self, mock_exe_exists):
-        mock_exe_exists.return_value = False
-        self.assertRaises(
-            errors.NoInstallationError, self.config.prepare)
+    @mock.patch("certbot_apache.configurator.ApacheConfigurator.init_augeas")
+    @mock.patch("certbot_apache.configurator.path_surgery")
+    def test_prepare_no_install(self, mock_surgery, _init_augeas):
+        silly_path = {"PATH": "/tmp/nothingness2342"}
+        mock_surgery.return_value = False
+        with mock.patch.dict('os.environ', silly_path):
+            self.assertRaises(errors.NoInstallationError, self.config.prepare)
+            self.assertEquals(mock_surgery.call_count, 1)
 
     @mock.patch("certbot_apache.augeas_configurator.AugeasConfigurator.init_augeas")
     def test_prepare_no_augeas(self, mock_init_augeas):
@@ -86,6 +89,7 @@ class MultipleVhostsTest(util.ApacheTest):
         self.assertRaises(
             errors.NotSupportedError, self.config.prepare)
 
+
     def test_add_parser_arguments(self):  # pylint: disable=no-self-use
         from certbot_apache.configurator import ApacheConfigurator
         # Weak test..
@@ -497,13 +501,8 @@ class MultipleVhostsTest(util.ApacheTest):
 
         # Test Listen statements with specific ip listeed
         self.config.prepare_server_https("443")
-        # Should only be 2 here, as the third interface
-        # already listens to the correct port
-        self.assertEqual(mock_add_dir.call_count, 2)
-
-        # Check argument to new Listen statements
-        self.assertEqual(mock_add_dir.call_args_list[0][0][2], ["1.2.3.4:443"])
-        self.assertEqual(mock_add_dir.call_args_list[1][0][2], ["[::1]:443"])
+        # Should be 0 as one interface already listens to 443
+        self.assertEqual(mock_add_dir.call_count, 0)
 
         # Reset return lists and inputs
         mock_add_dir.reset_mock()
@@ -519,6 +518,28 @@ class MultipleVhostsTest(util.ApacheTest):
         self.assertEqual(mock_add_dir.call_args_list[2][0][2],
                          ["1.1.1.1:8080", "https"])
 
+        # mock_get.side_effect = ["1.2.3.4:80", "[::1]:80"]
+        # mock_find.return_value = ["test1", "test2", "test3"]
+        # self.config.parser.get_arg = mock_get
+        # self.config.prepare_server_https("8080", temp=True)
+        # self.assertEqual(self.listens, 0)
+
+    def test_prepare_server_https_needed_listen(self):
+        mock_find = mock.Mock()
+        mock_find.return_value = ["test1", "test2"]
+        mock_get = mock.Mock()
+        mock_get.side_effect = ["1.2.3.4:8080", "80"]
+        mock_add_dir = mock.Mock()
+        mock_enable = mock.Mock()
+
+        self.config.parser.find_dir = mock_find
+        self.config.parser.get_arg = mock_get
+        self.config.parser.add_dir_to_ifmodssl = mock_add_dir
+        self.config.enable_mod = mock_enable
+
+        self.config.prepare_server_https("443")
+        self.assertEqual(mock_add_dir.call_count, 1)
+
     def test_prepare_server_https_mixed_listen(self):
 
         mock_find = mock.Mock()
@@ -1093,16 +1114,19 @@ class MultipleVhostsTest(util.ApacheTest):
         self.config._enable_redirect(self.vh_truth[1], "")
         self.assertEqual(len(self.config.vhosts), 9)
 
-    def test_sift_line(self):
+    def test_sift_rewrite_rule(self):
         # pylint: disable=protected-access
         small_quoted_target = "RewriteRule ^ \"http://\""
-        self.assertFalse(self.config._sift_line(small_quoted_target))
+        self.assertFalse(self.config._sift_rewrite_rule(small_quoted_target))
 
         https_target = "RewriteRule ^ https://satoshi"
-        self.assertTrue(self.config._sift_line(https_target))
+        self.assertTrue(self.config._sift_rewrite_rule(https_target))
 
         normal_target = "RewriteRule ^/(.*) http://www.a.com:1234/$1 [L,R]"
-        self.assertFalse(self.config._sift_line(normal_target))
+        self.assertFalse(self.config._sift_rewrite_rule(normal_target))
+
+        not_rewriterule = "NotRewriteRule ^ ..."
+        self.assertFalse(self.config._sift_rewrite_rule(not_rewriterule))
 
     @mock.patch("certbot_apache.configurator.zope.component.getUtility")
     def test_make_vhost_ssl_with_existing_rewrite_rule(self, mock_get_utility):
@@ -1131,7 +1155,61 @@ class MultipleVhostsTest(util.ApacheTest):
                                   "[L,QSA,R=permanent]")
         self.assertTrue(commented_rewrite_rule in conf_text)
         mock_get_utility().add_message.assert_called_once_with(mock.ANY,
+
                                                                mock.ANY)
+    @mock.patch("certbot_apache.configurator.zope.component.getUtility")
+    def test_make_vhost_ssl_with_existing_rewrite_conds(self, mock_get_utility):
+        self.config.parser.modules.add("rewrite_module")
+
+        http_vhost = self.vh_truth[0]
+
+        self.config.parser.add_dir(
+            http_vhost.path, "RewriteEngine", "on")
+
+        # Add a chunk that should not be commented out.
+        self.config.parser.add_dir(http_vhost.path,
+                "RewriteCond", ["%{DOCUMENT_ROOT}/%{REQUEST_FILENAME}", "!-f"])
+        self.config.parser.add_dir(
+            http_vhost.path, "RewriteRule",
+            ["^(.*)$", "b://u%{REQUEST_URI}", "[P,QSA,L]"])
+
+        # Add a chunk that should be commented out.
+        self.config.parser.add_dir(http_vhost.path,
+                "RewriteCond", ["%{HTTPS}", "!=on"])
+        self.config.parser.add_dir(http_vhost.path,
+                "RewriteCond", ["%{HTTPS}", "!^$"])
+        self.config.parser.add_dir(
+            http_vhost.path, "RewriteRule",
+            ["^",
+             "https://%{SERVER_NAME}%{REQUEST_URI}",
+             "[L,QSA,R=permanent]"])
+
+        self.config.save()
+
+        ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[0])
+
+        conf_line_set = set(open(ssl_vhost.filep).read().splitlines())
+
+        not_commented_cond1 = ("RewriteCond "
+                "%{DOCUMENT_ROOT}/%{REQUEST_FILENAME} !-f")
+        not_commented_rewrite_rule = ("RewriteRule "
+            "^(.*)$ b://u%{REQUEST_URI} [P,QSA,L]")
+
+        commented_cond1 = "# RewriteCond %{HTTPS} !=on"
+        commented_cond2 = "# RewriteCond %{HTTPS} !^$"
+        commented_rewrite_rule = ("# RewriteRule ^ "
+                                  "https://%{SERVER_NAME}%{REQUEST_URI} "
+                                  "[L,QSA,R=permanent]")
+
+        self.assertTrue(not_commented_cond1 in conf_line_set)
+        self.assertTrue(not_commented_rewrite_rule in conf_line_set)
+
+        self.assertTrue(commented_cond1 in conf_line_set)
+        self.assertTrue(commented_cond2 in conf_line_set)
+        self.assertTrue(commented_rewrite_rule in conf_line_set)
+        mock_get_utility().add_message.assert_called_once_with(mock.ANY,
+                                                               mock.ANY)
+
 
     def get_achalls(self):
         """Return testing achallenges."""

certbot-apache/certbot_apache/tests/display_ops_test.py

@@ -38,7 +38,7 @@ class SelectVhostTest(unittest.TestCase):
         try:
             self._call(self.vhosts)
         except errors.MissingCommandlineFlag as e:
-            self.assertTrue("VirtualHost directives" in e.message)
+            self.assertTrue("vhost ambiguity" in e.message)
 
     @mock.patch("certbot_apache.display_ops.zope.component.getUtility")
     def test_more_info_cancel(self, mock_util):

certbot-apache/certbot_apache/tests/obj_test.py

@@ -22,6 +22,9 @@ class VirtualHostTest(unittest.TestCase):
         self.vhost2 = VirtualHost(
             "fp", "vhp", set([self.addr2]), False, False, "localhost")
 
+    def test_repr(self):
+        self.assertEqual(repr(self.addr2), "certbot_apache.obj.Addr(('127.0.0.1', '443'))")
+
     def test_eq(self):
         self.assertTrue(self.vhost1b == self.vhost1)
         self.assertFalse(self.vhost1 == self.vhost2)

certbot-apache/certbot_apache/tls_sni_01.py

@@ -129,6 +129,7 @@ class ApacheTlsSni01(common.TLSSNI01):
             # because it's a new vhost that's not configured yet (GH #677),
             # or perhaps because there were multiple <VirtualHost> sections
             # in the config file (GH #1042).  See also GH #2600.
+            logger.warn("Falling back to default vhost %s...", default_addr)
             addrs.add(default_addr)
             return addrs
 

certbot-nginx/certbot_nginx/configurator.py

@@ -152,17 +152,17 @@ class NginxConfigurator(common.Plugin):
                 "install a cert.")
 
         vhost = self.choose_vhost(domain)
-        cert_directives = [['ssl_certificate', fullchain_path],
-                           ['ssl_certificate_key', key_path]]
+        cert_directives = [['\n', 'ssl_certificate', ' ', fullchain_path],
+                           ['\n', 'ssl_certificate_key', ' ', key_path]]
 
         # OCSP stapling was introduced in Nginx 1.3.7. If we have that version
         # or greater, add config settings for it.
         stapling_directives = []
         if self.version >= (1, 3, 7):
             stapling_directives = [
-                ['ssl_trusted_certificate', chain_path],
-                ['ssl_stapling', 'on'],
-                ['ssl_stapling_verify', 'on']]
+                ['\n', 'ssl_trusted_certificate', ' ', chain_path],
+                ['\n', 'ssl_stapling', ' ', 'on'],
+                ['\n', 'ssl_stapling_verify', ' ', 'on'], ['\n']]
 
         if len(stapling_directives) != 0 and not chain_path:
             raise errors.PluginError(
@@ -225,7 +225,7 @@ class NginxConfigurator(common.Plugin):
         if not matches:
             # No matches. Create a new vhost with this name in nginx.conf.
             filep = self.parser.loc["root"]
-            new_block = [['server'], [['server_name', target_name]]]
+            new_block = [['server'], [['\n', 'server_name', ' ', target_name]]]
             self.parser.add_http_directives(filep, new_block)
             vhost = obj.VirtualHost(filep, set([]), False, True,
                                     set([target_name]), list(new_block[1]))
@@ -337,10 +337,10 @@ class NginxConfigurator(common.Plugin):
 
         """
         snakeoil_cert, snakeoil_key = self._get_snakeoil_paths()
-        ssl_block = [['listen', '{0} ssl'.format(self.config.tls_sni_01_port)],
-                     ['ssl_certificate', snakeoil_cert],
-                     ['ssl_certificate_key', snakeoil_key],
-                     ['include', self.parser.loc["ssl_options"]]]
+        ssl_block = [['\n', 'listen', ' ', '{0} ssl'.format(self.config.tls_sni_01_port)],
+                     ['\n', 'ssl_certificate', ' ', snakeoil_cert],
+                     ['\n', 'ssl_certificate_key', ' ', snakeoil_key],
+                     ['\n', 'include', ' ', self.parser.loc["ssl_options"]]]
         self.parser.add_server_directives(
             vhost.filep, vhost.names, ssl_block, replace=False)
         vhost.ssl = True
@@ -689,11 +689,6 @@ def nginx_restart(nginx_ctl, nginx_conf="/etc/nginx.conf"):
 
 def temp_install(options_ssl):
     """Temporary install for convenience."""
-    # WARNING: THIS IS A POTENTIAL SECURITY VULNERABILITY
-    # THIS SHOULD BE HANDLED BY THE PACKAGE MANAGER
-    # AND TAKEN OUT BEFORE RELEASE, INSTEAD
-    # SHOWING A NICE ERROR MESSAGE ABOUT THE PROBLEM.
-
     # Check to make sure options-ssl.conf is installed
     if not os.path.isfile(options_ssl):
         shutil.copyfile(constants.MOD_SSL_CONF_SRC, options_ssl)

certbot-nginx/certbot_nginx/nginxparser.py

@@ -1,23 +1,30 @@
 """Very low-level nginx config parser based on pyparsing."""
+# Forked from https://github.com/fatiherikli/nginxparser (MIT Licensed)
+import copy
+import logging
 import string
 
 from pyparsing import (
-    Literal, White, Word, alphanums, CharsNotIn, Forward, Group,
+    Literal, White, Word, alphanums, CharsNotIn, Combine, Forward, Group,
     Optional, OneOrMore, Regex, ZeroOrMore)
 from pyparsing import stringEnd
 from pyparsing import restOfLine
 
+logger = logging.getLogger(__name__)
 
 class RawNginxParser(object):
     # pylint: disable=expression-not-assigned
     """A class that parses nginx configuration with pyparsing."""
 
     # constants
+    space = Optional(White())
+    nonspace = Regex(r"\S+")
     left_bracket = Literal("{").suppress()
-    right_bracket = Literal("}").suppress()
+    right_bracket = space.leaveWhitespace() + Literal("}").suppress()
     semicolon = Literal(";").suppress()
-    space = White().suppress()
     key = Word(alphanums + "_/+-.")
+    dollar_var = Combine(Literal('$') + nonspace)
+    condition = Regex(r"\(.+\)")
     # Matches anything that is not a special character AND any chars in single
     # or double quotes
     value = Regex(r"((\".*\")?(\'.*\')?[^\{\};,]?)+")
@@ -26,20 +33,25 @@ class RawNginxParser(object):
     modifier = Literal("=") | Literal("~*") | Literal("~") | Literal("^~")
 
     # rules
-    comment = Literal('#') + restOfLine()
-    assignment = (key + Optional(space + value, default=None) + semicolon)
-    location_statement = Optional(space + modifier) + Optional(space + location)
-    if_statement = Literal("if") + space + Regex(r"\(.+\)") + space
-    map_statement = Literal("map") + space + Regex(r"\S+") + space + Regex(r"\$\S+") + space
+    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
+    map_statement = space + Literal("map") + space + nonspace + space + dollar_var + space
     block = Forward()
 
     block << Group(
-        (Group(key + location_statement) ^ Group(if_statement) ^ Group(map_statement)) +
+        # key could for instance be "server" or "http", or "location" (in which case
+        # location_statement needs to have a non-empty location)
+        (Group(space + key + location_statement) ^ Group(if_statement) ^
+        Group(map_statement)).leaveWhitespace() +
         left_bracket +
-        Group(ZeroOrMore(Group(comment | assignment) | block)) +
+        Group(ZeroOrMore(Group(comment | assignment) | block) + space).leaveWhitespace() +
         right_bracket)
 
-    script = OneOrMore(Group(comment | assignment) ^ block) + stringEnd
+    script = OneOrMore(Group(comment | assignment) ^ block) + space + stringEnd
+    script.parseWithTabs()
 
     def __init__(self, source):
         self.source = source
@@ -52,42 +64,47 @@ class RawNginxParser(object):
         """Returns the parsed tree as a list."""
         return self.parse().asList()
 
-
 class RawNginxDumper(object):
     # pylint: disable=too-few-public-methods
     """A class that dumps nginx configuration from the provided tree."""
-    def __init__(self, blocks, indentation=4):
+    def __init__(self, blocks):
         self.blocks = blocks
-        self.indentation = indentation
 
-    def __iter__(self, blocks=None, current_indent=0, spacer=' '):
+    def __iter__(self, blocks=None):
         """Iterates the dumped nginx content."""
         blocks = blocks or self.blocks
-        for key, values in blocks:
-            indentation = spacer * current_indent
+        for b0 in blocks:
+            if isinstance(b0, str):
+                yield b0
+                continue
+            b = copy.deepcopy(b0)
+            if spacey(b[0]):
+                yield b.pop(0) # indentation
+                if not b:
+                    continue
+            key, values = b.pop(0), b.pop(0)
+
             if isinstance(key, list):
-                if current_indent:
-                    yield ''
-                yield indentation + spacer.join(key) + ' {'
-
+                yield "".join(key) + '{'
                 for parameter in values:
-                    dumped = self.__iter__([parameter], current_indent + self.indentation)
-                    for line in dumped:
+                    for line in self.__iter__([parameter]): # negate "for b0 in blocks"
                         yield line
-
-                yield indentation + '}'
+                yield '}'
             else:
-                if key == '#':
-                    yield spacer * current_indent + key + values
-                else:
-                    if values is None:
-                        yield spacer * current_indent + key + ';'
-                    else:
-                        yield spacer * current_indent + key + spacer + values + ';'
+                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 + ';'
 
     def __str__(self):
         """Return the parsed block as a string."""
-        return '\n'.join(self) + '\n'
+        return ''.join(self)
 
 
 # Shortcut functions to respect Python's serialization interface
@@ -101,7 +118,7 @@ def loads(source):
     :rtype: list
 
     """
-    return RawNginxParser(source).as_list()
+    return UnspacedList(RawNginxParser(source).as_list())
 
 
 def load(_file):
@@ -115,24 +132,143 @@ def load(_file):
     return loads(_file.read())
 
 
-def dumps(blocks, indentation=4):
+def dumps(blocks):
     """Dump to a string.
 
-    :param list block: The parsed tree
+    :param UnspacedList block: The parsed tree
     :param int indentation: The number of spaces to indent
     :rtype: str
 
     """
-    return str(RawNginxDumper(blocks, indentation))
+    return str(RawNginxDumper(blocks.spaced))
 
 
-def dump(blocks, _file, indentation=4):
+def dump(blocks, _file):
     """Dump to a file.
 
-    :param list block: The parsed tree
+    :param UnspacedList block: The parsed tree
     :param file _file: The file to dump to
     :param int indentation: The number of spaces to indent
     :rtype: NoneType
 
     """
-    return _file.write(dumps(blocks, indentation))
+    return _file.write(dumps(blocks))
+
+
+spacey = lambda x: (isinstance(x, str) and x.isspace()) or x == ''
+
+class UnspacedList(list):
+    """Wrap a list [of lists], making any whitespace entries magically invisible"""
+
+    def __init__(self, list_source):
+        # ensure our argument is not a generator, and duplicate any sublists
+        self.spaced = copy.deepcopy(list(list_source))
+        self.dirty = False
+
+        # Turn self into a version of the source list that has spaces removed
+        # and all sub-lists also UnspacedList()ed
+        list.__init__(self, list_source)
+        for i, entry in reversed(list(enumerate(self))):
+            if isinstance(entry, list):
+                sublist = UnspacedList(entry)
+                list.__setitem__(self, i, sublist)
+                self.spaced[i] = sublist.spaced
+            elif spacey(entry):
+                # don't delete comments
+                if "#" not in self[:i]:
+                    list.__delitem__(self, i)
+
+    def _coerce(self, inbound):
+        """
+        Coerce some inbound object to be appropriately usable in this object
+
+        :param inbound: string or None or list or UnspacedList
+        :returns: (coerced UnspacedList or string or None, spaced equivalent)
+        :rtype: tuple
+
+        """
+        if not isinstance(inbound, list):                      # str or None
+            return (inbound, inbound)
+        else:
+            if not hasattr(inbound, "spaced"):
+                inbound = UnspacedList(inbound)
+            return (inbound, inbound.spaced)
+
+
+    def insert(self, i, x):
+        item, spaced_item = self._coerce(x)
+        self.spaced.insert(self._spaced_position(i), spaced_item)
+        list.insert(self, i, item)
+        self.dirty = True
+
+    def append(self, x):
+        item, spaced_item = self._coerce(x)
+        self.spaced.append(spaced_item)
+        list.append(self, item)
+        self.dirty = True
+
+    def extend(self, x):
+        item, spaced_item = self._coerce(x)
+        self.spaced.extend(spaced_item)
+        list.extend(self, item)
+        self.dirty = True
+
+    def __add__(self, other):
+        l = copy.deepcopy(self)
+        l.extend(other)
+        l.dirty = True
+        return l
+
+    def pop(self, _i=None):
+        raise NotImplementedError("UnspacedList.pop() not yet implemented")
+    def remove(self, _):
+        raise NotImplementedError("UnspacedList.remove() not yet implemented")
+    def reverse(self):
+        raise NotImplementedError("UnspacedList.reverse() not yet implemented")
+    def sort(self, _cmp=None, _key=None, _Rev=None):
+        raise NotImplementedError("UnspacedList.sort() not yet implemented")
+    def __setslice__(self, _i, _j, _newslice):
+        raise NotImplementedError("Slice operations on UnspacedLists not yet implemented")
+
+    def __setitem__(self, i, value):
+        if isinstance(i, slice):
+            raise NotImplementedError("Slice operations on UnspacedLists not yet implemented")
+        item, spaced_item = self._coerce(value)
+        self.spaced.__setitem__(self._spaced_position(i), spaced_item)
+        list.__setitem__(self, i, item)
+        self.dirty = True
+
+    def __delitem__(self, i):
+        self.spaced.__delitem__(self._spaced_position(i))
+        list.__delitem__(self, i)
+        self.dirty = True
+
+    def __deepcopy__(self, memo):
+        l = UnspacedList(self[:])
+        l.spaced = copy.deepcopy(self.spaced, memo=memo)
+        l.dirty = self.dirty
+        return l
+
+    def is_dirty(self):
+        """Recurse through the parse tree to figure out if any sublists are dirty"""
+        if self.dirty:
+            return True
+        return any((isinstance(x, list) and x.is_dirty() for x in self))
+
+    def _spaced_position(self, idx):
+        "Convert from indexes in the unspaced list to positions in the spaced one"
+        pos = spaces = 0
+        # Normalize indexes like list[-1] etc, and save the result
+        if idx < 0:
+            idx = len(self) + idx
+        if not 0 <= idx < len(self):
+            raise IndexError("list index out of range")
+        idx0 = idx
+        # Count the number of spaces in the spaced list before idx in the unspaced one
+        while idx != -1:
+            if spacey(self.spaced[pos]):
+                spaces += 1
+            else:
+                idx -= 1
+            pos += 1
+        return idx0 + spaces

certbot-nginx/certbot_nginx/obj.py

@@ -85,6 +85,9 @@ class Addr(common.Addr):
 
         return parts
 
+    def __repr__(self):
+        return "Addr(" + self.__str__() + ")"
+
     def __eq__(self, other):
         if isinstance(other, self.__class__):
             return (self.tup == other.tup and
@@ -126,6 +129,9 @@ class VirtualHost(object):  # pylint: disable=too-few-public-methods
                 "enabled: %s" % (self.filep, addr_str,
                                  self.names, self.ssl, self.enabled))
 
+    def __repr__(self):
+        return "VirtualHost(" + self.__str__().replace("\n", ", ") + ")\n"
+
     def __eq__(self, other):
         if isinstance(other, self.__class__):
             return (self.filep == other.filep and

certbot-nginx/certbot_nginx/parser.py

@@ -1,4 +1,5 @@
 """NginxParser is a member object of the NginxConfigurator class."""
+import copy
 import glob
 import logging
 import os
@@ -17,7 +18,7 @@ logger = logging.getLogger(__name__)
 class NginxParser(object):
     """Class handles the fine details of parsing the Nginx Configuration.
 
-    :ivar str root: Normalized abosulte path to the server root
+    :ivar str root: Normalized absolute path to the server root
         directory. Without trailing slash.
     :ivar dict parsed: Mapping of file paths to parsed trees
 
@@ -113,6 +114,7 @@ class NginxParser(object):
         for filename in servers:
             for server in servers[filename]:
                 # Parse the server block into a VirtualHost object
+
                 parsed_server = parse_server(server)
                 vhost = obj.VirtualHost(filename,
                                         parsed_server['addrs'],
@@ -132,7 +134,7 @@ class NginxParser(object):
         :rtype: list
 
         """
-        result = list(block)  # Copy the list to keep self.parsed idempotent
+        result = copy.deepcopy(block)  # Copy the list to keep self.parsed idempotent
         for directive in block:
             if _is_include_directive(directive):
                 included_files = glob.glob(
@@ -153,7 +155,9 @@ class NginxParser(object):
         :rtype: list
 
         """
-        files = glob.glob(filepath)
+        files = glob.glob(filepath) # nginx on unix calls glob(3) for this
+                                    # XXX Windows nginx uses FindFirstFile, and
+                                    # should have a narrower call here
         trees = []
         for item in files:
             if item in self.parsed and not override:
@@ -201,21 +205,27 @@ class NginxParser(object):
         raise errors.NoInstallationError(
             "Could not find configuration root")
 
-    def filedump(self, ext='tmp'):
+    def filedump(self, ext='tmp', lazy=True):
         """Dumps parsed configurations into files.
 
         :param str ext: The file extension to use for the dumped files. If
             empty, this overrides the existing conf files.
+        :param bool lazy: Only write files that have been modified
 
         """
+        # Best-effort atomicity is enforced above us by reverter.py
         for filename in self.parsed:
             tree = self.parsed[filename]
             if ext:
                 filename = filename + os.path.extsep + ext
             try:
-                logger.debug('Dumping to %s:\n%s', filename, nginxparser.dumps(tree))
+                if lazy and not tree.is_dirty():
+                    continue
+                out = nginxparser.dumps(tree)
+                logger.debug('Writing nginx conf tree to %s:\n%s', filename, out)
                 with open(filename, 'w') as _file:
-                    nginxparser.dump(tree, _file)
+                    _file.write(out)
+
             except IOError:
                 logger.error("Could not open file for writing: %s", filename)
 
@@ -463,6 +473,8 @@ def parse_server(server):
                      'names': set()}
 
     for directive in server:
+        if not directive:
+            continue
         if directive[0] == 'listen':
             addr = obj.Addr.fromstring(directive[1])
             parsed_server['addrs'].add(addr)
@@ -503,6 +515,11 @@ def _add_directive(block, directive, replace):
     See _add_directives for more documentation.
 
     """
+    directive = nginxparser.UnspacedList(directive)
+    if len(directive) == 0:
+        # whitespace
+        block.append(directive)
+        return
     location = -1
     # Find the index of a config line where the name of the directive matches
     # the name of the directive we want to add.

certbot-nginx/certbot_nginx/tests/configurator_test.py

@@ -83,7 +83,7 @@ class NginxConfiguratorTest(util.NginxTest):
         filep = self.config.parser.abs_path('sites-enabled/example.com')
         self.config.parser.add_server_directives(
             filep, set(['.example.com', 'example.*']),
-            [['listen', '5001 ssl']],
+            [['listen', ' ', '5001 ssl']],
             replace=False)
         self.config.save()
 
@@ -265,7 +265,8 @@ class NginxConfiguratorTest(util.NginxTest):
 
     @mock.patch("certbot_nginx.configurator.tls_sni_01.NginxTlsSni01.perform")
     @mock.patch("certbot_nginx.configurator.NginxConfigurator.restart")
-    def test_perform(self, mock_restart, mock_perform):
+    @mock.patch("certbot_nginx.configurator.NginxConfigurator.revert_challenge_config")
+    def test_perform_and_cleanup(self, mock_revert, mock_restart, mock_perform):
         # Only tests functionality specific to configurator.perform
         # Note: As more challenges are offered this will have to be expanded
         achall1 = achallenges.KeyAuthorizationAnnotatedChallenge(
@@ -291,7 +292,11 @@ class NginxConfiguratorTest(util.NginxTest):
 
         self.assertEqual(mock_perform.call_count, 1)
         self.assertEqual(responses, expected)
-        self.assertEqual(mock_restart.call_count, 1)
+
+        self.config.cleanup([achall1, achall2])
+        self.assertEqual(0, self.config._chall_out) # pylint: disable=protected-access
+        self.assertEqual(mock_revert.call_count, 1)
+        self.assertEqual(mock_restart.call_count, 2)
 
     @mock.patch("certbot_nginx.configurator.subprocess.Popen")
     def test_get_version(self, mock_popen):

certbot-nginx/certbot_nginx/tests/nginxparser_test.py

@@ -1,11 +1,13 @@
 """Test for certbot_nginx.nginxparser."""
+import copy
 import operator
+import os
 import unittest
 
 from pyparsing import ParseException
 
 from certbot_nginx.nginxparser import (
-    RawNginxParser, loads, load, dumps, dump)
+    RawNginxParser, loads, load, dumps, dump, UnspacedList)
 from certbot_nginx.tests import util
 
 
@@ -17,39 +19,39 @@ class TestRawNginxParser(unittest.TestCase):
 
     def test_assignments(self):
         parsed = RawNginxParser.assignment.parseString('root /test;').asList()
-        self.assertEqual(parsed, ['root', '/test'])
-        parsed = RawNginxParser.assignment.parseString('root /test;'
-                                                       'foo bar;').asList()
-        self.assertEqual(parsed, ['root', '/test'], ['foo', 'bar'])
+        self.assertEqual(parsed, ['root', ' ', '/test'])
+        parsed = RawNginxParser.assignment.parseString('root /test;foo bar;').asList()
+        self.assertEqual(parsed, ['root', ' ', '/test'], ['foo', ' ', 'bar'])
 
     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'], []]])
-        parsed = RawNginxParser.block.parseString('foo { bar foo; }').asList()
-        self.assertEqual(parsed, [[['foo'], [['bar', 'foo']]]])
+        self.assertEqual(parsed, [[['location', ' ', '/foo'], []]])
+        parsed = RawNginxParser.block.parseString('foo { bar foo ; }').asList()
+        self.assertEqual(parsed, [[['foo', ' '], [[' ', 'bar', ' ', 'foo '], ' ']]])
 
     def test_nested_blocks(self):
         parsed = RawNginxParser.block.parseString('foo { bar {} }').asList()
         block, content = FIRST(parsed)
-        self.assertEqual(FIRST(content), [['bar'], []])
+        self.assertEqual(FIRST(content), [[' ', 'bar', ' '], []])
         self.assertEqual(FIRST(block), 'foo')
 
     def test_dump_as_string(self):
-        dumped = dumps([
-            ['user', 'www-data'],
-            [['server'], [
-                ['listen', '80'],
-                ['server_name', 'foo.com'],
-                ['root', '/home/ubuntu/sites/foo/'],
-                [['location', '/status'], [
-                    ['check_status', None],
-                    [['types'], [['image/jpeg', 'jpg']]],
+        dumped = dumps(UnspacedList([
+            ['user', ' ', 'www-data'],
+            [['\n', 'server', ' '], [
+                ['\n    ', 'listen', ' ', '80'],
+                ['\n    ', 'server_name', ' ', 'foo.com'],
+                ['\n    ', 'root', ' ', '/home/ubuntu/sites/foo/'],
+                [['\n\n    ', 'location', ' ', '/status', ' '], [
+                    ['\n        ', 'check_status', ''],
+                    [['\n\n        ', 'types', ' '],
+                    [['\n            ', 'image/jpeg', ' ', 'jpg']]],
                 ]]
-            ]]])
+            ]]]))
 
-        self.assertEqual(dumped,
+        self.assertEqual(dumped.split('\n'),
                          'user www-data;\n'
                          'server {\n'
                          '    listen 80;\n'
@@ -60,10 +62,7 @@ class TestRawNginxParser(unittest.TestCase):
                          '        check_status;\n'
                          '\n'
                          '        types {\n'
-                         '            image/jpeg jpg;\n'
-                         '        }\n'
-                         '    }\n'
-                         '}\n')
+                         '            image/jpeg jpg;}}}'.split('\n'))
 
     def test_parse_from_file(self):
         with open(util.get_data_filename('foo.conf')) as handle:
@@ -116,24 +115,29 @@ class TestRawNginxParser(unittest.TestCase):
 
     def test_dump_as_file(self):
         with open(util.get_data_filename('nginx.conf')) as handle:
-            parsed = util.filter_comments(load(handle))
-        parsed[-1][-1].append([['server'],
-                               [['listen', '443 ssl'],
-                                ['server_name', 'localhost'],
-                                ['ssl_certificate', 'cert.pem'],
-                                ['ssl_certificate_key', 'cert.key'],
-                                ['ssl_session_cache', 'shared:SSL:1m'],
-                                ['ssl_session_timeout', '5m'],
-                                ['ssl_ciphers', 'HIGH:!aNULL:!MD5'],
-                                [['location', '/'],
-                                 [['root', 'html'],
-                                  ['index', 'index.html index.htm']]]]])
+            parsed = load(handle)
+        parsed[-1][-1].append(UnspacedList([['server'],
+                               [['listen', ' ', '443 ssl'],
+                                ['server_name', ' ', 'localhost'],
+                                ['ssl_certificate', ' ', 'cert.pem'],
+                                ['ssl_certificate_key', ' ', 'cert.key'],
+                                ['ssl_session_cache', ' ', 'shared:SSL:1m'],
+                                ['ssl_session_timeout', ' ', '5m'],
+                                ['ssl_ciphers', ' ', 'HIGH:!aNULL:!MD5'],
+                                [['location', ' ', '/'],
+                                 [['root', ' ', 'html'],
+                                  ['index', ' ', 'index.html index.htm']]]]]))
 
         with open(util.get_data_filename('nginx.new.conf'), 'w') as handle:
             dump(parsed, handle)
         with open(util.get_data_filename('nginx.new.conf')) as handle:
-            parsed_new = util.filter_comments(load(handle))
-        self.assertEquals(parsed, parsed_new)
+            parsed_new = load(handle)
+        try:
+            self.maxDiff = None
+            self.assertEquals(parsed[0], parsed_new[0])
+            self.assertEquals(parsed[1:], parsed_new[1:])
+        finally:
+            os.unlink(util.get_data_filename('nginx.new.conf'))
 
     def test_comments(self):
         with open(util.get_data_filename('minimalistic_comments.conf')) as handle:
@@ -145,20 +149,23 @@ class TestRawNginxParser(unittest.TestCase):
         with open(util.get_data_filename('minimalistic_comments.new.conf')) as handle:
             parsed_new = load(handle)
 
-        self.assertEquals(parsed, parsed_new)
+        try:
+            self.assertEquals(parsed, parsed_new)
 
-        self.assertEqual(parsed_new, [
-            ['#', " Use bar.conf when it's a full moon!"],
-            ['include', 'foo.conf'],
-            ['#', ' Kilroy was here'],
-            ['check_status', None],
-            [['server'],
-             [['#', ''],
-              ['#', " Don't forget to open up your firewall!"],
-              ['#', ''],
-              ['listen', '1234'],
-              ['#', ' listen 80;']]],
-        ])
+            self.assertEqual(parsed_new, [
+                ['#', " Use bar.conf when it's a full moon!"],
+                ['include', 'foo.conf'],
+                ['#', ' Kilroy was here'],
+                ['check_status'],
+                [['server'],
+                 [['#', ''],
+                  ['#', " Don't forget to open up your firewall!"],
+                  ['#', ''],
+                  ['listen', '1234'],
+                  ['#', ' listen 80;']]],
+            ])
+        finally:
+            os.unlink(util.get_data_filename('minimalistic_comments.new.conf'))
 
     def test_issue_518(self):
         parsed = loads('if ($http_accept ~* "webp") { set $webp "true"; }')
@@ -168,5 +175,73 @@ class TestRawNginxParser(unittest.TestCase):
              [['set', '$webp "true"']]]
         ])
 
+class TestUnspacedList(unittest.TestCase):
+    """Test the UnspacedList data structure"""
+    def setUp(self):
+        self.a = ["\n    ", "things", " ", "quirk"]
+        self.b = ["y", " "]
+        self.l = self.a[:]
+        self.l2 = self.b[:]
+        self.ul = UnspacedList(self.l)
+        self.ul2 = UnspacedList(self.l2)
+
+    def test_construction(self):
+        self.assertEqual(self.ul, ["things", "quirk"])
+        self.assertEqual(self.ul2, ["y"])
+
+    def test_append(self):
+        ul3 = copy.deepcopy(self.ul)
+        ul3.append("wise")
+        self.assertEqual(ul3, ["things", "quirk", "wise"])
+        self.assertEqual(ul3.spaced, self.a + ["wise"])
+
+    def test_add(self):
+        ul3 = self.ul + self.ul2
+        self.assertEqual(ul3, ["things", "quirk", "y"])
+        self.assertEqual(ul3.spaced, self.a + self.b)
+        self.assertEqual(self.ul.spaced, self.a)
+        ul3 = self.ul + self.l2
+        self.assertEqual(ul3, ["things", "quirk", "y"])
+        self.assertEqual(ul3.spaced, self.a + self.b)
+
+    def test_extend(self):
+        ul3 = copy.deepcopy(self.ul)
+        ul3.extend(self.ul2)
+        self.assertEqual(ul3, ["things", "quirk", "y"])
+        self.assertEqual(ul3.spaced, self.a + self.b)
+        self.assertEqual(self.ul.spaced, self.a)
+
+    def test_set(self):
+        ul3 = copy.deepcopy(self.ul)
+        ul3[0] = "zither"
+        l = ["\n ", "zather", "zest"]
+        ul3[1] = UnspacedList(l)
+        self.assertEqual(ul3, ["zither", ["zather", "zest"]])
+        self.assertEqual(ul3.spaced, [self.a[0], "zither", " ", l])
+
+    def test_get(self):
+        self.assertRaises(IndexError, self.ul2.__getitem__, 2)
+        self.assertRaises(IndexError, self.ul2.__getitem__, -3)
+
+    def test_rawlists(self):
+        ul3 = copy.deepcopy(self.ul)
+        ul3.insert(0, "some")
+        ul3.append("why")
+        ul3.extend(["did", "whether"])
+        del ul3[2]
+        self.assertEqual(ul3, ["some", "things", "why", "did", "whether"])
+
+    def test_is_dirty(self):
+        self.assertEqual(False, self.ul2.is_dirty())
+        ul3 = UnspacedList([])
+        ul3.append(self.ul)
+        self.assertEqual(False, self.ul.is_dirty())
+        self.assertEqual(True, ul3.is_dirty())
+        ul4 = UnspacedList([[1], [2, 3, 4]])
+        self.assertEqual(False, ul4.is_dirty())
+        ul4[1][2] = 5
+        self.assertEqual(True, ul4.is_dirty())
+
+
 if __name__ == '__main__':
     unittest.main()  # pragma: no cover

certbot-nginx/certbot_nginx/tests/parser_test.py

@@ -66,7 +66,7 @@ class NginxParserTest(util.NginxTest):
 
     def test_filedump(self):
         nparser = parser.NginxParser(self.config_path, self.ssl_options)
-        nparser.filedump('test')
+        nparser.filedump('test', lazy=False)
         # pylint: disable=protected-access
         parsed = nparser._parse_files(nparser.abs_path(
             'sites-enabled/example.com.test'))
@@ -126,7 +126,7 @@ class NginxParserTest(util.NginxTest):
         nparser.add_server_directives(nparser.abs_path('nginx.conf'),
                                       set(['localhost',
                                            r'~^(www\.)?(example|bar)\.']),
-                                      [['foo', 'bar'], ['ssl_certificate',
+                                      [['foo', 'bar'], ['\n ', 'ssl_certificate', ' ',
                                                         '/etc/ssl/cert.pem']],
                                       replace=False)
         ssl_re = re.compile(r'\n\s+ssl_certificate /etc/ssl/cert.pem')

certbot-nginx/certbot_nginx/tests/testdata/etc_nginx/minimalistic_comments.new.conf

@@ -1,11 +0,0 @@
-# Use bar.conf when it's a full moon!
-include foo.conf;
-# Kilroy was here
-check_status;
-server {
-    #
-    # Don't forget to open up your firewall!
-    #
-    listen 1234;
-    # listen 80;
-}

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

@@ -1,83 +0,0 @@
-user nobody;
-worker_processes 1;
-error_log logs/error.log;
-error_log logs/error.log  notice;
-error_log logs/error.log  info;
-pid logs/nginx.pid;
-events {
-    worker_connections 1024;
-}
-include foo.conf;
-http {
-    include mime.types;
-    include sites-enabled/*;
-    default_type application/octet-stream;
-    log_format main  '$remote_addr - $remote_user [$time_local] "$request" '
-                      '$status $body_bytes_sent "$http_referer" '
-                      '"$http_user_agent" "$http_x_forwarded_for"';
-    access_log logs/access.log  main;
-    sendfile on;
-    tcp_nopush on;
-    keepalive_timeout 0;
-    gzip on;
-
-    server {
-        listen 8080;
-        server_name localhost;
-        server_name ~^(www\.)?(example|bar)\.;
-        charset koi8-r;
-        access_log logs/host.access.log  main;
-
-        location / {
-            root html;
-            index index.html index.htm;
-        }
-        error_page 404              /404.html;
-        error_page 500 502 503 504  /50x.html;
-
-        location = /50x.html {
-            root html;
-        }
-
-        location ~ \.php$ {
-            proxy_pass http://127.0.0.1;
-        }
-
-        location ~ \.php$ {
-            root html;
-            fastcgi_pass 127.0.0.1:9000;
-            fastcgi_index index.php;
-            fastcgi_param SCRIPT_FILENAME  /scripts$fastcgi_script_name;
-        }
-
-        location ~ /\.ht {
-            deny all;
-        }
-    }
-
-    server {
-        listen 8000;
-        listen somename:8080;
-        include server.conf;
-
-        location / {
-            root html;
-            index index.html index.htm;
-        }
-    }
-
-    server {
-        listen 443 ssl;
-        server_name localhost;
-        ssl_certificate cert.pem;
-        ssl_certificate_key cert.key;
-        ssl_session_cache shared:SSL:1m;
-        ssl_session_timeout 5m;
-        ssl_ciphers HIGH:!aNULL:!MD5;
-
-        location / {
-            root html;
-            index index.html index.htm;
-        }
-    }
-}

certbot-nginx/certbot_nginx/tests/tls_sni_01_test.py

@@ -80,7 +80,7 @@ class TlsSniPerformTest(util.NginxTest):
 
         mock_setup_cert.assert_called_once_with(self.achalls[0])
         self.assertEqual([response], responses)
-        self.assertEqual(mock_save.call_count, 2)
+        self.assertEqual(mock_save.call_count, 1)
 
         # Make sure challenge config is included in main config
         http = self.sni.configurator.parser.parsed[

certbot-nginx/certbot_nginx/tests/util.py

@@ -1,4 +1,5 @@
 """Common utilities for certbot_nginx."""
+import copy
 import os
 import pkg_resources
 import unittest
@@ -82,12 +83,15 @@ def filter_comments(tree):
 
     def traverse(tree):
         """Generator dropping comment nodes"""
-        for key, values in tree:
+        for entry in tree:
+            key, values = entry
             if isinstance(key, list):
-                yield [key, filter_comments(values)]
+                new = copy.deepcopy(entry)
+                new[1] = filter_comments(values)
+                yield new
             else:
                 if key != '#':
-                    yield [key, values]
+                    yield entry
 
     return list(traverse(tree))
 

certbot-nginx/certbot_nginx/tls_sni_01.py

@@ -46,8 +46,6 @@ class NginxTlsSni01(common.TLSSNI01):
         if not self.achalls:
             return []
 
-        self.configurator.save()
-
         addresses = []
         default_addr = "{0} default_server ssl".format(
             self.configurator.config.tls_sni_01_port)
@@ -93,10 +91,10 @@ class NginxTlsSni01(common.TLSSNI01):
         # Add the 'include' statement for the challenges if it doesn't exist
         # already in the main config
         included = False
-        include_directive = ['include', self.challenge_conf]
+        include_directive = ['include', ' ', self.challenge_conf]
         root = self.configurator.parser.loc["root"]
 
-        bucket_directive = ['server_names_hash_bucket_size', '128']
+        bucket_directive = ['server_names_hash_bucket_size', ' ', '128']
 
         main = self.configurator.parser.parsed[root]
         for key, body in main:
@@ -118,6 +116,7 @@ class NginxTlsSni01(common.TLSSNI01):
 
         config = [self._make_server_block(pair[0], pair[1])
                   for pair in itertools.izip(self.achalls, ll_addrs)]
+        config = nginxparser.UnspacedList(config)
 
         self.configurator.reverter.register_file_creation(
             True, self.challenge_conf)
@@ -142,19 +141,19 @@ class NginxTlsSni01(common.TLSSNI01):
         document_root = os.path.join(
             self.configurator.config.work_dir, "tls_sni_01_page")
 
-        block = [['listen', str(addr)] for addr in addrs]
+        block = [['listen', ' ', str(addr)] for addr in addrs]
 
-        block.extend([['server_name',
+        block.extend([['server_name', ' ',
                        achall.response(achall.account_key).z_domain],
-                      ['include', self.configurator.parser.loc["ssl_options"]],
+                      ['include', ' ', self.configurator.parser.loc["ssl_options"]],
                       # access and error logs necessary for
                       # integration testing (non-root)
-                      ['access_log', os.path.join(
+                      ['access_log', ' ', os.path.join(
                           self.configurator.config.work_dir, 'access.log')],
-                      ['error_log', os.path.join(
+                      ['error_log', ' ', os.path.join(
                           self.configurator.config.work_dir, 'error.log')],
-                      ['ssl_certificate', self.get_cert_path(achall)],
-                      ['ssl_certificate_key', self.get_key_path(achall)],
-                      [['location', '/'], [['root', document_root]]]])
+                      ['ssl_certificate', ' ', self.get_cert_path(achall)],
+                      ['ssl_certificate_key', ' ', self.get_key_path(achall)],
+                      [['location', ' ', '/'], [['root', ' ', document_root]]]])
 
         return [['server'], block]

certbot/cli.py

@@ -721,10 +721,10 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False):  # pylint: dis
              "(both can be renewed in parallel)")
     helpful.add(
         "automation", "--os-packages-only", action="store_true",
-        help="(letsencrypt-auto only) install OS package dependencies and then stop")
+        help="(certbot-auto only) install OS package dependencies and then stop")
     helpful.add(
         "automation", "--no-self-upgrade", action="store_true",
-        help="(letsencrypt-auto only) prevent the letsencrypt-auto script from"
+        help="(certbot-auto only) prevent the certbot-auto script from"
              " upgrading itself to newer released versions")
     helpful.add(
         "automation", "-q", "--quiet", dest="quiet", action="store_true",
@@ -737,7 +737,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False):  # pylint: dis
         "really know what you're doing!")
     helpful.add(
         "testing", "--debug", action="store_true",
-        help="Show tracebacks in case of errors, and allow letsencrypt-auto "
+        help="Show tracebacks in case of errors, and allow certbot-auto "
              "execution on experimental platforms")
     helpful.add(
         "testing", "--no-verify-ssl", action="store_true",
@@ -773,7 +773,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False):  # pylint: dis
     helpful.add(
         "security", "--hsts", action="store_true",
         help="Add the Strict-Transport-Security header to every HTTP response."
-             " Forcing browser to use always use SSL for the domain."
+             " Forcing browser to always use SSL for the domain."
              " Defends against SSL Stripping.", dest="hsts", default=False)
     helpful.add(
         "security", "--no-hsts", action="store_false",

certbot/client.py

@@ -282,7 +282,7 @@ class Client(object):
                 "by your operating system package manager")
 
         if self.config.dry_run:
-            logger.info("Dry run: Skipping creating new lineage for %s",
+            logger.debug("Dry run: Skipping creating new lineage for %s",
                         domains[0])
             return None
         else:

certbot/constants.py

@@ -18,7 +18,7 @@ CLI_DEFAULTS = dict(
         os.path.join(os.environ.get("XDG_CONFIG_HOME", "~/.config"),
                      "letsencrypt", "cli.ini"),
     ],
-    verbose_count=-(logging.WARNING / 10),
+    verbose_count=-(logging.INFO / 10),
     server="https://acme-v01.api.letsencrypt.org/directory",
     rsa_key_size=2048,
     rollback_checkpoints=1,

certbot/display/util.py

@@ -1,4 +1,5 @@
 """Certbot display."""
+import logging
 import os
 import textwrap
 
@@ -9,6 +10,9 @@ from certbot import interfaces
 from certbot import errors
 from certbot.display import completer
 
+
+logger = logging.getLogger(__name__)
+
 WIDTH = 72
 HEIGHT = 20
 
@@ -29,6 +33,9 @@ CANCEL = "cancel"
 HELP = "help"
 """Display exit code when for when the user requests more help."""
 
+ESC = "esc"
+"""Display exit code when the user hits Escape"""
+
 
 def _wrap_lines(msg):
     """Format lines nicely to 80 chars.
@@ -51,6 +58,24 @@ def _wrap_lines(msg):
 
     return os.linesep.join(fixed_l)
 
+
+def _clean(dialog_result):
+    """Treat sundy python-dialog return codes as CANCEL
+
+    :param tuple dialog_result: (code, result)
+    :returns: the argument but with unknown codes set to -1 (Error)
+    :rtype: tuple
+    """
+    code, result = dialog_result
+    if code in (OK, HELP):
+        return dialog_result
+    elif code in (CANCEL, ESC):
+        return (CANCEL, result)
+    else:
+        logger.debug("Surprising dialog return code %s", code)
+        return (CANCEL, result)
+
+
 @zope.interface.implementer(interfaces.IDisplay)
 class NcursesDisplay(object):
     """Ncurses-based display."""
@@ -58,6 +83,7 @@ class NcursesDisplay(object):
     def __init__(self, width=WIDTH, height=HEIGHT):
         super(NcursesDisplay, self).__init__()
         self.dialog = dialog.Dialog()
+        assert OK == self.dialog.DIALOG_OK, "What kind of absurdity is this?"
         self.width = width
         self.height = height
 
@@ -92,7 +118,7 @@ class NcursesDisplay(object):
         :param dict unused_kwargs: absorbs default / cli_args
 
         :returns: tuple of the form (`code`, `index`) where
-            `code` - int display exit code
+            `code` - display exit code
             `int` - index of the selected item
         :rtype: tuple
 
@@ -111,7 +137,7 @@ class NcursesDisplay(object):
         # Can accept either tuples or just the actual choices
         if choices and isinstance(choices[0], tuple):
             # pylint: disable=star-args
-            code, selection = self.dialog.menu(message, **menu_options)
+            code, selection = _clean(self.dialog.menu(message, **menu_options))
 
             # Return the selection index
             for i, choice in enumerate(choices):
@@ -126,9 +152,9 @@ class NcursesDisplay(object):
                 (str(i), choice) for i, choice in enumerate(choices, 1)
             ]
             # pylint: disable=star-args
-            code, index = self.dialog.menu(message, **menu_options)
+            code, index = _clean(self.dialog.menu(message, **menu_options))
 
-            if code == CANCEL:
+            if code == CANCEL or index == "":
                 return code, -1
 
             return code, int(index) - 1
@@ -140,7 +166,7 @@ class NcursesDisplay(object):
         :param dict _kwargs: absorbs default / cli_args
 
         :returns: tuple of the form (`code`, `string`) where
-            `code` - int display exit code
+            `code` - display exit code
             `string` - input entered by the user
 
         """
@@ -148,7 +174,7 @@ class NcursesDisplay(object):
         # each section takes at least one line, plus extras if it's longer than self.width
         wordlines = [1 + (len(section) / self.width) for section in sections]
         height = 6 + sum(wordlines) + len(sections)
-        return self.dialog.inputbox(message, width=self.width, height=height)
+        return _clean(self.dialog.inputbox(message, width=self.width, height=height))
 
     def yesno(self, message, yes_label="Yes", no_label="No", **unused_kwargs):
         """Display a Yes/No dialog box.
@@ -179,13 +205,13 @@ class NcursesDisplay(object):
 
 
         :returns: tuple of the form (`code`, `list_tags`) where
-            `code` - int display exit code
+            `code` - display exit code
             `list_tags` - list of str tags selected by the user
 
         """
         choices = [(tag, "", default_status) for tag in tags]
-        return self.dialog.checklist(
-            message, width=self.width, height=self.height, choices=choices)
+        return _clean(self.dialog.checklist(
+            message, width=self.width, height=self.height, choices=choices))
 
     def directory_select(self, message, **unused_kwargs):
         """Display a directory selection screen.
@@ -193,14 +219,14 @@ class NcursesDisplay(object):
         :param str message: prompt to give the user
 
         :returns: tuple of the form (`code`, `string`) where
-            `code` - int display exit code
+            `code` - display exit code
             `string` - input entered by the user
 
         """
         root_directory = os.path.abspath(os.sep)
-        return self.dialog.dselect(
+        return _clean(self.dialog.dselect(
             filepath=root_directory, width=self.width,
-            height=self.height, help_button=True, title=message)
+            height=self.height, help_button=True, title=message))
 
 
 @zope.interface.implementer(interfaces.IDisplay)
@@ -355,7 +381,7 @@ class FileDisplay(object):
         :param str message: prompt to give the user
 
         :returns: tuple of the form (`code`, `string`) where
-            `code` - int display exit code
+            `code` - display exit code
             `string` - input entered by the user
 
         """

certbot/interfaces.py

@@ -180,6 +180,9 @@ class IAuthenticator(IPlugin):
     def cleanup(achalls):
         """Revert changes and shutdown after challenges complete.
 
+        This method should be able to revert all changes made by
+        perform, even if perform exited abnormally.
+
         :param list achalls: Non-empty (guaranteed) list of
             :class:`~certbot.achallenges.AnnotatedChallenge`
             instances, a subset of those previously passed to :func:`perform`.
@@ -238,6 +241,14 @@ class IInstaller(IPlugin):
 
     Represents any server that an X509 certificate can be placed.
 
+    It is assumed that :func:`save` is the only method that finalizes a
+    checkpoint. This is important to ensure that checkpoints are
+    restored in a consistent manner if requested by the user or in case
+    of an error.
+
+    Using :class:`certbot.reverter.Reverter` to implement checkpoints,
+    rollback, and recovery can dramatically simplify plugin development.
+
     """
 
     def get_all_names():
@@ -304,8 +315,11 @@ class IInstaller(IPlugin):
 
         Both title and temporary are needed because a save may be
         intended to be permanent, but the save is not ready to be a full
-        checkpoint. If an exception is raised, it is assumed a new
-        checkpoint was not created.
+        checkpoint.
+
+        It is assumed that at most one checkpoint is finalized by this
+        method. Additionally, if an exception is raised, it is assumed a
+        new checkpoint was not finalized.
 
         :param str title: The title of the save. If a title is given, the
             configuration will be saved as a new checkpoint and put in a

certbot/main.py

@@ -2,7 +2,6 @@
 from __future__ import print_function
 import atexit
 import dialog
-import errno
 import functools
 import logging.handlers
 import os
@@ -36,6 +35,13 @@ 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, "
+    "--logs-dir, and --work-dir to writeable paths."))
+
+
 logger = logging.getLogger(__name__)
 
 
@@ -84,14 +90,17 @@ def _auth_from_domains(le_client, config, domains, lineage=None):
     if action == "reinstall":
         # The lineage already exists; allow the caller to try installing
         # it without getting a new certificate at all.
+        logger.info("Keeping the existing certificate")
         return lineage, "reinstall"
 
     hooks.pre_hook(config)
     try:
         if action == "renew":
+            logger.info("Renewing an existing certificate")
             renewal.renew_cert(config, domains, le_client, lineage)
         elif action == "newcert":
             # TREAT AS NEW REQUEST
+            logger.info("Obtaining a new certificate")
             lineage = le_client.obtain_and_enroll_certificate(domains)
             if lineage is False:
                 raise errors.Error("Certificate could not be obtained")
@@ -527,7 +536,7 @@ def _csr_obtain_cert(config, le_client):
     csr, typ = config.actual_csr
     certr, chain = le_client.obtain_certificate_from_csr(config.domains, csr, typ)
     if config.dry_run:
-        logger.info(
+        logger.debug(
             "Dry run: skipping saving certificate to %s", config.cert_path)
     else:
         cert_path, _, cert_fullchain = le_client.save_certificate(
@@ -593,13 +602,8 @@ def setup_log_file_handler(config, logfile, fmt):
     try:
         handler = logging.handlers.RotatingFileHandler(
             log_file_path, maxBytes=2 ** 20, backupCount=10)
-    except IOError as e:
-        if e.errno == errno.EACCES:
-            msg = ("Access denied writing to {0}. To run as non-root, set " +
-                "--logs-dir, --config-dir, --work-dir to writable paths.")
-            raise errors.Error(msg.format(log_file_path))
-        else:
-            raise
+    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).
@@ -625,11 +629,12 @@ def _cli_log_handler(config, level, fmt):
 
 def setup_logging(config, cli_handler_factory, logfile):
     """Setup logging."""
-    fmt = "%(asctime)s:%(levelname)s:%(name)s:%(message)s"
+    file_fmt = "%(asctime)s:%(levelname)s:%(name)s:%(message)s"
+    cli_fmt = "%(message)s"
     level = -config.verbose_count * 10
     file_handler, log_file_path = setup_log_file_handler(
-        config, logfile=logfile, fmt=fmt)
-    cli_handler = cli_handler_factory(config, level, fmt)
+        config, logfile=logfile, fmt=file_fmt)
+    cli_handler = cli_handler_factory(config, level, cli_fmt)
 
     # TODO: use fileConfig?
 
@@ -698,6 +703,23 @@ def _handle_exception(exc_type, exc_value, trace, config):
             traceback.format_exception(exc_type, exc_value, trace)))
 
 
+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 main(cli_args=sys.argv[1:]):
     """Command line argument parsing and main script execution."""
     sys.excepthook = functools.partial(_handle_exception, config=None)
@@ -708,16 +730,16 @@ def main(cli_args=sys.argv[1:]):
     config = configuration.NamespaceConfig(args)
     zope.component.provideUtility(config)
 
-    # Setup logging ASAP, otherwise "No handlers could be found for
-    # logger ..." TODO: this should be done before plugins discovery
-    for directory in config.config_dir, config.work_dir:
-        util.make_or_verify_dir(
-            directory, constants.CONFIG_DIRS_MODE, os.geteuid(),
-            "--strict-permissions" in cli_args)
+    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
-    util.make_or_verify_dir(
-        config.logs_dir, 0o700, os.geteuid(), "--strict-permissions" in cli_args)
+    make_or_verify_core_dir(config.logs_dir, 0o700,
+                            os.geteuid(), config.strict_permissions)
+    # Setup logging ASAP, otherwise "No handlers could be found for
+    # logger ..." TODO: this should be done before plugins discovery
     setup_logging(config, _cli_log_handler, logfile='letsencrypt.log')
     cli.possible_deprecation_warning(config)
 

certbot/plugins/selection.py

@@ -260,14 +260,9 @@ def diagnose_configurator_problem(cfg_type, requested, plugins):
                    "your existing configuration.\nThe error was: {1!r}"
                    .format(requested, plugins[requested].problem))
     elif cfg_type == "installer":
-        if os.path.exists("/etc/debian_version"):
-            # Debian... installers are at least possible
-            msg = ('No installers seem to be present and working on your system; '
-                   'fix that or try running certbot with the "certonly" command')
-        else:
-            # XXX update this logic as we make progress on #788 and nginx support
-            msg = ('No installers are available on your OS yet; try running '
-                   '"letsencrypt-auto certonly" to get a cert you can install manually')
+        msg = ('No installer plugins seem to be present and working on your system; '
+               'fix that or try running certbot with the "certonly" command to obtain'
+               ' a certificate you can install manually')
     else:
         msg = "{0} could not be determined or is not installed".format(cfg_type)
     raise errors.PluginSelectionError(msg)

certbot/plugins/util.py

@@ -1,15 +1,45 @@
 """Plugin utilities."""
 import logging
+import os
 import socket
 
 import psutil
 import zope.component
 
 from certbot import interfaces
+from certbot import util
 
 
 logger = logging.getLogger(__name__)
 
+def path_surgery(restart_cmd):
+    """Attempt to perform PATH surgery to find restart_cmd
+
+    Mitigates https://github.com/certbot/certbot/issues/1833
+
+    :param str restart_cmd: the command that is being searched for in the PATH
+
+    :returns: True if the operation succeeded, False otherwise
+    """
+    dirs = ("/usr/sbin", "/usr/local/bin", "/usr/local/sbin")
+    path = os.environ["PATH"]
+    added = []
+    for d in dirs:
+        if d not in path:
+            path += os.pathsep + d
+            added.append(d)
+
+    if any(added):
+        logger.debug("Can't find %s, attempting PATH mitigation by adding %s",
+                     restart_cmd, os.pathsep.join(added))
+        os.environ["PATH"] = path
+
+    if util.exe_exists(restart_cmd):
+        return True
+    else:
+        expanded = " expanded" if any(added) else ""
+        logger.warn("Failed to find %s in%s PATH: %s", restart_cmd, expanded, path)
+        return False
 
 def already_listening(port, renewer=False):
     """Check if a process is already listening on the port.

certbot/plugins/util_test.py

@@ -1,9 +1,33 @@
 """Tests for certbot.plugins.util."""
+import os
 import unittest
 
 import mock
 import psutil
 
+class PathSurgeryTest(unittest.TestCase):
+    """Tests for certbot.plugins.path_surgery."""
+
+    @mock.patch("certbot.plugins.util.logger.warn")
+    @mock.patch("certbot.plugins.util.logger.debug")
+    def test_path_surgery(self, mock_debug, mock_warn):
+        from certbot.plugins.util import path_surgery
+        all_path = {"PATH": "/usr/local/bin:/bin/:/usr/sbin/:/usr/local/sbin/"}
+        with mock.patch.dict('os.environ', all_path):
+            with mock.patch('certbot.util.exe_exists') as mock_exists:
+                mock_exists.return_value = True
+                self.assertEquals(path_surgery("eg"), True)
+                self.assertEquals(mock_debug.call_count, 0)
+                self.assertEquals(mock_warn.call_count, 0)
+                self.assertEquals(os.environ["PATH"], all_path["PATH"])
+        no_path = {"PATH": "/tmp/"}
+        with mock.patch.dict('os.environ', no_path):
+            path_surgery("thingy")
+            self.assertEquals(mock_debug.call_count, 1)
+            self.assertEquals(mock_warn.call_count, 1)
+            self.assertTrue("Failed to find" in mock_warn.call_args[0][0])
+            self.assertTrue("/usr/local/bin" in os.environ["PATH"])
+            self.assertTrue("/tmp" in os.environ["PATH"])
 
 class AlreadyListeningTest(unittest.TestCase):
     """Tests for certbot.plugins.already_listening."""

certbot/renewal.py

@@ -108,7 +108,7 @@ def _restore_webroot_config(config, renewalparams):
         if not cli.set_by_cli("webroot_map"):
             config.namespace.webroot_map = renewalparams["webroot_map"]
     elif "webroot_path" in renewalparams:
-        logger.info("Ancient renewal conf file without webroot-map, restoring webroot-path")
+        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]
@@ -194,7 +194,7 @@ def _restore_required_config_elements(config, renewalparams):
 def should_renew(config, lineage):
     "Return true if any of the circumstances for automatic renewal apply."
     if config.renew_by_default:
-        logger.info("Auto-renewal forced with --force-renewal...")
+        logger.debug("Auto-renewal forced with --force-renewal...")
         return True
     if lineage.should_autorenew(interactive=True):
         logger.info("Cert is due for renewal, auto-renewing...")
@@ -236,7 +236,7 @@ def renew_cert(config, domains, le_client, lineage):
     _avoid_invalidating_lineage(config, lineage, original_server)
     new_certr, new_chain, new_key, _ = le_client.obtain_certificate(domains)
     if config.dry_run:
-        logger.info("Dry run: skipping updating lineage at %s",
+        logger.debug("Dry run: skipping updating lineage at %s",
                     os.path.dirname(lineage.cert))
     else:
         prior_version = lineage.latest_common_version()

certbot/reporter.py

@@ -58,7 +58,7 @@ class Reporter(object):
         """
         assert self.HIGH_PRIORITY <= priority <= self.LOW_PRIORITY
         self.messages.put(self._msg_type(priority, msg, on_crash))
-        logger.info("Reporting to user: %s", msg)
+        logger.debug("Reporting to user: %s", msg)
 
     def atexit_print_messages(self, pid=None):
         """Function to be registered with atexit to print messages.

certbot/reverter.py

@@ -24,6 +24,39 @@ logger = logging.getLogger(__name__)
 class Reverter(object):
     """Reverter Class - save and revert configuration checkpoints.
 
+    This class can be used by the plugins, especially Installers, to
+    undo changes made to the user's system. Modifications to files and
+    commands to do undo actions taken by the plugin should be registered
+    with this class before the action is taken.
+
+    Once a change has been registered with this class, there are three
+    states the change can be in. First, the change can be a temporary
+    change. This should be used for changes that will soon be reverted,
+    such as config changes for the purpose of solving a challenge.
+    Changes are added to this state through calls to
+    :func:`~add_to_temp_checkpoint` and reverted when
+    :func:`~revert_temporary_config` or :func:`~recovery_routine` is
+    called.
+
+    The second state a change can be in is in progress. These changes
+    are not temporary, however, they also have not been finalized in a
+    checkpoint. A change must become in progress before it can be
+    finalized. Changes are added to this state through calls to
+    :func:`~add_to_checkpoint` and reverted when
+    :func:`~recovery_routine` is called.
+
+    The last state a change can be in is finalized in a checkpoint. A
+    change is put into this state by first becoming an in progress
+    change and then calling :func:`~finalize_checkpoint`. Changes
+    in this state can be reverted through calls to
+    :func:`~rollback_checkpoints`.
+
+    As a final note, creating new files and registering undo commands
+    are handled specially and use the methods
+    :func:`~register_file_creation` and :func:`~register_undo_command`
+    respectively. Both of these methods can be used to create either
+    temporary or in progress changes.
+
     .. note:: Consider moving everything over to CSV format.
 
     :param config: Configuration.

certbot/tests/display/util_test.py

@@ -96,6 +96,7 @@ class NcursesDisplayTest(unittest.TestCase):
     @mock.patch("certbot.display.util."
                 "dialog.Dialog.inputbox")
     def test_input(self, mock_input):
+        mock_input.return_value = (mock.MagicMock(), mock.MagicMock())
         self.displayer.input("message")
         self.assertEqual(mock_input.call_count, 1)
 
@@ -112,6 +113,7 @@ class NcursesDisplayTest(unittest.TestCase):
     @mock.patch("certbot.display.util."
                 "dialog.Dialog.checklist")
     def test_checklist(self, mock_checklist):
+        mock_checklist.return_value = (mock.MagicMock(), mock.MagicMock())
         self.displayer.checklist("message", TAGS)
 
         choices = [
@@ -125,6 +127,7 @@ class NcursesDisplayTest(unittest.TestCase):
 
     @mock.patch("certbot.display.util.dialog.Dialog.dselect")
     def test_directory_select(self, mock_dselect):
+        mock_dselect.return_value = (mock.MagicMock(), mock.MagicMock())
         self.displayer.directory_select("message")
         self.assertEqual(mock_dselect.call_count, 1)
 

certbot/tests/main_test.py

@@ -1,12 +1,14 @@
 """Tests for certbot.main."""
+import os
+import shutil
+import tempfile
 import unittest
 
-
 import mock
 
-
 from certbot import cli
 from certbot import configuration
+from certbot import errors
 from certbot.plugins import disco as plugins_disco
 
 
@@ -44,5 +46,51 @@ class ObtainCertTest(unittest.TestCase):
         self.assertFalse(pause)
 
 
+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 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)
+
+
 if __name__ == '__main__':
     unittest.main()  # pragma: no cover

certbot/util.py

@@ -95,6 +95,7 @@ def make_or_verify_dir(directory, mode=0o755, uid=0, strict=False):
     :param str directory: Path to a directory.
     :param int mode: Directory mode.
     :param int uid: Directory owner.
+    :param bool strict: require directory to be owned by current user
 
     :raises .errors.Error: if a directory already exists,
         but has wrong permissions or owner

docs/cli-help.txt

@@ -147,7 +147,7 @@ security:
                         HTTPS for the newly authenticated vhost. (default:
                         None)
   --hsts                Add the Strict-Transport-Security header to every HTTP
-                        response. Forcing browser to use always use SSL for
+                        response. Forcing browser to always use SSL for
                         the domain. Defends against SSL Stripping. (default:
                         False)
   --no-hsts             Do not automatically add the Strict-Transport-Security

docs/using.rst

@@ -428,8 +428,8 @@ Operating System Packages
 
 **FreeBSD**
 
-  * Port: ``cd /usr/ports/security/py-letsencrypt && make install clean``
-  * Package: ``pkg install py27-letsencrypt``
+  * Port: ``cd /usr/ports/security/py-certbot && make install clean``
+  * Package: ``pkg install py27-certbot``
 
 **OpenBSD**
 

letsencrypt-auto-source/letsencrypt-auto

@@ -34,6 +34,7 @@ Help for certbot itself cannot be provided until it is installed.
   -n, --non-interactive, --noninteractive   run without asking for user input
   --no-self-upgrade                         do not download updates
   --os-packages-only                        install OS dependencies and exit
+  -q, --quiet                               provide only update/error output
   -v, --verbose                             provide more output
 
 All arguments are accepted and forwarded to the Certbot client when run."
@@ -52,15 +53,19 @@ for arg in "$@" ; do
       HELP=1;;
     --noninteractive|--non-interactive)
       ASSUME_YES=1;;
+    --quiet)
+      QUIET=1;;
     --verbose)
       VERBOSE=1;;
     -[!-]*)
-      while getopts ":hnv" short_arg $arg; do
+      while getopts ":hnvq" short_arg $arg; do
         case "$short_arg" in
           h)
             HELP=1;;
           n)
             ASSUME_YES=1;;
+          q)
+            QUIET=1;;
           v)
             VERBOSE=1;;
         esac
@@ -598,28 +603,29 @@ ConfigArgParse==0.10.0 \
     --hash=sha256:3b50a83dd58149dfcee98cb6565265d10b53e9c0a2bca7eeef7fb5f5524890a7
 configobj==5.0.6 \
     --hash=sha256:a2f5650770e1c87fb335af19a9b7eb73fc05ccf22144eb68db7d00cd2bcb0902
-cryptography==1.2.3 \
-    --hash=sha256:031938f73a5c5eb3e809e18ff7caeb6865351871417be6050cb8c86a9a202b9a \
-    --hash=sha256:a179a38d50f8d68b491d7a313db78f8cabe290842cecddddc7b34d408e59db0a \
-    --hash=sha256:906c88b2aadcf99cfabb24098263d1bf65ab0c8688acde10dae1f09d865920f1 \
-    --hash=sha256:6e706c5c6088770b1d1b634e959e21963e315b0255f5f4777125ad3d54082977 \
-    --hash=sha256:f5ebf8e31c48f8707921dca0e994de77813a9c9b9bf03c119c5ddf97bdcffe73 \
-    --hash=sha256:c7b89e42288cc7fbee3812e99ef5c744f22452e11d6822f6807afc6d6b3be83e \
-    --hash=sha256:8408d29865947109d8b68f1837a7cde1aa4dc86e0f79ca3ba58c0c44e443d6a5 \
-    --hash=sha256:c7e76cf3c3d925dd31fa238cfb806cffba718c0f08707d77a538768477969956 \
-    --hash=sha256:7d8de35380f31702758b7753bb5c40723832c73006dedb2f9099bf61a37f7287 \
-    --hash=sha256:5edbee71fae5469ee83fe0a37866b9398c8ce3a46325c24fcedfbf097bb48a19 \
-    --hash=sha256:594edafe4801c13bdc1cc305e7704a90c19617e95936f6ab457ee4ffe000ba50 \
-    --hash=sha256:b7fdb16a0a7f481be42da744bfe1ea2163025de21f90f2c688a316f3c354da9c \
-    --hash=sha256:207b8bf0fe0907336df38b733b487521cf9e138189aba9234ad54fe545dd0db8 \
-    --hash=sha256:509a2f05386270cf783993c90d49ffefb3dd62aee45bf1ea8ce3d2cde7271c21 \
-    --hash=sha256:ac69b65dd1af0179ede40c9f15788c88f73e628ea6c0519de3838e279bb388c6 \
-    --hash=sha256:8df6fad6c6ae12fd7004ea29357f0a2b4d3774eaeca7656530d08d2d90cd41aa \
-    --hash=sha256:0b8b96dd81cc1533a04f30382c0fe21c1972e189f794d0c4261a18cec08fd9b5 \
-    --hash=sha256:cae8fca1883f23c50ea78d89de6fe4fefdb4cea83177760f47177559414ded93 \
-    --hash=sha256:1a471ca576a9cdce1b1cd9f3a22b1d09ee44d46862037557de17919c0db44425 \
-    --hash=sha256:8ec4e8e3d453b3a1b63b5f57737a434dcf1ee4a2f26f6ff7c5a37c3f679104d2 \
-    --hash=sha256:8eb11c77dd8e73f48df6b2f7a7e16173fe0fe8fdfe266232832e88477e08454e
+cryptography==1.3.4 \
+    --hash=sha256:bede00edd11a2a62c8c98c271cc103fa3a3d72acf64f6e5e4eaf251128897b17 \
+    --hash=sha256:53b39e687b744bb548a98f40736cc529d9f60959b4e6cc551322cf9505d35eb3 \
+    --hash=sha256:474b73ad1139b4e423e46bbd818efd0d5c0df1c65d9f7c957d64c9215d77afde \
+    --hash=sha256:aaddf9592d5b99e32dd518bb4a25b147c124f9d6b4ad64b94f01b15d1666b8c8 \
+    --hash=sha256:6dcad2f407db8c3cd6ecd78361439c449a4f94786b46c54507e7e68f51e1709d \
+    --hash=sha256:475c153fc622e656f1f10a9c9941d0ac7ab18df7c38d35d563a437c1c0e34f24 \
+    --hash=sha256:86dd61df581cba04e89e45081efbc531faff1c9d99c77b1ce97f87216c356353 \
+    --hash=sha256:75cc697e4ef5fdd0102ca749114c6370dbd11db0c9132a18834858c2566247e3 \
+    --hash=sha256:ea03ad5b9df6d79fc9fc1ab23729e01e1c920d2974c5e3c634ccf45a5c378452 \
+    --hash=sha256:c8872b8fe4f3416d6338ab99612f49ab314f7856cb43bffab2a32d28a6267be8 \
+    --hash=sha256:468fc6e16eaec6ceaa6bc341273e6e9912d01b42b740f8cf896ace7fcd6a321d \
+    --hash=sha256:d6fea3c6502735011c5d61a62aef1c1d770fc6a2def45d9e6c0d94c9651e3317 \
+    --hash=sha256:3cf95f179f4bead3d5649b91860ef4cf60ad4244209190fc405908272576d961 \
+    --hash=sha256:141f77e60a5b9158309b2b60288c7f81d37faa15c22a69b94c190ceefaaa6236 \
+    --hash=sha256:87b7a1fe703c6424451f3372d1879dae91c7fe5e13375441a72833db76fee30e \
+    --hash=sha256:f5ee3cb0cf1a6550bf483ccffa6608db267a377b45f7e3a8201a86d1d8feb19f \
+    --hash=sha256:4e097286651ea318300af3251375d48b71b8228481c56cd617ddd4459a1ff261 \
+    --hash=sha256:1e3d3ae3f22f22d50d340f47f25227511326f3f1396c6d2446a5b45b516c4313 \
+    --hash=sha256:6a057941cb64d79834ea3cf99093fcc4787c2a5d44f686c4f297361ddc419bcd \
+    --hash=sha256:68b3d5390b92559ddd3353c73ab2dfcff758f9c4ec4f5d5226ccede0e5d779f4 \
+    --hash=sha256:545dc003b4b6081f9c3e452da15d819b04b696f49484aff64c0a2aedf766bef8 \
+    --hash=sha256:423ff890c01be7c70dbfeaa967eeef5146f1a43a5f810ffdc07b178e48a105a9
 enum34==1.1.2 \
     --hash=sha256:2475d7fcddf5951e92ff546972758802de5260bf409319a9f1934e6bbc8b1dc7 \
     --hash=sha256:35907defb0f992b75ab7788f65fedc1cf20ffa22688e0e6f6f12afc06b3ea501
@@ -679,9 +685,9 @@ pyasn1==0.1.9 \
     --hash=sha256:5191ff6b9126d2c039dd87f8ff025bed274baf07fa78afa46f556b1ad7265d6e \
     --hash=sha256:8323e03637b2d072cc7041300bac6ec448c3c28950ab40376036788e9a1af629 \
     --hash=sha256:853cacd96d1f701ddd67aa03ecc05f51890135b7262e922710112f12a2ed2a7f
-pyOpenSSL==0.15.1 \
-    --hash=sha256:88e45e6bb25dfed272a1ef2e728461d44b634c2cd689e989b6e56a349c5a3ae5 \
-    --hash=sha256:f0a26070d6db0881de8bcc7846934b7c3c930d8f9c79d45883ee48984bc0d672
+pyopenssl==16.0.0 \
+    --hash=sha256:5add70cf00273bf957ca31fdb0df9b0ae4639e081897d5f86a0ae1f104901230 \
+    --hash=sha256:363d10ee43d062285facf4e465f4f5163f9f702f9134f0a5896f134cbb92d17d
 pyRFC3339==1.0 \
     --hash=sha256:eea31835c56e2096af4363a5745a784878a61d043e247d3a6d6a0a32a9741f56 \
     --hash=sha256:8dfbc6c458b8daba1c0f3620a8c78008b323a268b27b7359e92a4ae41325f535
@@ -926,8 +932,10 @@ UNLIKELY_EOF
   fi
   if [ -n "$SUDO" ]; then
     # SUDO is su wrapper or sudo
-    echo "Requesting root privileges to run certbot..."
-    echo "  $VENV_BIN/letsencrypt" "$@"
+    if [  "$QUIET" != 1 ]; then
+      echo "Requesting root privileges to run certbot..."
+      echo "  $VENV_BIN/letsencrypt" "$@"
+    fi
   fi
   if [ -z "$SUDO_ENV" ] ; then
     # SUDO is su wrapper / noop

letsencrypt-auto-source/letsencrypt-auto.template

@@ -34,6 +34,7 @@ Help for certbot itself cannot be provided until it is installed.
   -n, --non-interactive, --noninteractive   run without asking for user input
   --no-self-upgrade                         do not download updates
   --os-packages-only                        install OS dependencies and exit
+  -q, --quiet                               provide only update/error output
   -v, --verbose                             provide more output
 
 All arguments are accepted and forwarded to the Certbot client when run."
@@ -52,15 +53,19 @@ for arg in "$@" ; do
       HELP=1;;
     --noninteractive|--non-interactive)
       ASSUME_YES=1;;
+    --quiet)
+      QUIET=1;;
     --verbose)
       VERBOSE=1;;
     -[!-]*)
-      while getopts ":hnv" short_arg $arg; do
+      while getopts ":hnvq" short_arg $arg; do
         case "$short_arg" in
           h)
             HELP=1;;
           n)
             ASSUME_YES=1;;
+          q)
+            QUIET=1;;
           v)
             VERBOSE=1;;
         esac
@@ -262,8 +267,10 @@ UNLIKELY_EOF
   fi
   if [ -n "$SUDO" ]; then
     # SUDO is su wrapper or sudo
-    echo "Requesting root privileges to run certbot..."
-    echo "  $VENV_BIN/letsencrypt" "$@"
+    if [  "$QUIET" != 1 ]; then
+      echo "Requesting root privileges to run certbot..."
+      echo "  $VENV_BIN/letsencrypt" "$@"
+    fi
   fi
   if [ -z "$SUDO_ENV" ] ; then
     # SUDO is su wrapper / noop

letsencrypt-auto-source/pieces/letsencrypt-auto-requirements.txt

@@ -32,28 +32,29 @@ ConfigArgParse==0.10.0 \
     --hash=sha256:3b50a83dd58149dfcee98cb6565265d10b53e9c0a2bca7eeef7fb5f5524890a7
 configobj==5.0.6 \
     --hash=sha256:a2f5650770e1c87fb335af19a9b7eb73fc05ccf22144eb68db7d00cd2bcb0902
-cryptography==1.2.3 \
-    --hash=sha256:031938f73a5c5eb3e809e18ff7caeb6865351871417be6050cb8c86a9a202b9a \
-    --hash=sha256:a179a38d50f8d68b491d7a313db78f8cabe290842cecddddc7b34d408e59db0a \
-    --hash=sha256:906c88b2aadcf99cfabb24098263d1bf65ab0c8688acde10dae1f09d865920f1 \
-    --hash=sha256:6e706c5c6088770b1d1b634e959e21963e315b0255f5f4777125ad3d54082977 \
-    --hash=sha256:f5ebf8e31c48f8707921dca0e994de77813a9c9b9bf03c119c5ddf97bdcffe73 \
-    --hash=sha256:c7b89e42288cc7fbee3812e99ef5c744f22452e11d6822f6807afc6d6b3be83e \
-    --hash=sha256:8408d29865947109d8b68f1837a7cde1aa4dc86e0f79ca3ba58c0c44e443d6a5 \
-    --hash=sha256:c7e76cf3c3d925dd31fa238cfb806cffba718c0f08707d77a538768477969956 \
-    --hash=sha256:7d8de35380f31702758b7753bb5c40723832c73006dedb2f9099bf61a37f7287 \
-    --hash=sha256:5edbee71fae5469ee83fe0a37866b9398c8ce3a46325c24fcedfbf097bb48a19 \
-    --hash=sha256:594edafe4801c13bdc1cc305e7704a90c19617e95936f6ab457ee4ffe000ba50 \
-    --hash=sha256:b7fdb16a0a7f481be42da744bfe1ea2163025de21f90f2c688a316f3c354da9c \
-    --hash=sha256:207b8bf0fe0907336df38b733b487521cf9e138189aba9234ad54fe545dd0db8 \
-    --hash=sha256:509a2f05386270cf783993c90d49ffefb3dd62aee45bf1ea8ce3d2cde7271c21 \
-    --hash=sha256:ac69b65dd1af0179ede40c9f15788c88f73e628ea6c0519de3838e279bb388c6 \
-    --hash=sha256:8df6fad6c6ae12fd7004ea29357f0a2b4d3774eaeca7656530d08d2d90cd41aa \
-    --hash=sha256:0b8b96dd81cc1533a04f30382c0fe21c1972e189f794d0c4261a18cec08fd9b5 \
-    --hash=sha256:cae8fca1883f23c50ea78d89de6fe4fefdb4cea83177760f47177559414ded93 \
-    --hash=sha256:1a471ca576a9cdce1b1cd9f3a22b1d09ee44d46862037557de17919c0db44425 \
-    --hash=sha256:8ec4e8e3d453b3a1b63b5f57737a434dcf1ee4a2f26f6ff7c5a37c3f679104d2 \
-    --hash=sha256:8eb11c77dd8e73f48df6b2f7a7e16173fe0fe8fdfe266232832e88477e08454e
+cryptography==1.3.4 \
+    --hash=sha256:bede00edd11a2a62c8c98c271cc103fa3a3d72acf64f6e5e4eaf251128897b17 \
+    --hash=sha256:53b39e687b744bb548a98f40736cc529d9f60959b4e6cc551322cf9505d35eb3 \
+    --hash=sha256:474b73ad1139b4e423e46bbd818efd0d5c0df1c65d9f7c957d64c9215d77afde \
+    --hash=sha256:aaddf9592d5b99e32dd518bb4a25b147c124f9d6b4ad64b94f01b15d1666b8c8 \
+    --hash=sha256:6dcad2f407db8c3cd6ecd78361439c449a4f94786b46c54507e7e68f51e1709d \
+    --hash=sha256:475c153fc622e656f1f10a9c9941d0ac7ab18df7c38d35d563a437c1c0e34f24 \
+    --hash=sha256:86dd61df581cba04e89e45081efbc531faff1c9d99c77b1ce97f87216c356353 \
+    --hash=sha256:75cc697e4ef5fdd0102ca749114c6370dbd11db0c9132a18834858c2566247e3 \
+    --hash=sha256:ea03ad5b9df6d79fc9fc1ab23729e01e1c920d2974c5e3c634ccf45a5c378452 \
+    --hash=sha256:c8872b8fe4f3416d6338ab99612f49ab314f7856cb43bffab2a32d28a6267be8 \
+    --hash=sha256:468fc6e16eaec6ceaa6bc341273e6e9912d01b42b740f8cf896ace7fcd6a321d \
+    --hash=sha256:d6fea3c6502735011c5d61a62aef1c1d770fc6a2def45d9e6c0d94c9651e3317 \
+    --hash=sha256:3cf95f179f4bead3d5649b91860ef4cf60ad4244209190fc405908272576d961 \
+    --hash=sha256:141f77e60a5b9158309b2b60288c7f81d37faa15c22a69b94c190ceefaaa6236 \
+    --hash=sha256:87b7a1fe703c6424451f3372d1879dae91c7fe5e13375441a72833db76fee30e \
+    --hash=sha256:f5ee3cb0cf1a6550bf483ccffa6608db267a377b45f7e3a8201a86d1d8feb19f \
+    --hash=sha256:4e097286651ea318300af3251375d48b71b8228481c56cd617ddd4459a1ff261 \
+    --hash=sha256:1e3d3ae3f22f22d50d340f47f25227511326f3f1396c6d2446a5b45b516c4313 \
+    --hash=sha256:6a057941cb64d79834ea3cf99093fcc4787c2a5d44f686c4f297361ddc419bcd \
+    --hash=sha256:68b3d5390b92559ddd3353c73ab2dfcff758f9c4ec4f5d5226ccede0e5d779f4 \
+    --hash=sha256:545dc003b4b6081f9c3e452da15d819b04b696f49484aff64c0a2aedf766bef8 \
+    --hash=sha256:423ff890c01be7c70dbfeaa967eeef5146f1a43a5f810ffdc07b178e48a105a9
 enum34==1.1.2 \
     --hash=sha256:2475d7fcddf5951e92ff546972758802de5260bf409319a9f1934e6bbc8b1dc7 \
     --hash=sha256:35907defb0f992b75ab7788f65fedc1cf20ffa22688e0e6f6f12afc06b3ea501
@@ -113,9 +114,9 @@ pyasn1==0.1.9 \
     --hash=sha256:5191ff6b9126d2c039dd87f8ff025bed274baf07fa78afa46f556b1ad7265d6e \
     --hash=sha256:8323e03637b2d072cc7041300bac6ec448c3c28950ab40376036788e9a1af629 \
     --hash=sha256:853cacd96d1f701ddd67aa03ecc05f51890135b7262e922710112f12a2ed2a7f
-pyOpenSSL==0.15.1 \
-    --hash=sha256:88e45e6bb25dfed272a1ef2e728461d44b634c2cd689e989b6e56a349c5a3ae5 \
-    --hash=sha256:f0a26070d6db0881de8bcc7846934b7c3c930d8f9c79d45883ee48984bc0d672
+pyopenssl==16.0.0 \
+    --hash=sha256:5add70cf00273bf957ca31fdb0df9b0ae4639e081897d5f86a0ae1f104901230 \
+    --hash=sha256:363d10ee43d062285facf4e465f4f5163f9f702f9134f0a5896f134cbb92d17d
 pyRFC3339==1.0 \
     --hash=sha256:eea31835c56e2096af4363a5745a784878a61d043e247d3a6d6a0a32a9741f56 \
     --hash=sha256:8dfbc6c458b8daba1c0f3620a8c78008b323a268b27b7359e92a4ae41325f535