From 24505c48b684189e63a59b83e7ccb6a8bcba8c78 Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Tue, 21 Jan 2025 13:24:13 +0000 Subject: [PATCH 1/5] Disallow non-int elements in OID The OID class is designed to represent a tuple of ints. It was never designed to contain non-int elements (although it can translate non-int values to int elements when instances are constructed). The Zino code has bugs that actually would feed the OID class with non-int elements, and because the OID class accepted it, strange workarounds have been written by those who did not understand that this was not the intended behavior. This change enforces all elements to be int on instantiation - presumably at the cost of instantiation becoming slower. --- src/zino/oid.py | 5 +++++ tests/oid_test.py | 7 +++++++ 2 files changed, 12 insertions(+) diff --git a/src/zino/oid.py b/src/zino/oid.py index 0f6098400..dcfe2ee9d 100644 --- a/src/zino/oid.py +++ b/src/zino/oid.py @@ -35,6 +35,11 @@ def __new__(cls, oid): return oid return tuple.__new__(cls, oid) + def __init__(self, oid): + super().__init__() + if not all(isinstance(i, int) for i in self): + raise TypeError("Not all arguments could be converted to `int`") + def __str__(self): return SEPARATOR + SEPARATOR.join([str(i) for i in self]) diff --git a/tests/oid_test.py b/tests/oid_test.py index 47168b605..27d461e98 100644 --- a/tests/oid_test.py +++ b/tests/oid_test.py @@ -1,3 +1,5 @@ +import pytest + from zino.oid import OID @@ -38,3 +40,8 @@ def test_can_create_oid_from_bytestring(): oid_bytestring = b".1.2.3.4" oid = OID(oid_bytestring) assert str(oid) == ".1.2.3.4" + + +def test_when_oid_contains_non_numeric_elements_it_should_raise_typeerror(): + with pytest.raises(TypeError): + OID((1, 2, 3, "foo", 4)) From 236bb67fecf6bb70f31a7f728079784c142f936a Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Tue, 21 Jan 2025 13:30:17 +0000 Subject: [PATCH 2/5] Unroll symbolic PySNMP indices to numeric ones An `Identifier` was intended to represent an object's MIB name, symbolic object name and numeric OID index. However, due to a misunderstanding of how PySNMP works, this was not always the case: `getMibSymbol` will actually break down the remaining OID index to one or more symbolic indices according to the MIB specification, so the third element of the `getMibSymbol()` call may actually be a tuple of various objects that represents individual indices. While this could be helpful in the codebase, it was not the intention of `_convert_varbind` and `Identifier`. To keep Zino's interface as intended here, this change tries to unroll all the symbolic indices back to a single OID object - likely with some performance penalty. We might want to look into changing the codebase to work with symbolic indexes at a later time. --- src/zino/snmp.py | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/zino/snmp.py b/src/zino/snmp.py index c7cea0682..a4f835f26 100644 --- a/src/zino/snmp.py +++ b/src/zino/snmp.py @@ -5,6 +5,7 @@ import threading from collections import defaultdict from dataclasses import dataclass +from functools import reduce from ipaddress import ip_address from typing import Any, NamedTuple, Sequence, Tuple, Union @@ -499,11 +500,26 @@ def udp_transport_target(self) -> Union[UdpTransportTarget, Udp6TransportTarget] def _convert_varbind(ident: ObjectIdentity, value: ObjectType) -> SNMPVarBind: """Converts a PySNMP varbind pair to an Identifier/value pair""" - mib, obj, row_index = ident.getMibSymbol() + mib, obj, indices = ident.getMibSymbol() value = _mib_value_to_python(value) + row_index = reduce(lambda acc, index: acc + _index_to_tuple(index), indices, ()) return Identifier(mib, obj, OID(row_index)), value +def _index_to_tuple(index: ObjectIdentity) -> Tuple[int, ...]: + """Unrolls a PySNMP index object to a tuple of integers""" + try: + return tuple(index) + except TypeError: + try: + return index.asTuple() + except AttributeError: + try: + return index.getOid() + except AttributeError: + return (int(index),) + + def _mib_value_to_python(value: SupportedTypes) -> Union[str, int, OID]: """Translates various PySNMP mib value objects to plainer Python objects, such as strings, integers or OIDs""" if isinstance(value, univ.Integer): From b9370c3ed24bea85d251bdb7d0241445c36cc076 Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Tue, 21 Jan 2025 13:32:02 +0000 Subject: [PATCH 3/5] Remove `bgpstatemonitortask` workarounds bgpstatemonitortask.py contained some strange workarounds for behaviors that were actually bugs in the `_convert_varbind` implementation. These bugs have been fixed, causing the workarounds to fail: This fixes the failures by removing the workarounds. --- src/zino/tasks/bgpstatemonitortask.py | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/zino/tasks/bgpstatemonitortask.py b/src/zino/tasks/bgpstatemonitortask.py index af5a9d5e5..1796a7488 100644 --- a/src/zino/tasks/bgpstatemonitortask.py +++ b/src/zino/tasks/bgpstatemonitortask.py @@ -158,7 +158,7 @@ async def _get_cisco_bgp_info(self) -> Optional[list[BaseBGPRow]]: return None for oid, result in cisco_bgp_info.items(): - result["cbgpPeer2RemoteAddr"] = ip_address(oid) + result["cbgpPeer2RemoteAddr"] = ip_address(bytes(oid[1:])) cisco_bgp_info = self._transform_variables_from_specific_to_general( bgp_info=cisco_bgp_info, bgp_style=BGPStyle.CISCO @@ -192,15 +192,7 @@ async def _get_bgp_info(self, mib_name: str, variables: Iterable[str]) -> Option max_repetitions=3, ) - cleaned_bgp_info = dict() - - for oid, entry in bgp_info.items(): - if len(oid) == 1: - cleaned_bgp_info[oid[0].prettyPrint()] = entry - else: - cleaned_bgp_info[oid[1].prettyPrint()] = entry - - return cleaned_bgp_info + return bgp_info def _transform_variables_from_specific_to_general( self, bgp_info: SparseWalkResponse, bgp_style: BGPStyle From 29b2441139a4202cab9fae2c1083cc58a559b2e3 Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Tue, 21 Jan 2025 13:45:22 +0000 Subject: [PATCH 4/5] Add news fragment --- changelog.d/389.fixed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/389.fixed.md diff --git a/changelog.d/389.fixed.md b/changelog.d/389.fixed.md new file mode 100644 index 000000000..6a6ab46af --- /dev/null +++ b/changelog.d/389.fixed.md @@ -0,0 +1 @@ +Fixed unintended symbolic translations in the SNMP abstraction layer, and removed now obsolete workarounds for the problem. From 83d6c4e54350139ab08c569298014c434e9c5395 Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Wed, 29 Jan 2025 12:55:23 +0000 Subject: [PATCH 5/5] Fix broken interpretation of CISCO bgp peer table The table index for an address like `10.0.0.42` will be `.1.4.10.0.0.42`, where `1 is the address type and `4` is the address length - while the remainder is the address itself. We don't really need the `1` and the `4` to detect the address type, as `ip_address` will parse things correctly if given just a series of bytes. It turns out that the index unrolling first implemented in `_convert_varbind()` will cause some elements of the `row_index` OID to be left out, causing mayhem for client code that relies on these values. --- src/zino/snmp.py | 20 +++++--------------- src/zino/tasks/bgpstatemonitortask.py | 4 ++-- 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/src/zino/snmp.py b/src/zino/snmp.py index a4f835f26..0e3fd057f 100644 --- a/src/zino/snmp.py +++ b/src/zino/snmp.py @@ -5,7 +5,6 @@ import threading from collections import defaultdict from dataclasses import dataclass -from functools import reduce from ipaddress import ip_address from typing import Any, NamedTuple, Sequence, Tuple, Union @@ -502,22 +501,13 @@ def _convert_varbind(ident: ObjectIdentity, value: ObjectType) -> SNMPVarBind: """Converts a PySNMP varbind pair to an Identifier/value pair""" mib, obj, indices = ident.getMibSymbol() value = _mib_value_to_python(value) - row_index = reduce(lambda acc, index: acc + _index_to_tuple(index), indices, ()) - return Identifier(mib, obj, OID(row_index)), value + prefix = SNMP._oid_to_object_type(mib, obj) + SNMP._resolve_object(prefix) + prefix = OID(prefix[0]) + row_index = OID(ident).strip_prefix(prefix) -def _index_to_tuple(index: ObjectIdentity) -> Tuple[int, ...]: - """Unrolls a PySNMP index object to a tuple of integers""" - try: - return tuple(index) - except TypeError: - try: - return index.asTuple() - except AttributeError: - try: - return index.getOid() - except AttributeError: - return (int(index),) + return Identifier(mib, obj, row_index), value def _mib_value_to_python(value: SupportedTypes) -> Union[str, int, OID]: diff --git a/src/zino/tasks/bgpstatemonitortask.py b/src/zino/tasks/bgpstatemonitortask.py index 1796a7488..c584c39d7 100644 --- a/src/zino/tasks/bgpstatemonitortask.py +++ b/src/zino/tasks/bgpstatemonitortask.py @@ -158,7 +158,8 @@ async def _get_cisco_bgp_info(self) -> Optional[list[BaseBGPRow]]: return None for oid, result in cisco_bgp_info.items(): - result["cbgpPeer2RemoteAddr"] = ip_address(bytes(oid[1:])) + _addr_type, _addr_len, addr = oid[0], oid[1], oid[2:] + result["cbgpPeer2RemoteAddr"] = ip_address(bytes(addr)) cisco_bgp_info = self._transform_variables_from_specific_to_general( bgp_info=cisco_bgp_info, bgp_style=BGPStyle.CISCO @@ -191,7 +192,6 @@ async def _get_bgp_info(self, mib_name: str, variables: Iterable[str]) -> Option *((mib_name, var) for var in variables), max_repetitions=3, ) - return bgp_info def _transform_variables_from_specific_to_general(