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

Resolve incoming trap varbinds properly #290

Merged
merged 8 commits into from
Jul 5, 2024

Conversation

lunkwill42
Copy link
Member

@lunkwill42 lunkwill42 commented Jul 4, 2024

Scope and purpose

The varbinds of an incoming trap message were incorrectly resolved (most likely due to a misunderstanding of PySNMP complexity).

In fact, the original resolver only resolved the name of the incoming variable, but the subtype of the value also needs to be resolved according to the MIB definition of the incoming variable.

This new resolver ensures the value is also resolved to the correct type. An example: The incoming raw value may be an ASN1 OctetString, but seen in light of the resolved MIB object name, it might actually be resolved as a InetAddress textual convention type instead (which would heavily influence how the incoming value is parsed to a Python value).

This issue was discovered during implementation of #288 as incoming BGP operational state values were not translated to names, but stayed as opaque integers

This pull request

  • changes the internal implementation of the trap varbind resolver, but no external usage is affected (except for improved ability to resolve incoming value types according to MIBs)

Contributor Checklist

Every pull request should have this checklist filled out, no matter how small it is.
More information about contributing to Zino can be found in the
README.

  • Added a changelog fragment for towncrier
  • Added/amended tests for new/changed code
  • Added/changed documentation
  • Linted/formatted the code with black, ruff and isort, easiest by using pre-commit
  • The first line of the commit message continues the sentence "If applied, this commit will ...", starts with a capital letter, does not end with punctuation and is 50 characters or less long. See https://cbea.ms/git-commit/
  • If applicable: Created new issues if this PR does not fix the issue completely/there is further work to be done

The varbinds of an incoming trap message were incorrectly resolved (most
likely due to a misunderstanding of PySNMP complexity).

In fact, the original resolver only resolved the name of the incoming
variable, but the subtype of the value also needs to be resolved
according to the MIB definition of the incoming variable.

This new resolver ensures the value is also resolved to the correct
type. An example: The incoming raw value may be an ASN1 OctetString,
but seen in light of the resolved MIB object name, it might actually
be resolved as a InetAddress textual convention type instead (which
would heavily influence how the incoming value is parsed to a Python
value).
@lunkwill42 lunkwill42 self-assigned this Jul 4, 2024
Copy link

github-actions bot commented Jul 4, 2024

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ PYTHON black 6 0 0.6s
✅ PYTHON isort 6 0 0.26s
✅ PYTHON ruff 6 0 0.01s

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

@lunkwill42 lunkwill42 marked this pull request as ready for review July 4, 2024 14:18
Copy link

github-actions bot commented Jul 4, 2024

Test results

    3 files      3 suites   59s ⏱️
  416 tests   416 ✅ 0 💤 0 ❌
1 248 runs  1 246 ✅ 2 💤 0 ❌

Results for commit 42cbb27.

♻️ This comment has been updated with latest results.

Copy link

codecov bot commented Jul 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.90%. Comparing base (cfef3d8) to head (42cbb27).
Report is 11 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #290      +/-   ##
==========================================
+ Coverage   98.89%   98.90%   +0.01%     
==========================================
  Files          51       52       +1     
  Lines        6912     6972      +60     
==========================================
+ Hits         6835     6895      +60     
  Misses         77       77              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Base automatically changed from bugfix/bgp-peer-session-optionalize to master July 5, 2024 05:45
@lunkwill42 lunkwill42 added the bug Something isn't working label Jul 5, 2024
Add trap observer class for Juniper BGP traps
Copy link

sonarcloud bot commented Jul 5, 2024

@lunkwill42 lunkwill42 merged commit eadfb12 into master Jul 5, 2024
11 checks passed
@lunkwill42
Copy link
Member Author

No comments received in an IRL code review within our team, so I'm merging this now.

@lunkwill42 lunkwill42 deleted the bugfix/trap-varbind-resolve branch July 5, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant