diff --git a/.travis.yml b/.travis.yml index 46b14fe63..3041fdd82 100644 --- a/.travis.yml +++ b/.travis.yml @@ -21,6 +21,15 @@ env: - TOXENV=lint - TOXENV=cover + +# Only build pushes to the master branch, PRs, and branches beginning with +# `test-`. This reduces the number of simultaneous Travis runs, which speeds +# turnaround time on review since there is a cap of 5 simultaneous runs. +branches: + only: + - master + - /^test-.*$/ + sudo: false # containers addons: # make sure simplehttp simple verification works (custom /etc/hosts) diff --git a/acme/acme/jose/interfaces_test.py b/acme/acme/jose/interfaces_test.py index 91e6f4416..84dc2a1be 100644 --- a/acme/acme/jose/interfaces_test.py +++ b/acme/acme/jose/interfaces_test.py @@ -1,6 +1,8 @@ """Tests for acme.jose.interfaces.""" import unittest +import six + class JSONDeSerializableTest(unittest.TestCase): # pylint: disable=too-many-instance-attributes @@ -90,8 +92,9 @@ class JSONDeSerializableTest(unittest.TestCase): self.assertEqual('["foo1", "foo2"]', self.seq.json_dumps()) def test_json_dumps_pretty(self): - self.assertEqual( - self.seq.json_dumps_pretty(), '[\n "foo1", \n "foo2"\n]') + filler = ' ' if six.PY2 else '' + self.assertEqual(self.seq.json_dumps_pretty(), + '[\n "foo1",{0}\n "foo2"\n]'.format(filler)) def test_json_dump_default(self): from acme.jose.interfaces import JSONDeSerializable diff --git a/docs/contributing.rst b/docs/contributing.rst index 3277d321a..614f6f2aa 100644 --- a/docs/contributing.rst +++ b/docs/contributing.rst @@ -267,6 +267,22 @@ Please: .. _PEP 8 - Style Guide for Python Code: https://www.python.org/dev/peps/pep-0008 +Submitting a pull request +========================= + +Steps: + +1. Write your code! +2. Make sure your environment is set up properly and that you're in your + virtualenv. You can do this by running ``./bootstrap/dev/venv.sh``. + (this is a **very important** step) +3. Run ``./pep8.travis.sh`` to do a cursory check of your code style. + Fix any errors. +4. Run ``tox -e lint`` to check for pylint errors. Fix any errors. +5. Run ``tox`` to run the entire test suite including coverage. Fix any errors. +6. If your code touches communication with an ACME server/Boulder, you + should run the integration tests, see `integration`_. +7. Submit the PR. Updating the documentation ========================== diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index f3d2b5f9a..de69af91c 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -1,5 +1,6 @@ """Apache Configuration based off of Augeas Configurator.""" # pylint: disable=too-many-lines +import filecmp import itertools import logging import os @@ -163,7 +164,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): temp_install(self.mod_ssl_conf) - def deploy_cert(self, domain, cert_path, key_path, chain_path=None): + def deploy_cert(self, domain, cert_path, key_path, + chain_path=None, fullchain_path=None): # pylint: disable=unused-argument """Deploys certificate to specified virtual host. Currently tries to find the last directives to deploy the cert in @@ -945,9 +947,11 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): """ enabled_dir = os.path.join(self.parser.root, "sites-enabled") for entry in os.listdir(enabled_dir): - if os.path.realpath(os.path.join(enabled_dir, entry)) == avail_fp: - return True - + try: + if filecmp.cmp(avail_fp, os.path.join(enabled_dir, entry)): + return True + except OSError: + pass return False def enable_site(self, vhost): diff --git a/letsencrypt-compatibility-test/letsencrypt_compatibility_test/configurators/apache/a2enmod.sh b/letsencrypt-compatibility-test/letsencrypt_compatibility_test/configurators/apache/a2enmod.sh index f822a1f7b..4da9288a2 100755 --- a/letsencrypt-compatibility-test/letsencrypt_compatibility_test/configurators/apache/a2enmod.sh +++ b/letsencrypt-compatibility-test/letsencrypt_compatibility_test/configurators/apache/a2enmod.sh @@ -1,33 +1,18 @@ #!/bin/bash # An extremely simplified version of `a2enmod` for enabling modules in the -# httpd docker image. First argument is the server_root and the second is the -# module to be enabled. +# httpd docker image. First argument is the Apache ServerRoot which should be +# an absolute path. The second is the module to be enabled, such as `ssl`. -APACHE_CONFDIR=$1 +confdir=$1 +module=$2 -enable () { - echo "LoadModule "$1"_module /usr/local/apache2/modules/mod_"$1".so" >> \ - $APACHE_CONFDIR"/test.conf" - available_base="/mods-available/"$1".conf" - available_conf=$APACHE_CONFDIR$available_base - enabled_dir=$APACHE_CONFDIR"/mods-enabled" - enabled_conf=$enabled_dir"/"$1".conf" - if [ -e "$available_conf" -a -d "$enabled_dir" -a ! -e "$enabled_conf" ] - then - ln -s "..$available_base" $enabled_conf - fi -} - -if [ $2 == "ssl" ] +echo "LoadModule ${module}_module " \ + "/usr/local/apache2/modules/mod_${module}.so" >> "${confdir}/test.conf" +availbase="/mods-available/${module}.conf" +availconf=$confdir$availbase +enabldir="$confdir/mods-enabled" +enablconf="$enabldir/${module}.conf" +if [ -e $availconf -a -d $enabldir -a ! -e $enablconf ] then - # Enables ssl and all its dependencies - enable "setenvif" - enable "mime" - enable "socache_shmcb" - enable "ssl" -elif [ $2 == "rewrite" ] -then - enable "rewrite" -else - exit 1 + ln -s "..$availbase" $enablconf fi diff --git a/letsencrypt-compatibility-test/letsencrypt_compatibility_test/configurators/apache/common.py b/letsencrypt-compatibility-test/letsencrypt_compatibility_test/configurators/apache/common.py index 0d3dbb1b5..5f183b611 100644 --- a/letsencrypt-compatibility-test/letsencrypt_compatibility_test/configurators/apache/common.py +++ b/letsencrypt-compatibility-test/letsencrypt_compatibility_test/configurators/apache/common.py @@ -175,12 +175,13 @@ class Proxy(configurators_common.Proxy): else: return {"example.com"} - def deploy_cert(self, domain, cert_path, key_path, chain_path=None): + def deploy_cert(self, domain, cert_path, key_path, chain_path=None, + fullchain_path=None): """Installs cert""" cert_path, key_path, chain_path = self.copy_certs_and_keys( cert_path, key_path, chain_path) self._apache_configurator.deploy_cert( - domain, cert_path, key_path, chain_path) + domain, cert_path, key_path, chain_path, fullchain_path) def _is_apache_command(command): diff --git a/letsencrypt-compatibility-test/letsencrypt_compatibility_test/util.py b/letsencrypt-compatibility-test/letsencrypt_compatibility_test/util.py index 6181da16b..43070cf03 100644 --- a/letsencrypt-compatibility-test/letsencrypt_compatibility_test/util.py +++ b/letsencrypt-compatibility-test/letsencrypt_compatibility_test/util.py @@ -39,7 +39,7 @@ def create_le_config(parent_dir): def extract_configs(configs, parent_dir): """Extracts configs to a new dir under parent_dir and returns it""" - config_dir = os.path.join(parent_dir, "renewal") + config_dir = os.path.join(parent_dir, "configs") if os.path.isdir(configs): shutil.copytree(configs, config_dir, symlinks=True) diff --git a/letsencrypt-nginx/letsencrypt_nginx/configurator.py b/letsencrypt-nginx/letsencrypt_nginx/configurator.py index a88607e58..d1ab8f3d1 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/configurator.py +++ b/letsencrypt-nginx/letsencrypt_nginx/configurator.py @@ -6,6 +6,7 @@ import shutil import socket import subprocess import sys +import time import OpenSSL import zope.interface @@ -117,30 +118,44 @@ class NginxConfigurator(common.Plugin): temp_install(self.mod_ssl_conf) # Entry point in main.py for installing cert - def deploy_cert(self, domain, cert_path, key_path, chain_path=None): + def deploy_cert(self, domain, cert_path, key_path, + chain_path, fullchain_path): # pylint: disable=unused-argument """Deploys certificate to specified virtual host. .. note:: Aborts if the vhost is missing ssl_certificate or ssl_certificate_key. - .. note:: Nginx doesn't have a cert chain directive, so the last - parameter is always ignored. It expects the cert file to have - the concatenated chain. + .. note:: Nginx doesn't have a cert chain directive. + It expects the cert file to have the concatenated chain. + However, we use the chain file as input to the + ssl_trusted_certificate directive, used for verify OCSP responses. .. note:: This doesn't save the config files! """ vhost = self.choose_vhost(domain) - directives = [['ssl_certificate', cert_path], - ['ssl_certificate_key', key_path]] + cert_directives = [['ssl_certificate', fullchain_path], + ['ssl_certificate_key', key_path]] + + # OCSP stapling was introduced in Nginx 1.3.7. If we have that version + # or greater, add config settings for it. + stapling_directives = [] + if self.version >= (1, 3, 7): + stapling_directives = [ + ['ssl_trusted_certificate', chain_path], + ['ssl_stapling', 'on'], + ['ssl_stapling_verify', 'on']] try: self.parser.add_server_directives(vhost.filep, vhost.names, - directives, True) + cert_directives, replace=True) + self.parser.add_server_directives(vhost.filep, vhost.names, + stapling_directives, replace=False) logger.info("Deployed Certificate to VirtualHost %s for %s", vhost.filep, vhost.names) - except errors.MisconfigurationError: + except errors.MisconfigurationError as error: + logger.debug(error) logger.warn( "Cannot find a cert or key directive in %s for %s. " "VirtualHost was not modified.", vhost.filep, vhost.names) @@ -598,6 +613,10 @@ def nginx_restart(nginx_ctl, nginx_conf="/etc/nginx.conf"): except (OSError, ValueError): logger.fatal("Nginx Restart Failed - Please Check the Configuration") sys.exit(1) + # Nginx can take a moment to recognize a newly added TLS SNI servername, so sleep + # for a second. TODO: Check for expected servername and loop until it + # appears or return an error if looping too long. + time.sleep(1) return True diff --git a/letsencrypt-nginx/letsencrypt_nginx/dvsni.py b/letsencrypt-nginx/letsencrypt_nginx/dvsni.py index bd9ca783f..9ac2fcd7c 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/dvsni.py +++ b/letsencrypt-nginx/letsencrypt_nginx/dvsni.py @@ -90,14 +90,22 @@ class NginxDvsni(common.Dvsni): # Add the 'include' statement for the challenges if it doesn't exist # already in the main config included = False - directive = ['include', self.challenge_conf] + include_directive = ['include', self.challenge_conf] root = self.configurator.parser.loc["root"] + + bucket_directive = ['server_names_hash_bucket_size', '128'] + main = self.configurator.parser.parsed[root] - for entry in main: - if entry[0] == ['http']: - body = entry[1] - if directive not in body: - body.append(directive) + for key, body in main: + if key == ['http']: + found_bucket = False + for key, _ in body: + if key == bucket_directive[0]: + found_bucket = True + if not found_bucket: + body.insert(0, bucket_directive) + if include_directive not in body: + body.insert(0, include_directive) included = True break if not included: diff --git a/letsencrypt-nginx/letsencrypt_nginx/parser.py b/letsencrypt-nginx/letsencrypt_nginx/parser.py index ef87cc653..fc8ed95f1 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/parser.py +++ b/letsencrypt-nginx/letsencrypt_nginx/parser.py @@ -476,8 +476,12 @@ def _add_directives(block, directives, replace=False): :param list directives: The new directives. """ - if replace: - for directive in directives: + for directive in directives: + if not replace: + # We insert new directives at the top of the block, mostly + # to work around https://trac.nginx.org/nginx/ticket/810 + block.insert(0, directive) + else: changed = False if len(directive) == 0: continue @@ -489,5 +493,3 @@ def _add_directives(block, directives, replace=False): raise errors.MisconfigurationError( 'LetsEncrypt expected directive for %s in the Nginx ' 'config but did not find it.' % directive[0]) - else: - block.extend(directives) diff --git a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py b/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py index 977a65330..203f9920c 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py +++ b/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py @@ -63,11 +63,11 @@ class NginxConfiguratorTest(util.NginxTest): # pylint: disable=protected-access parsed = self.config.parser._parse_files(filep, override=True) - self.assertEqual([[['server'], [['listen', '69.50.225.155:9000'], + self.assertEqual([[['server'], [['listen', '5001 ssl'], + ['listen', '69.50.225.155:9000'], ['listen', '127.0.0.1'], ['server_name', '.example.com'], - ['server_name', 'example.*'], - ['listen', '5001 ssl']]]], + ['server_name', 'example.*']]]], parsed[0]) def test_choose_vhost(self): @@ -96,18 +96,49 @@ class NginxConfiguratorTest(util.NginxTest): def test_more_info(self): self.assertTrue('nginx.conf' in self.config.more_info()) + def test_deploy_cert_stapling(self): + # Choose a version of Nginx greater than 1.3.7 so stapling code gets + # invoked. + self.config.version = (1, 9, 6) + example_conf = self.config.parser.abs_path('sites-enabled/example.com') + self.config.deploy_cert( + "www.example.com", + "example/cert.pem", + "example/key.pem", + "example/chain.pem", + "example/fullchain.pem") + self.config.save() + self.config.parser.load() + generated_conf = self.config.parser.parsed[example_conf] + + self.assertTrue(util.contains_at_depth(generated_conf, + ['ssl_stapling', 'on'], 2)) + self.assertTrue(util.contains_at_depth(generated_conf, + ['ssl_stapling_verify', 'on'], 2)) + self.assertTrue(util.contains_at_depth(generated_conf, + ['ssl_trusted_certificate', 'example/chain.pem'], 2)) + def test_deploy_cert(self): server_conf = self.config.parser.abs_path('server.conf') nginx_conf = self.config.parser.abs_path('nginx.conf') example_conf = self.config.parser.abs_path('sites-enabled/example.com') + # Choose a version of Nginx less than 1.3.7 so stapling code doesn't get + # invoked. + self.config.version = (1, 3, 1) # Get the default SSL vhost self.config.deploy_cert( "www.example.com", - "example/cert.pem", "example/key.pem") + "example/cert.pem", + "example/key.pem", + "example/chain.pem", + "example/fullchain.pem") self.config.deploy_cert( "another.alias", - "/etc/nginx/cert.pem", "/etc/nginx/key.pem") + "/etc/nginx/cert.pem", + "/etc/nginx/key.pem", + "/etc/nginx/chain.pem", + "/etc/nginx/fullchain.pem") self.config.save() self.config.parser.load() @@ -119,35 +150,34 @@ class NginxConfiguratorTest(util.NginxTest): access_log = os.path.join(self.work_dir, "access.log") error_log = os.path.join(self.work_dir, "error.log") self.assertEqual([[['server'], - [['listen', '69.50.225.155:9000'], + [['include', self.config.parser.loc["ssl_options"]], + ['ssl_certificate_key', 'example/key.pem'], + ['ssl_certificate', 'example/fullchain.pem'], + ['error_log', error_log], + ['access_log', access_log], + + ['listen', '5001 ssl'], + ['listen', '69.50.225.155:9000'], ['listen', '127.0.0.1'], ['server_name', '.example.com'], - ['server_name', 'example.*'], - ['listen', '5001 ssl'], - ['access_log', access_log], - ['error_log', error_log], - ['ssl_certificate', 'example/cert.pem'], - ['ssl_certificate_key', 'example/key.pem'], - ['include', - self.config.parser.loc["ssl_options"]]]]], + ['server_name', 'example.*']]]], parsed_example_conf) self.assertEqual([['server_name', 'somename alias another.alias']], parsed_server_conf) - self.assertEqual([['server'], - [['listen', '8000'], - ['listen', 'somename:8080'], - ['include', 'server.conf'], - [['location', '/'], - [['root', 'html'], - ['index', 'index.html index.htm']]], - ['listen', '5001 ssl'], - ['access_log', access_log], - ['error_log', error_log], - ['ssl_certificate', '/etc/nginx/cert.pem'], - ['ssl_certificate_key', '/etc/nginx/key.pem'], - ['include', - self.config.parser.loc["ssl_options"]]]], - parsed_nginx_conf[-1][-1][-1]) + self.assertTrue(util.contains_at_depth(parsed_nginx_conf, + [['server'], + [['include', self.config.parser.loc["ssl_options"]], + ['ssl_certificate_key', '/etc/nginx/key.pem'], + ['ssl_certificate', '/etc/nginx/fullchain.pem'], + ['error_log', error_log], + ['access_log', access_log], + ['listen', '5001 ssl'], + ['listen', '8000'], + ['listen', 'somename:8080'], + ['include', 'server.conf'], + [['location', '/'], + [['root', 'html'], ['index', 'index.html index.htm']]]]], + 2)) def test_get_all_certs_keys(self): nginx_conf = self.config.parser.abs_path('nginx.conf') @@ -156,16 +186,22 @@ class NginxConfiguratorTest(util.NginxTest): # Get the default SSL vhost self.config.deploy_cert( "www.example.com", - "example/cert.pem", "example/key.pem") + "example/cert.pem", + "example/key.pem", + "example/chain.pem", + "example/fullchain.pem") self.config.deploy_cert( "another.alias", - "/etc/nginx/cert.pem", "/etc/nginx/key.pem") + "/etc/nginx/cert.pem", + "/etc/nginx/key.pem", + "/etc/nginx/chain.pem", + "/etc/nginx/fullchain.pem") self.config.save() self.config.parser.load() self.assertEqual(set([ - ('example/cert.pem', 'example/key.pem', example_conf), - ('/etc/nginx/cert.pem', '/etc/nginx/key.pem', nginx_conf), + ('example/fullchain.pem', 'example/key.pem', example_conf), + ('/etc/nginx/fullchain.pem', '/etc/nginx/key.pem', nginx_conf), ]), self.config.get_all_certs_keys()) @mock.patch("letsencrypt_nginx.configurator.dvsni.NginxDvsni.perform") diff --git a/letsencrypt-nginx/letsencrypt_nginx/tests/dvsni_test.py b/letsencrypt-nginx/letsencrypt_nginx/tests/dvsni_test.py index a09bebba2..9fc0a1ad7 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tests/dvsni_test.py +++ b/letsencrypt-nginx/letsencrypt_nginx/tests/dvsni_test.py @@ -85,7 +85,8 @@ class DvsniPerformTest(util.NginxTest): # Make sure challenge config is included in main config http = self.sni.configurator.parser.parsed[ self.sni.configurator.parser.loc["root"]][-1] - self.assertTrue(['include', self.sni.challenge_conf] in http[1]) + self.assertTrue( + util.contains_at_depth(http, ['include', self.sni.challenge_conf], 1)) def test_perform2(self): acme_responses = [] @@ -108,7 +109,8 @@ class DvsniPerformTest(util.NginxTest): http = self.sni.configurator.parser.parsed[ self.sni.configurator.parser.loc["root"]][-1] self.assertTrue(['include', self.sni.challenge_conf] in http[1]) - self.assertTrue(['server_name', 'blah'] in http[1][-2][1]) + self.assertTrue( + util.contains_at_depth(http, ['server_name', 'blah'], 3)) self.assertEqual(len(sni_responses), 3) for i in xrange(3): diff --git a/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py b/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py index 0af81aefe..b28640d7f 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py +++ b/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py @@ -128,18 +128,20 @@ class NginxParserTest(util.NginxTest): r'~^(www\.)?(example|bar)\.']), [['foo', 'bar'], ['ssl_certificate', '/etc/ssl/cert.pem']]) - ssl_re = re.compile(r'foo bar;\n\s+ssl_certificate /etc/ssl/cert.pem') - self.assertEqual(1, len(re.findall(ssl_re, nginxparser.dumps( - nparser.parsed[nparser.abs_path('nginx.conf')])))) - nparser.add_server_directives(nparser.abs_path('server.conf'), + ssl_re = re.compile(r'\n\s+ssl_certificate /etc/ssl/cert.pem') + dump = nginxparser.dumps(nparser.parsed[nparser.abs_path('nginx.conf')]) + self.assertEqual(1, len(re.findall(ssl_re, dump))) + + server_conf = nparser.abs_path('server.conf') + nparser.add_server_directives(server_conf, set(['alias', 'another.alias', 'somename']), [['foo', 'bar'], ['ssl_certificate', '/etc/ssl/cert2.pem']]) - self.assertEqual(nparser.parsed[nparser.abs_path('server.conf')], - [['server_name', 'somename alias another.alias'], + self.assertEqual(nparser.parsed[server_conf], + [['ssl_certificate', '/etc/ssl/cert2.pem'], ['foo', 'bar'], - ['ssl_certificate', '/etc/ssl/cert2.pem']]) + ['server_name', 'somename alias another.alias']]) def test_add_http_directives(self): nparser = parser.NginxParser(self.config_path, self.ssl_options) @@ -148,8 +150,15 @@ class NginxParserTest(util.NginxTest): [['listen', '80'], ['server_name', 'localhost']]] nparser.add_http_directives(filep, block) - self.assertEqual(nparser.parsed[filep][-1][0], ['http']) - self.assertEqual(nparser.parsed[filep][-1][1][-1], block) + root = nparser.parsed[filep] + self.assertTrue(util.contains_at_depth(root, ['http'], 1)) + self.assertTrue(util.contains_at_depth(root, block, 2)) + + # Check that our server block got inserted first among all server + # blocks. + http_block = [x for x in root if x[0] == ['http']][0][1] + server_blocks = [x for x in http_block if x[0] == ['server']] + self.assertEqual(server_blocks[0], block) def test_replace_server_directives(self): nparser = parser.NginxParser(self.config_path, self.ssl_options) diff --git a/letsencrypt-nginx/letsencrypt_nginx/tests/util.py b/letsencrypt-nginx/letsencrypt_nginx/tests/util.py index 363944490..953c5d367 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tests/util.py +++ b/letsencrypt-nginx/letsencrypt_nginx/tests/util.py @@ -85,3 +85,22 @@ def filter_comments(tree): yield [key, values] return list(traverse(tree)) + + +def contains_at_depth(haystack, needle, n): + """Is the needle in haystack at depth n? + + Return true if the needle is present in one of the sub-iterables in haystack + at depth n. Haystack must be an iterable. + """ + # Specifically use hasattr rather than isinstance(..., collections.Iterable) + # because we want to include lists but reject strings. + if not hasattr(haystack, '__iter__'): + return False + if n == 0: + return needle in haystack + else: + for item in haystack: + if contains_at_depth(item, needle, n - 1): + return True + return False diff --git a/letsencrypt-nginx/tests/boulder-integration.conf.sh b/letsencrypt-nginx/tests/boulder-integration.conf.sh index 006d68836..12610d895 100755 --- a/letsencrypt-nginx/tests/boulder-integration.conf.sh +++ b/letsencrypt-nginx/tests/boulder-integration.conf.sh @@ -20,7 +20,6 @@ events { } http { - server_names_hash_bucket_size 2048; # Set an array of temp and cache file options that will otherwise default to # restricted locations accessible only to root. client_body_temp_path $root/client_body; diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 77480f69f..9a0dd9108 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -397,9 +397,9 @@ def run(args, config, plugins): # pylint: disable=too-many-branches,too-many-lo lineage = _auth_from_domains(le_client, config, domains, plugins) - # TODO: We also need to pass the fullchain (for Nginx) le_client.deploy_certificate( - domains, lineage.privkey, lineage.cert, lineage.chain) + domains, lineage.privkey, lineage.cert, + lineage.chain, lineage.fullchain) le_client.enhance_config(domains, args.redirect) if len(lineage.available_versions("cert")) == 1: @@ -452,7 +452,8 @@ def install(args, config, plugins): args, config, authenticator=None, installer=installer) assert args.cert_path is not None # required=True in the subparser le_client.deploy_certificate( - domains, args.key_path, args.cert_path, args.chain_path) + domains, args.key_path, args.cert_path, args.chain_path, + args.fullchain_path) le_client.enhance_config(domains, args.redirect) @@ -867,6 +868,8 @@ def _paths_parser(helpful): default_cp = None if verb == "auth": default_cp = flag_default("auth_chain_path") + add("paths", "--fullchain-path", default=default_cp, + help="Accompanying path to a full certificate chain (cert plus chain).") add("paths", "--chain-path", default=default_cp, help="Accompanying path to a certificate chain.") add("paths", "--config-dir", default=flag_default("config_dir"), @@ -903,9 +906,23 @@ def _plugins_parsing(helpful, plugins): helpful.add_plugin_args(plugins) -def _setup_logging(args): - level = -args.verbose_count * 10 - fmt = "%(asctime)s:%(levelname)s:%(name)s:%(message)s" +def setup_log_file_handler(args, logfile, fmt): + """Setup file debug logging.""" + log_file_path = os.path.join(args.logs_dir, logfile) + handler = logging.handlers.RotatingFileHandler( + log_file_path, maxBytes=2 ** 20, backupCount=10) + # rotate on each invocation, rollover only possible when maxBytes + # is nonzero and backupCount is nonzero, so we set maxBytes as big + # as possible not to overrun in single CLI invocation (1MB). + handler.doRollover() # TODO: creates empty letsencrypt.log.1 file + handler.setLevel(logging.DEBUG) + handler_formatter = logging.Formatter(fmt=fmt) + handler_formatter.converter = time.gmtime # don't use localtime + handler.setFormatter(handler_formatter) + return handler, log_file_path + + +def _cli_log_handler(args, level, fmt): if args.text_mode: handler = colored_logging.StreamHandler() handler.setFormatter(logging.Formatter(fmt)) @@ -914,30 +931,26 @@ def _setup_logging(args): # dialog box is small, display as less as possible handler.setFormatter(logging.Formatter("%(message)s")) handler.setLevel(level) + return handler + + +def setup_logging(args, cli_handler_factory, logfile): + """Setup logging.""" + fmt = "%(asctime)s:%(levelname)s:%(name)s:%(message)s" + level = -args.verbose_count * 10 + file_handler, log_file_path = setup_log_file_handler( + args, logfile=logfile, fmt=fmt) + cli_handler = cli_handler_factory(args, level, fmt) # TODO: use fileConfig? - # unconditionally log to file for debugging purposes - # TODO: change before release? - log_file_name = os.path.join(args.logs_dir, 'letsencrypt.log') - file_handler = logging.handlers.RotatingFileHandler( - log_file_name, maxBytes=2 ** 20, backupCount=10) - # rotate on each invocation, rollover only possible when maxBytes - # is nonzero and backupCount is nonzero, so we set maxBytes as big - # as possible not to overrun in single CLI invocation (1MB). - file_handler.doRollover() # TODO: creates empty letsencrypt.log.1 file - file_handler.setLevel(logging.DEBUG) - file_handler_formatter = logging.Formatter(fmt=fmt) - file_handler_formatter.converter = time.gmtime # don't use localtime - file_handler.setFormatter(file_handler_formatter) - root_logger = logging.getLogger() root_logger.setLevel(logging.DEBUG) # send all records to handlers - root_logger.addHandler(handler) + root_logger.addHandler(cli_handler) root_logger.addHandler(file_handler) logger.debug("Root logging level set at %d", level) - logger.info("Saving debug log to %s", log_file_name) + logger.info("Saving debug log to %s", log_file_path) def _handle_exception(exc_type, exc_value, trace, args): @@ -1006,7 +1019,7 @@ def main(cli_args=sys.argv[1:]): # private key! #525 le_util.make_or_verify_dir( args.logs_dir, 0o700, os.geteuid(), "--strict-permissions" in cli_args) - _setup_logging(args) + setup_logging(args, _cli_log_handler, logfile='letsencrypt.log') # do not log `args`, as it contains sensitive data (e.g. revoke --key)! logger.debug("Arguments: %r", cli_args) diff --git a/letsencrypt/client.py b/letsencrypt/client.py index 7a78add38..123bab121 100644 --- a/letsencrypt/client.py +++ b/letsencrypt/client.py @@ -336,7 +336,8 @@ class Client(object): return os.path.abspath(act_cert_path), cert_chain_abspath - def deploy_certificate(self, domains, privkey_path, cert_path, chain_path): + def deploy_certificate(self, domains, privkey_path, + cert_path, chain_path, fullchain_path): """Install certificate :param list domains: list of domains to install the certificate @@ -357,8 +358,10 @@ class Client(object): # TODO: Provide a fullchain reference for installers like # nginx that want it self.installer.deploy_cert( - dom, os.path.abspath(cert_path), - os.path.abspath(privkey_path), chain_path) + domain=dom, cert_path=os.path.abspath(cert_path), + key_path=os.path.abspath(privkey_path), + chain_path=chain_path, + fullchain_path=fullchain_path) self.installer.save("Deployed Let's Encrypt Certificate") # sites may have been enabled / final cleanup diff --git a/letsencrypt/constants.py b/letsencrypt/constants.py index 762409d25..362009ec6 100644 --- a/letsencrypt/constants.py +++ b/letsencrypt/constants.py @@ -27,6 +27,7 @@ CLI_DEFAULTS = dict( auth_cert_path="./cert.pem", auth_chain_path="./chain.pem", + strict_permissions=False, ) """Defaults for CLI flags and `.IConfig` attributes.""" diff --git a/letsencrypt/interfaces.py b/letsencrypt/interfaces.py index 5e82d61aa..8bf714c88 100644 --- a/letsencrypt/interfaces.py +++ b/letsencrypt/interfaces.py @@ -241,13 +241,15 @@ class IInstaller(IPlugin): """ - def deploy_cert(domain, cert_path, key_path, chain_path=None): + def deploy_cert(domain, cert_path, key_path, chain_path, fullchain_path): """Deploy certificate. :param str domain: domain to deploy certificate file :param str cert_path: absolute path to the certificate file :param str key_path: absolute path to the private key file :param str chain_path: absolute path to the certificate chain file + :param str fullchain_path: absolute path to the certificate fullchain + file (cert plus chain) :raises .PluginError: when cert cannot be deployed diff --git a/letsencrypt/plugins/manual.py b/letsencrypt/plugins/manual.py index 9d5ef87e9..99463c362 100644 --- a/letsencrypt/plugins/manual.py +++ b/letsencrypt/plugins/manual.py @@ -129,12 +129,18 @@ binary for temporary key/certificate generation.""".replace("\n", "") ct=response.CONTENT_TYPE, port=port) if self.conf("test-mode"): logger.debug("Test mode. Executing the manual command: %s", command) + # sh shipped with OS X does't support echo -n + if sys.platform == "darwin": + executable = "/bin/bash" + else: + executable = None try: self._httpd = subprocess.Popen( command, # don't care about setting stdout and stderr, # we're in test mode anyway shell=True, + executable=executable, # "preexec_fn" is UNIX specific, but so is "command" preexec_fn=os.setsid) except OSError as error: # ValueError should not happen! diff --git a/letsencrypt/plugins/null.py b/letsencrypt/plugins/null.py index 4ba6c9d64..e87537684 100644 --- a/letsencrypt/plugins/null.py +++ b/letsencrypt/plugins/null.py @@ -30,7 +30,8 @@ class Installer(common.Plugin): def get_all_names(self): return [] - def deploy_cert(self, domain, cert_path, key_path, chain_path=None): + def deploy_cert(self, domain, cert_path, key_path, + chain_path=None, fullchain_path=None): pass # pragma: no cover def enhance(self, domain, enhancement, options=None): diff --git a/letsencrypt/plugins/standalone/authenticator.py b/letsencrypt/plugins/standalone/authenticator.py index 968063781..a9972fba2 100644 --- a/letsencrypt/plugins/standalone/authenticator.py +++ b/letsencrypt/plugins/standalone/authenticator.py @@ -1,4 +1,5 @@ """Standalone authenticator.""" +import logging import os import psutil import signal @@ -19,6 +20,9 @@ from letsencrypt import interfaces from letsencrypt.plugins import common +logger = logging.getLogger(__name__) + + class StandaloneAuthenticator(common.Plugin): # pylint: disable=too-many-instance-attributes """Standalone authenticator. @@ -301,10 +305,21 @@ class StandaloneAuthenticator(common.Plugin): :param int port: The TCP port in question. :returns: True or False.""" - listeners = [conn.pid for conn in psutil.net_connections() + try: + net_connections = psutil.net_connections() + except psutil.AccessDenied as error: + logger.info("Access denied when trying to list network " + "connections: %s. Are you root?", error) + # this function is just a pre-check that often causes false + # positives and problems in testing (c.f. #680 on Mac, #255 + # generally); we will fail later in bind() anyway + return False + + listeners = [conn.pid for conn in net_connections if conn.status == 'LISTEN' and conn.type == socket.SOCK_STREAM and conn.laddr[1] == port] + try: if listeners and listeners[0] is not None: # conn.pid may be None if the current process doesn't have diff --git a/letsencrypt/renewer.py b/letsencrypt/renewer.py index 1c9cddc95..8d8540e6f 100644 --- a/letsencrypt/renewer.py +++ b/letsencrypt/renewer.py @@ -8,6 +8,7 @@ within lineages of successor certificates, according to configuration. """ import argparse +import logging import os import sys @@ -17,10 +18,13 @@ import zope.component from letsencrypt import account from letsencrypt import configuration +from letsencrypt import constants +from letsencrypt import colored_logging from letsencrypt import cli from letsencrypt import client from letsencrypt import crypto_util from letsencrypt import errors +from letsencrypt import le_util from letsencrypt import notify from letsencrypt import storage @@ -28,6 +32,9 @@ from letsencrypt.display import util as display_util from letsencrypt.plugins import disco as plugins_disco +logger = logging.getLogger(__name__) + + class _AttrDict(dict): """Attribute dictionary. @@ -104,6 +111,12 @@ def renew(cert, old_version): # (where fewer than all names were renewed) +def _cli_log_handler(args, level, fmt): # pylint: disable=unused-argument + handler = colored_logging.StreamHandler() + handler.setFormatter(logging.Formatter(fmt)) + return handler + + def _paths_parser(parser): add = parser.add_argument_group("paths").add_argument add("--config-dir", default=cli.flag_default("config_dir"), @@ -119,11 +132,16 @@ def _paths_parser(parser): def _create_parser(): parser = argparse.ArgumentParser() #parser.add_argument("--cron", action="store_true", help="Run as cronjob.") - # pylint: disable=protected-access + parser.add_argument( + "-v", "--verbose", dest="verbose_count", action="count", + default=cli.flag_default("verbose_count"), help="This flag can be used " + "multiple times to incrementally increase the verbosity of output, " + "e.g. -vvv.") + return _paths_parser(parser) -def main(config=None, args=sys.argv[1:]): +def main(config=None, cli_args=sys.argv[1:]): """Main function for autorenewer script.""" # TODO: Distinguish automated invocation from manual invocation, # perhaps by looking at sys.argv[0] and inhibiting automated @@ -133,8 +151,13 @@ def main(config=None, args=sys.argv[1:]): zope.component.provideUtility(display_util.FileDisplay(sys.stdout)) - cli_config = configuration.RenewerConfiguration( - _create_parser().parse_args(args)) + args = _create_parser().parse_args(cli_args) + + uid = os.geteuid() + le_util.make_or_verify_dir(args.logs_dir, 0o700, uid) + cli.setup_logging(args, _cli_log_handler, logfile='renewer.log') + + cli_config = configuration.RenewerConfiguration(args) config = storage.config_with_defaults(config) # Now attempt to read the renewer config file and augment or replace @@ -145,6 +168,9 @@ def main(config=None, args=sys.argv[1:]): # specify a config file on the command line, which, if provided, should # take precedence over this one. config.merge(configobj.ConfigObj(cli_config.renewer_config_file)) + # Ensure that all of the needed folders have been created before continuing + le_util.make_or_verify_dir(cli_config.work_dir, + constants.CONFIG_DIRS_MODE, uid) for i in os.listdir(cli_config.renewal_configs_dir): print "Processing", i diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index d0fae370d..f690e77f9 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -75,7 +75,10 @@ class CLITest(unittest.TestCase): output.truncate(0) self.assertRaises(SystemExit, self._call_stdout, ['-h', 'nginx']) out = output.getvalue() - self.assertTrue("--nginx-ctl" in out) + from letsencrypt.plugins import disco + if "nginx" in disco.PluginsRegistry.find_all(): + # may be false while building distributions without plugins + self.assertTrue("--nginx-ctl" in out) self.assertTrue("--manual-test-mode" not in out) self.assertTrue("--checkpoints" not in out) output.truncate(0) diff --git a/letsencrypt/tests/client_test.py b/letsencrypt/tests/client_test.py index 1e63bdbb6..fddb86607 100644 --- a/letsencrypt/tests/client_test.py +++ b/letsencrypt/tests/client_test.py @@ -177,15 +177,19 @@ class ClientTest(unittest.TestCase): def test_deploy_certificate(self): self.assertRaises(errors.Error, self.client.deploy_certificate, - ["foo.bar"], "key", "cert", "chain") + ["foo.bar"], "key", "cert", "chain", "fullchain") installer = mock.MagicMock() self.client.installer = installer - self.client.deploy_certificate(["foo.bar"], "key", "cert", "chain") + self.client.deploy_certificate(["foo.bar"], "key", "cert", "chain", + "fullchain") installer.deploy_cert.assert_called_once_with( - "foo.bar", os.path.abspath("cert"), - os.path.abspath("key"), os.path.abspath("chain")) + cert_path=os.path.abspath("cert"), + chain_path=os.path.abspath("chain"), + domain='foo.bar', + fullchain_path='fullchain', + key_path=os.path.abspath("key")) self.assertEqual(installer.save.call_count, 1) installer.restart.assert_called_once_with() diff --git a/letsencrypt/tests/renewer_test.py b/letsencrypt/tests/renewer_test.py index 6f115abf9..bc8afbcb5 100644 --- a/letsencrypt/tests/renewer_test.py +++ b/letsencrypt/tests/renewer_test.py @@ -44,8 +44,15 @@ class BaseRenewableCertTest(unittest.TestCase): self.tempdir = tempfile.mkdtemp() self.cli_config = configuration.RenewerConfiguration( - namespace=mock.MagicMock(config_dir=self.tempdir)) + namespace=mock.MagicMock( + config_dir=self.tempdir, + work_dir=self.tempdir, + logs_dir=self.tempdir, + ) + ) + # TODO: maybe provide RenewerConfiguration.make_dirs? + # TODO: main() should create those dirs, c.f. #902 os.makedirs(os.path.join(self.tempdir, "live", "example.org")) os.makedirs(os.path.join(self.tempdir, "archive", "example.org")) os.makedirs(os.path.join(self.tempdir, "renewal")) @@ -62,6 +69,9 @@ class BaseRenewableCertTest(unittest.TestCase): self.test_rc = storage.RenewableCert( self.config, self.defaults, self.cli_config) + def tearDown(self): + shutil.rmtree(self.tempdir) + def _write_out_ex_kinds(self): for kind in ALL_FOUR: where = getattr(self.test_rc, kind) @@ -79,11 +89,6 @@ class BaseRenewableCertTest(unittest.TestCase): class RenewableCertTests(BaseRenewableCertTest): # pylint: disable=too-many-public-methods """Tests for letsencrypt.renewer.*.""" - def setUp(self): - super(RenewableCertTests, self).setUp() - - def tearDown(self): - shutil.rmtree(self.tempdir) def test_initialization(self): self.assertEqual(self.test_rc.lineagename, "example.org") @@ -665,11 +670,17 @@ class RenewableCertTests(BaseRenewableCertTest): # This should fail because the renewal itself appears to fail self.assertFalse(renewer.renew(self.test_rc, 1)) + def _common_cli_args(self): + return [ + "--config-dir", self.cli_config.config_dir, + "--work-dir", self.cli_config.work_dir, + "--logs-dir", self.cli_config.logs_dir, + ] + @mock.patch("letsencrypt.renewer.notify") @mock.patch("letsencrypt.storage.RenewableCert") @mock.patch("letsencrypt.renewer.renew") def test_main(self, mock_renew, mock_rc, mock_notify): - """Test for main() function.""" from letsencrypt import renewer mock_rc_instance = mock.MagicMock() mock_rc_instance.should_autodeploy.return_value = True @@ -691,8 +702,7 @@ class RenewableCertTests(BaseRenewableCertTest): "example.com.conf"), "w") as f: f.write("cert = cert.pem\nprivkey = privkey.pem\n") f.write("chain = chain.pem\nfullchain = fullchain.pem\n") - renewer.main(self.defaults, args=[ - '--config-dir', self.cli_config.config_dir]) + renewer.main(self.defaults, cli_args=self._common_cli_args()) self.assertEqual(mock_rc.call_count, 2) self.assertEqual(mock_rc_instance.update_all_links_to.call_count, 2) self.assertEqual(mock_notify.notify.call_count, 4) @@ -705,8 +715,7 @@ class RenewableCertTests(BaseRenewableCertTest): mock_happy_instance.should_autorenew.return_value = False mock_happy_instance.latest_common_version.return_value = 10 mock_rc.return_value = mock_happy_instance - renewer.main(self.defaults, args=[ - '--config-dir', self.cli_config.config_dir]) + renewer.main(self.defaults, cli_args=self._common_cli_args()) self.assertEqual(mock_rc.call_count, 4) self.assertEqual(mock_happy_instance.update_all_links_to.call_count, 0) self.assertEqual(mock_notify.notify.call_count, 4) @@ -717,8 +726,7 @@ class RenewableCertTests(BaseRenewableCertTest): with open(os.path.join(self.cli_config.renewal_configs_dir, "bad.conf"), "w") as f: f.write("incomplete = configfile\n") - renewer.main(self.defaults, args=[ - '--config-dir', self.cli_config.config_dir]) + renewer.main(self.defaults, cli_args=self._common_cli_args()) # The errors.CertStorageError is caught inside and nothing happens. diff --git a/tests/boulder-integration.sh b/tests/boulder-integration.sh index 25db8ba6d..981c8967a 100755 --- a/tests/boulder-integration.sh +++ b/tests/boulder-integration.sh @@ -14,6 +14,12 @@ export PATH="/usr/sbin:$PATH" # /usr/sbin/nginx export GOPATH="${GOPATH:-/tmp/go}" export PATH="$GOPATH/bin:$PATH" +if [ `uname` == 'Darwin' ]; then + readlink="greadlink" +else + readlink="readlink" +fi + common() { letsencrypt_test \ --authenticator standalone \ @@ -49,7 +55,7 @@ dir="$root/conf/archive/le1.wtf" for x in cert chain fullchain privkey; do latest="$(ls -1t $dir/ | grep -e "^${x}" | head -n1)" - live="$(readlink -f "$root/conf/live/le1.wtf/${x}.pem")" + live="$($readlink -f "$root/conf/live/le1.wtf/${x}.pem")" [ "${dir}/${latest}" = "$live" ] # renewer fails this test done diff --git a/tools/dev-release.sh b/tools/dev-release.sh index d93a6d21f..cebe5001c 100755 --- a/tools/dev-release.sh +++ b/tools/dev-release.sh @@ -88,8 +88,7 @@ mkdir ../kgs kgs="../kgs/$version" pip freeze | tee $kgs pip install nose -# TODO: letsencrypt_apache fails due to symlink, c.f. #838 -nosetests letsencrypt $SUBPKGS || true +nosetests letsencrypt $SUBPKGS echo "New root: $root" echo "KGS is at $root/kgs"