From 86ab9a2afc6de4e51301ee3dc278a17e7ea06413 Mon Sep 17 00:00:00 2001 From: Ola Bini Date: Thu, 21 Jan 2016 19:08:38 -0500 Subject: [PATCH 01/51] Make ssl includes be an array of directives instead of an include of a file. This will be the first step to filter out the things that would conflict with earlier declarations --- .../letsencrypt_nginx/configurator.py | 3 +-- letsencrypt-nginx/letsencrypt_nginx/parser.py | 18 +++++++++++++++++- .../tests/configurator_test.py | 13 +++++++------ .../letsencrypt_nginx/tests/parser_test.py | 11 +++++++++++ .../letsencrypt_nginx/tls_sni_01.py | 4 ++-- 5 files changed, 38 insertions(+), 11 deletions(-) diff --git a/letsencrypt-nginx/letsencrypt_nginx/configurator.py b/letsencrypt-nginx/letsencrypt_nginx/configurator.py index efa7e08b4..918ba53c2 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/configurator.py +++ b/letsencrypt-nginx/letsencrypt_nginx/configurator.py @@ -332,8 +332,7 @@ class NginxConfigurator(common.Plugin): snakeoil_cert, snakeoil_key = self._get_snakeoil_paths() ssl_block = [['listen', '{0} ssl'.format(self.config.tls_sni_01_port)], ['ssl_certificate', snakeoil_cert], - ['ssl_certificate_key', snakeoil_key], - ['include', self.parser.loc["ssl_options"]]] + ['ssl_certificate_key', snakeoil_key]] + self.parser.loc["ssl_options"] self.parser.add_server_directives( vhost.filep, vhost.names, ssl_block, replace=False) vhost.ssl = True diff --git a/letsencrypt-nginx/letsencrypt_nginx/parser.py b/letsencrypt-nginx/letsencrypt_nginx/parser.py index 3b1dd049e..935bf40dd 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/parser.py +++ b/letsencrypt-nginx/letsencrypt_nginx/parser.py @@ -169,6 +169,18 @@ class NginxParser(object): logger.debug("Could not parse file: %s", item) return trees + def _parse_ssl_options(self, ssl_options): + if ssl_options is not None: + try: + with open(ssl_options) as _file: + return nginxparser.load(_file) + except IOError: + logger.debug("Could not open file: %s", item) + except pyparsing.ParseException: + logger.debug("Could not parse file: %s", item) + else: + return [] + def _set_locations(self, ssl_options): """Set default location for directives. @@ -188,7 +200,7 @@ class NginxParser(object): name = default return {"root": root, "default": default, "listen": listen, - "name": name, "ssl_options": ssl_options} + "name": name, "ssl_options": self._parse_ssl_options(ssl_options)} def _find_config_root(self): """Find the Nginx Configuration Root file.""" @@ -503,6 +515,10 @@ def _add_directive(block, directive, replace): See _add_directives for more documentation. """ + if directive[0] == '#': + block.append(directive) + return + 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. diff --git a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py b/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py index 4fce33079..f35dcdfc0 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py +++ b/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py @@ -216,9 +216,9 @@ class NginxConfiguratorTest(util.NginxTest): ['listen', '5001 ssl'], ['ssl_certificate', 'example/fullchain.pem'], - ['ssl_certificate_key', 'example/key.pem'], - ['include', self.config.parser.loc["ssl_options"]] - ]]], + ['ssl_certificate_key', 'example/key.pem']] + + util.filter_comments(self.config.parser.loc["ssl_options"]) + ]], parsed_example_conf) self.assertEqual([['server_name', 'somename alias another.alias']], parsed_server_conf) @@ -234,8 +234,9 @@ class NginxConfiguratorTest(util.NginxTest): ['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"]]]], + ['ssl_certificate_key', '/etc/nginx/key.pem']]+ + util.filter_comments(self.config.parser.loc["ssl_options"]) + ], 2)) def test_get_all_certs_keys(self): @@ -394,6 +395,6 @@ class NginxConfiguratorTest(util.NginxTest): 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 diff --git a/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py b/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py index b64f1dee3..1190a2326 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py +++ b/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py @@ -249,5 +249,16 @@ class NginxParserTest(util.NginxTest): ]) self.assertTrue(server['ssl']) + def test_ssl_options_should_be_parsed_ssl_directives(self): + nparser = parser.NginxParser(self.config_path, self.ssl_options) + self.assertEqual(nparser.loc["ssl_options"], + [['ssl_session_cache', 'shared:SSL:1m'], + ['ssl_session_timeout', '1440m'], + ['ssl_protocols', 'TLSv1 TLSv1.1 TLSv1.2'], + ['ssl_prefer_server_ciphers', 'on'], + ['#', ' Using list of ciphers from "Bulletproof SSL and TLS"'], + ['ssl_ciphers', '"ECDHE-ECDSA-AES128-GCM-SHA256 ECDHE-ECDSA-AES256-GCM-SHA384 ECDHE-ECDSA-AES128-SHA ECDHE-ECDSA-AES256-SHA ECDHE-ECDSA-AES128-SHA256 ECDHE-ECDSA-AES256-SHA384 ECDHE-RSA-AES128-GCM-SHA256 ECDHE-RSA-AES256-GCM-SHA384 ECDHE-RSA-AES128-SHA ECDHE-RSA-AES128-SHA256 ECDHE-RSA-AES256-SHA384 DHE-RSA-AES128-GCM-SHA256 DHE-RSA-AES256-GCM-SHA384 DHE-RSA-AES128-SHA DHE-RSA-AES256-SHA DHE-RSA-AES128-SHA256 DHE-RSA-AES256-SHA256 EDH-RSA-DES-CBC3-SHA"'] + ]) + if __name__ == "__main__": unittest.main() # pragma: no cover diff --git a/letsencrypt-nginx/letsencrypt_nginx/tls_sni_01.py b/letsencrypt-nginx/letsencrypt_nginx/tls_sni_01.py index e59281c4c..5afc950ad 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tls_sni_01.py +++ b/letsencrypt-nginx/letsencrypt_nginx/tls_sni_01.py @@ -146,7 +146,6 @@ class NginxTlsSni01(common.TLSSNI01): block.extend([['server_name', achall.response(achall.account_key).z_domain], - ['include', self.configurator.parser.loc["ssl_options"]], # access and error logs necessary for # integration testing (non-root) ['access_log', os.path.join( @@ -155,6 +154,7 @@ class NginxTlsSni01(common.TLSSNI01): self.configurator.config.work_dir, 'error.log')], ['ssl_certificate', self.get_cert_path(achall)], ['ssl_certificate_key', self.get_key_path(achall)], - [['location', '/'], [['root', document_root]]]]) + [['location', '/'], [['root', document_root]]]] + + self.configurator.parser.loc["ssl_options"]) return [['server'], block] From 25674b4d559a630e15ac2941e410bc4ca04586e6 Mon Sep 17 00:00:00 2001 From: Ola Bini Date: Fri, 22 Jan 2016 12:12:29 -0500 Subject: [PATCH 02/51] Change name of session cache in order to minimize risk of conflict --- letsencrypt-nginx/letsencrypt_nginx/options-ssl-nginx.conf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt-nginx/letsencrypt_nginx/options-ssl-nginx.conf b/letsencrypt-nginx/letsencrypt_nginx/options-ssl-nginx.conf index f0081c1fc..3faab8818 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/options-ssl-nginx.conf +++ b/letsencrypt-nginx/letsencrypt_nginx/options-ssl-nginx.conf @@ -1,4 +1,4 @@ -ssl_session_cache shared:SSL:1m; +ssl_session_cache shared:le_nginx_SSL:1m; ssl_session_timeout 1440m; ssl_protocols TLSv1 TLSv1.1 TLSv1.2; From d53da41f007e7f8750a528554bf4000b3d7c6a03 Mon Sep 17 00:00:00 2001 From: Ola Bini Date: Fri, 22 Jan 2016 15:03:51 -0500 Subject: [PATCH 03/51] Stupid mistake, forgot to change the test --- letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py b/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py index 1190a2326..3fc6a214e 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py +++ b/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py @@ -252,7 +252,7 @@ class NginxParserTest(util.NginxTest): def test_ssl_options_should_be_parsed_ssl_directives(self): nparser = parser.NginxParser(self.config_path, self.ssl_options) self.assertEqual(nparser.loc["ssl_options"], - [['ssl_session_cache', 'shared:SSL:1m'], + [['ssl_session_cache', 'shared:le_nginx_SSL:1m'], ['ssl_session_timeout', '1440m'], ['ssl_protocols', 'TLSv1 TLSv1.1 TLSv1.2'], ['ssl_prefer_server_ciphers', 'on'], From 593e220353f720282e112809fddd954e575c6779 Mon Sep 17 00:00:00 2001 From: Ola Bini Date: Fri, 22 Jan 2016 16:28:05 -0500 Subject: [PATCH 04/51] A final small fix, hopefully --- letsencrypt-nginx/letsencrypt_nginx/parser.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/letsencrypt-nginx/letsencrypt_nginx/parser.py b/letsencrypt-nginx/letsencrypt_nginx/parser.py index 935bf40dd..bd9bebe08 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/parser.py +++ b/letsencrypt-nginx/letsencrypt_nginx/parser.py @@ -175,11 +175,10 @@ class NginxParser(object): with open(ssl_options) as _file: return nginxparser.load(_file) except IOError: - logger.debug("Could not open file: %s", item) + logger.debug("Could not open file: %s", ssl_options) except pyparsing.ParseException: - logger.debug("Could not parse file: %s", item) - else: - return [] + logger.debug("Could not parse file: %s", ssl_options) + return [] def _set_locations(self, ssl_options): """Set default location for directives. From 578db8b8406382da21db6731f4c63dbbeb9a4b2d Mon Sep 17 00:00:00 2001 From: Ola Bini Date: Thu, 21 Jan 2016 19:08:38 -0500 Subject: [PATCH 05/51] Make ssl includes be an array of directives instead of an include of a file. This will be the first step to filter out the things that would conflict with earlier declarations --- .../letsencrypt_nginx/configurator.py | 3 +-- letsencrypt-nginx/letsencrypt_nginx/parser.py | 18 +++++++++++++++++- .../tests/configurator_test.py | 13 +++++++------ .../letsencrypt_nginx/tests/parser_test.py | 11 +++++++++++ .../letsencrypt_nginx/tls_sni_01.py | 4 ++-- 5 files changed, 38 insertions(+), 11 deletions(-) diff --git a/letsencrypt-nginx/letsencrypt_nginx/configurator.py b/letsencrypt-nginx/letsencrypt_nginx/configurator.py index efa7e08b4..918ba53c2 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/configurator.py +++ b/letsencrypt-nginx/letsencrypt_nginx/configurator.py @@ -332,8 +332,7 @@ class NginxConfigurator(common.Plugin): snakeoil_cert, snakeoil_key = self._get_snakeoil_paths() ssl_block = [['listen', '{0} ssl'.format(self.config.tls_sni_01_port)], ['ssl_certificate', snakeoil_cert], - ['ssl_certificate_key', snakeoil_key], - ['include', self.parser.loc["ssl_options"]]] + ['ssl_certificate_key', snakeoil_key]] + self.parser.loc["ssl_options"] self.parser.add_server_directives( vhost.filep, vhost.names, ssl_block, replace=False) vhost.ssl = True diff --git a/letsencrypt-nginx/letsencrypt_nginx/parser.py b/letsencrypt-nginx/letsencrypt_nginx/parser.py index 3b1dd049e..935bf40dd 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/parser.py +++ b/letsencrypt-nginx/letsencrypt_nginx/parser.py @@ -169,6 +169,18 @@ class NginxParser(object): logger.debug("Could not parse file: %s", item) return trees + def _parse_ssl_options(self, ssl_options): + if ssl_options is not None: + try: + with open(ssl_options) as _file: + return nginxparser.load(_file) + except IOError: + logger.debug("Could not open file: %s", item) + except pyparsing.ParseException: + logger.debug("Could not parse file: %s", item) + else: + return [] + def _set_locations(self, ssl_options): """Set default location for directives. @@ -188,7 +200,7 @@ class NginxParser(object): name = default return {"root": root, "default": default, "listen": listen, - "name": name, "ssl_options": ssl_options} + "name": name, "ssl_options": self._parse_ssl_options(ssl_options)} def _find_config_root(self): """Find the Nginx Configuration Root file.""" @@ -503,6 +515,10 @@ def _add_directive(block, directive, replace): See _add_directives for more documentation. """ + if directive[0] == '#': + block.append(directive) + return + 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. diff --git a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py b/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py index 4fce33079..f35dcdfc0 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py +++ b/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py @@ -216,9 +216,9 @@ class NginxConfiguratorTest(util.NginxTest): ['listen', '5001 ssl'], ['ssl_certificate', 'example/fullchain.pem'], - ['ssl_certificate_key', 'example/key.pem'], - ['include', self.config.parser.loc["ssl_options"]] - ]]], + ['ssl_certificate_key', 'example/key.pem']] + + util.filter_comments(self.config.parser.loc["ssl_options"]) + ]], parsed_example_conf) self.assertEqual([['server_name', 'somename alias another.alias']], parsed_server_conf) @@ -234,8 +234,9 @@ class NginxConfiguratorTest(util.NginxTest): ['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"]]]], + ['ssl_certificate_key', '/etc/nginx/key.pem']]+ + util.filter_comments(self.config.parser.loc["ssl_options"]) + ], 2)) def test_get_all_certs_keys(self): @@ -394,6 +395,6 @@ class NginxConfiguratorTest(util.NginxTest): 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 diff --git a/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py b/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py index b64f1dee3..1190a2326 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py +++ b/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py @@ -249,5 +249,16 @@ class NginxParserTest(util.NginxTest): ]) self.assertTrue(server['ssl']) + def test_ssl_options_should_be_parsed_ssl_directives(self): + nparser = parser.NginxParser(self.config_path, self.ssl_options) + self.assertEqual(nparser.loc["ssl_options"], + [['ssl_session_cache', 'shared:SSL:1m'], + ['ssl_session_timeout', '1440m'], + ['ssl_protocols', 'TLSv1 TLSv1.1 TLSv1.2'], + ['ssl_prefer_server_ciphers', 'on'], + ['#', ' Using list of ciphers from "Bulletproof SSL and TLS"'], + ['ssl_ciphers', '"ECDHE-ECDSA-AES128-GCM-SHA256 ECDHE-ECDSA-AES256-GCM-SHA384 ECDHE-ECDSA-AES128-SHA ECDHE-ECDSA-AES256-SHA ECDHE-ECDSA-AES128-SHA256 ECDHE-ECDSA-AES256-SHA384 ECDHE-RSA-AES128-GCM-SHA256 ECDHE-RSA-AES256-GCM-SHA384 ECDHE-RSA-AES128-SHA ECDHE-RSA-AES128-SHA256 ECDHE-RSA-AES256-SHA384 DHE-RSA-AES128-GCM-SHA256 DHE-RSA-AES256-GCM-SHA384 DHE-RSA-AES128-SHA DHE-RSA-AES256-SHA DHE-RSA-AES128-SHA256 DHE-RSA-AES256-SHA256 EDH-RSA-DES-CBC3-SHA"'] + ]) + if __name__ == "__main__": unittest.main() # pragma: no cover diff --git a/letsencrypt-nginx/letsencrypt_nginx/tls_sni_01.py b/letsencrypt-nginx/letsencrypt_nginx/tls_sni_01.py index e59281c4c..5afc950ad 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tls_sni_01.py +++ b/letsencrypt-nginx/letsencrypt_nginx/tls_sni_01.py @@ -146,7 +146,6 @@ class NginxTlsSni01(common.TLSSNI01): block.extend([['server_name', achall.response(achall.account_key).z_domain], - ['include', self.configurator.parser.loc["ssl_options"]], # access and error logs necessary for # integration testing (non-root) ['access_log', os.path.join( @@ -155,6 +154,7 @@ class NginxTlsSni01(common.TLSSNI01): self.configurator.config.work_dir, 'error.log')], ['ssl_certificate', self.get_cert_path(achall)], ['ssl_certificate_key', self.get_key_path(achall)], - [['location', '/'], [['root', document_root]]]]) + [['location', '/'], [['root', document_root]]]] + + self.configurator.parser.loc["ssl_options"]) return [['server'], block] From 067e51170296fae8f908f60e357d99cbe62a09bc Mon Sep 17 00:00:00 2001 From: Ola Bini Date: Fri, 22 Jan 2016 12:12:29 -0500 Subject: [PATCH 06/51] Change name of session cache in order to minimize risk of conflict --- letsencrypt-nginx/letsencrypt_nginx/options-ssl-nginx.conf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt-nginx/letsencrypt_nginx/options-ssl-nginx.conf b/letsencrypt-nginx/letsencrypt_nginx/options-ssl-nginx.conf index f0081c1fc..3faab8818 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/options-ssl-nginx.conf +++ b/letsencrypt-nginx/letsencrypt_nginx/options-ssl-nginx.conf @@ -1,4 +1,4 @@ -ssl_session_cache shared:SSL:1m; +ssl_session_cache shared:le_nginx_SSL:1m; ssl_session_timeout 1440m; ssl_protocols TLSv1 TLSv1.1 TLSv1.2; From 613250e1b26f4c28354932340830eb0b09e5e3cb Mon Sep 17 00:00:00 2001 From: Ola Bini Date: Fri, 22 Jan 2016 15:03:51 -0500 Subject: [PATCH 07/51] Stupid mistake, forgot to change the test --- letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py b/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py index 1190a2326..3fc6a214e 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py +++ b/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py @@ -252,7 +252,7 @@ class NginxParserTest(util.NginxTest): def test_ssl_options_should_be_parsed_ssl_directives(self): nparser = parser.NginxParser(self.config_path, self.ssl_options) self.assertEqual(nparser.loc["ssl_options"], - [['ssl_session_cache', 'shared:SSL:1m'], + [['ssl_session_cache', 'shared:le_nginx_SSL:1m'], ['ssl_session_timeout', '1440m'], ['ssl_protocols', 'TLSv1 TLSv1.1 TLSv1.2'], ['ssl_prefer_server_ciphers', 'on'], From f4a130eab3529fb8860669ecce3b5d2fd8393dec Mon Sep 17 00:00:00 2001 From: Ola Bini Date: Fri, 22 Jan 2016 16:28:05 -0500 Subject: [PATCH 08/51] A final small fix, hopefully --- letsencrypt-nginx/letsencrypt_nginx/parser.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/letsencrypt-nginx/letsencrypt_nginx/parser.py b/letsencrypt-nginx/letsencrypt_nginx/parser.py index 935bf40dd..bd9bebe08 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/parser.py +++ b/letsencrypt-nginx/letsencrypt_nginx/parser.py @@ -175,11 +175,10 @@ class NginxParser(object): with open(ssl_options) as _file: return nginxparser.load(_file) except IOError: - logger.debug("Could not open file: %s", item) + logger.debug("Could not open file: %s", ssl_options) except pyparsing.ParseException: - logger.debug("Could not parse file: %s", item) - else: - return [] + logger.debug("Could not parse file: %s", ssl_options) + return [] def _set_locations(self, ssl_options): """Set default location for directives. From 2b097b0fb2c65939e310d3dd271f1d65a11f8c9e Mon Sep 17 00:00:00 2001 From: Ola Bini Date: Mon, 25 Jan 2016 11:39:46 -0500 Subject: [PATCH 09/51] Fix lint issues --- letsencrypt-nginx/letsencrypt_nginx/parser.py | 2 +- .../letsencrypt_nginx/tests/configurator_test.py | 2 +- .../letsencrypt_nginx/tests/parser_test.py | 11 +++++++++-- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/letsencrypt-nginx/letsencrypt_nginx/parser.py b/letsencrypt-nginx/letsencrypt_nginx/parser.py index bd9bebe08..1cf805eef 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/parser.py +++ b/letsencrypt-nginx/letsencrypt_nginx/parser.py @@ -179,7 +179,7 @@ class NginxParser(object): except pyparsing.ParseException: logger.debug("Could not parse file: %s", ssl_options) return [] - + def _set_locations(self, ssl_options): """Set default location for directives. diff --git a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py b/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py index f35dcdfc0..c91372651 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py +++ b/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py @@ -395,6 +395,6 @@ class NginxConfiguratorTest(util.NginxTest): 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 diff --git a/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py b/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py index 3fc6a214e..d66206a57 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py +++ b/letsencrypt-nginx/letsencrypt_nginx/tests/parser_test.py @@ -257,8 +257,15 @@ class NginxParserTest(util.NginxTest): ['ssl_protocols', 'TLSv1 TLSv1.1 TLSv1.2'], ['ssl_prefer_server_ciphers', 'on'], ['#', ' Using list of ciphers from "Bulletproof SSL and TLS"'], - ['ssl_ciphers', '"ECDHE-ECDSA-AES128-GCM-SHA256 ECDHE-ECDSA-AES256-GCM-SHA384 ECDHE-ECDSA-AES128-SHA ECDHE-ECDSA-AES256-SHA ECDHE-ECDSA-AES128-SHA256 ECDHE-ECDSA-AES256-SHA384 ECDHE-RSA-AES128-GCM-SHA256 ECDHE-RSA-AES256-GCM-SHA384 ECDHE-RSA-AES128-SHA ECDHE-RSA-AES128-SHA256 ECDHE-RSA-AES256-SHA384 DHE-RSA-AES128-GCM-SHA256 DHE-RSA-AES256-GCM-SHA384 DHE-RSA-AES128-SHA DHE-RSA-AES256-SHA DHE-RSA-AES128-SHA256 DHE-RSA-AES256-SHA256 EDH-RSA-DES-CBC3-SHA"'] + ['ssl_ciphers', '"ECDHE-ECDSA-AES128-GCM-SHA256 ECDHE-ECDSA-'+ + 'AES256-GCM-SHA384 ECDHE-ECDSA-AES128-SHA ECDHE-ECDSA-AES256'+ + '-SHA ECDHE-ECDSA-AES128-SHA256 ECDHE-ECDSA-AES256-SHA384'+ + ' ECDHE-RSA-AES128-GCM-SHA256 ECDHE-RSA-AES256-GCM-SHA384'+ + ' ECDHE-RSA-AES128-SHA ECDHE-RSA-AES128-SHA256 ECDHE-RSA-'+ + 'AES256-SHA384 DHE-RSA-AES128-GCM-SHA256 DHE-RSA-AES256-GCM'+ + '-SHA384 DHE-RSA-AES128-SHA DHE-RSA-AES256-SHA DHE-RSA-'+ + 'AES128-SHA256 DHE-RSA-AES256-SHA256 EDH-RSA-DES-CBC3-SHA"'] ]) - + if __name__ == "__main__": unittest.main() # pragma: no cover From 6ffa14c632775e361fdce96badca09c5e195f93d Mon Sep 17 00:00:00 2001 From: Ola Bini Date: Mon, 25 Jan 2016 14:22:03 -0500 Subject: [PATCH 10/51] Bad merge managed to introduce whitespace again. Sigh. --- letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py b/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py index f35dcdfc0..c91372651 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py +++ b/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py @@ -395,6 +395,6 @@ class NginxConfiguratorTest(util.NginxTest): 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 44113a5d068ef224295994fcd60f9024b0a6854d Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 7 Jul 2016 17:25:09 -0700 Subject: [PATCH 11/51] Automatically enable EPEL (after prompting users) --- letsencrypt-auto-source/letsencrypt-auto | 24 +++++++++++++++---- .../pieces/bootstrappers/rpm_common.sh | 24 +++++++++++++++---- 2 files changed, 40 insertions(+), 8 deletions(-) diff --git a/letsencrypt-auto-source/letsencrypt-auto b/letsencrypt-auto-source/letsencrypt-auto index 22c914ed8..4abe7be38 100755 --- a/letsencrypt-auto-source/letsencrypt-auto +++ b/letsencrypt-auto-source/letsencrypt-auto @@ -281,6 +281,26 @@ BootstrapRpmCommon() { exit 1 fi + if [ "$ASSUME_YES" = 1 ]; then + yes_flag="-y" + fi + + if ! $SUDO $tool list *virtualenv > /dev/null 2>&1; then + echo "To use Certbot, packages from the EPEL repository need to be installed." + if [ "$ASSUME_YES" = 1 ]; then + /bin/echo -n "Enabling the EPEL repository in 3 seconds..." + sleep 1s + /bin/echo -ne "\e[0K\rEnabling the EPEL repository in 2 seconds..." + sleep 1s + /bin/echo -e "\e[0K\rEnabling the EPEL repository in 1 seconds..." + sleep 1s + fi + if ! $SUDO $tool install $yes_flag epel-release; then + echo "Could not enable EPEL. Aborting bootstrap!" + exit 1 + fi + fi + pkgs=" gcc dialog @@ -318,10 +338,6 @@ BootstrapRpmCommon() { " fi - if [ "$ASSUME_YES" = 1 ]; then - yes_flag="-y" - fi - if ! $SUDO $tool install $yes_flag $pkgs; then echo "Could not install OS dependencies. Aborting bootstrap!" exit 1 diff --git a/letsencrypt-auto-source/pieces/bootstrappers/rpm_common.sh b/letsencrypt-auto-source/pieces/bootstrappers/rpm_common.sh index 0f98b4bbc..e9865aed3 100755 --- a/letsencrypt-auto-source/pieces/bootstrappers/rpm_common.sh +++ b/letsencrypt-auto-source/pieces/bootstrappers/rpm_common.sh @@ -17,6 +17,26 @@ BootstrapRpmCommon() { exit 1 fi + if [ "$ASSUME_YES" = 1 ]; then + yes_flag="-y" + fi + + if ! $SUDO $tool list *virtualenv > /dev/null 2>&1; then + echo "To use Certbot, packages from the EPEL repository need to be installed." + if [ "$ASSUME_YES" = 1 ]; then + /bin/echo -n "Enabling the EPEL repository in 3 seconds..." + sleep 1s + /bin/echo -ne "\e[0K\rEnabling the EPEL repository in 2 seconds..." + sleep 1s + /bin/echo -e "\e[0K\rEnabling the EPEL repository in 1 seconds..." + sleep 1s + fi + if ! $SUDO $tool install $yes_flag epel-release; then + echo "Could not enable EPEL. Aborting bootstrap!" + exit 1 + fi + fi + pkgs=" gcc dialog @@ -54,10 +74,6 @@ BootstrapRpmCommon() { " fi - if [ "$ASSUME_YES" = 1 ]; then - yes_flag="-y" - fi - if ! $SUDO $tool install $yes_flag $pkgs; then echo "Could not install OS dependencies. Aborting bootstrap!" exit 1 From 9c915b0ae4e42b3c4158b32efe35ae3bdfb1d476 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 14 Jul 2016 18:15:01 -0700 Subject: [PATCH 12/51] Fix tests --- certbot-nginx/certbot_nginx/parser.py | 2 +- certbot-nginx/certbot_nginx/tests/parser_test.py | 2 +- certbot-nginx/certbot_nginx/tests/util.py | 13 ++++++++++--- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index f208d0833..5b6860690 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -177,7 +177,7 @@ class NginxParser(object): if ssl_options is not None: try: with open(ssl_options) as _file: - return nginxparser.load(_file) + return nginxparser.load(_file).spaced except IOError: logger.debug("Could not open file: %s", ssl_options) except pyparsing.ParseException: diff --git a/certbot-nginx/certbot_nginx/tests/parser_test.py b/certbot-nginx/certbot_nginx/tests/parser_test.py index ddd375d96..d0bc32297 100644 --- a/certbot-nginx/certbot_nginx/tests/parser_test.py +++ b/certbot-nginx/certbot_nginx/tests/parser_test.py @@ -251,7 +251,7 @@ class NginxParserTest(util.NginxTest): def test_ssl_options_should_be_parsed_ssl_directives(self): nparser = parser.NginxParser(self.config_path, self.ssl_options) - self.assertEqual(nparser.loc["ssl_options"], + self.assertEqual(nginxparser.UnspacedList(nparser.loc["ssl_options"]), [['ssl_session_cache', 'shared:le_nginx_SSL:1m'], ['ssl_session_timeout', '1440m'], ['ssl_protocols', 'TLSv1 TLSv1.1 TLSv1.2'], diff --git a/certbot-nginx/certbot_nginx/tests/util.py b/certbot-nginx/certbot_nginx/tests/util.py index 866e5a9c7..96fdac527 100644 --- a/certbot-nginx/certbot_nginx/tests/util.py +++ b/certbot-nginx/certbot_nginx/tests/util.py @@ -17,6 +17,7 @@ from certbot.plugins import common from certbot_nginx import constants from certbot_nginx import configurator +from certbot_nginx import nginxparser class NginxTest(unittest.TestCase): # pylint: disable=too-few-public-methods @@ -84,14 +85,20 @@ def filter_comments(tree): def traverse(tree): """Generator dropping comment nodes""" for entry in tree: - key, values = entry + # key, values = entry + spaceless = [e for e in entry if not nginxparser.spacey(e)] + if spaceless: + key = spaceless[0] + values = spaceless[1] if len(spaceless) > 1 else None + else: + key = values = "" if isinstance(key, list): new = copy.deepcopy(entry) new[1] = filter_comments(values) yield new else: - if key != '#': - yield entry + if key != '#' and spaceless: + yield spaceless return list(traverse(tree)) From dbb2398270e611a1f7211cf95f7b8e60ffa3aec0 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 15 Jul 2016 09:25:12 -0700 Subject: [PATCH 13/51] Add _comment_spaced_block --- certbot-nginx/certbot_nginx/options-ssl-nginx.conf | 1 - certbot-nginx/certbot_nginx/parser.py | 14 ++++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/certbot-nginx/certbot_nginx/options-ssl-nginx.conf b/certbot-nginx/certbot_nginx/options-ssl-nginx.conf index 3faab8818..89c920b3e 100644 --- a/certbot-nginx/certbot_nginx/options-ssl-nginx.conf +++ b/certbot-nginx/certbot_nginx/options-ssl-nginx.conf @@ -4,5 +4,4 @@ ssl_session_timeout 1440m; ssl_protocols TLSv1 TLSv1.1 TLSv1.2; ssl_prefer_server_ciphers on; -# Using list of ciphers from "Bulletproof SSL and TLS" ssl_ciphers "ECDHE-ECDSA-AES128-GCM-SHA256 ECDHE-ECDSA-AES256-GCM-SHA384 ECDHE-ECDSA-AES128-SHA ECDHE-ECDSA-AES256-SHA ECDHE-ECDSA-AES128-SHA256 ECDHE-ECDSA-AES256-SHA384 ECDHE-RSA-AES128-GCM-SHA256 ECDHE-RSA-AES256-GCM-SHA384 ECDHE-RSA-AES128-SHA ECDHE-RSA-AES128-SHA256 ECDHE-RSA-AES256-SHA384 DHE-RSA-AES128-GCM-SHA256 DHE-RSA-AES256-GCM-SHA384 DHE-RSA-AES128-SHA DHE-RSA-AES256-SHA DHE-RSA-AES128-SHA256 DHE-RSA-AES256-SHA256 EDH-RSA-DES-CBC3-SHA"; diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index 5b6860690..5bfd9182b 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -566,3 +566,17 @@ def _add_directive(block, directive, replace): directive, block[location])) else: block.append(directive) + +def _comment_spaced_block(block): + """Adds a "managed by Certbot" comment to every directive.""" + comment = " # managed by Certbot" + indent = 80 - len(comment) + for i, entry in enumerate(block): + if isinstance(entry, list): + line = "".join(entry) + line = "".join(c for c in line if c != "\n") + linelength = len(line) + extra = indent - linelength + if extra < 0: + extra = 0 + block[i][-1] += extra * " " + comment From 5d7ef49fac778aa266a8f9d2d5ab9fc3a1030150 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 18 Jul 2016 15:25:09 -0700 Subject: [PATCH 14/51] _add_directive cleanup --- certbot-nginx/certbot_nginx/parser.py | 44 +++++++++++---------------- 1 file changed, 18 insertions(+), 26 deletions(-) diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index 5bfd9182b..cd256d0c4 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -518,7 +518,9 @@ def _add_directives(block, directives, replace): for directive in directives: _add_directive(block, directive, replace) -repeatable_directives = set(['server_name', 'listen', 'include']) + +REPEATABLE_DIRECTIVES = set(['server_name', 'listen', 'include']) + def _add_directive(block, directive, replace): """Adds or replaces a single directive in a config block. @@ -527,27 +529,20 @@ def _add_directive(block, directive, replace): """ directive = nginxparser.UnspacedList(directive) - if len(directive) == 0: - # whitespace + if len(directive) == 0 or directive[0] == '#': + # whitespace or comment block.append(directive) return - if directive[0] == '#': - block.append(directive) - return - - 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 + # the name of the directive we want to add. If no line exists, use None. + location = next((index for index, line in enumerate(block) + if line and line[0] == directive[0]), None) if replace: - if location == -1: + if location is None: raise errors.MisconfigurationError( - 'expected directive for %s in the Nginx ' - 'config but did not find it.' % directive[0]) + 'expected directive for {0} in the Nginx ' + 'config but did not find it.'.format(directive[0])) block[location] = directive else: # Append directive. Fail if the name is not a repeatable directive name, @@ -555,17 +550,14 @@ def _add_directive(block, directive, replace): # in the config file. 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( - 'tried to insert directive "%s" but found conflicting "%s".' % ( - directive, block[location])) - else: + if location is None or (isinstance(directive_name, str) and + directive_name in REPEATABLE_DIRECTIVES): block.append(directive) + elif block[location][1] != directive_value: + raise errors.MisconfigurationError( + 'tried to insert directive "{0}" but found ' + 'conflicting "{1}".'.format(directive, block[location])) + def _comment_spaced_block(block): """Adds a "managed by Certbot" comment to every directive.""" From aa33c0fa83fd7230af457074d4142fb29c452588 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 18 Jul 2016 15:33:28 -0700 Subject: [PATCH 15/51] does it work? --- certbot-nginx/certbot_nginx/parser.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index cd256d0c4..5350e04d7 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -520,6 +520,7 @@ def _add_directives(block, directives, replace): REPEATABLE_DIRECTIVES = set(['server_name', 'listen', 'include']) +COMMENT = [" ", "#", " managed by Certbot"] def _add_directive(block, directive, replace): @@ -544,6 +545,7 @@ def _add_directive(block, directive, replace): 'expected directive for {0} in the Nginx ' 'config but did not find it.'.format(directive[0])) block[location] = directive + block.insert(location + 1, COMMENT) 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 @@ -553,6 +555,7 @@ def _add_directive(block, directive, replace): if location is None or (isinstance(directive_name, str) and directive_name in REPEATABLE_DIRECTIVES): block.append(directive) + block.append(COMMENT) elif block[location][1] != directive_value: raise errors.MisconfigurationError( 'tried to insert directive "{0}" but found ' From bd21325fcdd13c88b443233d1d55939d6868eadd Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 18 Jul 2016 18:12:44 -0700 Subject: [PATCH 16/51] newline logic --- certbot-nginx/certbot_nginx/parser.py | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index 5350e04d7..57ea2db56 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -517,10 +517,26 @@ def _add_directives(block, directives, replace): """ for directive in directives: _add_directive(block, directive, replace) - + last = block[-1] + if not (isinstance(last, str) and '\n' in last): + block.append('\n') + REPEATABLE_DIRECTIVES = set(['server_name', 'listen', 'include']) -COMMENT = [" ", "#", " managed by Certbot"] +COMMENT_STR = ' managed by Certbot' +COMMENT = [' ', '#', ' managed by Certbot'] + + +def _comment_directive(block, location): + """Add a comment to the end of the line at location.""" + block.insert(location + 1, COMMENT[:]) + + if len(block) > location + 2: # there is a block after us + next_entry = block[location + 2] + if isinstance(next_entry, list): + next_entry = next_entry.spaced[0] + if "\n" not in next_entry: + block.insert(location + 2, '\n') def _add_directive(block, directive, replace): From 5dd8f70e567f4540e75b0cac0c3a4fbc0769b137 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 18 Jul 2016 18:19:14 -0700 Subject: [PATCH 17/51] better newline logic --- certbot-nginx/certbot_nginx/parser.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index 57ea2db56..e8fd50452 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -529,14 +529,19 @@ COMMENT = [' ', '#', ' managed by Certbot'] def _comment_directive(block, location): """Add a comment to the end of the line at location.""" + if len(block) > location + 1: # there is a block after us + next_entry = block[location + 1] + else: + # we're at the end of the block, pretend there's a newline after us; it will actually be added later in + # add_directives + next_entry = "\n" + if isinstance(next_entry, list): + if COMMENT[-1] in next_entry[-1]: + return + next_entry = next_entry.spaced[0] block.insert(location + 1, COMMENT[:]) - - if len(block) > location + 2: # there is a block after us - next_entry = block[location + 2] - if isinstance(next_entry, list): - next_entry = next_entry.spaced[0] - if "\n" not in next_entry: - block.insert(location + 2, '\n') + if "\n" not in next_entry: + block.insert(location + 2, '\n') def _add_directive(block, directive, replace): From ed4fc9d2f73631b9fa57f111ac1ee51a7f0fd4c6 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 18 Jul 2016 18:20:21 -0700 Subject: [PATCH 18/51] call _comment_directive --- certbot-nginx/certbot_nginx/parser.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index e8fd50452..a79136836 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -566,7 +566,7 @@ def _add_directive(block, directive, replace): 'expected directive for {0} in the Nginx ' 'config but did not find it.'.format(directive[0])) block[location] = directive - block.insert(location + 1, COMMENT) + _comment_directive(block, location) 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 @@ -576,7 +576,7 @@ def _add_directive(block, directive, replace): if location is None or (isinstance(directive_name, str) and directive_name in REPEATABLE_DIRECTIVES): block.append(directive) - block.append(COMMENT) + _comment_directive(block, len(block)) elif block[location][1] != directive_value: raise errors.MisconfigurationError( 'tried to insert directive "{0}" but found ' From 2ce5b195e54d50881ebb506e070cb74108b38384 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 18 Jul 2016 18:23:54 -0700 Subject: [PATCH 19/51] check certbot --- certbot-nginx/certbot_nginx/parser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index a79136836..b7bfebb32 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -536,7 +536,7 @@ def _comment_directive(block, location): # add_directives next_entry = "\n" if isinstance(next_entry, list): - if COMMENT[-1] in next_entry[-1]: + if "Certbot" in next_entry[-1]: return next_entry = next_entry.spaced[0] block.insert(location + 1, COMMENT[:]) From e5cb04ee7da2f498ca5e08598980ca9c2e7914d3 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Thu, 21 Jul 2016 13:26:57 -0700 Subject: [PATCH 20/51] A couple of fixes --- certbot-nginx/certbot_nginx/parser.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index b7bfebb32..71521110e 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -520,7 +520,7 @@ def _add_directives(block, directives, replace): last = block[-1] if not (isinstance(last, str) and '\n' in last): block.append('\n') - + REPEATABLE_DIRECTIVES = set(['server_name', 'listen', 'include']) COMMENT_STR = ' managed by Certbot' @@ -531,7 +531,7 @@ def _comment_directive(block, location): """Add a comment to the end of the line at location.""" if len(block) > location + 1: # there is a block after us next_entry = block[location + 1] - else: + else: # we're at the end of the block, pretend there's a newline after us; it will actually be added later in # add_directives next_entry = "\n" @@ -576,7 +576,7 @@ def _add_directive(block, directive, replace): if location is None or (isinstance(directive_name, str) and directive_name in REPEATABLE_DIRECTIVES): block.append(directive) - _comment_directive(block, len(block)) + _comment_directive(block, len(block) - 1) elif block[location][1] != directive_value: raise errors.MisconfigurationError( 'tried to insert directive "{0}" but found ' From 85d9ab4d5c2d2afd3c36229a771f77e4536853cf Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Thu, 21 Jul 2016 13:39:13 -0700 Subject: [PATCH 21/51] UnspacedList._spaced_position: support the slice at the end fo the list - Which is needed for .insert()ing at the end, for instance. --- certbot-nginx/certbot_nginx/nginxparser.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/certbot-nginx/certbot_nginx/nginxparser.py b/certbot-nginx/certbot_nginx/nginxparser.py index 1859777d8..a4d4a452f 100644 --- a/certbot-nginx/certbot_nginx/nginxparser.py +++ b/certbot-nginx/certbot_nginx/nginxparser.py @@ -279,6 +279,9 @@ class UnspacedList(list): # Normalize indexes like list[-1] etc, and save the result if idx < 0: idx = len(self) + idx + if idx == len(self): + # not an index, but the slice at the end of the list + return len(self.spaced) if not 0 <= idx < len(self): raise IndexError("list index out of range") idx0 = idx From f98470d4a05620e09eea6992ccd6deda15ffacbf Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 26 Jul 2016 17:01:24 -0700 Subject: [PATCH 22/51] Revert "UnspacedList._spaced_position: support the slice at the end fo the list" This reverts commit 85d9ab4d5c2d2afd3c36229a771f77e4536853cf. --- certbot-nginx/certbot_nginx/nginxparser.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/certbot-nginx/certbot_nginx/nginxparser.py b/certbot-nginx/certbot_nginx/nginxparser.py index a4d4a452f..1859777d8 100644 --- a/certbot-nginx/certbot_nginx/nginxparser.py +++ b/certbot-nginx/certbot_nginx/nginxparser.py @@ -279,9 +279,6 @@ class UnspacedList(list): # Normalize indexes like list[-1] etc, and save the result if idx < 0: idx = len(self) + idx - if idx == len(self): - # not an index, but the slice at the end of the list - return len(self.spaced) if not 0 <= idx < len(self): raise IndexError("list index out of range") idx0 = idx From 4eb38fe1670bea6d9e49cd7cb8fd3be46a404be8 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 26 Jul 2016 17:09:01 -0700 Subject: [PATCH 23/51] Make spaced list handle an insert past the end of the list --- certbot-nginx/certbot_nginx/nginxparser.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/certbot-nginx/certbot_nginx/nginxparser.py b/certbot-nginx/certbot_nginx/nginxparser.py index 1859777d8..edb280d08 100644 --- a/certbot-nginx/certbot_nginx/nginxparser.py +++ b/certbot-nginx/certbot_nginx/nginxparser.py @@ -215,7 +215,8 @@ class UnspacedList(list): def insert(self, i, x): item, spaced_item = self._coerce(x) - self.spaced.insert(self._spaced_position(i), spaced_item) + self.spaced.insert(self._spaced_position(i) if i < len(self) else i, + spaced_item) list.insert(self, i, item) self.dirty = True From e1f560dca3acde9f10687233eda8e2d7915e68a4 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 26 Jul 2016 17:23:24 -0700 Subject: [PATCH 24/51] Neaten --- certbot-nginx/certbot_nginx/parser.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index 71521110e..9dcee8bef 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -532,8 +532,8 @@ def _comment_directive(block, location): if len(block) > location + 1: # there is a block after us next_entry = block[location + 1] else: - # we're at the end of the block, pretend there's a newline after us; it will actually be added later in - # add_directives + # we're at the end of the block, pretend there's a newline after us; + # it will actually be added later in add_directives next_entry = "\n" if isinstance(next_entry, list): if "Certbot" in next_entry[-1]: From 1060ea7c3d21693bfe7cbc3aadc92dbc901febe9 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 26 Jul 2016 17:36:58 -0700 Subject: [PATCH 25/51] delint --- certbot-nginx/certbot_nginx/parser.py | 4 ++-- certbot-nginx/certbot_nginx/tests/configurator_test.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index c0ce6f185..8500179b1 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -532,8 +532,8 @@ def _comment_directive(block, location): if len(block) > location + 1: # there is a block after us next_entry = block[location + 1] else: - # we're at the end of the block, pretend there's a newline after us; it will actually be added later in - # add_directives + # we're at the end of the block, pretend there's a newline after us; it + # will actually be added later in add_directives next_entry = "\n" if isinstance(next_entry, list): if "Certbot" in next_entry[-1]: diff --git a/certbot-nginx/certbot_nginx/tests/configurator_test.py b/certbot-nginx/certbot_nginx/tests/configurator_test.py index d8fc849b7..f77b9d7cd 100644 --- a/certbot-nginx/certbot_nginx/tests/configurator_test.py +++ b/certbot-nginx/certbot_nginx/tests/configurator_test.py @@ -234,7 +234,7 @@ class NginxConfiguratorTest(util.NginxTest): ['index', 'index.html index.htm']]], ['listen', '5001 ssl'], ['ssl_certificate', '/etc/nginx/fullchain.pem'], - ['ssl_certificate_key', '/etc/nginx/key.pem']]+ + ['ssl_certificate_key', '/etc/nginx/key.pem']] + util.filter_comments(self.config.parser.loc["ssl_options"]) ], 2)) From 89758decbb2b1b18e8b56c49c73a6cb1c852e5ec Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Fri, 29 Jul 2016 17:28:22 -0700 Subject: [PATCH 26/51] Fix a test --- certbot-nginx/certbot_nginx/tests/configurator_test.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/certbot-nginx/certbot_nginx/tests/configurator_test.py b/certbot-nginx/certbot_nginx/tests/configurator_test.py index a6cd0d588..19dcd157a 100644 --- a/certbot-nginx/certbot_nginx/tests/configurator_test.py +++ b/certbot-nginx/certbot_nginx/tests/configurator_test.py @@ -94,7 +94,8 @@ class NginxConfiguratorTest(util.NginxTest): ['listen', '127.0.0.1'], ['server_name', '.example.com'], ['server_name', 'example.*'], - ['listen', '5001 ssl'] + ['listen', '5001 ssl'], + ['#', ' managed by Certbot'] ]]], parsed[0]) From 3a2df72bceeada0a0a4fc7b0cc7df7d926092451 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Fri, 5 Aug 2016 15:36:24 -0700 Subject: [PATCH 27/51] Add newlines to the ends of blocks more correctly --- certbot-nginx/certbot_nginx/parser.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index ca02b6316..4577f9fa5 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -518,8 +518,8 @@ def _add_directives(block, directives, replace): for directive in directives: _add_directive(block, directive, replace) last = block[-1] - if not (isinstance(last, str) and '\n' in last): - block.append('\n') + if not '\n' in last: # could be " \n " or ["\n"] ! + block.append(nginxparser.UnspacedList('\n')) REPEATABLE_DIRECTIVES = set(['server_name', 'listen', 'include']) From cdc894601c83d75e2038c5bc9d6f97fc6fbc3eef Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Fri, 5 Aug 2016 15:36:40 -0700 Subject: [PATCH 28/51] Tolerate our own added newlines --- certbot-nginx/certbot_nginx/parser.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index 4577f9fa5..536602e27 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -329,6 +329,8 @@ class NginxParser(object): tup = [None, None, vhost.filep] if vhost.ssl: for directive in vhost.raw: + if not directive: + continue if directive[0] == 'ssl_certificate': tup[0] = directive[1] elif directive[0] == 'ssl_certificate_key': From 460f49778f163fc137bb8eeb64135ec9dcbc9b65 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Fri, 5 Aug 2016 15:37:01 -0700 Subject: [PATCH 29/51] Fix tests for our new spacey, commented world --- certbot-nginx/certbot_nginx/tests/parser_test.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/certbot-nginx/certbot_nginx/tests/parser_test.py b/certbot-nginx/certbot_nginx/tests/parser_test.py index aec6f2a23..09b1cfd0b 100644 --- a/certbot-nginx/certbot_nginx/tests/parser_test.py +++ b/certbot-nginx/certbot_nginx/tests/parser_test.py @@ -144,7 +144,10 @@ class NginxParserTest(util.NginxTest): self.assertEqual(nparser.parsed[server_conf], [['server_name', 'somename alias another.alias'], ['foo', 'bar'], - ['ssl_certificate', '/etc/ssl/cert2.pem'] + ['#', ' managed by Certbot'], + ['ssl_certificate', '/etc/ssl/cert2.pem'], + ['#', ' managed by Certbot'], + [], [] ]) def test_add_http_directives(self): @@ -174,8 +177,8 @@ class NginxParserTest(util.NginxTest): nparser.parsed[filep], [[['server'], [['listen', '69.50.225.155:9000'], ['listen', '127.0.0.1'], - ['server_name', 'foobar.com'], - ['server_name', 'example.*'], + ['server_name', 'foobar.com'], ['#', ' managed by Certbot'], + ['server_name', 'example.*'], [] ]]]) self.assertRaises(errors.MisconfigurationError, nparser.add_server_directives, @@ -256,7 +259,6 @@ class NginxParserTest(util.NginxTest): ['ssl_session_timeout', '1440m'], ['ssl_protocols', 'TLSv1 TLSv1.1 TLSv1.2'], ['ssl_prefer_server_ciphers', 'on'], - ['#', ' Using list of ciphers from "Bulletproof SSL and TLS"'], ['ssl_ciphers', '"ECDHE-ECDSA-AES128-GCM-SHA256 ECDHE-ECDSA-'+ 'AES256-GCM-SHA384 ECDHE-ECDSA-AES128-SHA ECDHE-ECDSA-AES256'+ '-SHA ECDHE-ECDSA-AES128-SHA256 ECDHE-ECDSA-AES256-SHA384'+ From 7deb1f0ad625ddd2d3603bcd8858ad674ee27fae Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Mon, 8 Aug 2016 12:15:18 -0700 Subject: [PATCH 30/51] Fix bug with UnpsacedList.insert to final position - which only applied when the list actually contained spaces --- certbot-nginx/certbot_nginx/nginxparser.py | 5 +++-- .../certbot_nginx/tests/nginxparser_test.py | 20 +++++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/certbot-nginx/certbot_nginx/nginxparser.py b/certbot-nginx/certbot_nginx/nginxparser.py index 50cd15c29..7e2cd533a 100644 --- a/certbot-nginx/certbot_nginx/nginxparser.py +++ b/certbot-nginx/certbot_nginx/nginxparser.py @@ -227,8 +227,8 @@ class UnspacedList(list): def insert(self, i, x): item, spaced_item = self._coerce(x) - self.spaced.insert(self._spaced_position(i) if i < len(self) else i, - spaced_item) + slicepos = self._spaced_position(i) if i < len(self) else len(self.spaced) + self.spaced.insert(slicepos, spaced_item) list.insert(self, i, item) self.dirty = True @@ -292,6 +292,7 @@ class UnspacedList(list): # Normalize indexes like list[-1] etc, and save the result if idx < 0: idx = len(self) + idx + if not 0 <= idx < len(self): raise IndexError("list index out of range") idx0 = idx diff --git a/certbot-nginx/certbot_nginx/tests/nginxparser_test.py b/certbot-nginx/certbot_nginx/tests/nginxparser_test.py index 5fa9a7d1e..5c8d6d215 100644 --- a/certbot-nginx/certbot_nginx/tests/nginxparser_test.py +++ b/certbot-nginx/certbot_nginx/tests/nginxparser_test.py @@ -223,6 +223,26 @@ class TestUnspacedList(unittest.TestCase): self.assertRaises(IndexError, self.ul2.__getitem__, 2) self.assertRaises(IndexError, self.ul2.__getitem__, -3) + def test_insert(self): + x = UnspacedList( + [['\n ', 'listen', ' ', '69.50.225.155:9000'], + ['\n ', 'listen', ' ', '127.0.0.1'], + ['\n ', 'server_name', ' ', '.example.com'], + ['\n ', 'server_name', ' ', 'example.*'], '\n', + ['listen', ' ', '5001 ssl']]) + x.insert(5, "FROGZ") + self.assertEqual(x, + [['listen', '69.50.225.155:9000'], ['listen', '127.0.0.1'], + ['server_name', '.example.com'], ['server_name', 'example.*'], + ['listen', '5001 ssl'], 'FROGZ']) + self.assertEqual(x.spaced, + [['\n ', 'listen', ' ', '69.50.225.155:9000'], + ['\n ', 'listen', ' ', '127.0.0.1'], + ['\n ', 'server_name', ' ', '.example.com'], + ['\n ', 'server_name', ' ', 'example.*'], '\n', + ['listen', ' ', '5001 ssl'], + 'FROGZ']) + def test_rawlists(self): ul3 = copy.deepcopy(self.ul) ul3.insert(0, "some") From da7e429125c7100653e0479fb5ecaf459e9a55d2 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Mon, 8 Aug 2016 15:14:06 -0700 Subject: [PATCH 31/51] Work around horrible spaciness API usage bug --- certbot-nginx/certbot_nginx/configurator.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index 0a8d2a88a..afa8b8087 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -338,11 +338,16 @@ class NginxConfigurator(common.Plugin): """ snakeoil_cert, snakeoil_key = self._get_snakeoil_paths() + options_subblock = self.parser.loc["ssl_options"] + # the options file doesn't have a newline at the beginning, but there + # needs to be one when it's dropped into the file + if "\n" not in options_subblock[0]: + options_subblock[0].insert(0, "\n") ssl_block = ( [['\n ', 'listen', ' ', '{0} ssl'.format(self.config.tls_sni_01_port)], ['\n ', 'ssl_certificate', ' ', snakeoil_cert], ['\n ', 'ssl_certificate_key', ' ', snakeoil_key]] + - self.parser.loc["ssl_options"]) + options_subblock) self.parser.add_server_directives( vhost.filep, vhost.names, ssl_block, replace=False) From f0c2ed305958a8354077090d98ffb2b2bd6d191e Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Mon, 8 Aug 2016 15:45:49 -0700 Subject: [PATCH 32/51] Lint, improve coverage, rm unused code --- certbot-nginx/certbot_nginx/parser.py | 16 +--------------- .../certbot_nginx/tests/parser_test.py | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index 536602e27..cf1f3c1db 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -540,7 +540,7 @@ def _comment_directive(block, location): if isinstance(next_entry, list): if "Certbot" in next_entry[-1]: return - next_entry = next_entry.spaced[0] + next_entry = next_entry.spaced[0] # pylint: disable=no-member block.insert(location + 1, COMMENT[:]) if "\n" not in next_entry: block.insert(location + 2, '\n') @@ -584,17 +584,3 @@ def _add_directive(block, directive, replace): 'tried to insert directive "{0}" but found ' 'conflicting "{1}".'.format(directive, block[location])) - -def _comment_spaced_block(block): - """Adds a "managed by Certbot" comment to every directive.""" - comment = " # managed by Certbot" - indent = 80 - len(comment) - for i, entry in enumerate(block): - if isinstance(entry, list): - line = "".join(entry) - line = "".join(c for c in line if c != "\n") - linelength = len(line) - extra = indent - linelength - if extra < 0: - extra = 0 - block[i][-1] += extra * " " + comment diff --git a/certbot-nginx/certbot_nginx/tests/parser_test.py b/certbot-nginx/certbot_nginx/tests/parser_test.py index 09b1cfd0b..963b4c815 100644 --- a/certbot-nginx/certbot_nginx/tests/parser_test.py +++ b/certbot-nginx/certbot_nginx/tests/parser_test.py @@ -219,6 +219,23 @@ class NginxParserTest(util.NginxTest): self.assertEqual(winner, parser.get_best_match(target_name, names[i])) + def test_comment_directive(self): + # pylint: disable=protected-access + block = nginxparser.UnspacedList([ + ["\n", "a", " ", "b", "\n"], + ["c", " ", "d"], + ["\n", "e", " ", "f"]]) + from certbot_nginx.parser import _comment_directive, COMMENT + _comment_directive(block, 1) + _comment_directive(block, 0) + self.assertEqual(block.spaced, [ + ["\n", "a", " ", "b", "\n"], + COMMENT, + "\n", + ["c", " ", "d"], + COMMENT, + ["\n", "e", " ", "f"]]) + def test_get_all_certs_keys(self): nparser = parser.NginxParser(self.config_path, self.ssl_options) filep = nparser.abs_path('sites-enabled/example.com') From 712bd9ee6b06d0964f53418fe7cb338d35f59ed1 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Mon, 8 Aug 2016 17:58:22 -0700 Subject: [PATCH 33/51] Copy nginx options file into integration testing environment --- certbot-nginx/tests/boulder-integration.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/certbot-nginx/tests/boulder-integration.sh b/certbot-nginx/tests/boulder-integration.sh index bd35aee21..53dddf7e8 100755 --- a/certbot-nginx/tests/boulder-integration.sh +++ b/certbot-nginx/tests/boulder-integration.sh @@ -7,6 +7,7 @@ export PATH="/usr/sbin:$PATH" # /usr/sbin/nginx nginx_root="$root/nginx" mkdir $nginx_root root="$nginx_root" ./certbot-nginx/tests/boulder-integration.conf.sh > $nginx_root/nginx.conf +cp certbot-nginx/certbot_nginx/options-ssl-nginx.conf $nginx_root killall nginx || true nginx -c $nginx_root/nginx.conf From b5fa0fbad758ce4680631599de12557f602692cf Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Mon, 8 Aug 2016 18:08:11 -0700 Subject: [PATCH 34/51] This is reportedly the correct magic --- certbot-nginx/certbot_nginx/configurator.py | 2 +- certbot-nginx/certbot_nginx/parser.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index afa8b8087..5e415bce6 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -341,7 +341,7 @@ class NginxConfigurator(common.Plugin): options_subblock = self.parser.loc["ssl_options"] # the options file doesn't have a newline at the beginning, but there # needs to be one when it's dropped into the file - if "\n" not in options_subblock[0]: + if options_subblock and "\n" not in options_subblock[0]: options_subblock[0].insert(0, "\n") ssl_block = ( [['\n ', 'listen', ' ', '{0} ssl'.format(self.config.tls_sni_01_port)], diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index cf1f3c1db..73224ea1f 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -179,7 +179,7 @@ class NginxParser(object): with open(ssl_options) as _file: return nginxparser.load(_file).spaced except IOError: - logger.debug("Could not open file: %s", ssl_options) + logger.warn("Missing NGINX TLS options file: %s", ssl_options) except pyparsing.ParseException: logger.debug("Could not parse file: %s", ssl_options) return [] From 9c168017aeccfc28b2a4f22c321753108794f75b Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Mon, 8 Aug 2016 18:17:02 -0700 Subject: [PATCH 35/51] That was not the correct magic --- certbot-nginx/tests/boulder-integration.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot-nginx/tests/boulder-integration.sh b/certbot-nginx/tests/boulder-integration.sh index 53dddf7e8..58613d86f 100755 --- a/certbot-nginx/tests/boulder-integration.sh +++ b/certbot-nginx/tests/boulder-integration.sh @@ -7,7 +7,7 @@ export PATH="/usr/sbin:$PATH" # /usr/sbin/nginx nginx_root="$root/nginx" mkdir $nginx_root root="$nginx_root" ./certbot-nginx/tests/boulder-integration.conf.sh > $nginx_root/nginx.conf -cp certbot-nginx/certbot_nginx/options-ssl-nginx.conf $nginx_root +cp certbot-nginx/certbot_nginx/options-ssl-nginx.conf "$root"/conf killall nginx || true nginx -c $nginx_root/nginx.conf From 0b0eca323cc92a42c492336921a22d7a9ac883d3 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 16 Aug 2016 15:36:41 -0700 Subject: [PATCH 36/51] Remove extra newline --- certbot-nginx/certbot_nginx/nginxparser.py | 1 - 1 file changed, 1 deletion(-) diff --git a/certbot-nginx/certbot_nginx/nginxparser.py b/certbot-nginx/certbot_nginx/nginxparser.py index 7e2cd533a..6f2a3ec70 100644 --- a/certbot-nginx/certbot_nginx/nginxparser.py +++ b/certbot-nginx/certbot_nginx/nginxparser.py @@ -292,7 +292,6 @@ class UnspacedList(list): # Normalize indexes like list[-1] etc, and save the result if idx < 0: idx = len(self) + idx - if not 0 <= idx < len(self): raise IndexError("list index out of range") idx0 = idx From 7fb5cf1cf54c5c5ab10f368d0798ca0fa5164792 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 16 Aug 2016 15:46:31 -0700 Subject: [PATCH 37/51] Catch all pyparsing exceptions --- certbot-nginx/certbot_nginx/parser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index 73224ea1f..229b720b6 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -169,7 +169,7 @@ class NginxParser(object): trees.append(parsed) except IOError: logger.warning("Could not open file: %s", item) - except pyparsing.ParseException: + except pyparsing.ParseBaseException: logger.debug("Could not parse file: %s", item) return trees From ae23800e538473e9c335d92d15991f13c5f32641 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 16 Aug 2016 16:37:40 -0700 Subject: [PATCH 38/51] Comment code that confused bmw --- certbot-nginx/certbot_nginx/parser.py | 1 + 1 file changed, 1 insertion(+) diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index 229b720b6..703a33cf1 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -329,6 +329,7 @@ class NginxParser(object): tup = [None, None, vhost.filep] if vhost.ssl: for directive in vhost.raw: + # A directive can be an empty list to preserve whitespace if not directive: continue if directive[0] == 'ssl_certificate': From 3d4f822be0e3f6ff0522a48bda8e0b673bbfb26e Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 16 Aug 2016 16:41:23 -0700 Subject: [PATCH 39/51] Handle case where block is empty -- not sure if it ever happens, but let's not error out unnecessarily --- certbot-nginx/certbot_nginx/parser.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index 703a33cf1..afea71c26 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -520,8 +520,7 @@ def _add_directives(block, directives, replace): """ for directive in directives: _add_directive(block, directive, replace) - last = block[-1] - if not '\n' in last: # could be " \n " or ["\n"] ! + if block and '\n' not in block[-1]: # could be " \n " or ["\n"] ! block.append(nginxparser.UnspacedList('\n')) From 671d7ee19439a97b19ab215c920f925085f781e1 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 16 Aug 2016 17:45:43 -0700 Subject: [PATCH 40/51] Fix up COMMENT constants --- certbot-nginx/certbot_nginx/parser.py | 6 +++--- certbot-nginx/certbot_nginx/tests/parser_test.py | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index afea71c26..5880074f8 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -525,8 +525,8 @@ def _add_directives(block, directives, replace): REPEATABLE_DIRECTIVES = set(['server_name', 'listen', 'include']) -COMMENT_STR = ' managed by Certbot' -COMMENT = [' ', '#', ' managed by Certbot'] +COMMENT = ' managed by Certbot' +COMMENT_BLOCK = [' ', '#', COMMENT] def _comment_directive(block, location): @@ -541,7 +541,7 @@ def _comment_directive(block, location): if "Certbot" in next_entry[-1]: return next_entry = next_entry.spaced[0] # pylint: disable=no-member - block.insert(location + 1, COMMENT[:]) + block.insert(location + 1, COMMENT_BLOCK[:]) if "\n" not in next_entry: block.insert(location + 2, '\n') diff --git a/certbot-nginx/certbot_nginx/tests/parser_test.py b/certbot-nginx/certbot_nginx/tests/parser_test.py index 963b4c815..fc6c8ff64 100644 --- a/certbot-nginx/certbot_nginx/tests/parser_test.py +++ b/certbot-nginx/certbot_nginx/tests/parser_test.py @@ -225,15 +225,15 @@ class NginxParserTest(util.NginxTest): ["\n", "a", " ", "b", "\n"], ["c", " ", "d"], ["\n", "e", " ", "f"]]) - from certbot_nginx.parser import _comment_directive, COMMENT + from certbot_nginx.parser import _comment_directive, COMMENT_BLOCK _comment_directive(block, 1) _comment_directive(block, 0) self.assertEqual(block.spaced, [ ["\n", "a", " ", "b", "\n"], - COMMENT, + COMMENT_BLOCK, "\n", ["c", " ", "d"], - COMMENT, + COMMENT_BLOCK, ["\n", "e", " ", "f"]]) def test_get_all_certs_keys(self): From 76c2fe579a1c9d49f9ebca6e716b4a2eec76b9d9 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 16 Aug 2016 18:30:45 -0700 Subject: [PATCH 41/51] Make _comment_directive more defensive --- certbot-nginx/certbot_nginx/parser.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index 5880074f8..d98b5225d 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -531,18 +531,17 @@ COMMENT_BLOCK = [' ', '#', COMMENT] def _comment_directive(block, location): """Add a comment to the end of the line at location.""" - if len(block) > location + 1: # there is a block after us - next_entry = block[location + 1] - else: - # we're at the end of the block, pretend there's a newline after us; - # it will actually be added later in add_directives - next_entry = "\n" - if isinstance(next_entry, list): - if "Certbot" in next_entry[-1]: + next_entry = block[location + 1] if location + 1 < len(block) else None + if isinstance(next_entry, list) and next_entry: + if COMMENT in next_entry[-1]: return - next_entry = next_entry.spaced[0] # pylint: disable=no-member + elif isinstance(next_entry, nginxparser.UnspacedList): + next_entry = next_entry.spaced[0] + else: + next_entry = next_entry[0] + block.insert(location + 1, COMMENT_BLOCK[:]) - if "\n" not in next_entry: + if next_entry is not None and "\n" not in next_entry: block.insert(location + 2, '\n') From 76d17bfd0f84fb42e71fe262cb056def533a71b1 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 16 Aug 2016 18:40:05 -0700 Subject: [PATCH 42/51] Avoid modifying parsed ssl_options --- certbot-nginx/certbot_nginx/configurator.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index 5e415bce6..1a14a3866 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -338,16 +338,14 @@ class NginxConfigurator(common.Plugin): """ snakeoil_cert, snakeoil_key = self._get_snakeoil_paths() - options_subblock = self.parser.loc["ssl_options"] # the options file doesn't have a newline at the beginning, but there # needs to be one when it's dropped into the file - if options_subblock and "\n" not in options_subblock[0]: - options_subblock[0].insert(0, "\n") ssl_block = ( [['\n ', 'listen', ' ', '{0} ssl'.format(self.config.tls_sni_01_port)], ['\n ', 'ssl_certificate', ' ', snakeoil_cert], - ['\n ', 'ssl_certificate_key', ' ', snakeoil_key]] + - options_subblock) + ['\n ', 'ssl_certificate_key', ' ', snakeoil_key], + ['\n']] + + self.parser.loc["ssl_options"]) self.parser.add_server_directives( vhost.filep, vhost.names, ssl_block, replace=False) From 971d6d75401fa228468ea376829dcdb6072736da Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 16 Aug 2016 18:50:18 -0700 Subject: [PATCH 43/51] Don't hardcode comment added by Certbot --- certbot-nginx/certbot_nginx/tests/parser_test.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/certbot-nginx/certbot_nginx/tests/parser_test.py b/certbot-nginx/certbot_nginx/tests/parser_test.py index fc6c8ff64..71807d4f4 100644 --- a/certbot-nginx/certbot_nginx/tests/parser_test.py +++ b/certbot-nginx/certbot_nginx/tests/parser_test.py @@ -141,12 +141,13 @@ class NginxParserTest(util.NginxTest): replace=False) nparser.add_server_directives(server_conf, names, [['foo', 'bar']], replace=False) + from certbot_nginx.parser import COMMENT self.assertEqual(nparser.parsed[server_conf], [['server_name', 'somename alias another.alias'], ['foo', 'bar'], - ['#', ' managed by Certbot'], + ['#', COMMENT], ['ssl_certificate', '/etc/ssl/cert2.pem'], - ['#', ' managed by Certbot'], + ['#', COMMENT], [], [] ]) @@ -173,11 +174,12 @@ class NginxParserTest(util.NginxTest): filep = nparser.abs_path('sites-enabled/example.com') nparser.add_server_directives( filep, target, [['server_name', 'foobar.com']], replace=True) + from certbot_nginx.parser import COMMENT self.assertEqual( nparser.parsed[filep], [[['server'], [['listen', '69.50.225.155:9000'], ['listen', '127.0.0.1'], - ['server_name', 'foobar.com'], ['#', ' managed by Certbot'], + ['server_name', 'foobar.com'], ['#', COMMENT], ['server_name', 'example.*'], [] ]]]) self.assertRaises(errors.MisconfigurationError, From 5ec22438ff24579736d94a4fa831cb3ab015866d Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 16 Aug 2016 19:04:05 -0700 Subject: [PATCH 44/51] Make sure mod_ssl_conf exists so it can be parsed --- certbot-nginx/certbot_nginx/configurator.py | 4 ++-- certbot-nginx/tests/boulder-integration.sh | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index 1a14a3866..0383e0693 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -117,6 +117,8 @@ class NginxConfigurator(common.Plugin): # Make sure configuration is valid self.config_test() + # temp_install must be run before creating the NginxParser + temp_install(self.mod_ssl_conf) self.parser = parser.NginxParser( self.conf('server-root'), self.mod_ssl_conf) @@ -124,8 +126,6 @@ class NginxConfigurator(common.Plugin): if self.version is None: self.version = self.get_version() - 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, fullchain_path=None): diff --git a/certbot-nginx/tests/boulder-integration.sh b/certbot-nginx/tests/boulder-integration.sh index 58613d86f..bd35aee21 100755 --- a/certbot-nginx/tests/boulder-integration.sh +++ b/certbot-nginx/tests/boulder-integration.sh @@ -7,7 +7,6 @@ export PATH="/usr/sbin:$PATH" # /usr/sbin/nginx nginx_root="$root/nginx" mkdir $nginx_root root="$nginx_root" ./certbot-nginx/tests/boulder-integration.conf.sh > $nginx_root/nginx.conf -cp certbot-nginx/certbot_nginx/options-ssl-nginx.conf "$root"/conf killall nginx || true nginx -c $nginx_root/nginx.conf From 1aa18a3bad5fb4d435b967a6dcb4cdc5f34809d5 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 16 Aug 2016 19:10:57 -0700 Subject: [PATCH 45/51] Add test to prevent regressing and not copying ssl_options to /etc/letsencrypt --- certbot-nginx/certbot_nginx/tests/configurator_test.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/certbot-nginx/certbot_nginx/tests/configurator_test.py b/certbot-nginx/certbot_nginx/tests/configurator_test.py index 800387e57..ec7408f27 100644 --- a/certbot-nginx/certbot_nginx/tests/configurator_test.py +++ b/certbot-nginx/certbot_nginx/tests/configurator_test.py @@ -39,6 +39,8 @@ class NginxConfiguratorTest(util.NginxTest): def test_prepare(self): self.assertEqual((1, 6, 2), self.config.version) self.assertEqual(5, len(self.config.parser.parsed)) + # ensure we successfully parsed a file for ssl_options + self.assertTrue(self.config.parser.loc["ssl_options"]) @mock.patch("certbot_nginx.configurator.util.exe_exists") @mock.patch("certbot_nginx.configurator.subprocess.Popen") From 465aa38143e67d00befda3a59e51c943b1c4f6fb Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 16 Aug 2016 19:33:19 -0700 Subject: [PATCH 46/51] Revert "Catch all pyparsing exceptions" This reverts commit 7fb5cf1cf54c5c5ab10f368d0798ca0fa5164792. --- certbot-nginx/certbot_nginx/parser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index d98b5225d..681fadc55 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -169,7 +169,7 @@ class NginxParser(object): trees.append(parsed) except IOError: logger.warning("Could not open file: %s", item) - except pyparsing.ParseBaseException: + except pyparsing.ParseException: logger.debug("Could not parse file: %s", item) return trees From 449487e8cbcf4dce88c8ffd6a7d8f64cab3cd0c3 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 16 Aug 2016 19:34:16 -0700 Subject: [PATCH 47/51] Catch all pyparsing exceptions --- certbot-nginx/certbot_nginx/parser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index 681fadc55..90bb49aaf 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -180,7 +180,7 @@ class NginxParser(object): return nginxparser.load(_file).spaced except IOError: logger.warn("Missing NGINX TLS options file: %s", ssl_options) - except pyparsing.ParseException: + except pyparsing.ParseBaseException: logger.debug("Could not parse file: %s", ssl_options) return [] From 73fdc08d8348335a883f9a21a00ed082f59e3057 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 16 Aug 2016 21:04:28 -0700 Subject: [PATCH 48/51] don't hardcode certbot comment --- .../certbot_nginx/tests/configurator_test.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/certbot-nginx/certbot_nginx/tests/configurator_test.py b/certbot-nginx/certbot_nginx/tests/configurator_test.py index ec7408f27..9e0c0dda5 100644 --- a/certbot-nginx/certbot_nginx/tests/configurator_test.py +++ b/certbot-nginx/certbot_nginx/tests/configurator_test.py @@ -13,6 +13,7 @@ from acme import messages from certbot import achallenges from certbot import errors +from certbot_nginx import parser from certbot_nginx.tests import util @@ -91,14 +92,13 @@ 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'], - ['listen', '127.0.0.1'], - ['server_name', '.example.com'], - ['server_name', 'example.*'], - ['listen', '5001 ssl'], - ['#', ' managed by Certbot'] - ]]], + self.assertEqual([[['server'], + [['listen', '69.50.225.155:9000'], + ['listen', '127.0.0.1'], + ['server_name', '.example.com'], + ['server_name', 'example.*'], + ['listen', '5001 ssl'], + ['#', parser.COMMENT]]]], parsed[0]) def test_choose_vhost(self): From 5c16b43221a903cac615287998e4c9f6772909ac Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 17 Aug 2016 17:00:51 -0700 Subject: [PATCH 49/51] satisfy OCD by removing space --- letsencrypt-auto-source/letsencrypt-auto | 2 +- letsencrypt-auto-source/pieces/bootstrappers/rpm_common.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/letsencrypt-auto-source/letsencrypt-auto b/letsencrypt-auto-source/letsencrypt-auto index 4abe7be38..5aa49ccf2 100755 --- a/letsencrypt-auto-source/letsencrypt-auto +++ b/letsencrypt-auto-source/letsencrypt-auto @@ -285,7 +285,7 @@ BootstrapRpmCommon() { yes_flag="-y" fi - if ! $SUDO $tool list *virtualenv > /dev/null 2>&1; then + if ! $SUDO $tool list *virtualenv >/dev/null 2>&1; then echo "To use Certbot, packages from the EPEL repository need to be installed." if [ "$ASSUME_YES" = 1 ]; then /bin/echo -n "Enabling the EPEL repository in 3 seconds..." diff --git a/letsencrypt-auto-source/pieces/bootstrappers/rpm_common.sh b/letsencrypt-auto-source/pieces/bootstrappers/rpm_common.sh index e9865aed3..3c12a5f4d 100755 --- a/letsencrypt-auto-source/pieces/bootstrappers/rpm_common.sh +++ b/letsencrypt-auto-source/pieces/bootstrappers/rpm_common.sh @@ -21,7 +21,7 @@ BootstrapRpmCommon() { yes_flag="-y" fi - if ! $SUDO $tool list *virtualenv > /dev/null 2>&1; then + if ! $SUDO $tool list *virtualenv >/dev/null 2>&1; then echo "To use Certbot, packages from the EPEL repository need to be installed." if [ "$ASSUME_YES" = 1 ]; then /bin/echo -n "Enabling the EPEL repository in 3 seconds..." From 156c6415c2a6472e5e96a3e1150d399fb0b6ab74 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 17 Aug 2016 17:31:56 -0700 Subject: [PATCH 50/51] error out when we can't simply install epel-release --- letsencrypt-auto-source/letsencrypt-auto | 4 ++++ letsencrypt-auto-source/pieces/bootstrappers/rpm_common.sh | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/letsencrypt-auto-source/letsencrypt-auto b/letsencrypt-auto-source/letsencrypt-auto index 5aa49ccf2..7c851ffc4 100755 --- a/letsencrypt-auto-source/letsencrypt-auto +++ b/letsencrypt-auto-source/letsencrypt-auto @@ -287,6 +287,10 @@ BootstrapRpmCommon() { if ! $SUDO $tool list *virtualenv >/dev/null 2>&1; then echo "To use Certbot, packages from the EPEL repository need to be installed." + if ! $SUDO $tool list epel-release >/dev/null 2>&1; then + echo "Please enable this repository and try running Certbot again." + exit 1 + fi if [ "$ASSUME_YES" = 1 ]; then /bin/echo -n "Enabling the EPEL repository in 3 seconds..." sleep 1s diff --git a/letsencrypt-auto-source/pieces/bootstrappers/rpm_common.sh b/letsencrypt-auto-source/pieces/bootstrappers/rpm_common.sh index 3c12a5f4d..2fd629ff8 100755 --- a/letsencrypt-auto-source/pieces/bootstrappers/rpm_common.sh +++ b/letsencrypt-auto-source/pieces/bootstrappers/rpm_common.sh @@ -23,6 +23,10 @@ BootstrapRpmCommon() { if ! $SUDO $tool list *virtualenv >/dev/null 2>&1; then echo "To use Certbot, packages from the EPEL repository need to be installed." + if ! $SUDO $tool list epel-release >/dev/null 2>&1; then + echo "Please enable this repository and try running Certbot again." + exit 1 + fi if [ "$ASSUME_YES" = 1 ]; then /bin/echo -n "Enabling the EPEL repository in 3 seconds..." sleep 1s From df61b0e3497412238f841abb07e8ef148bbb4ba5 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Thu, 18 Aug 2016 13:56:15 -0700 Subject: [PATCH 51/51] Check for comments more accurately --- certbot-nginx/certbot_nginx/parser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot-nginx/certbot_nginx/parser.py b/certbot-nginx/certbot_nginx/parser.py index 90bb49aaf..3919858d9 100644 --- a/certbot-nginx/certbot_nginx/parser.py +++ b/certbot-nginx/certbot_nginx/parser.py @@ -533,7 +533,7 @@ def _comment_directive(block, location): """Add a comment to the end of the line at location.""" next_entry = block[location + 1] if location + 1 < len(block) else None if isinstance(next_entry, list) and next_entry: - if COMMENT in next_entry[-1]: + if len(next_entry) >= 2 and next_entry[-2] == "#" and COMMENT in next_entry[-1]: return elif isinstance(next_entry, nginxparser.UnspacedList): next_entry = next_entry.spaced[0]