Apache plugin wildcard support for ACMEv2 #5608

joohoi · 2018-02-22T15:08:59Z

Functional example for wildcard certificate support in Apache plugin.

In deploy_cert() and enhance(), the user will be presented with a dialog to choose from the VirtualHosts that can be covered by the wildcard domain name. The (multiple) selection result will then be handled in a similar way that we previously handled a single VirtualHost that was returned by the _find_best_vhost().

Additionally the selected VirtualHosts are added to a dictionary that maps selections to a wildcard domain to be reused in the later enhance() call and not forcing the user to select the same VirtualHosts again.

The code here should be almost completely compatible with Nginx plugin as well, so when we get this merged, we can bring the changes over to Nginx plugin also.

ohemorange · 2018-02-23T23:35:41Z

certbot-apache/certbot_apache/configurator.py

@@ -1377,7 +1478,18 @@ def enhance(self, domain, enhancement, options=None):
            raise errors.PluginError(
                "Unsupported enhancement: {0}".format(enhancement))
        try:
-            func(self.choose_vhost(domain), options)
+            if self.wildcard_domain(domain):


can we refactor this so there's less inside the try block?

ohemorange · 2018-02-24T00:11:13Z

certbot-apache/certbot_apache/configurator.py

+            for vhost in deploy_vhosts:
+                self._deploy_cert(vhost, cert_path, key_path,
+                                  chain_path, fullchain_path)
+                if domain not in self.wildcard_vhosts.keys():


don't think you need .keys() here

also, why not do this inside choose_vhosts_wildcard?

ohemorange · 2018-02-24T00:11:49Z

certbot-apache/certbot_apache/configurator.py

+            return fnmatch.fnmatch(name, domain)
+
+
+    def choose_vhosts_wildcard(self, domain, create_ssl=True):


this can probably be internal

ohemorange · 2018-02-24T00:16:17Z

certbot-apache/certbot_apache/configurator.py

+                    filtered_vhosts[name] = vhost
+                elif name not in filtered_vhosts.keys():
+                    # Add if not in list previously
+                    filtered_vhosts[name] = vhost


what happens if there aren't any? do we want to create a new one?

Yeah, we do create one when appending the vhosts to the final list in the method.

ohemorange · 2018-02-24T00:20:20Z

certbot-apache/certbot_apache/configurator.py

+            vhost = self.choose_vhost(domain)
+            self._deploy_cert(vhost, cert_path, key_path, chain_path, fullchain_path)
+
+    def vhosts_for_wildcard(self, domain):


also can be internal?

ohemorange · 2018-02-24T00:25:41Z

certbot-apache/certbot_apache/configurator.py

-            func(self.choose_vhost(domain), options)
+            if self.wildcard_domain(domain):
+                # Handle virtualhosts configured for a wildcard domain
+                if domain in self.wildcard_vhosts.keys():


can this if (if domain in self.wildcard_vhosts:) go inside choose_vhosts_wildcard?

oh yeah, if both this and the other thing go into the function, that cleans everything else up a bunch!

these are my thoughts as I do the nginx one, it may be clearer to look at the PR I put up for that (#5619)

ohemorange · 2018-02-24T00:35:13Z

What happens if apache is authenticator and it's given a wildcard domain?

ohemorange · 2018-02-24T00:51:00Z

certbot-apache/certbot_apache/configurator.py

@@ -152,6 +153,9 @@ def __init__(self, *args, **kwargs):
        self.assoc = dict()
        # Outstanding challenges
        self._chall_out = set()
+        # List of vhosts configured per wildcard domain on this run.
+        # used by deploy_cert() and enhance()
+        self.wildcard_vhosts = dict()


underscores everywhere

joohoi · 2018-02-27T18:24:37Z

What happens if apache is authenticator and it's given a wildcard domain?

We had a short discussion about this yesterday, but Certbot should raise an exception when Apache plugin is connecting to ACME server that doesn't allow wildcard challenges over HTTP or TLS-SNI challenges.

All the review comments should now be addressed, and this is ready for the next round.

bmw

This is great @joohoi.

Other than the minor inline comments, when testing this I created a bunch of vhosts but forgot to enable them and when I ran Certbot, it obtained the cert but Apache deploy_cert and enhance just quietly did nothing (because there were no matching vhosts). Took me a few to figure out what was going on.

Any reason not to do what we currently do for single vhosts? If there's no matching vhosts (either because one doesn't exist or the user doesn't select one), we error out telling them to create a vhost. This seems preferable to me.

bmw · 2018-02-28T03:57:58Z

certbot/tests/display/ops_test.py

@@ -300,7 +300,7 @@ def test_get_valid_domains(self):
        from certbot.display.ops import get_valid_domains
        all_valid = ["example.com", "second.example.com",
                     "also.example.com", "under_score.example.com",
-                     "justtld", "*.wildcard.com"]
+                     "justtld"]


