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

Implemented Snmp v1 and v2c #1020

Open
wants to merge 3 commits into
base: develop
from

Conversation

@Yagoor
Copy link

commented Aug 6, 2019

Dear all,

This is a lightweight implementation of SNMP.
Currently, only the SNMP v1 and v2c are implemented. The SNMP v3 cannot be fully implemented hence why I didn't implement it partially.

Currently, I'm still working on unit tests and documentation. But the core code is already in place and working.

@Yagoor

This comment has been minimized.

Copy link
Author

commented Aug 6, 2019

It would be interesting to know from the community which data types are more relevant. There are several data types on the ASN1.1 that can be used in the SNMP protocol. But I do not see a point in implementing all of them and increasing the code footprint.

I would also like to know if the best approach is to provide by default some MIB implementations, like the IF-MIB or the SYSTEM-MIB.

I'm currently working on a MIB for the Contiki. I already got a Private Enterprise Number from IANA. The Contiki-ng number is 54352. The complete list is here: https://www.iana.org/assignments/enterprise-numbers/enterprise-numbers

@joakimeriksson

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

Cool stuff! Some things I would like and that I saw in the diffs:

  1. A short description of the use cases you would see for this in a 6LoWPAN context (e.g. resource constrained IoT devices setup).

  2. There are a bunch of formatting changes which I do not think should be in this PR. We would like to have the fewest possible diffs possible to make it easy to do the code review and would like to avoid any formatting changes that are not strongly motivated.

@Yagoor

This comment has been minimized.

Copy link
Author

commented Aug 7, 2019

Hi @joakimeriksson

  1. This is what my master thesis is entirely about. There are many scenarios in the WSN context that would be interesting to use the SNMP. It is good to check the sysuptime of a sensor that is deployed in a very remote area, for example, is order to check if it is reaching its lifetime end. Or check the power usage of a sensor that again is in a very remote area. If this sensor should be working on a very unexcpeted scenario, such as a dam breaking alert. You don't want to wait for it to break to realize that the sensor is dead. That is why I only implemented the Get functionalities. The Set would be irrelevant since WSN networks should be able to reconfigure itself. And to Set we would need to implement the SNMP v3 with its authentication securities.

  2. I'll rollback those files that I changed outside the snmp folder. I ran the uncrustify script in all files that I changed therefore those files are not following the coding style.

@Yagoor Yagoor force-pushed the Yagoor:snmp-bsd branch from 94b5004 to 7fd1624 Aug 15, 2019

@Yagoor

This comment has been minimized.

Copy link
Author

commented Aug 15, 2019

I added one example of a SNMP Server using the API and I reverted the formatting changes.
What would be the next natural step?

@g-oikonomou
Copy link
Member

left a comment

This is really cool, very professional-looking contribution and good job on the doxygen.

You will need to include a travis compile test for the platforms you have tested at least. What platforms did you actually test this on?

Some other changes requested, nothing major.

The naming convention is non-standard in snmp-SNMP-MIB-2-System.c but as far as I can understand you are using the exact names as they appear in the spec, so I am happy with what you have done.

Instead of examples/snmp/server I would have used examples/snmp-server unless you are planning to add a client component too (but I see little value in that).

As we discussed on gitter, I'd rather go without SET functionality (at least for now). Perhaps one day over DTLS ;)

I guess the key remaining question is: How do we know this is SNMP-compliant? :) Is there an easy way to test it? It would be really cool to have a test similar to what we have with the native MQTT-client. That is a test where we use some existing tool to send commands to the server and check for the correctness of the replies.

Again, thanks for this!

examples/snmp/server/project-conf.h Outdated Show resolved Hide resolved
os/net/app-layer/snmp/snmp-ber.c Show resolved Hide resolved
os/net/app-layer/snmp/snmp-ber.c Outdated Show resolved Hide resolved
os/net/app-layer/snmp/snmp-engine.c Show resolved Hide resolved
os/net/app-layer/snmp/snmp.h Outdated Show resolved Hide resolved
os/net/app-layer/snmp/snmp.h Show resolved Hide resolved
Initial version of SNMP implementation
Improved SNMP implementation

Fixed defines due to style

SNMP Server example with SNMP-MIB-2 System

Renamed resource

Removed debug info from app-layer

Removed debug info from example

Removed debug info from app-layer

Removed debug info from app-layer

Renamed example

Implemented some changes from PR

@Yagoor Yagoor force-pushed the Yagoor:snmp-bsd branch from f8b2f1b to 1671d3b Sep 3, 2019

@Yagoor

This comment has been minimized.

Copy link
Author

commented Sep 3, 2019

Thank you for the review.

First of all, my history was messy and after implementing the changes you requested I rebased the branch, it should be easier to see the changes as well.

The answer for your questions:

1 - I tested this on the native, cooja and I'll test it in the cc26x0-cc13x0, which is the only physical platform I have.

2 - There is a easy way to test if it is SNMP-compliant. I'll implement the Test and commit it here. I know that it is SNMP compliant now, but I do see the point to have a regression test.

Yagoor added 2 commits Sep 3, 2019
@Yagoor

This comment has been minimized.

Copy link
Author

commented Sep 7, 2019

I created the SNMP compliant test.

Is there any other request I should work on?

@Yagoor Yagoor requested a review from g-oikonomou Sep 10, 2019

@g-oikonomou

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

As far as I can tell we have a single test for SNMP execution and it uses platform native. Right? In that case, I'd suggest moving the test under 08-native-runs. It's OK to add smitools and other packages to tho docker image.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.