From 24f9da527542a9d88a1c12b407f6885c14666b42 Mon Sep 17 00:00:00 2001 From: yan Date: Mon, 18 May 2015 14:31:47 -0400 Subject: [PATCH 1/8] Add support and tests for some Nginx config edge cases 1. Match "if" statements 2. Allow special characters in nginx directives when enclosed in single or double quotes. --- letsencrypt_nginx/nginxparser.py | 10 ++++--- letsencrypt_nginx/tests/nginxparser_test.py | 20 ++++++++++++++ .../tests/testdata/etc_nginx/edge_cases.conf | 27 +++++++++++++++++++ 3 files changed, 54 insertions(+), 3 deletions(-) create mode 100644 letsencrypt_nginx/tests/testdata/etc_nginx/edge_cases.conf diff --git a/letsencrypt_nginx/nginxparser.py b/letsencrypt_nginx/nginxparser.py index 18ba8b0bd..f24455d59 100644 --- a/letsencrypt_nginx/nginxparser.py +++ b/letsencrypt_nginx/nginxparser.py @@ -3,7 +3,7 @@ import string from pyparsing import ( Literal, White, Word, alphanums, CharsNotIn, Forward, Group, - Optional, OneOrMore, ZeroOrMore, pythonStyleComment) + Optional, OneOrMore, Regex, ZeroOrMore, pythonStyleComment) class RawNginxParser(object): @@ -16,17 +16,21 @@ class RawNginxParser(object): semicolon = Literal(";").suppress() space = White().suppress() key = Word(alphanums + "_/") - value = CharsNotIn("{};,") + # Matches anything that is not a special character AND any chars in single + # or double quotes + value = Regex(r"((\".*\")?(\'.*\')?[^\{\};,]?)+") location = CharsNotIn("{};," + string.whitespace) # modifier for location uri [ = | ~ | ~* | ^~ ] modifier = Literal("=") | Literal("~*") | Literal("~") | Literal("^~") # rules assignment = (key + Optional(space + value) + semicolon) + location_statement = Optional(space + modifier) + Optional(space + location) + if_statement = Literal("if") + space + Regex(r"\(.+\)") + space block = Forward() block << Group( - Group(key + Optional(space + modifier) + Optional(space + location)) + (Group(key + location_statement) ^ Group(if_statement)) + left_bracket + Group(ZeroOrMore(Group(assignment) | block)) + right_bracket) diff --git a/letsencrypt_nginx/tests/nginxparser_test.py b/letsencrypt_nginx/tests/nginxparser_test.py index 23f46c72a..73a89534b 100644 --- a/letsencrypt_nginx/tests/nginxparser_test.py +++ b/letsencrypt_nginx/tests/nginxparser_test.py @@ -84,6 +84,26 @@ class TestRawNginxParser(unittest.TestCase): ]]]]] ) + def test_parse_from_file2(self): + parsed = load(open(util.get_data_filename('edge_cases.conf'))) + self.assertEqual( + parsed, + [[['server'], [['server_name', 'simple']]], + [['server'], + [['server_name', 'with.if'], + [['location', '~', '^/services/.+$'], + [[['if', '($request_filename ~* \\.(ttf|woff)$)'], + [['add_header', 'Access-Control-Allow-Origin "*"']]]]]]], + [['server'], + [['server_name', 'with.complicated.headers'], + [['location', '~*', '\\.(?:gif|jpe?g|png)$'], + [['add_header', 'Pragma public'], + ['add_header', + 'Cache-Control \'public, must-revalidate, proxy-revalidate\'' + ' "test,;{}" foo'], + ['blah', '"hello;world"'], + ['try_files', '$uri @rewrites']]]]]]) + def test_dump_as_file(self): parsed = load(open(util.get_data_filename('nginx.conf'))) parsed[-1][-1].append([['server'], diff --git a/letsencrypt_nginx/tests/testdata/etc_nginx/edge_cases.conf b/letsencrypt_nginx/tests/testdata/etc_nginx/edge_cases.conf new file mode 100644 index 000000000..477cb1c45 --- /dev/null +++ b/letsencrypt_nginx/tests/testdata/etc_nginx/edge_cases.conf @@ -0,0 +1,27 @@ +# This is not a valid nginx config file but it tests edge cases in valid nginx syntax + +server { + server_name simple; +} + +server { + server_name with.if; + location ~ ^/services/.+$ { + if ($request_filename ~* \.(ttf|woff)$) { + add_header Access-Control-Allow-Origin "*"; + } + } +} + +server { + server_name with.complicated.headers; + + location ~* \.(?:gif|jpe?g|png)$ { + + add_header Pragma public; + add_header Cache-Control 'public, must-revalidate, proxy-revalidate' "test,;{}" foo; + blah "hello;world"; + + try_files $uri @rewrites; + } +} From c0acf8239db8027ac1d3a8c964530661a21dc6ae Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Tue, 19 May 2015 19:19:29 +0000 Subject: [PATCH 2/8] network2: Log GET/POST uri --- letsencrypt/network2.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/letsencrypt/network2.py b/letsencrypt/network2.py index e66afb3a6..b9a34b10f 100644 --- a/letsencrypt/network2.py +++ b/letsencrypt/network2.py @@ -115,6 +115,7 @@ class Network(object): :rtype: `requests.Response` """ + logging.debug('Sending GET request to %s', uri) try: response = requests.get(uri, **kwargs) except requests.exceptions.RequestException as error: @@ -133,7 +134,7 @@ class Network(object): :rtype: `requests.Response` """ - logging.debug('Sending POST data: %s', data) + logging.debug('Sending POST data to %s: %s', uri, data) try: response = requests.post(uri, data=data, **kwargs) except requests.exceptions.RequestException as error: From 2cadfdaae1e7d72888ea5bf957f48163107197e1 Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Tue, 19 May 2015 19:21:04 +0000 Subject: [PATCH 3/8] response might carry binary data, use repr() in network2 logging --- letsencrypt/network2.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/network2.py b/letsencrypt/network2.py index b9a34b10f..b82b05bbe 100644 --- a/letsencrypt/network2.py +++ b/letsencrypt/network2.py @@ -139,7 +139,7 @@ class Network(object): response = requests.post(uri, data=data, **kwargs) except requests.exceptions.RequestException as error: raise errors.NetworkError(error) - logging.debug('Received response %s: %s', response, response.text) + logging.debug('Received response %s: %r', response, response.text) self._check_response(response, content_type=content_type) return response From 41115bfc770819d310e39f797a97feaddac09fc8 Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Tue, 19 May 2015 19:21:19 +0000 Subject: [PATCH 4/8] Spec and Boulder compatibility fixes. Relevant acme-spec: - https://github.com/letsencrypt/acme-spec/issues/127 - https://github.com/letsencrypt/acme-spec/pull/119 - https://github.com/letsencrypt/acme-spec/issues/98 - https://github.com/letsencrypt/acme-spec/issues/92 Relevant boulder: - https://github.com/letsencrypt/boulder/pull/170 - https://github.com/letsencrypt/boulder/issues/128 --- acme/messages2.py | 6 ------ letsencrypt/network2.py | 4 ++-- letsencrypt/tests/auth_handler_test.py | 2 -- letsencrypt/tests/network2_test.py | 16 ++++++++++------ 4 files changed, 12 insertions(+), 16 deletions(-) diff --git a/acme/messages2.py b/acme/messages2.py index 419bb0b4e..ac26d2e97 100644 --- a/acme/messages2.py +++ b/acme/messages2.py @@ -18,11 +18,9 @@ class Error(jose.JSONObjectWithFields, Exception): 'badCSR': 'The CSR is unacceptable (e.g., due to a short key)', } - # TODO: Boulder omits 'type' and 'instance', spec requires, boulder#128 typ = jose.Field('type', omitempty=True) title = jose.Field('title', omitempty=True) detail = jose.Field('detail') - instance = jose.Field('instance', omitempty=True) @typ.encoder def typ(value): # pylint: disable=missing-docstring,no-self-argument @@ -227,10 +225,6 @@ class Authorization(ResourceBody): challenges = jose.Field('challenges', omitempty=True) combinations = jose.Field('combinations', omitempty=True) - # TODO: acme-spec #92, #98 - key = Registration._fields['key'] - contact = Registration._fields['contact'] - status = jose.Field('status', omitempty=True, decoder=Status.from_json) # TODO: 'expires' is allowed for Authorization Resources in # general, but for Key Authorization '[t]he "expires" field MUST diff --git a/letsencrypt/network2.py b/letsencrypt/network2.py index b82b05bbe..28cb702a3 100644 --- a/letsencrypt/network2.py +++ b/letsencrypt/network2.py @@ -248,6 +248,7 @@ class Network(object): def _authzr_from_response(self, response, identifier, uri=None, new_cert_uri=None): + # pylint: disable=no-self-use if new_cert_uri is None: try: new_cert_uri = response.links['next']['url'] @@ -258,8 +259,7 @@ class Network(object): body=messages2.Authorization.from_json(response.json()), uri=response.headers.get('Location', uri), new_cert_uri=new_cert_uri) - if (authzr.body.key != self.key.public() - or authzr.body.identifier != identifier): + if authzr.body.identifier != identifier: raise errors.UnexpectedUpdate(authzr) return authzr diff --git a/letsencrypt/tests/auth_handler_test.py b/letsencrypt/tests/auth_handler_test.py index f7c7a888f..85bcfe8cf 100644 --- a/letsencrypt/tests/auth_handler_test.py +++ b/letsencrypt/tests/auth_handler_test.py @@ -276,8 +276,6 @@ class PollChallengesTest(unittest.TestCase): identifier=authzr.body.identifier, challenges=new_challbs, combinations=authzr.body.combinations, - key=authzr.body.key, - contact=authzr.body.contact, status=status_, ), ) diff --git a/letsencrypt/tests/network2_test.py b/letsencrypt/tests/network2_test.py index d7f50328a..bfe3e89b4 100644 --- a/letsencrypt/tests/network2_test.py +++ b/letsencrypt/tests/network2_test.py @@ -72,7 +72,7 @@ class NetworkTest(unittest.TestCase): self.authz = messages2.Authorization( identifier=messages2.Identifier( typ=messages2.IDENTIFIER_FQDN, value='example.com'), - challenges=(challb,), combinations=None, key=KEY.public()) + challenges=(challb,), combinations=None) self.authzr = messages2.AuthorizationResource( body=self.authz, uri=authzr_uri, new_cert_uri='https://www.letsencrypt-demo.org/acme/new-cert') @@ -258,11 +258,10 @@ class NetworkTest(unittest.TestCase): # TODO: test POST call arguments # TODO: split here and separate test - authz_wrong_key = self.authz.update(key=KEY2.public()) - self.response.json.return_value = authz_wrong_key.to_json() - self.assertRaises( - errors.UnexpectedUpdate, self.net.request_challenges, - self.identifier, self.regr) + self.response.json.return_value = self.authz.update( + identifier=self.identifier.update(value='foo')).to_json() + self.assertRaises(errors.UnexpectedUpdate, self.net.request_challenges, + self.identifier, self.authzr.uri) def test_request_challenges_missing_next(self): self.response.status_code = httplib.CREATED @@ -336,6 +335,11 @@ class NetworkTest(unittest.TestCase): self.assertEqual((self.authzr, self.response), self.net.poll(self.authzr)) + # TODO: split here and separate test + self.response.json.return_value = self.authz.update( + identifier=self.identifier.update(value='foo')).to_json() + self.assertRaises(errors.UnexpectedUpdate, self.net.poll, self.authzr) + def test_request_issuance(self): self.response.content = CERT.as_der() self.response.headers['Location'] = self.certr.uri From 0018bc05006fe69b1556ca2d91dbcadf76f1f43a Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Tue, 19 May 2015 19:50:00 +0000 Subject: [PATCH 5/8] Error: typ/title no omitempty --- acme/messages2.py | 4 ++-- acme/messages2_test.py | 2 +- letsencrypt/tests/network2_test.py | 3 ++- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/acme/messages2.py b/acme/messages2.py index ac26d2e97..3db14aa2e 100644 --- a/acme/messages2.py +++ b/acme/messages2.py @@ -18,8 +18,8 @@ class Error(jose.JSONObjectWithFields, Exception): 'badCSR': 'The CSR is unacceptable (e.g., due to a short key)', } - typ = jose.Field('type', omitempty=True) - title = jose.Field('title', omitempty=True) + typ = jose.Field('type') + title = jose.Field('title') detail = jose.Field('detail') @typ.encoder diff --git a/acme/messages2_test.py b/acme/messages2_test.py index 5f9441b4f..d10d57973 100644 --- a/acme/messages2_test.py +++ b/acme/messages2_test.py @@ -21,7 +21,7 @@ class ErrorTest(unittest.TestCase): def setUp(self): from acme.messages2 import Error - self.error = Error(detail='foo', typ='malformed') + self.error = Error(detail='foo', typ='malformed', title='title') def test_typ_prefix(self): self.assertEqual('malformed', self.error.typ) diff --git a/letsencrypt/tests/network2_test.py b/letsencrypt/tests/network2_test.py index bfe3e89b4..3df5b6dab 100644 --- a/letsencrypt/tests/network2_test.py +++ b/letsencrypt/tests/network2_test.py @@ -114,7 +114,8 @@ class NetworkTest(unittest.TestCase): def test_check_response_not_ok_jobj_error(self): self.response.ok = False - self.response.json.return_value = messages2.Error(detail='foo') + self.response.json.return_value = messages2.Error( + detail='foo', typ='serverInternal', title='some title').to_json() # pylint: disable=protected-access self.assertRaises( messages2.Error, self.net._check_response, self.response) From cd6b9bc9c72adb75bd05e1f59f72660d4395f343 Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Tue, 19 May 2015 20:09:11 +0000 Subject: [PATCH 6/8] Fix coverage for acme.messages2.Error --- acme/messages2_test.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/acme/messages2_test.py b/acme/messages2_test.py index d10d57973..62de0832c 100644 --- a/acme/messages2_test.py +++ b/acme/messages2_test.py @@ -22,6 +22,7 @@ class ErrorTest(unittest.TestCase): def setUp(self): from acme.messages2 import Error self.error = Error(detail='foo', typ='malformed', title='title') + self.jobj = {'detail': 'foo', 'title': 'some title'} def test_typ_prefix(self): self.assertEqual('malformed', self.error.typ) @@ -32,15 +33,15 @@ class ErrorTest(unittest.TestCase): def test_typ_decoder_missing_prefix(self): from acme.messages2 import Error - self.assertRaises(jose.DeserializationError, Error.from_json, - {'detail': 'foo', 'type': 'malformed'}) - self.assertRaises(jose.DeserializationError, Error.from_json, - {'detail': 'foo', 'type': 'not valid bare type'}) + self.jobj['type'] = 'malfomed' + self.assertRaises(jose.DeserializationError, Error.from_json, self.jobj) + self.jobj['type'] = 'not balid bare type' + self.assertRaises(jose.DeserializationError, Error.from_json, self.jobj) def test_typ_decoder_not_recognized(self): from acme.messages2 import Error - self.assertRaises(jose.DeserializationError, Error.from_json, - {'detail': 'foo', 'type': 'urn:acme:error:baz'}) + self.jobj['type'] = 'urn:acme:error:baz' + self.assertRaises(jose.DeserializationError, Error.from_json, self.jobj) def test_description(self): self.assertEqual( From ac0868b6ded3ab4ce47b56bc5b0e1ff2e1750f19 Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Tue, 19 May 2015 20:13:55 +0000 Subject: [PATCH 7/8] acme.messages2.Error title is omitempty --- acme/messages2.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acme/messages2.py b/acme/messages2.py index 3db14aa2e..253aaa95b 100644 --- a/acme/messages2.py +++ b/acme/messages2.py @@ -19,7 +19,7 @@ class Error(jose.JSONObjectWithFields, Exception): } typ = jose.Field('type') - title = jose.Field('title') + title = jose.Field('title', omitempty=True) detail = jose.Field('detail') @typ.encoder From 58156a29d376a83b01be8256894f277d5e5f5b0e Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Tue, 19 May 2015 17:06:06 -0700 Subject: [PATCH 8/8] Fix typos --- acme/messages2_test.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/acme/messages2_test.py b/acme/messages2_test.py index 62de0832c..c1521e2c3 100644 --- a/acme/messages2_test.py +++ b/acme/messages2_test.py @@ -33,9 +33,9 @@ class ErrorTest(unittest.TestCase): def test_typ_decoder_missing_prefix(self): from acme.messages2 import Error - self.jobj['type'] = 'malfomed' + self.jobj['type'] = 'malformed' self.assertRaises(jose.DeserializationError, Error.from_json, self.jobj) - self.jobj['type'] = 'not balid bare type' + self.jobj['type'] = 'not valid bare type' self.assertRaises(jose.DeserializationError, Error.from_json, self.jobj) def test_typ_decoder_not_recognized(self):