From 5efdda092212a063cbddab82ef6ceefa790f1da8 Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Tue, 28 Apr 2015 11:31:04 +0000 Subject: [PATCH 1/5] Fix some of #362 nitpicks --- letsencrypt/acme/messages2.py | 4 ++-- letsencrypt/client/account.py | 9 ++++++++- letsencrypt/client/network2.py | 12 +++++------- letsencrypt/client/tests/account_test.py | 2 +- letsencrypt/client/tests/acme_util.py | 4 ++-- letsencrypt/client/tests/display/ops_test.py | 3 +-- letsencrypt/client/tests/recovery_token_test.py | 3 ++- 7 files changed, 21 insertions(+), 16 deletions(-) diff --git a/letsencrypt/acme/messages2.py b/letsencrypt/acme/messages2.py index 86a703155..147b61704 100644 --- a/letsencrypt/acme/messages2.py +++ b/letsencrypt/acme/messages2.py @@ -164,8 +164,8 @@ class ChallengeBody(ResourceBody): .. todo:: Confusingly, this has a similar name to `.challenges.Challenge`, - as well as `.achallenges.AnnotateChallenge`. Please use names - such as ``challb`` to distinguish instanced of this class from + as well as `.achallenges.AnnotatedChallenge`. Please use names + such as ``challb`` to distinguish instances of this class from ``achall``. :ivar letsencrypt.acme.challenges.Challenge: Wrapped challenge. diff --git a/letsencrypt/client/account.py b/letsencrypt/client/account.py index ed37b6446..6b1e99fc3 100644 --- a/letsencrypt/client/account.py +++ b/letsencrypt/client/account.py @@ -55,6 +55,8 @@ class Account(object): """URI link for new registrations.""" if self.regr is not None: return self.regr.uri + else: + return None @property def new_authzr_uri(self): # pylint: disable=missing-docstring @@ -65,16 +67,22 @@ class Account(object): # Default: spec says they "may" provide the header # ugh.. acme-spec #93 return "https://%s/acme/new-authz" % self.config.server + else: + return None @property def terms_of_service(self): # pylint: disable=missing-docstring if self.regr is not None: return self.regr.terms_of_service + else: + return None @property def recovery_token(self): # pylint: disable=missing-docstring if self.regr is not None and self.regr.body is not None: return self.regr.body.recovery_token + else: + return None def save(self): """Save account to disk.""" @@ -112,7 +120,6 @@ class Account(object): @classmethod def from_existing_account(cls, config, email=None): """Populate an account from an existing email.""" - config_fp = os.path.join( config.accounts_dir, cls._get_config_filename(email)) return cls._from_config_fp(config, config_fp) diff --git a/letsencrypt/client/network2.py b/letsencrypt/client/network2.py index 1793cdb1c..0599bdf5e 100644 --- a/letsencrypt/client/network2.py +++ b/letsencrypt/client/network2.py @@ -201,13 +201,10 @@ class Network(object): """ details = ( "mailto:" + account.email if account.email is not None else None, - "tel:" + account.phone if account.phone is not None else None + "tel:" + account.phone if account.phone is not None else None, ) - - contact_tuple = tuple(det for det in details if det is not None) - - account.regr = self.register(contact=contact_tuple) - + account.regr = self.register(contact=tuple( + det for det in details if det is not None)) return account def update_registration(self, regr): @@ -376,7 +373,6 @@ class Network(object): updated_authzr = self._authzr_from_response( response, authzr.body.identifier, authzr.uri, authzr.new_cert_uri) # TODO: check and raise UnexpectedUpdate - return updated_authzr, response def request_issuance(self, csr, authzrs): @@ -534,6 +530,8 @@ class Network(object): """ if certr.cert_chain_uri is not None: return self._get_cert(certr.cert_chain_uri)[1] + else: + return None def revoke(self, certr, when=messages2.Revocation.NOW): """Revoke certificate. diff --git a/letsencrypt/client/tests/account_test.py b/letsencrypt/client/tests/account_test.py index 2a675c15c..0cb3346c8 100644 --- a/letsencrypt/client/tests/account_test.py +++ b/letsencrypt/client/tests/account_test.py @@ -169,7 +169,7 @@ class SafeEmailTest(unittest.TestCase): addrs = [ "letsencrypt@letsencrypt.org", "tbd.ade@gmail.com", - "abc_def.jdk@hotmail.museum" + "abc_def.jdk@hotmail.museum", ] for addr in addrs: self.assertTrue(self._call(addr), "%s failed." % addr) diff --git a/letsencrypt/client/tests/acme_util.py b/letsencrypt/client/tests/acme_util.py index a81c68aa7..6ae6ef56e 100644 --- a/letsencrypt/client/tests/acme_util.py +++ b/letsencrypt/client/tests/acme_util.py @@ -75,7 +75,7 @@ def chall_to_challb(chall, status): # pylint: disable=redefined-outer-name """Return ChallengeBody from Challenge.""" kwargs = { "chall": chall, - "uri": chall.typ+"_uri", + "uri": chall.typ + "_uri", "status": status, } @@ -129,7 +129,7 @@ def gen_authzr(authz_status, domain, challs, statuses, combos=True): now = datetime.datetime.now() authz_kwargs.update({ "status": authz_status, - "expires": datetime.datetime(now.year, now.month+1, now.day), + "expires": datetime.datetime(now.year, now.month + 1, now.day), }) else: authz_kwargs.update({ diff --git a/letsencrypt/client/tests/display/ops_test.py b/letsencrypt/client/tests/display/ops_test.py index 9359e53d0..de5745e8e 100644 --- a/letsencrypt/client/tests/display/ops_test.py +++ b/letsencrypt/client/tests/display/ops_test.py @@ -7,10 +7,10 @@ import unittest import mock import zope.component +from letsencrypt.client import account from letsencrypt.client import le_util from letsencrypt.client.display import util as display_util - class ChooseAuthenticatorTest(unittest.TestCase): """Test choose_authenticator function.""" def setUp(self): @@ -59,7 +59,6 @@ class ChooseAuthenticatorTest(unittest.TestCase): class ChooseAccountTest(unittest.TestCase): """Test choose_account.""" def setUp(self): - from letsencrypt.client import account zope.component.provideUtility(display_util.FileDisplay(sys.stdout)) self.accounts_dir = tempfile.mkdtemp("accounts") diff --git a/letsencrypt/client/tests/recovery_token_test.py b/letsencrypt/client/tests/recovery_token_test.py index 4d47f9fbe..0de31a8d0 100644 --- a/letsencrypt/client/tests/recovery_token_test.py +++ b/letsencrypt/client/tests/recovery_token_test.py @@ -49,7 +49,8 @@ class RecoveryTokenTest(unittest.TestCase): # SHOULD throw an error (OSError other than nonexistent file) self.assertRaises( OSError, self.rec_token.cleanup, - achallenges.RecoveryToken(challb=None, domain="a"+"r"*10000+".com")) + achallenges.RecoveryToken( + challb=None, domain=("a" + "r" * 10000 + ".com"))) def test_perform_stored(self): self.rec_token.store_token("example4.com", 444) From ff569084f86ffdced5a4f96436628a89f804b1c9 Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Wed, 29 Apr 2015 08:19:08 +0000 Subject: [PATCH 2/5] Fix empty email problem, EMAIL_REGEX = re.compile(...), pep8 --- letsencrypt/client/account.py | 18 ++++++++++-------- letsencrypt/client/tests/account_test.py | 16 ++++++++++++++-- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/letsencrypt/client/account.py b/letsencrypt/client/account.py index 6b1e99fc3..ef8831fa5 100644 --- a/letsencrypt/client/account.py +++ b/letsencrypt/client/account.py @@ -35,7 +35,7 @@ class Account(object): # Just make sure we don't get pwned # Make sure that it also doesn't start with a period or have two consecutive # periods <- this needs to be done in addition to the regex - EMAIL_REGEX = "[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+$" + EMAIL_REGEX = re.compile("[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+$") def __init__(self, config, key, email=None, phone=None, regr=None): le_util.make_or_verify_dir( @@ -192,13 +192,14 @@ class Account(object): "Enter email address (optional, press Enter to skip)") if code == display_util.OK: - if email == "" or cls.safe_email(email): - email = email if email != "" else None + if not email or cls.safe_email(email): + email = email if email else None le_util.make_or_verify_dir( config.account_keys_dir, 0o700, os.geteuid()) key = crypto_util.init_save_key( - config.rsa_key_size, config.account_keys_dir, email) + config.rsa_key_size, config.account_keys_dir, + cls._get_config_filename(email)) return cls(config, key, email) else: return None @@ -206,7 +207,8 @@ class Account(object): @classmethod def safe_email(cls, email): """Scrub email address before using it.""" - if re.match(cls.EMAIL_REGEX, email): - return bool(not email.startswith(".") and ".." not in email) - logging.warn("Invalid email address.") - return False + if cls.EMAIL_REGEX.match(email): + return not email.startswith(".") and ".." not in email + else: + logging.warn("Invalid email address.") + return False diff --git a/letsencrypt/client/tests/account_test.py b/letsencrypt/client/tests/account_test.py index 0cb3346c8..269328f27 100644 --- a/letsencrypt/client/tests/account_test.py +++ b/letsencrypt/client/tests/account_test.py @@ -64,14 +64,26 @@ class AccountTest(unittest.TestCase): zope.component.provideUtility(displayer) mock_util().input.return_value = (display_util.OK, self.email) - mock_key.return_value = self.key - acc = account.Account.from_prompts(self.config) + acc = account.Account.from_prompts(self.config) self.assertEqual(acc.email, self.email) self.assertEqual(acc.key, self.key) self.assertEqual(acc.config, self.config) + @mock.patch("letsencrypt.client.account.zope.component.getUtility") + @mock.patch("letsencrypt.client.account.crypto_util.init_save_key") + def test_prompts_empty_email(self, mock_key, mock_util): + displayer = display_util.FileDisplay(sys.stdout) + zope.component.provideUtility(displayer) + + mock_util().input.return_value = (display_util.OK, "") + acc = account.Account.from_prompts(self.config) + self.assertTrue(acc.email is None) + # _get_config_filename | pylint: disable=protected-access + mock_key.assert_called_once_with( + mock.ANY, mock.ANY, acc._get_config_filename(None)) + @mock.patch("letsencrypt.client.account.zope.component.getUtility") def test_prompts_cancel(self, mock_util): # displayer = display_util.FileDisplay(sys.stdout) From 79b0ed5cd3f55c8f1daf07502d3413df7a23782c Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Wed, 29 Apr 2015 08:23:42 +0000 Subject: [PATCH 3/5] log act_cert_path --- letsencrypt/client/client.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/letsencrypt/client/client.py b/letsencrypt/client/client.py index 183cd6f94..6e98a92bc 100644 --- a/letsencrypt/client/client.py +++ b/letsencrypt/client/client.py @@ -165,8 +165,8 @@ class Client(object): cert_file.write(certr.body.as_pem()) finally: cert_file.close() - logging.info( - "Server issued certificate; certificate written to %s", cert_path) + logging.info("Server issued certificate; certificate written to %s", + act_cert_path) if certr.cert_chain_uri: # TODO: Except From 3ba8acc57e37489ce62e0854de3d5159fe6e6981 Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Wed, 29 Apr 2015 09:15:31 +0000 Subject: [PATCH 4/5] Ref to letsencrypt/boulder#128 --- letsencrypt/acme/messages2.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/acme/messages2.py b/letsencrypt/acme/messages2.py index 147b61704..463198d5e 100644 --- a/letsencrypt/acme/messages2.py +++ b/letsencrypt/acme/messages2.py @@ -18,7 +18,7 @@ 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 + # 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') From 18a1d01d8f493a8ed577f95959b393a43a0ee56e Mon Sep 17 00:00:00 2001 From: Jakub Warmuz Date: Wed, 29 Apr 2015 19:49:24 +0000 Subject: [PATCH 5/5] Ref to letsencrypt/boulder#130 --- letsencrypt/client/network2.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/client/network2.py b/letsencrypt/client/network2.py index 0599bdf5e..abe48adb5 100644 --- a/letsencrypt/client/network2.py +++ b/letsencrypt/client/network2.py @@ -320,7 +320,7 @@ class Network(object): except KeyError: # TODO: Right now Boulder responds with the authorization resource # instead of a challenge resource... this can be uncommented - # once the error is fixed. + # once the error is fixed (boulder#130). return None # raise errors.NetworkError('"up" Link header missing') challr = messages2.ChallengeResource(