Implement the --cert-name flag to select a lineage by its name. #3785

ohemorange · 2016-11-11T23:33:31Z

Step 3 of #3615.

ohemorange · 2016-11-11T23:33:36Z

Pushing to github to see missing comment updates and test coverage, not ready for review.

coveralls · 2016-11-11T23:49:34Z

Coverage decreased (-0.2%) to 98.585% when pulling c23d31c on certname-flag into 1dd1afd on master.

coveralls · 2016-11-12T00:50:40Z

Coverage decreased (-0.1%) to 98.644% when pulling 22642bc on certname-flag into 1dd1afd on master.

coveralls · 2016-11-22T19:42:36Z

Coverage decreased (-0.1%) to 98.699% when pulling ebbeaf5 on certname-flag into 908e8a8 on master.

…rts with the lineage name

ohemorange · 2016-11-29T20:05:04Z

Update: this also contains a --new-cert-name flag to go along with a rename command. I'm starting to think that it may be appropriate to move verbs like rename, certificates, update_symlinks, and revoke to an interactive cert manager, but that is definitely not appropriate for this PR.

Once coverage and tests are happy I expect this to be ready for review.

coveralls · 2016-11-29T20:10:51Z

Coverage increased (+0.07%) to 98.921% when pulling da6a477 on certname-flag into 908e8a8 on master.

coveralls · 2016-11-29T21:01:39Z

Coverage increased (+0.07%) to 98.921% when pulling a63004a on certname-flag into 7951ba7 on master.

coveralls · 2016-11-30T01:36:01Z

Coverage increased (+0.07%) to 98.914% when pulling da05230 on certname-flag into 83966cd on master.

schoen · 2016-11-30T19:47:46Z

Test 1: I have an --apache lineage called tomarbanhoeruim.loyalty.org and I ran

certbot certonly --cert-name tomarbanhoeruim.loyalty.org --force-renew

But I was interactively prompted for an authenticator, which I didn't expect.

I then tried

certbot run --cert-name tomarbanhoeruim.loyalty.org --force-renew

In this case the renewal appeared to work, but I got mysterious behavior on the install attempt:

Performing the following challenges:
tls-sni-01 challenge for tomarbanhoeruim.loyalty.org
Waiting for verification...
Cleaning up challenges
Generating key (2048 bits): /etc/letsencrypt/keys/0016_key-certbot.pem
Creating CSR: /etc/letsencrypt/csr/0016_csr-certbot.pem
An unexpected error occurred:
TypeError: 'NoneType' object is not iterable
Please see the logfiles in /var/log/letsencrypt for more details.

IMPORTANT NOTES:
 - Congratulations! Your certificate and chain have been saved at
   /etc/letsencrypt/live/tomarbanhoeruim.loyalty.org/fullchain.pem.

(Also not sure why I can get "Congratulations" with the error: I guess because auth worked but install didn't?)

2016-11-30 19:40:39,374:DEBUG:certbot.main:Exiting abnormally:
Traceback (most recent call last):
  File "/tmp/certbot/venv/bin/certbot", line 11, in <module>
    load_entry_point('certbot', 'console_scripts', 'certbot')()
  File "/tmp/certbot/certbot/main.py", line 806, in main
    return config.func(config, plugins)
  File "/tmp/certbot/certbot/main.py", line 555, in run
    lineage.chain, lineage.fullchain)
  File "/tmp/certbot/certbot/client.py", line 371, in deploy_certificate
    for dom in domains:
TypeError: 'NoneType' object is not iterable

Can any of these behaviors be attributed to this PR? I can try to run the same commands with an older branch for comparison.

schoen

These are all textual/documentation related but I also wonder if you can repeat my tests in an environment of your own and see if you get the same problems, or if it succeeds for you.

schoen · 2016-11-30T18:39:22Z

certbot/cert_manager.py

