Merge pull request #1455 from letsencrypt/useragent

Add a User Agent string for client analytics

5 files changed, 174 insertions, 51 deletions

diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py
index 30ac81092..54da4fdcc 100644
--- a/letsencrypt/cli.py
+++ b/letsencrypt/cli.py
@@ -17,7 +17,6 @@ import zope.component
 import zope.interface.exceptions
 import zope.interface.verify
 
-from acme import client as acme_client
 from acme import jose
 
 import letsencrypt
@@ -39,7 +38,6 @@ from letsencrypt.display import util as display_util
 from letsencrypt.display import ops as display_ops
 from letsencrypt.plugins import disco as plugins_disco
 
-
 logger = logging.getLogger(__name__)
 
 
@@ -304,7 +302,7 @@ def _report_new_cert(cert_path, fullchain_path):
     reporter_util.add_message(msg, reporter_util.MEDIUM_PRIORITY)
 
 
-def _auth_from_domains(le_client, config, domains, plugins):
+def _auth_from_domains(le_client, config, domains):
     """Authenticate and enroll certificate."""
     # Note: This can raise errors... caught above us though.
     lineage = _treat_as_renewal(config, domains)
@@ -325,7 +323,7 @@ def _auth_from_domains(le_client, config, domains, plugins):
         #       configuration values from this attempt? <- Absolutely (jdkasten)
     else:
         # TREAT AS NEW REQUEST
-        lineage = le_client.obtain_and_enroll_certificate(domains, plugins)
+        lineage = le_client.obtain_and_enroll_certificate(domains)
         if not lineage:
             raise errors.Error("Certificate could not be obtained")
 
@@ -425,14 +423,23 @@ def choose_configurator_plugins(args, config, plugins, verb):
             authenticator = display_ops.pick_authenticator(config, req_auth, plugins)
     logger.debug("Selected authenticator %s and installer %s", authenticator, installer)
 
+    # Report on any failures
     if need_inst and not installer:
         diagnose_configurator_problem("installer", req_inst, plugins)
     if need_auth and not authenticator:
         diagnose_configurator_problem("authenticator", req_auth, plugins)
 
+    record_chosen_plugins(config, plugins, authenticator, installer)
     return installer, authenticator
 
 
+def record_chosen_plugins(config, plugins, auth, inst):
+    "Update the config entries to reflect the plugins we actually selected."
+    cn = config.namespace
+    cn.authenticator = plugins.find_init(auth).name if auth else "none"
+    cn.installer = plugins.find_init(inst).name if inst else "none"
+
+
 # TODO: Make run as close to auth + install as possible
 # Possible difficulties: args.csr was hacked into auth
 def run(args, config, plugins):  # pylint: disable=too-many-branches,too-many-locals
@@ -447,7 +454,7 @@ def run(args, config, plugins):  # pylint: disable=too-many-branches,too-many-lo
     # TODO: Handle errors from _init_le_client?
     le_client = _init_le_client(args, config, authenticator, installer)
 
