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

x-forwarder-for logic not working for ipv6 addresses #275

Closed
macrauder opened this Issue Dec 4, 2013 · 12 comments

Comments

Projects
None yet
3 participants
@macrauder

macrauder commented Dec 4, 2013

Hi,

the new x-forwarder logic isn't working with ipv6 ip adresses and the corresponding fallback logic.
The current logic is splitting the ":" char. But in ipv6 it isn't correct
ipv6 ip = "2001:db8:0:8d3:0:8a2e:70:7344"
ipv4 address in ipv6 style = "::ffff:1.2.3.4"
So we should'nt us ":" to split ports and addresses, the old logic was running fine for me.

@evantahler

This comment has been minimized.

Member

evantahler commented Dec 5, 2013

I've been digging into this, and there are a few things at play:

  • you are correct, and splitting by : is a bad idea for ipv6
  • while not that common, there are certain load balancers/proxies that will append the port (even in ipv6) to thier x-forwarded-for, so we need to account for them
  • while it is possible to detect what type of connection the client came in on (req.connection._peername.family => "IPv6") this API is likely to change (hence the _), and that doesn't help us to know if the port is being reported or not

What I think might be an OK solution is as follows:

  • we know that all IPv4 address have 0 colons
  • IPv6 addresses can have 3 ([::ffff:127.0.0.1]) or 7 ([0:0:0:0:0:0:0:1])
  • so we can safely remove the last part of the string split by ":" if the number of colons in the string is not 0, 3, or 7 (or the number of parts when split should number 1,4, or 8).

implemented here: 3202e2f

Thoughts?

@macrauder

This comment has been minimized.

macrauder commented Dec 6, 2013

I think that there is currently no 100% save way to deal with the different x-forward headers.
Your solution sounds good and should fit well for the most scenarios.

@evantahler

This comment has been minimized.

Member

evantahler commented Dec 6, 2013

I'm not too happy with this solution, but I'm glad this will work for you

@evantahler evantahler closed this Dec 6, 2013

@nullivex

This comment has been minimized.

Contributor

nullivex commented Dec 6, 2013

Hey Guys,

I have built several IP management systems before and was looking through the commit. Valid IPv6 notation comes in a lot of different formats and I dont know if the rules in your patch work quite right. However here are some of the rules.

  • The address must start with hex and end with hex and can use "::" in the middle to signal a string of zeros think of the following 2001::1 would be the equivalent of 2001:0000:0000:0000:0000:0000:0000:0001 which is how the IPv6 parsing code will treat it.
  • The address can start with all zeros this is done in the format you mentioned ::1 which is the real address of 0000:0000:0000:0000:0000:0000:0000:0001
  • However most real addresses look more like 2604:4480::5

Its not a good ideal to try and parse them as a string like you are doing in the patch this is not going to be reliable and will require continual patching.

From the Wikipedia documentation: http://en.wikipedia.org/wiki/IPv6_address

It should be using square bracket notation as noted here: http://tools.ietf.org/html/rfc5952#page-11

I would remove this patch and parse the notations provided in the IETF document. Proxies need to fix their bugs of appending :num to the address without brackets.

I took a look at this library: https://github.com/beaugunderson/javascript-ipv6 but it doesnt properly support this format by default. I filed an issue against it being supported here: beaugunderson/ip-address#12

Parsing should be pretty simple without that library being patched though. I put together this example.

var parseIPv6URI = function(addr){
  var host = '::1'
    , port = '80'
    , regexp = new RegExp(/\[([0-9a-f:]+)\]:([0-9]{1,5})/)
  //if we have brackets parse them and find a port
  if(-1 < addr.indexOf('[') && -1 < addr.indexOf(']')){
    var res = regexp.exec(addr)
    if(null === res){
      throw new Error('failed to parse address')
    }
    host = res[1]
    port = res[2]
  } else {
    host = addr
  }
  return {host: host, port: parseInt(port,10)}
}

//address with port
console.log(parseIPv6URI('[2604:4480::5]:80'))
//address without port
console.log(parseIPv6URI('2604:4480::5'))
//full uri
console.log(parseIPv6URI('http://[2604:4480::5]:80/foo/bar'))
//failing address
console.log(parseIPv6URI('[2604:4480:z:5]:80'))

Here is the output

{ host: '2604:4480::5', port: 80 }
{ host: '2604:4480::5', port: 80 }
{ host: '2604:4480::5', port: 80 }

C:\Users\nulli_000\WebstormProjects\test\test.js:9
      throw new Error('failed to parse address')
            ^
Error: failed to parse address
@nullivex

This comment has been minimized.

Contributor

nullivex commented Dec 6, 2013

I submitted a pull request on the other repo but I figured this might be useful code to this issue.

beaugunderson/ip-address#13

@evantahler

This comment has been minimized.

Member

evantahler commented Dec 8, 2013

Thanks for this!

The problem I'm still stuck on is determining if an address is v4 or v6. For example if you bind the http server to "::", all addresses appear to be coming from ipv6, but the ipv4 addresses will be reported on the connection of that type.

Perhaps we can use the colon thing to determine if we are looking at an ipv4 or ipv6 address (more than 1 colon in all cases means ipv6). We can use your regex above on v6, and go back to the behavior we used to use for v4.

Thoughts?

@nullivex

This comment has been minimized.

Contributor

nullivex commented Dec 8, 2013

Well ipv6 doesnt have dots. So you could do an indexOf on it and check for a dot.

If there are no dots or colons then you should treat it as a long ipv4 eg: 199.87.232.5 in octet notation is really 3344427013 in long notation which is what you should default to since its not possible to transport IPv6 as an integer.

@evantahler

This comment has been minimized.

Member

evantahler commented Dec 9, 2013

Thanks!

Using your regexp, I've gotten us to here: fe42b24

@evantahler evantahler reopened this Dec 9, 2013

@evantahler

This comment has been minimized.

Member

evantahler commented Dec 9, 2013

and a note about how we got here c84999d

@evantahler evantahler closed this Dec 9, 2013

@nullivex

This comment has been minimized.

Contributor

nullivex commented Dec 9, 2013

Awesome! Glad it was of some use.

@nullivex

This comment has been minimized.

Contributor

nullivex commented Dec 9, 2013

The only thing I was going to note is that the function I submitted to the ipv6 library is a bit more complete than the one you implemented. Might be worth adding it as it passes more tests.

@evantahler

This comment has been minimized.

Member

evantahler commented Dec 9, 2013

Cool. Keep us posted as it matures.

On Sunday, December 8, 2013, Bryan Tong wrote:

The only thing I was going to note is that the function I submitted to the
ipv6 library is a bit more complete than the one you implemented. Might be
worth adding it as it passes more tests.


Reply to this email directly or view it on GitHubhttps://github.com//issues/275#issuecomment-30104503
.

Evan Tahler
412.897.6361
evantahler@gmail.com

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