From 05f5e4c70062135f15ea7810f67911ffdae710fa Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Mon, 27 Nov 2023 15:16:18 +0100 Subject: [PATCH 1/3] Add regression tests for #2767 While all data we have has SNMPv2c profiles store their version number as either the string or the integer 2, some users report that they appear to be stored as the string `2c`, which crashes ipdevpoll's `SNMPParameters.factory()` method. --- tests/unittests/ipdevpoll/snmp/common_test.py | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/unittests/ipdevpoll/snmp/common_test.py b/tests/unittests/ipdevpoll/snmp/common_test.py index f2fae2adbf..cc40a173ab 100644 --- a/tests/unittests/ipdevpoll/snmp/common_test.py +++ b/tests/unittests/ipdevpoll/snmp/common_test.py @@ -1,7 +1,10 @@ +from unittest.mock import Mock + import pytest from nav.ipdevpoll.snmp.common import SNMPParameters from nav.Snmp.defines import AuthenticationProtocol, PrivacyProtocol, SecurityLevel +from nav.models.manage import ManagementProfile class TestSNMPParameters: @@ -41,6 +44,34 @@ def test_should_contain_sec_name_cmdline_argument(self, snmpv3_params): assert "-u foobar" in args +class TestSNMPParametersFactory: + def test_snmp_profile_with_v2c_version_string_should_be_parsed_without_error( + self, snmpv2c_profile + ): + mock_netbox = Mock() + mock_netbox.get_preferred_snmp_management_profile.return_value = snmpv2c_profile + params = SNMPParameters.factory(mock_netbox) + assert params.version == 2 + + def test_snmp_profile_with_v2_version_string_should_be_parsed_without_error( + self, snmpv2c_profile + ): + mock_netbox = Mock() + mock_netbox.get_preferred_snmp_management_profile.return_value = snmpv2c_profile + snmpv2c_profile.configuration["version"] = "2" + params = SNMPParameters.factory(mock_netbox) + assert params.version == 2 + + def test_snmp_profile_with_v2_version_integer_should_be_parsed_without_error( + self, snmpv2c_profile + ): + mock_netbox = Mock() + mock_netbox.get_preferred_snmp_management_profile.return_value = snmpv2c_profile + snmpv2c_profile.configuration["version"] = 2 + params = SNMPParameters.factory(mock_netbox) + assert params.version == 2 + + @pytest.fixture def snmpv3_params(): param = SNMPParameters( @@ -53,3 +84,16 @@ def snmpv3_params(): priv_password="secret2", ) yield param + + +@pytest.fixture +def snmpv2c_profile(): + profile = ManagementProfile( + protocol=ManagementProfile.PROTOCOL_SNMP, + configuration={ + "version": "2c", + "community": "private", + "write": True, + }, + ) + yield profile From feaecfaa76d8e336ce04e45af0c55ed1f8917142 Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Mon, 27 Nov 2023 15:17:56 +0100 Subject: [PATCH 2/3] Use SNMP profiles' own `snmp_version` property `ManagementProfile.snmp_version` already does the heavy lifting for us when it comes to parse the SNMP version value of an SNMP profile, so let's use that instead to avoid issues like the one described in #2767 --- python/nav/ipdevpoll/snmp/common.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/python/nav/ipdevpoll/snmp/common.py b/python/nav/ipdevpoll/snmp/common.py index 795d399519..b17447e1cb 100644 --- a/python/nav/ipdevpoll/snmp/common.py +++ b/python/nav/ipdevpoll/snmp/common.py @@ -204,13 +204,11 @@ def factory( if netbox: profile = netbox.get_preferred_snmp_management_profile() if profile: - if profile.protocol == profile.PROTOCOL_SNMPV3: - kwargs["version"] = 3 kwargs_out.update( {k: v for k, v in profile.configuration.items() if hasattr(cls, k)} ) - # Sometimes profiles store the version number as a string - kwargs_out["version"] = int(kwargs_out["version"]) + # Let profile object parse its own version to an int + kwargs_out["version"] = profile.snmp_version else: _logger.debug("%r has no snmp profile", netbox) return None From 98b67c85802450f5ec30787a657762237f2a5301 Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Mon, 27 Nov 2023 15:36:46 +0100 Subject: [PATCH 3/3] Factor out redundancy from v2c profile tests --- tests/unittests/ipdevpoll/snmp/common_test.py | 24 ++++--------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/tests/unittests/ipdevpoll/snmp/common_test.py b/tests/unittests/ipdevpoll/snmp/common_test.py index cc40a173ab..6b34caffd7 100644 --- a/tests/unittests/ipdevpoll/snmp/common_test.py +++ b/tests/unittests/ipdevpoll/snmp/common_test.py @@ -45,29 +45,13 @@ def test_should_contain_sec_name_cmdline_argument(self, snmpv3_params): class TestSNMPParametersFactory: - def test_snmp_profile_with_v2c_version_string_should_be_parsed_without_error( - self, snmpv2c_profile + @pytest.mark.parametrize("version_value", (2, "2", "2c")) + def test_snmp_v2_profile_should_be_parsed_without_error( + self, snmpv2c_profile, version_value ): mock_netbox = Mock() mock_netbox.get_preferred_snmp_management_profile.return_value = snmpv2c_profile - params = SNMPParameters.factory(mock_netbox) - assert params.version == 2 - - def test_snmp_profile_with_v2_version_string_should_be_parsed_without_error( - self, snmpv2c_profile - ): - mock_netbox = Mock() - mock_netbox.get_preferred_snmp_management_profile.return_value = snmpv2c_profile - snmpv2c_profile.configuration["version"] = "2" - params = SNMPParameters.factory(mock_netbox) - assert params.version == 2 - - def test_snmp_profile_with_v2_version_integer_should_be_parsed_without_error( - self, snmpv2c_profile - ): - mock_netbox = Mock() - mock_netbox.get_preferred_snmp_management_profile.return_value = snmpv2c_profile - snmpv2c_profile.configuration["version"] = 2 + snmpv2c_profile.configuration["version"] = version_value params = SNMPParameters.factory(mock_netbox) assert params.version == 2