Merge remote-tracking branch 'origin/master' into nginx-map-parsing

Author: Seth Schoen

certbot-apache/certbot_apache/configurator.py

@@ -18,6 +18,7 @@
 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 @@ def mod_ssl_conf(self):
         return os.path.join(self.config.config_dir,
                             constants.MOD_SSL_CONF_DEST)
 
+
     def prepare(self):
         """Prepare the authenticator/installer.
 
@@ -157,8 +159,11 @@ def prepare(self):
             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()
@@ -819,7 +824,7 @@ def _get_ssl_vhost_path(self, non_ssl_vh_fp):
         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:
@@ -870,18 +875,62 @@ def _copy_create_ssl_vhost_skeleton(self, avail_fp, ssl_fp):
             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/tests/configurator_test.py

@@ -1,4 +1,4 @@
-# pylint: disable=too-many-public-methods
+# pylint: disable=too-many-public-methods,too-many-lines
 """Test for certbot_apache.configurator."""
 import os
 import shutil
@@ -49,11 +49,14 @@ def tearDown(self):
         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 @@ def test_prepare_old_aug(self, mock_exe_exists, _):
         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..
@@ -1110,16 +1114,19 @@ def test_create_own_redirect_for_old_apache_version(self):
         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):
@@ -1147,9 +1154,63 @@ def test_make_vhost_ssl_with_existing_rewrite_rule(self, mock_get_utility):
                                   "https://%{SERVER_NAME}%{REQUEST_URI} "
                                   "[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."""
         account_key = self.rsa512jwk

certbot-compatibility-test/nginx/README

@@ -0,0 +1,27 @@
+Eventually there will also be a compatibility test here like the Apache one.
+
+Right now, this is data for the roundtrip test (checking that the parser
+can parse each file and that the reserialized config file it generates is
+identical to the original).
+
+If run in a virtualenv or otherwise so that certbot_nginx can be imported,
+the roundtrip test can run as
+
+python roundtrip.py nginx-roundtrip-testdata
+
+It gives exit status 0 for success and 1 if at least one parse or roundtrip
+failure occurred.
+
+
+The directory nginx-roundtrip-testdata includes some config files that were
+contributed to our project as well as most of the configs linked from
+
+https://www.nginx.com/resources/wiki/start/
+
+Some exceptions that were skipped are
+
+https://www.nginx.com/resources/wiki/start/topics/recipes/moinmoin/
+https://www.nginx.com/resources/wiki/start/topics/examples/SSL-Offloader/ (not much nginx configuration)
+https://www.nginx.com/resources/wiki/start/topics/examples/xsendfile/ (likewise)
+https://www.nginx.com/resources/wiki/start/topics/examples/x-accel/
+https://www.nginx.com/resources/wiki/start/topics/examples/fcgiwrap/

certbot-compatibility-test/nginx/nginx-roundtrip-testdata/79-configs/site-10033

@@ -0,0 +1,34 @@
+upstream django_server_random18709.example.org {
+    server unix:/srv/http/random22194/live/website.sock;
+}
+
+server {
+    listen 80;
+    server_name random18709.example.org;
+
+    location /media/ {
+        alias /srv/http/random22194/live/dynamic/public/;
+        expires 7d;
+        include upload_folder_security_params;
+    }
+    location /static/ {
+        alias /srv/http/random22194/live/static_collected/;
+        expires 7d;
+    }
+
+    location / {
+        proxy_pass http://django_server_random18709.example.org;
+        proxy_set_header Host $host;
+        proxy_set_header X-Real-IP $remote_addr;
+        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
+    }
+
+    access_log /var/log/nginx/random22194/live/access.log combined_plus;
+    error_log  /var/log/nginx/random22194/live/error.log;
+}
+
+server {
+    server_name www.random18709.example.org;
+    server_name random24607.example.org www.random24607.example.org;
+    return 301 http://random18709.example.org$request_uri;
+}

certbot-compatibility-test/nginx/nginx-roundtrip-testdata/79-configs/site-10571

@@ -0,0 +1,71 @@
+upstream django_server_random1413.example.org {
+    server unix:/srv/http/random25151/live/website.sock;
+}
+
+server {
+    listen 443;
+    server_name www.random25266.example.org;
+
+    ssl on;
+    ssl_certificate     /etc/ssl/public/random25266.example.org.bundle.crt;
+    ssl_certificate_key /etc/ssl/private/random25266.example.org.key;
+
+    location /media/ {
+        alias /srv/http/random25151/live/dynamic/public/;
+        expires 7d;
+        include upload_folder_security_params;
+    }
+    location /static/ {
+        alias /srv/http/random25151/live/static_collected/;
+        expires 7d;
+    }
+
+    location / {
+        proxy_pass http://django_server_random1413.example.org;
+        proxy_set_header Host $host;
+        proxy_set_header X-Real-IP $remote_addr;
+        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
+    }
+
+    access_log /var/log/nginx/random25151/live/access.log combined_plus;
+    error_log  /var/log/nginx/random25151/live/error.log;
+}
+
+
+server {
+    listen 443;
+    server_name random1413.example.org www.random1413.example.org;
+
+    ssl on;
+    ssl_certificate     /etc/ssl/public/random1413.example.org.bundle.crt;
+    ssl_certificate_key /etc/ssl/private/random1413.example.org.key;
+
+    location / {
+        return 301 https://www.random25266.example.org$request_uri;
+    }
+}
+
+server {
+    listen 443;
+    server_name random25266.example.org;
+
+    ssl on;
+    ssl_certificate     /etc/ssl/public/random25266.example.org.bundle.crt;
+    ssl_certificate_key /etc/ssl/private/random25266.example.org.key;
+
+    location / {
+        return 301 https://www.random25266.example.org$request_uri;
+    }
+}
+
+server {
+    listen 80;
+    server_name random1413.example.org www.random1413.example.org;
+    server_name random28524.example.org www.random28524.example.org;
+    server_name random25266.example.org www.random25266.example.org;
+    server_name random26791.example.org www.random26791.example.org;
+
+    location / {
+        return 301 https://www.random25266.example.org$request_uri;
+    }
+}