Add option to specify revocation reason (#3242) (#3988)

This includes two new tests in the integration test script to check that
boulder gets the correct code. The encoding is specified in RFC5280
5.3.1. The codes that boulder will accept are a subset of that,
specified in `boulder.revocation.reasons.go`.

acme/acme/client.py

@@ -481,17 +481,21 @@ class Client(object):  # pylint: disable=too-many-instance-attributes
                 "Recursion limit reached. Didn't get {0}".format(uri))
         return chain
 
-    def revoke(self, cert):
+    def revoke(self, cert, rsn):
         """Revoke certificate.
 
         :param .ComparableX509 cert: `OpenSSL.crypto.X509` wrapped in
             `.ComparableX509`
 
+        :param int rsn: Reason code for certificate revocation.
+
         :raises .ClientError: If revocation is unsuccessful.
 
         """
         response = self.net.post(self.directory[messages.Revocation],
-                                 messages.Revocation(certificate=cert),
+                                 messages.Revocation(
+                                     certificate=cert,
+                                     reason=rsn),
                                  content_type=None)
         if response.status_code != http_client.OK:
             raise errors.ClientError(

acme/acme/client_test.py

@@ -81,6 +81,9 @@ class ClientTest(unittest.TestCase):
             uri='https://www.letsencrypt-demo.org/acme/cert/1',
             cert_chain_uri='https://www.letsencrypt-demo.org/ca')
 
+        # Reason code for revocation
+        self.rsn = 1
+
     def test_init_downloads_directory(self):
         uri = 'http://www.letsencrypt-demo.org/directory'
         from acme.client import Client
@@ -427,13 +430,22 @@ class ClientTest(unittest.TestCase):
         self.assertRaises(errors.Error, self.client.fetch_chain, self.certr)
 
     def test_revoke(self):
-        self.client.revoke(self.certr.body)
+        self.client.revoke(self.certr.body, self.rsn)
         self.net.post.assert_called_once_with(
             self.directory[messages.Revocation], mock.ANY, content_type=None)
 
+    def test_revocation_payload(self):
+        obj = messages.Revocation(certificate=self.certr.body, reason=self.rsn)
+        self.assertTrue('reason' in obj.to_partial_json().keys())
+        self.assertEquals(self.rsn, obj.to_partial_json()['reason'])
+
     def test_revoke_bad_status_raises_error(self):
         self.response.status_code = http_client.METHOD_NOT_ALLOWED
-        self.assertRaises(errors.ClientError, self.client.revoke, self.certr)
+        self.assertRaises(
+            errors.ClientError,
+            self.client.revoke,
+            self.certr,
+            self.rsn)
 
 
 class ClientNetworkTest(unittest.TestCase):

acme/acme/messages.py

@@ -469,3 +469,4 @@ class Revocation(jose.JSONObjectWithFields):
     resource = fields.Resource(resource_type)
     certificate = jose.Field(
         'certificate', decoder=jose.decode_cert, encoder=jose.encode_cert)
+    reason = jose.Field('reason')

certbot/cli.py

@@ -1072,6 +1072,11 @@ def _create_subparsers(helpful):
                 "--csr", type=read_file,
                 help="Path to a Certificate Signing Request (CSR) in DER or PEM format."
                 " Currently --csr only works with the 'certonly' subcommand.")
+    helpful.add("revoke",
+                "--reason", dest="reason",
+                choices=CaseInsensitiveList(constants.REVOCATION_REASONS.keys()),
+                action=_EncodeReasonAction, default=0,
+                help="Specify reason for revoking certificate.")
     helpful.add("rollback",
                 "--checkpoints", type=int, metavar="N",
                 default=flag_default("rollback_checkpoints"),
@@ -1088,6 +1093,16 @@ def _create_subparsers(helpful):
                 const=interfaces.IInstaller, help="Limit to installer plugins only.")
 
 
+class CaseInsensitiveList(list):
+    """A list that will ignore case when searching.
+
+    This class is passed to the `choices` argument of `argparse.add_arguments`
+    through the `helpful` wrapper. It is necessary due to special handling of
+    command line arguments by `set_by_cli` in which the `type_func` is not applied."""
+    def __contains__(self, element):
+        return super(CaseInsensitiveList, self).__contains__(element.lower())
+
+
 def _paths_parser(helpful):
     add = helpful.add
     verb = helpful.verb
@@ -1166,6 +1181,15 @@ def _plugins_parsing(helpful, plugins):
     helpful.add_plugin_args(plugins)
 
 
+class _EncodeReasonAction(argparse.Action):
+    """Action class for parsing revocation reason."""
+
+    def __call__(self, parser, namespace, reason, option_string=None):
+        """Encodes the reason for certificate revocation."""
+        code = constants.REVOCATION_REASONS[reason.lower()]
+        setattr(namespace, self.dest, code)
+
+
 class _DomainsAction(argparse.Action):
     """Action class for parsing domains."""
 

certbot/constants.py

@@ -35,6 +35,17 @@ CLI_DEFAULTS = dict(
 )
 STAGING_URI = "https://acme-staging.api.letsencrypt.org/directory"
 
+# The set of reasons for revoking a certificate is defined in RFC 5280 in
+# section 5.3.1. The reasons that users are allowed to submit are restricted to
+# those accepted by the ACME server implementation. They are listed in
+# `letsencrypt.boulder.revocation.reasons.go`.
+REVOCATION_REASONS = {
+    "unspecified": 0,
+    "keycompromise": 1,
+    "affiliationchanged": 3,
+    "superseded": 4,
+    "cessationofoperation": 5}
+
 """Defaults for CLI flags and `.IConfig` attributes."""
 
 QUIET_LOGGING_LEVEL = logging.WARNING

certbot/main.py

@@ -550,8 +550,10 @@ def revoke(config, unused_plugins):  # TODO: coop with renewal config
         key = acc.key
     acme = client.acme_from_config_key(config, key)
     cert = crypto_util.pyopenssl_load_certificate(config.cert_path[1])[0]
+    logger.debug("Reason code for revocation: %s", config.reason)
+
     try:
-        acme.revoke(jose.ComparableX509(cert))
+        acme.revoke(jose.ComparableX509(cert), config.reason)
     except acme_errors.ClientError as e:
         return e.message
 

certbot/tests/cli_test.py

@@ -121,6 +121,7 @@ class ParseTest(unittest.TestCase):
         out = self._help_output(['--help', 'revoke'])
         self.assertTrue("--cert-path" in out)
         self.assertTrue("--key-path" in out)
+        self.assertTrue("--reason" in out)
 
         out = self._help_output(['-h', 'config_changes'])
         self.assertTrue("--cert-path" not in out)
@@ -262,6 +263,14 @@ class ParseTest(unittest.TestCase):
         self.assertFalse(cli.option_was_set(
             config_dir_option, cli.flag_default(config_dir_option)))
 
+    def test_encode_revocation_reason(self):
+        for reason, code in constants.REVOCATION_REASONS.items():
+            namespace = self.parse(['--reason', reason])
+            self.assertEqual(namespace.reason, code)
+        for reason, code in constants.REVOCATION_REASONS.items():
+            namespace = self.parse(['--reason', reason.upper()])
+            self.assertEqual(namespace.reason, code)
+
     def test_force_interactive(self):
         self.assertRaises(
             errors.Error, self.parse, "renew --force-interactive".split())

certbot/tests/main_test.py

@@ -215,7 +215,7 @@ class RevokeTest(unittest.TestCase):
             'cert.pem'))
 
         self.patches = [
-            mock.patch('acme.client.Client'),
+            mock.patch('acme.client.Client', autospec=True),
             mock.patch('certbot.client.Client'),
             mock.patch('certbot.main._determine_account'),
             mock.patch('certbot.main.display_ops.success_revocation')
@@ -241,8 +241,9 @@ class RevokeTest(unittest.TestCase):
         for patch in self.patches:
             patch.stop()
 
-    def _call(self):
+    def _call(self, extra_args=""):
-        args = 'revoke --cert-path={0}'.format(self.tmp_cert_path).split()
+        args = 'revoke --cert-path={0} ' + extra_args
+        args = args.format(self.tmp_cert_path).split()
         plugins = disco.PluginsRegistry.find_all()
         config = configuration.NamespaceConfig(
             cli.prepare_and_parse_args(plugins, args))
@@ -250,6 +251,17 @@ class RevokeTest(unittest.TestCase):
         from certbot.main import revoke
         revoke(config, plugins)
 
+    @mock.patch('certbot.main.client.acme_client')
+    def test_revoke_with_reason(self, mock_acme_client):
+        mock_revoke = mock_acme_client.Client().revoke
+        expected = []
+        for reason, code in constants.REVOCATION_REASONS.items():
+            self._call("--reason " + reason)
+            expected.append(mock.call(mock.ANY, code))
+            self._call("--reason " + reason.upper())
+            expected.append(mock.call(mock.ANY, code))
+        self.assertEqual(expected, mock_revoke.call_args_list)
+
     def test_revocation_success(self):
         self._call()
         self.mock_success_revoke.assert_called_once_with(self.tmp_cert_path)
@@ -1062,7 +1074,9 @@ class MainTest(unittest.TestCase):  # pylint: disable=too-many-public-methods
         with open(CERT, 'rb') 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_revoke.assert_called_once_with(
+                    jose.ComparableX509(cert),
+                    mock.ANY)
 
     @mock.patch('certbot.main._determine_account')
     def test_revoke_without_key(self, mock_determine_account):
@@ -1071,7 +1085,9 @@ class MainTest(unittest.TestCase):  # pylint: disable=too-many-public-methods
         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_revoke.assert_called_once_with(
+                    jose.ComparableX509(cert),
+                    mock.ANY)
 
     def test_agree_dev_preview_config(self):
         with mock.patch('certbot.main.run') as mocked_run:

tests/boulder-integration.sh

@@ -162,7 +162,14 @@ common revoke --cert-path "$root/conf/live/le1.wtf/cert.pem"
 # revoke by cert key
 common revoke --cert-path "$root/conf/live/le2.wtf/cert.pem" \
        --key-path "$root/conf/live/le2.wtf/privkey.pem"
-
+# Get new certs to test revoke with a reason, by account and by cert key
+common --domains le1.wtf
+common revoke --cert-path "$root/conf/live/le1.wtf/cert.pem" \
+    --reason cessationOfOperation
+common --domains le2.wtf
+common revoke --cert-path "$root/conf/live/le2.wtf/cert.pem" \
+    --key-path "$root/conf/live/le2.wtf/privkey.pem" \
+    --reason keyCompromise
 if type nginx;
 then
     . ./certbot-nginx/tests/boulder-integration.sh