Skip to content
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

Fixed Length Strings As Index to Table Includes Length in OID #141

Closed
WakkaTrout opened this issue Nov 2, 2024 · 4 comments
Closed

Fixed Length Strings As Index to Table Includes Length in OID #141

WakkaTrout opened this issue Nov 2, 2024 · 4 comments
Labels
area:pysmi PySMI package bug Something isn't working

Comments

@WakkaTrout
Copy link

WakkaTrout commented Nov 2, 2024

Context:
I received an older proprietary MIB that has a fixed length OCTET string as the index for a table. The MIB does not have the IMPLIED keyword in it, but the length of the OCTET string is fixed. A snippet of the MIB with the names changed is shown below (forgive my inability to format the below in a nice way):

ExampleTextualConv ::= TEXTUAL-CONVENTION
STATUS current
SYNTAX OCTET STRING (SIZE(8))

exampleTable OBJECT-TYPE
SYNTAX SEQUENCE OF exampleTableEntry
MAX-ACCESS not-accessible
STATUS current
::= { exampleOID 1 }

exampleTableEntry OBJECT-TYPE
SYNTAX ExampleTableEntry
MAX-ACCESS not-accessible
STATUS current
INDEX {exampleIndex}
::= { exampleTable 1 }

ExampleTableEntry ::= SEQUENCE
{
exampleIndex ExampleTextualConv
exampleRowStatus RowStatus
}

exampleIndex OBJECT-TYPE
SYNTAX ExampleTextualConv
MAX-ACCESS not-accessible
STATUS current
::= { exampleTableEntry 1 }

exampleRowStatus OBJECT-TYPE
SYNTAX RowStatus
MAX-ACCESS read-create
STATUS current
::= { exampleTableEntry 2 }

From my understanding of how SNMPv2 and later works, if there is a string in an OID that is fixed length, then the length of the string is not included in the SNMP message OID. In the above example, exampleIndex is a fixed length string and therefore, the length of the string is not included in the OID when requesting table rows.

What I am seeing:
In the newest release of pysnmp, 7.1.8, I am seeing the lengths of the string be included in the OID's on wireshark. I ran mibdump on the above mib and used the generated python files.

My Fix
To get it to work, I had to add the isFixedLength() function and the getFixedLength() function to the generated textual convention.

After that, I ran into a bug where there was a name clash as seen below.

return obj.clone(tuple(value[:value])), value[value:]

I fixed the above by just renaming the return 'value' to 'value2'.
return obj.clone(tuple(value[:value2])), value[value2:]
Please correct me if I am doing something incorrectly with mibdump that led me to have to do the above

@dcvmoole
Copy link

dcvmoole commented Nov 2, 2024

Hello! You have done everything correctly, but you have run into a few problems that all need to be fixed.

Originally, it has been pysmi's job to assign a value to the fixedLength attribute of any class for a (simple or TEXTUAL-CONVENTION) type whose subtype is a single size value. Remnants of that can be seen in some of the base MIBs included with pysnmp, for example:

fixedLength = 6
The OctetString class's isFixedLength() and getFixedLength() methods would pick up that attribute value and make it available to the OID parsing and generation code in SNMPv2-SMI.py. However, recently, two three things changed:

  1. An older PEP-8 update broke the code you point out, by accidentally introducing the "value" collision (267333988).
  2. Large structural changes to pysmi (namely, a switch to Jinja2 templates) resulted in fixedLength not being assigned at all in Python code generated for MIBs anymore, resulting in further breakage of this functionality for all MIBs except the pysnmp-included base MIBs.
  3. A more recent PEP-8 update also renamed the use, but not the assignments, of fixedLength to fixed_length, thereby breaking the functionality for the base MIBs as well (4c71d45fa). This is highly similar to the breaking rename of namedValues in the same commit, which will similarly have to be rolled back for now I'm afraid.

As a result, no part of this functionality works anymore now :) and the lack of related unit tests have unfortunately let all that happen, at least until now.

FWIW, just this week I skipped over point 2 above in the context of lextudio/pysmi#8, because I could not find any uses of the fixedLength values assigned in the base MIBs (due to point 3 above) and therefore (mistakenly, without checking) assumed that pysnmp had switched over to making use of subtypeSpec, which is the full set of constraints from which pysnmp could also derive fixed lengths itself. However, the latter would impose more pysnmp run-time overhead, whereas pysmi can easily pre-generate the values to save on such overhead. Therefore, I believe we are best off by restoring this functionality in pysmi.

As such, I think the path forward to resolve this issue as follows:

  • I will resolve point 2 above in pysmi ASAP, so that assignment of fixedLength is restored to how it was in the pre-Jinja2 days. That upcoming change will include new unit tests to ensure that isFixedLength/getFixedLength work as expected, but any functionality built on top of those methods is outside the scope of the pysmi unit tests.
  • Points 1 and 3 will need to be fixed in pysnmp. I will eventually submit a fix for this part, unless someone beats me to it, which I would in fact prefer.
  • pysnmp's OID generation and parsing could really use unit tests as well. How and when those are added, will probably depend on how much effort it is to set up the infrastructure to conduct such tests. As always, making the first unit test of its kind is hard, but adding more afterward is relatively easy. There is overlap with DISPLAY-HINT parsing is not correct #139 in that respect.

@lextm lextm added more info needed We need more information about this issue area:pysmi PySMI package priority:low Low priority items. labels Nov 3, 2024
@lextm
Copy link

lextm commented Nov 3, 2024

Just shipped pysnmp release 7.1.9 and pysmi 1.5.8 that contains more recent changes on MIB parsing.

@WakkaTrout Initial testing shows there is no issue with the latest pysnmp and pysmi. We can close this issue once you confirm on your side.

@WakkaTrout
Copy link
Author

WakkaTrout commented Nov 4, 2024

@lextm This issue is not fixed, I am still getting an "'int' object is not subscriptable" error from this line of code:

return obj.clone(tuple(value[:value])), value[value:]

Additionally, as dcvmoole mentioned, with the PEPT-8 changes in pysnmp, the octet string class has methods named is_fixed_length() and get_fixed_length() but everywhere else in the code still uses the pre-PEPT-8 changes with the name isFixedLength() and getFixedLength(). This will have to be rectified to get this to all work.

I did a little digging for the exact RFC this behavior is defined in. It is RFC 2578 Section 7.7 Bullet 2

@lextudio-support
Copy link

lextudio-support commented Nov 4, 2024

@WakkaTrout The PEP 8 renaming doesn't block anything right now, as rfc1902.py has already contained the compatibility API to remap the methods/properties.

You will have to provide more code to reproduce the remaining issues, as the code above was tested on our side and no longer shows any errors. "'int' object is not subscriptable" should be caused by something else.

We recommend you open a new issue, so that we can start from scratch. The changes in latest pysnmp and pysmi packages already changed the landscape.

@lextm lextm added bug Something isn't working and removed more info needed We need more information about this issue priority:low Low priority items. labels Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:pysmi PySMI package bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants