New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SiteAccess: Make VirtualHostMonster support IPv6 #314

Merged
merged 4 commits into from Oct 17, 2018

Conversation

Projects
None yet
6 participants
@lukenowak
Contributor

lukenowak commented Sep 6, 2018

from https://bugs.launchpad.net/zope2/+bug/699865 :

VirtualHostMonster does not work with IPv6 named hosts.

In case of such rewrite configuration:

RewriteRule (.*) http://10.0.243.129:9280/VirtualHostBase/https/[%{SERVER_ADDR}]:4080$1 [L,P]

When SERVER_ADDR is fd00::74ba VirtualHostMonster dies with:

Traceback (most recent call last):
File "/eggs/Zope2-2.12.14-py2.6-linux-x86_64.egg/ZPublisher/BeforeTraverse.py", line 145, in call
meth(*(container, request, None)[:args])
File "/eggs/Zope2-2.12.14-py2.6-linux-x86_64.egg/Products/SiteAccess/VirtualHostMonster.py", line 154, in call
host, port = host.split(':')
ValueError: too many values to unpack

This is because IPv6 addresses contain ":" in them.

@jmuchemb

This comment has been minimized.

Member

jmuchemb commented Sep 6, 2018

Thanks. But for us, we want to have it in Zope 2.13, right ?

@lukenowak

This comment has been minimized.

Contributor

lukenowak commented Sep 6, 2018

@jmuchemb shall I use different destination branch?

@leorochael

This comment has been minimized.

Contributor

leorochael commented Sep 6, 2018

Hi Luke,

Thanks for reporting the issue and providing the patch.

It seems to me that the code might fail when an IPv6 host is passed, but not a port. Eg:

  • /VirtualHostBase/http/[::1]/folder/

The host.rsplit(':', 1) looks like it's going to cut inside the IPv6 address in this case.

My take is that the code should do host.rsplit(':', 1) regardless of whether an IPv6 address is passed or not.

If we want to make the port mandatory in the VHM URL, then the if ':' in host becomes redundant, but then this change can't be made into the 2.13 branch as it's breaking compatibility.

On the other hand, if we keep the port optional, then we must somehow check for the presence of the : only in the part of the string after the IPv6 URL. I.e., if host.startswith('[') then locate the next ']and look for the:` after that.

@tseaver

This comment has been minimized.

Member

tseaver commented Sep 6, 2018

@jmuchemb Doing the work on master first makes sense: we can then backport to 2.13.

SiteAccess: Make VirtualHostMonster support IPv6
from https://bugs.launchpad.net/zope2/+bug/699865 :

VirtualHostMonster does not work with IPv6 named hosts.

In case of such rewrite configuration:

RewriteRule (.*) http://10.0.243.129:9280/VirtualHostBase/https/[%{SERVER_ADDR}]:4080$1 [L,P]

When SERVER_ADDR is fd00::74ba VirtualHostMonster dies with:

Traceback (most recent call last):
  File "/eggs/Zope2-2.12.14-py2.6-linux-x86_64.egg/ZPublisher/BeforeTraverse.py", line 145, in __call__
    meth(*(container, request, None)[:args])
  File "/eggs/Zope2-2.12.14-py2.6-linux-x86_64.egg/Products/SiteAccess/VirtualHostMonster.py", line 154, in __call__
    host, port = host.split(':')
ValueError: too many values to unpack

This is because IPv6 addresses contain ":" in them.

@lukenowak lukenowak force-pushed the lukenowak:site-access-vhm-ipv6-support branch from ef0c0b3 to b792acb Sep 7, 2018

@lukenowak

This comment has been minimized.

Contributor

lukenowak commented Sep 7, 2018

@leorochael in 71f0c96 I hope I covered your comments and those tests are passing. Is that it, or I missed the problem?

@tseaver

tseaver approved these changes Sep 7, 2018

@leorochael

I'm with intermittent Internet (Piraju, Luke, you know) will comment better on Monday.

@leorochael

Despite my comments, I'm not going to block this PR by requesting changes. Feel free to take my suggestions into account or not.