+
+    """
+    if not config.certname:
+        raise errors.ConfigurationError("Specify a certificate name with "


I think this might be clearer as "Specify an existing certificate name with" (for parallelism with the other message).

schoen · 2016-11-30T18:48:26Z

certbot/cli.py

+        "--cert-name", dest="certname",
+        metavar="CERTNAME", default=None,
+        help="Certificate name to apply. Only one certificate name can be used "
+             "per Certbot run. Show certificate names by running certificates "


I think the switch between addressees of the imperative verbs here ("Show certificate names" and "create a new certificate") will confuse readers, because one is saying "[you can] show certificate names by..." and the other is saying "[the program will] create a new certificate...".

Could we change the text for the "Show certificate names" sentence to something like

(You can see certificate names by running certificates command.)

or

(To see certificate names, run certificates command.)

or

(Names can be found with the certificates command.)?

Yes and maybe also better to say something like by running "certbot certificates" or by running cerbot with the "certificates" subcommand?

Good call. I like To see certificate names, run "certbot certificates".

schoen · 2016-11-30T18:50:12Z

certbot/cert_manager.py

+            logger.debug("Traceback was:\n%s", traceback.format_exc())
+            continue
+        rv = func(candidate_lineage, rv)
+    return rv


I was originally wondering about the case where there's more than one matching lineage and whether we'd simply get func() of the last one, but now I see that since the intermediate rv is an argument to func, someone writing a func could also choose to have it accumulate values as it goes, like in a list, if they want information about multiple lineages. So, nice, flexible design there.

Flexibility is my goal! I'll put this in the docstring to make it clear.

schoen · 2016-11-30T18:51:44Z

certbot/cli.py

+        "rename",
+        "--new-cert-name", dest="new_certname",
+        metavar="NEW_CERTNAME", default=None,
+        help="New name for the certificate. Must be a valid filename.")


Is it valid to use only --new-cert-name with a new lineage, instead of --cert-name? (Are they synonyms when there is no existing lineage with the specified name?) I can definitely imagine some users mentally parsing this as (new-cert)-name instead of new-(cert-name) and so trying to use it in that case.

Oh good point. Maybe --updated-cert-name is better? Or just --new-certname and then also call it --certname in other places? I don't want to make this flag for anything other than setting a new cert name.

schoen · 2016-11-30T18:52:13Z

certbot/cli.py

+        "automation", "--renew-with-new-domains",
+        action="store_true", dest="renew_with_new_domains", help="If a "
+             "certificate already exists for the requested certificate name "
+             "but does not match the request domains, renew it now, "


should probably be "requested domains" here

So it should.

schoen · 2016-11-30T18:54:23Z

certbot/main.py

@@ -172,7 +172,7 @@ def _handle_identical_cert_request(config, lineage):
        return "reinstall", lineage
    question = (
        "You have an existing certificate that contains exactly the same "
-        "domains you requested and isn't close to expiry."
+        "domains or certificate name you requested and isn't close to expiry."


Maybe this would be clearer as "has exactly the same domains or certificate name", since people might not think of the certificate as "containing" its own certificate name? (At least knowing the implementation details, I don't.)

If you're feeling fancy, provide a question whose wording varies depending on whether the user actually provided domains or a certificate name (or if you agree that's a good idea but don't want to do it in this PR, open a new ticket for it).

I'll put it to has for now.

schoen · 2016-11-30T18:56:32Z

certbot/main.py

+    """
+    if config.renew_with_new_domains:
+        return
+    msg = ("Confirm that you intend to update certificate {0} "


Would it be helpful or unhelpful to put quotes around the lineage name when displaying it?

I have no opinion about this. Let me know if you want me to change it.

I guess I'm also not sure, so I won't suggest a change.

schoen · 2016-11-30T18:57:50Z

certbot/storage.py

@@ -96,6 +96,25 @@ def write_renewal_config(o_filename, n_filename, archive_dir, target, relevant_d
        config.write(outfile=f)
    return config

+def rename_renewal_config(prev_name, new_name, cli_config):
+    """Rename's cli_config.certname's config to cli_config.new_certname.


should be "Renames"

pde

A couple of changes requested to use the IDisplay mechanisms for [non]interactivity in standard ways, plus a small change in storage, plus some nits/optional suggestions.

pde · 2016-11-30T22:29:48Z

certbot/main.py

+               new_domains,
+               old_domains))
+    obj = zope.component.getUtility(interfaces.IDisplay)
+    if not obj.yesno(msg, "Update cert", "Cancel"):


Thanks to @PaulSD for noticing this: you should add default=True to this obj.yesno() call, so that this path can be run non-interactively.

I had added a cli flag because I didn't know about this default=True thing. It seems like we usually have both a noninteractive option and a cli flag, is that right? Or should I remove the cli flag given that it can be run noninteractively?

Good question. In most cases the CLI flags of that sort (like --keep-until-expiring) predated the -n flag, so we've never asked ourselves that question!

Argument 1: -n does the job, so let's just make everyone use it if they want non-interactive execution

Argument 2: the user might want to be asked about --domains interactively, but not this update confirm thing, so they might want to provide the flag for "Update" on the command line (or put it in /etc/letsencrypt/cli.ini) while still running interactively.

Which do we find more persuasive?

Let's keep both, because a user might not know about non-interactive mode and just look for the specific command-line flag they want.

While I don't feel strongly, I think it is the user's responsibility to look at the documentation of the available flags more closely if (s)he doesn't immediately find what (s)he needs.

Furthermore, if you're running in interactive mode, I don't think it is so bad to get an extra question; in other words, I don't think there needs to be a switch to mute specific aspects of the interactive mode. (The number of questions is low anyways.) It's a trade-off with code simplicity, which I also consider valuable.

pde · 2016-11-30T22:31:32Z

certbot/cert_manager.py

+        raise errors.ConfigurationError("Specify a certificate name with "
+            "with flag --cert-name.")
+    if not config.new_certname:
+        raise errors.ConfigurationError("Specify a new name for certificate {0} "


This should be errors.MissingCommandlineFlag.

But even better would be to do something like calling disp.input("Enter the new name for certificate {0}".format(config.certname), flag="--new-cert-name"). That will raise a nicely formatted errors.MissingCommandlineFlag for you.

ah! I didn't know we had that but I like it!

In addition, the previous check can probably provide a list of cert names.

ok it now does that.

pde · 2016-11-30T22:39:39Z

certbot/cert_manager.py

+def lineage_for_certname(config, certname):
+    """Find a lineage object with name certname.
+    """
+    def func(candidate_lineage, rv):


Style nit: Maybe nicer to call this matches_certname or certname_match_fn or something?

This function returns a lineage object for a given certname. Is it that the name is too long?

My nit was suggesting something slightly more descriptive instead of func. But feel free to ignore this suggestion!

Oh that makes more sense! I can do that.

pde · 2016-11-30T22:41:16Z

certbot/cli.py

+        "--cert-name", dest="certname",
+        metavar="CERTNAME", default=None,
+        help="Certificate name to apply. Only one certificate name can be used "
+             "per Certbot run. Show certificate names by running certificates "


Yes and maybe also better to say something like by running "certbot certificates" or by running cerbot with the "certificates" subcommand?

pde · 2016-11-30T22:42:30Z

certbot/main.py

@@ -68,17 +70,17 @@ def _report_successful_dry_run(config):
                                  reporter_util.HIGH_PRIORITY, on_crash=False)


-def _auth_from_domains(le_client, config, domains, lineage=None):
+def _auth_from_available(le_client, config, domains=None, certname=None, lineage=None):
    """Authenticate and enroll certificate.


Maybe we should document what is going on here a little bit more?

Ha yes. Especially given that this is the primary main authentication method, as far as I can tell.

Yup; it was already mysterious, and is becoming more so :)

How do you feel about the current documentation?

pde · 2016-11-30T22:43:59Z

certbot/main.py

@@ -172,7 +172,7 @@ def _handle_identical_cert_request(config, lineage):
        return "reinstall", lineage
    question = (
        "You have an existing certificate that contains exactly the same "
-        "domains you requested and isn't close to expiry."
+        "domains or certificate name you requested and isn't close to expiry."


If you're feeling fancy, provide a question whose wording varies depending on whether the user actually provided domains or a certificate name (or if you agree that's a good idea but don't want to do it in this PR, open a new ticket for it).

pde · 2016-11-30T22:47:47Z

certbot/storage.py

+        cli_config.renewal_configs_dir, prev_name) + ".conf"
+    new_filename = os.path.join(
+        cli_config.renewal_configs_dir, new_name) + ".conf"
+    if os.path.isfile(new_filename):


I think this should probably be os.path.exists(new_filename), so that it errors on directories, links, etc.

coveralls · 2016-12-01T02:14:49Z

Coverage increased (+0.05%) to 98.894% when pulling 4374876 on certname-flag into 83966cd on master.

schoen · 2016-12-01T18:04:05Z

@ohemorange, yes, I'll try getting a new lineage with master, then running the closest equivalents to those, then running those with this branch, and see if the problem recurs or not.

ohemorange · 2016-12-01T18:24:36Z

certbot/tests/cert_manager_test.py

+        self.assertTrue(mock_renewer_config)
+
+
+class RenameLineageTest(storage_test.BaseRenewableCertTest):


There's a BaseCertManagerTest that will work here, why not use that?

What a good idea, I should do that!

coveralls · 2016-12-01T18:48:53Z

Coverage increased (+0.07%) to 98.912% when pulling 78d9cf8 on certname-flag into 83966cd on master.

coveralls · 2016-12-01T19:08:55Z

Coverage increased (+0.07%) to 98.912% when pulling 4675186 on certname-flag into 83966cd on master.

coveralls · 2016-12-02T23:01:34Z

Coverage increased (+0.07%) to 98.912% when pulling 2c87cbb on certname-flag into 83966cd on master.

…to certname-flag

coveralls · 2016-12-06T00:49:31Z

Coverage increased (+0.07%) to 98.923% when pulling e6e3261 on certname-flag into 65d9e99 on master.

ohemorange · 2016-12-06T22:30:13Z

Make coveralls report back?

coveralls · 2016-12-06T22:51:51Z

Coverage increased (+0.07%) to 98.925% when pulling f56a11d on certname-flag into 184d673 on master.

coveralls · 2016-12-07T00:10:48Z

Coverage increased (+0.07%) to 98.925% when pulling f56a11d on certname-flag into 184d673 on master.

ohemorange added 5 commits November 9, 2016 00:25

Rename and simplify main functions

ce9305f

pass certname to auth method

995eae8

Merge branch 'master' into certname-flag

c6853e1

find cert by certname flag

52206a9

Implement --cert-name command

c23d31c

ohemorange added the area: cert management label Nov 11, 2016

ohemorange added this to the 0.10.0 milestone Nov 11, 2016

more coverage

22642bc

pde assigned schoen Nov 15, 2016

ohemorange added 2 commits November 22, 2016 18:41

Merge branch 'master' into certname-flag

5426721

update based on change to master

ebbeaf5

PaulSD mentioned this pull request Nov 26, 2016

Cert Management Improvement Project (C-MIP) #3615

Closed

ohemorange added 3 commits November 28, 2016 16:10

add test coverage for new main methods (unlinted)

60bb060

don't ask to confirm new cert when we have domains and no existing ce…

e395f7a

…rts with the lineage name

Refactor and add --new-cert-name flag

da6a477

ohemorange assigned bmw Nov 29, 2016

Merge branch 'master' into certname-flag

a63004a

ohemorange mentioned this pull request Nov 29, 2016

[changes requested] Refactor cli_test.py and main_test.py #3828

Merged

Merge branch 'master' into certname-flag

da05230

schoen suggested changes Nov 30, 2016

View reviewed changes

pde suggested changes Nov 30, 2016

View reviewed changes

Update docs, cli flags, and error messages

4374876

schoen approved these changes Dec 1, 2016

View reviewed changes

ohemorange commented Dec 1, 2016

View reviewed changes

fix no certname test

78d9cf8

use the handy dandy BaseCertManagerTest

4675186

xrange --> range for py3

2c87cbb

ohemorange added 2 commits December 6, 2016 00:38

Merge branch 'master' into certname-flag

33c2a23

Merge branch 'certname-flag' of https://github.com/certbot/certbot in…

e6e3261

…to certname-flag

ohemorange added 3 commits December 6, 2016 20:07

Merge branch 'master' into certname-flag

2bfddcb

allow noninteractive and more descriptive function names

4562d78

oneline some doctrings

f56a11d

ohemorange closed this Dec 6, 2016

ohemorange reopened this Dec 6, 2016

ohemorange added 2 commits December 7, 2016 23:53

unit test coverage

ab01927

Merge branch 'master' into certname-flag

41aecf5

pde approved these changes Dec 8, 2016

View reviewed changes

pde assigned ohemorange and unassigned schoen and bmw Dec 8, 2016

ohemorange merged commit 0a7ca2f into master Dec 8, 2016

ohemorange deleted the certname-flag branch December 8, 2016 19:53

		self.assertTrue(mock_renewer_config)


		class RenameLineageTest(storage_test.BaseRenewableCertTest):

Implement the --cert-name flag to select a lineage by its name. #3785

Implement the --cert-name flag to select a lineage by its name. #3785

Conversation

ohemorange commented Nov 11, 2016

ohemorange commented Nov 11, 2016

coveralls commented Nov 11, 2016

coveralls commented Nov 12, 2016

coveralls commented Nov 22, 2016

ohemorange commented Nov 29, 2016

coveralls commented Nov 29, 2016

coveralls commented Nov 29, 2016

coveralls commented Nov 30, 2016

schoen commented Nov 30, 2016

schoen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterthomassen Dec 3, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 1, 2016

schoen commented Dec 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 1, 2016

coveralls commented Dec 1, 2016

coveralls commented Dec 2, 2016

coveralls commented Dec 6, 2016

ohemorange commented Dec 6, 2016

coveralls commented Dec 6, 2016

coveralls commented Dec 7, 2016

peterthomassen Dec 3, 2016 •

edited