encode_ip_timestamp has bug with IPv6

[thanhlim]

Error:
ip_chars = ''.join(map(chr, map(int, ip.split('.'))))
ValueError: invalid literal for int() with base 10: '2620:0:1000:fd73:7c07:c0a0:75bd:2dde'

The solution is relatively simple to ensure that an exception doesn't occur when we have an ipv6 address.

Reference Method:
def encode_ip_timestamp(ip, timestamp):
ip_chars = ''.join(map(chr, map(int, ip.split('.'))))
t = int(timestamp)
ts = ((t & 0xff000000) >> 24,
(t & 0xff0000) >> 16,
(t & 0xff00) >> 8,
t & 0xff)
ts_chars = ''.join(map(chr, ts))
return bytes_(ip_chars + ts_chars)

[digitalresistor]

Looking at the function: https://github.com/Pylons/pyramid/blob/master/pyramid/authentication.py#L748 it doesn't provide anything of value.

There is no reason from a cryptographic standpoint to convert the timestamp to bytes (e.g. str(time.time()) would just do as well), and then concatenating the IP address, to then pass it into a hash function.

https://github.com/Pylons/pyramid/blob/master/pyramid/authentication.py#L740

Could be refactored as follows:

ip + str(int(timestamp)) + secret + userid + b'\0'

Why int() and then str()? Well we want to drop the decimal from the timestamp if it exists. Nothing of value would be lost. Unless encode_ip_timestamp is used elsewhere in the source code that function could be dropped entirely.

On another note, the default hashalg on https://github.com/Pylons/pyramid/blob/master/pyramid/authentication.py#L412 should be changed to SHA-1 at least, or SHA-2 family (SHA-256). Although I am not aware of any inherent issues with HMAC(MD5) it is no longer recommended for use, HMAC(SHA-1) has also been deprecated by NIST, and HMAC(SHA-192) or higher is recommended, although it looks like a warning is thrown about the default value (excellent!)

If I get some time later tonight I may make the changes above to drop the requirement for the encode_ip_timestamp() function and submit a pull request. Unless someone objects and can provide a good reason for keeping the encode_ip_timestamp() (maybe leave it in for backwards compatibility, deprecate it over time?).

[digitalresistor]

@thanhlim do be aware that using the

``include_ip``

       Default: ``False``.  Make the requesting IP address part of
       the authentication data in the cookie.  Optional.

functionality will mean that if the user has their IPv6 addresses given to them using SLAAC that their session ticket will expire whenever their IP changes, which is dependant on their local router advertising the prefix, and the preferred lifetime/valid lifetime. See http://en.wikipedia.org/wiki/IPv6_address#Address_lifetime for more information. Using the IP address in the auth ticket will mean your users get logged out sooner and more often, so your mile-age may vary and caveat emptor.

[thanhlim]

Agreed. I've been planning to migrate away from MD5 in the very near future and was planning on those changes.

Also, thinking about it, I should remove the requirement to use include_ip as well. Even though the app runs almost exclusively on the mobile device using wireless, and not wifi, and thus using MobileIP, there could be issues with technology changes that might change the IP address.

Anyway, just wanted to give you the heads up about what I saw out in my logs.

[digitalresistor]

Please see my pull request #837 for a fix that you can deploy. Hopefully my pull request is accepted and it will be fixed for everyone soon.

[mmerickel]

I've merged #837, hopefully that solves these concerns. Pyramid deprecated md5 in the AuthTktAuthenticationPolicy in 1.4 by adding the hashalg parameter, if this wasn't known already.