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

Net-SNMP: Issue 129 #24

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ashraful-islam
Copy link

As part of this issue/initiate: markabrahams/node-net-snmp#129
I have decided to re-create the library in typescript entirely.

Also, except introducing the chai(for assert) I didn't change the tests entirely.
Every test appears to pass as is.

Please review at your leisure and leave some feedbacks :)

@ashraful-islam ashraful-islam mentioned this pull request May 29, 2023
@markabrahams
Copy link
Owner

Thanks for this @ashraful-islam - that's quite a refactor! :-)

A couple of initial thoughts on this:

(1) I think it would be best to maintain the same export structure for backwards compatibility (even though it's a bit weird doubling up Reader and Writer at two different levels). The current structure is:

{
    Ber: { <the export you have now> }
    BerReader: Reader,
    BerWriter: Writer
}

(2) The node-net-snmp library needs to work against this, and my initial tests suggest that the community-based message parsing works OK with this new code, and noAuthNoPriv v3 also works. However, authNoPriv and authPriv v3 give an error message:
ResponseInvalidError: Wrong Digest (incorrect password, community or key)
so this needs to be fixed.

@ashraful-islam
Copy link
Author

@markabrahams Hey, thanks for taking the time to look into it.

So, my todo for now is:

  • fix the export to maintain backward compatibility
  • check this PR w/ net-snmp to debug the authNoPriv & authPriv(v3) issues.

I will update this branch with these changes and fix for the second point soon. 😃

@ashraful-islam ashraful-islam marked this pull request as draft May 30, 2023 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants