Added name check and attempted to clean up code - is PEP8 conformant

letsencrypt/client/client.py

@@ -47,34 +47,7 @@ class Client(object):
         self.csr_file = cert_signing_request
         self.key_file = private_key
 
-        # If CSR is provided, the private key should also be provided.
-        if self.csr_file and not self.key_file:
-            logger.fatal("Please provide the private key file used in \
-            generating the provided CSR")
-            sys.exit(1)
-        # If CSR is provided, it must be readable and valid.
-        try:
-            if self.csr_file and not crypto_util.valid_csr(self.csr_file):
-                logger.fatal("The provided CSR is not a valid CSR")
-                sys.exit(1)
-        except IOError, e:
-            logger.fatal("The provided CSR could not be read")
-            sys.exit(1)
-        # If key is provided, it must be readable and valid.
-        try:
-            if self.key_file and not crypto_util.valid_privkey(self.key_file):
-                logger.fatal("The provided key is not a valid key")
-                sys.exit(1)
-        except IOError, e:
-            logger.fatal("The provided key could not be read")
-            sys.exit(1)
-        # If CSR and key are provided, the key must be the same key used
-        # in the CSR.
-        if self.csr_file and self.key_file and not \
-                crypto_util.csr_matches_pubkey(self.csr_file, self.key_file):
-            logger.fatal("The provided key is not the same key referred to by \
-            the CSR file")
-            sys.exit(1)
+        self.__validate_csr_key_cli()
 
         self.server_url = "https://%s/acme/" % self.server
 
@@ -119,6 +92,9 @@ class Client(object):
         # Get key and csr to perform challenges
         _, csr_der = self.get_key_csr_pem()
 
+        if not crypto_util.csr_matches_names(self.csr_file, self.names):
+            raise Exception("CSR subject does not contain the specified name")
+
         # Perform Challenges
         responses, challenge_objs = self.verify_identity(challenge_msg)
         # Get Authorization
@@ -658,6 +634,40 @@ class Client(object):
         else:
             return key_pem, csr_pem
 
+    def __validate_csr_key_cli(self):
+        """
+        Verifies that the client key and csr arguments are valid and correspond
+        to one another"""
+        # If CSR is provided, the private key should also be provided.
+        if self.csr_file and not self.key_file:
+            logger.fatal(("Please provide the private key file used in "
+                          "generating the provided CSR"))
+            sys.exit(1)
+        # If CSR is provided, it must be readable and valid.
+        try:
+            if self.csr_file and not crypto_util.valid_csr(self.csr_file):
+                logger.fatal("The provided CSR is not a valid CSR")
+                sys.exit(1)
+        except IOError, e:
+            raise Exception("The provided CSR could not be read")
+        # If key is provided, it must be readable and valid.
+        try:
+            if self.key_file and not crypto_util.valid_privkey(self.key_file):
+                logger.fatal("The provided key is not a valid key")
+                sys.exit(1)
+        except IOError, e:
+            raise Exception("The provided key could not be read")
+
+        # If CSR and key are provided, the key must be the same key used
+        # in the CSR.
+        if self.csr_file and self.key_file:
+            try:
+                if not crypto_util.csr_matches_pubkey(self.csr_file,
+                                                      self.key_file):
+                    raise Exception("The key and CSR do not match")
+            except IOError, e:
+                raise Exception("The key or CSR files could not be read")
+
     # def choice_of_ca(self):
     #     choices = self.get_cas()
     #     message = ("Pick a Certificate Authority. "

letsencrypt/client/crypto_util.py

@@ -148,12 +148,12 @@ def make_ss_cert(key_file, domains):
     cert.set_not_before(current)
     cert.set_not_after(expire)
 
-    name = cert.get_subject()
-    name.C = "US"
-    name.ST = "Michigan"
-    name.L = "Ann Arbor"
-    name.O = "University of Michigan and the EFF"
-    name.CN = domains[0]
+    subject = cert.get_subject()
+    subject.C = "US"
+    subject.ST = "Michigan"
+    subject.L = "Ann Arbor"
+    subject.O = "University of Michigan and the EFF"
+    subject.CN = domains[0]
     cert.set_issuer(cert.get_subject())
 
     cert.add_ext(M2Crypto.X509.new_extension('basicConstraints', 'CA:FALSE'))
@@ -199,10 +199,15 @@ def get_cert_info(filename):
     }
 
 
+# WARNING: the csr and private key file are possible attack vectors for TOCTOU
+# We should either...
+# A. Do more checks to verify that the CSR is trusted/valid
+# B. Audit the parsing code for vulnerabilities
+
 def valid_csr(csr_filename):
-    """Check if csr_filename is a valid CSR.  (Currently, could raise
-    non-X.509-related errors such as IOError associated with problems
-    reading the file.)
+    """Check if csr_filename is a valid CSR for the given domains.
+    (Currently, could raise non-X.509-related errors such as IOError
+    associated with problems reading the file.)
 
     :param csr_filename: Path to the purported CSR file.
     :type csr_filename: str
@@ -217,6 +222,32 @@ def valid_csr(csr_filename):
         return False
 
 
+def csr_matches_names(csr_filename, domains):
+    """Check if csr_filename contains the subject of one of the domains
+    M2Crypto currently does not expose the OpenSSL interface to
+    also check the SAN extension. This is insufficient for full testing
+    (Currently, could raise non-X.509-related errors such as IOError
+    associated with problems reading the file.)
+
+    :param csr_filename: Path to the purported CSR file.
+    :type csr_filename: str
+
+    :param domains: domains the csr should contain
+    :type domains: list
+
+    :returns: If the csr subject contains one of the domains
+    :rtype: bool"""
+
+    try:
+        csr = M2Crypto.X509.load_request(csr_filename)
+        subject = csr.get_subject()
+
+        return subject.CN in domains
+
+    except M2Crypto.X509.X509Error:
+        return False
+
+
 def valid_privkey(privkey_filename):
     """Check if privkey_filename is a valid RSA private key.  (Currently,
     could raise non-RSA-related errors such as IOError associated with