host, port = host.split(':')
if host.startswith('['):
# IPv6 address passed
host, port = host.rsplit(':', 1)

This comment has been minimized.

@leorochael

leorochael Sep 9, 2018

Contributor

Ok, so here is my take:

As I mentioned earlier, this inner if seems redundant.

host.rsplit(':', 1) should work equally well for IPv4 adresses or plain hostnames.

I would prefer this inner if removed and the IPv6 code path used for all cases. It should be trivial to verify that it works, now that you wrote all tests.

Additionally, as I mentioned before, it seemed to me that the IPv6-no-port scenario shouldn't work with this code path, but the tests clearly show it does, and now I understand why:

"[::1]".rsplit(':', 1) == ["[:", "1]"]

And since "1]" is not a standard port for any protocol, the .setServerURL() call joins it back to the hostname with the : char and it all works out in the end...

...except for the fact that [: is not a real hostname or IPv{4,6} address and 1] is not a real port. So if someone in the future decides to do something inside .setServerURL() with the host or the port separately, that code could break.

What I suggest instead is to replace the whole block from 150 to 159 (i.e. the outer if ':' in host: block along with its else: block) with the following:

host, port = HOST_PORT_SPLIT_RE.match(case).group('host', 'port')
request.setServerURL(protocol, host, port)

Where HOST_PORT_SPLIT_RE is defined at the module level as:

HOST_PORT_SPLIT_RE = re.compile(r'(?P<host>.*?)(?::(?P<port>\d+))?$')

I was going to suggest using splitport() imported from ZPublisher.HTTPRequest, instead of HOST_PORT_SPLIT_RE, but it's being deprecated, unfortunately.

This comment has been minimized.

@jmuchemb

jmuchemb Sep 9, 2018

Member

You're right about the IPv6-no-port scenario. It's too ugly to keep code that does not split correctly, even if the current implementation of setServerURL rectifies things.

However, I find overkill to use a regex.

I suugest:

                host = stack.pop()
                if host.endswith(']'):
                    # IPv6 without port
                    request.setServerURL(protocol, host)
                else:
                    request.setServerURL(protocol, *host.rsplit(':', 1))

This comment has been minimized.

@jerome-nexedi

jerome-nexedi Oct 2, 2018

I was going to suggest using splitport() imported from ZPublisher.HTTPRequest, instead of HOST_PORT_SPLIT_RE, but it's being deprecated, unfortunately.

I feel it would be OK to reuse ZPublisher.HTTPRequest.splitport() here, even if the actual function behind is deprecated. It would be just one more place where this is used in Zope. When laterZPublisher.HTTPRequest.splitport() will be replaced by something more modern, all users of this import will then use the new implementation.

PS:
I also find that the regex solution would be natural for this, but I want to point out that the regex used in python's splitport ( re.compile('(.*):([0-9]*)$', re.DOTALL) ) is a bit "easier" than the suggested HOST_PORT_SPLIT_RE.

This comment has been minimized.

@leorochael

leorochael Oct 3, 2018

Contributor

Indeed the suggested regex is a little more complex. The intention is to always parse all parts in a single match() call, instead of needing one or two extra conditionals.

I don't mind reusing the deprecated splitport() if imported via ZPublisher.HTTPRequest as sugested by @jerome-nexedi.

This comment has been minimized.

@leorochael

leorochael Oct 3, 2018

Contributor

I'm also OK with @jmuchemb's suggestion, BTW.

This comment has been minimized.

@lukenowak

lukenowak Oct 17, 2018

Contributor

With splitport the implementation simplifies a lot, see e6b0e76.

self.assertEqual(self.app.REQUEST['ACTUAL_URL'],
'http://[::1]/folder/')

def testIPv6Noport(self):

This comment has been minimized.

@leorochael

leorochael Sep 9, 2018

Contributor

nitpick: capitalyze Port, as in testIPv6NoPort()

This comment has been minimized.

@lukenowak

lukenowak Oct 17, 2018

Contributor

Done in ee8cc4b

lukenowak added some commits Oct 17, 2018

fixup! SiteAccess: Make VirtualHostMonster support IPv6
Fixed test name with capitalized Port
fixup! SiteAccess: Make VirtualHostMonster support IPv6
Use splitport for easy port splitting.

