From 7a4c7acdfb1afa534f310078ad1ffba4ecb60947 Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Wed, 1 Apr 2015 07:56:38 +0000 Subject: [PATCH] Fix review comments --- docs/api/acme/index.rst | 2 + letsencrypt/acme/messages2.py | 94 ++++++++++++++--------- letsencrypt/client/network2.py | 46 ++++++++--- letsencrypt/client/tests/network2_test.py | 13 +++- 4 files changed, 105 insertions(+), 50 deletions(-) diff --git a/docs/api/acme/index.rst b/docs/api/acme/index.rst index 9eb93ec6c..20206183a 100644 --- a/docs/api/acme/index.rst +++ b/docs/api/acme/index.rst @@ -1,6 +1,8 @@ :mod:`letsencrypt.acme` ======================= +.. contents:: + .. automodule:: letsencrypt.acme :members: diff --git a/letsencrypt/acme/messages2.py b/letsencrypt/acme/messages2.py index ecb0d9868..f4c1e9dce 100644 --- a/letsencrypt/acme/messages2.py +++ b/letsencrypt/acme/messages2.py @@ -43,7 +43,8 @@ class Error(jose.JSONObjectWithFields, Exception): return without_prefix @property - def description(self): # pylint: disable=missing-docstring,no-self-argument + def description(self): + """Hardcoded error description based on its type.""" return self.ERROR_TYPE_DESCRIPTIONS[self.typ] @@ -91,7 +92,11 @@ IDENTIFIER_FQDN = IdentifierType('dns') # IdentifierDNS in Boulder class Identifier(jose.JSONObjectWithFields): - """ACME identifier.""" + """ACME identifier. + + :ivar letsencrypt.acme.messages2.IdentifierType typ: + + """ typ = jose.Field('type', decoder=IdentifierType.from_json) value = jose.Field('value') @@ -99,32 +104,35 @@ class Identifier(jose.JSONObjectWithFields): class Resource(jose.ImmutableMap): """ACME Resource. - :param body: Resource body. - :type body: Instance of `ResourceBody` (subclass). - - :param str uri: Location of the resource. + :ivar letsencrypt.acme.messages2.ResourceBody body: Resource body. + :ivar str uri: Location of the resource. """ __slots__ = ('body', 'uri') class ResourceBody(jose.JSONObjectWithFields): - """ACME Resource body.""" + """ACME Resource Body.""" class RegistrationResource(Resource): - """Registration resource. + """Registration Resource. - :ivar body: `Registration` - :ivar str uri: URI of the resource. - :ivar new_authzr_uri: URI found in the 'next' Link header + :ivar letsencrypt.acme.messages2.Registration body: + :ivar str new_authzr_uri: URI found in the 'next' ``Link`` header + :ivar str terms_of_service: URL for the CA TOS. """ __slots__ = ('body', 'uri', 'new_authzr_uri', 'terms_of_service') class Registration(ResourceBody): - """Registration resource body.""" + """Registration Resource Body. + + :ivar letsencrypt.acme.jose.jwk.JWK key: Public key. + :ivar tuple contact: + + """ # on new-reg key server ignores 'key' and populates it based on # JWS.signature.combined.jwk @@ -135,10 +143,10 @@ class Registration(ResourceBody): class ChallengeResource(Resource, jose.JSONObjectWithFields): - """Challenge resource. + """Challenge Resource. - :ivar body: `.challenges.ChallengeBody` - :ivar authzr_uri: URI found in the 'up' Link header. + :ivar letsencrypt.acme.messages2.ChallengeBody body: + :ivar str authzr_uri: URI found in the 'up' ``Link`` header. """ __slots__ = ('body', 'authzr_uri') @@ -151,18 +159,21 @@ class ChallengeResource(Resource, jose.JSONObjectWithFields): class ChallengeBody(ResourceBody): - """Challenge resource body. - - Confusingly, this has a similar name to `.challenges.Challenge`, as - well as `.achallanges.AnnotatedChallenge` or - `.achallanges.IndexedChallenge`. Use names such as ``challb`` to - distinguish instances of this class from ``achall`` or ``ichall``. + """Challenge Resource Body. .. todo:: - This class could be integrated with challenges.Challenge, but - this way it would be confusing when compared to acme-spec, where - all challenges are presented without 'uri', 'status', or - 'validated' fields. + Confusingly, this has a similar name to `.challenges.Challenge`, + as well as `.achallenges.AnnotatedChallenge` or + `.achallenges.Indexed`... Once `messages2` and `network2` is + integrated with the rest of the client, this class functionality + will be merged with `.challenges.Challenge`. Meanwhile, + separation allows the ``master`` to be still interoperable with + Node.js server (protocol v00). For the time being use names such + as ``challb`` to distinguish instances of this class from + ``achall`` or ``ichall``. + + :ivar letsencrypt.acme.messages2.Status status: + :ivar datetime.datetime validated: """ @@ -184,19 +195,26 @@ class ChallengeBody(ResourceBody): class AuthorizationResource(Resource): - """Authorization resource. + """Authorization Resource. - :ivar body: `Authorization` - :ivar new_cert_uri: URI found in the 'next' Link header + :ivar letsencrypt.acme.messages2.Authorization body: + :ivar str new_cert_uri: URI found in the 'next' ``Link`` header """ __slots__ = ('body', 'uri', 'new_cert_uri') class Authorization(ResourceBody): - """Authorization resource body. + """Authorization Resource Body. - :ivar challenges: `list` of `Challenge` + :ivar letsencrypt.acme.messages2.Identifier identifier: + :ivar list challenges: `list` of `Challenge` + :ivar tuple combinations: Challenge combinations (`tuple` of `tuple` + of `int`, as opposed to `list` of `list` from the spec). + :ivar letsencrypt.acme.jose.jwk.JWK key: Public key. + :ivar tuple contact: + :ivar letsencrypt.acme.messages2.Status status: + :ivar datetime.datetime expires: """ @@ -229,7 +247,9 @@ class Authorization(ResourceBody): class CertificateRequest(jose.JSONObjectWithFields): """ACME new-cert request. - :ivar csr: `M2Crypto.X509.Request` wrapped in `.ComparableX509` + :ivar letsencrypt.acme.jose.util.ComparableX509 csr: + `M2Crypto.X509.Request` wrapped in `.ComparableX509` + :ivar tuple authorizations: `tuple` of URIs (`str`) """ csr = jose.Field('csr', decoder=jose.decode_csr, encoder=jose.encode_csr) @@ -237,11 +257,12 @@ class CertificateRequest(jose.JSONObjectWithFields): class CertificateResource(Resource): - """Authorization resource. + """Certificate Resource. - :ivar body: `M2Crypto.X509.X509` wrapped in `.ComparableX509` - :ivar cert_chain_uri: URI found in the 'up' Link header - :ivar authzrs: `list` of `AuthorizationResource`. + :ivar letsencrypt.acme.jose.util.ComparableX509 body: + `M2Crypto.X509.X509` wrapped in `.ComparableX509` + :ivar str cert_chain_uri: URI found in the 'up' ``Link`` header + :ivar tuple authzrs: `tuple` of `AuthorizationResource`. """ __slots__ = ('body', 'uri', 'cert_chain_uri', 'authzrs') @@ -250,7 +271,8 @@ class CertificateResource(Resource): class Revocation(jose.JSONObjectWithFields): """Revocation message. - :ivar revoke: Either a `datetime.datetime` or `NOW`. + :ivar revoke: Either a `datetime.datetime` or `Revocation.NOW`. + :ivar tuple authorizations: Same as `CertificateRequest.authorizations` """ diff --git a/letsencrypt/client/network2.py b/letsencrypt/client/network2.py index 13c3e8149..c2f535096 100644 --- a/letsencrypt/client/network2.py +++ b/letsencrypt/client/network2.py @@ -23,13 +23,17 @@ requests.packages.urllib3.contrib.pyopenssl.inject_into_urllib3() class Network(object): """ACME networking. + .. todo:: + Clean up raised error types hierarchy, document, and handle (wrap) + instances of `.DeserializationError` raised in `from_json()``. + :ivar str new_reg_uri: Location of new-reg :ivar key: `.JWK` (private) :ivar alg: `.JWASignature` """ - DER_CONTENT_TYPE = 'application/plix-cert' + DER_CONTENT_TYPE = 'application/pkix-cert' JSON_CONTENT_TYPE = 'application/json' JSON_ERROR_CONTENT_TYPE = 'application/problem+json' @@ -58,6 +62,16 @@ class Network(object): HTTP header is ignored if response is an expected JSON object (c.f. Boulder #56). + :param str content_type: Expected Content-Type response header. + If JSON is expected and not present in server response, this + function will raise an error. Otherwise, wrong Content-Type + is ignored, but logged. + + :raises letsencrypt.messages2.Error: If server response body + carries HTTP Problem (draft-ietf-appsawg-http-problem-00). + :raises letsencrypt.errors.NetworkError: In case of other + networking errors. + """ response_ct = response.headers.get('Content-Type') @@ -222,9 +236,12 @@ class Network(object): :param identifier: Identifier to be challenged. :type identifier: `.messages2.Identifier` - :pram regr: Registration resource. + :param regr: Registration Resource. :type regr: `.RegistrationResource` + :returns: Authorization Resource. + :rtype: `.AuthorizationResource` + """ new_authz = messages2.Authorization(identifier=identifier) response = self._post(regr.new_authzr_uri, self._wrap_in_jws(new_authz)) @@ -232,7 +249,15 @@ class Network(object): return self._authzr_from_response(response, identifier) def request_domain_challenges(self, domain, regr): - """Request challenges for domain names.""" + """Request challenges for domain names. + + This is simply a convenience function that wraps around + `request_challenges`, but works with domain names instead of + generic identifiers. + + :param str domain: Domain name to be challenged. + + """ return self.request_challenges(messages2.Identifier( typ=messages2.IDENTIFIER_FQDN, value=domain), regr) @@ -245,7 +270,7 @@ class Network(object): :param response: Corresponding Challenge response :type response: `.challenges.ChallengeResponse` - :returns: Challenge resource with updated body. + :returns: Challenge Resource with updated body. :rtype: `.ChallengeResource` :raises errors.UnexpectedUpdate: @@ -345,10 +370,7 @@ class Network(object): content_type=content_type, headers={'Accept': content_type}) - try: - cert_chain_uri = response.links['up']['url'] - except KeyError: - raise errors.NetworkError('"up" Link missing') + cert_chain_uri = response.links.get('up', {}).get('url') try: uri = response.headers['Location'] @@ -451,6 +473,9 @@ class Network(object): :rtype: `.CertificateResource` """ + # TODO: If a client sends a refresh request and the server is + # not willing to refresh the certificate, the server MUST + # respond with status code 403 (Forbidden) return self.check_cert(certr) def fetch_chain(self, certr): @@ -459,11 +484,12 @@ class Network(object): :param certr: Certificate Resource :type certr: `.CertificateResource` - :returns: Certificate chain + :returns: Certificate chain, or `None` if no "up" Link was provided. :rtype: `M2Crypto.X509.X509` wrapped in `.ComparableX509` """ - return self._get_cert(certr.cert_chain_uri) + if certr.cert_chain_uri is not None: + return self._get_cert(certr.cert_chain_uri) def revoke(self, certr, when=messages2.Revocation.NOW): """Revoke certificate. diff --git a/letsencrypt/client/tests/network2_test.py b/letsencrypt/client/tests/network2_test.py index d7aa74929..c2a7d877a 100644 --- a/letsencrypt/client/tests/network2_test.py +++ b/letsencrypt/client/tests/network2_test.py @@ -317,13 +317,14 @@ class NetworkTest(unittest.TestCase): # TODO: check POST args def test_request_issuance_missing_up(self): + self.response.content = CERT.as_der() + self.response.headers['Location'] = self.certr.uri self._mock_post_get() - self.assertRaises( - errors.NetworkError, self.net.request_issuance, - CSR, (self.authzr,)) + self.assertEqual( + self.certr.update(cert_chain_uri=None), + self.net.request_issuance(CSR, (self.authzr,))) def test_request_issuance_missing_location(self): - self.response.links['up'] = {'url': self.certr.cert_chain_uri} self._mock_post_get() self.assertRaises( errors.NetworkError, self.net.request_issuance, @@ -437,6 +438,10 @@ class NetworkTest(unittest.TestCase): self.assertEqual(self.net._get_cert(self.certr.cert_chain_uri), self.net.fetch_chain(self.certr)) + def test_fetch_chain_no_up_link(self): + self.assertTrue(self.net.fetch_chain(self.certr.update( + cert_chain_uri=None)) is None) + def test_revoke(self): self._mock_post_get() self.net.revoke(self.certr, when=messages2.Revocation.NOW)