On Fri, May 07, 2021 at 04:18:50PM +0200, Martijn van Duren wrote:
> When moving the traphandler to the snmpe process I overlooked the fact
> that "type" is being saved inside the switch statement under the
> sm_context name. RFC3411 talks about pduType, and the name context means
> something completely different in the v3 world.
>
> The diff below moves our naming closer to the RFCs, which should
> hopefully prevent further confusion in the future.
>
> While here I made the debug output print the pduType in a human readable
> format.
>
> The invalid varbind check can be simplified a simple "{}" in the
> ober_scanf_elements allowing me to just drop the type variable.
>
> OK?
I tested it and the diff looks good and legit for me.
> martijn@
>
> Index: snmp.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/snmpd/snmp.h,v
> retrieving revision 1.16
> diff -u -p -r1.16 snmp.h
> --- snmp.h 30 Jun 2020 17:11:49 -0000 1.16
> +++ snmp.h 7 May 2021 14:17:12 -0000
> @@ -77,7 +77,7 @@ enum snmp_version {
> SNMP_V3 = 3
> };
>
> -enum snmp_context {
> +enum snmp_pdutype {
> SNMP_C_GETREQ = 0,
> SNMP_C_GETNEXTREQ = 1,
> SNMP_C_GETRESP = 2,
> Index: snmpd.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/snmpd/snmpd.h,v
> retrieving revision 1.94
> diff -u -p -r1.94 snmpd.h
> --- snmpd.h 5 Feb 2021 10:30:45 -0000 1.94
> +++ snmpd.h 7 May 2021 14:17:12 -0000
> @@ -384,7 +384,7 @@ struct snmp_message {
> socklen_t sm_slen;
> int sm_sock_tcp;
> int sm_aflags;
> - int sm_type;
> + enum snmp_pdutype sm_pdutype;
> struct event sm_sockev;
> char sm_host[HOST_NAME_MAX+1];
> in_port_t sm_port;
> @@ -405,7 +405,6 @@ struct snmp_message {
>
> /* V1, V2c */
> char sm_community[SNMPD_MAXCOMMUNITYLEN];
> - int sm_context;
>
> /* V3 */
> long long sm_msgid;
> Index: snmpe.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v
> retrieving revision 1.70
> diff -u -p -r1.70 snmpe.c
> --- snmpe.c 22 Feb 2021 11:31:09 -0000 1.70
> +++ snmpe.c 7 May 2021 14:17:12 -0000
> @@ -41,6 +41,7 @@
> #include "mib.h"
>
> void snmpe_init(struct privsep *, struct privsep_proc *, void *);
> +const char *snmpe_pdutype2string(enum snmp_pdutype);
> int snmpe_parse(struct snmp_message *);
> void snmpe_tryparse(int, struct snmp_message *);
> int snmpe_parsevarbinds(struct snmp_message *);
> @@ -194,6 +195,36 @@ snmpe_bind(struct address *addr)
> return (-1);
> }
>
> +const char *
> +snmpe_pdutype2string(enum snmp_pdutype pdutype)
> +{
> + static char unknown[sizeof("Unknown (4294967295)")];
> +
> + switch (pdutype) {
> + case SNMP_C_GETREQ:
> + return "GetRequest";
> + case SNMP_C_GETNEXTREQ:
> + return "GetNextRequest";
> + case SNMP_C_GETRESP:
> + return "Response";
> + case SNMP_C_SETREQ:
> + return "SetRequest";
> + case SNMP_C_TRAP:
> + return "Trap";
> + case SNMP_C_GETBULKREQ:
> + return "GetBulkRequest";
> + case SNMP_C_INFORMREQ:
> + return "InformRequest";
> + case SNMP_C_TRAPV2:
> + return "SNMPv2-Trap";
> + case SNMP_C_REPORT:
> + return "Report";
> + }
> +
> + snprintf(unknown, sizeof(unknown), "Unknown (%u)", pdutype);
> + return unknown;
> +}
> +
> int
> snmpe_parse(struct snmp_message *msg)
> {
> @@ -202,7 +233,6 @@ snmpe_parse(struct snmp_message *msg)
> struct ber_element *a;
> long long ver, req;
> long long errval, erridx;
> - unsigned int type;
> u_int class;
> char *comn;
> char *flagstr, *ctxname;
> @@ -271,15 +301,15 @@ snmpe_parse(struct snmp_message *msg)
> goto fail;
> }
>
> - if (ober_scanf_elements(msg->sm_pdu, "t{e", &class, &type, &a) != 0)
> + if (ober_scanf_elements(msg->sm_pdu, "t{e", &class, &(msg->sm_pdutype),
> + &a) != 0)
> goto parsefail;
>
> /* SNMP PDU context */
> if (class != BER_CLASS_CONTEXT)
> goto parsefail;
>
> - msg->sm_type = type;
> - switch (type) {
> + switch (msg->sm_pdutype) {
> case SNMP_C_GETBULKREQ:
> if (msg->sm_version == SNMP_V1) {
> stats->snmp_inbadversions++;
> @@ -294,7 +324,7 @@ snmpe_parse(struct snmp_message *msg)
> /* FALLTHROUGH */
>
> case SNMP_C_GETNEXTREQ:
> - if (type == SNMP_C_GETNEXTREQ)
> + if (msg->sm_pdutype == SNMP_C_GETNEXTREQ)
> stats->snmp_ingetnexts++;
> if (!(msg->sm_aflags & ADDRESS_FLAG_READ)) {
> msg->sm_errstr = "read requests disabled";
> @@ -308,7 +338,6 @@ snmpe_parse(struct snmp_message *msg)
> msg->sm_errstr = "wrong read community";
> goto fail;
> }
> - msg->sm_context = type;
> break;
>
> case SNMP_C_SETREQ:
> @@ -327,7 +356,6 @@ snmpe_parse(struct snmp_message *msg)
> msg->sm_errstr = "wrong write community";
> goto fail;
> }
> - msg->sm_context = type;
> break;
>
> case SNMP_C_GETRESP:
> @@ -341,7 +369,7 @@ snmpe_parse(struct snmp_message *msg)
> goto parsefail;
> }
> case SNMP_C_TRAPV2:
> - if (type == SNMP_C_TRAPV2 &&
> + if (msg->sm_pdutype == SNMP_C_TRAPV2 &&
> !(msg->sm_version == SNMP_V2 ||
> msg->sm_version != SNMP_V3)) {
> msg->sm_errstr = "trapv2 request on !SNMPv2C or "
> @@ -370,25 +398,19 @@ snmpe_parse(struct snmp_message *msg)
> goto fail;
> /* Shortcircuit */
> return 0;
> -
> default:
> msg->sm_errstr = "invalid context";
> goto parsefail;
> }
>
> /* SNMP PDU */
> - if (ober_scanf_elements(a, "iiie{et}$",
> + if (ober_scanf_elements(a, "iiie{e{}}$",
> &req, &errval, &erridx, &msg->sm_pduend,
> - &msg->sm_varbind, &class, &type) != 0) {
> + &msg->sm_varbind) != 0) {
> stats->snmp_silentdrops++;
> msg->sm_errstr = "invalid PDU";
> goto fail;
> }
> - if (class != BER_CLASS_UNIVERSAL || type != BER_TYPE_SEQUENCE) {
> - stats->snmp_silentdrops++;
> - msg->sm_errstr = "invalid varbind";
> - goto fail;
> - }
>
> msg->sm_request = req;
> msg->sm_error = errval;
> @@ -396,16 +418,18 @@ snmpe_parse(struct snmp_message *msg)
>
> print_host(ss, msg->sm_host, sizeof(msg->sm_host));
> if (msg->sm_version == SNMP_V3)
> - log_debug("%s: %s:%hd: SNMPv3 context %d, flags %#x, "
> + log_debug("%s: %s:%hd: SNMPv3 pdutype %s, flags %#x, "
> "secmodel %lld, user '%s', ctx-engine %s, ctx-name '%s', "
> "request %lld", __func__, msg->sm_host, msg->sm_port,
> - msg->sm_context, msg->sm_flags, msg->sm_secmodel,
> - msg->sm_username, tohexstr(msg->sm_ctxengineid,
> - msg->sm_ctxengineid_len), msg->sm_ctxname, msg->sm_request);
> + snmpe_pdutype2string(msg->sm_pdutype), msg->sm_flags,
> + msg->sm_secmodel, msg->sm_username,
> + tohexstr(msg->sm_ctxengineid, msg->sm_ctxengineid_len),
> + msg->sm_ctxname, msg->sm_request);
> else
> - log_debug("%s: %s:%hd: SNMPv%d '%s' context %d request %lld",
> + log_debug("%s: %s:%hd: SNMPv%d '%s' pdutype %s request %lld",
> __func__, msg->sm_host, msg->sm_port, msg->sm_version + 1,
> - msg->sm_community, msg->sm_context, msg->sm_request);
> + msg->sm_community, snmpe_pdutype2string(msg->sm_pdutype),
> + msg->sm_request);
>
> return (0);
>
> @@ -451,7 +475,7 @@ snmpe_parsevarbinds(struct snmp_message
> * XXX intotalreqvars should only be incremented after all are
> * succeeded
> */
> - switch (msg->sm_context) {
> + switch (msg->sm_pdutype) {
> case SNMP_C_GETNEXTREQ:
> if ((rvarbind = ober_add_sequence(NULL)) == NULL)
> goto varfail;
> @@ -766,8 +790,8 @@ snmpe_recvmsg(int fd, short sig, void *a
> void
> snmpe_dispatchmsg(struct snmp_message *msg)
> {
> - if (msg->sm_type == SNMP_C_TRAP ||
> - msg->sm_type == SNMP_C_TRAPV2) {
> + if (msg->sm_pdutype == SNMP_C_TRAP ||
> + msg->sm_pdutype == SNMP_C_TRAPV2) {
> snmp_msgfree(msg);
> return;
> }
> @@ -776,7 +800,7 @@ snmpe_dispatchmsg(struct snmp_message *m
> (void) snmpe_parsevarbinds(msg);
>
> /* respond directly */
> - msg->sm_context = SNMP_C_GETRESP;
> + msg->sm_pdutype = SNMP_C_GETRESP;
> snmpe_response(msg);
> }
>
> @@ -888,7 +912,7 @@ snmpe_encode(struct snmp_message *msg)
> }
>
> if (!ober_printf_elements(epdu, "tiii{e}", BER_CLASS_CONTEXT,
> - msg->sm_context, msg->sm_request,
> + msg->sm_pdutype, msg->sm_request,
> msg->sm_error, msg->sm_errorindex,
> msg->sm_varbindresp)) {
> ober_free_elements(pdu);
> Index: usm.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/snmpd/usm.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 usm.c
> --- usm.c 22 Feb 2021 11:31:09 -0000 1.18
> +++ usm.c 7 May 2021 14:17:14 -0000
> @@ -548,7 +548,7 @@ usm_make_report(struct snmp_message *msg
> {
> struct ber_oid usmstat = OID(MIB_usmStats, 0, 0);
>
> - msg->sm_context = SNMP_C_REPORT;
> + msg->sm_pdutype = SNMP_C_REPORT;
> usmstat.bo_id[OIDIDX_usmStats] = msg->sm_usmerr;
> usmstat.bo_n = OIDIDX_usmStats + 2;
> if (msg->sm_varbindresp != NULL)
>
>
