From 2409aa647510e9e23e8cea9b51e083e23336fd79 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Fri, 30 Jan 2015 16:08:18 -0800 Subject: [PATCH 01/45] Initial commit for standalone authenticator branch --- letsencrypt/client/CONFIG.py | 3 + .../client/standalone_authenticator.py | 503 ++++++++++++++++++ 2 files changed, 506 insertions(+) create mode 100644 letsencrypt/client/standalone_authenticator.py diff --git a/letsencrypt/client/CONFIG.py b/letsencrypt/client/CONFIG.py index 5a07a4aa2..2f9428995 100644 --- a/letsencrypt/client/CONFIG.py +++ b/letsencrypt/client/CONFIG.py @@ -75,6 +75,9 @@ S_SIZE = 32 NONCE_SIZE = 16 """byte size of Nonce""" +PORT = 443 +"""TCP port on which to perform (standalone) challenge""" + # Key Sizes RSA_KEY_SIZE = 2048 """Key size""" diff --git a/letsencrypt/client/standalone_authenticator.py b/letsencrypt/client/standalone_authenticator.py new file mode 100644 index 000000000..17cddb5a3 --- /dev/null +++ b/letsencrypt/client/standalone_authenticator.py @@ -0,0 +1,503 @@ +#!/usr/bin/env python +# -*- coding: utf-8 -*- + +"""An authenticator that doesn't rely on any existing server program, +but instead creates its own ephemeral TCP listener on the specified port +in order to respond to incoming DVSNI challenges from the certificate +authority.""" + +import zope.interface +import zope.component +from letsencrypt.client import CONFIG +from letsencrypt.client import interfaces +from letsencrypt.client.challenge_util import DvsniChall +from letsencrypt.client.challenge_util import dvsni_gen_cert +import os +import sys +import signal +import time +import socket +import struct +import Crypto.Random +import M2Crypto.X509 +import OpenSSL.crypto +import OpenSSL.SSL + + +def unpack_2bytes(two_bytes): + """Interpret a two-byte string as an integer. E.g. 't_' -> 29791.""" + assert len(two_bytes) == 2 + return struct.unpack(">H", two_bytes)[0] + + +def unpack_3bytes(three_bytes): + """Interpret a three-byte string as an integer. E.g. '0M~' -> 3165566.""" + assert len(three_bytes) == 3 + return struct.unpack(">I", chr(0) + three_bytes)[0] + + +def pack_2bytes(value): + """Interpret an integer less than 65536 as a two-byte string. E.g. + 29791 -> 't_'.""" + assert value < 65536 + return struct.pack(">H", value) + + +def pack_3bytes(value): + """Interpret an integer less than 16777216 as a three-byte string. + E.g. '0M~' -> 3165566.""" + assert value < 16777216 + return struct.pack(">I", value)[1:] + + +def tls_parse_client_hello(tls_record): + # pylint: disable=too-many-return-statements + """If possible, parse the specified TLS record as a ClientHello and + return the first host_name indicated in a Server Name Indication + extension within that ClientHello. If the TLS record could not + be parsed or there is no such extension or host_name present, + return None. + + :param str tls_record: The TLS record to be parsed (which is assumed + to contain a single ClientHello handshake message).""" + + # TLS handshake? + if tls_record[0] != chr(0x16): + return None + + # TLS version + tls_version = tls_record[1:3] + if map(ord, tls_version) not in [[0x03, 0x01], [0x03, 0x02], [0x03, 0x03]]: + return None + + # TLS record length + tls_record_len = unpack_2bytes(tls_record[3:5]) + if len(tls_record) < tls_record_len: + return None + + # Handshake type, length, and version + handshake_type = tls_record[5] + if handshake_type != chr(0x01): + return None + handshake_len = unpack_3bytes(tls_record[6:9]) + handshake_version = tls_record[9:11] + handshake = tls_record[11:] + + # Handshake length includes handshake_version (2 bytes) + if len(handshake) + 2 < handshake_len: + return None + if map(ord, handshake_version) not in [[0x03, 0x01], [0x03, 0x02], + [0x03, 0x03]]: + return None + + # Random + unused_random = handshake[0:32] + + # Session ID + session_id_length = ord(handshake[32]) + i = 33 + i += session_id_length + + # Ciphersuites + ciphersuites_length = unpack_2bytes(handshake[i:i+2]) + if ciphersuites_length >= 2: + best_ciphersuite = handshake[i+2:i+4] + else: + best_ciphersuite = chr(0) + chr(0) + i += 2 + i += ciphersuites_length + + # Compression methods + compression_length = ord(handshake[i]) + i += 1 + i += compression_length + + # ClientHello extensions + extensions_length = unpack_2bytes(handshake[i:i+2]) + i += 2 + if extensions_length < 10: + # Minimum size of a 1-byte SNI hostname extension + return None + + while i < len(handshake): + # XXX If stated extension lengths are wrong or inconsistent or + # XXX if the packet has been truncated in the middle of an + # XXX extension, this may crash or hang! This needs to be updated + # XXX to fail cleanly when confronted with inconsistent extension + # XXX fields. + extension_type = handshake[i:i+2] + if extension_type == "\0\0": + # SNI + extension_length = unpack_2bytes(handshake[i+2:i+4]) + i += 4 + unused_server_name_list_length = unpack_2bytes(handshake[i:i+2]) + first_sn_type = handshake[i+2] + if first_sn_type != "\0": + # SNI extension referenced something other than a + # hostname + return False + first_sn_length = unpack_2bytes(handshake[i+3:i+5]) + first_sn = handshake[i+5:i+5+first_sn_length] + return best_ciphersuite, first_sn + else: + # Other than SNI + extension_length = unpack_2bytes(handshake[i+2:i+4]) + i += 4 + i += extension_length + continue + return None + + +def tls_generate_server_hello(ciphersuite): + """Generate a TLS 1.2 ServerHello message. + + :param ciphersuite str: The ciphersuite that the ServerHello will + claim to have selected (two bytes).""" + + # Handshake type: ServerHello (0x02) + server_hello = chr(0x02) + # ServerHello length (38 bytes based on below) + server_hello += chr(0x0) + chr(0x0) + chr(38) + # TLS version (0x0303) + server_hello += chr(0x03) + chr(0x03) + # Server Random + server_hello += Crypto.Random.new().read(32) + # Session ID length (0) + server_hello += chr(0x0) + # Ciphersuite + server_hello += ciphersuite + # Compression method (null) + server_hello += chr(0x0) + # Extension length (2 bytes) + extensions go here if any extensions + # are required, BUT if no extensions are present then the extensions + # and extension length field are both omitted entirely (rather than + # declaring extension length 0x0000) - see RFC 5246 p. 42. + + # TLS handshake + tls_record = chr(0x16) + # TLS version + tls_record += chr(0x03) + chr(0x03) + # TLS record length + assert len(server_hello) < 256 + tls_record += chr(0) + chr(len(server_hello)) + # Append server hello handshake + tls_record += server_hello + return tls_record + + +def tls_generate_cert_msg(cert_pem): + """Generate a TLS 1.2 Certificate handshake message containing a + single certificate. + + :param str cert_pem: The certificate to be include in the message (in + PEM format).""" + + cert_as_der = M2Crypto.X509.load_cert_string(cert_pem).as_der() + # Handshake type: Certificate (0x0b) + cert_msg = chr(0x0b) + + cert_msg_length = len(cert_as_der) + 6 + cert_msg += pack_3bytes(cert_msg_length) + + certs_length = len(cert_as_der) + 3 + cert_msg += pack_3bytes(certs_length) + + cert_length = len(cert_as_der) + cert_msg += pack_3bytes(cert_length) + + cert_msg += cert_as_der + + # TLS handshake + tls_record = chr(0x16) + # TLS version + tls_record += chr(0x03) + chr(0x03) + # TLS record length + assert len(cert_msg) < 65536 + tls_record += pack_2bytes(len(cert_msg)) + # Append certificate handshake + tls_record += cert_msg + return tls_record + + +def tls_generate_server_hello_done(): + """Generate a TLS 1.2 ServerHelloDone message.""" + + return "16030300040e000000".decode("hex") + + +class StandaloneAuthenticator(object): + """The StandaloneAuthenticator class itself, which can be invoked + by the Let's Encrypt client according to the IAuthenticator API + interface.""" + zope.interface.implements(interfaces.IAuthenticator) + + def __init__(self): + self.child_pid = None + self.parent_pid = os.getpid() + self.subproc_ready = False + self.subproc_inuse = False + self.subproc_cantbind = False + self.tasks = {} + self.which = None + self.sock = None + self.connection = None + self.private_key = None + self.ssl_conn = None + + def client_signal_handler(self, sig, unused_frame): + """Signal handler for the parent process (to receive inter-process + communication from the child process in the form of Unix + signals.""" + # signal handler for use in parent process + # subprocess → client READY : SIGIO + # subprocess → client INUSE : SIGUSR1 + # subprocess → client CANTBIND: SIGUSR2 + if sig == signal.SIGIO: + self.subproc_ready = True + elif sig == signal.SIGUSR1: + self.subproc_inuse = True + elif sig == signal.SIGUSR2: + self.subproc_cantbind = True + else: + # NOTREACHED + assert False + + def subproc_signal_handler(self, sig, unused_frame): + """Signal handler for the child process (to receive inter-process + communication from the parent process in the form of Unix + signals.""" + # signal handler for use in subprocess + # client → subprocess CLEANUP : SIGINT + if sig == signal.SIGINT: + try: + self.ssl_conn.shutdown() + self.ssl_conn.close() + except BaseException: + # There might not even be any currently active SSL connection. + pass + try: + self.connection.close() + except BaseException: + # There might not even be any currently active connection. + pass + try: + self.sock.close() + except BaseException: + # Various things can go wrong in the course of closing these + # connections, but none of them can clearly be usefully + # reported here and none of them should impede us from + # exiting as gracefully as possible. + pass + os.kill(self.parent_pid, signal.SIGUSR1) + sys.exit(0) + + def sni_callback(self, connection): + """Used internally to set a new OpenSSL context object for this + connection when an incoming connection provides an SNI name (in + order to serve the appropriate certificate, if any).""" + + sni_name = connection.get_servername() + if sni_name in self.tasks: + pem_cert = self.tasks[sni_name] + else: + # TODO: Should we really present a certificate if we get an + # unexpected SNI name? Or should we just disconnect? + pem_cert = self.tasks.values()[0] + cert = OpenSSL.crypto.load_certificate(OpenSSL.crypto.FILETYPE_PEM, + pem_cert) + new_ctx = OpenSSL.SSL.Context(OpenSSL.SSL.TLSv1_METHOD) + new_ctx.set_verify(OpenSSL.SSL.VERIFY_NONE, lambda: False) + new_ctx.use_certificate(cert) + new_ctx.use_privatekey(self.private_key) + connection.set_context(new_ctx) + + def start_listener(self, port, key): + """Create a child process which will start a TCP listener on the + specified port to perform the specified DVSNI challenges. + + :param int port: The TCP port to bind. + :param str key: The private key to use (in PEM format). + """ + fork_result = os.fork() + Crypto.Random.atfork() + if fork_result: + # PARENT process (still the Let's Encrypt client process) + self.which = "parent" + self.child_pid = fork_result + signal.signal(signal.SIGIO, self.client_signal_handler) + signal.signal(signal.SIGUSR1, self.client_signal_handler) + signal.signal(signal.SIGUSR2, self.client_signal_handler) + display = zope.component.getUtility(interfaces.IDisplay) + start_time = time.time() + while time.time() < start_time + 5: + if self.subproc_ready: + return True + if self.subproc_inuse: + display.generic_notification( + "Could not bind TCP port {} because it is already in " + "use it is already in use by another process on this " + "system (such as a web server).".format(CONFIG.PORT)) + return False + if self.subproc_cantbind: + display.generic_notification( + "Could not bind TCP port {} because you don't have " + "the appropriate permissions (for example, you " + "aren't running this program as " + "root).".format(CONFIG.PORT)) + return False + time.sleep(0.1) + display.generic_notification( + "Subprocess unexpectedly timed out while trying to bind TCP " + "port {}.".format(CONFIG.PORT)) + return False + else: + # CHILD process (the TCP listener subprocess) + self.which = "child" + self.child_pid = os.getpid() + signal.signal(signal.SIGINT, self.subproc_signal_handler) + self.sock = socket.socket() + try: + self.sock.bind(("0.0.0.0", port)) + except socket.error, error: + if error.errno == socket.errno.EACCES: + # Signal permissions denied to bind TCP port + os.kill(self.parent_pid, signal.SIGUSR2) + elif error.errno == socket.errno.EADDRINUSE: + # Signal TCP port is already in use + os.kill(self.parent_pid, signal.SIGUSR1) + else: + # XXX: How to handle unknown errors in binding? + raise error + sys.exit(1) + # XXX: We could use poll mechanism to handle simultaneous + # XXX: rather than sequential inbound TCP connections here + self.sock.listen(1) + # Signal that we've successfully bound TCP port + os.kill(self.parent_pid, signal.SIGIO) + self.private_key = OpenSSL.crypto.load_privatekey( + OpenSSL.crypto.FILETYPE_PEM, key.pem) + + while True: + self.connection, _ = self.sock.accept() + + # The code below uses the PyOpenSSL bindings to respond to + # the client. This may expose us to bugs and vulnerabilities + # in OpenSSL (and creates additional dependencies). + ctx = OpenSSL.SSL.Context(OpenSSL.SSL.TLSv1_METHOD) + ctx.set_verify(OpenSSL.SSL.VERIFY_NONE, lambda: False) + pem_cert = self.tasks.values()[0] + first_cert = OpenSSL.crypto.load_certificate( + OpenSSL.crypto.FILETYPE_PEM, pem_cert) + ctx.use_certificate(first_cert) + ctx.use_privatekey(self.private_key) + ctx.set_cipher_list("HIGH") + ctx.set_tlsext_servername_callback(self.sni_callback) + self.ssl_conn = OpenSSL.SSL.Connection(ctx, self.connection) + self.ssl_conn.set_accept_state() + self.ssl_conn.do_handshake() + self.ssl_conn.shutdown() + self.ssl_conn.close() + + # The code below uses the minimal pure Python implementation + # of TLS ClientHello, ServerHello, and Certificate messages + # (as an alternative to a full TLS implementation). It will + # not reach Finished state with a compliant TLS implementation. + # + # client_hello = self.connection.recv(65536) + # result = tls_parse_client_hello(client_hello) + # if result is None: + # print "No SNI found in ClientHello, dropping connection" + # self.connection.close() + # continue + # ciphersuite, sni = result + # if sni in self.tasks: + # pem_cert = self.tasks[sni] + # else: + # # We don't know which cert to send! + # print "Unexpected SNI value", sni + # # Choose the "first" cert and send it (but maybe we + # # should just disconnect instead?) + # pem_cert = self.tasks.values()[0] + # self.connection.send(tls_generate_server_hello(ciphersuite)) + # self.connection.send(tls_generate_cert_msg(pem_cert)) + # self.connection.send(tls_generate_server_hello_done()) + # self.connection.close() + + # IAuthenticator method implementations follow + + def get_chall_pref(self, unused_domain): + """IAuthenticator interface method: Return a list of challenge + types that this authenticator can perform for this domain. In + the case of the StandaloneAuthenticator, the only challenge + type that can ever be performed is dvsni. + """ + return ["dvsni"] + + def perform(self, chall_list): + """IAuthenticator interface method: Attempt to perform the + specified challenges, returning the status of each. For the + StandaloneAuthenticator, because there is no convenient way to add + additional requests, this should only be invoked once; subsequent + invocations are an error. To perform validations for multiple + independent sets of domains, a separate StandaloneAuthenticator + should be instantiated. + """ + if self.child_pid or self.tasks: + # We should not be willing to continue with perform + # if there were existing pending challenges. + # TODO: Specify a correct exception subclass. + raise Exception(".perform() was called with pending tasks!") + results_if_success = [] + results_if_failure = [] + assert chall_list + for chall in chall_list: + if isinstance(chall, DvsniChall): + # We will attempt to do it + name, r_b64 = chall.domain, chall.r_b64 + nonce, key = chall.nonce, chall.key + cert, s_b64 = dvsni_gen_cert(name, r_b64, nonce, key) + self.tasks[nonce + CONFIG.INVALID_EXT] = cert + results_if_success.append({"type": "dvsni", "s": s_b64}) + results_if_failure.append(None) + else: + # We will not attempt to do this challenge because it + # is not a type we can handle + results_if_success.append(False) + results_if_failure.append(False) + assert self.tasks + # Try to do the authentication; note that this creates + # the listener subprocess via os.fork() + if self.start_listener(CONFIG.PORT, key): + return results_if_success + else: + # TODO: This should probably raise a DVAuthError exception + # rather than returning a list of None objects. + return results_if_failure + + def cleanup(self, chall_list): + """IAuthenticator interface method: Remove each of the specified + challenges from the list of challenges that still need to be + performed. (In the case of the StandaloneAuthenticator, if some + challenges are removed from the list, the authenticator socket + will still respond to those challenges.) Once all challenges + have been removed from the list, the listener is deactivated and + stops listening. + """ + # Remove this from pending tasks list + for chall in chall_list: + assert isinstance(chall, DvsniChall) + nonce = chall.nonce + if nonce + CONFIG.INVALID_EXT in self.tasks: + del self.tasks[nonce + CONFIG.INVALID_EXT] + else: + # Could not find the challenge to remove! + assert False + if self.child_pid and not self.tasks: + # There are no remaining challenges, so + # try to shutdown self.child_pid cleanly. + # TODO: ignore any signals from child during this process + os.kill(self.child_pid, signal.SIGINT) + time.sleep(1) + # TODO: restore original signal handlers in parent process + # by resetting their actions to SIG_DFL + print "TCP listener subprocess has been told to shut down" From c43ecf924ce1128dfa6d08e6e53e683815b2e0af Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 2 Feb 2015 17:02:22 -0800 Subject: [PATCH 02/45] Declare dependency on PyOpenSSL package --- requirements.txt | 1 + setup.py | 1 + 2 files changed, 2 insertions(+) diff --git a/requirements.txt b/requirements.txt index a95a0807f..a364c4e8a 100644 --- a/requirements.txt +++ b/requirements.txt @@ -5,3 +5,4 @@ python-augeas==0.5.0 requests==2.4.3 argparse==1.2.2 mock==1.0.1 +PyOpenSSL==0.13 diff --git a/setup.py b/setup.py index a2a4fd9e9..71867ef1a 100755 --- a/setup.py +++ b/setup.py @@ -29,6 +29,7 @@ install_requires = [ 'python-augeas', 'python2-pythondialog', 'requests', + 'PyOpenSSL', 'zope.component', 'zope.interface', # order of items in install_requires DOES matter and M2Crypto has From 867b719de519ce6161396e74f8c79d3334e3af0a Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 2 Feb 2015 17:02:59 -0800 Subject: [PATCH 03/45] Move Key namedtuple definition into le_util.py --- letsencrypt/client/client.py | 4 ++-- letsencrypt/client/le_util.py | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/letsencrypt/client/client.py b/letsencrypt/client/client.py index 223a1ce3a..a2b6dea9b 100644 --- a/letsencrypt/client/client.py +++ b/letsencrypt/client/client.py @@ -22,6 +22,7 @@ from letsencrypt.client import reverter from letsencrypt.client import revoker from letsencrypt.client.apache import configurator +from letsencrypt.le_util import Key class Client(object): @@ -43,7 +44,6 @@ class Client(object): """ zope.interface.implements(interfaces.IAuthenticator) - Key = collections.namedtuple("Key", "file pem") # Note: form is the type of data, "pem" or "der" CSR = collections.namedtuple("CSR", "file data form") @@ -365,7 +365,7 @@ def init_key(key_size): logging.info("Generating key (%d bits): %s", key_size, key_filename) - return Client.Key(key_filename, key_pem) + return Key(key_filename, key_pem) def init_csr(privkey, names): diff --git a/letsencrypt/client/le_util.py b/letsencrypt/client/le_util.py index 59b581a45..31d3fcb5e 100644 --- a/letsencrypt/client/le_util.py +++ b/letsencrypt/client/le_util.py @@ -3,9 +3,12 @@ import base64 import errno import os import stat +import collections from letsencrypt.client import errors +Key = collections.namedtuple("Key", "file pem") + def make_or_verify_dir(directory, mode=0o755, uid=0): """Make sure directory exists with proper permissions. From 220c61f1242659fbdd0efdb121f4b1167f9ea61c Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 2 Feb 2015 17:03:29 -0800 Subject: [PATCH 04/45] Small fixes to how errors are reported --- letsencrypt/client/standalone_authenticator.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/letsencrypt/client/standalone_authenticator.py b/letsencrypt/client/standalone_authenticator.py index 17cddb5a3..1ebd91f3a 100644 --- a/letsencrypt/client/standalone_authenticator.py +++ b/letsencrypt/client/standalone_authenticator.py @@ -135,7 +135,7 @@ def tls_parse_client_hello(tls_record): if first_sn_type != "\0": # SNI extension referenced something other than a # hostname - return False + return None first_sn_length = unpack_2bytes(handshake[i+3:i+5]) first_sn = handshake[i+5:i+5+first_sn_length] return best_ciphersuite, first_sn @@ -491,7 +491,7 @@ class StandaloneAuthenticator(object): del self.tasks[nonce + CONFIG.INVALID_EXT] else: # Could not find the challenge to remove! - assert False + raise ValueError("could not find the challenge to remove") if self.child_pid and not self.tasks: # There are no remaining challenges, so # try to shutdown self.child_pid cleanly. From 361478eca7b4105407a9832322befe60f12e4295 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 2 Feb 2015 17:03:48 -0800 Subject: [PATCH 05/45] Initial set of unit tests for standalone authenticator --- .../tests/standalone_authenticator_test.py | 190 ++++++++++++++++++ 1 file changed, 190 insertions(+) create mode 100644 letsencrypt/client/tests/standalone_authenticator_test.py diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py new file mode 100644 index 000000000..9ec339039 --- /dev/null +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -0,0 +1,190 @@ +#!/usr/bin/env python + +"""Tests for standalone_authenticator.py.""" + +import unittest +import mock +import pkg_resources +from letsencrypt.client.challenge_util import DvsniChall + + +class PackAndUnpackTests(unittest.TestCase): + def test_pack_and_unpack_bytes(self): + from letsencrypt.client.standalone_authenticator import \ + unpack_2bytes, unpack_3bytes, pack_2bytes, pack_3bytes + self.assertEqual(unpack_2bytes("JZ"), 19034) + self.assertEqual(unpack_2bytes(chr(0)*2), 0) + self.assertEqual(unpack_2bytes(chr(255)*2), 65535) + + self.assertEqual(unpack_3bytes("abc"), 6382179) + self.assertEqual(unpack_3bytes(chr(0)*3), 0) + self.assertEqual(unpack_3bytes(chr(255)*3), 16777215) + + self.assertEqual(pack_2bytes(12), chr(0) + chr(12)) + self.assertEqual(pack_2bytes(1729), chr(6) + chr(193)) + + self.assertEqual(pack_3bytes(0), chr(0)*3) + self.assertEqual(pack_3bytes(12345678), chr(0xbc) + "aN") + + +class TLSParseClientHelloTest(unittest.TestCase): + def test_tls_parse_client_hello(self): + from letsencrypt.client.standalone_authenticator import \ + tls_parse_client_hello + client_hello = "16030100c4010000c003030cfef9971eda442c60cbb6c397" \ + "7957a81a8ada317e800b7867a8c61f71c40cab000020c02b" \ + "c02fc00ac009c013c014c007c011003300320039002f0035" \ + "000a000500040100007700000010000e00000b7777772e65" \ + "66662e6f7267ff01000100000a0008000600170018001900" \ + "0b00020100002300003374000000100021001f0568322d31" \ + "3408737064792f332e3106737064792f3308687474702f31" \ + "2e31000500050100000000000d0012001004010501020104" \ + "030503020304020202".decode("hex") + return_value = tls_parse_client_hello(client_hello) + self.assertEqual(return_value, (chr(0xc0) + chr(0x2b), "www.eff.org")) + # TODO: The failure cases are extremely numerous and require + # constructing TLS ClientHello messages that are individually + # defective or surprising in distinct ways. (Each invalid TLS + # record is invalid in its own way.) + +class TLSGenerateServerHelloTest(unittest.TestCase): + def test_tls_generate_server_hello(self): + from letsencrypt.client.standalone_authenticator import \ + tls_generate_server_hello + server_hello = tls_generate_server_hello("Q!") + self.assertEqual(server_hello[:11].encode("hex"), + '160303002a020000260303') + self.assertEqual(server_hello[43:], chr(0) + 'Q!' + chr(0)) + + +class TLSServerHelloDoneTest(unittest.TestCase): + def test_tls_generate_server_hello_done(self): + from letsencrypt.client.standalone_authenticator import \ + tls_generate_server_hello_done + self.assertEqual(tls_generate_server_hello_done().encode("hex"), \ + "16030300040e000000") + + +class ChallPrefTest(unittest.TestCase): + def setUp(self): + from letsencrypt.client.standalone_authenticator import \ + StandaloneAuthenticator + self.authenticator = StandaloneAuthenticator() + + def test_chall_pref(self): + self.assertEqual(self.authenticator.get_chall_pref("example.com"), + ["dvsni"]) + + +class SNICallbackTest(unittest.TestCase): + def setUp(self): + from letsencrypt.client.standalone_authenticator import \ + StandaloneAuthenticator + from letsencrypt.client.challenge_util import dvsni_gen_cert + from letsencrypt.client import le_util + import OpenSSL.crypto + self.authenticator = StandaloneAuthenticator() + r = "x" * 32 + name, r_b64 = "example.com", le_util.jose_b64encode(r) + RSA256_KEY = pkg_resources.resource_string(__name__, + 'testdata/rsa256_key.pem') + nonce, key = "abcdef", le_util.Key("foo", RSA256_KEY) + self.cert = dvsni_gen_cert(name, r_b64, nonce, key)[0] + self.authenticator.private_key = OpenSSL.crypto.load_privatekey( + OpenSSL.crypto.FILETYPE_PEM, key.pem) + self.authenticator.tasks = {"abcdef.acme.invalid": self.cert} + self.authenticator.child_pid = 12345 + + def test_real_servername(self): + import OpenSSL.SSL + connection = mock.MagicMock() + connection.get_servername.return_value = "abcdef.acme.invalid" + self.authenticator.sni_callback(connection) + self.assertEqual(connection.set_context.call_count, 1) + called_ctx = connection.set_context.call_args[0][0] + self.assertIsInstance(called_ctx, OpenSSL.SSL.Context) + + +class ClientSignalHandlerTest(unittest.TestCase): + def setUp(self): + from letsencrypt.client.standalone_authenticator import \ + StandaloneAuthenticator + self.authenticator = StandaloneAuthenticator() + self.authenticator.tasks = {"foononce.acme.invalid": "stuff"} + self.authenticator.child_pid = 12345 + + def test_client_signal_handler(self): + import signal + self.assertEquals(self.authenticator.subproc_ready, False) + self.assertEquals(self.authenticator.subproc_inuse, False) + self.assertEquals(self.authenticator.subproc_cantbind, False) + self.authenticator.client_signal_handler(signal.SIGIO, None) + self.assertEquals(self.authenticator.subproc_ready, True) + + self.authenticator.client_signal_handler(signal.SIGUSR1, None) + self.assertEquals(self.authenticator.subproc_inuse, True) + + self.authenticator.client_signal_handler(signal.SIGUSR2, None) + self.assertEquals(self.authenticator.subproc_cantbind, True) + +class SubprocSignalHandlerTest(unittest.TestCase): + def setUp(self): + from letsencrypt.client.standalone_authenticator import \ + StandaloneAuthenticator + self.authenticator = StandaloneAuthenticator() + self.authenticator.tasks = {"foononce.acme.invalid": "stuff"} + self.authenticator.child_pid = 12345 + self.authenticator.parent_pid = 23456 + + @mock.patch("letsencrypt.client.standalone_authenticator.os.kill") + @mock.patch("letsencrypt.client.standalone_authenticator.sys.exit") + def test_subproc_signal_handler(self, mock_exit, mock_kill): + import signal + self.authenticator.ssl_conn = mock.MagicMock() + self.authenticator.connection = mock.MagicMock() + self.authenticator.sock = mock.MagicMock() + self.authenticator.subproc_signal_handler(signal.SIGINT, None) + self.assertEquals(self.authenticator.ssl_conn.shutdown.call_count, 1) + self.assertEquals(self.authenticator.ssl_conn.close.call_count, 1) + self.assertEquals(self.authenticator.connection.close.call_count, 1) + self.assertEquals(self.authenticator.sock.close.call_count, 1) + # TODO: We should test that we correctly survive each of the above + # raising an exception of some kind (since they're likely to + # do so in practice if there's no live TLS connection at the + # time the subprocess is told to clean up). + self.assertEquals(mock_kill.call_count, 1) + self.assertEquals(mock_exit.call_count, 1) + + +class CleanupTest(unittest.TestCase): + def setUp(self): + from letsencrypt.client.standalone_authenticator import \ + StandaloneAuthenticator + self.authenticator = StandaloneAuthenticator() + self.authenticator.tasks = {"foononce.acme.invalid": "stuff"} + self.authenticator.child_pid = 12345 + + @mock.patch("letsencrypt.client.standalone_authenticator.os.kill") + @mock.patch("letsencrypt.client.standalone_authenticator.time.sleep") + def test_cleanup(self, mock_sleep, mock_kill): + mock_sleep.return_value = None + mock_kill.return_value = None + chall = DvsniChall("foo.example.com", "whee", "foononce", "key") + self.authenticator.cleanup([chall]) + self.assertEqual(mock_kill.call_count, 1) + self.assertEqual(mock_sleep.call_count, 1) + + def test_bad_cleanup(self): + chall = DvsniChall("bad.example.com", "whee", "badnonce", "key") + with self.assertRaises(ValueError): + self.authenticator.cleanup([chall]) + + +if __name__ == '__main__': + unittest.main() + + +# TODO: Unit tests for the following functions +# def tls_generate_cert_msg(cert_pem): +# def start_listener(self, port, key): +# def perform(self, chall_list): From 191b0d7be43967e35625fd71baa24abf6f3c900f Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Tue, 3 Feb 2015 14:23:41 -0800 Subject: [PATCH 06/45] Add unit test for tls_generate_cert_msg --- .../tests/standalone_authenticator_test.py | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index 9ec339039..aa614c22d 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -57,6 +57,33 @@ class TLSGenerateServerHelloTest(unittest.TestCase): self.assertEqual(server_hello[43:], chr(0) + 'Q!' + chr(0)) +class TLSGenerateCertMsgTest(unittest.TestCase): + def test_tls_generate_cert_msg(self): + from letsencrypt.client.standalone_authenticator import \ + tls_generate_cert_msg + cert = pkg_resources.resource_string(__name__, + 'testdata/cert.pem') + cert_msg = tls_generate_cert_msg(cert) + self.assertEqual(cert_msg.encode("hex"), + "16030301ec0b0001e80001e50001e2308201de30820188a0030201020202" + "0539300d06092a864886f70d01010b05003077310b300906035504061302" + "55533111300f06035504080c084d6963686967616e311230100603550407" + "0c09416e6e204172626f72312b3029060355040a0c22556e697665727369" + "7479206f66204d6963686967616e20616e64207468652045464631143012" + "06035504030c0b6578616d706c652e636f6d301e170d3134313231313232" + "333434355a170d3134313231383232333434355a3077310b300906035504" + "06130255533111300f06035504080c084d6963686967616e311230100603" + "5504070c09416e6e204172626f72312b3029060355040a0c22556e697665" + "7273697479206f66204d6963686967616e20616e64207468652045464631" + "14301206035504030c0b6578616d706c652e636f6d305c300d06092a8648" + "86f70d0101010500034b003048024100ac7573b451ed1fddae705243fcdf" + "c75bd02c751b14b875010410e51f036545dddfa79f34aefdbee90584df47" + "1681d9894bce8e6d1cfa9544e8af84744fedc2e50203010001300d06092a" + "864886f70d01010b05000341002db8cf421dc0854a4a59ed92c965bebeb3" + "25ea411f97cc9dd7e4dd7269d748d3e9513ed7828db63874d9ae7a1a8ada" + "02f2404f9fc7ebb13c1af27fa1c36707fa") + + class TLSServerHelloDoneTest(unittest.TestCase): def test_tls_generate_server_hello_done(self): from letsencrypt.client.standalone_authenticator import \ @@ -185,6 +212,5 @@ if __name__ == '__main__': # TODO: Unit tests for the following functions -# def tls_generate_cert_msg(cert_pem): # def start_listener(self, port, key): # def perform(self, chall_list): From 63bf55a748833f82bf7027f5e842e88f37d9ad0a Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Tue, 3 Feb 2015 15:51:37 -0800 Subject: [PATCH 07/45] Split out parent and child listeners into methods --- .../client/standalone_authenticator.py | 202 +++++++++--------- 1 file changed, 105 insertions(+), 97 deletions(-) diff --git a/letsencrypt/client/standalone_authenticator.py b/letsencrypt/client/standalone_authenticator.py index 1ebd91f3a..0bf0d9db6 100644 --- a/letsencrypt/client/standalone_authenticator.py +++ b/letsencrypt/client/standalone_authenticator.py @@ -238,7 +238,6 @@ class StandaloneAuthenticator(object): self.subproc_inuse = False self.subproc_cantbind = False self.tasks = {} - self.which = None self.sock = None self.connection = None self.private_key = None @@ -311,6 +310,109 @@ class StandaloneAuthenticator(object): new_ctx.use_privatekey(self.private_key) connection.set_context(new_ctx) + def do_parent_process(self, port): + """Perform the parent process side of the TCP listener task. This + should only be called by start_listener().""" + + signal.signal(signal.SIGIO, self.client_signal_handler) + signal.signal(signal.SIGUSR1, self.client_signal_handler) + signal.signal(signal.SIGUSR2, self.client_signal_handler) + display = zope.component.getUtility(interfaces.IDisplay) + start_time = time.time() + while time.time() < start_time + 5: + if self.subproc_ready: + return True + if self.subproc_inuse: + display.generic_notification( + "Could not bind TCP port {} because it is already in " + "use it is already in use by another process on this " + "system (such as a web server).".format(CONFIG.PORT)) + return False + if self.subproc_cantbind: + display.generic_notification( + "Could not bind TCP port {} because you don't have " + "the appropriate permissions (for example, you " + "aren't running this program as " + "root).".format(CONFIG.PORT)) + return False + time.sleep(0.1) + display.generic_notification( + "Subprocess unexpectedly timed out while trying to bind TCP " + "port {}.".format(CONFIG.PORT)) + return False + + def do_child_process(self, port, key): + """Perform the child process side of the TCP listener task. This + should only be called by start_listener().""" + signal.signal(signal.SIGINT, self.subproc_signal_handler) + self.sock = socket.socket() + try: + self.sock.bind(("0.0.0.0", port)) + except socket.error, error: + if error.errno == socket.errno.EACCES: + # Signal permissions denied to bind TCP port + os.kill(self.parent_pid, signal.SIGUSR2) + elif error.errno == socket.errno.EADDRINUSE: + # Signal TCP port is already in use + os.kill(self.parent_pid, signal.SIGUSR1) + else: + # XXX: How to handle unknown errors in binding? + raise error + sys.exit(1) + # XXX: We could use poll mechanism to handle simultaneous + # XXX: rather than sequential inbound TCP connections here + self.sock.listen(1) + # Signal that we've successfully bound TCP port + os.kill(self.parent_pid, signal.SIGIO) + self.private_key = OpenSSL.crypto.load_privatekey( + OpenSSL.crypto.FILETYPE_PEM, key.pem) + + while True: + self.connection, _ = self.sock.accept() + + # The code below uses the PyOpenSSL bindings to respond to + # the client. This may expose us to bugs and vulnerabilities + # in OpenSSL (and creates additional dependencies). + ctx = OpenSSL.SSL.Context(OpenSSL.SSL.TLSv1_METHOD) + ctx.set_verify(OpenSSL.SSL.VERIFY_NONE, lambda: False) + pem_cert = self.tasks.values()[0] + first_cert = OpenSSL.crypto.load_certificate( + OpenSSL.crypto.FILETYPE_PEM, pem_cert) + ctx.use_certificate(first_cert) + ctx.use_privatekey(self.private_key) + ctx.set_cipher_list("HIGH") + ctx.set_tlsext_servername_callback(self.sni_callback) + self.ssl_conn = OpenSSL.SSL.Connection(ctx, self.connection) + self.ssl_conn.set_accept_state() + self.ssl_conn.do_handshake() + self.ssl_conn.shutdown() + self.ssl_conn.close() + + # The code below uses the minimal pure Python implementation + # of TLS ClientHello, ServerHello, and Certificate messages + # (as an alternative to a full TLS implementation). It will + # not reach Finished state with a compliant TLS implementation. + # + # client_hello = self.connection.recv(65536) + # result = tls_parse_client_hello(client_hello) + # if result is None: + # print "No SNI found in ClientHello, dropping connection" + # self.connection.close() + # continue + # ciphersuite, sni = result + # if sni in self.tasks: + # pem_cert = self.tasks[sni] + # else: + # # We don't know which cert to send! + # print "Unexpected SNI value", sni + # # Choose the "first" cert and send it (but maybe we + # # should just disconnect instead?) + # pem_cert = self.tasks.values()[0] + # self.connection.send(tls_generate_server_hello(ciphersuite)) + # self.connection.send(tls_generate_cert_msg(pem_cert)) + # self.connection.send(tls_generate_server_hello_done()) + # self.connection.close() + def start_listener(self, port, key): """Create a child process which will start a TCP listener on the specified port to perform the specified DVSNI challenges. @@ -322,106 +424,12 @@ class StandaloneAuthenticator(object): Crypto.Random.atfork() if fork_result: # PARENT process (still the Let's Encrypt client process) - self.which = "parent" self.child_pid = fork_result - signal.signal(signal.SIGIO, self.client_signal_handler) - signal.signal(signal.SIGUSR1, self.client_signal_handler) - signal.signal(signal.SIGUSR2, self.client_signal_handler) - display = zope.component.getUtility(interfaces.IDisplay) - start_time = time.time() - while time.time() < start_time + 5: - if self.subproc_ready: - return True - if self.subproc_inuse: - display.generic_notification( - "Could not bind TCP port {} because it is already in " - "use it is already in use by another process on this " - "system (such as a web server).".format(CONFIG.PORT)) - return False - if self.subproc_cantbind: - display.generic_notification( - "Could not bind TCP port {} because you don't have " - "the appropriate permissions (for example, you " - "aren't running this program as " - "root).".format(CONFIG.PORT)) - return False - time.sleep(0.1) - display.generic_notification( - "Subprocess unexpectedly timed out while trying to bind TCP " - "port {}.".format(CONFIG.PORT)) - return False + self.do_parent_process(port) else: # CHILD process (the TCP listener subprocess) - self.which = "child" self.child_pid = os.getpid() - signal.signal(signal.SIGINT, self.subproc_signal_handler) - self.sock = socket.socket() - try: - self.sock.bind(("0.0.0.0", port)) - except socket.error, error: - if error.errno == socket.errno.EACCES: - # Signal permissions denied to bind TCP port - os.kill(self.parent_pid, signal.SIGUSR2) - elif error.errno == socket.errno.EADDRINUSE: - # Signal TCP port is already in use - os.kill(self.parent_pid, signal.SIGUSR1) - else: - # XXX: How to handle unknown errors in binding? - raise error - sys.exit(1) - # XXX: We could use poll mechanism to handle simultaneous - # XXX: rather than sequential inbound TCP connections here - self.sock.listen(1) - # Signal that we've successfully bound TCP port - os.kill(self.parent_pid, signal.SIGIO) - self.private_key = OpenSSL.crypto.load_privatekey( - OpenSSL.crypto.FILETYPE_PEM, key.pem) - - while True: - self.connection, _ = self.sock.accept() - - # The code below uses the PyOpenSSL bindings to respond to - # the client. This may expose us to bugs and vulnerabilities - # in OpenSSL (and creates additional dependencies). - ctx = OpenSSL.SSL.Context(OpenSSL.SSL.TLSv1_METHOD) - ctx.set_verify(OpenSSL.SSL.VERIFY_NONE, lambda: False) - pem_cert = self.tasks.values()[0] - first_cert = OpenSSL.crypto.load_certificate( - OpenSSL.crypto.FILETYPE_PEM, pem_cert) - ctx.use_certificate(first_cert) - ctx.use_privatekey(self.private_key) - ctx.set_cipher_list("HIGH") - ctx.set_tlsext_servername_callback(self.sni_callback) - self.ssl_conn = OpenSSL.SSL.Connection(ctx, self.connection) - self.ssl_conn.set_accept_state() - self.ssl_conn.do_handshake() - self.ssl_conn.shutdown() - self.ssl_conn.close() - - # The code below uses the minimal pure Python implementation - # of TLS ClientHello, ServerHello, and Certificate messages - # (as an alternative to a full TLS implementation). It will - # not reach Finished state with a compliant TLS implementation. - # - # client_hello = self.connection.recv(65536) - # result = tls_parse_client_hello(client_hello) - # if result is None: - # print "No SNI found in ClientHello, dropping connection" - # self.connection.close() - # continue - # ciphersuite, sni = result - # if sni in self.tasks: - # pem_cert = self.tasks[sni] - # else: - # # We don't know which cert to send! - # print "Unexpected SNI value", sni - # # Choose the "first" cert and send it (but maybe we - # # should just disconnect instead?) - # pem_cert = self.tasks.values()[0] - # self.connection.send(tls_generate_server_hello(ciphersuite)) - # self.connection.send(tls_generate_cert_msg(pem_cert)) - # self.connection.send(tls_generate_server_hello_done()) - # self.connection.close() + self.do_child_process(port, key) # IAuthenticator method implementations follow From e9b67ff6f9153292f29f97fc1ff7bc2356f817d0 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Tue, 3 Feb 2015 17:59:35 -0800 Subject: [PATCH 08/45] Several more unit tests for StandaloneAuthenticator --- .../tests/standalone_authenticator_test.py | 137 +++++++++++++++++- 1 file changed, 129 insertions(+), 8 deletions(-) diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index aa614c22d..978550e4f 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -47,6 +47,7 @@ class TLSParseClientHelloTest(unittest.TestCase): # defective or surprising in distinct ways. (Each invalid TLS # record is invalid in its own way.) + class TLSGenerateServerHelloTest(unittest.TestCase): def test_tls_generate_server_hello(self): from letsencrypt.client.standalone_authenticator import \ @@ -142,17 +143,17 @@ class ClientSignalHandlerTest(unittest.TestCase): def test_client_signal_handler(self): import signal - self.assertEquals(self.authenticator.subproc_ready, False) - self.assertEquals(self.authenticator.subproc_inuse, False) - self.assertEquals(self.authenticator.subproc_cantbind, False) + self.assertFalse(self.authenticator.subproc_ready) + self.assertFalse(self.authenticator.subproc_inuse) + self.assertFalse(self.authenticator.subproc_cantbind) self.authenticator.client_signal_handler(signal.SIGIO, None) - self.assertEquals(self.authenticator.subproc_ready, True) + self.assertTrue(self.authenticator.subproc_ready) self.authenticator.client_signal_handler(signal.SIGUSR1, None) - self.assertEquals(self.authenticator.subproc_inuse, True) + self.assertTrue(self.authenticator.subproc_inuse) self.authenticator.client_signal_handler(signal.SIGUSR2, None) - self.assertEquals(self.authenticator.subproc_cantbind, True) + self.assertTrue(self.authenticator.subproc_cantbind) class SubprocSignalHandlerTest(unittest.TestCase): def setUp(self): @@ -183,6 +184,127 @@ class SubprocSignalHandlerTest(unittest.TestCase): self.assertEquals(mock_exit.call_count, 1) +class PerformTest(unittest.TestCase): + def setUp(self): + from letsencrypt.client.standalone_authenticator import \ + StandaloneAuthenticator + self.authenticator = StandaloneAuthenticator() + + def test_can_perform(self): + """What happens if start_listener() returns True.""" + from letsencrypt.client import le_util + RSA256_KEY = pkg_resources.resource_string(__name__, + 'testdata/rsa256_key.pem') + key = le_util.Key("something", RSA256_KEY) + chall1 = DvsniChall("foo.example.com", "whee", "foononce", key) + chall2 = DvsniChall("bar.example.com", "whee", "barnonce", key) + bad_chall = ("This", "Represents", "A Non-DVSNI", "Challenge") + self.authenticator.start_listener = mock.Mock() + self.authenticator.start_listener.return_value = True + result = self.authenticator.perform([chall1, chall2, bad_chall]) + self.assertEqual(len(self.authenticator.tasks), 2) + self.assertTrue( + self.authenticator.tasks.has_key("foononce.acme.invalid")) + self.assertTrue( + self.authenticator.tasks.has_key("barnonce.acme.invalid")) + self.assertIsInstance(result, list) + self.assertEqual(len(result), 3) + self.assertIsInstance(result[0], dict) + self.assertIsInstance(result[1], dict) + self.assertFalse(result[2]) + self.assertTrue(result[0].has_key("s")) + self.assertTrue(result[1].has_key("s")) + self.assertEqual(self.authenticator.start_listener.call_count, 1) + + def test_cannot_perform(self): + """What happens if start_listener() returns False.""" + from letsencrypt.client import le_util + RSA256_KEY = pkg_resources.resource_string(__name__, + 'testdata/rsa256_key.pem') + key = le_util.Key("something", RSA256_KEY) + chall1 = DvsniChall("foo.example.com", "whee", "foononce", key) + chall2 = DvsniChall("bar.example.com", "whee", "barnonce", key) + bad_chall = ("This", "Represents", "A Non-DVSNI", "Challenge") + self.authenticator.start_listener = mock.Mock() + self.authenticator.start_listener.return_value = False + result = self.authenticator.perform([chall1, chall2, bad_chall]) + self.assertEqual(len(self.authenticator.tasks), 2) + self.assertTrue( + self.authenticator.tasks.has_key("foononce.acme.invalid")) + self.assertTrue( + self.authenticator.tasks.has_key("barnonce.acme.invalid")) + self.assertIsInstance(result, list) + self.assertEqual(len(result), 3) + self.assertEqual(result, [None, None, False]) + self.assertEqual(self.authenticator.start_listener.call_count, 1) + +class StartListenerTest(unittest.TestCase): + def setUp(self): + from letsencrypt.client.standalone_authenticator import \ + StandaloneAuthenticator + self.authenticator = StandaloneAuthenticator() + + @mock.patch("letsencrypt.client.standalone_authenticator.Crypto.Random.atfork") + @mock.patch("letsencrypt.client.standalone_authenticator.os.fork") + def test_start_listener_fork_parent(self, mock_fork, mock_atfork): + self.authenticator.do_parent_process = mock.Mock() + mock_fork.return_value = 22222 + self.authenticator.start_listener(1717, "key") + self.assertEqual(self.authenticator.child_pid, 22222) + self.assertEqual(self.authenticator.do_parent_process.call_count, 1) + self.assertEqual(mock_atfork.call_count, 1) + + @mock.patch("letsencrypt.client.standalone_authenticator.Crypto.Random.atfork") + @mock.patch("letsencrypt.client.standalone_authenticator.os.fork") + def test_start_listener_fork_child(self, mock_fork, mock_atfork): + import os + self.authenticator.do_parent_process = mock.Mock() + self.authenticator.do_child_process = mock.Mock() + mock_fork.return_value = 0 + self.authenticator.start_listener(1717, "key") + self.assertEqual(self.authenticator.child_pid, os.getpid()) + self.assertEqual(self.authenticator.do_child_process.call_count, 1) + self.assertEqual(mock_atfork.call_count, 1) + +class DoParentProcessTest(unittest.TestCase): + def setUp(self): + from letsencrypt.client.standalone_authenticator import \ + StandaloneAuthenticator + self.authenticator = StandaloneAuthenticator() + + @mock.patch("letsencrypt.client.standalone_authenticator.signal.signal") + @mock.patch("letsencrypt.client.standalone_authenticator.zope.component.getUtility") + def test_do_parent_process_ok(self, mock_getUtility, mock_signal): + self.authenticator.subproc_ready = True + result = self.authenticator.do_parent_process(1717) + self.assertTrue(result) + self.assertEqual(mock_signal.call_count, 3) + + @mock.patch("letsencrypt.client.standalone_authenticator.signal.signal") + @mock.patch("letsencrypt.client.standalone_authenticator.zope.component.getUtility") + def test_do_parent_process_inuse(self, mock_getUtility, mock_signal): + self.authenticator.subproc_inuse = True + result = self.authenticator.do_parent_process(1717) + self.assertFalse(result) + self.assertEqual(mock_signal.call_count, 3) + + @mock.patch("letsencrypt.client.standalone_authenticator.signal.signal") + @mock.patch("letsencrypt.client.standalone_authenticator.zope.component.getUtility") + def test_do_parent_process_cantbind(self, mock_getUtility, mock_signal): + self.authenticator.subproc_cantbind = True + result = self.authenticator.do_parent_process(1717) + self.assertFalse(result) + self.assertEqual(mock_signal.call_count, 3) + + @mock.patch("letsencrypt.client.standalone_authenticator.signal.signal") + @mock.patch("letsencrypt.client.standalone_authenticator.zope.component.getUtility") + def test_do_parent_process_timeout(self, mock_getUtility, mock_signal): + # Times out in 5 seconds and returns False. + result = self.authenticator.do_parent_process(1717) + self.assertFalse(result) + self.assertEqual(mock_signal.call_count, 3) + + class CleanupTest(unittest.TestCase): def setUp(self): from letsencrypt.client.standalone_authenticator import \ @@ -212,5 +334,4 @@ if __name__ == '__main__': # TODO: Unit tests for the following functions -# def start_listener(self, port, key): -# def perform(self, chall_list): +# do_child_process From f9b0d8d0bf632d14def5ff6f5837cb33579a7b5e Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Wed, 4 Feb 2015 22:17:11 -0800 Subject: [PATCH 09/45] Add unit tests for listener child process --- .../tests/standalone_authenticator_test.py | 108 +++++++++++++++++- 1 file changed, 104 insertions(+), 4 deletions(-) diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index 978550e4f..97ccaa5a5 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -8,6 +8,33 @@ import pkg_resources from letsencrypt.client.challenge_util import DvsniChall +# ErrorAfter/CallableExhausted from +# http://igorsobreira.com/2013/03/17/testing-infinite-loops.html +# to allow interrupting infinite loop under test after one +# iteration. + +class ErrorAfter_socket_accept(object): + """ + Callable that will raise `CallableExhausted` + exception after `limit` calls, modified to also return + a tuple simulating the return values of a socket.accept() + call + """ + def __init__(self, limit): + self.limit = limit + self.calls = 0 + + def __call__(self): + self.calls += 1 + if self.calls > self.limit: + raise CallableExhausted + # Modified here for a single use as socket.accept() + return (mock.MagicMock(), "ignored") + +class CallableExhausted(Exception): + pass + + class PackAndUnpackTests(unittest.TestCase): def test_pack_and_unpack_bytes(self): from letsencrypt.client.standalone_authenticator import \ @@ -305,6 +332,83 @@ class DoParentProcessTest(unittest.TestCase): self.assertEqual(mock_signal.call_count, 3) +class DoChildProcessTest(unittest.TestCase): + def setUp(self): + from letsencrypt.client.standalone_authenticator import \ + StandaloneAuthenticator + from letsencrypt.client.challenge_util import dvsni_gen_cert + from letsencrypt.client import le_util + import OpenSSL.crypto + self.authenticator = StandaloneAuthenticator() + r = "x" * 32 + name, r_b64 = "example.com", le_util.jose_b64encode(r) + RSA256_KEY = pkg_resources.resource_string(__name__, + 'testdata/rsa256_key.pem') + nonce, key = "abcdef", le_util.Key("foo", RSA256_KEY) + self.key = key + self.cert = dvsni_gen_cert(name, r_b64, nonce, key)[0] + self.authenticator.private_key = OpenSSL.crypto.load_privatekey( + OpenSSL.crypto.FILETYPE_PEM, key.pem) + self.authenticator.tasks = {"abcdef.acme.invalid": self.cert} + self.authenticator.parent_pid = 12345 + + @mock.patch("letsencrypt.client.standalone_authenticator.socket.socket") + @mock.patch("letsencrypt.client.standalone_authenticator.os.kill") + @mock.patch("letsencrypt.client.standalone_authenticator.sys.exit") + def test_do_child_process_cantbind1(self, mock_exit, mock_kill, mock_socket): + import socket, signal + mock_exit.side_effect = IndentationError("subprocess would exit here") + eaccess = socket.error(socket.errno.EACCES, "Permission denied") + sample_socket = mock.MagicMock() + sample_socket.bind.side_effect = eaccess + mock_socket.return_value = sample_socket + # Using the IndentationError as an error that cannot easily be + # generated at runtime, to indicate the behavior of sys.exit has + # taken effect without actually causing the test process to exit. + # (Just replacing it with a no-op causes logic errors because the + # do_child_process code assumes that calling sys.exit() will + # cause subsequent code not to be executed.) + with self.assertRaises(IndentationError): + result = self.authenticator.do_child_process(1717, self.key) + self.assertEqual(mock_exit.call_count, 1) + mock_kill.assert_called_once_with(12345, signal.SIGUSR2) + + @mock.patch("letsencrypt.client.standalone_authenticator.socket.socket") + @mock.patch("letsencrypt.client.standalone_authenticator.os.kill") + @mock.patch("letsencrypt.client.standalone_authenticator.sys.exit") + def test_do_child_process_cantbind2(self, mock_exit, mock_kill, mock_socket): + import socket, signal + mock_exit.side_effect = IndentationError("subprocess would exit here") + eaccess = socket.error(socket.errno.EADDRINUSE, "Port already in use") + sample_socket = mock.MagicMock() + sample_socket.bind.side_effect = eaccess + mock_socket.return_value = sample_socket + with self.assertRaises(IndentationError): + result = self.authenticator.do_child_process(1717, self.key) + self.assertEqual(mock_exit.call_count, 1) + mock_kill.assert_called_once_with(12345, signal.SIGUSR1) + + @mock.patch("letsencrypt.client.standalone_authenticator.OpenSSL.SSL.Connection") + @mock.patch("letsencrypt.client.standalone_authenticator.socket.socket") + @mock.patch("letsencrypt.client.standalone_authenticator.os.kill") + def test_do_child_process_success(self, mock_kill, mock_socket, mock_Connection): + import socket, signal + sample_socket = mock.MagicMock() + sample_socket.accept.side_effect = ErrorAfter_socket_accept(2) + mock_socket.return_value = sample_socket + mock_Connection.return_value = mock.MagicMock() + with self.assertRaises(CallableExhausted): + result = self.authenticator.do_child_process(1717, self.key) + mock_socket.assert_called_once_with() + sample_socket.bind.assert_called_once_with(("0.0.0.0", 1717)) + sample_socket.listen.assert_called_once_with(1) + self.assertEqual(sample_socket.accept.call_count, 3) + mock_kill.assert_called_once_with(12345, signal.SIGIO) + # TODO: We could have some tests about the fact that the listener + # asks OpenSSL to negotiate a TLS connection (and correctly + # sets the SNI callback function). + + class CleanupTest(unittest.TestCase): def setUp(self): from letsencrypt.client.standalone_authenticator import \ @@ -331,7 +435,3 @@ class CleanupTest(unittest.TestCase): if __name__ == '__main__': unittest.main() - - -# TODO: Unit tests for the following functions -# do_child_process From 3d2f564478e84cb049f90f83195b8cc9e13a4135 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Wed, 4 Feb 2015 22:32:27 -0800 Subject: [PATCH 10/45] Replace some call_counts with more specific assertions --- .../tests/standalone_authenticator_test.py | 27 ++++++++++--------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index 97ccaa5a5..dde7e6d9b 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -207,8 +207,9 @@ class SubprocSignalHandlerTest(unittest.TestCase): # raising an exception of some kind (since they're likely to # do so in practice if there's no live TLS connection at the # time the subprocess is told to clean up). - self.assertEquals(mock_kill.call_count, 1) - self.assertEquals(mock_exit.call_count, 1) + mock_kill.assert_called_once_with(self.authenticator.parent_pid, + signal.SIGUSR1) + mock_exit.assert_called_once_with(0) class PerformTest(unittest.TestCase): @@ -241,7 +242,7 @@ class PerformTest(unittest.TestCase): self.assertFalse(result[2]) self.assertTrue(result[0].has_key("s")) self.assertTrue(result[1].has_key("s")) - self.assertEqual(self.authenticator.start_listener.call_count, 1) + self.authenticator.start_listener.assert_called_once_with(443, key) def test_cannot_perform(self): """What happens if start_listener() returns False.""" @@ -263,7 +264,7 @@ class PerformTest(unittest.TestCase): self.assertIsInstance(result, list) self.assertEqual(len(result), 3) self.assertEqual(result, [None, None, False]) - self.assertEqual(self.authenticator.start_listener.call_count, 1) + self.authenticator.start_listener.assert_called_once_with(443, key) class StartListenerTest(unittest.TestCase): def setUp(self): @@ -278,8 +279,8 @@ class StartListenerTest(unittest.TestCase): mock_fork.return_value = 22222 self.authenticator.start_listener(1717, "key") self.assertEqual(self.authenticator.child_pid, 22222) - self.assertEqual(self.authenticator.do_parent_process.call_count, 1) - self.assertEqual(mock_atfork.call_count, 1) + self.authenticator.do_parent_process.assert_called_once_with(1717) + mock_atfork.assert_called_once_with() @mock.patch("letsencrypt.client.standalone_authenticator.Crypto.Random.atfork") @mock.patch("letsencrypt.client.standalone_authenticator.os.fork") @@ -290,8 +291,9 @@ class StartListenerTest(unittest.TestCase): mock_fork.return_value = 0 self.authenticator.start_listener(1717, "key") self.assertEqual(self.authenticator.child_pid, os.getpid()) - self.assertEqual(self.authenticator.do_child_process.call_count, 1) - self.assertEqual(mock_atfork.call_count, 1) + self.authenticator.do_child_process.assert_called_once_with(1717, + "key") + mock_atfork.assert_called_once_with() class DoParentProcessTest(unittest.TestCase): def setUp(self): @@ -370,7 +372,7 @@ class DoChildProcessTest(unittest.TestCase): # cause subsequent code not to be executed.) with self.assertRaises(IndentationError): result = self.authenticator.do_child_process(1717, self.key) - self.assertEqual(mock_exit.call_count, 1) + mock_exit.assert_called_once_with(1) mock_kill.assert_called_once_with(12345, signal.SIGUSR2) @mock.patch("letsencrypt.client.standalone_authenticator.socket.socket") @@ -385,7 +387,7 @@ class DoChildProcessTest(unittest.TestCase): mock_socket.return_value = sample_socket with self.assertRaises(IndentationError): result = self.authenticator.do_child_process(1717, self.key) - self.assertEqual(mock_exit.call_count, 1) + mock_exit.assert_called_once_with(1) mock_kill.assert_called_once_with(12345, signal.SIGUSR1) @mock.patch("letsencrypt.client.standalone_authenticator.OpenSSL.SSL.Connection") @@ -420,12 +422,13 @@ class CleanupTest(unittest.TestCase): @mock.patch("letsencrypt.client.standalone_authenticator.os.kill") @mock.patch("letsencrypt.client.standalone_authenticator.time.sleep") def test_cleanup(self, mock_sleep, mock_kill): + import signal mock_sleep.return_value = None mock_kill.return_value = None chall = DvsniChall("foo.example.com", "whee", "foononce", "key") self.authenticator.cleanup([chall]) - self.assertEqual(mock_kill.call_count, 1) - self.assertEqual(mock_sleep.call_count, 1) + mock_kill.assert_called_once_with(12345, signal.SIGINT) + mock_sleep.assert_called_once_with(1) def test_bad_cleanup(self): chall = DvsniChall("bad.example.com", "whee", "badnonce", "key") From ff3c0c668975ff2366b6a926f3b67c825746acf1 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Wed, 4 Feb 2015 22:33:40 -0800 Subject: [PATCH 11/45] Comment out debug print statement --- letsencrypt/client/standalone_authenticator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/client/standalone_authenticator.py b/letsencrypt/client/standalone_authenticator.py index 0bf0d9db6..5a1e3d5dc 100644 --- a/letsencrypt/client/standalone_authenticator.py +++ b/letsencrypt/client/standalone_authenticator.py @@ -508,4 +508,4 @@ class StandaloneAuthenticator(object): time.sleep(1) # TODO: restore original signal handlers in parent process # by resetting their actions to SIG_DFL - print "TCP listener subprocess has been told to shut down" + # print "TCP listener subprocess has been told to shut down" From 41284ffc0a27b6e24dff57a08a69b21c3fc355bc Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Wed, 4 Feb 2015 22:36:24 -0800 Subject: [PATCH 12/45] Let tests specify how long parent waits for child process --- letsencrypt/client/standalone_authenticator.py | 7 ++++--- letsencrypt/client/tests/standalone_authenticator_test.py | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/letsencrypt/client/standalone_authenticator.py b/letsencrypt/client/standalone_authenticator.py index 5a1e3d5dc..abc7a6983 100644 --- a/letsencrypt/client/standalone_authenticator.py +++ b/letsencrypt/client/standalone_authenticator.py @@ -310,16 +310,17 @@ class StandaloneAuthenticator(object): new_ctx.use_privatekey(self.private_key) connection.set_context(new_ctx) - def do_parent_process(self, port): + def do_parent_process(self, port, delay_amount=5): """Perform the parent process side of the TCP listener task. This - should only be called by start_listener().""" + should only be called by start_listener(). We will wait up to + delay_amount seconds to hear from the child process via a signal.""" signal.signal(signal.SIGIO, self.client_signal_handler) signal.signal(signal.SIGUSR1, self.client_signal_handler) signal.signal(signal.SIGUSR2, self.client_signal_handler) display = zope.component.getUtility(interfaces.IDisplay) start_time = time.time() - while time.time() < start_time + 5: + while time.time() < start_time + delay_amount: if self.subproc_ready: return True if self.subproc_inuse: diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index dde7e6d9b..ca0802097 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -329,7 +329,7 @@ class DoParentProcessTest(unittest.TestCase): @mock.patch("letsencrypt.client.standalone_authenticator.zope.component.getUtility") def test_do_parent_process_timeout(self, mock_getUtility, mock_signal): # Times out in 5 seconds and returns False. - result = self.authenticator.do_parent_process(1717) + result = self.authenticator.do_parent_process(1717, delay_amount=1) self.assertFalse(result) self.assertEqual(mock_signal.call_count, 3) From ef34c06c8fc594ee74e8346fbf4fa016e08bd54e Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Wed, 4 Feb 2015 23:26:10 -0800 Subject: [PATCH 13/45] Convert two assertions to exceptions --- letsencrypt/client/standalone_authenticator.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/letsencrypt/client/standalone_authenticator.py b/letsencrypt/client/standalone_authenticator.py index abc7a6983..d8618a0ff 100644 --- a/letsencrypt/client/standalone_authenticator.py +++ b/letsencrypt/client/standalone_authenticator.py @@ -458,7 +458,9 @@ class StandaloneAuthenticator(object): raise Exception(".perform() was called with pending tasks!") results_if_success = [] results_if_failure = [] - assert chall_list + if not chall_list or not isinstance(chall_list, list): + # TODO: Specify a correct exception subclass. + raise Exception(".perform() was called without challenge list") for chall in chall_list: if isinstance(chall, DvsniChall): # We will attempt to do it @@ -473,7 +475,9 @@ class StandaloneAuthenticator(object): # is not a type we can handle results_if_success.append(False) results_if_failure.append(False) - assert self.tasks + if not self.tasks: + # TODO: Specify a correct exception subclass. + raise Exception("nothing for .perform() to do") # Try to do the authentication; note that this creates # the listener subprocess via os.fork() if self.start_listener(CONFIG.PORT, key): From db3c98b45a381f8903e76144a06a3ce101d2cdc6 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Thu, 5 Feb 2015 13:03:20 -0800 Subject: [PATCH 14/45] Adding some docstrings --- .../tests/standalone_authenticator_test.py | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index ca0802097..a9bc087b6 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -36,6 +36,8 @@ class CallableExhausted(Exception): class PackAndUnpackTests(unittest.TestCase): + """Tests for byte packing and unpacking routines used for TLS + parsing.""" def test_pack_and_unpack_bytes(self): from letsencrypt.client.standalone_authenticator import \ unpack_2bytes, unpack_3bytes, pack_2bytes, pack_3bytes @@ -55,6 +57,7 @@ class PackAndUnpackTests(unittest.TestCase): class TLSParseClientHelloTest(unittest.TestCase): + """Test for tls_parse_client_hello() function.""" def test_tls_parse_client_hello(self): from letsencrypt.client.standalone_authenticator import \ tls_parse_client_hello @@ -76,6 +79,7 @@ class TLSParseClientHelloTest(unittest.TestCase): class TLSGenerateServerHelloTest(unittest.TestCase): + """Tests for tls_generate_server_hello() function.""" def test_tls_generate_server_hello(self): from letsencrypt.client.standalone_authenticator import \ tls_generate_server_hello @@ -86,6 +90,7 @@ class TLSGenerateServerHelloTest(unittest.TestCase): class TLSGenerateCertMsgTest(unittest.TestCase): + """Tests for tls_generate_cert_msg() function.""" def test_tls_generate_cert_msg(self): from letsencrypt.client.standalone_authenticator import \ tls_generate_cert_msg @@ -113,6 +118,7 @@ class TLSGenerateCertMsgTest(unittest.TestCase): class TLSServerHelloDoneTest(unittest.TestCase): + """Tests for tls_generate_server_hello_done() function.""" def test_tls_generate_server_hello_done(self): from letsencrypt.client.standalone_authenticator import \ tls_generate_server_hello_done @@ -121,6 +127,7 @@ class TLSServerHelloDoneTest(unittest.TestCase): class ChallPrefTest(unittest.TestCase): + """Tests for chall_pref() method.""" def setUp(self): from letsencrypt.client.standalone_authenticator import \ StandaloneAuthenticator @@ -132,6 +139,7 @@ class ChallPrefTest(unittest.TestCase): class SNICallbackTest(unittest.TestCase): + """Tests for sni_callback() method.""" def setUp(self): from letsencrypt.client.standalone_authenticator import \ StandaloneAuthenticator @@ -161,6 +169,7 @@ class SNICallbackTest(unittest.TestCase): class ClientSignalHandlerTest(unittest.TestCase): + """Tests for client_signal_handler() method.""" def setUp(self): from letsencrypt.client.standalone_authenticator import \ StandaloneAuthenticator @@ -183,6 +192,7 @@ class ClientSignalHandlerTest(unittest.TestCase): self.assertTrue(self.authenticator.subproc_cantbind) class SubprocSignalHandlerTest(unittest.TestCase): + """Tests for subproc_signal_handler() method.""" def setUp(self): from letsencrypt.client.standalone_authenticator import \ StandaloneAuthenticator @@ -213,6 +223,7 @@ class SubprocSignalHandlerTest(unittest.TestCase): class PerformTest(unittest.TestCase): + """Tests for perform() method.""" def setUp(self): from letsencrypt.client.standalone_authenticator import \ StandaloneAuthenticator @@ -267,6 +278,7 @@ class PerformTest(unittest.TestCase): self.authenticator.start_listener.assert_called_once_with(443, key) class StartListenerTest(unittest.TestCase): + """Tests for start_listener() method.""" def setUp(self): from letsencrypt.client.standalone_authenticator import \ StandaloneAuthenticator @@ -296,6 +308,7 @@ class StartListenerTest(unittest.TestCase): mock_atfork.assert_called_once_with() class DoParentProcessTest(unittest.TestCase): + """Tests for do_parent_process() method.""" def setUp(self): from letsencrypt.client.standalone_authenticator import \ StandaloneAuthenticator @@ -328,13 +341,16 @@ class DoParentProcessTest(unittest.TestCase): @mock.patch("letsencrypt.client.standalone_authenticator.signal.signal") @mock.patch("letsencrypt.client.standalone_authenticator.zope.component.getUtility") def test_do_parent_process_timeout(self, mock_getUtility, mock_signal): - # Times out in 5 seconds and returns False. + # Normally times out in 5 seconds and returns False. We can + # now set delay_amount to a lower value so that it times out + # faster than it would under normal use. result = self.authenticator.do_parent_process(1717, delay_amount=1) self.assertFalse(result) self.assertEqual(mock_signal.call_count, 3) class DoChildProcessTest(unittest.TestCase): + """Tests for do_child_process() method.""" def setUp(self): from letsencrypt.client.standalone_authenticator import \ StandaloneAuthenticator @@ -412,6 +428,7 @@ class DoChildProcessTest(unittest.TestCase): class CleanupTest(unittest.TestCase): + """Tests for cleanup() method.""" def setUp(self): from letsencrypt.client.standalone_authenticator import \ StandaloneAuthenticator From 8997248abb25e4c4e846d1c1c4d7da59089c26e6 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Thu, 5 Feb 2015 13:14:03 -0800 Subject: [PATCH 15/45] Use caller-specified TCP port no. in UI messages --- letsencrypt/client/standalone_authenticator.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/letsencrypt/client/standalone_authenticator.py b/letsencrypt/client/standalone_authenticator.py index d8618a0ff..d68a43b47 100644 --- a/letsencrypt/client/standalone_authenticator.py +++ b/letsencrypt/client/standalone_authenticator.py @@ -327,19 +327,19 @@ class StandaloneAuthenticator(object): display.generic_notification( "Could not bind TCP port {} because it is already in " "use it is already in use by another process on this " - "system (such as a web server).".format(CONFIG.PORT)) + "system (such as a web server).".format(port)) return False if self.subproc_cantbind: display.generic_notification( "Could not bind TCP port {} because you don't have " "the appropriate permissions (for example, you " "aren't running this program as " - "root).".format(CONFIG.PORT)) + "root).".format(port)) return False time.sleep(0.1) display.generic_notification( "Subprocess unexpectedly timed out while trying to bind TCP " - "port {}.".format(CONFIG.PORT)) + "port {}.".format(port)) return False def do_child_process(self, port, key): From 53f3f19f6dc5e996c97199fda0bb4857a658c087 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Thu, 5 Feb 2015 15:57:10 -0800 Subject: [PATCH 16/45] Check invalid cases for pack and unpack functions --- .../client/tests/standalone_authenticator_test.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index a9bc087b6..7329ad84f 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -55,6 +55,18 @@ class PackAndUnpackTests(unittest.TestCase): self.assertEqual(pack_3bytes(0), chr(0)*3) self.assertEqual(pack_3bytes(12345678), chr(0xbc) + "aN") + def test_invalid_pack_and_unpack(self): + from letsencrypt.client.standalone_authenticator import \ + unpack_2bytes, unpack_3bytes, pack_2bytes, pack_3bytes + with self.assertRaises(AssertionError): + pack_2bytes(65537) + with self.assertRaises(AssertionError): + pack_3bytes(500000000) + with self.assertRaises(AssertionError): + unpack_2bytes("foo") + with self.assertRaises(AssertionError): + unpack_3bytes("food") + class TLSParseClientHelloTest(unittest.TestCase): """Test for tls_parse_client_hello() function.""" From ee8e4343bca5d8df90ea96392b53278e3ac41579 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Fri, 6 Feb 2015 14:46:45 -0800 Subject: [PATCH 17/45] Reformat continuation indentations --- .../tests/standalone_authenticator_test.py | 71 ++++++++++--------- 1 file changed, 39 insertions(+), 32 deletions(-) diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index 7329ad84f..66e48fef7 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -97,7 +97,7 @@ class TLSGenerateServerHelloTest(unittest.TestCase): tls_generate_server_hello server_hello = tls_generate_server_hello("Q!") self.assertEqual(server_hello[:11].encode("hex"), - '160303002a020000260303') + '160303002a020000260303') self.assertEqual(server_hello[43:], chr(0) + 'Q!' + chr(0)) @@ -107,26 +107,29 @@ class TLSGenerateCertMsgTest(unittest.TestCase): from letsencrypt.client.standalone_authenticator import \ tls_generate_cert_msg cert = pkg_resources.resource_string(__name__, - 'testdata/cert.pem') + 'testdata/cert.pem') cert_msg = tls_generate_cert_msg(cert) self.assertEqual(cert_msg.encode("hex"), - "16030301ec0b0001e80001e50001e2308201de30820188a0030201020202" - "0539300d06092a864886f70d01010b05003077310b300906035504061302" - "55533111300f06035504080c084d6963686967616e311230100603550407" - "0c09416e6e204172626f72312b3029060355040a0c22556e697665727369" - "7479206f66204d6963686967616e20616e64207468652045464631143012" - "06035504030c0b6578616d706c652e636f6d301e170d3134313231313232" - "333434355a170d3134313231383232333434355a3077310b300906035504" - "06130255533111300f06035504080c084d6963686967616e311230100603" - "5504070c09416e6e204172626f72312b3029060355040a0c22556e697665" - "7273697479206f66204d6963686967616e20616e64207468652045464631" - "14301206035504030c0b6578616d706c652e636f6d305c300d06092a8648" - "86f70d0101010500034b003048024100ac7573b451ed1fddae705243fcdf" - "c75bd02c751b14b875010410e51f036545dddfa79f34aefdbee90584df47" - "1681d9894bce8e6d1cfa9544e8af84744fedc2e50203010001300d06092a" - "864886f70d01010b05000341002db8cf421dc0854a4a59ed92c965bebeb3" - "25ea411f97cc9dd7e4dd7269d748d3e9513ed7828db63874d9ae7a1a8ada" - "02f2404f9fc7ebb13c1af27fa1c36707fa") + "16030301ec0b0001e80001e50001e2308201de30820188a003" + "02010202020539300d06092a864886f70d01010b0500307731" + "0b30090603550406130255533111300f06035504080c084d69" + "63686967616e3112301006035504070c09416e6e204172626f" + "72312b3029060355040a0c22556e6976657273697479206f66" + "204d6963686967616e20616e64207468652045464631143012" + "06035504030c0b6578616d706c652e636f6d301e170d313431" + "3231313232333434355a170d3134313231383232333434355a" + "3077310b30090603550406130255533111300f06035504080c" + "084d6963686967616e3112301006035504070c09416e6e2041" + "72626f72312b3029060355040a0c22556e6976657273697479" + "206f66204d6963686967616e20616e64207468652045464631" + "14301206035504030c0b6578616d706c652e636f6d305c300d" + "06092a864886f70d0101010500034b003048024100ac7573b4" + "51ed1fddae705243fcdfc75bd02c751b14b875010410e51f03" + "6545dddfa79f34aefdbee90584df471681d9894bce8e6d1cfa" + "9544e8af84744fedc2e50203010001300d06092a864886f70d" + "01010b05000341002db8cf421dc0854a4a59ed92c965bebeb3" + "25ea411f97cc9dd7e4dd7269d748d3e9513ed7828db63874d9" + "ae7a1a8ada02f2404f9fc7ebb13c1af27fa1c36707fa") class TLSServerHelloDoneTest(unittest.TestCase): @@ -147,7 +150,7 @@ class ChallPrefTest(unittest.TestCase): def test_chall_pref(self): self.assertEqual(self.authenticator.get_chall_pref("example.com"), - ["dvsni"]) + ["dvsni"]) class SNICallbackTest(unittest.TestCase): @@ -158,15 +161,16 @@ class SNICallbackTest(unittest.TestCase): from letsencrypt.client.challenge_util import dvsni_gen_cert from letsencrypt.client import le_util import OpenSSL.crypto + from OpenSSL.crypto import FILETYPE_PEM self.authenticator = StandaloneAuthenticator() r = "x" * 32 name, r_b64 = "example.com", le_util.jose_b64encode(r) RSA256_KEY = pkg_resources.resource_string(__name__, - 'testdata/rsa256_key.pem') + 'testdata/rsa256_key.pem') nonce, key = "abcdef", le_util.Key("foo", RSA256_KEY) self.cert = dvsni_gen_cert(name, r_b64, nonce, key)[0] - self.authenticator.private_key = OpenSSL.crypto.load_privatekey( - OpenSSL.crypto.FILETYPE_PEM, key.pem) + private_key = OpenSSL.crypto.load_privatekey(FILETYPE_PEM, key.pem) + self.authenticator.private_key = private_key self.authenticator.tasks = {"abcdef.acme.invalid": self.cert} self.authenticator.child_pid = 12345 @@ -230,7 +234,7 @@ class SubprocSignalHandlerTest(unittest.TestCase): # do so in practice if there's no live TLS connection at the # time the subprocess is told to clean up). mock_kill.assert_called_once_with(self.authenticator.parent_pid, - signal.SIGUSR1) + signal.SIGUSR1) mock_exit.assert_called_once_with(0) @@ -245,7 +249,7 @@ class PerformTest(unittest.TestCase): """What happens if start_listener() returns True.""" from letsencrypt.client import le_util RSA256_KEY = pkg_resources.resource_string(__name__, - 'testdata/rsa256_key.pem') + 'testdata/rsa256_key.pem') key = le_util.Key("something", RSA256_KEY) chall1 = DvsniChall("foo.example.com", "whee", "foononce", key) chall2 = DvsniChall("bar.example.com", "whee", "barnonce", key) @@ -271,7 +275,7 @@ class PerformTest(unittest.TestCase): """What happens if start_listener() returns False.""" from letsencrypt.client import le_util RSA256_KEY = pkg_resources.resource_string(__name__, - 'testdata/rsa256_key.pem') + 'testdata/rsa256_key.pem') key = le_util.Key("something", RSA256_KEY) chall1 = DvsniChall("foo.example.com", "whee", "foononce", key) chall2 = DvsniChall("bar.example.com", "whee", "barnonce", key) @@ -316,7 +320,7 @@ class StartListenerTest(unittest.TestCase): self.authenticator.start_listener(1717, "key") self.assertEqual(self.authenticator.child_pid, os.getpid()) self.authenticator.do_child_process.assert_called_once_with(1717, - "key") + "key") mock_atfork.assert_called_once_with() class DoParentProcessTest(unittest.TestCase): @@ -369,23 +373,25 @@ class DoChildProcessTest(unittest.TestCase): from letsencrypt.client.challenge_util import dvsni_gen_cert from letsencrypt.client import le_util import OpenSSL.crypto + from OpenSSL.crypto import FILETYPE_PEM self.authenticator = StandaloneAuthenticator() r = "x" * 32 name, r_b64 = "example.com", le_util.jose_b64encode(r) RSA256_KEY = pkg_resources.resource_string(__name__, - 'testdata/rsa256_key.pem') + 'testdata/rsa256_key.pem') nonce, key = "abcdef", le_util.Key("foo", RSA256_KEY) self.key = key self.cert = dvsni_gen_cert(name, r_b64, nonce, key)[0] - self.authenticator.private_key = OpenSSL.crypto.load_privatekey( - OpenSSL.crypto.FILETYPE_PEM, key.pem) + private_key = OpenSSL.crypto.load_privatekey(FILETYPE_PEM, key.pem) + self.authenticator.private_key = private_key self.authenticator.tasks = {"abcdef.acme.invalid": self.cert} self.authenticator.parent_pid = 12345 @mock.patch("letsencrypt.client.standalone_authenticator.socket.socket") @mock.patch("letsencrypt.client.standalone_authenticator.os.kill") @mock.patch("letsencrypt.client.standalone_authenticator.sys.exit") - def test_do_child_process_cantbind1(self, mock_exit, mock_kill, mock_socket): + def test_do_child_process_cantbind1(self, mock_exit, mock_kill, + mock_socket): import socket, signal mock_exit.side_effect = IndentationError("subprocess would exit here") eaccess = socket.error(socket.errno.EACCES, "Permission denied") @@ -406,7 +412,8 @@ class DoChildProcessTest(unittest.TestCase): @mock.patch("letsencrypt.client.standalone_authenticator.socket.socket") @mock.patch("letsencrypt.client.standalone_authenticator.os.kill") @mock.patch("letsencrypt.client.standalone_authenticator.sys.exit") - def test_do_child_process_cantbind2(self, mock_exit, mock_kill, mock_socket): + def test_do_child_process_cantbind2(self, mock_exit, mock_kill, + mock_socket): import socket, signal mock_exit.side_effect = IndentationError("subprocess would exit here") eaccess = socket.error(socket.errno.EADDRINUSE, "Port already in use") From b61708c47fa6c073303e1c3eaffbceab156f3de7 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Fri, 6 Feb 2015 15:08:48 -0800 Subject: [PATCH 18/45] Improving test coverage --- .../tests/standalone_authenticator_test.py | 38 +++++++++++++++++-- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index 66e48fef7..0fd34bfff 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -207,6 +207,14 @@ class ClientSignalHandlerTest(unittest.TestCase): self.authenticator.client_signal_handler(signal.SIGUSR2, None) self.assertTrue(self.authenticator.subproc_cantbind) + # Testing the unreached path for a signal other than these + # specified (which can't occur in normal use because this + # function is only set as a signal handler for the above three + # signals). + with self.assertRaises(AssertionError): + self.authenticator.client_signal_handler(signal.SIGPIPE, None) + + class SubprocSignalHandlerTest(unittest.TestCase): """Tests for subproc_signal_handler() method.""" def setUp(self): @@ -229,10 +237,32 @@ class SubprocSignalHandlerTest(unittest.TestCase): self.assertEquals(self.authenticator.ssl_conn.close.call_count, 1) self.assertEquals(self.authenticator.connection.close.call_count, 1) self.assertEquals(self.authenticator.sock.close.call_count, 1) - # TODO: We should test that we correctly survive each of the above - # raising an exception of some kind (since they're likely to - # do so in practice if there's no live TLS connection at the - # time the subprocess is told to clean up). + mock_kill.assert_called_once_with(self.authenticator.parent_pid, + signal.SIGUSR1) + mock_exit.assert_called_once_with(0) + + @mock.patch("letsencrypt.client.standalone_authenticator.os.kill") + @mock.patch("letsencrypt.client.standalone_authenticator.sys.exit") + def test_subproc_signal_handler_trouble(self, mock_exit, mock_kill): + """Test how the signal handler survives attempting to shut down + a non-existent connection (because none was established or active + at the time the signal handler tried to perform the cleanup).""" + import signal + self.authenticator.ssl_conn = mock.MagicMock() + self.authenticator.connection = mock.MagicMock() + self.authenticator.sock = mock.MagicMock() + # AttributeError simulates the case where one of these properties + # is None because no connection exists. We raise it for + # ssl_conn.close() instead of ssl_conn.shutdown() for better code + # coverage. + self.authenticator.ssl_conn.close.side_effect = AttributeError("!") + self.authenticator.connection.close.side_effect = AttributeError("!") + self.authenticator.sock.close.side_effect = AttributeError("!") + self.authenticator.subproc_signal_handler(signal.SIGINT, None) + self.assertEquals(self.authenticator.ssl_conn.shutdown.call_count, 1) + self.assertEquals(self.authenticator.ssl_conn.close.call_count, 1) + self.assertEquals(self.authenticator.connection.close.call_count, 1) + self.assertEquals(self.authenticator.sock.close.call_count, 1) mock_kill.assert_called_once_with(self.authenticator.parent_pid, signal.SIGUSR1) mock_exit.assert_called_once_with(0) From 65de5fa71eca68401ca5661a57c6dca335321a51 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Fri, 6 Feb 2015 15:21:00 -0800 Subject: [PATCH 19/45] Further improvements to test coverage --- .../tests/standalone_authenticator_test.py | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index 0fd34bfff..142e57de1 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -183,6 +183,21 @@ class SNICallbackTest(unittest.TestCase): called_ctx = connection.set_context.call_args[0][0] self.assertIsInstance(called_ctx, OpenSSL.SSL.Context) + def test_fake_servername(self): + """Test the behavior of the SNI callback when an unexpected SNI + name is received. (Currently the expected behavior in this case + is to return the "first" certificate with which the listener + was configured, although they are stored in an unordered data + structure so this might not be the one that was first in the + challenge list passed to the perform method. In the future, this + might result in dropping the connection instead.)""" + import OpenSSL.SSL + connection = mock.MagicMock() + connection.get_servername.return_value = "example.com" + self.authenticator.sni_callback(connection) + self.assertEqual(connection.set_context.call_count, 1) + called_ctx = connection.set_context.call_args[0][0] + self.assertIsInstance(called_ctx, OpenSSL.SSL.Context) class ClientSignalHandlerTest(unittest.TestCase): """Tests for client_signal_handler() method.""" @@ -455,6 +470,20 @@ class DoChildProcessTest(unittest.TestCase): mock_exit.assert_called_once_with(1) mock_kill.assert_called_once_with(12345, signal.SIGUSR1) + @mock.patch("letsencrypt.client.standalone_authenticator.socket.socket") + def test_do_child_process_cantbind3(self, mock_socket): + """Test case where attempt to bind socket results in an unhandled + socket error. (The expected behavior is arguably wrong because it + will crash the program; the reason for the expected behavior is + that we don't have a way to report arbitrary socket errors.)""" + import socket, signal + eio = socket.error(socket.errno.EIO, "Imaginary unhandled error") + sample_socket = mock.MagicMock() + sample_socket.bind.side_effect = eio + mock_socket.return_value = sample_socket + with self.assertRaises(socket.error): + result = self.authenticator.do_child_process(1717, self.key) + @mock.patch("letsencrypt.client.standalone_authenticator.OpenSSL.SSL.Connection") @mock.patch("letsencrypt.client.standalone_authenticator.socket.socket") @mock.patch("letsencrypt.client.standalone_authenticator.os.kill") From 5a9e394827e09a66d7bbfd1f0bbd4fc7ae8ba05c Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Fri, 6 Feb 2015 15:25:12 -0800 Subject: [PATCH 20/45] Test trying to perform challenges with others pending --- letsencrypt/client/tests/standalone_authenticator_test.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index 142e57de1..891954b2c 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -338,6 +338,13 @@ class PerformTest(unittest.TestCase): self.assertEqual(result, [None, None, False]) self.authenticator.start_listener.assert_called_once_with(443, key) + def test_perform_with_pending_tasks(self): + self.authenticator.tasks = {"foononce.acme.invalid": "cert_data"} + extra_challenge = DvsniChall("a", "b", "c", "d") + with self.assertRaises(Exception): + self.authenticator.perform([extra_challenge]) + + class StartListenerTest(unittest.TestCase): """Tests for start_listener() method.""" def setUp(self): From 31b1369752a889cb4fa4558100117efca00b7a59 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Fri, 6 Feb 2015 15:28:27 -0800 Subject: [PATCH 21/45] Improve coverage for perform() error cases --- .../client/tests/standalone_authenticator_test.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index 891954b2c..5eed38b8a 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -344,6 +344,18 @@ class PerformTest(unittest.TestCase): with self.assertRaises(Exception): self.authenticator.perform([extra_challenge]) + def test_perform_without_challenge_list(self): + extra_challenge = DvsniChall("a", "b", "c", "d") + # This is wrong because a challenge must be specified. + with self.assertRaises(Exception): + self.authenticator.perform([]) + # This is wrong because it must be a list, not a bare challenge. + with self.assertRaises(Exception): + self.authenticator.perform(extra_challenge) + # This is wrong because the list must contain at least one challenge. + with self.assertRaises(Exception): + self.authenticator.perform(range(20)) + class StartListenerTest(unittest.TestCase): """Tests for start_listener() method.""" From 52d19898504b3e65228d0d6c019df0dd29f42333 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Fri, 6 Feb 2015 15:54:08 -0800 Subject: [PATCH 22/45] Complete move of Key into le_util --- letsencrypt/client/client.py | 2 +- letsencrypt/client/tests/apache/configurator_test.py | 3 ++- letsencrypt/client/tests/apache/dvsni_test.py | 4 ++-- letsencrypt/client/tests/challenge_util_test.py | 2 +- letsencrypt/scripts/main.py | 3 ++- 5 files changed, 8 insertions(+), 6 deletions(-) diff --git a/letsencrypt/client/client.py b/letsencrypt/client/client.py index a2b6dea9b..4abfeb0c9 100644 --- a/letsencrypt/client/client.py +++ b/letsencrypt/client/client.py @@ -22,7 +22,7 @@ from letsencrypt.client import reverter from letsencrypt.client import revoker from letsencrypt.client.apache import configurator -from letsencrypt.le_util import Key +from letsencrypt.client.le_util import Key class Client(object): diff --git a/letsencrypt/client/tests/apache/configurator_test.py b/letsencrypt/client/tests/apache/configurator_test.py index fc71dfbee..10c2f21cc 100644 --- a/letsencrypt/client/tests/apache/configurator_test.py +++ b/letsencrypt/client/tests/apache/configurator_test.py @@ -15,6 +15,7 @@ from letsencrypt.client.apache import obj from letsencrypt.client.apache import parser from letsencrypt.client.tests.apache import util +from letsencrypt.client.le_util import Key class TwoVhost80Test(util.ApacheTest): @@ -164,7 +165,7 @@ class TwoVhost80Test(util.ApacheTest): def test_perform(self, mock_restart, mock_dvsni_perform): # Only tests functionality specific to configurator.perform # Note: As more challenges are offered this will have to be expanded - auth_key = client.Client.Key(self.rsa256_file, self.rsa256_pem) + auth_key = Key(self.rsa256_file, self.rsa256_pem) chall1 = challenge_util.DvsniChall( "encryption-example.demo", "jIq_Xy1mXGN37tb4L6Xj_es58fW571ZNyXekdZzhh7Q", diff --git a/letsencrypt/client/tests/apache/dvsni_test.py b/letsencrypt/client/tests/apache/dvsni_test.py index a50f0a3f6..13f120e3c 100644 --- a/letsencrypt/client/tests/apache/dvsni_test.py +++ b/letsencrypt/client/tests/apache/dvsni_test.py @@ -10,7 +10,7 @@ from letsencrypt.client import client from letsencrypt.client import CONFIG from letsencrypt.client.tests.apache import util - +from letsencrypt.client.le_util import Key class DvsniPerformTest(util.ApacheTest): """Test the ApacheDVSNI challenge.""" @@ -33,7 +33,7 @@ class DvsniPerformTest(util.ApacheTest): rsa256_pem = pkg_resources.resource_string( "letsencrypt.client.tests", 'testdata/rsa256_key.pem') - auth_key = client.Client.Key(rsa256_file, rsa256_pem) + auth_key = Key(rsa256_file, rsa256_pem) self.challs = [] self.challs.append(challenge_util.DvsniChall( "encryption-example.demo", diff --git a/letsencrypt/client/tests/challenge_util_test.py b/letsencrypt/client/tests/challenge_util_test.py index 84a561d5d..28e7e17c3 100644 --- a/letsencrypt/client/tests/challenge_util_test.py +++ b/letsencrypt/client/tests/challenge_util_test.py @@ -23,7 +23,7 @@ class DvsniGenCertTest(unittest.TestCase): r_b64 = le_util.jose_b64encode(dvsni_r) pem = pkg_resources.resource_string( __name__, os.path.join("testdata", "rsa256_key.pem")) - key = client.Client.Key("path", pem) + key = le_util.Key("path", pem) nonce = "12345ABCDE" cert_pem, s_b64 = self._call(domain, r_b64, nonce, key) diff --git a/letsencrypt/scripts/main.py b/letsencrypt/scripts/main.py index b79455c9f..7507b0620 100755 --- a/letsencrypt/scripts/main.py +++ b/letsencrypt/scripts/main.py @@ -19,6 +19,7 @@ from letsencrypt.client import display from letsencrypt.client import errors from letsencrypt.client import interfaces from letsencrypt.client import log +from letsencrypt.client.le_util import Key def main(): # pylint: disable=too-many-statements,too-many-branches @@ -118,7 +119,7 @@ def main(): # pylint: disable=too-many-statements,too-many-branches if args.privkey is None: privkey = client.init_key(args.key_size) else: - privkey = client.Client.Key(args.privkey[0], args.privkey[1]) + privkey = Key(args.privkey[0], args.privkey[1]) acme = client.Client(args.server, privkey, auth, installer) From 4b8eae1084014ac38a2eb0a9e82c981bc409b2e5 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Fri, 6 Feb 2015 16:12:37 -0800 Subject: [PATCH 23/45] Small changes to try to make pylint happier --- .../tests/standalone_authenticator_test.py | 39 +++++++++++-------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index 5eed38b8a..7710e540d 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -8,12 +8,11 @@ import pkg_resources from letsencrypt.client.challenge_util import DvsniChall -# ErrorAfter/CallableExhausted from +# Classes based on to allow interrupting infinite loop under test +# after one iteration, based on. # http://igorsobreira.com/2013/03/17/testing-infinite-loops.html -# to allow interrupting infinite loop under test after one -# iteration. -class ErrorAfter_socket_accept(object): +class SocketAcceptOnlyNTimes(object): """ Callable that will raise `CallableExhausted` exception after `limit` calls, modified to also return @@ -32,6 +31,8 @@ class ErrorAfter_socket_accept(object): return (mock.MagicMock(), "ignored") class CallableExhausted(Exception): + """Exception raised when a method is called more than the + specified number of times.""" pass @@ -396,36 +397,40 @@ class DoParentProcessTest(unittest.TestCase): @mock.patch("letsencrypt.client.standalone_authenticator.signal.signal") @mock.patch("letsencrypt.client.standalone_authenticator.zope.component.getUtility") - def test_do_parent_process_ok(self, mock_getUtility, mock_signal): + def test_do_parent_process_ok(self, mock_get_utility, mock_signal): self.authenticator.subproc_ready = True result = self.authenticator.do_parent_process(1717) self.assertTrue(result) + self.assertEqual(mock_get_utility.call_count, 1) self.assertEqual(mock_signal.call_count, 3) @mock.patch("letsencrypt.client.standalone_authenticator.signal.signal") @mock.patch("letsencrypt.client.standalone_authenticator.zope.component.getUtility") - def test_do_parent_process_inuse(self, mock_getUtility, mock_signal): + def test_do_parent_process_inuse(self, mock_get_utility, mock_signal): self.authenticator.subproc_inuse = True result = self.authenticator.do_parent_process(1717) self.assertFalse(result) + self.assertEqual(mock_get_utility.call_count, 1) self.assertEqual(mock_signal.call_count, 3) @mock.patch("letsencrypt.client.standalone_authenticator.signal.signal") @mock.patch("letsencrypt.client.standalone_authenticator.zope.component.getUtility") - def test_do_parent_process_cantbind(self, mock_getUtility, mock_signal): + def test_do_parent_process_cantbind(self, mock_get_utility, mock_signal): self.authenticator.subproc_cantbind = True result = self.authenticator.do_parent_process(1717) self.assertFalse(result) + self.assertEqual(mock_get_utility.call_count, 1) self.assertEqual(mock_signal.call_count, 3) @mock.patch("letsencrypt.client.standalone_authenticator.signal.signal") @mock.patch("letsencrypt.client.standalone_authenticator.zope.component.getUtility") - def test_do_parent_process_timeout(self, mock_getUtility, mock_signal): + def test_do_parent_process_timeout(self, mock_get_utility, mock_signal): # Normally times out in 5 seconds and returns False. We can # now set delay_amount to a lower value so that it times out # faster than it would under normal use. result = self.authenticator.do_parent_process(1717, delay_amount=1) self.assertFalse(result) + self.assertEqual(mock_get_utility.call_count, 1) self.assertEqual(mock_signal.call_count, 3) @@ -469,7 +474,7 @@ class DoChildProcessTest(unittest.TestCase): # do_child_process code assumes that calling sys.exit() will # cause subsequent code not to be executed.) with self.assertRaises(IndentationError): - result = self.authenticator.do_child_process(1717, self.key) + self.authenticator.do_child_process(1717, self.key) mock_exit.assert_called_once_with(1) mock_kill.assert_called_once_with(12345, signal.SIGUSR2) @@ -485,7 +490,7 @@ class DoChildProcessTest(unittest.TestCase): sample_socket.bind.side_effect = eaccess mock_socket.return_value = sample_socket with self.assertRaises(IndentationError): - result = self.authenticator.do_child_process(1717, self.key) + self.authenticator.do_child_process(1717, self.key) mock_exit.assert_called_once_with(1) mock_kill.assert_called_once_with(12345, signal.SIGUSR1) @@ -495,25 +500,25 @@ class DoChildProcessTest(unittest.TestCase): socket error. (The expected behavior is arguably wrong because it will crash the program; the reason for the expected behavior is that we don't have a way to report arbitrary socket errors.)""" - import socket, signal + import socket eio = socket.error(socket.errno.EIO, "Imaginary unhandled error") sample_socket = mock.MagicMock() sample_socket.bind.side_effect = eio mock_socket.return_value = sample_socket with self.assertRaises(socket.error): - result = self.authenticator.do_child_process(1717, self.key) + self.authenticator.do_child_process(1717, self.key) @mock.patch("letsencrypt.client.standalone_authenticator.OpenSSL.SSL.Connection") @mock.patch("letsencrypt.client.standalone_authenticator.socket.socket") @mock.patch("letsencrypt.client.standalone_authenticator.os.kill") - def test_do_child_process_success(self, mock_kill, mock_socket, mock_Connection): - import socket, signal + def test_do_child_process_success(self, mock_kill, mock_socket, mock_connection): + import signal sample_socket = mock.MagicMock() - sample_socket.accept.side_effect = ErrorAfter_socket_accept(2) + sample_socket.accept.side_effect = SocketAcceptOnlyNTimes(2) mock_socket.return_value = sample_socket - mock_Connection.return_value = mock.MagicMock() + mock_connection.return_value = mock.MagicMock() with self.assertRaises(CallableExhausted): - result = self.authenticator.do_child_process(1717, self.key) + self.authenticator.do_child_process(1717, self.key) mock_socket.assert_called_once_with() sample_socket.bind.assert_called_once_with(("0.0.0.0", 1717)) sample_socket.listen.assert_called_once_with(1) From 50a6a28e73c4d9821c18b3572478ac14acab23e5 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Fri, 6 Feb 2015 19:02:17 -0800 Subject: [PATCH 24/45] Exclude tls_parse_client_hello from coverage (As an alternative to commenting out this function, which is currently totally unused. The function does have tests; they just don't cover the failure cases.) --- letsencrypt/client/standalone_authenticator.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/letsencrypt/client/standalone_authenticator.py b/letsencrypt/client/standalone_authenticator.py index d68a43b47..b4e32dc68 100644 --- a/letsencrypt/client/standalone_authenticator.py +++ b/letsencrypt/client/standalone_authenticator.py @@ -50,7 +50,9 @@ def pack_3bytes(value): return struct.pack(">I", value)[1:] -def tls_parse_client_hello(tls_record): +# Exclude this function from coverage testing because it is currently +# not used. +def tls_parse_client_hello(tls_record): # pragma: no cover # pylint: disable=too-many-return-statements """If possible, parse the specified TLS record as a ClientHello and return the first host_name indicated in a Server Name Indication From 76fb3b54e29a6ce3f904f851e4e1be53398a1e23 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Sun, 8 Feb 2015 11:06:04 -0800 Subject: [PATCH 25/45] Satisfy pylint on various points --- .../client/standalone_authenticator.py | 21 +++---- .../tests/standalone_authenticator_test.py | 60 +++++++++++-------- 2 files changed, 47 insertions(+), 34 deletions(-) diff --git a/letsencrypt/client/standalone_authenticator.py b/letsencrypt/client/standalone_authenticator.py index b4e32dc68..e7f4e276b 100644 --- a/letsencrypt/client/standalone_authenticator.py +++ b/letsencrypt/client/standalone_authenticator.py @@ -53,7 +53,8 @@ def pack_3bytes(value): # Exclude this function from coverage testing because it is currently # not used. def tls_parse_client_hello(tls_record): # pragma: no cover - # pylint: disable=too-many-return-statements + # pylint: disable=too-many-return-statements,too-many-locals,bad-builtin + # pylint: disable=too-many-branches """If possible, parse the specified TLS record as a ClientHello and return the first host_name indicated in a Server Name Indication extension within that ClientHello. If the TLS record could not @@ -228,6 +229,7 @@ def tls_generate_server_hello_done(): class StandaloneAuthenticator(object): + # pylint: disable=too-many-instance-attributes """The StandaloneAuthenticator class itself, which can be invoked by the Let's Encrypt client according to the IAuthenticator API interface.""" @@ -236,9 +238,7 @@ class StandaloneAuthenticator(object): def __init__(self): self.child_pid = None self.parent_pid = os.getpid() - self.subproc_ready = False - self.subproc_inuse = False - self.subproc_cantbind = False + self.subproc_state = None self.tasks = {} self.sock = None self.connection = None @@ -254,11 +254,11 @@ class StandaloneAuthenticator(object): # subprocess → client INUSE : SIGUSR1 # subprocess → client CANTBIND: SIGUSR2 if sig == signal.SIGIO: - self.subproc_ready = True + self.subproc_state = "ready" elif sig == signal.SIGUSR1: - self.subproc_inuse = True + self.subproc_state = "inuse" elif sig == signal.SIGUSR2: - self.subproc_cantbind = True + self.subproc_state = "cantbind" else: # NOTREACHED assert False @@ -323,15 +323,15 @@ class StandaloneAuthenticator(object): display = zope.component.getUtility(interfaces.IDisplay) start_time = time.time() while time.time() < start_time + delay_amount: - if self.subproc_ready: + if self.subproc_state == "ready": return True - if self.subproc_inuse: + if self.subproc_state == "inuse": display.generic_notification( "Could not bind TCP port {} because it is already in " "use it is already in use by another process on this " "system (such as a web server).".format(port)) return False - if self.subproc_cantbind: + if self.subproc_state == "cantbind": display.generic_notification( "Could not bind TCP port {} because you don't have " "the appropriate permissions (for example, you " @@ -437,6 +437,7 @@ class StandaloneAuthenticator(object): # IAuthenticator method implementations follow def get_chall_pref(self, unused_domain): + # pylint: disable=no-self-use """IAuthenticator interface method: Return a list of challenge types that this authenticator can perform for this domain. In the case of the StandaloneAuthenticator, the only challenge diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index 7710e540d..244b60685 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -13,6 +13,7 @@ from letsencrypt.client.challenge_util import DvsniChall # http://igorsobreira.com/2013/03/17/testing-infinite-loops.html class SocketAcceptOnlyNTimes(object): + # pylint: disable=too-few-public-methods """ Callable that will raise `CallableExhausted` exception after `limit` calls, modified to also return @@ -31,6 +32,7 @@ class SocketAcceptOnlyNTimes(object): return (mock.MagicMock(), "ignored") class CallableExhausted(Exception): + # pylint: disable=too-few-public-methods """Exception raised when a method is called more than the specified number of times.""" pass @@ -70,6 +72,7 @@ class PackAndUnpackTests(unittest.TestCase): class TLSParseClientHelloTest(unittest.TestCase): + # pylint: disable=too-few-public-methods """Test for tls_parse_client_hello() function.""" def test_tls_parse_client_hello(self): from letsencrypt.client.standalone_authenticator import \ @@ -92,6 +95,7 @@ class TLSParseClientHelloTest(unittest.TestCase): class TLSGenerateServerHelloTest(unittest.TestCase): + # pylint: disable=too-few-public-methods """Tests for tls_generate_server_hello() function.""" def test_tls_generate_server_hello(self): from letsencrypt.client.standalone_authenticator import \ @@ -103,6 +107,7 @@ class TLSGenerateServerHelloTest(unittest.TestCase): class TLSGenerateCertMsgTest(unittest.TestCase): + # pylint: disable=too-few-public-methods """Tests for tls_generate_cert_msg() function.""" def test_tls_generate_cert_msg(self): from letsencrypt.client.standalone_authenticator import \ @@ -134,6 +139,7 @@ class TLSGenerateCertMsgTest(unittest.TestCase): class TLSServerHelloDoneTest(unittest.TestCase): + # pylint: disable=too-few-public-methods """Tests for tls_generate_server_hello_done() function.""" def test_tls_generate_server_hello_done(self): from letsencrypt.client.standalone_authenticator import \ @@ -164,11 +170,10 @@ class SNICallbackTest(unittest.TestCase): import OpenSSL.crypto from OpenSSL.crypto import FILETYPE_PEM self.authenticator = StandaloneAuthenticator() - r = "x" * 32 - name, r_b64 = "example.com", le_util.jose_b64encode(r) - RSA256_KEY = pkg_resources.resource_string(__name__, - 'testdata/rsa256_key.pem') - nonce, key = "abcdef", le_util.Key("foo", RSA256_KEY) + name, r_b64 = "example.com", le_util.jose_b64encode("x" * 32) + test_key = pkg_resources.resource_string(__name__, + 'testdata/rsa256_key.pem') + nonce, key = "abcdef", le_util.Key("foo", test_key) self.cert = dvsni_gen_cert(name, r_b64, nonce, key)[0] private_key = OpenSSL.crypto.load_privatekey(FILETYPE_PEM, key.pem) self.authenticator.private_key = private_key @@ -294,9 +299,9 @@ class PerformTest(unittest.TestCase): def test_can_perform(self): """What happens if start_listener() returns True.""" from letsencrypt.client import le_util - RSA256_KEY = pkg_resources.resource_string(__name__, - 'testdata/rsa256_key.pem') - key = le_util.Key("something", RSA256_KEY) + test_key = pkg_resources.resource_string(__name__, + 'testdata/rsa256_key.pem') + key = le_util.Key("something", test_key) chall1 = DvsniChall("foo.example.com", "whee", "foononce", key) chall2 = DvsniChall("bar.example.com", "whee", "barnonce", key) bad_chall = ("This", "Represents", "A Non-DVSNI", "Challenge") @@ -320,9 +325,9 @@ class PerformTest(unittest.TestCase): def test_cannot_perform(self): """What happens if start_listener() returns False.""" from letsencrypt.client import le_util - RSA256_KEY = pkg_resources.resource_string(__name__, - 'testdata/rsa256_key.pem') - key = le_util.Key("something", RSA256_KEY) + test_key = pkg_resources.resource_string(__name__, + 'testdata/rsa256_key.pem') + key = le_util.Key("something", test_key) chall1 = DvsniChall("foo.example.com", "whee", "foononce", key) chall2 = DvsniChall("bar.example.com", "whee", "barnonce", key) bad_chall = ("This", "Represents", "A Non-DVSNI", "Challenge") @@ -365,7 +370,8 @@ class StartListenerTest(unittest.TestCase): StandaloneAuthenticator self.authenticator = StandaloneAuthenticator() - @mock.patch("letsencrypt.client.standalone_authenticator.Crypto.Random.atfork") + @mock.patch("letsencrypt.client.standalone_authenticator." + "Crypto.Random.atfork") @mock.patch("letsencrypt.client.standalone_authenticator.os.fork") def test_start_listener_fork_parent(self, mock_fork, mock_atfork): self.authenticator.do_parent_process = mock.Mock() @@ -375,7 +381,8 @@ class StartListenerTest(unittest.TestCase): self.authenticator.do_parent_process.assert_called_once_with(1717) mock_atfork.assert_called_once_with() - @mock.patch("letsencrypt.client.standalone_authenticator.Crypto.Random.atfork") + @mock.patch("letsencrypt.client.standalone_authenticator." + "Crypto.Random.atfork") @mock.patch("letsencrypt.client.standalone_authenticator.os.fork") def test_start_listener_fork_child(self, mock_fork, mock_atfork): import os @@ -396,7 +403,8 @@ class DoParentProcessTest(unittest.TestCase): self.authenticator = StandaloneAuthenticator() @mock.patch("letsencrypt.client.standalone_authenticator.signal.signal") - @mock.patch("letsencrypt.client.standalone_authenticator.zope.component.getUtility") + @mock.patch("letsencrypt.client.standalone_authenticator." + "zope.component.getUtility") def test_do_parent_process_ok(self, mock_get_utility, mock_signal): self.authenticator.subproc_ready = True result = self.authenticator.do_parent_process(1717) @@ -405,7 +413,8 @@ class DoParentProcessTest(unittest.TestCase): self.assertEqual(mock_signal.call_count, 3) @mock.patch("letsencrypt.client.standalone_authenticator.signal.signal") - @mock.patch("letsencrypt.client.standalone_authenticator.zope.component.getUtility") + @mock.patch("letsencrypt.client.standalone_authenticator." + "zope.component.getUtility") def test_do_parent_process_inuse(self, mock_get_utility, mock_signal): self.authenticator.subproc_inuse = True result = self.authenticator.do_parent_process(1717) @@ -414,7 +423,8 @@ class DoParentProcessTest(unittest.TestCase): self.assertEqual(mock_signal.call_count, 3) @mock.patch("letsencrypt.client.standalone_authenticator.signal.signal") - @mock.patch("letsencrypt.client.standalone_authenticator.zope.component.getUtility") + @mock.patch("letsencrypt.client.standalone_authenticator." + "zope.component.getUtility") def test_do_parent_process_cantbind(self, mock_get_utility, mock_signal): self.authenticator.subproc_cantbind = True result = self.authenticator.do_parent_process(1717) @@ -423,7 +433,8 @@ class DoParentProcessTest(unittest.TestCase): self.assertEqual(mock_signal.call_count, 3) @mock.patch("letsencrypt.client.standalone_authenticator.signal.signal") - @mock.patch("letsencrypt.client.standalone_authenticator.zope.component.getUtility") + @mock.patch("letsencrypt.client.standalone_authenticator." + "zope.component.getUtility") def test_do_parent_process_timeout(self, mock_get_utility, mock_signal): # Normally times out in 5 seconds and returns False. We can # now set delay_amount to a lower value so that it times out @@ -444,11 +455,10 @@ class DoChildProcessTest(unittest.TestCase): import OpenSSL.crypto from OpenSSL.crypto import FILETYPE_PEM self.authenticator = StandaloneAuthenticator() - r = "x" * 32 - name, r_b64 = "example.com", le_util.jose_b64encode(r) - RSA256_KEY = pkg_resources.resource_string(__name__, - 'testdata/rsa256_key.pem') - nonce, key = "abcdef", le_util.Key("foo", RSA256_KEY) + name, r_b64 = "example.com", le_util.jose_b64encode("x" * 32) + test_key = pkg_resources.resource_string(__name__, + 'testdata/rsa256_key.pem') + nonce, key = "abcdef", le_util.Key("foo", test_key) self.key = key self.cert = dvsni_gen_cert(name, r_b64, nonce, key)[0] private_key = OpenSSL.crypto.load_privatekey(FILETYPE_PEM, key.pem) @@ -508,10 +518,12 @@ class DoChildProcessTest(unittest.TestCase): with self.assertRaises(socket.error): self.authenticator.do_child_process(1717, self.key) - @mock.patch("letsencrypt.client.standalone_authenticator.OpenSSL.SSL.Connection") + @mock.patch("letsencrypt.client.standalone_authenticator." + "OpenSSL.SSL.Connection") @mock.patch("letsencrypt.client.standalone_authenticator.socket.socket") @mock.patch("letsencrypt.client.standalone_authenticator.os.kill") - def test_do_child_process_success(self, mock_kill, mock_socket, mock_connection): + def test_do_child_process_success(self, mock_kill, mock_socket, + mock_connection): import signal sample_socket = mock.MagicMock() sample_socket.accept.side_effect = SocketAcceptOnlyNTimes(2) From 30c11920d968a7db289d2eeedcd8424212892a7f Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Sun, 8 Feb 2015 11:09:02 -0800 Subject: [PATCH 26/45] Tests to follow new convention for subproc_state --- .../tests/standalone_authenticator_test.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index 244b60685..5c980539b 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -216,17 +216,15 @@ class ClientSignalHandlerTest(unittest.TestCase): def test_client_signal_handler(self): import signal - self.assertFalse(self.authenticator.subproc_ready) - self.assertFalse(self.authenticator.subproc_inuse) - self.assertFalse(self.authenticator.subproc_cantbind) + self.assertEqual(self.authenticator.subproc_state, None) self.authenticator.client_signal_handler(signal.SIGIO, None) - self.assertTrue(self.authenticator.subproc_ready) + self.assertEqual(self.authenticator.subproc_state, "ready") self.authenticator.client_signal_handler(signal.SIGUSR1, None) - self.assertTrue(self.authenticator.subproc_inuse) + self.assertEqual(self.authenticator.subproc_state, "inuse") self.authenticator.client_signal_handler(signal.SIGUSR2, None) - self.assertTrue(self.authenticator.subproc_cantbind) + self.assertEqual(self.authenticator.subproc_state, "cantbind") # Testing the unreached path for a signal other than these # specified (which can't occur in normal use because this @@ -406,7 +404,7 @@ class DoParentProcessTest(unittest.TestCase): @mock.patch("letsencrypt.client.standalone_authenticator." "zope.component.getUtility") def test_do_parent_process_ok(self, mock_get_utility, mock_signal): - self.authenticator.subproc_ready = True + self.authenticator.subproc_state = "ready" result = self.authenticator.do_parent_process(1717) self.assertTrue(result) self.assertEqual(mock_get_utility.call_count, 1) @@ -416,7 +414,7 @@ class DoParentProcessTest(unittest.TestCase): @mock.patch("letsencrypt.client.standalone_authenticator." "zope.component.getUtility") def test_do_parent_process_inuse(self, mock_get_utility, mock_signal): - self.authenticator.subproc_inuse = True + self.authenticator.subproc_state = "inuse" result = self.authenticator.do_parent_process(1717) self.assertFalse(result) self.assertEqual(mock_get_utility.call_count, 1) @@ -426,7 +424,7 @@ class DoParentProcessTest(unittest.TestCase): @mock.patch("letsencrypt.client.standalone_authenticator." "zope.component.getUtility") def test_do_parent_process_cantbind(self, mock_get_utility, mock_signal): - self.authenticator.subproc_cantbind = True + self.authenticator.subproc_state = "cantbind" result = self.authenticator.do_parent_process(1717) self.assertFalse(result) self.assertEqual(mock_get_utility.call_count, 1) From f6e192bfafd3b37145d0b329d08550c72009fcd0 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Sun, 8 Feb 2015 17:26:18 -0800 Subject: [PATCH 27/45] Remove redundant import of client --- letsencrypt/client/tests/apache/configurator_test.py | 1 - letsencrypt/client/tests/apache/dvsni_test.py | 1 - letsencrypt/client/tests/challenge_util_test.py | 1 - 3 files changed, 3 deletions(-) diff --git a/letsencrypt/client/tests/apache/configurator_test.py b/letsencrypt/client/tests/apache/configurator_test.py index 10c2f21cc..f8c983c31 100644 --- a/letsencrypt/client/tests/apache/configurator_test.py +++ b/letsencrypt/client/tests/apache/configurator_test.py @@ -7,7 +7,6 @@ import unittest import mock from letsencrypt.client import challenge_util -from letsencrypt.client import client from letsencrypt.client import errors from letsencrypt.client.apache import configurator diff --git a/letsencrypt/client/tests/apache/dvsni_test.py b/letsencrypt/client/tests/apache/dvsni_test.py index 13f120e3c..d09cc56e2 100644 --- a/letsencrypt/client/tests/apache/dvsni_test.py +++ b/letsencrypt/client/tests/apache/dvsni_test.py @@ -6,7 +6,6 @@ import shutil import mock from letsencrypt.client import challenge_util -from letsencrypt.client import client from letsencrypt.client import CONFIG from letsencrypt.client.tests.apache import util diff --git a/letsencrypt/client/tests/challenge_util_test.py b/letsencrypt/client/tests/challenge_util_test.py index 28e7e17c3..e59585b77 100644 --- a/letsencrypt/client/tests/challenge_util_test.py +++ b/letsencrypt/client/tests/challenge_util_test.py @@ -7,7 +7,6 @@ import unittest import M2Crypto from letsencrypt.client import challenge_util -from letsencrypt.client import client from letsencrypt.client import CONFIG from letsencrypt.client import le_util From 3453db09857bf51178e6bbbee65ffd2d01786c7f Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Sun, 8 Feb 2015 21:04:54 -0800 Subject: [PATCH 28/45] Eliminate use of Python 2.7-specific unittest features --- .../tests/standalone_authenticator_test.py | 63 +++++++++---------- 1 file changed, 29 insertions(+), 34 deletions(-) diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index 5c980539b..21a738363 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -61,14 +61,10 @@ class PackAndUnpackTests(unittest.TestCase): def test_invalid_pack_and_unpack(self): from letsencrypt.client.standalone_authenticator import \ unpack_2bytes, unpack_3bytes, pack_2bytes, pack_3bytes - with self.assertRaises(AssertionError): - pack_2bytes(65537) - with self.assertRaises(AssertionError): - pack_3bytes(500000000) - with self.assertRaises(AssertionError): - unpack_2bytes("foo") - with self.assertRaises(AssertionError): - unpack_3bytes("food") + self.assertRaises(AssertionError, pack_2bytes, 65537) + self.assertRaises(AssertionError, pack_3bytes, 500000000) + self.assertRaises(AssertionError, unpack_2bytes, "foo") + self.assertRaises(AssertionError, unpack_3bytes, "food") class TLSParseClientHelloTest(unittest.TestCase): @@ -187,7 +183,7 @@ class SNICallbackTest(unittest.TestCase): self.authenticator.sni_callback(connection) self.assertEqual(connection.set_context.call_count, 1) called_ctx = connection.set_context.call_args[0][0] - self.assertIsInstance(called_ctx, OpenSSL.SSL.Context) + self.assertTrue(isinstance(called_ctx, OpenSSL.SSL.Context)) def test_fake_servername(self): """Test the behavior of the SNI callback when an unexpected SNI @@ -203,7 +199,7 @@ class SNICallbackTest(unittest.TestCase): self.authenticator.sni_callback(connection) self.assertEqual(connection.set_context.call_count, 1) called_ctx = connection.set_context.call_args[0][0] - self.assertIsInstance(called_ctx, OpenSSL.SSL.Context) + self.assertTrue(isinstance(called_ctx, OpenSSL.SSL.Context)) class ClientSignalHandlerTest(unittest.TestCase): """Tests for client_signal_handler() method.""" @@ -230,8 +226,9 @@ class ClientSignalHandlerTest(unittest.TestCase): # specified (which can't occur in normal use because this # function is only set as a signal handler for the above three # signals). - with self.assertRaises(AssertionError): - self.authenticator.client_signal_handler(signal.SIGPIPE, None) + self.assertRaises(AssertionError, + self.authenticator.client_signal_handler, + signal.SIGPIPE, None) class SubprocSignalHandlerTest(unittest.TestCase): @@ -311,10 +308,10 @@ class PerformTest(unittest.TestCase): self.authenticator.tasks.has_key("foononce.acme.invalid")) self.assertTrue( self.authenticator.tasks.has_key("barnonce.acme.invalid")) - self.assertIsInstance(result, list) + self.assertTrue(isinstance(result, list)) self.assertEqual(len(result), 3) - self.assertIsInstance(result[0], dict) - self.assertIsInstance(result[1], dict) + self.assertTrue(isinstance(result[0], dict)) + self.assertTrue(isinstance(result[1], dict)) self.assertFalse(result[2]) self.assertTrue(result[0].has_key("s")) self.assertTrue(result[1].has_key("s")) @@ -337,7 +334,7 @@ class PerformTest(unittest.TestCase): self.authenticator.tasks.has_key("foononce.acme.invalid")) self.assertTrue( self.authenticator.tasks.has_key("barnonce.acme.invalid")) - self.assertIsInstance(result, list) + self.assertTrue(isinstance(result, list)) self.assertEqual(len(result), 3) self.assertEqual(result, [None, None, False]) self.authenticator.start_listener.assert_called_once_with(443, key) @@ -345,20 +342,19 @@ class PerformTest(unittest.TestCase): def test_perform_with_pending_tasks(self): self.authenticator.tasks = {"foononce.acme.invalid": "cert_data"} extra_challenge = DvsniChall("a", "b", "c", "d") - with self.assertRaises(Exception): - self.authenticator.perform([extra_challenge]) + self.assertRaises(Exception, self.authenticator.perform, + [extra_challenge]) def test_perform_without_challenge_list(self): extra_challenge = DvsniChall("a", "b", "c", "d") # This is wrong because a challenge must be specified. - with self.assertRaises(Exception): - self.authenticator.perform([]) + self.assertRaises(Exception, self.authenticator.perform, []) # This is wrong because it must be a list, not a bare challenge. - with self.assertRaises(Exception): - self.authenticator.perform(extra_challenge) + self.assertRaises(Exception, self.authenticator.perform, + extra_challenge) # This is wrong because the list must contain at least one challenge. - with self.assertRaises(Exception): - self.authenticator.perform(range(20)) + self.assertRaises(Exception, self.authenticator.perform, + range(20)) class StartListenerTest(unittest.TestCase): @@ -481,8 +477,8 @@ class DoChildProcessTest(unittest.TestCase): # (Just replacing it with a no-op causes logic errors because the # do_child_process code assumes that calling sys.exit() will # cause subsequent code not to be executed.) - with self.assertRaises(IndentationError): - self.authenticator.do_child_process(1717, self.key) + self.assertRaises(IndentationError, + self.authenticator.do_child_process, 1717, self.key) mock_exit.assert_called_once_with(1) mock_kill.assert_called_once_with(12345, signal.SIGUSR2) @@ -497,8 +493,8 @@ class DoChildProcessTest(unittest.TestCase): sample_socket = mock.MagicMock() sample_socket.bind.side_effect = eaccess mock_socket.return_value = sample_socket - with self.assertRaises(IndentationError): - self.authenticator.do_child_process(1717, self.key) + self.assertRaises(IndentationError, + self.authenticator.do_child_process, 1717, self.key) mock_exit.assert_called_once_with(1) mock_kill.assert_called_once_with(12345, signal.SIGUSR1) @@ -513,8 +509,8 @@ class DoChildProcessTest(unittest.TestCase): sample_socket = mock.MagicMock() sample_socket.bind.side_effect = eio mock_socket.return_value = sample_socket - with self.assertRaises(socket.error): - self.authenticator.do_child_process(1717, self.key) + self.assertRaises(socket.error, + self.authenticator.do_child_process, 1717, self.key) @mock.patch("letsencrypt.client.standalone_authenticator." "OpenSSL.SSL.Connection") @@ -527,8 +523,8 @@ class DoChildProcessTest(unittest.TestCase): sample_socket.accept.side_effect = SocketAcceptOnlyNTimes(2) mock_socket.return_value = sample_socket mock_connection.return_value = mock.MagicMock() - with self.assertRaises(CallableExhausted): - self.authenticator.do_child_process(1717, self.key) + self.assertRaises(CallableExhausted, + self.authenticator.do_child_process, 1717, self.key) mock_socket.assert_called_once_with() sample_socket.bind.assert_called_once_with(("0.0.0.0", 1717)) sample_socket.listen.assert_called_once_with(1) @@ -561,8 +557,7 @@ class CleanupTest(unittest.TestCase): def test_bad_cleanup(self): chall = DvsniChall("bad.example.com", "whee", "badnonce", "key") - with self.assertRaises(ValueError): - self.authenticator.cleanup([chall]) + self.assertRaises(ValueError, self.authenticator.cleanup, [chall]) if __name__ == '__main__': From b324f9d9125cc1b8c164110ba671abcb47b86d1e Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Sun, 8 Feb 2015 21:15:49 -0800 Subject: [PATCH 29/45] Python 2.6 .format() requires parameter in format string --- letsencrypt/client/standalone_authenticator.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/letsencrypt/client/standalone_authenticator.py b/letsencrypt/client/standalone_authenticator.py index e7f4e276b..0a64b7fe5 100644 --- a/letsencrypt/client/standalone_authenticator.py +++ b/letsencrypt/client/standalone_authenticator.py @@ -327,13 +327,13 @@ class StandaloneAuthenticator(object): return True if self.subproc_state == "inuse": display.generic_notification( - "Could not bind TCP port {} because it is already in " + "Could not bind TCP port {0} because it is already in " "use it is already in use by another process on this " "system (such as a web server).".format(port)) return False if self.subproc_state == "cantbind": display.generic_notification( - "Could not bind TCP port {} because you don't have " + "Could not bind TCP port {0} because you don't have " "the appropriate permissions (for example, you " "aren't running this program as " "root).".format(port)) @@ -341,7 +341,7 @@ class StandaloneAuthenticator(object): time.sleep(0.1) display.generic_notification( "Subprocess unexpectedly timed out while trying to bind TCP " - "port {}.".format(port)) + "port {0}.".format(port)) return False def do_child_process(self, port, key): From 1c6865c329da178285997e4b56cd9f9ed9ebfa34 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 9 Feb 2015 09:35:37 -0800 Subject: [PATCH 30/45] Move abbreviated DVSNI code into a separate branch --- .../client/standalone_authenticator.py | 231 ------------------ .../tests/standalone_authenticator_test.py | 106 -------- 2 files changed, 337 deletions(-) diff --git a/letsencrypt/client/standalone_authenticator.py b/letsencrypt/client/standalone_authenticator.py index 0a64b7fe5..83ffd22c4 100644 --- a/letsencrypt/client/standalone_authenticator.py +++ b/letsencrypt/client/standalone_authenticator.py @@ -17,217 +17,11 @@ import sys import signal import time import socket -import struct import Crypto.Random -import M2Crypto.X509 import OpenSSL.crypto import OpenSSL.SSL -def unpack_2bytes(two_bytes): - """Interpret a two-byte string as an integer. E.g. 't_' -> 29791.""" - assert len(two_bytes) == 2 - return struct.unpack(">H", two_bytes)[0] - - -def unpack_3bytes(three_bytes): - """Interpret a three-byte string as an integer. E.g. '0M~' -> 3165566.""" - assert len(three_bytes) == 3 - return struct.unpack(">I", chr(0) + three_bytes)[0] - - -def pack_2bytes(value): - """Interpret an integer less than 65536 as a two-byte string. E.g. - 29791 -> 't_'.""" - assert value < 65536 - return struct.pack(">H", value) - - -def pack_3bytes(value): - """Interpret an integer less than 16777216 as a three-byte string. - E.g. '0M~' -> 3165566.""" - assert value < 16777216 - return struct.pack(">I", value)[1:] - - -# Exclude this function from coverage testing because it is currently -# not used. -def tls_parse_client_hello(tls_record): # pragma: no cover - # pylint: disable=too-many-return-statements,too-many-locals,bad-builtin - # pylint: disable=too-many-branches - """If possible, parse the specified TLS record as a ClientHello and - return the first host_name indicated in a Server Name Indication - extension within that ClientHello. If the TLS record could not - be parsed or there is no such extension or host_name present, - return None. - - :param str tls_record: The TLS record to be parsed (which is assumed - to contain a single ClientHello handshake message).""" - - # TLS handshake? - if tls_record[0] != chr(0x16): - return None - - # TLS version - tls_version = tls_record[1:3] - if map(ord, tls_version) not in [[0x03, 0x01], [0x03, 0x02], [0x03, 0x03]]: - return None - - # TLS record length - tls_record_len = unpack_2bytes(tls_record[3:5]) - if len(tls_record) < tls_record_len: - return None - - # Handshake type, length, and version - handshake_type = tls_record[5] - if handshake_type != chr(0x01): - return None - handshake_len = unpack_3bytes(tls_record[6:9]) - handshake_version = tls_record[9:11] - handshake = tls_record[11:] - - # Handshake length includes handshake_version (2 bytes) - if len(handshake) + 2 < handshake_len: - return None - if map(ord, handshake_version) not in [[0x03, 0x01], [0x03, 0x02], - [0x03, 0x03]]: - return None - - # Random - unused_random = handshake[0:32] - - # Session ID - session_id_length = ord(handshake[32]) - i = 33 - i += session_id_length - - # Ciphersuites - ciphersuites_length = unpack_2bytes(handshake[i:i+2]) - if ciphersuites_length >= 2: - best_ciphersuite = handshake[i+2:i+4] - else: - best_ciphersuite = chr(0) + chr(0) - i += 2 - i += ciphersuites_length - - # Compression methods - compression_length = ord(handshake[i]) - i += 1 - i += compression_length - - # ClientHello extensions - extensions_length = unpack_2bytes(handshake[i:i+2]) - i += 2 - if extensions_length < 10: - # Minimum size of a 1-byte SNI hostname extension - return None - - while i < len(handshake): - # XXX If stated extension lengths are wrong or inconsistent or - # XXX if the packet has been truncated in the middle of an - # XXX extension, this may crash or hang! This needs to be updated - # XXX to fail cleanly when confronted with inconsistent extension - # XXX fields. - extension_type = handshake[i:i+2] - if extension_type == "\0\0": - # SNI - extension_length = unpack_2bytes(handshake[i+2:i+4]) - i += 4 - unused_server_name_list_length = unpack_2bytes(handshake[i:i+2]) - first_sn_type = handshake[i+2] - if first_sn_type != "\0": - # SNI extension referenced something other than a - # hostname - return None - first_sn_length = unpack_2bytes(handshake[i+3:i+5]) - first_sn = handshake[i+5:i+5+first_sn_length] - return best_ciphersuite, first_sn - else: - # Other than SNI - extension_length = unpack_2bytes(handshake[i+2:i+4]) - i += 4 - i += extension_length - continue - return None - - -def tls_generate_server_hello(ciphersuite): - """Generate a TLS 1.2 ServerHello message. - - :param ciphersuite str: The ciphersuite that the ServerHello will - claim to have selected (two bytes).""" - - # Handshake type: ServerHello (0x02) - server_hello = chr(0x02) - # ServerHello length (38 bytes based on below) - server_hello += chr(0x0) + chr(0x0) + chr(38) - # TLS version (0x0303) - server_hello += chr(0x03) + chr(0x03) - # Server Random - server_hello += Crypto.Random.new().read(32) - # Session ID length (0) - server_hello += chr(0x0) - # Ciphersuite - server_hello += ciphersuite - # Compression method (null) - server_hello += chr(0x0) - # Extension length (2 bytes) + extensions go here if any extensions - # are required, BUT if no extensions are present then the extensions - # and extension length field are both omitted entirely (rather than - # declaring extension length 0x0000) - see RFC 5246 p. 42. - - # TLS handshake - tls_record = chr(0x16) - # TLS version - tls_record += chr(0x03) + chr(0x03) - # TLS record length - assert len(server_hello) < 256 - tls_record += chr(0) + chr(len(server_hello)) - # Append server hello handshake - tls_record += server_hello - return tls_record - - -def tls_generate_cert_msg(cert_pem): - """Generate a TLS 1.2 Certificate handshake message containing a - single certificate. - - :param str cert_pem: The certificate to be include in the message (in - PEM format).""" - - cert_as_der = M2Crypto.X509.load_cert_string(cert_pem).as_der() - # Handshake type: Certificate (0x0b) - cert_msg = chr(0x0b) - - cert_msg_length = len(cert_as_der) + 6 - cert_msg += pack_3bytes(cert_msg_length) - - certs_length = len(cert_as_der) + 3 - cert_msg += pack_3bytes(certs_length) - - cert_length = len(cert_as_der) - cert_msg += pack_3bytes(cert_length) - - cert_msg += cert_as_der - - # TLS handshake - tls_record = chr(0x16) - # TLS version - tls_record += chr(0x03) + chr(0x03) - # TLS record length - assert len(cert_msg) < 65536 - tls_record += pack_2bytes(len(cert_msg)) - # Append certificate handshake - tls_record += cert_msg - return tls_record - - -def tls_generate_server_hello_done(): - """Generate a TLS 1.2 ServerHelloDone message.""" - - return "16030300040e000000".decode("hex") - - class StandaloneAuthenticator(object): # pylint: disable=too-many-instance-attributes """The StandaloneAuthenticator class itself, which can be invoked @@ -391,31 +185,6 @@ class StandaloneAuthenticator(object): self.ssl_conn.shutdown() self.ssl_conn.close() - # The code below uses the minimal pure Python implementation - # of TLS ClientHello, ServerHello, and Certificate messages - # (as an alternative to a full TLS implementation). It will - # not reach Finished state with a compliant TLS implementation. - # - # client_hello = self.connection.recv(65536) - # result = tls_parse_client_hello(client_hello) - # if result is None: - # print "No SNI found in ClientHello, dropping connection" - # self.connection.close() - # continue - # ciphersuite, sni = result - # if sni in self.tasks: - # pem_cert = self.tasks[sni] - # else: - # # We don't know which cert to send! - # print "Unexpected SNI value", sni - # # Choose the "first" cert and send it (but maybe we - # # should just disconnect instead?) - # pem_cert = self.tasks.values()[0] - # self.connection.send(tls_generate_server_hello(ciphersuite)) - # self.connection.send(tls_generate_cert_msg(pem_cert)) - # self.connection.send(tls_generate_server_hello_done()) - # self.connection.close() - def start_listener(self, port, key): """Create a child process which will start a TCP listener on the specified port to perform the specified DVSNI challenges. diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index 21a738363..d855cced9 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -38,112 +38,6 @@ class CallableExhausted(Exception): pass -class PackAndUnpackTests(unittest.TestCase): - """Tests for byte packing and unpacking routines used for TLS - parsing.""" - def test_pack_and_unpack_bytes(self): - from letsencrypt.client.standalone_authenticator import \ - unpack_2bytes, unpack_3bytes, pack_2bytes, pack_3bytes - self.assertEqual(unpack_2bytes("JZ"), 19034) - self.assertEqual(unpack_2bytes(chr(0)*2), 0) - self.assertEqual(unpack_2bytes(chr(255)*2), 65535) - - self.assertEqual(unpack_3bytes("abc"), 6382179) - self.assertEqual(unpack_3bytes(chr(0)*3), 0) - self.assertEqual(unpack_3bytes(chr(255)*3), 16777215) - - self.assertEqual(pack_2bytes(12), chr(0) + chr(12)) - self.assertEqual(pack_2bytes(1729), chr(6) + chr(193)) - - self.assertEqual(pack_3bytes(0), chr(0)*3) - self.assertEqual(pack_3bytes(12345678), chr(0xbc) + "aN") - - def test_invalid_pack_and_unpack(self): - from letsencrypt.client.standalone_authenticator import \ - unpack_2bytes, unpack_3bytes, pack_2bytes, pack_3bytes - self.assertRaises(AssertionError, pack_2bytes, 65537) - self.assertRaises(AssertionError, pack_3bytes, 500000000) - self.assertRaises(AssertionError, unpack_2bytes, "foo") - self.assertRaises(AssertionError, unpack_3bytes, "food") - - -class TLSParseClientHelloTest(unittest.TestCase): - # pylint: disable=too-few-public-methods - """Test for tls_parse_client_hello() function.""" - def test_tls_parse_client_hello(self): - from letsencrypt.client.standalone_authenticator import \ - tls_parse_client_hello - client_hello = "16030100c4010000c003030cfef9971eda442c60cbb6c397" \ - "7957a81a8ada317e800b7867a8c61f71c40cab000020c02b" \ - "c02fc00ac009c013c014c007c011003300320039002f0035" \ - "000a000500040100007700000010000e00000b7777772e65" \ - "66662e6f7267ff01000100000a0008000600170018001900" \ - "0b00020100002300003374000000100021001f0568322d31" \ - "3408737064792f332e3106737064792f3308687474702f31" \ - "2e31000500050100000000000d0012001004010501020104" \ - "030503020304020202".decode("hex") - return_value = tls_parse_client_hello(client_hello) - self.assertEqual(return_value, (chr(0xc0) + chr(0x2b), "www.eff.org")) - # TODO: The failure cases are extremely numerous and require - # constructing TLS ClientHello messages that are individually - # defective or surprising in distinct ways. (Each invalid TLS - # record is invalid in its own way.) - - -class TLSGenerateServerHelloTest(unittest.TestCase): - # pylint: disable=too-few-public-methods - """Tests for tls_generate_server_hello() function.""" - def test_tls_generate_server_hello(self): - from letsencrypt.client.standalone_authenticator import \ - tls_generate_server_hello - server_hello = tls_generate_server_hello("Q!") - self.assertEqual(server_hello[:11].encode("hex"), - '160303002a020000260303') - self.assertEqual(server_hello[43:], chr(0) + 'Q!' + chr(0)) - - -class TLSGenerateCertMsgTest(unittest.TestCase): - # pylint: disable=too-few-public-methods - """Tests for tls_generate_cert_msg() function.""" - def test_tls_generate_cert_msg(self): - from letsencrypt.client.standalone_authenticator import \ - tls_generate_cert_msg - cert = pkg_resources.resource_string(__name__, - 'testdata/cert.pem') - cert_msg = tls_generate_cert_msg(cert) - self.assertEqual(cert_msg.encode("hex"), - "16030301ec0b0001e80001e50001e2308201de30820188a003" - "02010202020539300d06092a864886f70d01010b0500307731" - "0b30090603550406130255533111300f06035504080c084d69" - "63686967616e3112301006035504070c09416e6e204172626f" - "72312b3029060355040a0c22556e6976657273697479206f66" - "204d6963686967616e20616e64207468652045464631143012" - "06035504030c0b6578616d706c652e636f6d301e170d313431" - "3231313232333434355a170d3134313231383232333434355a" - "3077310b30090603550406130255533111300f06035504080c" - "084d6963686967616e3112301006035504070c09416e6e2041" - "72626f72312b3029060355040a0c22556e6976657273697479" - "206f66204d6963686967616e20616e64207468652045464631" - "14301206035504030c0b6578616d706c652e636f6d305c300d" - "06092a864886f70d0101010500034b003048024100ac7573b4" - "51ed1fddae705243fcdfc75bd02c751b14b875010410e51f03" - "6545dddfa79f34aefdbee90584df471681d9894bce8e6d1cfa" - "9544e8af84744fedc2e50203010001300d06092a864886f70d" - "01010b05000341002db8cf421dc0854a4a59ed92c965bebeb3" - "25ea411f97cc9dd7e4dd7269d748d3e9513ed7828db63874d9" - "ae7a1a8ada02f2404f9fc7ebb13c1af27fa1c36707fa") - - -class TLSServerHelloDoneTest(unittest.TestCase): - # pylint: disable=too-few-public-methods - """Tests for tls_generate_server_hello_done() function.""" - def test_tls_generate_server_hello_done(self): - from letsencrypt.client.standalone_authenticator import \ - tls_generate_server_hello_done - self.assertEqual(tls_generate_server_hello_done().encode("hex"), \ - "16030300040e000000") - - class ChallPrefTest(unittest.TestCase): """Tests for chall_pref() method.""" def setUp(self): From 56a60d0acf41eb1b80b5d7f63b0d6f32ced90b5d Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 9 Feb 2015 09:44:49 -0800 Subject: [PATCH 31/45] Move imports to top of unit test file --- .../tests/standalone_authenticator_test.py | 34 ++++++------------- 1 file changed, 11 insertions(+), 23 deletions(-) diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index d855cced9..08312bd90 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -2,10 +2,19 @@ """Tests for standalone_authenticator.py.""" -import unittest import mock -import pkg_resources +import unittest + from letsencrypt.client.challenge_util import DvsniChall +from letsencrypt.client.challenge_util import dvsni_gen_cert +from letsencrypt.client import le_util +from OpenSSL.crypto import FILETYPE_PEM +import OpenSSL.crypto +import OpenSSL.SSL +import os +import pkg_resources +import signal +import socket # Classes based on to allow interrupting infinite loop under test @@ -55,10 +64,6 @@ class SNICallbackTest(unittest.TestCase): def setUp(self): from letsencrypt.client.standalone_authenticator import \ StandaloneAuthenticator - from letsencrypt.client.challenge_util import dvsni_gen_cert - from letsencrypt.client import le_util - import OpenSSL.crypto - from OpenSSL.crypto import FILETYPE_PEM self.authenticator = StandaloneAuthenticator() name, r_b64 = "example.com", le_util.jose_b64encode("x" * 32) test_key = pkg_resources.resource_string(__name__, @@ -71,7 +76,6 @@ class SNICallbackTest(unittest.TestCase): self.authenticator.child_pid = 12345 def test_real_servername(self): - import OpenSSL.SSL connection = mock.MagicMock() connection.get_servername.return_value = "abcdef.acme.invalid" self.authenticator.sni_callback(connection) @@ -87,7 +91,6 @@ class SNICallbackTest(unittest.TestCase): structure so this might not be the one that was first in the challenge list passed to the perform method. In the future, this might result in dropping the connection instead.)""" - import OpenSSL.SSL connection = mock.MagicMock() connection.get_servername.return_value = "example.com" self.authenticator.sni_callback(connection) @@ -105,7 +108,6 @@ class ClientSignalHandlerTest(unittest.TestCase): self.authenticator.child_pid = 12345 def test_client_signal_handler(self): - import signal self.assertEqual(self.authenticator.subproc_state, None) self.authenticator.client_signal_handler(signal.SIGIO, None) self.assertEqual(self.authenticator.subproc_state, "ready") @@ -138,7 +140,6 @@ class SubprocSignalHandlerTest(unittest.TestCase): @mock.patch("letsencrypt.client.standalone_authenticator.os.kill") @mock.patch("letsencrypt.client.standalone_authenticator.sys.exit") def test_subproc_signal_handler(self, mock_exit, mock_kill): - import signal self.authenticator.ssl_conn = mock.MagicMock() self.authenticator.connection = mock.MagicMock() self.authenticator.sock = mock.MagicMock() @@ -157,7 +158,6 @@ class SubprocSignalHandlerTest(unittest.TestCase): """Test how the signal handler survives attempting to shut down a non-existent connection (because none was established or active at the time the signal handler tried to perform the cleanup).""" - import signal self.authenticator.ssl_conn = mock.MagicMock() self.authenticator.connection = mock.MagicMock() self.authenticator.sock = mock.MagicMock() @@ -187,7 +187,6 @@ class PerformTest(unittest.TestCase): def test_can_perform(self): """What happens if start_listener() returns True.""" - from letsencrypt.client import le_util test_key = pkg_resources.resource_string(__name__, 'testdata/rsa256_key.pem') key = le_util.Key("something", test_key) @@ -213,7 +212,6 @@ class PerformTest(unittest.TestCase): def test_cannot_perform(self): """What happens if start_listener() returns False.""" - from letsencrypt.client import le_util test_key = pkg_resources.resource_string(__name__, 'testdata/rsa256_key.pem') key = le_util.Key("something", test_key) @@ -273,7 +271,6 @@ class StartListenerTest(unittest.TestCase): "Crypto.Random.atfork") @mock.patch("letsencrypt.client.standalone_authenticator.os.fork") def test_start_listener_fork_child(self, mock_fork, mock_atfork): - import os self.authenticator.do_parent_process = mock.Mock() self.authenticator.do_child_process = mock.Mock() mock_fork.return_value = 0 @@ -338,10 +335,6 @@ class DoChildProcessTest(unittest.TestCase): def setUp(self): from letsencrypt.client.standalone_authenticator import \ StandaloneAuthenticator - from letsencrypt.client.challenge_util import dvsni_gen_cert - from letsencrypt.client import le_util - import OpenSSL.crypto - from OpenSSL.crypto import FILETYPE_PEM self.authenticator = StandaloneAuthenticator() name, r_b64 = "example.com", le_util.jose_b64encode("x" * 32) test_key = pkg_resources.resource_string(__name__, @@ -359,7 +352,6 @@ class DoChildProcessTest(unittest.TestCase): @mock.patch("letsencrypt.client.standalone_authenticator.sys.exit") def test_do_child_process_cantbind1(self, mock_exit, mock_kill, mock_socket): - import socket, signal mock_exit.side_effect = IndentationError("subprocess would exit here") eaccess = socket.error(socket.errno.EACCES, "Permission denied") sample_socket = mock.MagicMock() @@ -381,7 +373,6 @@ class DoChildProcessTest(unittest.TestCase): @mock.patch("letsencrypt.client.standalone_authenticator.sys.exit") def test_do_child_process_cantbind2(self, mock_exit, mock_kill, mock_socket): - import socket, signal mock_exit.side_effect = IndentationError("subprocess would exit here") eaccess = socket.error(socket.errno.EADDRINUSE, "Port already in use") sample_socket = mock.MagicMock() @@ -398,7 +389,6 @@ class DoChildProcessTest(unittest.TestCase): socket error. (The expected behavior is arguably wrong because it will crash the program; the reason for the expected behavior is that we don't have a way to report arbitrary socket errors.)""" - import socket eio = socket.error(socket.errno.EIO, "Imaginary unhandled error") sample_socket = mock.MagicMock() sample_socket.bind.side_effect = eio @@ -412,7 +402,6 @@ class DoChildProcessTest(unittest.TestCase): @mock.patch("letsencrypt.client.standalone_authenticator.os.kill") def test_do_child_process_success(self, mock_kill, mock_socket, mock_connection): - import signal sample_socket = mock.MagicMock() sample_socket.accept.side_effect = SocketAcceptOnlyNTimes(2) mock_socket.return_value = sample_socket @@ -441,7 +430,6 @@ class CleanupTest(unittest.TestCase): @mock.patch("letsencrypt.client.standalone_authenticator.os.kill") @mock.patch("letsencrypt.client.standalone_authenticator.time.sleep") def test_cleanup(self, mock_sleep, mock_kill): - import signal mock_sleep.return_value = None mock_kill.return_value = None chall = DvsniChall("foo.example.com", "whee", "foononce", "key") From 6d0a14a0e539512871c7a98f570d6193dd930d40 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 9 Feb 2015 09:48:05 -0800 Subject: [PATCH 32/45] Rename CONFIG.PORT to STANDALONE_CHALLENGE_PORT --- letsencrypt/client/CONFIG.py | 2 +- letsencrypt/client/standalone_authenticator.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/letsencrypt/client/CONFIG.py b/letsencrypt/client/CONFIG.py index 2f9428995..ea1a5fa18 100644 --- a/letsencrypt/client/CONFIG.py +++ b/letsencrypt/client/CONFIG.py @@ -75,7 +75,7 @@ S_SIZE = 32 NONCE_SIZE = 16 """byte size of Nonce""" -PORT = 443 +STANDALONE_CHALLENGE_PORT = 443 """TCP port on which to perform (standalone) challenge""" # Key Sizes diff --git a/letsencrypt/client/standalone_authenticator.py b/letsencrypt/client/standalone_authenticator.py index 83ffd22c4..5043ccd26 100644 --- a/letsencrypt/client/standalone_authenticator.py +++ b/letsencrypt/client/standalone_authenticator.py @@ -252,7 +252,7 @@ class StandaloneAuthenticator(object): raise Exception("nothing for .perform() to do") # Try to do the authentication; note that this creates # the listener subprocess via os.fork() - if self.start_listener(CONFIG.PORT, key): + if self.start_listener(CONFIG.STANDALONE_CHALLENGE_PORT, key): return results_if_success else: # TODO: This should probably raise a DVAuthError exception From 470cad14ad9e5a8aab9dbd56efa8b0e3d7fdad09 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 9 Feb 2015 09:54:33 -0800 Subject: [PATCH 33/45] Add missing return statements in start_listener() --- letsencrypt/client/standalone_authenticator.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/letsencrypt/client/standalone_authenticator.py b/letsencrypt/client/standalone_authenticator.py index 5043ccd26..afffa7cb3 100644 --- a/letsencrypt/client/standalone_authenticator.py +++ b/letsencrypt/client/standalone_authenticator.py @@ -197,11 +197,15 @@ class StandaloneAuthenticator(object): if fork_result: # PARENT process (still the Let's Encrypt client process) self.child_pid = fork_result - self.do_parent_process(port) + # do_parent_process() can return True or False to indicate + # reported success or failure creating the listener. + return self.do_parent_process(port) else: # CHILD process (the TCP listener subprocess) self.child_pid = os.getpid() - self.do_child_process(port, key) + # do_child_process() is normally not expected to return but + # should terminate via sys.exit(). + return self.do_child_process(port, key) # IAuthenticator method implementations follow From 275d3e3da5ebcc0b12eea45d93edbb93a930da21 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 9 Feb 2015 10:00:01 -0800 Subject: [PATCH 34/45] Document and test start_listener() return value --- letsencrypt/client/standalone_authenticator.py | 2 ++ letsencrypt/client/tests/standalone_authenticator_test.py | 6 +++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/letsencrypt/client/standalone_authenticator.py b/letsencrypt/client/standalone_authenticator.py index afffa7cb3..fd28d3c2a 100644 --- a/letsencrypt/client/standalone_authenticator.py +++ b/letsencrypt/client/standalone_authenticator.py @@ -191,6 +191,8 @@ class StandaloneAuthenticator(object): :param int port: The TCP port to bind. :param str key: The private key to use (in PEM format). + :returns: True or False to indicate success or failure creating + the subprocess. """ fork_result = os.fork() Crypto.Random.atfork() diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index 08312bd90..911de737a 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -261,8 +261,12 @@ class StartListenerTest(unittest.TestCase): @mock.patch("letsencrypt.client.standalone_authenticator.os.fork") def test_start_listener_fork_parent(self, mock_fork, mock_atfork): self.authenticator.do_parent_process = mock.Mock() + self.authenticator.do_parent_process.return_value = True mock_fork.return_value = 22222 - self.authenticator.start_listener(1717, "key") + result = self.authenticator.start_listener(1717, "key") + # start_listener is expected to return the True or False return + # value from do_parent_process. + self.assertTrue(result) self.assertEqual(self.authenticator.child_pid, 22222) self.authenticator.do_parent_process.assert_called_once_with(1717) mock_atfork.assert_called_once_with() From 52ae977fb7db169c40886835fa4a80fa30735660 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 9 Feb 2015 10:03:23 -0800 Subject: [PATCH 35/45] Sort imports lexicographically --- letsencrypt/client/standalone_authenticator.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/letsencrypt/client/standalone_authenticator.py b/letsencrypt/client/standalone_authenticator.py index fd28d3c2a..70974180c 100644 --- a/letsencrypt/client/standalone_authenticator.py +++ b/letsencrypt/client/standalone_authenticator.py @@ -6,20 +6,20 @@ but instead creates its own ephemeral TCP listener on the specified port in order to respond to incoming DVSNI challenges from the certificate authority.""" -import zope.interface -import zope.component -from letsencrypt.client import CONFIG -from letsencrypt.client import interfaces from letsencrypt.client.challenge_util import DvsniChall from letsencrypt.client.challenge_util import dvsni_gen_cert -import os -import sys -import signal -import time -import socket +from letsencrypt.client import CONFIG +from letsencrypt.client import interfaces import Crypto.Random import OpenSSL.crypto import OpenSSL.SSL +import os +import signal +import socket +import sys +import time +import zope.component +import zope.interface class StandaloneAuthenticator(object): From 3f250084b0847339a794cf111b7b96f9a49359f6 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 9 Feb 2015 10:06:24 -0800 Subject: [PATCH 36/45] Sorting import statements --- letsencrypt/client/le_util.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/client/le_util.py b/letsencrypt/client/le_util.py index 31d3fcb5e..3257d5a18 100644 --- a/letsencrypt/client/le_util.py +++ b/letsencrypt/client/le_util.py @@ -1,9 +1,9 @@ """Utilities for all Let's Encrypt.""" import base64 +import collections import errno import os import stat -import collections from letsencrypt.client import errors From a543925e6492b41e8f28d701f7a29910fa8f5547 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 9 Feb 2015 11:57:50 -0800 Subject: [PATCH 37/45] Some formatting fixes --- letsencrypt/client/le_util.py | 1 + .../client/standalone_authenticator.py | 58 ++++++++++++------- .../tests/standalone_authenticator_test.py | 4 +- setup.py | 2 +- 4 files changed, 40 insertions(+), 25 deletions(-) diff --git a/letsencrypt/client/le_util.py b/letsencrypt/client/le_util.py index 3257d5a18..c4a0170e0 100644 --- a/letsencrypt/client/le_util.py +++ b/letsencrypt/client/le_util.py @@ -7,6 +7,7 @@ import stat from letsencrypt.client import errors + Key = collections.namedtuple("Key", "file pem") diff --git a/letsencrypt/client/standalone_authenticator.py b/letsencrypt/client/standalone_authenticator.py index 70974180c..9eb6bc392 100644 --- a/letsencrypt/client/standalone_authenticator.py +++ b/letsencrypt/client/standalone_authenticator.py @@ -1,11 +1,11 @@ #!/usr/bin/env python # -*- coding: utf-8 -*- -"""An authenticator that doesn't rely on any existing server program, -but instead creates its own ephemeral TCP listener on the specified port -in order to respond to incoming DVSNI challenges from the certificate -authority.""" +"""An authenticator that doesn't rely on any existing server program. +This authenticator creates its own ephemeral TCP listener on the specified +port in order to respond to incoming DVSNI challenges from the certificate +authority.""" from letsencrypt.client.challenge_util import DvsniChall from letsencrypt.client.challenge_util import dvsni_gen_cert from letsencrypt.client import CONFIG @@ -24,9 +24,11 @@ import zope.interface class StandaloneAuthenticator(object): # pylint: disable=too-many-instance-attributes - """The StandaloneAuthenticator class itself, which can be invoked - by the Let's Encrypt client according to the IAuthenticator API - interface.""" + """The StandaloneAuthenticator class itself. + + This authenticator can be invoked by the Let's Encrypt client + according to the IAuthenticator API interface. It creates a local + TCP listener on a specified port and satisfies DVSNI challenges.""" zope.interface.implements(interfaces.IAuthenticator) def __init__(self): @@ -40,9 +42,10 @@ class StandaloneAuthenticator(object): self.ssl_conn = None def client_signal_handler(self, sig, unused_frame): - """Signal handler for the parent process (to receive inter-process - communication from the child process in the form of Unix - signals.""" + """Signal handler for the parent process. + + This handler receives inter-process communication from the + child process in the form of Unix signals.""" # signal handler for use in parent process # subprocess → client READY : SIGIO # subprocess → client INUSE : SIGUSR1 @@ -58,9 +61,10 @@ class StandaloneAuthenticator(object): assert False def subproc_signal_handler(self, sig, unused_frame): - """Signal handler for the child process (to receive inter-process - communication from the parent process in the form of Unix - signals.""" + """Signal handler for the child process. + + This handler receives inter-process communication from the parent + process in the form of Unix signals.""" # signal handler for use in subprocess # client → subprocess CLEANUP : SIGINT if sig == signal.SIGINT: @@ -87,9 +91,11 @@ class StandaloneAuthenticator(object): sys.exit(0) def sni_callback(self, connection): - """Used internally to set a new OpenSSL context object for this - connection when an incoming connection provides an SNI name (in - order to serve the appropriate certificate, if any).""" + """Used internally to respond to incoming SNI names. + + This method will set a new OpenSSL context object for this + connection when an incoming connection provides an SNI name + (in order to serve the appropriate certificate, if any).""" sni_name = connection.get_servername() if sni_name in self.tasks: @@ -107,9 +113,14 @@ class StandaloneAuthenticator(object): connection.set_context(new_ctx) def do_parent_process(self, port, delay_amount=5): - """Perform the parent process side of the TCP listener task. This - should only be called by start_listener(). We will wait up to - delay_amount seconds to hear from the child process via a signal.""" + """Perform the parent process side of the TCP listener task. + + This should only be called by start_listener(). We will wait + up to delay_amount seconds to hear from the child process via + a signal. + + :returns: True or False according to whether we were notified + that the child process succeeded or failed in binding the port.""" signal.signal(signal.SIGIO, self.client_signal_handler) signal.signal(signal.SIGUSR1, self.client_signal_handler) @@ -139,8 +150,13 @@ class StandaloneAuthenticator(object): return False def do_child_process(self, port, key): - """Perform the child process side of the TCP listener task. This - should only be called by start_listener().""" + """Perform the child process side of the TCP listener task. + + This should only be called by start_listener(). + + Normally does not return; instead, the child process exits from + within this function or from within the child process signal + handler.""" signal.signal(signal.SIGINT, self.subproc_signal_handler) self.sock = socket.socket() try: diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index 911de737a..75a5d22f4 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -1,7 +1,6 @@ #!/usr/bin/env python """Tests for standalone_authenticator.py.""" - import mock import unittest @@ -44,7 +43,6 @@ class CallableExhausted(Exception): # pylint: disable=too-few-public-methods """Exception raised when a method is called more than the specified number of times.""" - pass class ChallPrefTest(unittest.TestCase): @@ -108,7 +106,7 @@ class ClientSignalHandlerTest(unittest.TestCase): self.authenticator.child_pid = 12345 def test_client_signal_handler(self): - self.assertEqual(self.authenticator.subproc_state, None) + self.assertTrue(self.authenticator.subproc_state is None) self.authenticator.client_signal_handler(signal.SIGIO, None) self.assertEqual(self.authenticator.subproc_state, "ready") diff --git a/setup.py b/setup.py index c4b44b2bd..1a3a6ddd3 100755 --- a/setup.py +++ b/setup.py @@ -26,10 +26,10 @@ install_requires = [ 'jsonschema', 'mock', 'pycrypto', + 'PyOpenSSL', 'python-augeas', 'python2-pythondialog', 'requests', - 'PyOpenSSL', 'zope.component', 'zope.interface', # order of items in install_requires DOES matter and M2Crypto has From 314dea3f5a17b7b1ef93a63ffcd45f7088cd04d2 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 9 Feb 2015 12:05:59 -0800 Subject: [PATCH 38/45] Change Key to le_util.Key --- letsencrypt/client/client.py | 3 +-- letsencrypt/client/tests/apache/configurator_test.py | 4 ++-- letsencrypt/client/tests/apache/dvsni_test.py | 4 ++-- letsencrypt/scripts/main.py | 4 ++-- 4 files changed, 7 insertions(+), 8 deletions(-) diff --git a/letsencrypt/client/client.py b/letsencrypt/client/client.py index 4abfeb0c9..c9fa1dbb5 100644 --- a/letsencrypt/client/client.py +++ b/letsencrypt/client/client.py @@ -22,7 +22,6 @@ from letsencrypt.client import reverter from letsencrypt.client import revoker from letsencrypt.client.apache import configurator -from letsencrypt.client.le_util import Key class Client(object): @@ -365,7 +364,7 @@ def init_key(key_size): logging.info("Generating key (%d bits): %s", key_size, key_filename) - return Key(key_filename, key_pem) + return le_util.Key(key_filename, key_pem) def init_csr(privkey, names): diff --git a/letsencrypt/client/tests/apache/configurator_test.py b/letsencrypt/client/tests/apache/configurator_test.py index f8c983c31..9ed56f89d 100644 --- a/letsencrypt/client/tests/apache/configurator_test.py +++ b/letsencrypt/client/tests/apache/configurator_test.py @@ -8,13 +8,13 @@ import mock from letsencrypt.client import challenge_util from letsencrypt.client import errors +from letsencrypt.client import le_util from letsencrypt.client.apache import configurator from letsencrypt.client.apache import obj from letsencrypt.client.apache import parser from letsencrypt.client.tests.apache import util -from letsencrypt.client.le_util import Key class TwoVhost80Test(util.ApacheTest): @@ -164,7 +164,7 @@ class TwoVhost80Test(util.ApacheTest): def test_perform(self, mock_restart, mock_dvsni_perform): # Only tests functionality specific to configurator.perform # Note: As more challenges are offered this will have to be expanded - auth_key = Key(self.rsa256_file, self.rsa256_pem) + auth_key = le_util.Key(self.rsa256_file, self.rsa256_pem) chall1 = challenge_util.DvsniChall( "encryption-example.demo", "jIq_Xy1mXGN37tb4L6Xj_es58fW571ZNyXekdZzhh7Q", diff --git a/letsencrypt/client/tests/apache/dvsni_test.py b/letsencrypt/client/tests/apache/dvsni_test.py index d09cc56e2..c5b49dc67 100644 --- a/letsencrypt/client/tests/apache/dvsni_test.py +++ b/letsencrypt/client/tests/apache/dvsni_test.py @@ -7,9 +7,9 @@ import mock from letsencrypt.client import challenge_util from letsencrypt.client import CONFIG +from letsencrypt.client import le_util from letsencrypt.client.tests.apache import util -from letsencrypt.client.le_util import Key class DvsniPerformTest(util.ApacheTest): """Test the ApacheDVSNI challenge.""" @@ -32,7 +32,7 @@ class DvsniPerformTest(util.ApacheTest): rsa256_pem = pkg_resources.resource_string( "letsencrypt.client.tests", 'testdata/rsa256_key.pem') - auth_key = Key(rsa256_file, rsa256_pem) + auth_key = le_util.Key(rsa256_file, rsa256_pem) self.challs = [] self.challs.append(challenge_util.DvsniChall( "encryption-example.demo", diff --git a/letsencrypt/scripts/main.py b/letsencrypt/scripts/main.py index 7507b0620..e659432e1 100755 --- a/letsencrypt/scripts/main.py +++ b/letsencrypt/scripts/main.py @@ -18,8 +18,8 @@ from letsencrypt.client import client from letsencrypt.client import display from letsencrypt.client import errors from letsencrypt.client import interfaces +from letsencrypt.client import le_util from letsencrypt.client import log -from letsencrypt.client.le_util import Key def main(): # pylint: disable=too-many-statements,too-many-branches @@ -119,7 +119,7 @@ def main(): # pylint: disable=too-many-statements,too-many-branches if args.privkey is None: privkey = client.init_key(args.key_size) else: - privkey = Key(args.privkey[0], args.privkey[1]) + privkey = le_util.Key(args.privkey[0], args.privkey[1]) acme = client.Client(args.server, privkey, auth, installer) From 93bc90203ae2e354a199728f4e6f9c91e4a2c047 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 9 Feb 2015 12:22:47 -0800 Subject: [PATCH 39/45] Use hanging indent style in several places --- .../tests/standalone_authenticator_test.py | 95 ++++++++++--------- 1 file changed, 50 insertions(+), 45 deletions(-) diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index 75a5d22f4..a8705d3cc 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -53,8 +53,8 @@ class ChallPrefTest(unittest.TestCase): self.authenticator = StandaloneAuthenticator() def test_chall_pref(self): - self.assertEqual(self.authenticator.get_chall_pref("example.com"), - ["dvsni"]) + self.assertEqual( + self.authenticator.get_chall_pref("example.com"), ["dvsni"]) class SNICallbackTest(unittest.TestCase): @@ -64,8 +64,8 @@ class SNICallbackTest(unittest.TestCase): StandaloneAuthenticator self.authenticator = StandaloneAuthenticator() name, r_b64 = "example.com", le_util.jose_b64encode("x" * 32) - test_key = pkg_resources.resource_string(__name__, - 'testdata/rsa256_key.pem') + test_key = pkg_resources.resource_string( + __name__, 'testdata/rsa256_key.pem') nonce, key = "abcdef", le_util.Key("foo", test_key) self.cert = dvsni_gen_cert(name, r_b64, nonce, key)[0] private_key = OpenSSL.crypto.load_privatekey(FILETYPE_PEM, key.pem) @@ -82,13 +82,14 @@ class SNICallbackTest(unittest.TestCase): self.assertTrue(isinstance(called_ctx, OpenSSL.SSL.Context)) def test_fake_servername(self): - """Test the behavior of the SNI callback when an unexpected SNI - name is received. (Currently the expected behavior in this case - is to return the "first" certificate with which the listener - was configured, although they are stored in an unordered data - structure so this might not be the one that was first in the - challenge list passed to the perform method. In the future, this - might result in dropping the connection instead.)""" + """Test behavior of SNI callback when an unexpected name is received. + + (Currently the expected behavior in this case is to return the + "first" certificate with which the listener was configured, + although they are stored in an unordered data structure so + this might not be the one that was first in the challenge list + passed to the perform method. In the future, this might result + in dropping the connection instead.)""" connection = mock.MagicMock() connection.get_servername.return_value = "example.com" self.authenticator.sni_callback(connection) @@ -120,9 +121,9 @@ class ClientSignalHandlerTest(unittest.TestCase): # specified (which can't occur in normal use because this # function is only set as a signal handler for the above three # signals). - self.assertRaises(AssertionError, - self.authenticator.client_signal_handler, - signal.SIGPIPE, None) + self.assertRaises( + AssertionError, self.authenticator.client_signal_handler, + signal.SIGPIPE, None) class SubprocSignalHandlerTest(unittest.TestCase): @@ -146,16 +147,17 @@ class SubprocSignalHandlerTest(unittest.TestCase): self.assertEquals(self.authenticator.ssl_conn.close.call_count, 1) self.assertEquals(self.authenticator.connection.close.call_count, 1) self.assertEquals(self.authenticator.sock.close.call_count, 1) - mock_kill.assert_called_once_with(self.authenticator.parent_pid, - signal.SIGUSR1) + mock_kill.assert_called_once_with( + self.authenticator.parent_pid, signal.SIGUSR1) mock_exit.assert_called_once_with(0) @mock.patch("letsencrypt.client.standalone_authenticator.os.kill") @mock.patch("letsencrypt.client.standalone_authenticator.sys.exit") def test_subproc_signal_handler_trouble(self, mock_exit, mock_kill): - """Test how the signal handler survives attempting to shut down - a non-existent connection (because none was established or active - at the time the signal handler tried to perform the cleanup).""" + """Test attempting to shut down a non-existent connection. + + (This could occur because none was established or active at the + time the signal handler tried to perform the cleanup).""" self.authenticator.ssl_conn = mock.MagicMock() self.authenticator.connection = mock.MagicMock() self.authenticator.sock = mock.MagicMock() @@ -171,8 +173,8 @@ class SubprocSignalHandlerTest(unittest.TestCase): self.assertEquals(self.authenticator.ssl_conn.close.call_count, 1) self.assertEquals(self.authenticator.connection.close.call_count, 1) self.assertEquals(self.authenticator.sock.close.call_count, 1) - mock_kill.assert_called_once_with(self.authenticator.parent_pid, - signal.SIGUSR1) + mock_kill.assert_called_once_with( + self.authenticator.parent_pid, signal.SIGUSR1) mock_exit.assert_called_once_with(0) @@ -185,8 +187,8 @@ class PerformTest(unittest.TestCase): def test_can_perform(self): """What happens if start_listener() returns True.""" - test_key = pkg_resources.resource_string(__name__, - 'testdata/rsa256_key.pem') + test_key = pkg_resources.resource_string( + __name__, 'testdata/rsa256_key.pem') key = le_util.Key("something", test_key) chall1 = DvsniChall("foo.example.com", "whee", "foononce", key) chall2 = DvsniChall("bar.example.com", "whee", "barnonce", key) @@ -210,8 +212,8 @@ class PerformTest(unittest.TestCase): def test_cannot_perform(self): """What happens if start_listener() returns False.""" - test_key = pkg_resources.resource_string(__name__, - 'testdata/rsa256_key.pem') + test_key = pkg_resources.resource_string( + __name__, 'testdata/rsa256_key.pem') key = le_util.Key("something", test_key) chall1 = DvsniChall("foo.example.com", "whee", "foononce", key) chall2 = DvsniChall("bar.example.com", "whee", "barnonce", key) @@ -232,19 +234,19 @@ class PerformTest(unittest.TestCase): def test_perform_with_pending_tasks(self): self.authenticator.tasks = {"foononce.acme.invalid": "cert_data"} extra_challenge = DvsniChall("a", "b", "c", "d") - self.assertRaises(Exception, self.authenticator.perform, - [extra_challenge]) + self.assertRaises( + Exception, self.authenticator.perform, [extra_challenge]) def test_perform_without_challenge_list(self): extra_challenge = DvsniChall("a", "b", "c", "d") # This is wrong because a challenge must be specified. self.assertRaises(Exception, self.authenticator.perform, []) # This is wrong because it must be a list, not a bare challenge. - self.assertRaises(Exception, self.authenticator.perform, - extra_challenge) + self.assertRaises( + Exception, self.authenticator.perform, extra_challenge) # This is wrong because the list must contain at least one challenge. - self.assertRaises(Exception, self.authenticator.perform, - range(20)) + self.assertRaises( + Exception, self.authenticator.perform, range(20)) class StartListenerTest(unittest.TestCase): @@ -278,8 +280,8 @@ class StartListenerTest(unittest.TestCase): mock_fork.return_value = 0 self.authenticator.start_listener(1717, "key") self.assertEqual(self.authenticator.child_pid, os.getpid()) - self.authenticator.do_child_process.assert_called_once_with(1717, - "key") + self.authenticator.do_child_process.assert_called_once_with( + 1717, "key") mock_atfork.assert_called_once_with() class DoParentProcessTest(unittest.TestCase): @@ -339,8 +341,8 @@ class DoChildProcessTest(unittest.TestCase): StandaloneAuthenticator self.authenticator = StandaloneAuthenticator() name, r_b64 = "example.com", le_util.jose_b64encode("x" * 32) - test_key = pkg_resources.resource_string(__name__, - 'testdata/rsa256_key.pem') + test_key = pkg_resources.resource_string( + __name__, 'testdata/rsa256_key.pem') nonce, key = "abcdef", le_util.Key("foo", test_key) self.key = key self.cert = dvsni_gen_cert(name, r_b64, nonce, key)[0] @@ -352,8 +354,8 @@ class DoChildProcessTest(unittest.TestCase): @mock.patch("letsencrypt.client.standalone_authenticator.socket.socket") @mock.patch("letsencrypt.client.standalone_authenticator.os.kill") @mock.patch("letsencrypt.client.standalone_authenticator.sys.exit") - def test_do_child_process_cantbind1(self, mock_exit, mock_kill, - mock_socket): + def test_do_child_process_cantbind1( + self, mock_exit, mock_kill, mock_socket): mock_exit.side_effect = IndentationError("subprocess would exit here") eaccess = socket.error(socket.errno.EACCES, "Permission denied") sample_socket = mock.MagicMock() @@ -365,8 +367,9 @@ class DoChildProcessTest(unittest.TestCase): # (Just replacing it with a no-op causes logic errors because the # do_child_process code assumes that calling sys.exit() will # cause subsequent code not to be executed.) - self.assertRaises(IndentationError, - self.authenticator.do_child_process, 1717, self.key) + self.assertRaises( + IndentationError, self.authenticator.do_child_process, 1717, + self.key) mock_exit.assert_called_once_with(1) mock_kill.assert_called_once_with(12345, signal.SIGUSR2) @@ -380,8 +383,9 @@ class DoChildProcessTest(unittest.TestCase): sample_socket = mock.MagicMock() sample_socket.bind.side_effect = eaccess mock_socket.return_value = sample_socket - self.assertRaises(IndentationError, - self.authenticator.do_child_process, 1717, self.key) + self.assertRaises( + IndentationError, self.authenticator.do_child_process, 1717, + self.key) mock_exit.assert_called_once_with(1) mock_kill.assert_called_once_with(12345, signal.SIGUSR1) @@ -395,8 +399,8 @@ class DoChildProcessTest(unittest.TestCase): sample_socket = mock.MagicMock() sample_socket.bind.side_effect = eio mock_socket.return_value = sample_socket - self.assertRaises(socket.error, - self.authenticator.do_child_process, 1717, self.key) + self.assertRaises( + socket.error, self.authenticator.do_child_process, 1717, self.key) @mock.patch("letsencrypt.client.standalone_authenticator." "OpenSSL.SSL.Connection") @@ -408,8 +412,9 @@ class DoChildProcessTest(unittest.TestCase): sample_socket.accept.side_effect = SocketAcceptOnlyNTimes(2) mock_socket.return_value = sample_socket mock_connection.return_value = mock.MagicMock() - self.assertRaises(CallableExhausted, - self.authenticator.do_child_process, 1717, self.key) + self.assertRaises( + CallableExhausted, self.authenticator.do_child_process, 1717, + self.key) mock_socket.assert_called_once_with() sample_socket.bind.assert_called_once_with(("0.0.0.0", 1717)) sample_socket.listen.assert_called_once_with(1) From d8fd3e4e61d01709024897ac9b304a93eccbac44 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 9 Feb 2015 14:23:09 -0800 Subject: [PATCH 40/45] Reorganizing import statements --- .../client/standalone_authenticator.py | 23 +++++----- .../tests/standalone_authenticator_test.py | 44 +++++++++++-------- 2 files changed, 39 insertions(+), 28 deletions(-) diff --git a/letsencrypt/client/standalone_authenticator.py b/letsencrypt/client/standalone_authenticator.py index 9eb6bc392..7723c840d 100644 --- a/letsencrypt/client/standalone_authenticator.py +++ b/letsencrypt/client/standalone_authenticator.py @@ -6,21 +6,23 @@ This authenticator creates its own ephemeral TCP listener on the specified port in order to respond to incoming DVSNI challenges from the certificate authority.""" -from letsencrypt.client.challenge_util import DvsniChall -from letsencrypt.client.challenge_util import dvsni_gen_cert -from letsencrypt.client import CONFIG -from letsencrypt.client import interfaces -import Crypto.Random -import OpenSSL.crypto -import OpenSSL.SSL + import os import signal import socket import sys import time + +import Crypto.Random +import OpenSSL.crypto +import OpenSSL.SSL import zope.component import zope.interface +from letsencrypt.client import challenge_util +from letsencrypt.client import CONFIG +from letsencrypt.client import interfaces + class StandaloneAuthenticator(object): # pylint: disable=too-many-instance-attributes @@ -256,11 +258,12 @@ class StandaloneAuthenticator(object): # TODO: Specify a correct exception subclass. raise Exception(".perform() was called without challenge list") for chall in chall_list: - if isinstance(chall, DvsniChall): + if isinstance(chall, challenge_util.DvsniChall): # We will attempt to do it name, r_b64 = chall.domain, chall.r_b64 nonce, key = chall.nonce, chall.key - cert, s_b64 = dvsni_gen_cert(name, r_b64, nonce, key) + cert, s_b64 = challenge_util.dvsni_gen_cert( + name, r_b64, nonce, key) self.tasks[nonce + CONFIG.INVALID_EXT] = cert results_if_success.append({"type": "dvsni", "s": s_b64}) results_if_failure.append(None) @@ -292,7 +295,7 @@ class StandaloneAuthenticator(object): """ # Remove this from pending tasks list for chall in chall_list: - assert isinstance(chall, DvsniChall) + assert isinstance(chall, challenge_util.DvsniChall) nonce = chall.nonce if nonce + CONFIG.INVALID_EXT in self.tasks: del self.tasks[nonce + CONFIG.INVALID_EXT] diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index a8705d3cc..9a035a3da 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -4,17 +4,17 @@ import mock import unittest -from letsencrypt.client.challenge_util import DvsniChall -from letsencrypt.client.challenge_util import dvsni_gen_cert -from letsencrypt.client import le_util -from OpenSSL.crypto import FILETYPE_PEM -import OpenSSL.crypto -import OpenSSL.SSL import os import pkg_resources import signal import socket +import OpenSSL.crypto +import OpenSSL.SSL + +from letsencrypt.client import challenge_util +from letsencrypt.client import le_util + # Classes based on to allow interrupting infinite loop under test # after one iteration, based on. @@ -67,8 +67,9 @@ class SNICallbackTest(unittest.TestCase): test_key = pkg_resources.resource_string( __name__, 'testdata/rsa256_key.pem') nonce, key = "abcdef", le_util.Key("foo", test_key) - self.cert = dvsni_gen_cert(name, r_b64, nonce, key)[0] - private_key = OpenSSL.crypto.load_privatekey(FILETYPE_PEM, key.pem) + self.cert = challenge_util.dvsni_gen_cert(name, r_b64, nonce, key)[0] + private_key = OpenSSL.crypto.load_privatekey( + OpenSSL.crypto.FILETYPE_PEM, key.pem) self.authenticator.private_key = private_key self.authenticator.tasks = {"abcdef.acme.invalid": self.cert} self.authenticator.child_pid = 12345 @@ -190,8 +191,10 @@ class PerformTest(unittest.TestCase): test_key = pkg_resources.resource_string( __name__, 'testdata/rsa256_key.pem') key = le_util.Key("something", test_key) - chall1 = DvsniChall("foo.example.com", "whee", "foononce", key) - chall2 = DvsniChall("bar.example.com", "whee", "barnonce", key) + chall1 = challenge_util.DvsniChall( + "foo.example.com", "whee", "foononce", key) + chall2 = challenge_util.DvsniChall( + "bar.example.com", "whee", "barnonce", key) bad_chall = ("This", "Represents", "A Non-DVSNI", "Challenge") self.authenticator.start_listener = mock.Mock() self.authenticator.start_listener.return_value = True @@ -215,8 +218,10 @@ class PerformTest(unittest.TestCase): test_key = pkg_resources.resource_string( __name__, 'testdata/rsa256_key.pem') key = le_util.Key("something", test_key) - chall1 = DvsniChall("foo.example.com", "whee", "foononce", key) - chall2 = DvsniChall("bar.example.com", "whee", "barnonce", key) + chall1 = challenge_util.DvsniChall( + "foo.example.com", "whee", "foononce", key) + chall2 = challenge_util.DvsniChall( + "bar.example.com", "whee", "barnonce", key) bad_chall = ("This", "Represents", "A Non-DVSNI", "Challenge") self.authenticator.start_listener = mock.Mock() self.authenticator.start_listener.return_value = False @@ -233,12 +238,12 @@ class PerformTest(unittest.TestCase): def test_perform_with_pending_tasks(self): self.authenticator.tasks = {"foononce.acme.invalid": "cert_data"} - extra_challenge = DvsniChall("a", "b", "c", "d") + extra_challenge = challenge_util.DvsniChall("a", "b", "c", "d") self.assertRaises( Exception, self.authenticator.perform, [extra_challenge]) def test_perform_without_challenge_list(self): - extra_challenge = DvsniChall("a", "b", "c", "d") + extra_challenge = challenge_util.DvsniChall("a", "b", "c", "d") # This is wrong because a challenge must be specified. self.assertRaises(Exception, self.authenticator.perform, []) # This is wrong because it must be a list, not a bare challenge. @@ -345,8 +350,9 @@ class DoChildProcessTest(unittest.TestCase): __name__, 'testdata/rsa256_key.pem') nonce, key = "abcdef", le_util.Key("foo", test_key) self.key = key - self.cert = dvsni_gen_cert(name, r_b64, nonce, key)[0] - private_key = OpenSSL.crypto.load_privatekey(FILETYPE_PEM, key.pem) + self.cert = challenge_util.dvsni_gen_cert(name, r_b64, nonce, key)[0] + private_key = OpenSSL.crypto.load_privatekey( + OpenSSL.crypto.FILETYPE_PEM, key.pem) self.authenticator.private_key = private_key self.authenticator.tasks = {"abcdef.acme.invalid": self.cert} self.authenticator.parent_pid = 12345 @@ -439,13 +445,15 @@ class CleanupTest(unittest.TestCase): def test_cleanup(self, mock_sleep, mock_kill): mock_sleep.return_value = None mock_kill.return_value = None - chall = DvsniChall("foo.example.com", "whee", "foononce", "key") + chall = challenge_util.DvsniChall( + "foo.example.com", "whee", "foononce", "key") self.authenticator.cleanup([chall]) mock_kill.assert_called_once_with(12345, signal.SIGINT) mock_sleep.assert_called_once_with(1) def test_bad_cleanup(self): - chall = DvsniChall("bad.example.com", "whee", "badnonce", "key") + chall = challenge_util.DvsniChall( + "bad.example.com", "whee", "badnonce", "key") self.assertRaises(ValueError, self.authenticator.cleanup, [chall]) From 2591abd535da78bf4b8a8faa751a3784850eb8b6 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 9 Feb 2015 14:25:18 -0800 Subject: [PATCH 41/45] Change assertion to a ValueError in signal handler --- letsencrypt/client/standalone_authenticator.py | 2 +- letsencrypt/client/tests/standalone_authenticator_test.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/letsencrypt/client/standalone_authenticator.py b/letsencrypt/client/standalone_authenticator.py index 7723c840d..646afc6a8 100644 --- a/letsencrypt/client/standalone_authenticator.py +++ b/letsencrypt/client/standalone_authenticator.py @@ -60,7 +60,7 @@ class StandaloneAuthenticator(object): self.subproc_state = "cantbind" else: # NOTREACHED - assert False + raise ValueError("Unexpected signal in signal handler") def subproc_signal_handler(self, sig, unused_frame): """Signal handler for the child process. diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index 9a035a3da..7bb15d5f0 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -123,7 +123,7 @@ class ClientSignalHandlerTest(unittest.TestCase): # function is only set as a signal handler for the above three # signals). self.assertRaises( - AssertionError, self.authenticator.client_signal_handler, + ValueError, self.authenticator.client_signal_handler, signal.SIGPIPE, None) From b4418f72ffd2c3ac32e254124ad53aa903852ca6 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 9 Feb 2015 14:42:16 -0800 Subject: [PATCH 42/45] Improve docstrings --- .../client/standalone_authenticator.py | 70 +++++++++++++------ 1 file changed, 48 insertions(+), 22 deletions(-) diff --git a/letsencrypt/client/standalone_authenticator.py b/letsencrypt/client/standalone_authenticator.py index 646afc6a8..7af869893 100644 --- a/letsencrypt/client/standalone_authenticator.py +++ b/letsencrypt/client/standalone_authenticator.py @@ -47,8 +47,9 @@ class StandaloneAuthenticator(object): """Signal handler for the parent process. This handler receives inter-process communication from the - child process in the form of Unix signals.""" - # signal handler for use in parent process + child process in the form of Unix signals. + + :param int sig: Which signal the process received.""" # subprocess → client READY : SIGIO # subprocess → client INUSE : SIGUSR1 # subprocess → client CANTBIND: SIGUSR2 @@ -66,8 +67,9 @@ class StandaloneAuthenticator(object): """Signal handler for the child process. This handler receives inter-process communication from the parent - process in the form of Unix signals.""" - # signal handler for use in subprocess + process in the form of Unix signals. + + :param int sig: Which signal the process received.""" # client → subprocess CLEANUP : SIGINT if sig == signal.SIGINT: try: @@ -97,7 +99,10 @@ class StandaloneAuthenticator(object): This method will set a new OpenSSL context object for this connection when an incoming connection provides an SNI name - (in order to serve the appropriate certificate, if any).""" + (in order to serve the appropriate certificate, if any). + + :param OpenSSL.Connection connection: The TLS connection object + on which the SNI extension was received.""" sni_name = connection.get_servername() if sni_name in self.tasks: @@ -121,6 +126,10 @@ class StandaloneAuthenticator(object): up to delay_amount seconds to hear from the child process via a signal. + :param int port: Which TCP port to bind. + :param float delay_amount: How long in seconds to wait for the + subprocess to notify us whether it succeeded. + :returns: True or False according to whether we were notified that the child process succeeded or failed in binding the port.""" @@ -158,7 +167,11 @@ class StandaloneAuthenticator(object): Normally does not return; instead, the child process exits from within this function or from within the child process signal - handler.""" + handler. + + :param int port: Which TCP port to bind. + :param le_util.Key key: The private key to use to respond to + DVSNI challenge requests.""" signal.signal(signal.SIGINT, self.subproc_signal_handler) self.sock = socket.socket() try: @@ -208,7 +221,8 @@ class StandaloneAuthenticator(object): specified port to perform the specified DVSNI challenges. :param int port: The TCP port to bind. - :param str key: The private key to use (in PEM format). + :param le_util.Key key: The private key to use to respond to + DVSNI challenge requests. :returns: True or False to indicate success or failure creating the subprocess. """ @@ -231,22 +245,31 @@ class StandaloneAuthenticator(object): def get_chall_pref(self, unused_domain): # pylint: disable=no-self-use - """IAuthenticator interface method: Return a list of challenge - types that this authenticator can perform for this domain. In - the case of the StandaloneAuthenticator, the only challenge - type that can ever be performed is dvsni. - """ + """IAuthenticator interface method get_chall_pref. + + Return a list of challenge types that this authenticator + can perform for this domain. In the case of the + StandaloneAuthenticator, the only challenge type that can ever + be performed is dvsni. + + :returns: A list containing only 'dvsni'.""" return ["dvsni"] def perform(self, chall_list): - """IAuthenticator interface method: Attempt to perform the + """IAuthenticator interface method perform. + + Attempt to perform the specified challenges, returning the status of each. For the StandaloneAuthenticator, because there is no convenient way to add additional requests, this should only be invoked once; subsequent invocations are an error. To perform validations for multiple independent sets of domains, a separate StandaloneAuthenticator should be instantiated. - """ + + :param list chall_list: A list of the the challenge objects to + be attempted by this authenticator. + :returns: A list in the same order containing, in each position, + the successfully configured challenge, False, or None.""" if self.child_pid or self.tasks: # We should not be willing to continue with perform # if there were existing pending challenges. @@ -285,14 +308,17 @@ class StandaloneAuthenticator(object): return results_if_failure def cleanup(self, chall_list): - """IAuthenticator interface method: Remove each of the specified - challenges from the list of challenges that still need to be - performed. (In the case of the StandaloneAuthenticator, if some - challenges are removed from the list, the authenticator socket - will still respond to those challenges.) Once all challenges - have been removed from the list, the listener is deactivated and - stops listening. - """ + """IAuthenticator interface method cleanup. + + Remove each of the specified challenges from the list of + challenges that still need to be performed. (In the case of + the StandaloneAuthenticator, if some challenges are removed + from the list, the authenticator socket will still respond to + those challenges.) Once all challenges have been removed from + the list, the listener is deactivated and stops listening. + + :param list chall_list: A list of the the challenge objects to + be deactivated.""" # Remove this from pending tasks list for chall in chall_list: assert isinstance(chall, challenge_util.DvsniChall) From 82617c79b2a1b1c2e4fbc4cb9614f43ac56853c3 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 9 Feb 2015 15:26:46 -0800 Subject: [PATCH 43/45] Remove two unused import statements --- letsencrypt/client/tests/apache/dvsni_test.py | 1 - letsencrypt/client/tests/challenge_util_test.py | 1 - 2 files changed, 2 deletions(-) diff --git a/letsencrypt/client/tests/apache/dvsni_test.py b/letsencrypt/client/tests/apache/dvsni_test.py index 862b82e88..7fbce9cbb 100644 --- a/letsencrypt/client/tests/apache/dvsni_test.py +++ b/letsencrypt/client/tests/apache/dvsni_test.py @@ -8,7 +8,6 @@ import mock from letsencrypt.client import challenge_util from letsencrypt.client import constants -from letsencrypt.client import client from letsencrypt.client import le_util from letsencrypt.client.tests.apache import util diff --git a/letsencrypt/client/tests/challenge_util_test.py b/letsencrypt/client/tests/challenge_util_test.py index 7400945d8..a8d40630e 100644 --- a/letsencrypt/client/tests/challenge_util_test.py +++ b/letsencrypt/client/tests/challenge_util_test.py @@ -7,7 +7,6 @@ import unittest import M2Crypto from letsencrypt.client import challenge_util -from letsencrypt.client import client from letsencrypt.client import constants from letsencrypt.client import le_util From 4d7a67388790930939121142dc943bea2173c5ca Mon Sep 17 00:00:00 2001 From: James Kasten Date: Mon, 2 Feb 2015 18:11:48 -0800 Subject: [PATCH 44/45] refactor client.namedtuples to le_util Conflicts: letsencrypt/client/client.py letsencrypt/client/le_util.py letsencrypt/client/tests/apache/dvsni_test.py letsencrypt/client/tests/challenge_util_test.py --- letsencrypt/client/apache/dvsni.py | 2 +- letsencrypt/client/auth_handler.py | 4 ++-- letsencrypt/client/challenge_util.py | 2 +- letsencrypt/client/client.py | 18 +++++++----------- letsencrypt/client/le_util.py | 21 +++++++++++---------- 5 files changed, 22 insertions(+), 25 deletions(-) diff --git a/letsencrypt/client/apache/dvsni.py b/letsencrypt/client/apache/dvsni.py index f9efdf559..9b4cd957a 100644 --- a/letsencrypt/client/apache/dvsni.py +++ b/letsencrypt/client/apache/dvsni.py @@ -18,7 +18,7 @@ class ApacheDvsni(object): :ivar dvsni_chall: Data required for challenges. where DvsniChall tuples have the following fields `domain` (`str`), `r_b64` (base64 `str`), `nonce` (hex `str`) - `key` (:class:`letsencrypt.client.client.Client.Key`) + `key` (:class:`letsencrypt.client.le_util.Key`) :type dvsni_chall: `list` of :class:`letsencrypt.client.challenge_util.DvsniChall` diff --git a/letsencrypt/client/auth_handler.py b/letsencrypt/client/auth_handler.py index 8e4331ac9..6f0ece535 100644 --- a/letsencrypt/client/auth_handler.py +++ b/letsencrypt/client/auth_handler.py @@ -25,7 +25,7 @@ class AuthHandler(object): # pylint: disable=too-many-instance-attributes :ivar list domains: list of str domains to get authorization :ivar dict authkey: Authorized Keys for each domain. - values are of type :class:`letsencrypt.client.client.Client.Key` + values are of type :class:`letsencrypt.client.le_util.Key` :ivar dict responses: keys: domain, values: list of dict responses :ivar dict msgs: ACME Challenge messages with domain as a key :ivar dict paths: optimal path for authorization. eg. paths[domain] @@ -56,7 +56,7 @@ class AuthHandler(object): # pylint: disable=too-many-instance-attributes :param dict msg: ACME challenge message :param authkey: authorized key for the challenge - :type authkey: :class:`letsencrypt.client.client.Client.Key` + :type authkey: :class:`letsencrypt.client.le_util.Key` """ if domain in self.domains: diff --git a/letsencrypt/client/challenge_util.py b/letsencrypt/client/challenge_util.py index 269e3ff17..b836fd142 100644 --- a/letsencrypt/client/challenge_util.py +++ b/letsencrypt/client/challenge_util.py @@ -35,7 +35,7 @@ def dvsni_gen_cert(name, r_b64, nonce, key): :param str nonce: hex value of nonce :param key: Key to perform challenge - :type key: :class:`letsencrypt.client.client.Client.Key` + :type key: :class:`letsencrypt.client.le_util.Key` :returns: tuple of (cert_pem, s) where cert_pem is the certificate in pem form diff --git a/letsencrypt/client/client.py b/letsencrypt/client/client.py index fdbad1840..b57333313 100644 --- a/letsencrypt/client/client.py +++ b/letsencrypt/client/client.py @@ -1,5 +1,4 @@ """ACME protocol client class and helper functions.""" -import collections import csv import logging import os @@ -30,7 +29,7 @@ class Client(object): :type network: :class:`letsencrypt.client.network.Network` :ivar authkey: Authorization Key - :type authkey: :class:`letsencrypt.client.client.Client.Key` + :type authkey: :class:`letsencrypt.client.le_util.Key` :ivar auth_handler: Object that supports the IAuthenticator interface. auth_handler contains both a dv_authenticator and a client_authenticator @@ -45,9 +44,6 @@ class Client(object): """ zope.interface.implements(interfaces.IAuthenticator) - # Note: form is the type of data, "pem" or "der" - CSR = collections.namedtuple("CSR", "file data form") - def __init__(self, config, authkey, dv_auth, installer): """Initialize a client. @@ -174,7 +170,7 @@ class Client(object): :param list domains: list of domains to install the certificate :param privkey: private key for certificate - :type privkey: :class:`Key` + :type privkey: :class:`letsencrypt.client.le_util.Key` :param str cert_file: certificate file path :param str chain_file: chain file path @@ -301,10 +297,10 @@ def validate_key_csr(privkey, csr=None): If csr is left as None, only the key will be validated. :param privkey: Key associated with CSR - :type privkey: :class:`letsencrypt.client.client.Client.Key` + :type privkey: :class:`letsencrypt.client.le_util.Key` :param csr: CSR - :type csr: :class:`letsencrypt.client.client.Client.CSR` + :type csr: :class:`letsencrypt.client.le_util.CSR` :raises LetsEncryptClientError: if validation fails @@ -321,7 +317,7 @@ def validate_key_csr(privkey, csr=None): if csr: if csr.form == "der": csr_obj = M2Crypto.X509.load_request_der_string(csr.data) - csr = Client.CSR(csr.file, csr_obj.as_pem(), "der") + csr = le_util.CSR(csr.file, csr_obj.as_pem(), "der") # If CSR is provided, it must be readable and valid. if csr.data and not crypto_util.valid_csr(csr.data): @@ -383,14 +379,14 @@ def init_csr(privkey, names, cert_dir): logging.info("Creating CSR: %s", csr_filename) - return Client.CSR(csr_filename, csr_der, "der") + return le_util.CSR(csr_filename, csr_der, "der") def csr_pem_to_der(csr): """Convert pem CSR to der.""" csr_obj = M2Crypto.X509.load_request_string(csr.data) - return Client.CSR(csr.file, csr_obj.as_der(), "der") + return le_util.CSR(csr.file, csr_obj.as_der(), "der") # This should be controlled by commandline parameters diff --git a/letsencrypt/client/le_util.py b/letsencrypt/client/le_util.py index c4a0170e0..9266f0ca9 100644 --- a/letsencrypt/client/le_util.py +++ b/letsencrypt/client/le_util.py @@ -9,7 +9,8 @@ from letsencrypt.client import errors Key = collections.namedtuple("Key", "file pem") - +# Note: form is the type of data, "pem" or "der" +CSR = collections.namedtuple("CSR", "file data form") def make_or_verify_dir(directory, mode=0o755, uid=0): """Make sure directory exists with proper permissions. @@ -32,8 +33,8 @@ def make_or_verify_dir(directory, mode=0o755, uid=0): if exception.errno == errno.EEXIST: if not check_permissions(directory, mode, uid): raise errors.LetsEncryptClientError( - '%s exists, but does not have the proper ' - 'permissions or owner' % directory) + "%s exists, but does not have the proper " + "permissions or owner" % directory) else: raise @@ -68,7 +69,7 @@ def unique_file(path, mode=0o777): fname = os.path.join(path, "%04d_%s" % (count, tail)) try: file_d = os.open(fname, os.O_CREAT | os.O_EXCL | os.O_RDWR, mode) - return os.fdopen(file_d, 'w'), fname + return os.fdopen(file_d, "w"), fname except OSError: pass count += 1 @@ -96,8 +97,8 @@ def jose_b64encode(data): """ if not isinstance(data, str): - raise TypeError('argument should be str or bytearray') - return base64.urlsafe_b64encode(data).rstrip('=') + raise TypeError("argument should be str or bytearray") + return base64.urlsafe_b64encode(data).rstrip("=") def jose_b64decode(data): @@ -115,11 +116,11 @@ def jose_b64decode(data): """ if isinstance(data, unicode): try: - data = data.encode('ascii') + data = data.encode("ascii") except UnicodeEncodeError: raise ValueError( - 'unicode argument should contain only ASCII characters') + "unicode argument should contain only ASCII characters") elif not isinstance(data, str): - raise TypeError('argument should be a str or unicode') + raise TypeError("argument should be a str or unicode") - return base64.urlsafe_b64decode(data + '=' * (4 - (len(data) % 4))) + return base64.urlsafe_b64decode(data + "=" * (4 - (len(data) % 4))) From 9cc7b0945bf321e255644bcec514d841a998cdab Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Tue, 10 Feb 2015 17:51:49 -0800 Subject: [PATCH 45/45] raise ValueError instead of raw Exception --- letsencrypt/client/standalone_authenticator.py | 9 +++------ .../client/tests/standalone_authenticator_test.py | 8 ++++---- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/letsencrypt/client/standalone_authenticator.py b/letsencrypt/client/standalone_authenticator.py index a6b774e56..a1b1daa58 100755 --- a/letsencrypt/client/standalone_authenticator.py +++ b/letsencrypt/client/standalone_authenticator.py @@ -273,13 +273,11 @@ class StandaloneAuthenticator(object): if self.child_pid or self.tasks: # We should not be willing to continue with perform # if there were existing pending challenges. - # TODO: Specify a correct exception subclass. - raise Exception(".perform() was called with pending tasks!") + raise ValueError(".perform() was called with pending tasks!") results_if_success = [] results_if_failure = [] if not chall_list or not isinstance(chall_list, list): - # TODO: Specify a correct exception subclass. - raise Exception(".perform() was called without challenge list") + raise ValueError(".perform() was called without challenge list") for chall in chall_list: if isinstance(chall, challenge_util.DvsniChall): # We will attempt to do it @@ -296,8 +294,7 @@ class StandaloneAuthenticator(object): results_if_success.append(False) results_if_failure.append(False) if not self.tasks: - # TODO: Specify a correct exception subclass. - raise Exception("nothing for .perform() to do") + raise ValueError("nothing for .perform() to do") # Try to do the authentication; note that this creates # the listener subprocess via os.fork() if self.start_listener(constants.DVSNI_CHALLENGE_PORT, key): diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index 7bb15d5f0..0beb0b1d9 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -240,18 +240,18 @@ class PerformTest(unittest.TestCase): self.authenticator.tasks = {"foononce.acme.invalid": "cert_data"} extra_challenge = challenge_util.DvsniChall("a", "b", "c", "d") self.assertRaises( - Exception, self.authenticator.perform, [extra_challenge]) + ValueError, self.authenticator.perform, [extra_challenge]) def test_perform_without_challenge_list(self): extra_challenge = challenge_util.DvsniChall("a", "b", "c", "d") # This is wrong because a challenge must be specified. - self.assertRaises(Exception, self.authenticator.perform, []) + self.assertRaises(ValueError, self.authenticator.perform, []) # This is wrong because it must be a list, not a bare challenge. self.assertRaises( - Exception, self.authenticator.perform, extra_challenge) + ValueError, self.authenticator.perform, extra_challenge) # This is wrong because the list must contain at least one challenge. self.assertRaises( - Exception, self.authenticator.perform, range(20)) + ValueError, self.authenticator.perform, range(20)) class StartListenerTest(unittest.TestCase):