Enable IPv6 support in standalone plugin #4773

ohemorange · 2017-06-02T00:09:15Z

…oth servers run on the same port.

ohemorange · 2017-06-02T00:12:02Z

All of the assumptions in code here are based on things tested manually.

That is:

If python is compiled without IPv6, binding to an IPv6 port will raise socket.error.
On FreeBSD, IPv4 traffic will not forward to a socket bound on an IPv6 port, but you can ask for two different servers on the same address/port, with different protocols.
On Ubuntu, IPv4 traffic will forward to a socket bound on an IPv6 port, but you cannot ask for two different servers on the same address/port, with different protocols.

ohemorange · 2017-06-02T00:15:03Z

@bmw, I haven't added tests (nor have I tested the whole thing manually...), because I made a few design decisions here based on the discussion in #1466 and other places that might seem weirder now that they've actually been implemented. No need to do a full review just yet, but if you're good with the overall structure, I'll:

add unit tests
clean up the pieces of system test code that I've been using, and
find some way to get them to run automatically on different flavors of unix.

ohemorange · 2017-06-05T08:57:21Z

check there's no race condition (things are deduped later) if a response comes in on both ports before we shut the server down

bmw · 2017-06-05T18:17:04Z

For the purposes of trying to land everything we want in time for the release, I've asked @zjs to take a look at this, but one thing I wanted to mention:

On Ubuntu, IPv4 traffic will forward to a socket bound on an IPv6 port, but you cannot ask for two different servers on the same address/port, with different protocols.

If desired, I believe we can work around this and get the same behavior as the BSDs using IPV6_V6ONLY as briefly mentioned here.

zjs · 2017-06-05T18:17:18Z

acme/acme/standalone.py

+                new_args = (new_address,) + args[1:]
+                server = ServerClass(*new_args, **kwargs)
+            except socket.error:
+                pass


nit: It seems like some logging here would be useful for debugging things if things don't work quite as we expect.

It's expected behavior that this will fail, so setting the level to DEBUG.

zjs · 2017-06-05T18:56:14Z

acme/acme/standalone_test.py

