From 107851ee9b5d13e42e8e76e39ffcced418a7b0e1 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 13 Dec 2016 17:32:46 -0800 Subject: [PATCH] Document defaults (#3863) * Begin fixing incorrect defaults * Fix more defaults * Make more defaults correct * Update cli-help.txt (To show what this PR does) * Lint * Extend argparse rather than vendoring it * lint * Move sample User Agent generation into the same module as UA generation * Revert cli-help.txt to previous release version * Slightly more consistent linebreaks --- certbot/cli.py | 68 +++++++++++++++++++++++--------------- certbot/client.py | 14 ++++++-- certbot/interfaces.py | 2 +- certbot/plugins/manual.py | 2 +- certbot/plugins/webroot.py | 2 +- 5 files changed, 56 insertions(+), 32 deletions(-) diff --git a/certbot/cli.py b/certbot/cli.py index 259953f5e..ef6257304 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -300,6 +300,21 @@ class HelpfulArgumentGroup(object): """Add a new command line argument to the argument group.""" self._parser.add(self._topic, *args, **kwargs) +class CustomHelpFormatter(argparse.HelpFormatter): + """This is a clone of ArgumentDefaultsHelpFormatter, with bugfixes. + + In particular we fix https://bugs.python.org/issue28742 + """ + + def _get_help_string(self, action): + helpstr = action.help + if '%(default)' not in action.help and '(default:' not in action.help: + if action.default != argparse.SUPPRESS: + defaulting_nargs = [argparse.OPTIONAL, argparse.ZERO_OR_MORE] + if action.option_strings or action.nargs in defaulting_nargs: + helpstr += ' (default: %(default)s)' + return helpstr + # The attributes here are: # short: a string that will be displayed by "certbot -h commands" # opts: a string that heads the section of flags with which this command is documented, @@ -423,7 +438,7 @@ class HelpfulArgumentParser(object): self.parser = configargparse.ArgParser( prog="certbot", usage=short_usage, - formatter_class=argparse.ArgumentDefaultsHelpFormatter, + formatter_class=CustomHelpFormatter, args_for_setting_config_path=["-c", "--config"], default_config_files=flag_default("config_files"), config_arg_help_message="path to config file (default: {0})".format( @@ -793,7 +808,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis metavar="DOMAIN", action=_DomainsAction, default=[], help="Domain names to apply. For multiple domains you can use " "multiple -d flags or enter a comma separated list of domains " - "as a parameter.") + "as a parameter. (default: Ask)") helpful.add( [None, "run", "certonly", "manage"], "--cert-name", dest="certname", @@ -842,11 +857,11 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis dest="reinstall", action="store_true", help="If the requested cert matches an existing cert, always keep the " "existing one until it is due for renewal (for the " - "'run' subcommand this means reinstall the existing cert)") + "'run' subcommand this means reinstall the existing cert). (default: Ask)") helpful.add( "automation", "--expand", action="store_true", help="If an existing cert covers some subset of the requested names, " - "always expand and replace it with the additional names.") + "always expand and replace it with the additional names. (default: Ask)") helpful.add( "automation", "--version", action="version", version="%(prog)s {0}".format(certbot.__version__), @@ -875,7 +890,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis "at this system. This option cannot be used with --csr.") helpful.add( "automation", "--agree-tos", dest="tos", action="store_true", - help="Agree to the ACME Subscriber Agreement") + help="Agree to the ACME Subscriber Agreement (default: Ask)") helpful.add( "automation", "--account", metavar="ACCOUNT_ID", help="Account ID to use") @@ -889,7 +904,8 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis helpful.add( "automation", "--no-self-upgrade", action="store_true", help="(certbot-auto only) prevent the certbot-auto script from" - " upgrading itself to newer released versions") + " upgrading itself to newer released versions (default: Upgrade" + " automatically)") helpful.add( ["automation", "renew", "certonly", "run"], "-q", "--quiet", dest="quiet", action="store_true", @@ -928,11 +944,11 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis helpful.add( "security", "--redirect", action="store_true", help="Automatically redirect all HTTP traffic to HTTPS for the newly " - "authenticated vhost.", dest="redirect", default=None) + "authenticated vhost. (default: Ask)", dest="redirect", default=None) helpful.add( "security", "--no-redirect", action="store_false", help="Do not automatically redirect all HTTP traffic to HTTPS for the newly " - "authenticated vhost.", dest="redirect", default=None) + "authenticated vhost. (default: Ask)", dest="redirect", default=None) helpful.add( "security", "--hsts", action="store_true", help="Add the Strict-Transport-Security header to every HTTP response." @@ -940,8 +956,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis " Defends against SSL Stripping.", dest="hsts", default=False) helpful.add( "security", "--no-hsts", action="store_false", - help="Do not automatically add the Strict-Transport-Security header" - " to every HTTP response.", dest="hsts", default=False) + help=argparse.SUPPRESS, dest="hsts", default=False) helpful.add( "security", "--uir", action="store_true", help="Add the \"Content-Security-Policy: upgrade-insecure-requests\"" @@ -949,9 +964,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis " https:// for every http:// resource.", dest="uir", default=None) helpful.add( "security", "--no-uir", action="store_false", - help="Do not automatically set the \"Content-Security-Policy:" - " upgrade-insecure-requests\" header to every HTTP response.", - dest="uir", default=None) + help=argparse.SUPPRESS, dest="uir", default=None) helpful.add( "security", "--staple-ocsp", action="store_true", help="Enables OCSP Stapling. A valid OCSP response is stapled to" @@ -959,8 +972,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis dest="staple", default=None) helpful.add( "security", "--no-staple-ocsp", action="store_false", - help="Do not automatically enable OCSP Stapling.", - dest="staple", default=None) + help=argparse.SUPPRESS, dest="staple", default=None) helpful.add( "security", "--strict-permissions", action="store_true", help="Require that all configuration files are owned by the current " @@ -1005,7 +1017,8 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis " see if the programs being run are in the $PATH, so that mistakes can" " be caught early, even when the hooks aren't being run just yet. The" " validation is rather simplistic and fails if you use more advanced" - " shell constructs, so you can use this switch to disable it.") + " shell constructs, so you can use this switch to disable it." + " (default: False)") helpful.add_deprecated_argument("--agree-dev-preview", 0) helpful.add_deprecated_argument("--dialog", 0) @@ -1025,12 +1038,15 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis def _create_subparsers(helpful): helpful.add("config_changes", "--num", type=int, help="How many past revisions you want to be displayed") + + from certbot.client import sample_user_agent # avoid import loops 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 "".') + 'Encrypt server, set this to "". ' + '(default: {0})'.format(sample_user_agent())) helpful.add("certonly", "--csr", type=read_file, help="Path to a Certificate Signing Request (CSR) in DER or PEM format." @@ -1103,20 +1119,18 @@ def _plugins_parsing(helpful, plugins): "a particular plugin by setting options provided below. Running " "--help will list flags specific to that plugin.") - helpful.add( - "plugins", "-a", "--authenticator", help="Authenticator plugin name.") - helpful.add( - "plugins", "-i", "--installer", help="Installer plugin name (also used to find domains).") - helpful.add( - "plugins", "--configurator", help="Name of the plugin that is " - "both an authenticator and an installer. Should not be used " - "together with --authenticator or --installer.") + helpful.add("plugins", "--configurator", + help="Name of the plugin that is both an authenticator and an installer." + " Should not be used together with --authenticator or --installer. " + "(default: Ask)") + helpful.add("plugins", "-a", "--authenticator", help="Authenticator plugin name.") + helpful.add("plugins", "-i", "--installer", + help="Installer plugin name (also used to find domains).") helpful.add(["plugins", "certonly", "run", "install", "config_changes"], "--apache", action="store_true", help="Obtain and install certs using Apache") helpful.add(["plugins", "certonly", "run", "install", "config_changes"], - "--nginx", action="store_true", - help="Obtain and install certs using Nginx") + "--nginx", action="store_true", help="Obtain and install certs using Nginx") helpful.add(["plugins", "certonly"], "--standalone", action="store_true", help='Obtain certs using a "standalone" webserver.') helpful.add(["plugins", "certonly"], "--script", action="store_true", diff --git a/certbot/client.py b/certbot/client.py index d58f9457f..6ff14bc56 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -38,11 +38,11 @@ def acme_from_config_key(config, key): "Wrangle ACME client construction" # TODO: Allow for other alg types besides RS256 net = acme_client.ClientNetwork(key, verify_ssl=(not config.no_verify_ssl), - user_agent=_determine_user_agent(config)) + user_agent=determine_user_agent(config)) return acme_client.Client(config.server, key=key, net=net) -def _determine_user_agent(config): +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) @@ -59,6 +59,16 @@ def _determine_user_agent(config): ua = config.user_agent return ua +def sample_user_agent(): + "Document what this Certbot's user agent string will be like." + class DummyConfig(object): + "Shim for computing a sample user agent." + def __init__(self): + self.authenticator = "XXX" + self.installer = "YYY" + self.user_agent = None + return determine_user_agent(DummyConfig()) + def register(config, account_storage, tos_cb=None): """Register new account with an ACME CA. diff --git a/certbot/interfaces.py b/certbot/interfaces.py index 8e7d887f0..6388bf936 100644 --- a/certbot/interfaces.py +++ b/certbot/interfaces.py @@ -201,7 +201,7 @@ class IConfig(zope.interface.Interface): """ server = zope.interface.Attribute("ACME Directory Resource URI.") email = zope.interface.Attribute( - "Email used for registration and recovery contact.") + "Email used for registration and recovery contact. (default: Ask)") rsa_key_size = zope.interface.Attribute("Size of the RSA key.") must_staple = zope.interface.Attribute( "Adds the OCSP Must Staple extension to the certificate. " diff --git a/certbot/plugins/manual.py b/certbot/plugins/manual.py index c124ce048..5f933f8bc 100644 --- a/certbot/plugins/manual.py +++ b/certbot/plugins/manual.py @@ -98,7 +98,7 @@ s.serve_forever()" """ add("test-mode", action="store_true", help="Test mode. Executes the manual command in subprocess.") add("public-ip-logging-ok", action="store_true", - help="Automatically allows public IP logging.") + help="Automatically allows public IP logging. (default: Ask)") def prepare(self): # pylint: disable=missing-docstring,no-self-use if self.config.noninteractive_mode and not self.conf("test-mode"): diff --git a/certbot/plugins/webroot.py b/certbot/plugins/webroot.py index 2c449fdca..e9c3bcdda 100644 --- a/certbot/plugins/webroot.py +++ b/certbot/plugins/webroot.py @@ -45,7 +45,7 @@ to serve all files under specified web root ({0}).""" "times to handle different domains; each domain will have " "the webroot path that preceded it. For instance: `-w " "/var/www/example -d example.com -d www.example.com -w " - "/var/www/thing -d thing.net -d m.thing.net`") + "/var/www/thing -d thing.net -d m.thing.net` (default: Ask)") add("map", default={}, action=_WebrootMapAction, help="JSON dictionary mapping domains to webroot paths; this " "implies -d for each entry. You may need to escape this from "