From c271dc58ce4efb884666217a182d2785a9a4bf42 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Thu, 24 Dec 2015 20:55:44 -0800 Subject: [PATCH 01/20] Fix the letsencrypt-auto update script --- tests/letstest/scripts/test_leauto_upgrades.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/letstest/scripts/test_leauto_upgrades.sh b/tests/letstest/scripts/test_leauto_upgrades.sh index 70f8a2293..b7849755a 100755 --- a/tests/letstest/scripts/test_leauto_upgrades.sh +++ b/tests/letstest/scripts/test_leauto_upgrades.sh @@ -7,7 +7,9 @@ cd letsencrypt #git checkout v0.1.0 use --branch instead SAVE="$PIP_EXTRA_INDEX_URL" unset PIP_EXTRA_INDEX_URL +export PIP_INDEX_URL="https://isnot.org/pip/0.1.0/" ./letsencrypt-auto -v --debug --version +unset PIP_INDEX_URL export PIP_EXTRA_INDEX_URL="$SAVE" From dc5c0933df775a49d93e0401cfc7306127fcb733 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Thu, 24 Dec 2015 21:39:28 -0800 Subject: [PATCH 02/20] Definitely need --debug for this --- tests/letstest/scripts/test_letsencrypt_auto_venv_only.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/letstest/scripts/test_letsencrypt_auto_venv_only.sh b/tests/letstest/scripts/test_letsencrypt_auto_venv_only.sh index 476ad8bde..234e70f68 100755 --- a/tests/letstest/scripts/test_letsencrypt_auto_venv_only.sh +++ b/tests/letstest/scripts/test_letsencrypt_auto_venv_only.sh @@ -4,4 +4,4 @@ cd letsencrypt # help installs virtualenv and does nothing else -./letsencrypt-auto -v --help all +./letsencrypt-auto -v --debug --help all From e0837ad41f1e2bee0df17ccb77948abc1767f0c9 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Fri, 25 Dec 2015 02:39:05 -0800 Subject: [PATCH 03/20] [letstest] create & reuse a persistent boulder server Reduces minimum multitester.py runtime to just a bit under 10 minutes --- tests/letstest/multitester.py | 87 ++++++++++++++++++++++++----------- 1 file changed, 59 insertions(+), 28 deletions(-) diff --git a/tests/letstest/multitester.py b/tests/letstest/multitester.py index 5aca79b7a..60be10447 100644 --- a/tests/letstest/multitester.py +++ b/tests/letstest/multitester.py @@ -77,6 +77,12 @@ parser.add_argument('--saveinstances', parser.add_argument('--alt_pip', default='', help="server from which to pull candidate release packages") +parser.add_argument('--killboulder', + action='store_true', + help="do not leave a persistent boulder server running") +parser.add_argument('--boulderonly', + action='store_true', + help="only make a boulder server") cl_args = parser.parse_args() # Credential Variables @@ -292,6 +298,29 @@ def grab_letsencrypt_log(): sudo('if [ -f ./letsencrypt.log ]; then \ cat ./letsencrypt.log; else echo "[nolocallog]"; fi') +def create_client_instances(targetlist): + "Create a fleet of client instances" + instances = [] + print("Creating instances: ", end="") + for target in targetlist: + if target['virt'] == 'hvm': + machine_type = 't2.micro' + else: + machine_type = 't1.micro' + if 'userdata' in target.keys(): + userdata = target['userdata'] + else: + userdata = '' + name = 'le-%s'%target['name'] + print(name, end=" ") + instances.append(make_instance(name, + target['ami'], + KEYNAME, + machine_type=machine_type, + userdata=userdata)) + print() + return instances + #------------------------------------------------------------------------------- # SCRIPT BEGINS #------------------------------------------------------------------------------- @@ -352,30 +381,28 @@ if not sg_exists: make_security_group() time.sleep(30) +boulder_preexists = False +boulder_servers = EC2.instances.filter(Filters=[ + {'Name': 'tag:Name', 'Values': ['le-boulderserver']}, + {'Name': 'instance-state-name', 'Values': ['running']}]) + +boulder_server = next(iter(boulder_servers), None) + print("Requesting Instances...") -boulder_server = make_instance('le-boulderserver', - BOULDER_AMI, - KEYNAME, - #machine_type='t2.micro', - machine_type='t2.medium', - security_groups=['letsencrypt_test']) - -instances = [] -for target in targetlist: - if target['virt'] == 'hvm': - machine_type = 't2.micro' - else: - machine_type = 't1.micro' - if 'userdata' in target.keys(): - userdata = target['userdata'] - else: - userdata = '' - instances.append(make_instance('le-%s'%target['name'], - target['ami'], +if boulder_server: + print("Found existing boulder server:", boulder_server) + boulder_preexists = True +else: + print("Can't find a boulder server, starting one...") + boulder_server = make_instance('le-boulderserver', + BOULDER_AMI, KEYNAME, - machine_type=machine_type, - userdata=userdata)) + machine_type='t2.micro', + #machine_type='t2.medium', + security_groups=['letsencrypt_test']) +if not cl_args.boulderonly: + instances = create_client_instances(targetlist) # Configure and launch boulder server #------------------------------------------------------------------------------- @@ -383,21 +410,24 @@ print("Waiting on Boulder Server") boulder_server = block_until_instance_ready(boulder_server) print(" server %s"%boulder_server) -print("Configuring and Launching Boulder") # env.host_string defines the ssh user and host for connection env.host_string = "ubuntu@%s"%boulder_server.public_ip_address print("Boulder Server at (SSH):", env.host_string) -config_and_launch_boulder(boulder_server) -# blocking often unnecessary, but cheap EC2 VMs can get very slow -block_until_http_ready('http://%s:4000'%boulder_server.public_ip_address, - wait_time=10, - timeout=500) +if not boulder_preexists: + print("Configuring and Launching Boulder") + config_and_launch_boulder(boulder_server) + # blocking often unnecessary, but cheap EC2 VMs can get very slow + block_until_http_ready('http://%s:4000'%boulder_server.public_ip_address, + wait_time=10, timeout=500) boulder_url = "http://%s:4000/directory"%boulder_server.private_ip_address print("Boulder Server at (public ip): http://%s:4000/directory"%boulder_server.public_ip_address) print("Boulder Server at (EC2 private ip): %s"%boulder_url) +if cl_args.boulderonly: + sys.exit(0) + # Install and launch client scripts in parallel #------------------------------------------------------------------------------- print("Uploading and running test script in parallel: %s"%cl_args.test_script) @@ -480,7 +510,8 @@ results_file.close() if not cl_args.saveinstances: print('Logs in ', LOGDIR) print('Terminating EC2 Instances and Cleaning Dangling EBS Volumes') - boulder_server.terminate() + if cl_args.killboulder: + boulder_server.terminate() terminate_and_clean(instances) else: # print login information for the boxes for debugging From f5a0268f172433f5cf91c11fc7dd8c12f5f3e3f0 Mon Sep 17 00:00:00 2001 From: TheNavigat Date: Tue, 24 Nov 2015 13:25:28 +0200 Subject: [PATCH 04/20] Adding Python 2.6/2.7 note to the docs --- docs/contributing.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/contributing.rst b/docs/contributing.rst index c71aefeec..ea9a9a16c 100644 --- a/docs/contributing.rst +++ b/docs/contributing.rst @@ -365,10 +365,12 @@ are provided here mainly for the :ref:`developers ` reference. In general: * ``sudo`` is required as a suggested way of running privileged process +* `Python`_ 2.6/2.7 is required * `Augeas`_ is required for the Python bindings * ``virtualenv`` and ``pip`` are used for managing other python library dependencies +.. _Python: https://wiki.python.org/moin/BeginnersGuide/Download .. _Augeas: http://augeas.net/ .. _Virtualenv: https://virtualenv.pypa.io From 3e1bc19e0f5e6ef417328c39c027c8e61ac50e2e Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Fri, 25 Dec 2015 10:43:52 -0800 Subject: [PATCH 05/20] Optional flag for faster AMIs --- tests/letstest/multitester.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/letstest/multitester.py b/tests/letstest/multitester.py index 60be10447..dee6968c3 100644 --- a/tests/letstest/multitester.py +++ b/tests/letstest/multitester.py @@ -83,6 +83,9 @@ parser.add_argument('--killboulder', parser.add_argument('--boulderonly', action='store_true', help="only make a boulder server") +parser.add_argument('--fast', + action='store_true', + help="use larger instance types to run faster (saves about a minute, probably not worth it)") cl_args = parser.parse_args() # Credential Variables @@ -304,9 +307,10 @@ def create_client_instances(targetlist): print("Creating instances: ", end="") for target in targetlist: if target['virt'] == 'hvm': - machine_type = 't2.micro' + machine_type = 't2.medium' if cl_args.fast else 't2.micro' else: - machine_type = 't1.micro' + # 32 bit systems + machine_type = 'c1.medium' if cl_args.fast else 't1.micro' if 'userdata' in target.keys(): userdata = target['userdata'] else: From 8f984bd24f2779490bf526d47f7fbe14633fcfb1 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Fri, 1 Jan 2016 16:35:57 -0800 Subject: [PATCH 06/20] Better Nginx error handling. Raise MisconfigurationError on restart failure, so we don't attempt to continue with an authorization we know will fail. Log at debug level the config files that are about to be written out, so it's easier to debug restart failures. Fix https://github.com/letsencrypt/letsencrypt/issues/942: Error out if adding a conflicting directive. Remove unnecessarily-inserted access_log and error_log directives. These were added to make integration testing easier but are no longer needed. Incidentally this makes the plugin work with some configs where it wouldn't have worked previously. Change the semantics of add_server_directives with replace=True so only the first instance of a given directive is replaced, not all of them. This works fine with the one place in the code that calls add_server_directives with replace=True, because all of the involved directives aren't allowed to be duplicated in a given block. Make add_http_directives do inserts into the structure itself, since its needs were significantly different than the more general add_server_directives. This also allows us to narrow the scope of the `block.insert(0, directive)` hack that we inserted to work around https://trac.nginx.org/nginx/ticket/810, since it's only necessary for http blocks. --- .../letsencrypt_nginx/configurator.py | 24 ++---- letsencrypt-nginx/letsencrypt_nginx/parser.py | 81 +++++++++++++------ .../tests/configurator_test.py | 63 ++++++++------- .../letsencrypt_nginx/tests/parser_test.py | 29 ++++--- .../tests/boulder-integration.conf.sh | 6 +- 5 files changed, 115 insertions(+), 88 deletions(-) diff --git a/letsencrypt-nginx/letsencrypt_nginx/configurator.py b/letsencrypt-nginx/letsencrypt_nginx/configurator.py index aaaf43c5f..89b1145e7 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/configurator.py +++ b/letsencrypt-nginx/letsencrypt_nginx/configurator.py @@ -311,17 +311,11 @@ class NginxConfigurator(common.Plugin): """ snakeoil_cert, snakeoil_key = self._get_snakeoil_paths() ssl_block = [['listen', '{0} ssl'.format(self.config.tls_sni_01_port)], - # access and error logs necessary for integration - # testing (non-root) - ['access_log', os.path.join( - self.config.work_dir, 'access.log')], - ['error_log', os.path.join( - self.config.work_dir, 'error.log')], ['ssl_certificate', snakeoil_cert], ['ssl_certificate_key', snakeoil_key], ['include', self.parser.loc["ssl_options"]]] self.parser.add_server_directives( - vhost.filep, vhost.names, ssl_block) + vhost.filep, vhost.names, ssl_block, replace=False) vhost.ssl = True vhost.raw.extend(ssl_block) vhost.addrs.add(obj.Addr( @@ -384,7 +378,7 @@ class NginxConfigurator(common.Plugin): [['return', '301 https://$host$request_uri']] ]] self.parser.add_server_directives( - vhost.filep, vhost.names, redirect_block) + vhost.filep, vhost.names, redirect_block, replace=False) logger.info("Redirecting all traffic to ssl in %s", vhost.filep) ###################################### @@ -393,11 +387,10 @@ class NginxConfigurator(common.Plugin): def restart(self): """Restarts nginx server. - :returns: Success - :rtype: bool + :raises .errors.MisconfigurationError: If either the reload fails. """ - return nginx_restart(self.conf('ctl'), self.nginx_conf) + nginx_restart(self.conf('ctl'), self.nginx_conf) def config_test(self): # pylint: disable=no-self-use """Check the configuration of Nginx for errors. @@ -631,19 +624,16 @@ def nginx_restart(nginx_ctl, nginx_conf="/etc/nginx.conf"): if nginx_proc.returncode != 0: # Enter recovery routine... - logger.error("Nginx Restart Failed!\n%s\n%s", stdout, stderr) - return False + raise errors.MisconfigurationError( + "nginx restart failed:\n%s\n%s" % (stdout, stderr)) except (OSError, ValueError): - logger.fatal("Nginx Restart Failed - Please Check the Configuration") - sys.exit(1) + raise errors.MisconfigurationError("nginx restart failed") # 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 - def temp_install(options_ssl): """Temporary install for convenience.""" diff --git a/letsencrypt-nginx/letsencrypt_nginx/parser.py b/letsencrypt-nginx/letsencrypt_nginx/parser.py index 14db2f8b7..1d424f834 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/parser.py +++ b/letsencrypt-nginx/letsencrypt_nginx/parser.py @@ -213,6 +213,7 @@ class NginxParser(object): if ext: filename = filename + os.path.extsep + ext try: + logger.debug('Dumping to %s:\n%s', filename, nginxparser.dumps(tree)) with open(filename, 'w') as _file: nginxparser.dump(tree, _file) except IOError: @@ -252,7 +253,7 @@ class NginxParser(object): return server_names == names def add_server_directives(self, filename, names, directives, - replace=False): + replace): """Add or replace directives in the first server block with names. ..note :: If replace is True, this raises a misconfiguration error @@ -269,20 +270,27 @@ class NginxParser(object): :param bool replace: Whether to only replace existing directives """ - _do_for_subarray(self.parsed[filename], - lambda x: self._has_server_names(x, names), - lambda x: _add_directives(x, directives, replace)) + try: + _do_for_subarray(self.parsed[filename], + lambda x: self._has_server_names(x, names), + lambda x: _add_directives(x, directives, replace)) + except errors.MisconfigurationError as err: + raise errors.MisconfigurationError("Problem in %s: %s" % (filename, err.message)) def add_http_directives(self, filename, directives): """Adds directives to the first encountered HTTP block in filename. + We insert new directives at the top of the block to work around + https://trac.nginx.org/nginx/ticket/810: If the first server block + doesn't enable OCSP stapling, stapling is broken for all blocks. + :param str filename: The absolute filename of the config file :param list directives: The directives to add """ _do_for_subarray(self.parsed[filename], lambda x: x[0] == ['http'], - lambda x: _add_directives(x[1], [directives], False)) + lambda x: x[1].insert(0, directives)) def get_all_certs_keys(self): """Gets all certs and keys in the nginx config. @@ -467,9 +475,14 @@ def _parse_server(server): return parsed_server -def _add_directives(block, directives, replace=False): - """Adds or replaces directives in a block. If the directive doesn't exist in - the entry already, raises a misconfiguration error. +def _add_directives(block, directives, replace): + """Adds or replaces directives in a config block. + + When replace=False, it's an error to try and add a directive that already + exists in the config block with a conflicting value. + + When replace=True, a directive with the same name MUST already exist in the + config block, and the first instance will be replaced. ..todo :: Find directives that are in included files. @@ -478,21 +491,39 @@ def _add_directives(block, directives, replace=False): """ 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 - # Only add directive if its not already in the block - if directive not in block: - block.insert(0, directive) - else: - changed = False - if len(directive) == 0: - continue - for index, line in enumerate(block): - if len(line) > 0 and line[0] == directive[0]: - block[index] = directive - changed = True - if not changed: + _add_directive(block, directive, replace) + +repeatable_directives = set(['server_name', 'listen', 'include']) + +def _add_directive(block, directive, replace): + """Adds or replaces a single directive in a config block. + + See _add_directives for more documentation. + + """ + location = -1 + # Find the index of a config line where the name of the directive matches + # the name of the directive we want to add. + for index, line in enumerate(block): + if len(line) > 0 and line[0] == directive[0]: + location = index + break + if replace: + if location == -1: + raise errors.MisconfigurationError( + 'expected directive for %s in the Nginx ' + 'config but did not find it.' % directive[0]) + block[location] = directive + else: + # Append directive. Fail if the name is not a repeatable directive name, + # and there is already a copy of that directive with a different value + # in the config file. + if location != -1 and directive[0].__str__() not in repeatable_directives: + if block[location][1] == directive[1]: + pass + else: raise errors.MisconfigurationError( - 'Let\'s Encrypt expected directive for %s in the Nginx ' - 'config but did not find it.' % directive[0]) + 'tried to insert directive "%s" but found conflicting "%s".' % ( + directive, block[location])) + else: + block.append(directive) diff --git a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py b/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py index 56ad5110c..f6a4f7eee 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py +++ b/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py @@ -65,16 +65,19 @@ class NginxConfiguratorTest(util.NginxTest): filep = self.config.parser.abs_path('sites-enabled/example.com') self.config.parser.add_server_directives( filep, set(['.example.com', 'example.*']), - [['listen', '5001 ssl']]) + [['listen', '5001 ssl']], + replace=False) self.config.save() # pylint: disable=protected-access parsed = self.config.parser._parse_files(filep, override=True) - self.assertEqual([[['server'], [['listen', '5001 ssl'], + self.assertEqual([[['server'], [ ['listen', '69.50.225.155:9000'], ['listen', '127.0.0.1'], ['server_name', '.example.com'], - ['server_name', 'example.*']]]], + ['server_name', 'example.*'], + ['listen', '5001 ssl'] + ]]], parsed[0]) def test_choose_vhost(self): @@ -154,38 +157,36 @@ class NginxConfiguratorTest(util.NginxTest): parsed_server_conf = util.filter_comments(self.config.parser.parsed[server_conf]) parsed_nginx_conf = util.filter_comments(self.config.parser.parsed[nginx_conf]) - access_log = os.path.join(self.work_dir, "access.log") - error_log = os.path.join(self.work_dir, "error.log") self.assertEqual([[['server'], - [['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.*']]]], + ['server_name', 'example.*'], + + ['listen', '5001 ssl'], + ['ssl_certificate', 'example/fullchain.pem'], + ['ssl_certificate_key', 'example/key.pem'], + ['include', self.config.parser.loc["ssl_options"]] + ]]], parsed_example_conf) self.assertEqual([['server_name', 'somename alias another.alias']], parsed_server_conf) - 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)) + self.assertTrue(util.contains_at_depth( + parsed_nginx_conf, + [['server'], + [ + ['listen', '8000'], + ['listen', 'somename:8080'], + ['include', 'server.conf'], + [['location', '/'], + [['root', 'html'], + ['index', 'index.html index.htm']]], + ['listen', '5001 ssl'], + ['ssl_certificate', '/etc/nginx/fullchain.pem'], + ['ssl_certificate_key', '/etc/nginx/key.pem'], + ['include', self.config.parser.loc["ssl_options"]]]], + 2)) def test_get_all_certs_keys(self): nginx_conf = self.config.parser.abs_path('nginx.conf') @@ -297,19 +298,19 @@ class NginxConfiguratorTest(util.NginxTest): mocked = mock_popen() mocked.communicate.return_value = ('', '') mocked.returncode = 0 - self.assertTrue(self.config.restart()) + self.config.restart() @mock.patch("letsencrypt_nginx.configurator.subprocess.Popen") def test_nginx_restart_fail(self, mock_popen): mocked = mock_popen() mocked.communicate.return_value = ('', '') mocked.returncode = 1 - self.assertFalse(self.config.restart()) + self.assertRaises(errors.MisconfigurationError, self.config.restart) @mock.patch("letsencrypt_nginx.configurator.subprocess.Popen") def test_no_nginx_start(self, mock_popen): mock_popen.side_effect = OSError("Can't find program") - self.assertRaises(SystemExit, self.config.restart) + self.assertRaises(errors.MisconfigurationError, self.config.restart) @mock.patch("letsencrypt_nginx.configurator.subprocess.Popen") def test_config_test(self, mock_popen): diff --git a/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py b/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py index 2d6156429..6559a5df6 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py +++ b/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py @@ -127,7 +127,8 @@ class NginxParserTest(util.NginxTest): set(['localhost', r'~^(www\.)?(example|bar)\.']), [['foo', 'bar'], ['ssl_certificate', - '/etc/ssl/cert.pem']]) + '/etc/ssl/cert.pem']], + replace=False) 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))) @@ -136,12 +137,15 @@ class NginxParserTest(util.NginxTest): names = set(['alias', 'another.alias', 'somename']) nparser.add_server_directives(server_conf, names, [['foo', 'bar'], ['ssl_certificate', - '/etc/ssl/cert2.pem']]) - nparser.add_server_directives(server_conf, names, [['foo', 'bar']]) + '/etc/ssl/cert2.pem']], + replace=False) + nparser.add_server_directives(server_conf, names, [['foo', 'bar']], + replace=False) self.assertEqual(nparser.parsed[server_conf], - [['ssl_certificate', '/etc/ssl/cert2.pem'], + [['server_name', 'somename alias another.alias'], ['foo', 'bar'], - ['server_name', 'somename alias another.alias']]) + ['ssl_certificate', '/etc/ssl/cert2.pem'] + ]) def test_add_http_directives(self): nparser = parser.NginxParser(self.config_path, self.ssl_options) @@ -165,17 +169,19 @@ class NginxParserTest(util.NginxTest): target = set(['.example.com', 'example.*']) filep = nparser.abs_path('sites-enabled/example.com') nparser.add_server_directives( - filep, target, [['server_name', 'foo bar']], True) + filep, target, [['server_name', 'foobar.com']], replace=True) self.assertEqual( nparser.parsed[filep], [[['server'], [['listen', '69.50.225.155:9000'], ['listen', '127.0.0.1'], - ['server_name', 'foo bar'], - ['server_name', 'foo bar']]]]) + ['server_name', 'foobar.com'], + ['server_name', 'example.*'], + ]]]) self.assertRaises(errors.MisconfigurationError, nparser.add_server_directives, - filep, set(['foo', 'bar']), - [['ssl_certificate', 'cert.pem']], True) + filep, set(['foobar.com', 'example.*']), + [['ssl_certificate', 'cert.pem']], + replace=True) def test_get_best_match(self): target_name = 'www.eff.org' @@ -217,7 +223,8 @@ class NginxParserTest(util.NginxTest): set(['.example.com', 'example.*']), [['ssl_certificate', 'foo.pem'], ['ssl_certificate_key', 'bar.key'], - ['listen', '443 ssl']]) + ['listen', '443 ssl']], + replace=False) c_k = nparser.get_all_certs_keys() self.assertEqual(set([('foo.pem', 'bar.key', filep)]), c_k) diff --git a/letsencrypt-nginx/tests/boulder-integration.conf.sh b/letsencrypt-nginx/tests/boulder-integration.conf.sh index 12610d895..d77669a76 100755 --- a/letsencrypt-nginx/tests/boulder-integration.conf.sh +++ b/letsencrypt-nginx/tests/boulder-integration.conf.sh @@ -20,13 +20,14 @@ events { } http { - # Set an array of temp and cache file options that will otherwise default to + # Set an array of temp, cache and log file options that will otherwise default to # restricted locations accessible only to root. client_body_temp_path $root/client_body; fastcgi_temp_path $root/fastcgi_temp; proxy_temp_path $root/proxy_temp; #scgi_temp_path $root/scgi_temp; #uwsgi_temp_path $root/uwsgi_temp; + access_log $root/error.log; # This should be turned off in a Virtualbox VM, as it can cause some # interesting issues with data corruption in delivered files. @@ -54,9 +55,6 @@ http { root $root/webroot; - access_log $root/access.log; - error_log $root/error.log; - location / { # First attempt to serve request as file, then as directory, then fall # back to index.html. From dc3a2da9b11b26be54b035ac1520128e8efbfe22 Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Sun, 3 Jan 2016 10:49:50 -0500 Subject: [PATCH 07/20] Fixed a typo in a comment --- acme/setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acme/setup.py b/acme/setup.py index ba2c88394..7314152cd 100644 --- a/acme/setup.py +++ b/acme/setup.py @@ -31,7 +31,7 @@ else: install_requires.append('mock') if sys.version_info < (2, 7, 9): - # For secure SSL connexion with Python 2.7 (InsecurePlatformWarning) + # For secure SSL connection with Python 2.7 (InsecurePlatformWarning) install_requires.append('ndg-httpsclient') install_requires.append('pyasn1') From 0454031cce4b88fef44e3e129e879a35b49c2314 Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Sun, 3 Jan 2016 14:37:08 -0500 Subject: [PATCH 08/20] Fixed a pair of typos in docstrings --- acme/acme/jose/json_util.py | 2 +- acme/acme/jose/util.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/acme/acme/jose/json_util.py b/acme/acme/jose/json_util.py index 7b95e3fce..977a06622 100644 --- a/acme/acme/jose/json_util.py +++ b/acme/acme/jose/json_util.py @@ -226,7 +226,7 @@ class JSONObjectWithFields(util.ImmutableMap, interfaces.JSONDeSerializable): :param str name: Name of the field to be encoded. - :raises erors.SerializationError: if field cannot be serialized + :raises errors.SerializationError: if field cannot be serialized :raises errors.Error: if field could not be found """ diff --git a/acme/acme/jose/util.py b/acme/acme/jose/util.py index ab3606efc..600077b20 100644 --- a/acme/acme/jose/util.py +++ b/acme/acme/jose/util.py @@ -130,7 +130,7 @@ class ImmutableMap(collections.Mapping, collections.Hashable): """Immutable key to value mapping with attribute access.""" __slots__ = () - """Must be overriden in subclasses.""" + """Must be overridden in subclasses.""" def __init__(self, **kwargs): if set(kwargs) != set(self.__slots__): From e722a381978afe3faaeb265eefbfc45b9a83e35f Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Mon, 4 Jan 2016 11:18:13 -0800 Subject: [PATCH 09/20] Clarify parser check for duplicate values. --- letsencrypt-nginx/letsencrypt_nginx/parser.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/letsencrypt-nginx/letsencrypt_nginx/parser.py b/letsencrypt-nginx/letsencrypt_nginx/parser.py index 1d424f834..c60d0102a 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/parser.py +++ b/letsencrypt-nginx/letsencrypt_nginx/parser.py @@ -518,8 +518,12 @@ def _add_directive(block, directive, replace): # Append directive. Fail if the name is not a repeatable directive name, # and there is already a copy of that directive with a different value # in the config file. - if location != -1 and directive[0].__str__() not in repeatable_directives: - if block[location][1] == directive[1]: + directive_name = directive[0] + directive_value = directive[1] + if location != -1 and directive_name.__str__() not in repeatable_directives: + if block[location][1] == directive_value: + # There's a conflict, but the existing value matches the one we + # want to insert, so it's fine. pass else: raise errors.MisconfigurationError( From f7fad9a4affc399a4c6953e330d59b20d44d9d2e Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Wed, 23 Dec 2015 23:25:41 -0800 Subject: [PATCH 10/20] Show error detail to the user. Previously this code would show the user a hardcoded message based on the error type, but it's much more useful to show the error detail field, which often helps debug the problem. --- letsencrypt/auth_handler.py | 7 +------ letsencrypt/tests/auth_handler_test.py | 2 +- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/letsencrypt/auth_handler.py b/letsencrypt/auth_handler.py index 027c11158..37b71775d 100644 --- a/letsencrypt/auth_handler.py +++ b/letsencrypt/auth_handler.py @@ -543,13 +543,8 @@ def _generate_failed_chall_msg(failed_achalls): msg = [ "The following '{0}' errors were reported by the server:".format(typ)] - problems = dict() for achall in failed_achalls: - problems.setdefault(achall.error.description, set()).add(achall.domain) - for problem in problems: - msg.append("\n\nDomains: ") - msg.append(", ".join(sorted(problems[problem]))) - msg.append("\nError: {0}".format(problem)) + msg.append("\n\nDomain: %s\nDetail: %s\n" % (achall.domain, achall.error.detail)) if typ in _ERROR_HELP: msg.append("\n\n") diff --git a/letsencrypt/tests/auth_handler_test.py b/letsencrypt/tests/auth_handler_test.py index be19ab036..199954278 100644 --- a/letsencrypt/tests/auth_handler_test.py +++ b/letsencrypt/tests/auth_handler_test.py @@ -467,7 +467,7 @@ class ReportFailedChallsTest(unittest.TestCase): auth_handler._report_failed_challs([self.http01, self.tls_sni_same]) call_list = mock_zope().add_message.call_args_list self.assertTrue(len(call_list) == 1) - self.assertTrue("Domains: example.com\n" in call_list[0][0][0]) + self.assertTrue("Domain: example.com\nDetail: detail" in call_list[0][0][0]) @mock.patch("letsencrypt.auth_handler.zope.component.getUtility") def test_different_errors_and_domains(self, mock_zope): From 4adc2826318168f8318a7784a5b190599a03a045 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Tue, 5 Jan 2016 11:02:03 -0800 Subject: [PATCH 11/20] Fix formatting of error messages. --- letsencrypt/auth_handler.py | 4 ++-- letsencrypt/tests/auth_handler_test.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/letsencrypt/auth_handler.py b/letsencrypt/auth_handler.py index 37b71775d..e77f83248 100644 --- a/letsencrypt/auth_handler.py +++ b/letsencrypt/auth_handler.py @@ -541,10 +541,10 @@ def _generate_failed_chall_msg(failed_achalls): """ typ = failed_achalls[0].error.typ msg = [ - "The following '{0}' errors were reported by the server:".format(typ)] + "The following errors were reported by the server:".format(typ)] for achall in failed_achalls: - msg.append("\n\nDomain: %s\nDetail: %s\n" % (achall.domain, achall.error.detail)) + msg.append("\n\nDomain: %s\nType: %s\nDetail: %s" % (achall.domain, achall.error.typ, achall.error.detail)) if typ in _ERROR_HELP: msg.append("\n\n") diff --git a/letsencrypt/tests/auth_handler_test.py b/letsencrypt/tests/auth_handler_test.py index 199954278..5b4c2bfc7 100644 --- a/letsencrypt/tests/auth_handler_test.py +++ b/letsencrypt/tests/auth_handler_test.py @@ -467,7 +467,7 @@ class ReportFailedChallsTest(unittest.TestCase): auth_handler._report_failed_challs([self.http01, self.tls_sni_same]) call_list = mock_zope().add_message.call_args_list self.assertTrue(len(call_list) == 1) - self.assertTrue("Domain: example.com\nDetail: detail" in call_list[0][0][0]) + self.assertTrue("Domain: example.com\nType: tls\nDetail: detail" in call_list[0][0][0]) @mock.patch("letsencrypt.auth_handler.zope.component.getUtility") def test_different_errors_and_domains(self, mock_zope): From cf74446b58fa1229c6b2ad6bf5465f3b9ebe41ef Mon Sep 17 00:00:00 2001 From: Reinaldo de Souza Jr Date: Mon, 4 Jan 2016 23:04:18 -0500 Subject: [PATCH 12/20] Add test for redirect enhancement --- .../letsencrypt_nginx/tests/configurator_test.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py b/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py index 56ad5110c..5e237b04a 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py +++ b/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py @@ -330,6 +330,17 @@ class NginxConfiguratorTest(util.NginxTest): OpenSSL.crypto.load_privatekey( OpenSSL.crypto.FILETYPE_PEM, key_file.read()) + def test_redirect_enhance(self): + expected = [ + ['if', '($scheme != "https")'], + [['return', '301 https://$host$request_uri']] + ] + + example_conf = self.config.parser.abs_path('sites-enabled/example.com') + self.config.enhance("www.example.com", "redirect") + + generated_conf = self.config.parser.parsed[example_conf] + self.assertTrue(util.contains_at_depth(generated_conf, expected, 2)) if __name__ == "__main__": unittest.main() # pragma: no cover From adcb7934aed5780ba96e9f5c861c467bff8f616d Mon Sep 17 00:00:00 2001 From: Reinaldo de Souza Jr Date: Mon, 4 Jan 2016 22:48:01 -0500 Subject: [PATCH 13/20] Improve choose_vhost() test by verifying the output file --- .../tests/configurator_test.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py b/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py index 5e237b04a..9a962a55f 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py +++ b/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py @@ -91,12 +91,26 @@ class NginxConfiguratorTest(util.NginxTest): 'test.www.example.com': foo_conf, 'abc.www.foo.com': foo_conf, 'www.bar.co.uk': localhost_conf} + + conf_path = {'localhost': "etc_nginx/nginx.conf", + 'alias': "etc_nginx/nginx.conf", + 'example.com': "etc_nginx/sites-enabled/example.com", + 'example.com.uk.test': "etc_nginx/sites-enabled/example.com", + 'www.example.com': "etc_nginx/sites-enabled/example.com", + 'test.www.example.com': "etc_nginx/foo.conf", + 'abc.www.foo.com': "etc_nginx/foo.conf", + 'www.bar.co.uk': "etc_nginx/nginx.conf"} + bad_results = ['www.foo.com', 'example', 't.www.bar.co', '69.255.225.155'] for name in results: - self.assertEqual(results[name], - self.config.choose_vhost(name).names) + vhost = self.config.choose_vhost(name) + path = os.path.relpath(vhost.filep, self.temp_dir) + + self.assertEqual(results[name], vhost.names) + self.assertEqual(conf_path[name], path) + for name in bad_results: self.assertEqual(set([name]), self.config.choose_vhost(name).names) From 20829e05ed6a665a224fdef43e4f91cd5c002199 Mon Sep 17 00:00:00 2001 From: Reinaldo de Souza Jr Date: Tue, 5 Jan 2016 15:15:29 -0500 Subject: [PATCH 14/20] Add missing test condition for prepare() --- .../tests/configurator_test.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py b/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py index 9a962a55f..3ad0b834f 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py +++ b/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py @@ -40,6 +40,23 @@ class NginxConfiguratorTest(util.NginxTest): self.assertEquals((1, 6, 2), self.config.version) self.assertEquals(5, len(self.config.parser.parsed)) + @mock.patch("letsencrypt_nginx.configurator.le_util.exe_exists") + @mock.patch("letsencrypt_nginx.configurator.subprocess.Popen") + def test_prepare_initializes_version(self, mock_popen, mock_exe_exists): + mock_popen().communicate.return_value = ( + "", "\n".join(["nginx version: nginx/1.6.2", + "built by clang 6.0 (clang-600.0.56)" + " (based on LLVM 3.5svn)", + "TLS SNI support enabled", + "configure arguments: --prefix=/usr/local/Cellar/" + "nginx/1.6.2 --with-http_ssl_module"])) + + mock_exe_exists.return_value = True + + self.config.version = None + self.config.prepare() + self.assertEquals((1, 6, 2), self.config.version) + @mock.patch("letsencrypt_nginx.configurator.socket.gethostbyaddr") def test_get_all_names(self, mock_gethostbyaddr): mock_gethostbyaddr.return_value = ('155.225.50.69.nephoscale.net', [], []) From 0a04efe40c55029ba66a5c8b69f0d3f646286aab Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Tue, 5 Jan 2016 17:23:32 -0800 Subject: [PATCH 15/20] Fix lint error and remove extra format() --- letsencrypt/auth_handler.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/letsencrypt/auth_handler.py b/letsencrypt/auth_handler.py index e77f83248..c63d8c8d4 100644 --- a/letsencrypt/auth_handler.py +++ b/letsencrypt/auth_handler.py @@ -540,11 +540,11 @@ def _generate_failed_chall_msg(failed_achalls): """ typ = failed_achalls[0].error.typ - msg = [ - "The following errors were reported by the server:".format(typ)] + msg = ["The following errors were reported by the server:"] for achall in failed_achalls: - msg.append("\n\nDomain: %s\nType: %s\nDetail: %s" % (achall.domain, achall.error.typ, achall.error.detail)) + msg.append("\n\nDomain: %s\nType: %s\nDetail: %s" % ( + achall.domain, achall.error.typ, achall.error.detail)) if typ in _ERROR_HELP: msg.append("\n\n") From 6548f343bfe8b9fc3e00e1ef2abe356ff4c3c13c Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Thu, 7 Jan 2016 21:20:14 +0000 Subject: [PATCH 16/20] Add invalidEmail error type to acme Related to: - #1923 - https://github.com/ietf-wg-acme/acme/pull/65 --- acme/acme/messages.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/acme/acme/messages.py b/acme/acme/messages.py index 0b73864ec..3b5739da1 100644 --- a/acme/acme/messages.py +++ b/acme/acme/messages.py @@ -25,6 +25,8 @@ class Error(jose.JSONObjectWithFields, errors.Error): ('connection', 'The server could not connect to the client to ' 'verify the domain'), ('dnssec', 'The server could not validate a DNSSEC signed domain'), + ('invalidEmail', + 'The provided email for a registration was invalid'), ('malformed', 'The request message was malformed'), ('rateLimited', 'There were too many requests of a given type'), ('serverInternal', 'The server experienced an internal error'), From 6fedd22dc8c2db00665fa4343779f4aa801d571e Mon Sep 17 00:00:00 2001 From: Noah Swartz Date: Fri, 8 Jan 2016 02:00:47 -0800 Subject: [PATCH 17/20] don't iDisplay if logging --- letsencrypt-apache/letsencrypt_apache/configurator.py | 2 +- letsencrypt/reverter.py | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 1baa06128..3a0b90fc0 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -1271,7 +1271,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): """ self.config_test() - logger.debug(self.reverter.view_config_changes()) + logger.debug(self.reverter.view_config_changes(logging=True)) self._reload() def _reload(self): diff --git a/letsencrypt/reverter.py b/letsencrypt/reverter.py index d5114ae71..e2bc28cb9 100644 --- a/letsencrypt/reverter.py +++ b/letsencrypt/reverter.py @@ -94,7 +94,7 @@ class Reverter(object): "Unable to load checkpoint during rollback") rollback -= 1 - def view_config_changes(self): + def view_config_changes(self, logging=False): """Displays all saved checkpoints. All checkpoints are printed by @@ -144,6 +144,8 @@ class Reverter(object): output.append(os.linesep) + if logging: + return os.linesep.join(output) zope.component.getUtility(interfaces.IDisplay).notification( os.linesep.join(output), display_util.HEIGHT) From 287be6be8eeac0e4da55c6088bda74ec3d48a8d7 Mon Sep 17 00:00:00 2001 From: Noah Swartz Date: Fri, 8 Jan 2016 02:47:59 -0800 Subject: [PATCH 18/20] fix linting issue --- letsencrypt-apache/letsencrypt_apache/configurator.py | 2 +- letsencrypt/reverter.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index d936309ae..2a9fb0250 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -1302,7 +1302,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): """ self.config_test() - logger.debug(self.reverter.view_config_changes(logging=True)) + logger.debug(self.reverter.view_config_changes(for_logging=True)) self._reload() def _reload(self): diff --git a/letsencrypt/reverter.py b/letsencrypt/reverter.py index e2bc28cb9..863074374 100644 --- a/letsencrypt/reverter.py +++ b/letsencrypt/reverter.py @@ -94,7 +94,7 @@ class Reverter(object): "Unable to load checkpoint during rollback") rollback -= 1 - def view_config_changes(self, logging=False): + def view_config_changes(self, for_logging=False): """Displays all saved checkpoints. All checkpoints are printed by @@ -144,7 +144,7 @@ class Reverter(object): output.append(os.linesep) - if logging: + if for_logging: return os.linesep.join(output) zope.component.getUtility(interfaces.IDisplay).notification( os.linesep.join(output), display_util.HEIGHT) From 2c8d042974dbbf78a03a95dad187ed39c867664a Mon Sep 17 00:00:00 2001 From: Noah Swartz Date: Fri, 8 Jan 2016 04:58:17 -0800 Subject: [PATCH 19/20] added in tls_sni logging from #2002 --- letsencrypt-apache/letsencrypt_apache/tls_sni_01.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/letsencrypt-apache/letsencrypt_apache/tls_sni_01.py b/letsencrypt-apache/letsencrypt_apache/tls_sni_01.py index 2049eb574..ca7985f35 100644 --- a/letsencrypt-apache/letsencrypt_apache/tls_sni_01.py +++ b/letsencrypt-apache/letsencrypt_apache/tls_sni_01.py @@ -1,12 +1,14 @@ """A class that performs TLS-SNI-01 challenges for Apache""" import os +import logging from letsencrypt.plugins import common from letsencrypt_apache import obj from letsencrypt_apache import parser +logger = logging.getLogger(__name__) class ApacheTlsSni01(common.TLSSNI01): """Class that performs TLS-SNI-01 challenges within the Apache configurator @@ -104,6 +106,7 @@ class ApacheTlsSni01(common.TLSSNI01): self.configurator.reverter.register_file_creation( True, self.challenge_conf) + logger.debug("writing a config file with text: %s", config_text) with open(self.challenge_conf, "w") as new_conf: new_conf.write(config_text) From b039c884d8ffa16e2b3ac9663cca634bfd2f8bc3 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 8 Jan 2016 14:09:44 -0500 Subject: [PATCH 20/20] Don't use cryptography version 1.2 --- acme/setup.py | 2 +- setup.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/acme/setup.py b/acme/setup.py index 7314152cd..54c4d82d9 100644 --- a/acme/setup.py +++ b/acme/setup.py @@ -9,7 +9,7 @@ version = '0.2.0.dev0' install_requires = [ # load_pem_private/public_key (>=0.6) # rsa_recover_prime_factors (>=0.8) - 'cryptography>=0.8', + 'cryptography>=0.8,<1.2', # Connection.set_tlsext_host_name (>=0.13), X509Req.get_extensions (>=0.15) 'PyOpenSSL>=0.15', 'pyrfc3339', diff --git a/setup.py b/setup.py index f95f672ff..4005b0973 100644 --- a/setup.py +++ b/setup.py @@ -33,7 +33,7 @@ version = meta['version'] install_requires = [ 'acme=={0}'.format(version), 'configobj', - 'cryptography>=0.7', # load_pem_x509_certificate + 'cryptography>=0.7,<1.2', # load_pem_x509_certificate 'parsedatetime', 'psutil>=2.1.0', # net_connections introduced in 2.1.0 'PyOpenSSL',