+        # connect to all addresses
+        for sockname in socknames:
+            host, port = sockname[:2]
+            cert = crypto_util.probe_sni(


This test fails for me locally with Error: [Errno -9] Address family for hostname not supported (which seems to match some of the Travis results.

01a5ada solves this for me and when run manually, the standalone plugin now successfully binds to IPv6 on my machine (Ubuntu, Python 2.7).

zjs · 2017-06-05T18:56:59Z

acme/acme/standalone.py

+                kwargs["ipv6"] = ip_version
+                new_address = (address[0],) + (port,) + address[2:]
+                new_args = (new_address,) + args[1:]
+                server = ServerClass(*new_args, **kwargs)


nit: lint doesn't like this use of star args

ok but the linter thinks it's bad for readability but really we want it for functionality so I'm going to tell it to mind its own business.

Comments have been addressed in subsequent commits.

ohemorange · 2017-06-06T01:31:02Z

If desired, I believe we can work around this and get the same behavior as the BSDs using IPV6_V6ONLY as briefly mentioned here.

Example of how to do this is in acme/acme/standalone_test.py, in case we want to.

ohemorange · 2017-06-06T01:49:37Z

Anyway, I ended up mimicking FreeBSD-like conditions in the unit tests, and Travis runs things on Ubuntu, so I no longer feel as strongly that we need to automate system tests to get this in.

ohemorange · 2017-06-06T02:47:13Z

Manual testing I've done:

Ubuntu with IPv6 enabled but no DNS record so it connects over IPv4
Ubuntu with IPv6 enabled + DNS
FreeBSD with IPv6 + DNS
FreeBSD with IPv4 only

ohemorange · 2017-06-06T18:32:41Z

Also, we do get BSD testing, because we run tests on OSX!

bmw · 2017-06-06T19:54:41Z

I'm reviewing this.

zjs · 2017-06-06T20:12:37Z

acme/acme/standalone.py

+            else:
+                self.servers.append(server)
+                # Always bind to the same port
+                port = server.socket.getsockname()[1]


The purpose of this wasn't immediately obvious to me. I think it's for when port is initially None. Perhaps for future readers it might be helpful to expand the comment to explain the case(s) where this is is used?

zjs

I'm happy with this.

I'll let @bmw decide if he wants to review before merging.

bmw

Posting this a little earlier than I normally would to get all my comments out there.

Just got a single cert on both Ubuntu for two domains where one was over IPv4 and one was over IPv6. Testing the same on FreeBSD now.

Other than my nitpicky inline comments and the things we talked about out of band, one concern I have is the combination of the unreleased --<challenge-name>-address flag and this PR. What would you expect Certbot's behavior to be when using this flag and standalone? Is what we do correct/OK? Do we want to support it long term?

bmw · 2017-06-06T19:48:05Z

acme/acme/standalone.py

+       """
+
+    def __init__(self, *args, **kwargs):
+        address = args[0]


nit: Since we'll blow up here if this class is called with no args, perhaps we should just add a server_address argument to __init__. This documents the need to provide this argument and it should have no negative effect as this is the name of the parameter used in socketserver.TCPServer (and its subclass BaseHTTPServer.HTTPServer).

bmw · 2017-06-06T20:55:45Z

acme/acme/standalone.py

+    def __init__(self, *args, **kwargs):
+        address = args[0]
+        port = address[1]
+        ServerClass = kwargs.pop("server_class", object)


Is using object the right thing here? Other thoughts:

Since this in theory is a general class for providing a single server object for IPv4 and IPv6, perhaps we should fall back to socketserver.TCPServer that all classes currently used by this server are based on.

Since using this class with an ACME client without providing a server_class is almost certainly an error, we should handle it as such. This currently happens when using object, but the error is pretty cryptic. I think the best way to do this is check if server_class is in kwargs and raise a TypeError if it's not. Alternatively, we can make server_class the first argument after self to __init__ and the TypeError will be handled for you, but this further deviates the class from the server objects it's trying to wrap.

I have a preference for approach number 2 as I think it'll help catch errors. We could always go the first route later if we wanted. You can do whatever you think is best though.

It can't be socketserver.TCPServer, because that doesn't like when you pass in an ipv6 kwarg. I think it's actually fine to make it a named argument, because you probably wouldn't directly use the Base class anyway, and our TLSSNI01DualNetworkedServers and HTTP01DualNetworkedServers set that argument for you.

bmw · 2017-06-06T21:11:24Z

acme/acme/standalone.py

+                self.servers.append(server)
+                # If two servers are set up and port 0 was passed in, ensure we always
+                # bind to the same port for both servers.
+                port = server.socket.getsockname()[1]


Nice catch.

bmw · 2017-06-06T21:18:20Z

acme/acme/standalone_test.py

@@ -1,6 +1,8 @@
 """Tests for acme.standalone."""
+import mock


super nit: Since this isn't part of the standard library, it should be moved down with the other 3rd party imports according to our style guides.

bmw · 2017-06-06T21:26:06Z

acme/acme/standalone_test.py

+                self.address_family = socket.AF_INET
+            socketserver.TCPServer.__init__(self, *args, **kwargs)
+            if ipv6:
+                # pylint: disable=no-member


With this line, the three disable=no-member comments below can be removed.

pylint scoping is strange.

ohemorange · 2017-06-06T22:27:09Z

Other than my nitpicky inline comments and the things we talked about out of band, one concern I have is the combination of the unreleased ---address flag and this PR. What would you expect Certbot's behavior to be when using this flag and standalone? Is what we do correct/OK? Do we want to support it long term?

I built this PR on top of #4694, so I was intending it to work correctly with these changes. Manual testing indicates that's the case. It's always fine to fail to start a server. So if you pass an IPv6 address, it just won't correctly start up the IPv4 server, and on FreeBSD vice versa.

bmw · 2017-06-06T22:36:09Z

I built this PR on top of #4694, so I was intending it to work correctly with these changes. Manual testing indicates that's the case.

It works, but it's a new flag that we have a chance to modify right now. Does it work the way we want long term? To be fair, this may not be the right place for this discussion as I don't think it should block this PR, however, it may cause us to block shipping #4694.

With that said, after thinking about it, I'm personally fine with things as they are now. Given I think we should support a flag like this, I think our options are:

Create both IPv4 and IPv6 versions of each flag.
Have one flag that can work for either address type.

Having separate flags makes Certbot uglier and increases complexity (what happens if you provide both IPv4 and IPv6 flags?). Currently, Certbot just works whether you provide an IPv4 or IPv6 address on the command line. As other plugins support this flag, we may have to write code that determines whether an address is IPv4 or IPv6 but then every plugin can benefit from it.

The only downside I see to this approach is you can't tell Certbot to listen on both IPv4 and IPv6 and specify the interface for each one. The only reason I could come up with where someone would want to do this is they have multiple domains that each require a different interface be used but they want all domains in the same cert. This is an extreme edge case though and I don't think it's unreasonable to have the user create two certs in this case.

I'd like someone else to make sure they agree with me here, but otherwise, I think the only thing left to worry about here are my obnoxious nits.

ohemorange · 2017-06-06T22:41:22Z

The only downside I see to this approach is you can't tell Certbot to listen on both IPv4 and IPv6 and specify the interface for each one. The only reason I could come up with where someone would want to do this is they have multiple domains that each require a different interface be used but they want all domains in the same cert. This is an extreme edge case though and I don't think it's unreasonable to have the user create two certs in this case.

I think it's reasonable to say "create two certs" for that case.

bmw

LGTM

ohemorange added 2 commits June 1, 2017 16:00

add TLSSNI01DualNetworkedServers

1895f3c

use DualNetworkedServers in certbot/plugins/standalone.pyalso, make b…

e3cc48d

…oth servers run on the same port.

ohemorange added this to the 0.15.0 milestone Jun 2, 2017

ohemorange requested a review from bmw June 2, 2017 00:09

ohemorange added 3 commits June 2, 2017 14:24

fix tests and syntax errors

60f109a

add docstrings, remove unused import and variable

7ff9acc

fix *args

318fa89

zjs previously requested changes Jun 5, 2017

View reviewed changes

ohemorange added 2 commits June 5, 2017 13:18

probe_sni connect on ipv6 and ipv4

914d44c

pylint and add logging message

01a5ada

ohemorange added 11 commits June 5, 2017 14:32

fix typo and add debugging information for remote tests

6a8591e

remove dead code in the hopes that tests will pass

69cb105

de-specify host protocol

517465e

rename variable

d38510b

also remove source_address?

63d501a

fix previous commit

18b70f5

getaddrinfo appears to need the address type

329888c

None, not empty string

8061f7a

put source_address back in

41098b3

copy dict

92a0071

get coverage

56ed88a

ohemorange added 2 commits June 5, 2017 18:52

mimic freebsd-like conditions to get test coverage

bec9223

add comment to test

7d7578a

test ServerManager taking into account BSD systems

baa7517

ohemorange assigned zjs and bmw Jun 6, 2017

bmw unassigned zjs Jun 6, 2017

zjs reviewed Jun 6, 2017

View reviewed changes

add comment

a6d37fd

zjs previously approved these changes Jun 6, 2017

View reviewed changes

pass tests even if python is compiled without ipv6 support

8bf51e7

ohemorange dismissed zjs’s stale review via 8bf51e7 June 6, 2017 21:56

bmw requested changes Jun 6, 2017

View reviewed changes

make server_address an argument to document it

0be642b

ohemorange added 2 commits June 6, 2017 15:37

pass ServerClass as a named argument to BaseDualNetworkedServers

288ea0b

change import order

d283103

remove extra pylint disables

ca4538b

ohemorange assigned bmw and unassigned bmw Jun 6, 2017

bmw approved these changes Jun 6, 2017

View reviewed changes

ohemorange merged commit 2391848 into master Jun 7, 2017

ohemorange deleted the ipv6 branch June 7, 2017 00:04

znerol mentioned this pull request Jan 21, 2018

IPv6 support prometheus-pve/prometheus-pve-exporter#6

Closed

MichaIng mentioned this pull request Jun 13, 2018

DietPi-LetsEncrypt | IPv6 support on Lighttpd with HTTPS MichaIng/DietPi#1840

Closed

alexzorin mentioned this pull request Jun 19, 2021

Improve standalone errors #8919

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable IPv6 support in standalone plugin #4773

Enable IPv6 support in standalone plugin #4773

ohemorange commented Jun 2, 2017 •

edited

Loading

ohemorange commented Jun 2, 2017

ohemorange commented Jun 2, 2017 •

edited

Loading

ohemorange commented Jun 5, 2017 •

edited

Loading

bmw commented Jun 5, 2017 •

edited

Loading

zjs Jun 5, 2017

ohemorange Jun 5, 2017

zjs Jun 5, 2017

zjs Jun 5, 2017 •

edited

Loading

zjs Jun 5, 2017

ohemorange Jun 5, 2017

ohemorange commented Jun 6, 2017 •

edited

Loading

ohemorange commented Jun 6, 2017

ohemorange commented Jun 6, 2017 •

edited

Loading

ohemorange commented Jun 6, 2017

bmw commented Jun 6, 2017

zjs Jun 6, 2017

zjs left a comment

bmw left a comment •

edited

Loading

bmw Jun 6, 2017

bmw Jun 6, 2017 •

edited

Loading

ohemorange Jun 6, 2017

bmw Jun 6, 2017

bmw Jun 6, 2017

bmw Jun 6, 2017

ohemorange Jun 6, 2017

ohemorange commented Jun 6, 2017 •

edited

Loading

bmw commented Jun 6, 2017 •

edited

Loading

ohemorange commented Jun 6, 2017

bmw left a comment

Enable IPv6 support in standalone plugin #4773

Enable IPv6 support in standalone plugin #4773

Conversation

ohemorange commented Jun 2, 2017 • edited Loading

ohemorange commented Jun 2, 2017

ohemorange commented Jun 2, 2017 • edited Loading

ohemorange commented Jun 5, 2017 • edited Loading

bmw commented Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zjs Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohemorange commented Jun 6, 2017 • edited Loading

ohemorange commented Jun 6, 2017

ohemorange commented Jun 6, 2017 • edited Loading

ohemorange commented Jun 6, 2017

bmw commented Jun 6, 2017

Choose a reason for hiding this comment

zjs left a comment

Choose a reason for hiding this comment

bmw left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmw Jun 6, 2017 • edited Loading

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 commented Jun 6, 2017 • edited Loading

bmw commented Jun 6, 2017 • edited Loading

ohemorange commented Jun 6, 2017

bmw left a comment

Choose a reason for hiding this comment

ohemorange commented Jun 2, 2017 •

edited

Loading

ohemorange commented Jun 2, 2017 •

edited

Loading

ohemorange commented Jun 5, 2017 •

edited

Loading

bmw commented Jun 5, 2017 •

edited

Loading

zjs Jun 5, 2017 •

edited

Loading

ohemorange commented Jun 6, 2017 •

edited

Loading

ohemorange commented Jun 6, 2017 •

edited

Loading

bmw left a comment •

edited

Loading

bmw Jun 6, 2017 •

edited

Loading

ohemorange commented Jun 6, 2017 •

edited

Loading

bmw commented Jun 6, 2017 •

edited

Loading