@jmuchemb jmuchemb merged commit b410c82 into zopefoundation:master Oct 17, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 79.643%
Details

@lukenowak lukenowak deleted the lukenowak:site-access-vhm-ipv6-support branch Oct 17, 2018

@jmuchemb

This comment has been minimized.

Member

jmuchemb commented Oct 17, 2018

@jmuchemb Doing the work on master first makes sense: we can then backport to 2.13.

@icemac Now that's merged, we'd like a new release of 2.13 with this change. However, I'm not used to the procedure for this repository. I anyway don't have rights for pypi.

@tseaver

This comment has been minimized.

Member

tseaver commented Oct 17, 2018

@jmuchemb We cannot make a 2.13 release from the master branch: it has moved on to 4.x. We need a PR which backports the changes to the 2.13 branch.

@jmuchemb

This comment has been minimized.

Member

jmuchemb commented Oct 17, 2018

We cannot make a 2.13 release from the master branch

Sure.

But a PR for a cherry-pick -x without conflict looks too much for me.

perrinjerome added a commit to perrinjerome/Zope that referenced this pull request Nov 2, 2018

SiteAccess: Make VirtualHostMonster support IPv6 (zopefoundation#314)
from https://bugs.launchpad.net/zope2/+bug/699865 :

VirtualHostMonster does not work with IPv6 named hosts.

In case of such rewrite configuration:

RewriteRule (.*) http://10.0.243.129:9280/VirtualHostBase/https/[%{SERVER_ADDR}]:4080$1 [L,P]

When SERVER_ADDR is fd00::74ba VirtualHostMonster dies with:

Traceback (most recent call last):
  File "/eggs/Zope2-2.12.14-py2.6-linux-x86_64.egg/ZPublisher/BeforeTraverse.py", line 145, in __call__
    meth(*(container, request, None)[:args])
  File "/eggs/Zope2-2.12.14-py2.6-linux-x86_64.egg/Products/SiteAccess/VirtualHostMonster.py", line 154, in __call__
    host, port = host.split(':')
ValueError: too many values to unpack

This is because IPv6 addresses contain ":" in them.

--
This is backport of zopefoundation#314 to
2.13 branch

This also include part of:
2e9d01e - flake8 Products/Shared/ZTUtils.
removing test_suite function from the test

perrinjerome added a commit to perrinjerome/Zope that referenced this pull request Nov 7, 2018

SiteAccess: Make VirtualHostMonster support IPv6 (zopefoundation#314)
from https://bugs.launchpad.net/zope2/+bug/699865 :

VirtualHostMonster does not work with IPv6 named hosts.

In case of such rewrite configuration:

RewriteRule (.*) http://10.0.243.129:9280/VirtualHostBase/https/[%{SERVER_ADDR}]:4080$1 [L,P]

When SERVER_ADDR is fd00::74ba VirtualHostMonster dies with:

Traceback (most recent call last):
  File "/eggs/Zope2-2.12.14-py2.6-linux-x86_64.egg/ZPublisher/BeforeTraverse.py", line 145, in __call__
    meth(*(container, request, None)[:args])
  File "/eggs/Zope2-2.12.14-py2.6-linux-x86_64.egg/Products/SiteAccess/VirtualHostMonster.py", line 154, in __call__
    host, port = host.split(':')
ValueError: too many values to unpack

This is because IPv6 addresses contain ":" in them.

--
This is backport of zopefoundation#314 to
2.13 branch

This also include part of:
2e9d01e - flake8 Products/Shared/ZTUtils.
removing test_suite function from the test
@icemac

This comment has been minimized.

Member

icemac commented Nov 7, 2018

@jmuchemb See #395 for the port to Zope 2.13.

icemac added a commit that referenced this pull request Nov 9, 2018

perrinjerome added a commit to perrinjerome/Zope that referenced this pull request Nov 15, 2018

perrinjerome added a commit to perrinjerome/Zope that referenced this pull request Nov 17, 2018

perrinjerome added a commit that referenced this pull request Nov 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment