From 94c23479e21d0387d3718639f1fb755c8895ca7b Mon Sep 17 00:00:00 2001 From: Craig Smith Date: Thu, 12 Jan 2017 12:56:55 +1030 Subject: [PATCH] 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 | 8 ++++++-- acme/acme/client_test.py | 16 ++++++++++++++-- acme/acme/messages.py | 1 + certbot/cli.py | 24 ++++++++++++++++++++++++ certbot/constants.py | 11 +++++++++++ certbot/main.py | 4 +++- certbot/tests/cli_test.py | 9 +++++++++ certbot/tests/main_test.py | 26 +++++++++++++++++++++----- tests/boulder-integration.sh | 9 ++++++++- 9 files changed, 97 insertions(+), 11 deletions(-) diff --git a/acme/acme/client.py b/acme/acme/client.py index b5db57235..26109352b 100644 --- a/acme/acme/client.py +++ b/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( diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index e0403ef28..4822a1ae6 100644 --- a/acme/acme/client_test.py +++ b/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): diff --git a/acme/acme/messages.py b/acme/acme/messages.py index a7c86a10c..29d719684 100644 --- a/acme/acme/messages.py +++ b/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') diff --git a/certbot/cli.py b/certbot/cli.py index 31b5bafb5..d51fd58e0 100644 --- a/certbot/cli.py +++ b/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.""" diff --git a/certbot/constants.py b/certbot/constants.py index 7d713d29f..f64ff7e1e 100644 --- a/certbot/constants.py +++ b/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 diff --git a/certbot/main.py b/certbot/main.py index d076fe4bc..ec901c501 100644 --- a/certbot/main.py +++ b/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 diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index 9404a8385..32aada811 100644 --- a/certbot/tests/cli_test.py +++ b/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()) diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 99aa0bdda..01ec2f061 100644 --- a/certbot/tests/main_test.py +++ b/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): - args = 'revoke --cert-path={0}'.format(self.tmp_cert_path).split() + def _call(self, extra_args=""): + 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: diff --git a/tests/boulder-integration.sh b/tests/boulder-integration.sh index fe2f223ef..30fc17f81 100755 --- a/tests/boulder-integration.sh +++ b/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