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: add SNMPv3 options and logging #25178

Closed
wants to merge 3 commits into from

Conversation

Chris1189
Copy link

Maintainer: @stintel
Compile tested: x86_64, Openwrt 23.05
Run tested: x86_64, Openwrt 23.05

Description:
Openssl is needed to implement encryption and authentification for SNMPv3,
therefore the Makefile is modified for that purpose.
Also snmpV3 prerequisites are added to snmpd.init file and the possibility
to log messages to syslog or a log file.

At last the deprecated option to specifiy a port at snmpd_trap_hostname_add()
and snmpd_trap_ip_add() is removed.

@Chris1189 Chris1189 force-pushed the pr/net-snmp branch 3 times, most recently from 09b46e5 to e389392 Compare October 25, 2024 05:33
@stintel
Copy link
Member

stintel commented Oct 25, 2024

Please make this optional. Enabling this unconditionally would result in my images being too large for my APs with 16MiB flash.

@stintel
Copy link
Member

stintel commented Oct 26, 2024

Also, I would not remove that deprecated setting just like that. Maybe make the init script very verbose about it being deprecated and removed in X time. Both in stdout and using logger. People might be using it, and possibly breaking existing configs without any warning is something I really cannot tolerate.

@Chris1189
Copy link
Author

I will look into your suggestions on Monday.
You mean to make compilation with openssl optional, do you?
I am going to test if snmpv3 is possible then.

I understand your point with the port topic and I can undo that.
net-snmp logs the deprecation (as far as I remember this is how I became aware of it).

Thank you for the review!

@stintel
Copy link
Member

stintel commented Oct 26, 2024

You could have a look at the lldp package for inspiration - it has optional snmp support.

@Chris1189
Copy link
Author

Chris1189 commented Nov 6, 2024

@stintel : I am still in the process of making this optional.
So far I have followed your advice with the lldp package and changed it accordingly.
Nevertheless I now have the problem that I wanted to prepare net-snmp to add also a PR to luci-app-snmpd package.

In this other contribution I am preparing, I need openssl for SNMPv3.
If it is optional in net-snmp some features will only work partially.

I have now three options:

  1. Write with sed to snmpd.init and adapt to it in the other package. But it is not "clean"
  2. I test the compiled net-snmp with ldd (if installed) and also inactivate the snmpd feature in net-snmp. And adapt the other package accordingly. This is also not a "clean" option in my opinion.
  3. I refactor net-snmp in a way that it has a "tiny" compile unit and a "full" one with openssl. This is my favorite.

Which one of these three options do you, as the maintainer, prefer?

@Chris1189 Chris1189 force-pushed the pr/net-snmp branch 4 times, most recently from 8f8a77d to f00cb28 Compare November 6, 2024 19:51
@Chris1189 Chris1189 changed the title net-snmp: remove deprecated port setting and add SNMPv3 options and logging net-snmp: add SNMPv3 options and logging Nov 6, 2024
@Chris1189 Chris1189 marked this pull request as draft November 6, 2024 20:17
@Chris1189 Chris1189 force-pushed the pr/net-snmp branch 2 times, most recently from 4d967f5 to 7993a4e Compare November 7, 2024 07:57
@Chris1189 Chris1189 marked this pull request as ready for review November 7, 2024 08:04
@Chris1189 Chris1189 marked this pull request as draft November 7, 2024 08:04
@Chris1189 Chris1189 force-pushed the pr/net-snmp branch 2 times, most recently from 3a71f50 to 2319948 Compare November 7, 2024 08:26
@Chris1189 Chris1189 marked this pull request as ready for review November 7, 2024 08:34
@stintel
Copy link
Member

stintel commented Nov 20, 2024

I have now three options:

1. Write with `sed` to snmpd.init and adapt to it in the other package. But it is not "clean"

2. I test the compiled net-snmp with `ldd` (if installed) and also inactivate the snmpd feature in net-snmp. And adapt the other package accordingly. This is also not a "clean" option in my opinion.

3. I refactor net-snmp in a way that it has a "tiny" compile unit and a "full" one with openssl. This is my favorite.

Actually I was thinking about it the other day. At some point I made miniupnpd-iptables and miniupnpd-nftables variants (7fbc5d4). That sounds like a good solution here indeed. I would avoid using tiny and full variants, instead, I'd go for -ssl and -nossl variants, like mosquitto.

@Chris1189
Copy link
Author

Thank you for your input. I will work further in that direction.

This commit integrates the option
to add openssl to net-snmp.
This way SNMP V3 can be modified

Signed-off-by: Christian Korber <ckorber@tdt.de>
Christian Korber added 2 commits November 25, 2024 10:20
This commit implements SNMPv3 functionality
to snmpd.init.
In particular it adds function snmpd_snmpdv3_add,
which sets the needed options in /var/run/snmpd.conf.
Additionally a possibility to download mib file
is also added.

Signed-off-by: Christian Korber <ckorber@tdt.de>
This commit adds logging to syslog and to a logfile.

Signed-off-by: Christian Korber <ckorber@tdt.de>
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