-    lineage = _auth_from_domains(le_client, config, domains, plugins)
+    lineage = _auth_from_domains(le_client, config, domains)
 
     le_client.deploy_certificate(
         domains, lineage.privkey, lineage.cert,
@@ -461,7 +468,7 @@ def run(args, config, plugins):  # pylint: disable=too-many-branches,too-many-lo
         display_ops.success_renewal(domains)
 
 
-def obtaincert(args, config, plugins):
+def obtain_cert(args, config, plugins):
     """Authenticate & obtain cert, but do not install it."""
 
     if args.domains is not None and args.csr is not None:
@@ -487,7 +494,7 @@ def obtaincert(args, config, plugins):
         _report_new_cert(cert_path, cert_fullchain)
     else:
         domains = _find_domains(args, installer)
-        _auth_from_domains(le_client, config, domains, plugins)
+        _auth_from_domains(le_client, config, domains)
 
 
 def install(args, config, plugins):
@@ -512,18 +519,19 @@ def install(args, config, plugins):
 
 def revoke(args, config, unused_plugins):  # TODO: coop with renewal config
     """Revoke a previously obtained certificate."""
+    # For user-agent construction
+    config.namespace.installer = config.namespace.authenticator = "none"
     if args.key_path is not None:  # revocation by cert key
         logger.debug("Revoking %s using cert key %s",
                      args.cert_path[0], args.key_path[0])
-        acme = acme_client.Client(
-            config.server, key=jose.JWK.load(args.key_path[1]))
+        key = jose.JWK.load(args.key_path[1])
     else:  # revocation by account key
         logger.debug("Revoking %s using Account Key", args.cert_path[0])
         acc, _ = _determine_account(args, config)
-        # pylint: disable=protected-access
-        acme = client._acme_from_config_key(config, acc.key)
-    acme.revoke(jose.ComparableX509(crypto_util.pyopenssl_load_certificate(
-        args.cert_path[1])[0]))
+        key = acc.key
+    acme = client.acme_from_config_key(config, key)
+    cert = crypto_util.pyopenssl_load_certificate(args.cert_path[1])[0]
+    acme.revoke(jose.ComparableX509(cert))
 
 
 def rollback(args, config, plugins):
@@ -625,7 +633,7 @@ class HelpfulArgumentParser(object):
     """
 
     # Maps verbs/subcommands to the functions that implement them
-    VERBS = {"auth": obtaincert, "certonly": obtaincert,
+    VERBS = {"auth": obtain_cert, "certonly": obtain_cert,
              "config_changes": config_changes, "everything": run,
              "install": install, "plugins": plugins_cmd,
              "revoke": revoke, "rollback": rollback, "run": run}
@@ -921,7 +929,13 @@ def _create_subparsers(helpful):
     helpful.add_group("revoke", description="Options for revocation of certs")
     helpful.add_group("rollback", description="Options for reverting config changes")
     helpful.add_group("plugins", description="Plugin options")
-
+    helpful.add(
+        None, "--user-agent", default=None,
+        help="Set a custom user agent string for the client. User agent strings allow "
+             "the CA to collect high level statistics about success rates by OS and "
+             "plugin. If you wish to hide your server OS version from the Let's "
+             'Encrypt server, set this to "".'
+    )
     helpful.add("certonly",
                 "--csr", type=read_file,
                 help="Path to a Certificate Signing Request (CSR) in DER"
diff --git a/letsencrypt/client.py b/letsencrypt/client.py
index 0f0ecdc6d..8e053e926 100644
--- a/letsencrypt/client.py
+++ b/letsencrypt/client.py
@@ -11,6 +11,8 @@ from acme import client as acme_client
 from acme import jose
 from acme import messages
 
+import letsencrypt
+
 from letsencrypt import account
 from letsencrypt import auth_handler
 from letsencrypt import configuration
@@ -31,10 +33,30 @@ from letsencrypt.display import enhancements
 logger = logging.getLogger(__name__)
 
 
-def _acme_from_config_key(config, key):
+def acme_from_config_key(config, key):
+    "Wrangle ACME client construction"
     # TODO: Allow for other alg types besides RS256
-    return acme_client.Client(directory=config.server, key=key,
-                              verify_ssl=(not config.no_verify_ssl))
+    net = acme_client.ClientNetwork(key, verify_ssl=(not config.no_verify_ssl),
+                                    user_agent=_determine_user_agent(config))
+    return acme_client.Client(config.server, key=key, net=net)
+
+
+def _determine_user_agent(config):
+    """
+    Set a user_agent string in the config based on the choice of plugins.
+    (this wasn't knowable at construction time)
+
+    :returns: the client's User-Agent string
+    :rtype: `str`
+    """
+
+    if config.user_agent is None:
+        ua = "LetsEncryptPythonClient/{0} ({1}) Authenticator/{2} Installer/{3}"
+        ua = ua.format(letsencrypt.__version__, " ".join(le_util.get_os_info()),
+                       config.authenticator, config.installer)
+    else:
+        ua = config.user_agent
+    return ua
 
 
 def register(config, account_storage, tos_cb=None):
@@ -86,7 +108,7 @@ def register(config, account_storage, tos_cb=None):
             public_exponent=65537,
             key_size=config.rsa_key_size,
             backend=default_backend())))
-    acme = _acme_from_config_key(config, key)
+    acme = acme_from_config_key(config, key)
     # TODO: add phone?
     regr = acme.register(messages.NewRegistration.from_data(email=config.email))
 
@@ -100,6 +122,7 @@ def register(config, account_storage, tos_cb=None):
     acc = account.Account(regr, key)
     account.report_new_account(acc, config)
     account_storage.save(acc)
+
     return acc, acme
 
 
@@ -128,7 +151,7 @@ class Client(object):
 
         # Initialize ACME if account is provided
         if acme is None and self.account is not None:
-            acme = _acme_from_config_key(config, self.account.key)
+            acme = acme_from_config_key(config, self.account.key)
         self.acme = acme
 
         # TODO: Check if self.config.enroll_autorenew is None. If
@@ -213,7 +236,7 @@ class Client(object):
 
         return self._obtain_certificate(domains, csr) + (key, csr)
 
-    def obtain_and_enroll_certificate(self, domains, plugins):
+    def obtain_and_enroll_certificate(self, domains):
         """Obtain and enroll certificate.
 
         Get a new certificate for the specified domains using the specified
@@ -230,13 +253,6 @@ class Client(object):
         """
         certr, chain, key, _ = self.obtain_certificate(domains)
 
-        # TODO: remove this dirty hack
-        self.config.namespace.authenticator = plugins.find_init(
-            self.dv_auth).name
-        if self.installer is not None:
-            self.config.namespace.installer = plugins.find_init(
-                self.installer).name
-
         # XXX: We clearly need a more general and correct way of getting
         # options into the configobj for the RenewableCert instance.
         # This is a quick-and-dirty way to do it to allow integration
diff --git a/letsencrypt/le_util.py b/letsencrypt/le_util.py
index 5626902ef..25260d755 100644
--- a/letsencrypt/le_util.py
+++ b/letsencrypt/le_util.py
@@ -3,6 +3,7 @@ import collections
 import errno
 import logging
 import os
+import platform
 import re
 import subprocess
 import stat
@@ -202,6 +203,45 @@ def safely_remove(path):
             raise
 
 
+def get_os_info():
+    """
+    Get Operating System type/distribution and major version
+
+    :returns: (os_name, os_version)
+    :rtype: `tuple` of `str`
+    """
+    info = platform.system_alias(
+        platform.system(),
+        platform.release(),
+        platform.version()
+    )
+    os_type, os_ver, _ = info
+    os_type = os_type.lower()
+    if os_type.startswith('linux'):
+        info = platform.linux_distribution()
+        # On arch, platform.linux_distribution() is reportedly ('','',''),
+        # so handle it defensively
+        if info[0]:
+            os_type = info[0]
+        if info[1]:
+            os_ver = info[1]
+    elif os_type.startswith('darwin'):
+        os_ver = subprocess.Popen(
+            ["sw_vers", "-productVersion"],
+            stdout=subprocess.PIPE
+        ).communicate()[0]
+        os_ver = os_ver.partition(".")[0]
+    elif os_type.startswith('freebsd'):
+        # eg "9.3-RC3-p1"
+        os_ver = os_ver.partition("-")[0]
+        os_ver = os_ver.partition(".")[0]
+    elif platform.win32_ver()[1]:
+        os_ver = platform.win32_ver()[1]
+    else:
+        # Cases known to fall here: Cygwin python
+        os_ver = ''
+    return os_type, os_ver
+
 # Just make sure we don't get pwned... Make sure that it also doesn't
 # start with a period or have two consecutive periods <- this needs to
 # be done in addition to the regex
diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py
index 811d11da8..500ff074e 100644
--- a/letsencrypt/tests/cli_test.py
+++ b/letsencrypt/tests/cli_test.py
@@ -10,9 +10,14 @@ import unittest
 
 import mock
 
+from acme import jose
+
 from letsencrypt import account
+from letsencrypt import cli
 from letsencrypt import configuration
+from letsencrypt import crypto_util
 from letsencrypt import errors
+from letsencrypt import le_util
 
 from letsencrypt.plugins import disco
 
@@ -20,7 +25,9 @@ from letsencrypt.tests import renewer_test
 from letsencrypt.tests import test_util
 
 
+CERT = test_util.vector_path('cert.pem')
 CSR = test_util.vector_path('csr.der')
+KEY = test_util.vector_path('rsa256_key.pem')
 
 
 class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
@@ -31,33 +38,36 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
         self.config_dir = os.path.join(self.tmp_dir, 'config')
         self.work_dir = os.path.join(self.tmp_dir, 'work')
         self.logs_dir = os.path.join(self.tmp_dir, 'logs')
+        self.standard_args = ['--text', '--config-dir', self.config_dir,
+            '--work-dir', self.work_dir, '--logs-dir', self.logs_dir,
+            '--agree-dev-preview']
 
     def tearDown(self):
         shutil.rmtree(self.tmp_dir)
 
     def _call(self, args):
-        from letsencrypt import cli
-        args = ['--text', '--config-dir', self.config_dir,
-                '--work-dir', self.work_dir, '--logs-dir', self.logs_dir,
-                '--agree-dev-preview'] + args
+        "Run the cli with output streams and actual client mocked out"
+        with mock.patch('letsencrypt.cli.client') as client:
+            ret, stdout, stderr = self._call_no_clientmock(args)
+            return ret, stdout, stderr, client
+
+    def _call_no_clientmock(self, args):
+        "Run the client with output streams mocked out"
+        args = self.standard_args + args
         with mock.patch('letsencrypt.cli.sys.stdout') as stdout:
             with mock.patch('letsencrypt.cli.sys.stderr') as stderr:
-                with mock.patch('letsencrypt.cli.client') as client:
-                    ret = cli.main(args)
-        return ret, stdout, stderr, client
+                ret = cli.main(args[:]) # NOTE: parser can alter its args!
+        return ret, stdout, stderr
 
     def _call_stdout(self, args):
         """
         Variant of _call that preserves stdout so that it can be mocked by the
         caller.
         """
-        from letsencrypt import cli
-        args = ['--text', '--config-dir', self.config_dir,
-                '--work-dir', self.work_dir, '--logs-dir', self.logs_dir,
-                '--agree-dev-preview'] + args
+        args = self.standard_args + args
         with mock.patch('letsencrypt.cli.sys.stderr') as stderr:
             with mock.patch('letsencrypt.cli.client') as client:
-                ret = cli.main(args)
+                ret = cli.main(args[:])  # NOTE: parser can alter its args!
         return ret, None, stderr, client
 
     def test_no_flags(self):
@@ -113,9 +123,34 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
         self.assertTrue("--key-path" not in out)
 
         out = self._help_output(['-h'])
-        from letsencrypt import cli
         self.assertTrue(cli.usage_strings(plugins)[0] in out)
 
+    @mock.patch('letsencrypt.cli.client.acme_client.Client')
+    @mock.patch('letsencrypt.cli._determine_account')
+    @mock.patch('letsencrypt.cli.client.Client.obtain_and_enroll_certificate')
+    @mock.patch('letsencrypt.cli._auth_from_domains')
+    def test_user_agent(self, _afd, _obt, det, _client):
+        # Normally the client is totally mocked out, but here we need more
+        # arguments to automate it...
+        args = ["--standalone", "certonly", "-m", "none@none.com",
+                "-d", "example.com", '--agree-tos'] + self.standard_args
+        det.return_value = mock.MagicMock(), None
+        with mock.patch('letsencrypt.cli.client.acme_client.ClientNetwork') as acme_net:
+            self._call_no_clientmock(args)
+            os_ver = " ".join(le_util.get_os_info())
+            ua = acme_net.call_args[1]["user_agent"]
+            self.assertTrue(os_ver in ua)
+            import platform
+            plat = platform.platform()
+            if "linux" in plat.lower():
+                self.assertTrue(platform.linux_distribution()[0] in ua)
+
+        with mock.patch('letsencrypt.cli.client.acme_client.ClientNetwork') as acme_net:
+            ua = "bandersnatch"
+            args += ["--user-agent", ua]
+            self._call_no_clientmock(args)
+            acme_net.assert_called_once_with(mock.ANY, verify_ssl=True, user_agent=ua)
+
     def test_install_abspath(self):
         cert = 'cert'
         key = 'key'
@@ -133,8 +168,9 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
         self.assertEqual(args.chain_path, os.path.abspath(chain))
         self.assertEqual(args.fullchain_path, os.path.abspath(fullchain))
 
+    @mock.patch('letsencrypt.cli.record_chosen_plugins')
     @mock.patch('letsencrypt.cli.display_ops')
-    def test_installer_selection(self, mock_display_ops):
+    def test_installer_selection(self, mock_display_ops, _rec):
         self._call(['install', '--domains', 'foo.bar', '--cert-path', 'cert',
                     '--key-path', 'key', '--chain-path', 'chain'])
         self.assertEqual(mock_display_ops.pick_installer.call_count, 1)
@@ -273,7 +309,6 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
                           ['-d', '*.wildcard.tld'])
 
     def test_parse_domains(self):
-        from letsencrypt import cli
         plugins = disco.PluginsRegistry.find_all()
 
         short_args = ['-d', 'example.com']
@@ -353,7 +388,8 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
     @mock.patch('letsencrypt.cli.display_ops.pick_installer')
     @mock.patch('letsencrypt.cli.zope.component.getUtility')
     @mock.patch('letsencrypt.cli._init_le_client')
-    def test_certonly_csr(self, mock_init, mock_get_utility,
+    @mock.patch('letsencrypt.cli.record_chosen_plugins')
+    def test_certonly_csr(self, _rec, mock_init, mock_get_utility,
                           mock_pick_installer, mock_notAfter):
         cert_path = '/etc/letsencrypt/live/blahcert.pem'
         date = '1970-01-01'
@@ -378,11 +414,31 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
         self.assertTrue(
             date in mock_get_utility().add_message.call_args[0][0])
 
+    @mock.patch('letsencrypt.cli.client.acme_client')
+    def test_revoke_with_key(self, mock_acme_client):
+        server = 'foo.bar'
+        self._call_no_clientmock(['--cert-path', CERT, '--key-path', KEY,
+                                 '--server', server, 'revoke'])
+        with open(KEY) as f:
+            mock_acme_client.Client.assert_called_once_with(
+                server, key=jose.JWK.load(f.read()), net=mock.ANY)
+        with open(CERT) as f:
+            cert = crypto_util.pyopenssl_load_certificate(f.read())[0]
+            mock_revoke = mock_acme_client.Client().revoke
+            mock_revoke.assert_called_once_with(jose.ComparableX509(cert))
+
+    @mock.patch('letsencrypt.cli._determine_account')
+    def test_revoke_without_key(self, mock_determine_account):
+        mock_determine_account.return_value = (mock.MagicMock(), None)
+        _, _, _, client = self._call(['--cert-path', CERT, 'revoke'])
+        with open(CERT) as f:
+            cert = crypto_util.pyopenssl_load_certificate(f.read())[0]
+            mock_revoke = client.acme_from_config_key().revoke
+            mock_revoke.assert_called_once_with(jose.ComparableX509(cert))
+
     @mock.patch('letsencrypt.cli.sys')
     def test_handle_exception(self, mock_sys):
         # pylint: disable=protected-access
-        from letsencrypt import cli
-
         mock_open = mock.mock_open()
         with mock.patch('letsencrypt.cli.open', mock_open, create=True):
             exception = Exception('detail')
@@ -415,7 +471,6 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
             traceback.format_exception_only(KeyboardInterrupt, interrupt)))
 
     def test_read_file(self):
-        from letsencrypt import cli
         rel_test_path = os.path.relpath(os.path.join(self.tmp_dir, 'foo'))
         self.assertRaises(
             argparse.ArgumentTypeError, cli.read_file, rel_test_path)
@@ -547,8 +602,6 @@ class MockedVerb(object):
 
     """
     def __init__(self, verb_name):
-        from letsencrypt import cli
-
         self.verb_dict = cli.HelpfulArgumentParser.VERBS
         self.verb_func = None
         self.verb_name = verb_name
diff --git a/letsencrypt/tests/client_test.py b/letsencrypt/tests/client_test.py
index f8da90f36..d396e25bc 100644
--- a/letsencrypt/tests/client_test.py
+++ b/letsencrypt/tests/client_test.py
@@ -70,8 +70,8 @@ class ClientTest(unittest.TestCase):
                 dv_auth=None, installer=None)
 
     def test_init_acme_verify_ssl(self):
-        self.acme_client.assert_called_once_with(
-            directory=mock.ANY, key=mock.ANY, verify_ssl=True)
+        net = self.acme_client.call_args[1]["net"]
+        self.assertTrue(net.verify_ssl)
 
     def _mock_obtain_certificate(self):
         self.client.auth_handler = mock.MagicMock()