From f57f35b1ddc488d2d28bbc387f1c27fe470fb2cc Mon Sep 17 00:00:00 2001 From: Noah Swartz Date: Tue, 26 Jul 2016 15:57:11 -0700 Subject: [PATCH 1/4] Start work on multivhost support in Apache * get through parsing * not slice * add mult vhost per file * idx line backwards * blocks be wrong * always close ifmod * let's not mess up indexes * don't double add multi * fix some lint, only dedupe multi * tests * fix lint * in progress bit flip * try to pick the right vhost * take Dominic's suggestion * don't redo search * add ancestor * we now support multiple vhosts * yay * add docstrings --- certbot-apache/certbot_apache/configurator.py | 176 ++++++++++------ certbot-apache/certbot_apache/obj.py | 3 +- .../certbot_apache/tests/configurator_test.py | 64 +++++- .../multi_vhosts/apache2/apache2.conf | 196 ++++++++++++++++++ .../multi_vhosts/apache2/envvars | 29 +++ .../multi_vhosts/apache2/ports.conf | 15 ++ .../apache2/sites-available/default.conf | 22 ++ .../apache2/sites-enabled/placeholder.conf | 0 certbot-apache/certbot_apache/tests/util.py | 17 ++ certbot/reverter.py | 2 +- 10 files changed, 453 insertions(+), 71 deletions(-) create mode 100644 certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/apache2.conf create mode 100644 certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/envvars create mode 100644 certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/ports.conf create mode 100644 certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-available/default.conf create mode 100644 certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-enabled/placeholder.conf diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index 39d25619d..aec1d692b 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -138,6 +138,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): self._enhance_func = {"redirect": self._enable_redirect, "ensure-http-header": self._set_http_header, "staple-ocsp": self._enable_ocsp_stapling} + self._skeletons = {} @property def mod_ssl_conf(self): @@ -589,6 +590,9 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): if realpath not in vhost_paths.keys(): vhs.append(new_vhost) vhost_paths[realpath] = new_vhost.filep + elif (realpath in vhost_paths.keys() + and new_vhost.path.endswith("]") and new_vhost not in vhs): + vhs.append(new_vhost) elif realpath == new_vhost.filep: # Prefer "real" vhost paths instead of symlinked ones # ex: sites-enabled/vh.conf -> sites-available/vh.conf @@ -792,20 +796,21 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): avail_fp = nonssl_vhost.filep ssl_fp = self._get_ssl_vhost_path(avail_fp) - self._copy_create_ssl_vhost_skeleton(avail_fp, ssl_fp) + vhost_num = -1 + if nonssl_vhost.path.endswith("]"): + # augeas doesn't zero index for whatever reason + vhost_num = int(nonssl_vhost.path[-2]) - 1 + self._copy_create_ssl_vhost_skeleton(avail_fp, ssl_fp, vhost_num) # Reload augeas to take into account the new vhost self.aug.load() # Get Vhost augeas path for new vhost vh_p = self.aug.match("/files%s//* [label()=~regexp('%s')]" % (self._escape(ssl_fp), parser.case_i("VirtualHost"))) - if len(vh_p) != 1: - logger.error("Error: should only be one vhost in %s", avail_fp) - raise errors.PluginError("Currently, we only support " - "configurations with one vhost per file") - else: - # This simplifies the process - vh_p = vh_p[0] + temp_vh = vh_p[0] + if self._skeletons[ssl_fp]: + temp_vh = vh_p[len(self._skeletons[ssl_fp]) -1] + vh_p = temp_vh # Update Addresses self._update_ssl_vhosts_addrs(vh_p) @@ -822,6 +827,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # We know the length is one because of the assertion above # Create the Vhost object ssl_vhost = self._create_vhost(vh_p) + ssl_vhost.ancestor = nonssl_vhost self.vhosts.append(ssl_vhost) # NOTE: Searches through Augeas seem to ruin changes to directives @@ -875,7 +881,38 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # Sift line if it redirects the request to a HTTPS site return target.startswith("https://") - def _copy_create_ssl_vhost_skeleton(self, avail_fp, ssl_fp): + def _section_blocks(self, blocks): + """A helper function for _create_block_segments that makes + a list of line numbers to not include in the return. + + :param list blocks: A list of indexes of where vhosts start and end. + + """ + out = [] + while len(blocks) > 1: + start = blocks[0] + end = blocks[1] + 1 + out += range(start, end) + blocks = blocks[2:] + return out + + def _create_block_segments(self, orig_file_list, vhost_num): + """A helper function for _copy_create_ssl_vhost_skeleton + that slices the appropriate vhost from the origin conf file. + + :param list orig_file_list: the original file converted to a list of strings. + "param int vhost_num: Which vhost the vhost is in the origin multivhost file. + + """ + blocks = [idx for idx, line in enumerate(orig_file_list) + if line.lstrip().startswith(" skeleton. :param str avail_fp: Pointer to the original available non-ssl vhost @@ -891,65 +928,77 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): try: with open(avail_fp, "r") as orig_file: - with open(ssl_fp, "w") as new_file: - new_file.write("\n") + orig_file_list = [line for line in orig_file] + if vhost_num != -1: + orig_file_list = self._create_block_segments(orig_file_list, vhost_num) - comment = ("# Some rewrite rules in this file were " - "disabled on your HTTPS site,\n" - "# because they have the potential to create " - "redirection loops.\n") + if ssl_fp in self._skeletons: + bit = "a" + self._skeletons[ssl_fp].append(avail_fp) + else: + bit = "w" + self._skeletons[ssl_fp] = [avail_fp] - for line in orig_file: - A = line.lstrip().startswith("RewriteCond") - B = line.lstrip().startswith("RewriteRule") + with open(ssl_fp, bit) as new_file: + new_file.write("\n") - if not (A or B): - new_file.write(line) - continue + comment = ("# Some rewrite rules in this file were " + "disabled on your HTTPS site,\n" + "# because they have the potential to create " + "redirection loops.\n") - # A RewriteRule that doesn't need filtering - if B and not self._sift_rewrite_rule(line): - new_file.write(line) - continue + orig_file_list = iter(orig_file_list) + for line in orig_file_list: + A = line.lstrip().startswith("RewriteCond") + B = line.lstrip().startswith("RewriteRule") - # A RewriteRule that does need filtering - if B and self._sift_rewrite_rule(line): + 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(comment) + sift = True + 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_list) + + # RewriteCond(s) must be followed by one RewriteRule + while not line.lstrip().startswith("RewriteRule"): + chunk.append(line) + line = next(orig_file_list) + + # 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("# " + line) + + new_file.write(''.join( + ['# ' + l for l in chunk])) + continue + else: + new_file.write(''.join(chunk)) 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("\n") + new_file.write("\n") except IOError: logger.fatal("Error writing/reading to file in make_vhost_ssl") raise errors.PluginError("Unable to write/read in make_vhost_ssl") @@ -962,6 +1011,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): "the potential to create redirection loops.".format(avail_fp, ssl_fp), reporter.MEDIUM_PRIORITY) + self.aug.set("/augeas/files%s/mtime" %(self._escape(ssl_fp)), "0") + self.aug.set("/augeas/files%s/mtime" %(self._escape(avail_fp)), "0") def _update_ssl_vhosts_addrs(self, vh_path): ssl_addrs = set() @@ -1008,12 +1059,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): self.parser.add_dir(vh_path, "Include", self.mod_ssl_conf) def _add_servername_alias(self, target_name, vhost): - fp = self._escape(vhost.filep) - vh_p = self.aug.match("/files%s//* [label()=~regexp('%s')]" % - (fp, parser.case_i("VirtualHost"))) - if not vh_p: - return - vh_path = vh_p[0] + vh_path = vhost.path if (self.parser.find_dir("ServerName", target_name, start=vh_path, exclude=False) or self.parser.find_dir("ServerAlias", target_name, @@ -1508,6 +1554,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): def _get_http_vhost(self, ssl_vhost): """Find appropriate HTTP vhost for ssl_vhost.""" # First candidate vhosts filter + if ssl_vhost.ancestor: + return ssl_vhost.ancestor candidate_http_vhs = [ vhost for vhost in self.vhosts if not vhost.ssl ] diff --git a/certbot-apache/certbot_apache/obj.py b/certbot-apache/certbot_apache/obj.py index 30cb24844..fd743fe79 100644 --- a/certbot-apache/certbot_apache/obj.py +++ b/certbot-apache/certbot_apache/obj.py @@ -123,7 +123,7 @@ class VirtualHost(object): # pylint: disable=too-few-public-methods strip_name = re.compile(r"^(?:.+://)?([^ :$]*)") def __init__(self, filep, path, addrs, ssl, enabled, name=None, - aliases=None, modmacro=False): + aliases=None, modmacro=False, ancestor=None): # pylint: disable=too-many-arguments """Initialize a VH.""" @@ -135,6 +135,7 @@ class VirtualHost(object): # pylint: disable=too-few-public-methods self.ssl = ssl self.enabled = enabled self.modmacro = modmacro + self.ancestor = ancestor def get_names(self): """Return a set of all names.""" diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index 45e701bd5..3dc3790a1 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -655,11 +655,6 @@ class MultipleVhostsTest(util.ApacheTest): len(self.config.parser.find_dir( directive, None, self.vh_truth[1].path, False)), 0) - def test_make_vhost_ssl_extra_vhs(self): - self.config.aug.match = mock.Mock(return_value=["p1", "p2"]) - self.assertRaises( - errors.PluginError, self.config.make_vhost_ssl, self.vh_truth[0]) - def test_make_vhost_ssl_bad_write(self): mock_open = mock.mock_open() # This calls open @@ -1345,6 +1340,65 @@ class AugeasVhostsTest(util.ApacheTest): self.config.choose_vhost(name) self.assertEqual(mock_select.call_count, 0) +class MultiVhostsTest(util.ApacheTest): + """Test vhosts with illegal names dependant on augeas version.""" + # pylint: disable=protected-access + + def setUp(self): # pylint: disable=arguments-differ + td = "debian_apache_2_4/multi_vhosts" + cr = "debian_apache_2_4/multi_vhosts/apache2" + vr = "debian_apache_2_4/multi_vhosts/apache2/sites-available" + super(MultiVhostsTest, self).setUp(test_dir=td, + config_root=cr, + vhost_root=vr) + + self.config = util.get_apache_configurator( + self.config_path, self.vhost_path, self.config_dir, self.work_dir) + self.vh_truth = util.get_vh_truth( + self.temp_dir, "debian_apache_2_4/multi_vhosts") + + def tearDown(self): + shutil.rmtree(self.temp_dir) + shutil.rmtree(self.config_dir) + shutil.rmtree(self.work_dir) + + def test_make_vhost_ssl(self): + ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[1]) + + self.assertEqual( + ssl_vhost.filep, + os.path.join(self.config_path, "sites-available", + "default-le-ssl.conf")) + + self.assertEqual(ssl_vhost.path, + "/files" + ssl_vhost.filep + "/IfModule/VirtualHost") + self.assertEqual(len(ssl_vhost.addrs), 1) + self.assertEqual(set([obj.Addr.fromstring("*:443")]), ssl_vhost.addrs) + self.assertEqual(ssl_vhost.name, "banana.vomit.com") + self.assertTrue(ssl_vhost.ssl) + self.assertFalse(ssl_vhost.enabled) + + self.assertTrue(self.config.parser.find_dir( + "SSLCertificateFile", None, ssl_vhost.path, False)) + self.assertTrue(self.config.parser.find_dir( + "SSLCertificateKeyFile", None, ssl_vhost.path, False)) + + self.assertEqual(self.config.is_name_vhost(self.vh_truth[1]), + self.config.is_name_vhost(ssl_vhost)) + + def test_make_2nd_vhost_ssl(self): + _ = self.config.make_vhost_ssl(self.vh_truth[0]) + _ = self.config.make_vhost_ssl(self.vh_truth[1]) + self.assertEqual( + len(self.config._skeletons[self.config._get_ssl_vhost_path(self.vh_truth[0].filep)]), 2) + + def test_cover_is_stupid_and_I_hate_it(self): + http_vhost = obj.VirtualHost(None, None, None, False, False, name="Noah") + ssl_vhost = obj.VirtualHost(None, None, None, False, False, name="Noah") + self.config.vhosts.append(http_vhost) + self.assertEqual(self.config._get_http_vhost(ssl_vhost), http_vhost) + + if __name__ == "__main__": unittest.main() # pragma: no cover diff --git a/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/apache2.conf b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/apache2.conf new file mode 100644 index 000000000..2a5bb7be2 --- /dev/null +++ b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/apache2.conf @@ -0,0 +1,196 @@ +# This is the main Apache server configuration file. It contains the +# configuration directives that give the server its instructions. +# See http://httpd.apache.org/docs/2.4/ for detailed information about +# the directives and /usr/share/doc/apache2/README.Debian about Debian specific +# hints. +# +# +# Summary of how the Apache 2 configuration works in Debian: +# The Apache 2 web server configuration in Debian is quite different to +# upstream's suggested way to configure the web server. This is because Debian's +# default Apache2 installation attempts to make adding and removing modules, +# virtual hosts, and extra configuration directives as flexible as possible, in +# order to make automating the changes and administering the server as easy as +# possible. + +# It is split into several files forming the configuration hierarchy outlined +# below, all located in the /etc/apache2/ directory: +# +# /etc/apache2/ +# |-- apache2.conf +# | `-- ports.conf +# |-- mods-enabled +# | |-- *.load +# | `-- *.conf +# |-- conf-enabled +# | `-- *.conf +# `-- sites-enabled +# `-- *.conf +# +# +# * apache2.conf is the main configuration file (this file). It puts the pieces +# together by including all remaining configuration files when starting up the +# web server. +# +# * ports.conf is always included from the main configuration file. It is +# supposed to determine listening ports for incoming connections which can be +# customized anytime. +# +# * Configuration files in the mods-enabled/, conf-enabled/ and sites-enabled/ +# directories contain particular configuration snippets which manage modules, +# global configuration fragments, or virtual host configurations, +# respectively. +# +# They are activated by symlinking available configuration files from their +# respective *-available/ counterparts. These should be managed by using our +# helpers a2enmod/a2dismod, a2ensite/a2dissite and a2enconf/a2disconf. See +# their respective man pages for detailed information. +# +# * The binary is called apache2. Due to the use of environment variables, in +# the default configuration, apache2 needs to be started/stopped with +# /etc/init.d/apache2 or apache2ctl. Calling /usr/bin/apache2 directly will not +# work with the default configuration. + + +# Global configuration + +# +# The accept serialization lock file MUST BE STORED ON A LOCAL DISK. +# +Mutex file:${APACHE_LOCK_DIR} default + +# +# PidFile: The file in which the server should record its process +# identification number when it starts. +# This needs to be set in /etc/apache2/envvars +# +PidFile ${APACHE_PID_FILE} + +# +# Timeout: The number of seconds before receives and sends time out. +# +Timeout 300 + +# +# KeepAlive: Whether or not to allow persistent connections (more than +# one request per connection). Set to "Off" to deactivate. +# +KeepAlive On + +# +# MaxKeepAliveRequests: The maximum number of requests to allow +# during a persistent connection. Set to 0 to allow an unlimited amount. +# We recommend you leave this number high, for maximum performance. +# +MaxKeepAliveRequests 100 + +# +# KeepAliveTimeout: Number of seconds to wait for the next request from the +# same client on the same connection. +# +KeepAliveTimeout 5 + + +# These need to be set in /etc/apache2/envvars +User ${APACHE_RUN_USER} +Group ${APACHE_RUN_GROUP} + +# +# HostnameLookups: Log the names of clients or just their IP addresses +# e.g., www.apache.org (on) or 204.62.129.132 (off). +# The default is off because it'd be overall better for the net if people +# had to knowingly turn this feature on, since enabling it means that +# each client request will result in AT LEAST one lookup request to the +# nameserver. +# +HostnameLookups Off + +# ErrorLog: The location of the error log file. +# If you do not specify an ErrorLog directive within a +# container, error messages relating to that virtual host will be +# logged here. If you *do* define an error logfile for a +# container, that host's errors will be logged there and not here. +# +ErrorLog ${APACHE_LOG_DIR}/error.log + +# +# LogLevel: Control the severity of messages logged to the error_log. +# Available values: trace8, ..., trace1, debug, info, notice, warn, +# error, crit, alert, emerg. +# It is also possible to configure the log level for particular modules, e.g. +# "LogLevel info ssl:warn" +# +LogLevel warn + +# Include module configuration: +IncludeOptional mods-enabled/*.load +IncludeOptional mods-enabled/*.conf + +# Include list of ports to listen on +Include ports.conf + + +# Sets the default security model of the Apache2 HTTPD server. It does +# not allow access to the root filesystem outside of /usr/share and /var/www. +# The former is used by web applications packaged in Debian, +# the latter may be used for local directories served by the web server. If +# your system is serving content from a sub-directory in /srv you must allow +# access here, or in any related virtual host. + + Options FollowSymLinks + AllowOverride None + Require all denied + + + + AllowOverride None + Require all granted + + + + Options Indexes FollowSymLinks + AllowOverride None + Require all granted + + +# AccessFileName: The name of the file to look for in each directory +# for additional configuration directives. See also the AllowOverride +# directive. +# +AccessFileName .htaccess + +# +# The following lines prevent .htaccess and .htpasswd files from being +# viewed by Web clients. +# + + Require all denied + + +# The following directives define some format nicknames for use with +# a CustomLog directive. +# +# These deviate from the Common Log Format definitions in that they use %O +# (the actual bytes sent including headers) instead of %b (the size of the +# requested file), because the latter makes it impossible to detect partial +# requests. +# +# Note that the use of %{X-Forwarded-For}i instead of %h is not recommended. +# Use mod_remoteip instead. +# +LogFormat "%v:%p %h %l %u %t \"%r\" %>s %O \"%{Referer}i\" \"%{User-Agent}i\"" vhost_combined +LogFormat "%h %l %u %t \"%r\" %>s %O \"%{Referer}i\" \"%{User-Agent}i\"" combined +LogFormat "%h %l %u %t \"%r\" %>s %O" common +LogFormat "%{Referer}i -> %U" referer +LogFormat "%{User-agent}i" agent + +# Include of directories ignores editors' and dpkg's backup files, +# see README.Debian for details. + +# Include generic snippets of statements +IncludeOptional conf-enabled/*.conf + +# Include the virtual host configurations: +IncludeOptional sites-enabled/*.conf + +# vim: syntax=apache ts=4 sw=4 sts=4 sr noet diff --git a/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/envvars b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/envvars new file mode 100644 index 000000000..a13d9a89e --- /dev/null +++ b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/envvars @@ -0,0 +1,29 @@ +# envvars - default environment variables for apache2ctl + +# this won't be correct after changing uid +unset HOME + +# for supporting multiple apache2 instances +if [ "${APACHE_CONFDIR##/etc/apache2-}" != "${APACHE_CONFDIR}" ] ; then + SUFFIX="-${APACHE_CONFDIR##/etc/apache2-}" +else + SUFFIX= +fi + +# Since there is no sane way to get the parsed apache2 config in scripts, some +# settings are defined via environment variables and then used in apache2ctl, +# /etc/init.d/apache2, /etc/logrotate.d/apache2, etc. +export APACHE_RUN_USER=www-data +export APACHE_RUN_GROUP=www-data +# temporary state file location. This might be changed to /run in Wheezy+1 +export APACHE_PID_FILE=/var/run/apache2/apache2$SUFFIX.pid +export APACHE_RUN_DIR=/var/run/apache2$SUFFIX +export APACHE_LOCK_DIR=/var/lock/apache2$SUFFIX +# Only /var/log/apache2 is handled by /etc/logrotate.d/apache2. +export APACHE_LOG_DIR=/var/log/apache2$SUFFIX + +## The locale used by some modules like mod_dav +export LANG=C + +export LANG + diff --git a/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/ports.conf b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/ports.conf new file mode 100644 index 000000000..5daec58c1 --- /dev/null +++ b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/ports.conf @@ -0,0 +1,15 @@ +# If you just change the port or add more ports here, you will likely also +# have to change the VirtualHost statement in +# /etc/apache2/sites-enabled/000-default.conf + +Listen 80 + + + Listen 443 + + + + Listen 443 + + +# vim: syntax=apache ts=4 sw=4 sts=4 sr noet diff --git a/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-available/default.conf b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-available/default.conf new file mode 100644 index 000000000..6ab206b2d --- /dev/null +++ b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-available/default.conf @@ -0,0 +1,22 @@ + + + ServerName banana.vomit.net + ServerAdmin webmaster@localhost + DocumentRoot /var/www/html + + ErrorLog ${APACHE_LOG_DIR}/error.log + CustomLog ${APACHE_LOG_DIR}/access.log combined + + + + + + ServerName banana.vomit.com + ServerAdmin webmaster@localhost + DocumentRoot /var/www/html + + ErrorLog ${APACHE_LOG_DIR}/error.log + CustomLog ${APACHE_LOG_DIR}/access.log combined + + +# vim: syntax=apache ts=4 sw=4 sts=4 sr noet diff --git a/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-enabled/placeholder.conf b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-enabled/placeholder.conf new file mode 100644 index 000000000..e69de29bb diff --git a/certbot-apache/certbot_apache/tests/util.py b/certbot-apache/certbot_apache/tests/util.py index 3c33a0e19..90880ac5d 100644 --- a/certbot-apache/certbot_apache/tests/util.py +++ b/certbot-apache/certbot_apache/tests/util.py @@ -164,5 +164,22 @@ def get_vh_truth(temp_dir, config_name): set([obj.Addr.fromstring("10.2.3.4:443")]), True, True, "ocspvhost.com")] return vh_truth + if config_name == "debian_apache_2_4/multi_vhosts": + prefix = os.path.join( + temp_dir, config_name, "apache2/sites-available") + aug_pre = "/files" + prefix + vh_truth = [ + obj.VirtualHost( + os.path.join(prefix, "default.conf"), + os.path.join(aug_pre, "default.conf/VirtualHost[1]"), + set([obj.Addr.fromstring("*:80")]), + False, True, "ip-172-30-0-17"), + obj.VirtualHost( + os.path.join(prefix, "default.conf"), + os.path.join(aug_pre, "default.conf/VirtualHost[2]"), + set([obj.Addr.fromstring("*:80")]), + False, True, "banana.vomit.com")] + return vh_truth + return None # pragma: no cover diff --git a/certbot/reverter.py b/certbot/reverter.py index 32355782e..34feafc7e 100644 --- a/certbot/reverter.py +++ b/certbot/reverter.py @@ -491,7 +491,7 @@ class Reverter(object): else: logger.warning( "File: %s - Could not be found to be deleted %s - " - "LE probably shut down unexpectedly", + "Certbot probably shut down unexpectedly", os.linesep, path) except (IOError, OSError): logger.fatal( From 2613a8b5795e9171be662e63ad641e66283d484b Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 28 Sep 2016 14:49:21 -0700 Subject: [PATCH 2/4] Continue work on Apache multivhost * Apache: do not assume directives will be CamelCased * Fixup * Elaborate * Simplify the definition of vh_p --- certbot-apache/certbot_apache/configurator.py | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index aec1d692b..e555e7610 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -805,12 +805,10 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # Reload augeas to take into account the new vhost self.aug.load() # Get Vhost augeas path for new vhost - vh_p = self.aug.match("/files%s//* [label()=~regexp('%s')]" % - (self._escape(ssl_fp), parser.case_i("VirtualHost"))) - temp_vh = vh_p[0] - if self._skeletons[ssl_fp]: - temp_vh = vh_p[len(self._skeletons[ssl_fp]) -1] - vh_p = temp_vh + matches = self.aug.match("/files%s//* [label()=~regexp('%s')]" % + (self._escape(ssl_fp), parser.case_i("VirtualHost"))) + skels = self._skeletons[ssl_fp] + vh_p = matches[len(skels) - 1] if skels else matches[0] # Update Addresses self._update_ssl_vhosts_addrs(vh_p) @@ -865,7 +863,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): :rtype: bool """ - if not line.lstrip().startswith("RewriteRule"): + if not line.lower().lstrip().startswith("rewriterule"): return False # According to: http://httpd.apache.org/docs/2.4/rewrite/flags.html @@ -887,6 +885,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): :param list blocks: A list of indexes of where vhosts start and end. + For instance, turns [2,5,13,15] into [[2,3,4,5], [13,14,15]]. + """ out = [] while len(blocks) > 1: @@ -949,8 +949,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): orig_file_list = iter(orig_file_list) for line in orig_file_list: - A = line.lstrip().startswith("RewriteCond") - B = line.lstrip().startswith("RewriteRule") + A = line.lower().lstrip().startswith("rewritecond") + B = line.lower().lstrip().startswith("rewriterule") if not (A or B): new_file.write(line) @@ -979,7 +979,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): line = next(orig_file_list) # RewriteCond(s) must be followed by one RewriteRule - while not line.lstrip().startswith("RewriteRule"): + while not line.lower().lstrip().startswith("rewriterule"): chunk.append(line) line = next(orig_file_list) From 6b26015752c4715d00241d078932e100ae12e189 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 29 Sep 2016 19:32:22 -0700 Subject: [PATCH 3/4] Further Apache multivhost improvements * Don't filter vhosts on path if you've done so already * add get_internal_aug_path * Use relative augeas paths to determine if a file contains multiple virtual hosts --- certbot-apache/certbot_apache/configurator.py | 86 +++++++++++++------ .../certbot_apache/tests/configurator_test.py | 12 ++- 2 files changed, 71 insertions(+), 27 deletions(-) diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index e555e7610..3aaafca4b 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -574,8 +574,9 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): """ # Search base config, and all included paths for VirtualHosts + file_paths = {} + internal_paths = defaultdict(set) vhs = [] - vhost_paths = {} for vhost_path in self.parser.parser_paths.keys(): paths = self.aug.match( ("/files%s//*[label()=~regexp('%s')]" % @@ -586,21 +587,33 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): new_vhost = self._create_vhost(path) if not new_vhost: continue + internal_path = get_internal_aug_path(new_vhost.path) realpath = os.path.realpath(new_vhost.filep) - if realpath not in vhost_paths.keys(): + if realpath not in file_paths: + file_paths[realpath] = new_vhost.filep + internal_paths[realpath].add(internal_path) vhs.append(new_vhost) - vhost_paths[realpath] = new_vhost.filep - elif (realpath in vhost_paths.keys() - and new_vhost.path.endswith("]") and new_vhost not in vhs): - vhs.append(new_vhost) - elif realpath == new_vhost.filep: + elif (realpath == new_vhost.filep and + realpath != file_paths[realpath]): # Prefer "real" vhost paths instead of symlinked ones # ex: sites-enabled/vh.conf -> sites-available/vh.conf # remove old (most likely) symlinked one - vhs = [v for v in vhs if v.filep != vhost_paths[realpath]] + new_vhs = [] + for v in vhs: + if v.filep == file_paths[realpath]: + internal_paths[realpath].remove( + get_internal_aug_path(v.path)) + else: + new_vhs.append(v) + vhs = new_vhs + + file_paths[realpath] = realpath + internal_paths[realpath].add(internal_path) + vhs.append(new_vhost) + elif internal_path not in internal_paths[realpath]: + internal_paths[realpath].add(internal_path) vhs.append(new_vhost) - vhost_paths[realpath] = realpath return vhs @@ -1867,25 +1880,46 @@ def get_file_path(vhost_path): :rtype: str """ - # Strip off /files/ - try: - if vhost_path.startswith("/files/"): - avail_fp = vhost_path[7:].split("/") - else: - return None - except AttributeError: - # If we received a None path + if vhost_path is None or not vhost_path.startswith("/files/"): return None - last_good = "" - # Loop through the path parts and validate after every addition - for p in avail_fp: - cur_path = last_good+"/"+p - if os.path.exists(cur_path): - last_good = cur_path - else: - break - return last_good + return _split_aug_path(vhost_path)[0] + + +def get_internal_aug_path(vhost_path): + """Get the Augeas path for a vhost with the file path removed. + + :param str vhost_path: Augeas virtual host path + + :returns: Augeas path to vhost relative to the containing file + :rtype: str + + """ + return _split_aug_path(vhost_path)[1] + + +def _split_aug_path(vhost_path): + """Splits an Augeas path into a file path and an internal path. + + After removing "/files", this function splits vhost_path into the + file path and the remaining Augeas path. + + :param str vhost_path: Augeas virtual host path + + :returns: file path and internal Augeas path + :rtype: `tuple` of `str` + + """ + # Strip off /files + file_path = vhost_path[6:] + internal_path = [] + + # Remove components from the end of file_path until it becomes valid + while not os.path.exists(file_path): + file_path, _, internal_path_part = file_path.rpartition("/") + internal_path.append(internal_path_part) + + return file_path, "/".join(reversed(internal_path)) def install_ssl_options_conf(options_ssl): diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index 3dc3790a1..9ac03f1f4 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -138,6 +138,17 @@ class MultipleVhostsTest(util.ApacheTest): self.assertEqual(get_file_path("nonexistent"), None) self.assertEqual(self.config._create_vhost("nonexistent"), None) # pylint: disable=protected-access + def test_get_aug_internal_path(self): + from certbot_apache.configurator import get_internal_aug_path + internal_paths = [ + "VirtualHost", "IfModule/VirtualHost", "VirtualHost", "VirtualHost", + "Macro/VirtualHost", "IfModule/VirtualHost", "VirtualHost", + "IfModule/VirtualHost"] + + for i, internal_path in enumerate(internal_paths): + self.assertEqual( + get_internal_aug_path(self.vh_truth[i].path), internal_path) + def test_bad_servername_alias(self): ssl_vh1 = obj.VirtualHost( "fp1", "ap1", set([obj.Addr(("*", "443"))]), @@ -1399,6 +1410,5 @@ class MultiVhostsTest(util.ApacheTest): self.assertEqual(self.config._get_http_vhost(ssl_vhost), http_vhost) - if __name__ == "__main__": unittest.main() # pragma: no cover From 65c7a5a6f79cb1703faa3548367816a1f1c0e099 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Wed, 28 Sep 2016 21:34:27 +0300 Subject: [PATCH 4/4] Add support for multivhosts in Apache * Case sensitivity fixes * Clean up merge leftovers * Get correct vhost paths when appending to already existing multivhost -le-ssl.conf * Test, lint and reverter fixes * Make py26 happy * Removed skeletons * Changed new vhost matching * Added span flag for augeas init * Extract VirtualHost using aug_span * Removed dead code * Fix tests to mitigate not being able to reload Augeas span values after write * Small fixes and test coverage * Implementing changes requested in review --- .../certbot_apache/augeas_configurator.py | 4 +- certbot-apache/certbot_apache/configurator.py | 311 ++++++++++-------- certbot-apache/certbot_apache/obj.py | 1 + .../certbot_apache/tests/configurator_test.py | 186 +++++------ .../apache2/sites-available/multi-vhost.conf | 38 +++ .../apache2/sites-enabled/default.conf | 1 + .../apache2/sites-enabled/multi-vhost.conf | 1 + .../apache2/sites-enabled/placeholder.conf | 0 certbot-apache/certbot_apache/tests/util.py | 19 +- 9 files changed, 318 insertions(+), 243 deletions(-) create mode 100644 certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-available/multi-vhost.conf create mode 120000 certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-enabled/default.conf create mode 120000 certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-enabled/multi-vhost.conf delete mode 100644 certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-enabled/placeholder.conf diff --git a/certbot-apache/certbot_apache/augeas_configurator.py b/certbot-apache/certbot_apache/augeas_configurator.py index 3735284ef..e053d0468 100644 --- a/certbot-apache/certbot_apache/augeas_configurator.py +++ b/certbot-apache/certbot_apache/augeas_configurator.py @@ -47,7 +47,9 @@ class AugeasConfigurator(common.Plugin): loadpath=constants.AUGEAS_LENS_DIR, # Do not save backup (we do it ourselves), do not load # anything by default - flags=(augeas.Augeas.NONE | augeas.Augeas.NO_MODL_AUTOLOAD)) + flags=(augeas.Augeas.NONE | + augeas.Augeas.NO_MODL_AUTOLOAD | + augeas.Augeas.ENABLE_SPAN)) self.recovery_routine() def check_parsing_errors(self, lens): diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index 3aaafca4b..787564141 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -138,7 +138,6 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): self._enhance_func = {"redirect": self._enable_redirect, "ensure-http-header": self._set_http_header, "staple-ocsp": self._enable_ocsp_stapling} - self._skeletons = {} @property def mod_ssl_conf(self): @@ -495,6 +494,32 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): return "" + def _get_vhost_names(self, path): + """Helper method for getting the ServerName and + ServerAlias values from vhost in path + + :param path: Path to read ServerName and ServerAliases from + + :returns: Tuple including ServerName and `list` of ServerAlias strings + """ + + servername_match = self.parser.find_dir( + "ServerName", None, start=path, exclude=False) + serveralias_match = self.parser.find_dir( + "ServerAlias", None, start=path, exclude=False) + + serveraliases = [] + for alias in serveralias_match: + serveralias = self.parser.get_arg(alias) + serveraliases.append(serveralias) + + servername = None + if servername_match: + # Get last ServerName as each overwrites the previous + servername = self.parser.get_arg(servername_match[-1]) + + return (servername, serveraliases) + def _add_servernames(self, host): """Helper function for get_virtual_hosts(). @@ -502,22 +527,15 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): :type host: :class:`~certbot_apache.obj.VirtualHost` """ - # Take the final ServerName as each overrides the previous - servername_match = self.parser.find_dir( - "ServerName", None, start=host.path, exclude=False) - serveralias_match = self.parser.find_dir( - "ServerAlias", None, start=host.path, exclude=False) - for alias in serveralias_match: - serveralias = self.parser.get_arg(alias) - if not host.modmacro: - host.aliases.add(serveralias) + servername, serveraliases = self._get_vhost_names(host.path) - if servername_match: - # Get last ServerName as each overwrites the previous - servername = self.parser.get_arg(servername_match[-1]) + for alias in serveraliases: if not host.modmacro: - host.name = servername + host.aliases.add(alias) + + if not host.modmacro: + host.name = servername def _create_vhost(self, path): """Used by get_virtual_hosts to create vhost objects @@ -582,7 +600,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): ("/files%s//*[label()=~regexp('%s')]" % (vhost_path, parser.case_i("VirtualHost")))) paths = [path for path in paths if - os.path.basename(path.lower()) == "virtualhost"] + "virtualhost" in os.path.basename(path).lower()] for path in paths: new_vhost = self._create_vhost(path) if not new_vhost: @@ -809,23 +827,27 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): avail_fp = nonssl_vhost.filep ssl_fp = self._get_ssl_vhost_path(avail_fp) - vhost_num = -1 - if nonssl_vhost.path.endswith("]"): - # augeas doesn't zero index for whatever reason - vhost_num = int(nonssl_vhost.path[-2]) - 1 - self._copy_create_ssl_vhost_skeleton(avail_fp, ssl_fp, vhost_num) + orig_matches = self.aug.match("/files%s//* [label()=~regexp('%s')]" % + (self._escape(ssl_fp), + parser.case_i("VirtualHost"))) + + self._copy_create_ssl_vhost_skeleton(nonssl_vhost, ssl_fp) # Reload augeas to take into account the new vhost self.aug.load() # Get Vhost augeas path for new vhost - matches = self.aug.match("/files%s//* [label()=~regexp('%s')]" % - (self._escape(ssl_fp), parser.case_i("VirtualHost"))) - skels = self._skeletons[ssl_fp] - vh_p = matches[len(skels) - 1] if skels else matches[0] + new_matches = self.aug.match("/files%s//* [label()=~regexp('%s')]" % + (self._escape(ssl_fp), + parser.case_i("VirtualHost"))) + + vh_p = self._get_new_vh_path(orig_matches, new_matches) + + if not vh_p: + raise errors.PluginError( + "Could not reverse map the HTTPS VirtualHost to the original") # Update Addresses self._update_ssl_vhosts_addrs(vh_p) - # Add directives self._add_dummy_ssl_directives(vh_p) self.save() @@ -852,6 +874,20 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): return ssl_vhost + def _get_new_vh_path(self, orig_matches, new_matches): + """ Helper method for make_vhost_ssl for matching augeas paths. Returns + VirtualHost path from new_matches that's not present in orig_matches. + + Paths are normalized, because augeas leaves indices out for paths + with only single directive with a similar key """ + + orig_matches = [i.replace("[1]", "") for i in orig_matches] + for match in new_matches: + if match.replace("[1]", "") not in orig_matches: + # Return the unmodified path + return match + return None + def _get_ssl_vhost_path(self, non_ssl_vh_fp): # Get filepath of new ssl_vhost if non_ssl_vh_fp.endswith(".conf"): @@ -892,43 +928,10 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # Sift line if it redirects the request to a HTTPS site return target.startswith("https://") - def _section_blocks(self, blocks): - """A helper function for _create_block_segments that makes - a list of line numbers to not include in the return. - - :param list blocks: A list of indexes of where vhosts start and end. - - For instance, turns [2,5,13,15] into [[2,3,4,5], [13,14,15]]. - - """ - out = [] - while len(blocks) > 1: - start = blocks[0] - end = blocks[1] + 1 - out += range(start, end) - blocks = blocks[2:] - return out - - def _create_block_segments(self, orig_file_list, vhost_num): - """A helper function for _copy_create_ssl_vhost_skeleton - that slices the appropriate vhost from the origin conf file. - - :param list orig_file_list: the original file converted to a list of strings. - "param int vhost_num: Which vhost the vhost is in the origin multivhost file. - - """ - blocks = [idx for idx, line in enumerate(orig_file_list) - if line.lstrip().startswith(" skeleton. - :param str avail_fp: Pointer to the original available non-ssl vhost + :param obj.VirtualHost vhost: Original VirtualHost object :param str ssl_fp: Full path where the new ssl_vhost will reside. A new file is created on the filesystem. @@ -936,81 +939,24 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): """ # First register the creation so that it is properly removed if # configuration is rolled back - self.reverter.register_file_creation(False, ssl_fp) + if os.path.exists(ssl_fp): + notes = "Appended new VirtualHost directive to file %s" % ssl_fp + files = set() + files.add(ssl_fp) + self.reverter.add_to_checkpoint(files, notes) + else: + self.reverter.register_file_creation(False, ssl_fp) sift = False try: - with open(avail_fp, "r") as orig_file: - orig_file_list = [line for line in orig_file] - if vhost_num != -1: - orig_file_list = self._create_block_segments(orig_file_list, vhost_num) + orig_contents = self._get_vhost_block(vhost) + ssl_vh_contents, sift = self._sift_rewrite_rules(orig_contents) - if ssl_fp in self._skeletons: - bit = "a" - self._skeletons[ssl_fp].append(avail_fp) - else: - bit = "w" - self._skeletons[ssl_fp] = [avail_fp] - - with open(ssl_fp, bit) as new_file: + with open(ssl_fp, "a") as new_file: new_file.write("\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") - - orig_file_list = iter(orig_file_list) - for line in orig_file_list: - A = line.lower().lstrip().startswith("rewritecond") - B = line.lower().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(comment) - sift = True - 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_list) - - # RewriteCond(s) must be followed by one RewriteRule - while not line.lower().lstrip().startswith("rewriterule"): - chunk.append(line) - line = next(orig_file_list) - - # 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("\n".join(ssl_vh_contents)) + # The content does not include the closing tag, so add it + new_file.write("\n") new_file.write("\n") except IOError: logger.fatal("Error writing/reading to file in make_vhost_ssl") @@ -1021,11 +967,96 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): reporter.add_message( "Some rewrite rules copied from {0} were disabled in the " "vhost for your HTTPS site located at {1} because they have " - "the potential to create redirection loops.".format(avail_fp, - ssl_fp), - reporter.MEDIUM_PRIORITY) - self.aug.set("/augeas/files%s/mtime" %(self._escape(ssl_fp)), "0") - self.aug.set("/augeas/files%s/mtime" %(self._escape(avail_fp)), "0") + "the potential to create redirection loops.".format( + vhost.filep, ssl_fp), reporter.MEDIUM_PRIORITY) + self.aug.set("/augeas/files%s/mtime" % (self._escape(ssl_fp)), "0") + self.aug.set("/augeas/files%s/mtime" % (self._escape(vhost.filep)), "0") + + def _sift_rewrite_rules(self, contents): + """ Helper function for _copy_create_ssl_vhost_skeleton to prepare the + new HTTPS VirtualHost contents. Currently disabling the rewrites """ + + result = [] + sift = False + contents = iter(contents) + + 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 contents: + A = line.lower().lstrip().startswith("rewritecond") + B = line.lower().lstrip().startswith("rewriterule") + + if not (A or B): + result.append(line) + continue + + # A RewriteRule that doesn't need filtering + if B and not self._sift_rewrite_rule(line): + result.append(line) + continue + + # A RewriteRule that does need filtering + if B and self._sift_rewrite_rule(line): + if not sift: + result.append(comment) + sift = True + result.append("# " + 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(contents) + + # RewriteCond(s) must be followed by one RewriteRule + while not line.lower().lstrip().startswith("rewriterule"): + chunk.append(line) + line = next(contents) + + # Now, current line must start with a RewriteRule + chunk.append(line) + + if self._sift_rewrite_rule(line): + if not sift: + result.append(comment) + sift = True + + result.append('\n'.join( + ['# ' + l for l in chunk])) + continue + else: + result.append('\n'.join(chunk)) + continue + return result, sift + + def _get_vhost_block(self, vhost): + """ Helper method to get VirtualHost contents from the original file. + This is done with help of augeas span, which returns the span start and + end positions + + :returns: `list` of VirtualHost block content lines without closing tag + """ + + try: + span_val = self.aug.span(vhost.path) + except ValueError: + logger.fatal("Error while reading the VirtualHost %s from " + "file %s", vhost.name, vhost.filep, exc_info=True) + raise errors.PluginError("Unable to read VirtualHost from file") + span_filep = span_val[0] + span_start = span_val[5] + span_end = span_val[6] + with open(span_filep, 'r') as fh: + fh.seek(span_start) + vh_contents = fh.read(span_end-span_start) + return vh_contents.split("\n") def _update_ssl_vhosts_addrs(self, vh_path): ssl_addrs = set() @@ -1073,10 +1104,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): def _add_servername_alias(self, target_name, vhost): vh_path = vhost.path - if (self.parser.find_dir("ServerName", target_name, - start=vh_path, exclude=False) or - self.parser.find_dir("ServerAlias", target_name, - start=vh_path, exclude=False)): + sname, saliases = self._get_vhost_names(vh_path) + if target_name == sname or target_name in saliases: return if self._has_matching_wildcard(vh_path, target_name): return @@ -1476,7 +1505,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): for re_path in rewrite_engine_path_list: # A RewriteEngine directive may also be included in per # directory .htaccess files. We only care about the VirtualHost. - if 'VirtualHost' in re_path: + if 'virtualhost' in re_path.lower(): return self.parser.get_arg(re_path) return False @@ -1880,7 +1909,7 @@ def get_file_path(vhost_path): :rtype: str """ - if vhost_path is None or not vhost_path.startswith("/files/"): + if not vhost_path or not vhost_path.startswith("/files/"): return None return _split_aug_path(vhost_path)[0] diff --git a/certbot-apache/certbot_apache/obj.py b/certbot-apache/certbot_apache/obj.py index fd743fe79..1e3579858 100644 --- a/certbot-apache/certbot_apache/obj.py +++ b/certbot-apache/certbot_apache/obj.py @@ -112,6 +112,7 @@ class VirtualHost(object): # pylint: disable=too-few-public-methods :ivar bool ssl: SSLEngine on in vhost :ivar bool enabled: Virtual host is enabled :ivar bool modmacro: VirtualHost is using mod_macro + :ivar VirtualHost ancestor: A non-SSL VirtualHost this is based on https://httpd.apache.org/docs/2.4/vhosts/details.html diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index 9ac03f1f4..c930b97fd 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -580,6 +580,7 @@ class MultipleVhostsTest(util.ApacheTest): # already listens to the correct port self.assertEqual(mock_add_dir.call_count, 0) + def test_make_vhost_ssl(self): ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[0]) @@ -800,6 +801,15 @@ class MultipleVhostsTest(util.ApacheTest): def test_supported_enhancements(self): self.assertTrue(isinstance(self.config.supported_enhancements(), list)) + def test_find_http_vhost_without_ancestor(self): + # pylint: disable=protected-access + vhost = self.vh_truth[0] + vhost.ssl = True + vhost.ancestor = None + res = self.config._get_http_vhost(vhost) + self.assertEqual(self.vh_truth[0].name, res.name) + self.assertEqual(self.vh_truth[0].aliases, res.aliases) + @mock.patch("certbot_apache.configurator.ApacheConfigurator._get_http_vhost") @mock.patch("certbot_apache.display_ops.select_vhost") @mock.patch("certbot.util.exe_exists") @@ -999,8 +1009,9 @@ class MultipleVhostsTest(util.ApacheTest): # three args to rw_rule self.assertEqual(len(rw_rule), 3) - self.assertTrue(rw_engine[0].startswith(self.vh_truth[3].path)) - self.assertTrue(rw_rule[0].startswith(self.vh_truth[3].path)) + # [:-3] to remove the vhost index number + self.assertTrue(rw_engine[0].startswith(self.vh_truth[3].path[:-3])) + self.assertTrue(rw_rule[0].startswith(self.vh_truth[3].path[:-3])) self.assertTrue("rewrite_module" in self.config.parser.modules) @@ -1048,9 +1059,9 @@ class MultipleVhostsTest(util.ApacheTest): self.assertEqual(len(rw_engine), 1) # three args to rw_rule + 1 arg for the pre existing rewrite self.assertEqual(len(rw_rule), 5) - - self.assertTrue(rw_engine[0].startswith(self.vh_truth[3].path)) - self.assertTrue(rw_rule[0].startswith(self.vh_truth[3].path)) + # [:-3] to remove the vhost index number + self.assertTrue(rw_engine[0].startswith(self.vh_truth[3].path[:-3])) + self.assertTrue(rw_rule[0].startswith(self.vh_truth[3].path[:-3])) self.assertTrue("rewrite_module" in self.config.parser.modules) @@ -1158,89 +1169,6 @@ class MultipleVhostsTest(util.ApacheTest): not_rewriterule = "NotRewriteRule ^ ..." self.assertFalse(self.config._sift_rewrite_rule(not_rewriterule)) - @certbot_util.patch_get_utility() - def test_make_vhost_ssl_with_existing_rewrite_rule(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") - - self.config.parser.add_dir( - http_vhost.path, "RewriteRule", - ["^", - "https://%{SERVER_NAME}%{REQUEST_URI}", - "[L,NE,R=permanent]"]) - self.config.save() - - ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[0]) - - self.assertTrue(self.config.parser.find_dir( - "RewriteEngine", "on", ssl_vhost.path, False)) - - conf_text = open(ssl_vhost.filep).read() - commented_rewrite_rule = ("# RewriteRule ^ " - "https://%{SERVER_NAME}%{REQUEST_URI} " - "[L,NE,R=permanent]") - self.assertTrue(commented_rewrite_rule in conf_text) - mock_get_utility().add_message.assert_called_once_with(mock.ANY, - - mock.ANY) - @certbot_util.patch_get_utility() - 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,NE,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,NE,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,NE,L]") - - commented_cond1 = "# RewriteCond %{HTTPS} !=on" - commented_cond2 = "# RewriteCond %{HTTPS} !^$" - commented_rewrite_rule = ("# RewriteRule ^ " - "https://%{SERVER_NAME}%{REQUEST_URI} " - "[L,NE,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 @@ -1351,6 +1279,12 @@ class AugeasVhostsTest(util.ApacheTest): self.config.choose_vhost(name) self.assertEqual(mock_select.call_count, 0) + def test_augeas_span_error(self): + broken_vhost = self.config.vhosts[0] + broken_vhost.path = broken_vhost.path + "/nonexistent" + self.assertRaises(errors.PluginError, self.config.make_vhost_ssl, + broken_vhost) + class MultiVhostsTest(util.ApacheTest): """Test vhosts with illegal names dependant on augeas version.""" # pylint: disable=protected-access @@ -1397,17 +1331,73 @@ class MultiVhostsTest(util.ApacheTest): self.assertEqual(self.config.is_name_vhost(self.vh_truth[1]), self.config.is_name_vhost(ssl_vhost)) - def test_make_2nd_vhost_ssl(self): - _ = self.config.make_vhost_ssl(self.vh_truth[0]) - _ = self.config.make_vhost_ssl(self.vh_truth[1]) - self.assertEqual( - len(self.config._skeletons[self.config._get_ssl_vhost_path(self.vh_truth[0].filep)]), 2) + mock_path = "certbot_apache.configurator.ApacheConfigurator._get_new_vh_path" + with mock.patch(mock_path) as mock_getpath: + mock_getpath.return_value = None + self.assertRaises(errors.PluginError, self.config.make_vhost_ssl, + self.vh_truth[1]) - def test_cover_is_stupid_and_I_hate_it(self): - http_vhost = obj.VirtualHost(None, None, None, False, False, name="Noah") - ssl_vhost = obj.VirtualHost(None, None, None, False, False, name="Noah") - self.config.vhosts.append(http_vhost) - self.assertEqual(self.config._get_http_vhost(ssl_vhost), http_vhost) + def test_get_new_path(self): + with_index_1 = ["/path[1]/section[1]"] + without_index = ["/path/section"] + with_index_2 = ["/path[2]/section[2]"] + self.assertEqual(self.config._get_new_vh_path(without_index, + with_index_1), + None) + self.assertEqual(self.config._get_new_vh_path(without_index, + with_index_2), + with_index_2[0]) + + both = with_index_1 + with_index_2 + self.assertEqual(self.config._get_new_vh_path(without_index, both), + with_index_2[0]) + + @certbot_util.patch_get_utility() + def test_make_vhost_ssl_with_existing_rewrite_rule(self, mock_get_utility): + self.config.parser.modules.add("rewrite_module") + + ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[4]) + + self.assertTrue(self.config.parser.find_dir( + "RewriteEngine", "on", ssl_vhost.path, False)) + + conf_text = open(ssl_vhost.filep).read() + commented_rewrite_rule = ("# RewriteRule \"^/secrets/(.+)\" " + "\"https://new.example.com/docs/$1\" [R,L]") + uncommented_rewrite_rule = ("RewriteRule \"^/docs/(.+)\" " + "\"http://new.example.com/docs/$1\" [R,L]") + self.assertTrue(commented_rewrite_rule in conf_text) + self.assertTrue(uncommented_rewrite_rule in conf_text) + mock_get_utility().add_message.assert_called_once_with(mock.ANY, + mock.ANY) + + @certbot_util.patch_get_utility() + def test_make_vhost_ssl_with_existing_rewrite_conds(self, mock_get_utility): + self.config.parser.modules.add("rewrite_module") + + ssl_vhost = self.config.make_vhost_ssl(self.vh_truth[3]) + + conf_lines = open(ssl_vhost.filep).readlines() + conf_line_set = [l.strip() for l in conf_lines] + not_commented_cond1 = ("RewriteCond " + "%{DOCUMENT_ROOT}/%{REQUEST_FILENAME} !-f") + not_commented_rewrite_rule = ("RewriteRule " + "^(.*)$ b://u%{REQUEST_URI} [P,NE,L]") + + commented_cond1 = "# RewriteCond %{HTTPS} !=on" + commented_cond2 = "# RewriteCond %{HTTPS} !^$" + commented_rewrite_rule = ("# RewriteRule ^ " + "https://%{SERVER_NAME}%{REQUEST_URI} " + "[L,NE,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) if __name__ == "__main__": diff --git a/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-available/multi-vhost.conf b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-available/multi-vhost.conf new file mode 100644 index 000000000..5f2b727bf --- /dev/null +++ b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-available/multi-vhost.conf @@ -0,0 +1,38 @@ + + ServerName 1.multi.vhost.tld + ServerAlias first.multi.vhost.tld + ServerAdmin webmaster@localhost + DocumentRoot /var/www/html + ErrorLog ${APACHE_LOG_DIR}/error.log + CustomLog ${APACHE_LOG_DIR}/access.log combined + + + + + ServerName 2.multi.vhost.tld + ServerAlias second.multi.vhost.tld + ServerAdmin webmaster@localhost + DocumentRoot /var/www/html + ErrorLog ${APACHE_LOG_DIR}/error.log + CustomLog ${APACHE_LOG_DIR}/access.log combined +RewriteEngine on +RewriteCond %{DOCUMENT_ROOT}/%{REQUEST_FILENAME} !-f +RewriteRule ^(.*)$ b://u%{REQUEST_URI} [P,NE,L] +RewriteCond %{HTTPS} !=on +RewriteCond %{HTTPS} !^$ +RewriteRule ^ https://%{SERVER_NAME}%{REQUEST_URI} [L,NE,R=permanent] + + + + + ServerName 3.multi.vhost.tld + ServerAlias third.multi.vhost.tld + ServerAdmin webmaster@localhost + DocumentRoot /var/www/html + ErrorLog ${APACHE_LOG_DIR}/error.log + CustomLog ${APACHE_LOG_DIR}/access.log combined +RewriteEngine on +RewriteRule "^/secrets/(.+)" "https://new.example.com/docs/$1" [R,L] +RewriteRule "^/docs/(.+)" "http://new.example.com/docs/$1" [R,L] + + diff --git a/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-enabled/default.conf b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-enabled/default.conf new file mode 120000 index 000000000..032e6bcf0 --- /dev/null +++ b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-enabled/default.conf @@ -0,0 +1 @@ +../sites-available/default.conf \ No newline at end of file diff --git a/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-enabled/multi-vhost.conf b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-enabled/multi-vhost.conf new file mode 120000 index 000000000..7f0910ff4 --- /dev/null +++ b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-enabled/multi-vhost.conf @@ -0,0 +1 @@ +../sites-available/multi-vhost.conf \ No newline at end of file diff --git a/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-enabled/placeholder.conf b/certbot-apache/certbot_apache/tests/testdata/debian_apache_2_4/multi_vhosts/apache2/sites-enabled/placeholder.conf deleted file mode 100644 index e69de29bb..000000000 diff --git a/certbot-apache/certbot_apache/tests/util.py b/certbot-apache/certbot_apache/tests/util.py index 90880ac5d..d40152ad5 100644 --- a/certbot-apache/certbot_apache/tests/util.py +++ b/certbot-apache/certbot_apache/tests/util.py @@ -178,8 +178,21 @@ def get_vh_truth(temp_dir, config_name): os.path.join(prefix, "default.conf"), os.path.join(aug_pre, "default.conf/VirtualHost[2]"), set([obj.Addr.fromstring("*:80")]), - False, True, "banana.vomit.com")] + False, True, "banana.vomit.com"), + obj.VirtualHost( + os.path.join(prefix, "multi-vhost.conf"), + os.path.join(aug_pre, "multi-vhost.conf/VirtualHost[1]"), + set([obj.Addr.fromstring("*:80")]), + False, True, "1.multi.vhost.tld"), + obj.VirtualHost( + os.path.join(prefix, "multi-vhost.conf"), + os.path.join(aug_pre, "multi-vhost.conf/IfModule/VirtualHost"), + set([obj.Addr.fromstring("*:80")]), + False, True, "2.multi.vhost.tld"), + obj.VirtualHost( + os.path.join(prefix, "multi-vhost.conf"), + os.path.join(aug_pre, "multi-vhost.conf/VirtualHost[2]"), + set([obj.Addr.fromstring("*:80")]), + False, True, "3.multi.vhost.tld")] return vh_truth - - return None # pragma: no cover