python-postgres / fe

py-postgresql: Query a PostgreSQL database with Python 3 using the PQv3 protocol.
BSD 3-Clause "New" or "Revised" License
86 stars 35 forks source link

lib.net_pack stores wrong bit length #71

Open FirefighterBlu3 opened 10 years ago

FirefighterBlu3 commented 10 years ago

when packing an ip_address() for io, the bits value is wrongly stored as a /0 instead of /32 or /128

the following will fix this:

--- /usr/lib/python3.4/site-packages/postgresql/types/io/lib.py.orig    2014-06-02 12:42:50.783337348 +0000
+++ /usr/lib/python3.4/site-packages/postgresql/types/io/lib.py 2014-06-02 12:56:30.460003975 +0000
@@ -283,7 +283,9 @@
        Pack Postgres' inet/cidr data structure.
        """
        family, mask, data = triple
-       return bytes((fmap[family], mask or 0, 0 if mask is None else 1, len(data))) + data
+       mflag = mask and 1 or 0
+       mask = mask and mask or {4:32, 6:128}[family]
+       return bytes((fmap[family], mask, mflag, len(data))) + data

 def net_unpack(data,
        # Map IP version number to PGSQL src/include/utils/inet.h.

the original code stores "1.2.3.4" as "1.2.3.4/0" instead of "1.2.3.4/32". the above fix will store the correct bits value

commonism commented 10 years ago

same as https://github.com/python-postgres/fe/issues/66

commonism commented 10 years ago

This patch does not work, as unpacking an ip address with bits set in the host mask like 1.1.1.1/24 is rejected by "ipaddress".

FirefighterBlu3 commented 9 years ago

@commonism correct. for type INET you cannot have a mask length, mask length is only valid for type CIDR. 1.1.1.1/24. unfortunately, this patch is still needed and it does work with ipaddress.

although, my local definition for this function is slightly different, using .get() instead.

    family, bits, data = triple
    is_cidr = bits and 1 or 0
    bits    = bits and bits or {4:32, 6:128}.get(family, 32)
    return bytes((fmap[family], bits, is_cidr, len(data))) + data

here's a sample program.

import postgresql as pgsql

pgdb = pgsql.open('pq://xx:xx@10.255.0.2/vse?[sslmode]=prefer')

pgdb.execute('DROP TABLE IF EXISTS test_table_a')
pgdb.execute('CREATE TABLE test_table_a (a INET, b CIDR)')

s = pgdb.prepare('INSERT INTO test_table_a (a,b) VALUES ($1,$2)')

s('1.1.1.1', '2.2.2.2')      # CIDR default address will yield  2.2.2.2/32

try:
  s('1.1.1.1/24', '2.2.2.2')
except:
  print('invalid notation for type INET')

s('1.1.1.1', '2.2.0.0/16')   # 2.2.2.2/16 will by default raise: DETAIL: Value has bits set to right of mask

q = pgdb.query('SELECT * FROM test_table_a')

for _ in q:
  print(_)

pgdb.close()

this results in the following output:

~$ python /tmp/p.py
invalid notation for type INET
(IPv4Address('1.1.1.1'), IPv4Network('2.2.2.2/32'))
(IPv4Address('1.1.1.1'), IPv4Network('2.2.0.0/16'))

unmodified lib.py:

vse=# select * from test_table_a;
    a    |     b
---------+------------
 1.1.1.1/0 | 2.2.2.2/32
 1.1.1.1/0 | 2.2.0.0/16
(2 rows)

with my patch:

vse=# select * from test_table_a;
    a    |     b
---------+------------
 1.1.1.1 | 2.2.2.2/32
 1.1.1.1 | 2.2.0.0/16
(2 rows)

1.1.1.1/0 causes problems with other software and should be in INET format which has no mask length.

commonism commented 9 years ago

You are wrong with your assumption INET can not have a mask.

http://www.postgresql.org/docs/9.4/static/datatype-net-types.html

8.9.1. inet The inet type holds an IPv4 or IPv6 host address, and optionally its subnet, all in one field. The subnet is represented by the number of network address bits present in the host address (the "netmask"). If the netmask is 32 and the address is IPv4, then the value does not indicate a subnet, only a single host. In IPv6, the address length is 128 bits, so 128 bits specify a unique host address. Note that if you want to accept only networks, you should use the cidr type rather than inet.

The input format for this type is address/y where address is an IPv4 or IPv6 address and y is the number of bits in the netmask. If the /y portion is missing, the netmask is 32 for IPv4 and 128 for IPv6, so the value represents just a single host. On display, the /y portion is suppressed if the netmask specifies a single host.

INET allows a mask.

8.9.3. inet vs. cidr The essential difference between inet and cidr data types is that inet accepts values with nonzero bits to the right of the netmask, whereas cidr does not.

INET could be mapped to IPvXInterface. https://docs.python.org/3.5/library/ipaddress.html#ipaddress.IPv4Interface

FirefighterBlu3 commented 9 years ago

My presentation was unclear and my intent was to point out that the core module ipaddress was unable to handle INET with a netmask, I apologize. For PostgreSQL type INET, you cannot have netmask bits set _when using ipaddress.ip_address()._ ip_address() will reject the data if a bitmask is present, and ipaddress is the default translator for py-postgresql in Python3.

>>> ipaddress.ip_address('1.1.1.1')
IPv4Address('1.1.1.1')
>>> ipaddress.ip_address('1.1.1.1/32')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib64/python3.4/ipaddress.py", line 54, in ip_address
    address)
ValueError: '1.1.1.1/32' does not appear to be an IPv4 or IPv6 address
>>> ipaddress.ip_network('1.1.1.1/32')
IPv4Network('1.1.1.1/32')

ipaddress.ip_network() can accept address data (and will translate) wherein the data appears to be a host but has a less than full width mask set.

>>> ipaddress.ip_network('1.1.1.1/24')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib64/python3.4/ipaddress.py", line 74, in ip_network
    return IPv4Network(address, strict)
  File "/usr/lib64/python3.4/ipaddress.py", line 1490, in __init__
    raise ValueError('%s has host bits set' % self)
ValueError: 1.1.1.1/24 has host bits set
>>> ipaddress.ip_network('1.1.1.1/24', strict=False)
IPv4Network('1.1.1.0/24')

Previous to my patch, py-postgresql was storing INET addresses with a netmask indicated as /0. PostgreSQL is happy storing this data with a mask length, but querying it results in exceptions in the form of:

>>> ipaddress.ip_address('1.1.1.1/0')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib64/python3.4/ipaddress.py", line 54, in ip_address
    address)
ValueError: '1.1.1.1/0' does not appear to be an IPv4 or IPv6 address

It's fine if other software is capable of using pgsql type INET with netmask bits. py-postgresql's data type translator uses ipaddress in Python3 and the basic INSERT/SELECT out of the box, will break.

In the past I had a modified ipaddress module that handled the hosts with a netmask length but the notion of least surprise is that these functions should work out of the box without having to modify core modules.

jwp commented 9 years ago

I think I remember running into this. It was a little annoying having to strip out the /0 from ''::inet::text casts. Provide some tests with the patch and I'll apply it.