Cleanup Installer API change

.pylintrc

@@ -322,7 +322,11 @@ max-attributes=7
 min-public-methods=2
 
 # Maximum number of public methods for a class (see R0904).
-max-public-methods=20
+# Pylint counts all of the public methods that you also inherit.
+# This has been reported/fixed as a bug, but until our version is fixed,
+# I think this will only cause us headaches. (Unittests are automatically over)
+# https://bitbucket.org/logilab/pylint/issue/248/too-many-public-methods-triggered-from
+max-public-methods=100
 
 
 [EXCEPTIONS]

README.md

@@ -127,13 +127,9 @@ optional arguments:
   -p PRIVKEY, --privkey PRIVKEY
                         Path to the private key file for certificate
                         generation.
-  -c CSR, --csr CSR     Path to the certificate signing request file
-                        corresponding to the private key file. The private key
-                        file argument is required if this argument is
-                        specified.
   -b N, --rollback N    Revert configuration N number of checkpoints.
   -k, --revoke          Revoke a certificate.
-  -v, --view-checkpoints
+  -v, --view-config-changes
                         View checkpoints and associated configuration changes.
   -r, --redirect        Automatically redirect all HTTP traffic to HTTPS for
                         the newly authenticated vhost.

letsencrypt/client/apache/configurator.py

@@ -49,14 +49,12 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
     Apache 2.2 and this code works for Ubuntu 14.04 Apache 2.4. Further
     notes below.
 
-    This class was originally developed for Apache 2.2 and has not seen a
-    an overhaul to include proper setup of new Apache configurations.
+    This class was originally developed for Apache 2.2 and I have been slowly
+    transitioning the codebase to work with all of the 2.4 features.
     I have implemented most of the changes... the missing ones are
     mod_ssl.c vs ssl_mod, and I need to account for configuration variables.
-    That being said, this class can still adequately configure most typical
-    Apache 2.4 servers as the deprecated NameVirtualHost has no effect
-    and the typical directories are parsed by the Augeas configuration
-    parser automatically.
+    This class can adequately configure most typical configurations but
+    is not ready to handle very complex configurations.
 
     .. todo:: Add support for config file variables Define rootDir /var/www/
     .. todo:: Add proper support for module configuration
@@ -125,8 +123,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         # Enable mod_ssl if it isn't already enabled
         # This is Let's Encrypt... we enable mod_ssl on initialization :)
         # TODO: attempt to make the check faster... this enable should
-        #       be asynchronous as it shouldn't be that time sensitive
-        #       on initialization
+        #     be asynchronous as it shouldn't be that time sensitive
+        #     on initialization
         self._prepare_server_https()
 
         self.enhance_func = {"redirect": self._enable_redirect}
@@ -136,10 +134,11 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         """Deploys certificate to specified virtual host.
 
         Currently tries to find the last directives to deploy the cert in
-        the given virtualhost. If it can't find the directives, it searches
-        the "included" confs. The function verifies that it has located
-        the three directives and finally modifies them to point to the correct
-        destination
+        the VHost associated with the given domain. If it can't find the
+        directives, it searches the "included" confs. The function verifies that
+        it has located the three directives and finally modifies them to point
+        to the correct destination. After the certificate is installed, the
+        VirtualHost is enabled if it isn't already.
 
         .. todo:: Make sure last directive is changed
 
@@ -190,11 +189,13 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         self.save_notes += "\tSSLCertificateKeyFile %s\n" % key
         if cert_chain:
             self.save_notes += "\tSSLCertificateChainFile %s\n" % cert_chain
-        # This is a significant operation, make a checkpoint
-        # return self.save()
+
+        # Make sure vhost is enabled
+        if not vhost.enabled:
+            self.enable_site(vhost)
 
     def choose_vhost(self, target_name):
-        """ Chooses a virtual host based on the given domain name.
+        """Chooses a virtual host based on the given domain name.
 
         .. todo:: This should maybe return list if no obvious answer
             is presented.
@@ -576,7 +577,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
             # Add virtual_server with redirect
             logging.debug(
                 "Did not find http version of ssl virtual host... creating")
-            return self.create_redirect_vhost(ssl_vhost)
+            return self._create_redirect_vhost(ssl_vhost)
         else:
             # Check if redirection already exists
             exists, code = self._existing_redirect(general_v)
@@ -643,7 +644,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         # Rewrite path exists but is not a letsencrypt https rule
         return True, 2
 
-    def create_redirect_vhost(self, ssl_vhost):
+    def _create_redirect_vhost(self, ssl_vhost):
         """Creates an http_vhost specifically to redirect for the ssl_vhost.
 
         :param ssl_vhost: ssl vhost

letsencrypt/client/client.py

@@ -59,23 +59,25 @@ class Client(object):
         :type dv_auth: :class:`letsencrypt.client.interfaces.IAuthenticator`
 
         """
+        sanity_check_names([server])
         self.network = network.Network(server)
         self.authkey = authkey
 
-        # sanity_check_names([server] + names)
-
         self.installer = installer
 
         client_auth = client_authenticator.ClientAuthenticator(server)
         self.auth_handler = auth_handler.AuthHandler(
             dv_auth, client_auth, self.network)
 
-    def obtain_certificate(self, domains,
+    def obtain_certificate(self, domains, csr=None,
                            cert_path=CONFIG.CERT_PATH,
                            chain_path=CONFIG.CHAIN_PATH):
         """Obtains a certificate from the ACME server.
 
         :param str domains: list of domains to get a certificate
+        :param csr: CSR must contain requested domains, the key used to generate
+            this CSR can be different than self.authkey
+        :type csr: :class:`CSR`
 
         :param str cert_path: Full desired path to end certificate.
         :param str chain_path: Full desired path to end chain file.
@@ -84,6 +86,7 @@ class Client(object):
         :rtype: `tuple` of `str`
 
         """
+        sanity_check_names(domains)
         # Request Challenges
         for name in domains:
             self.auth_handler.add_chall_msg(
@@ -93,7 +96,8 @@ class Client(object):
         self.auth_handler.get_authorizations()
 
         # Create CSR from names
-        csr = init_csr(self.authkey, domains)
+        if csr is None:
+            csr = init_csr(self.authkey, domains)
 
         # Retrieve certificate
         certificate_dict = self.acme_certificate(csr.data)
@@ -176,9 +180,6 @@ class Client(object):
         :param str chain_file: chain file path
 
         """
-        # Find set of virtual hosts to deploy certificates to
-        # vhost = self.get_virtual_hosts(self.names)
-
         chain = None if chain_file is None else os.path.abspath(chain_file)
 
         for dom in domains:
@@ -186,10 +187,6 @@ class Client(object):
                                        os.path.abspath(cert_file),
                                        os.path.abspath(privkey.file),
                                        chain)
-            # Enable any vhost that was issued to, but not enabled
-            # if not host.enabled:
-            #     logging.info("Enabling Site %s", host.filep)
-            #     self.installer.enable_site(host)
 
         self.installer.save("Deployed Let's Encrypt Certificate")
         # sites may have been enabled / final cleanup
@@ -278,32 +275,14 @@ class Client(object):
 
         """
         for dom in domains:
-            # TODO: change this to try/catch
-            self.installer.enhance(dom, "redirect")
+            try:
+                self.installer.enhance(dom, "redirect")
+            except errors.LetsEncryptConfiguratorError:
+                logging.warn('Unable to perform redirect for %s', dom)
 
         self.installer.save("Add Redirects")
         self.installer.restart()
 
-            # If successful, make sure redirect site is enabled
-            # if success:
-            #    self.installer.enable_site(redirect_vhost)
-
-    # def get_virtual_hosts(self, domains):
-    #     """Retrieve the appropriate virtual host for the domain
-
-    #     :param list domains: Domains to find ssl vhosts for
-
-    #     :returns: associated vhosts
-    #     :rtype: :class:`letsencrypt.client.apache.obj.VirtualHost`
-
-    #     """
-    #     vhost = set()
-    #     for name in domains:
-    #         host = self.installer.choose_virtual_host(name)
-    #         if host is not None:
-    #             vhost.add(host)
-    #     return vhost
-
 
 def validate_key_csr(privkey, csr=None):
     """Validate Key and CSR files.

letsencrypt/client/interfaces.py

@@ -1,7 +1,7 @@
 """Let's Encrypt client interfaces."""
 import zope.interface
 
-# pylint: disable=no-self-argument,no-method-argument,no-init
+# pylint: disable=no-self-argument,no-method-argument,no-init,too-many-public-methods
 
 
 class IAuthenticator(zope.interface.Interface):
@@ -114,7 +114,7 @@ class IInstaller(zope.interface.Interface):
     # def enable_site(vhost):
     #     """Enable the site at the given vhost.
 
-    #     :param vhost: domain 
+    #     :param vhost: domain
 
     #     """
 

letsencrypt/client/tests/apache/configurator_test.py

@@ -6,7 +6,6 @@ import shutil
 import unittest
 
 import mock
-import zope.component
 
 from letsencrypt.client import challenge_util
 from letsencrypt.client import client

letsencrypt/client/tests/apache/dvsni_test.py

@@ -5,7 +5,6 @@ import unittest
 import shutil
 
 import mock
-import zope.component
 
 from letsencrypt.client import challenge_util
 from letsencrypt.client import client
@@ -15,7 +14,7 @@ from letsencrypt.client.tests.apache import config_util
 
 
 class DvsniPerformTest(unittest.TestCase):
-
+    """Test the ApacheDVSNI challenge."""
     def setUp(self):
         from letsencrypt.client.apache import dvsni
 
@@ -60,10 +59,8 @@ class DvsniPerformTest(unittest.TestCase):
         resp = self.sni.perform()
         self.assertTrue(resp is None)
 
-    @mock.patch("letsencrypt.client.apache.configurator."
-                "ApacheConfigurator.restart")
     @mock.patch("letsencrypt.client.challenge_util.dvsni_gen_cert")
-    def test_perform1(self, mock_dvsni_gen_cert, mock_restart):
+    def test_perform1(self, mock_dvsni_gen_cert):
         chall = self.challs[0]
         self.sni.add_chall(chall)
         mock_dvsni_gen_cert.return_value = "randomS1"
@@ -87,10 +84,8 @@ class DvsniPerformTest(unittest.TestCase):
         self.assertEqual(len(responses), 1)
         self.assertEqual(responses[0]["s"], "randomS1")
 
-    @mock.patch("letsencrypt.client.apache.configurator."
-                "ApacheConfigurator.restart")
     @mock.patch("letsencrypt.client.challenge_util.dvsni_gen_cert")
-    def test_perform2(self, mock_dvsni_gen_cert, mock_restart):
+    def test_perform2(self, mock_dvsni_gen_cert):
         for chall in self.challs:
             self.sni.add_chall(chall)
 

letsencrypt/client/tests/client_authenticator_test.py

@@ -1,3 +1,4 @@
+"""Test the ClientAuthenticator dispatcher."""
 import unittest
 
 import mock
@@ -43,6 +44,7 @@ class PerformTest(unittest.TestCase):
 
 
 class CleanupTest(unittest.TestCase):
+    """Test the Authenticator cleanup function."""
     def setUp(self):
         from letsencrypt.client.client_authenticator import ClientAuthenticator
 
@@ -73,6 +75,7 @@ class CleanupTest(unittest.TestCase):
 
 
 def gen_client_resp(chall):
+    """Generate a dummy response."""
     return "%s%s" % (type(chall).__name__, chall.domain)
 
 

letsencrypt/scripts/main.py

@@ -34,11 +34,6 @@ def main():
     parser.add_argument("-p", "--privkey", dest="privkey", type=read_file,
                         help="Path to the private key file for certificate "
                              "generation.")
-    # parser.add_argument("-c", "--csr", dest="csr", type=read_file,
-    #                     help="Path to the certificate signing request file "
-    #                          "corresponding to the private key file. The "
-    #                          "private key file argument is required if this "
-    #                          "argument is specified.")
     parser.add_argument("-b", "--rollback", dest="rollback", type=int,
                         default=0, metavar="N",
                         help="Revert configuration N number of checkpoints.")
@@ -103,22 +98,11 @@ def main():
 
     domains = choose_names(installer) if args.domains is None else args.domains
 
-    # Enforce '--privkey' is set along with '--csr'.
-    # if args.csr and not args.privkey:
-    #     parser.error("private key file (--privkey) must be specified along{0} "
-    #                  "with the certificate signing request file (--csr)"
-    #                  .format(os.linesep))
-
     # Prepare for init of Client
     if args.privkey is None:
         privkey = client.init_key()
     else:
         privkey = client.Client.Key(args.privkey[0], args.privkey[1])
-    # if args.csr is None:
-    #     csr = client.init_csr(privkey, domains)
-    # else:
-    #     csr = client.csr_pem_to_der(
-    #         client.Client.CSR(args.csr[0], args.csr[1], "pem"))
 
     acme = client.Client(server, privkey, auth, installer)
 

setup.py

@@ -39,6 +39,7 @@ setup(
         'letsencrypt.client',
         'letsencrypt.client.apache',
         'letsencrypt.client.tests',
+        'letsencrypt.client.tests.apache',
         'letsencrypt.scripts',
     ],
     install_requires=install_requires,