Do not parse disabled configuration files from under sites-available on Debian / Ubuntu #4104
Conversation
|
I ran Apache test farm tests on this (with the change from #4126 merged in) and everything passed. If no one beats me to it, I'm going to try and review this first thing tomorrow morning. |
| @@ -278,6 +277,12 @@ def deploy_cert(self, domain, cert_path, key_path, | |||
| self.aug.set(path["cert_path"][-1], fullchain_path) | |||
| self.aug.set(path["cert_key"][-1], key_path) | |||
|
|
|||
| # Enable the new vhost if needed | |||
| if self.conf("handle-sites"): | |||
| if not self.is_site_enabled(vhost.filep): | |||
There was a problem hiding this comment.
Question: Why doesn't vhost.enabled work anymore? Calling is_site_enabled doesn't seem like it'll cause incorrect behavior, but I'm not sure it's necessary.
| # Must also attempt to parse virtual host root, however | ||
| # omit sites-available for debian / ubuntu | ||
| if (not self.vhostroot.endswith("sites-available") and | ||
| not constants.os_constant("handle_sites")): |
There was a problem hiding this comment.
Should we be calling parse_file() here at all anymore? If I understand correctly, Augeas already automatically parses any files included in an Apache configuration. With this in mind, why do we need to parse anything here manually?
If we need this call for some reason, is the logic here correct? Assuming constants.py doesn't change, we now don't parse the vhost directory if it ends in *sites-available or we're on Debian?
|
Another thing I forgot to take a look at during my review is whether calling If it's OK with you, I'd like to leave this out of 0.11.0 and merge it for 0.12.0. I'm not sure there's any problems here, but I have some questions and want to make sure this behaves as we expect. EDIT: I wanted to add that not getting this out on time is my own fault for not getting a chance to review this sooner. |
|
is_site_enabled method has been removed, as well as the parse_file call for constants.vhost_root. Would love a new test farm run. |
There was a problem hiding this comment.
All test farm tests pass 😄
In addition to my inline review comments, I'm curious if you took a look at the potential problems with the new calls to parse_file() that I mentioned here. I do not know there is a problem here, but this issue has bit us before and I think it has the potential to do so again if we're not careful.
| @@ -822,6 +810,8 @@ def make_vhost_ssl(self, nonssl_vhost): # pylint: disable=too-many-locals | |||
| # We know the length is one because of the assertion above | |||
| # Create the Vhost object | |||
| ssl_vhost = self._create_vhost(vh_p) | |||
| if self.conf("handle-sites"): | |||
| ssl_vhost.enabled = False | |||
There was a problem hiding this comment.
Is this the right logic? I think it works in the conventional case, but what if someone has a file in sites-enabled rather than a symlink?
There was a problem hiding this comment.
This is always newly generated vhost, and this is the only situation a vhost can actually be in disabled state in the current vhost object lifecycle (we "handle-sites" only for debian based distros).
However maybe we should instead do the handling logic in vhost object init and populate this value instead. Currently, vhosts are always created with enabled = True.
There was a problem hiding this comment.
Yeah I agree this is the only case the vhost can be in an enabled state, but I think it's possible it's already enabled. While it's maybe a bit of a silly example, I was able to get this PR to fail because it tries to enable an already enabled vhost. Starting with the default Apache2 config on Ubuntu 16.04, the changes I made were:
sudo a2dissite 000-default.conf
sudo cp /etc/apache2/sites-available/000-default.conf /etc/apache2/sites-enabled
sudo rm /etc/apache2/sites-available/*
sudo a2enmod ssl
In the end, your only vhost is a real file in sites-enabled which when we run Certbot it creates a vhost in sites-enabled which is marked as not enabled.
| return ssl_vhost | ||
|
|
||
| def _get_ssl_vhost_path(self, non_ssl_vh_fp): | ||
| # Get filepath of new ssl_vhost | ||
| # Make sure we use the realpath (eg. sites-available instead of | ||
| # sites-enabled |
| @@ -60,10 +60,6 @@ def __init__(self, aug, root, vhostroot, version=(2, 4)): | |||
| # Set up rest of locations | |||
| self.loc.update(self._set_locations()) | |||
|
|
|||
| # Must also attempt to parse virtual host root | |||
| self._parse_file(self.vhostroot + "/" + | |||
| constants.os_constant("vhost_files")) | |||
There was a problem hiding this comment.
When we talked about this on the call I think we expressed some concerns about whether or not we can safely do this. In the conventional case, this works fine but what about people who have been manually setting --apache-vhost-root? These people may not exist, but this might break things for them correct?
There was a problem hiding this comment.
You are right, I brought it back with a condition that we're running a distro with no "handle-sites" (anything but Debian / Ubuntu basically), or have defined a path other than the one in constants. Effectively letting users define the value, but ignoring sites-available.
| @@ -46,9 +46,7 @@ def __init__(self, aug, root, vhostroot, version=(2, 4)): | |||
| # Find configuration root and make sure augeas can parse it. | |||
| self.root = os.path.abspath(root) | |||
| self.loc = {"root": self._find_config_root()} | |||
| self._parse_file(self.loc["root"]) | |||
|
|
|||
| self.vhostroot = os.path.abspath(vhostroot) | |||
There was a problem hiding this comment.
We don't need to set self.vhostroot, but I think we still want to use the absolute path of vhostroot. If I take the default configuration on Ubuntu 16.04, disable all vhosts, and set --apache-vhost-root to a relative path to sites-available, Certbot fails unless we use the absolute path in the call to parse_file.
There was a problem hiding this comment.
You are right, absolute path will be used.
| file_paths[realpath] = realpath | ||
| internal_paths[realpath].add(internal_path) | ||
| vhs.append(new_vhost) | ||
| elif internal_path not in internal_paths[realpath]: |
There was a problem hiding this comment.
I think we still need this code here to support multiple vhosts per file. As an example, take the default Ubuntu 16.04 config and modify /etc/apache2/sites-available/000-default.conf to be:
<VirtualHost *:80>
# The ServerName directive sets the request scheme, hostname and port that
# the server uses to identify itself. This is used when creating
# redirection URLs. In the context of virtual hosts, the ServerName
# specifies what hostname must appear in the request's Host: header to
# match this virtual host. For the default virtual host (this file) this
# value is not decisive as it is used as a last resort host regardless.
# However, you must set it for any further virtual host explicitly.
ServerName example.org
ServerAdmin webmaster@localhost
DocumentRoot /var/www/html/org
# Available loglevels: trace8, ..., trace1, debug, info, notice, warn,
# error, crit, alert, emerg.
# It is also possible to configure the loglevel for particular
# modules, e.g.
#LogLevel info ssl:warn
ErrorLog ${APACHE_LOG_DIR}/error.log
CustomLog ${APACHE_LOG_DIR}/access.log combined
# For most configuration files from conf-available/, which are
# enabled or disabled at a global level, it is possible to
# include a line for only one particular virtual host. For example the
# following line enables the CGI configuration for this host only
# after it has been globally disabled with "a2disconf".
#Include conf-available/serve-cgi-bin.conf
</VirtualHost>
<VirtualHost *:80>
# The ServerName directive sets the request scheme, hostname and port that
# the server uses to identify itself. This is used when creating
# redirection URLs. In the context of virtual hosts, the ServerName
# specifies what hostname must appear in the request's Host: header to
# match this virtual host. For the default virtual host (this file) this
# value is not decisive as it is used as a last resort host regardless.
# However, you must set it for any further virtual host explicitly.
ServerName example.net
ServerAdmin webmaster@localhost
DocumentRoot /var/www/html/net
# Available loglevels: trace8, ..., trace1, debug, info, notice, warn,
# error, crit, alert, emerg.
# It is also possible to configure the loglevel for particular
# modules, e.g.
#LogLevel info ssl:warn
ErrorLog ${APACHE_LOG_DIR}/error.log
CustomLog ${APACHE_LOG_DIR}/access.log combined
# For most configuration files from conf-available/, which are
# enabled or disabled at a global level, it is possible to
# include a line for only one particular virtual host. For example the
# following line enables the CGI configuration for this host only
# after it has been globally disabled with "a2disconf".
#Include conf-available/serve-cgi-bin.conf
</VirtualHost>
# vim: syntax=apache ts=4 sw=4 sts=4 sr noet
While we don't error, we don't modify the configuration correctly and the return value here only contains one vhost. This works properly in master.
While not the fault of this PR, it's slightly terrifying that all our tests pass...
| @@ -625,28 +644,6 @@ def get_virtual_hosts(self): | |||
| file_paths[realpath] = new_vhost.filep | |||
| internal_paths[realpath].add(internal_path) | |||
| vhs.append(new_vhost) | |||
| elif (realpath == new_vhost.filep and | |||
| realpath != file_paths[realpath]): | |||
There was a problem hiding this comment.
If a user sets --apache-vhost-root, we may still want code like this. For example, if the user sets --apache-vhost-root to /etc/apache2/sites-available on Debian, whether the filep for a vhost points to sites-available or sites-enabled is undefined. This is because self.parser.parser_paths is a dict whose ordering is undefined when we turn it into a list and for each realpath, we take the first vhost we find.
Do we have a preference which vhost we use? We probably should just for consistency's sake. If we don't add this code back, we probably want something similar below for internal vhost paths to prevent a symlinked vhost from being added twice in this scenario.
There was a problem hiding this comment.
You are right, the removal has been undone.
| :param str non_ssl_vh_fp: File path of non-SSL vhost | ||
|
|
||
| :returns: File path for SSL vhost | ||
| :rtype: string |
There was a problem hiding this comment.
nit: This should be :rtype: str, otherwise, the built documentation points to https://docs.python.org/2/library/string.html
| # Defined by user on CLI | ||
|
|
||
| fp = (os.path.realpath(self.conf("vhost-root")) + | ||
| "/" + os.path.basename(non_ssl_vh_fp)) |
There was a problem hiding this comment.
nit: It's probably better to use os.path.join here than hard coding the forward slash.
| # Add the new file to augeas DOM | ||
| # self.parser.parse_file(inc_path) | ||
| # Reload augeas | ||
| # self.aug.load() |
There was a problem hiding this comment.
Is this code supposed to be uncommented? If not, perhaps these comments should be deleted.
There was a problem hiding this comment.
Yeah, removed the lines altogether
| raise errors.PluginError( | ||
| "Could not reverse map the HTTPS VirtualHost to the original") | ||
| # The vhost was not found on the currently parsed paths | ||
| if not self.conf("handle-sites"): |
There was a problem hiding this comment.
I think we may want to add the include whether or not handle-sites is True. If we don't, we're going to raise an exception down below.
There was a problem hiding this comment.
If handle-sites is true, we're handling the include using distribution specific manner (eg, enabling the site) in deploy_cert method instead.
There was a problem hiding this comment.
I was wrong about raising an exception in my original message, but a couple things after thinking about this and playing with it more:
- If
handle-sitesisFalse, why are we automatically enabling the site? - When playing with this code, I was able to reliably reproduce the bug I was worried about here. I did this by using the Fedora 26 Docker image and installing Certbot with our developer instructions and checking out this branch. After that, I made a simple vhost at an arbitrary location and created a symlink to it from
/etc/httpd/conf.d. When this is done and you run Certbot, theifbranch here is taken and anIncludewas meant to be added tohttpd.conf. If I callself.save()before callingparse_file, thisIncludeis written to the file, but if I callself.save()afterwards, it is not. This is a problem here and potentially anywhere else new we are callingmatchorparse_file.
cc @SwartzCr, you may be interested in point 2.
| # Add new file to augeas paths if we're supposed to handle | ||
| # activation (it's not included as default) | ||
| if self.conf("handle-sites"): | ||
| self.parser.parse_file(ssl_fp) |
There was a problem hiding this comment.
Why do we need this now when we didn't before? Why isn't the file included as default? At least in normal cases, we'll find it when we reload augeas in make_vhost_ssl. In the cases we won't, if you make my suggested change to always add an Include directive as necessary, we'll also handle this there.
There was a problem hiding this comment.
Before we parsed sites-available contents (added manually to augeas parse tree using value in constants.py). It's not included until the vhost is enabled by creating a symlink from sites-enabled. We need this temporarily to find the newly created vhost in the certificate configuration step.
There was a problem hiding this comment.
Calls to parse_file causing changes to be lost should certainly be addressed, however, if it is, perhaps we should just call this unconditionally here, especially if the logic in make_vhost_ssl I commented on is changed. Rather than parsing the file in two locations depending on whether or not sites-enabled is set, does it hurt anything to always ensure Augeas has parsed the file?
| # Add new file to augeas paths if we're supposed to handle | ||
| # activation (it's not included as default) | ||
| if self.conf("handle-sites"): | ||
| self.parser.parse_file(redirect_filepath) |
There was a problem hiding this comment.
Similar to my other comment, why do we need this change?
There was a problem hiding this comment.
We handle the actual include later in the process.
There was a problem hiding this comment.
Similar to my other comment, if problems with parse_file (and specifically match) can be addressed, perhaps we should just do this unconditionally?
| else: | ||
| logger.warning( | ||
| "Could not symlink %s to %s, got error: %s", enabled_path, | ||
| vhost.filep, err.strerror) |
There was a problem hiding this comment.
Turns out this is pretty complicated. I left a few comments inline.
I'm happy to keep reviewing this, however, if you think it'd be easier and not mess with your workflow too much, we could potentially land a PR that accomplishes this after your distro refactor. It may make things simpler and I don't think there's a need to rush this change out.
Totally up to you though. I'm happy either way.
| raise errors.PluginError( | ||
| "Could not reverse map the HTTPS VirtualHost to the original") | ||
| # The vhost was not found on the currently parsed paths | ||
| if not self.conf("handle-sites"): |
There was a problem hiding this comment.
I was wrong about raising an exception in my original message, but a couple things after thinking about this and playing with it more:
- If
handle-sitesisFalse, why are we automatically enabling the site? - When playing with this code, I was able to reliably reproduce the bug I was worried about here. I did this by using the Fedora 26 Docker image and installing Certbot with our developer instructions and checking out this branch. After that, I made a simple vhost at an arbitrary location and created a symlink to it from
/etc/httpd/conf.d. When this is done and you run Certbot, theifbranch here is taken and anIncludewas meant to be added tohttpd.conf. If I callself.save()before callingparse_file, thisIncludeis written to the file, but if I callself.save()afterwards, it is not. This is a problem here and potentially anywhere else new we are callingmatchorparse_file.
cc @SwartzCr, you may be interested in point 2.
|
|
||
| if self.conf("handle-sites") and "/sites-enabled/" not in ssl_vhost.filep: | ||
| # Set vhost as disabled for enabling later on | ||
| ssl_vhost.enabled = False |
There was a problem hiding this comment.
I think we may want to address it now. On Debian based systems, we're incorrectly marking vhosts as enabled which can cause problems later in execution.
For example, while it's not trivial to repro, starting with the default Apache config on Ubuntu 16.04, do the following:
sudo a2dissite 000-default.conf
sudo mkdir /etc/apache2/sites-available2
sudo cp /etc/apache2/sites-available/000-default.conf /etc/apache2/sites-available2/
# Add a ServerName directive to /etc/apache2/sites-available2/000-default.conf that you can obtain a cert for
sudo certbot -d foo.science --apache --no-redirect --apache-vhost-root /etc/apache2/sites-available2/
# Add a ServerAlias directive to /etc/apache2/sites-available2/000-default.conf
sudo certbot -d foo.science --apache --redirect --apache-vhost-root /etc/apache2/sites-available2/
On this branch, this last command fails because it things the disabled vhost in /etc/apache2/sites-available2 would conflict with a redirect vhost. If you checkout master, this problem goes away.
| # Add new file to augeas paths if we're supposed to handle | ||
| # activation (it's not included as default) | ||
| if self.conf("handle-sites"): | ||
| self.parser.parse_file(ssl_fp) |
There was a problem hiding this comment.
Calls to parse_file causing changes to be lost should certainly be addressed, however, if it is, perhaps we should just call this unconditionally here, especially if the logic in make_vhost_ssl I commented on is changed. Rather than parsing the file in two locations depending on whether or not sites-enabled is set, does it hurt anything to always ensure Augeas has parsed the file?
| # Add new file to augeas paths if we're supposed to handle | ||
| # activation (it's not included as default) | ||
| if self.conf("handle-sites"): | ||
| self.parser.parse_file(redirect_filepath) |
There was a problem hiding this comment.
Similar to my other comment, if problems with parse_file (and specifically match) can be addressed, perhaps we should just do this unconditionally?
joohoi
force-pushed
the
no-sites-available
branch
2 times, most recently
from
ce945a0
to
3a4683a
Compare
joohoi
force-pushed
the
no-sites-available
branch
from
3a4683a
to
0b3d1e3
Compare
|
This PR contains a lot of things I'm building on, and it's a nice overall cleanup of the configurator, hence: Ready for yet another round of review :) All your concerns were valid, and required actions so I cleaned up the AugeasConfigurator.save() method and implemented a check for unsaved changes and autosave functionality when doing destructive operations in Augeas DOM tree. I'm not very happy about the way that I had to implement it (passing the ApacheConfigurator instance reference to parser), but I toyed around with options at hand and this seemed like the only way to do this without duplicating too much code. This is result of how the augeas_configurator, configurator and parser relate to each other, it works and seems pretty robust whatsoever. I also implemented an actual check for vhost enabled state in the way we discussed on the last call, so it should reflect the actual state now. All the requested changes should have been made solving the issues in a generic way. |
|
I'm planning to hold off on reviewing this until the problem causing test failures is fixed, but let me know if you'd like me to review it sooner. |
| """Checks to see if the given site is enabled. | ||
|
|
||
| .. todo:: fix hardcoded sites-enabled, check os.path.samefile | ||
| def parsed_in_current(self, filep): |
There was a problem hiding this comment.
Feel free to ignore this comment if you disagree, but I think these three methods make more sense in parser.py.
There was a problem hiding this comment.
I do agree. These are now moved to parser.py
| # Must also attempt to parse virtual host root | ||
| self._parse_file(self.vhostroot + "/" + | ||
| constants.os_constant("vhost_files")) | ||
| self.existing_paths = copy.deepcopy(self.parser_paths) |
There was a problem hiding this comment.
One thing that worries me a bit is this is never updated if an Include directive is added later. What if we moved the add_include function into the parser and keep this value up to date?
There was a problem hiding this comment.
Nice catch! This functionality is now added to the PR
|
Suggested improvements have been done, good suggestions regarding code health, thanks! While adding new include paths to the |
| if not self.conf("handle-sites"): | ||
| if not self.parser.parsed_in_original(vhost.filep): | ||
| # Add direct include to root conf | ||
| self.parser.add_include(self.parser.loc["default"], vhost.filep) |
There was a problem hiding this comment.
We should probably set vhost.enabled to True after this to make sure the value stays up to date.
…on Debian / Ubuntu (#4104) This changes the apache plugin behaviour to only parse enabled configuration files and respecting the --apache-vhost-root CLI parameter for new SSL vhost creation. If --apache-vhost-root isn't defined, or doesn't exist, the SSL vhost will be created to originating non-SSL vhost directory. This PR also implements actual check for vhost enabled state, and makes sure parser.parse_file() does not discard changes in Augeas DOM, by doing an autosave. Also handles enabling the new SSL vhost, if it's on a path that's not parsed by Apache. Fixes: #1328 Fixes: #3545 Fixes: #3791 Fixes: #4523 Fixes: #4837 Fixes: #4905 * First changes * Handle rest of the errors * Test fixes * Final fixes * Make parse_files accessible and fix linter problems * Activate vhost at later time * Cleanup * Add a new test case, and fix old * Enable site later in deploy_cert * Make apache-conf-test default dummy configuration enabled * Remove is_sites_available as obsolete * Cleanup * Brought back conditional vhost_path parsing * Parenthesis * Fix merge leftovers * Fix to work with the recent changes to new file creation * Added fix and tests for non-symlink vhost in sites-enabled * Made vhostroot parameter for ApacheParser optional, and removed extra_path * Respect vhost-root, and add Include statements to root configuration if needed * Fixed site enabling order to prevent apache restart error while enabling mod_ssl * Don't exclude Ubuntu / Debian vhost-root cli argument * Changed the SSL vhost directory selection priority * Requested fixes for paths and vhost discovery * Make sure the Augeas DOM is written to disk before loading new files * Actual checking for if the file is parsed within existing Apache configuration * Fix the order of dummy SSL directives addition and enabling modules * Restructured site_enabled checks * Enabling vhost correctly for non-debian systems
This changes the apache plugin behaviour to only parse enabled configuration files,
while still writing new files to sites-available, and respecting the --apache-vhost-root CLI parameter for new SSL vhost creation. If --apache-vhost-root isn't defined, or doesn't exist, the SSL vhost will be created to originating non-SSL vhost directory.This PR also implements actual check for vhost enabled state, and makes sure parser.parse_file() does not discard changes in Augeas DOM, by doing an autosave.
Also handles enabling the new SSL vhost, if it's on a path that's not parsed by Apache.
Fixes: #1328
Fixes: #3545
Fixes: #3791
Fixes: #4523
Fixes: #4837
Fixes: #4905