Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use static directory under workdir for HTTP challenges #5428

Merged
merged 2 commits into from Jan 14, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion certbot-apache/certbot_apache/configurator.py
Expand Up @@ -1956,7 +1956,6 @@ def cleanup(self, achalls):
self.revert_challenge_config()
self.restart()
self.parser.reset_modules()
self.http_doer.cleanup()

def install_ssl_options_conf(self, options_ssl, options_ssl_digest):
"""Copy Certbot's SSL options file into the system's config dir if required."""
Expand Down
19 changes: 8 additions & 11 deletions certbot-apache/certbot_apache/http_01.py
@@ -1,8 +1,6 @@
"""A class that performs HTTP-01 challenges for Apache"""
import logging
import os
import shutil
import tempfile

from certbot.plugins import common

Expand Down Expand Up @@ -35,7 +33,9 @@ def __init__(self, *args, **kwargs):
self.challenge_conf = os.path.join(
self.configurator.conf("challenge-location"),
"le_http_01_challenge.conf")
self.challenge_dir = None
self.challenge_dir = os.path.join(
self.configurator.config.work_dir,
"http_challenges")

def perform(self):
"""Perform all HTTP-01 challenges."""
Expand All @@ -56,12 +56,6 @@ def perform(self):

return responses

def cleanup(self):
"""Cleanup the challenge directory."""
if self.challenge_dir:
shutil.rmtree(self.challenge_dir, ignore_errors=True)
self.challenge_dir = None

def prepare_http01_modules(self):
"""Make sure that we have the needed modules available for http01"""

Expand Down Expand Up @@ -92,8 +86,9 @@ def _mod_config(self):
new_conf.write(config_text)

def _set_up_challenges(self):
self.challenge_dir = tempfile.mkdtemp()
os.chmod(self.challenge_dir, 0o755)
if not os.path.isdir(self.challenge_dir):
os.makedirs(self.challenge_dir)
os.chmod(self.challenge_dir, 0o755)

responses = []
for achall in self.achalls:
Expand All @@ -105,8 +100,10 @@ def _set_up_challenge(self, achall):
response, validation = achall.response_and_validation()

name = os.path.join(self.challenge_dir, achall.chall.encode("token"))

with open(name, 'wb') as f:
f.write(validation.encode())
self.configurator.reverter.register_file_creation(True, name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should register the file with the reverter before creating it. While unlikely, if we get a signal/exception between writing the file and finishing registering it, it will never be deleted.

os.chmod(name, 0o644)

return response
5 changes: 0 additions & 5 deletions certbot-apache/certbot_apache/tests/configurator_test.py
Expand Up @@ -747,7 +747,6 @@ def test_perform(self, mock_restart, mock_tls_perform, mock_http_perform):
def test_cleanup(self, mock_cfg, mock_restart):
mock_cfg.return_value = ""
_, achalls = self.get_key_and_achalls()
self.config.http_doer = mock.MagicMock()

for achall in achalls:
self.config._chall_out.add(achall) # pylint: disable=protected-access
Expand All @@ -756,10 +755,8 @@ def test_cleanup(self, mock_cfg, mock_restart):
self.config.cleanup([achall])
if i == len(achalls) - 1:
self.assertTrue(mock_restart.called)
self.assertTrue(self.config.http_doer.cleanup.called)
else:
self.assertFalse(mock_restart.called)
self.assertFalse(self.config.http_doer.cleanup.called)

@mock.patch("certbot_apache.configurator.ApacheConfigurator.restart")
@mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg")
Expand All @@ -773,11 +770,9 @@ def test_cleanup_no_errors(self, mock_cfg, mock_restart):

self.config.cleanup([achalls[-1]])
self.assertFalse(mock_restart.called)
self.assertFalse(self.config.http_doer.cleanup.called)

self.config.cleanup(achalls)
self.assertTrue(mock_restart.called)
self.assertTrue(self.config.http_doer.cleanup.called)

@mock.patch("certbot.util.run_script")
def test_get_version(self, mock_script):
Expand Down
6 changes: 3 additions & 3 deletions certbot-apache/certbot_apache/tests/http_01_test.py
Expand Up @@ -100,6 +100,8 @@ def common_enable_modules_test(self, mock_enmod):

def common_perform_test(self, achalls):
"""Tests perform with the given achalls."""
challenge_dir = self.http.challenge_dir
self.assertFalse(os.path.exists(challenge_dir))
for achall in achalls:
self.http.add_chall(achall)

Expand All @@ -114,9 +116,7 @@ def common_perform_test(self, achalls):
for achall in achalls:
self._test_challenge_file(achall)

challenge_dir = self.http.challenge_dir
self.http.cleanup()
self.assertFalse(os.path.exists(challenge_dir))
self.assertTrue(os.path.exists(challenge_dir))

def _test_challenge_conf(self):
self.assertEqual(
Expand Down