Whoops. I think this was caused by me also making changes to this file in another PR, but we shouldn't remove the wildcard from the list of valid domains here.

You are right. This was removed at the early phase of the development, and is now brought back.

bmw · 2018-02-28T04:14:56Z

certbot-apache/certbot_apache/configurator.py

+        for vhost in vhosts:
+            self._deploy_cert(vhost, cert_path, key_path, chain_path, fullchain_path)
+
+    def choose_vhosts(self, domain, cached=False, create_if_no_ssl=True):


I don't care too much, but do we need the cached parameter? Do we ever expect there to be a case where both of the following are true:

We have cached vhosts for a domain.

We don't want to use them.

If this doesn't seem likely, maybe we can simplify things slightly and remove this parameter.

This is correct, we shouldn't hit such scenario. This parameter is now removed.

bmw · 2018-02-28T04:18:58Z

certbot-apache/certbot_apache/configurator.py

+            if not vhost.ssl and create_ssl:
+                return_vhosts.append(self.make_vhost_ssl(vhost))
+            else:
+                return_vhosts.append(vhost)


Assuming the only use case for calling this function with create_ssl=False is for enhance, I get not wanting create an SSL copy in this case, but I'm not sure we should ever be returning HTTP vhosts from this function. Which of our enhancements make sense when you don't have an SSL vhost for a domain?

Instead, maybe if create_ssl is False, we should remove HTTP vhosts from dialog_input before asking for user input.

This is a more sophisticated way to handle it, I changed the behavior accordingly. This also helps to reduce the static from the dialog in the first place.

bmw · 2018-02-28T04:22:23Z

certbot-apache/certbot_apache/configurator.py

+        not in scope: domain = *.wild.card, name = 1.2.wild.card
+        """
+        if len(name.split(".")) == len(domain.split(".")):
+            return fnmatch.fnmatch(name, domain)


This works for wildcards in ServerAlias directives too right? Very nice.

Yeah, vhost.get_names() (which is basically what's fed to this method) returns all the names associated to the VirtualHost, both ServerNames and ServerAlias. Furthermore, I don't think wildcard names are allowed in the ServerName in the first place.

bmw · 2018-02-28T04:30:24Z

certbot-apache/certbot_apache/display_ops.py

+    if not vhosts:
+        return list()
+    tags_list = [vhost.display_repr() for vhost in vhosts]
+    while True:


The same can be said about the old select_vhosts function down below, but why do we need this loop? To me, it looks like it will only ever execute once.

I presumed there being an undefined reason behind this, as select_vhost() function has the same behavior. It's been around since original implementation by James, introduced in 99fcb4f and now looking at the original commit, it looks like it was in place for a help selection that has since been removed. I cleaned this up from the select_vhost() too.

bmw · 2018-02-28T05:04:00Z

certbot-apache/certbot_apache/obj.py

+            "File: {filename}\n"
+            "Addresses: {addrs}\n"
+            "Names: {names}\n"
+            "HTTPS: {https}\n\n".format(


nit: The trailing empty line on the last entry looks a little awkward to me:

------------------------------------------------------------------------------- 1: File: /etc/apache2/sites-enabled/bmatch.conf Addresses: *:80 Names: geqgeqoc.example.org HTTPS: No 2: File: /etc/apache2/sites-enabled/amatch.conf Addresses: *:80 Names: a.example.org HTTPS: No 3: File: /etc/apache2/sites-enabled/wildcard.conf Addresses: *:80 Names: abc.def.example.org, *.example.org HTTPS: No -------------------------------------------------------------------------------

Unless you prefer it that way, can we remove it?

The way I'd personally do this is not to include the 2nd newline in this function and have the function calling display_repr iterate over each representation adding a newline to the beginning/end of each one except for the first/last one.

It looks a lot cleaner without the extra newline, I agree. Even though handling that requires a few more lines in the code. This is now implemented.

bmw · 2018-02-28T16:52:02Z

Looks good except I don't think my comment at #5608 (review) got addressed.

To expand a little more on why I think it's important, Certbot will currently exit with a zero status and no warnings if there's no matching vhosts so it's not clear it didn't do what the user asked.

While we may want to change the behavior around deploy/enhance if we fail for a domain in the future (what's being proposed is we continue execution so we work for subsequent domains), I don't think we should make any of those changes here as it requires some UI changes to make sure the domains we failed on is 100% clear and not hidden in the scrollback in the user's terminal. Currently if we fail on one, we exit with an error and rollback changes to all which is while arguably less useful, I think make things pretty clear to the user: your request to deploy/enhance the cert failed.

bmw

LGTM!

Thanks for turning this around so quickly @joohoi.

joohoi added 2 commits February 22, 2018 17:03

Apache plugin wildcard support

20af164

Present dialog only once per domain, added tests

1b55f94

joohoi changed the title ~~[WIP] Apache plugin wildcard support~~ Apache plugin wildcard support for ACMEv2 Feb 23, 2018

Merge remote-tracking branch 'origin/master' into apache_acmev2

8755c94

ohemorange reviewed Feb 23, 2018

View reviewed changes

ohemorange reviewed Feb 24, 2018

View reviewed changes

ohemorange mentioned this pull request Feb 24, 2018

Nginx plugin wildcard support for ACMEv2 #5619

Merged

3 tasks

Review comment fixes

fd68f74

ohemorange requested a review from bmw February 27, 2018 20:40

bmw requested changes Feb 28, 2018

View reviewed changes

Review comment fixes

1820b42

Raise exception if no VHosts selected for wildcard domain

a773f7c

bmw added this to the 0.22.0 milestone Feb 28, 2018

bmw approved these changes Feb 28, 2018

View reviewed changes

bmw merged commit e9bc4a3 into master Feb 28, 2018

joohoi mentioned this pull request Mar 12, 2018

Enhance verb #5596

Merged

osirisinferi mentioned this pull request Jun 6, 2021

Make wildcard hostnames from installer choosable ... #8888

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apache plugin wildcard support for ACMEv2 #5608

Apache plugin wildcard support for ACMEv2 #5608

joohoi commented Feb 22, 2018 •

edited

ohemorange Feb 23, 2018

ohemorange Feb 24, 2018

ohemorange Feb 24, 2018

ohemorange Feb 24, 2018

ohemorange Feb 24, 2018

joohoi Feb 27, 2018

ohemorange Feb 24, 2018

ohemorange Feb 24, 2018

ohemorange Feb 24, 2018 •

edited

ohemorange commented Feb 24, 2018

ohemorange Feb 24, 2018

joohoi commented Feb 27, 2018 •

edited

bmw left a comment

bmw Feb 28, 2018

joohoi Feb 28, 2018

bmw Feb 28, 2018

joohoi Feb 28, 2018

bmw Feb 28, 2018

joohoi Feb 28, 2018

bmw Feb 28, 2018 •

edited

joohoi Feb 28, 2018 •

edited

bmw Feb 28, 2018

joohoi Feb 28, 2018

bmw Feb 28, 2018 •

edited

joohoi Feb 28, 2018

bmw commented Feb 28, 2018

bmw left a comment

		return fnmatch.fnmatch(name, domain)


		def choose_vhosts_wildcard(self, domain, create_ssl=True):

Apache plugin wildcard support for ACMEv2 #5608

Apache plugin wildcard support for ACMEv2 #5608

Conversation

joohoi commented Feb 22, 2018 • 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

ohemorange Feb 24, 2018 • edited

Choose a reason for hiding this comment

ohemorange commented Feb 24, 2018

Choose a reason for hiding this comment

joohoi commented Feb 27, 2018 • edited

bmw 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

bmw Feb 28, 2018 • edited

Choose a reason for hiding this comment

joohoi Feb 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmw Feb 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmw commented Feb 28, 2018

bmw left a comment

Choose a reason for hiding this comment

joohoi commented Feb 22, 2018 •

edited

ohemorange Feb 24, 2018 •

edited

joohoi commented Feb 27, 2018 •

edited

bmw Feb 28, 2018 •

edited

joohoi Feb 28, 2018 •

edited

bmw Feb 28, 2018 •

edited