Support multiple security parameters for receiving SNMP V3 traps #457

zoedt · 2023-12-07T19:27:48Z

Greetings 😄 PR addressing issue #302

This PR adds multiple security parameter support for receiving SNMP V3 traps.

Notes:

For the USM model, the identifier for the security parameters to unmarshal with will be the username.
This is supporting that the configuration of multiple security parameters in the conf.yaml may have usernames with the same identifier, so multiple security parameters can be tried until one satisfies the authentication/privacy protocols

Example steps

Run the listener in trapserver_v3
Send a trap, examples below:

$ snmptrap -v3 -l authPriv -u myuser2 -a MD5 -A mypassword2 -x AES -X myprivacy2 127.0.0.1:9162 '' 1.3.6.1.4.1.8072.2.3.0.1 1.3.6.1.4.1.8072.2.3.2.1 i 123456

$ snmptrap -v3 -l authPriv -u myuser2 -a SHA -A mypassword2 -x DES -X myprivacy2 127.0.0.1:9162 '' 1.3.6.1.4.1.8072.2.3.0.1 1.3.6.1.4.1.8072.2.3.2.1 i 123456

…sernames Signed-off-by: zoe ✨ <9274242+zoedt@users.noreply.github.com>

…ching Signed-off-by: zoe ✨ <9274242+zoedt@users.noreply.github.com>

Signed-off-by: zoe ✨ <9274242+zoedt@users.noreply.github.com>

trap.go

trap_test.go

Signed-off-by: zoe ✨ <9274242+zoedt@users.noreply.github.com>

Hipska

Oh, now I see why UnmarshalTrapBase was returning the result. So what if instead of passing result as second parameter, you pass the secParams?

What do you think of my suggestions? I also think UnmarshalTrapBase does not need to be exported, just like the one for headers also isn't.

trap.go

examples/trapserver_v3/main.go

zoedt · 2023-12-12T21:13:32Z

cc @SuperQ @TimRots for any feedback as well

…ms, clearer names Signed-off-by: zoe ✨ <9274242+zoedt@users.noreply.github.com>

TimRots

Hi @zoedt , thank you for contributing this feature. Initial review looks great to me!
I dropped one implementation specific question for discussion and look forward to reviewing the rest of this feature soon.

TimRots · 2023-12-13T23:00:04Z

v3_map.go

+	}
+}
+
+func (spm *SnmpV3SecurityParametersTable) Add(key string, sp SnmpV3SecurityParameters) error {


Current implementation of this Add method allows for multiple UsmSecurityParameters to be associated with the same userName. This approach seems to offer some interesting flexibility. However, I'm keen to understand the specific scenarios or requirements that led to this design choice.

In light of the specifications outlined in RFC 3414, particularly the one-to-one relationship between userName and securityName, The RFC suggests that each userName should correspond to a unique set of security parameters. Were you aware of this?

If all reviewers eventually decide this behavior is OK we could perhaps log the specific occurrence where we append SP's to an existing userName, but lets not get ahead of things yet.

i did know about the specifications outlined in rfc 3414 - for my use case (for my team) we wanted to reduce the overhead of requiring those wanting to set up a listener of traps for multiple devices (and thus multiple securityNames/engine IDs) and reducing friction that way.

if we followed the unique set of security parameters per key of username/security name it would definitely not need the for loop to try each credential (as there would only be one possible one)

there could be the possibility of supporting that by changing the getIdentifier to return the username/securityName combo to follow the RFC guidelines, but maybe we can discuss further if we want to support both or have a strong opinion (for strictly following the RFC or the current path forward)

also (a tangent), i was curious about the gosnmp struct's contextEngineId - what should expected to be populated here? i might have missed something but i don't see it used very often beyond when snmp packets are made / should the informs be using that to discover the engine ID?

after talking w/ a coworker: maybe there can be some configuration based on however the user wants to set up:

if a security parameter is added with the authoritative engine ID then getIdentifier will return both username / security name

otherwise, it will defer to only adding the key as the username

for flexibility between both.

what do you think?

Sorry for slow replies. I'm sick and not really up for code(review) at this very moment but let's try:

My main point was that I'm a proponent to be verbose when appending multiple security parameters for the same userName, As that behavior is not specifically outlined in the RFC as best-practice nor requirement. Please log the occurrence.

What confuses me about the current implementation of the UsmSecurityParameters struct is that we seem to mix some Engine and User configs. This was never a problem but becomes confusing now that we allow multiple sec params to be defined by this code. Also the fact that we need to define 4 loggers in the example code convinces me that this implementation needs some care. If this would block your PR it would need to become a TODO item so that we can think and discuss this in a separate issue?

@SuperQ @Hipska what do you think about last mentioned item?

I have no further opinion on this.

I addressed some of the logging concerns with my latest commit, let me know your thoughts on that aspect of your feedback!

As for the mixing of engine / user configs - I just want to clarify: that would be around the engine ID, engine boots, engine time and username (as well as if they've configured authentication / privacy protocols)?

trap.go

marshal.go

v3_map.go

…params Signed-off-by: zoe ✨ <9274242+zoedt@users.noreply.github.com>

zoedt · 2023-12-18T17:02:18Z

Question:

I feel as though it’s confusing to only utilize the security parameters for storing the engine ID in the case of multiple listeners, should there be an explicit field utilized and should it be contextEngineID or a more explicit snmpEngineID (that can be added)?

More background + defining the terms I’m using because I’m not sure if I’m using them correctly:

SNMP manager

sends requests (polling) or receives traps from devices on the network via SNMP agents to get information about them

SNMP agent

runs on devices connected to the network the manager is on; receives the requests, returns information in response to manager

Engine ID

Currently: we use the singular SecurityParameters on the gosnmp struct to hold the engine ID of the host (SNMP manager) it’s running on
- [reference 1: Sending the SNMP engine ID for discovery] relevant PR
  - Example of where it’s being utilized, how the changes look in the PR following this logic
With the introduction of multiple security parameters for traps, that blurs the understanding of what should be utilized to be the definitive engine ID that should represent the “device” or unique identifier for the manager/listener
- The engine ID could be added to each SecurityParameter in the table/map and updated code to pass the original to be able to reference the engine ID
However: The gosnmp struct has a ContextEngineID
- ContextEngineID can represent the SNMP engine with a particular context name, though it seems confusing since it should be grouped together with a scopedPDU (which may not make sense in this situation?)
- When an SnmpPacket is made, the gosnmp’s context engine ID is used (i.e. sending a packet out)
  - This makes me think it should be representing the SNMP engine ID / unique identifier of the manager that it’s running on

zoedt · 2024-01-03T15:37:01Z

@TimRots @SuperQ hi! just wanted to bump this with a question: do we feel like the PR's main logic and additions are generally okay / will be accepted with some more feedback or tweaks?

my team is hoping to utilize this logic and we would like to understand if we use it as it is right now that it won't be something we'd have to maintain ourselves if this PR doesn't actually make it in

thank you!!

Signed-off-by: zoe ✨ <9274242+zoedt@users.noreply.github.com>

SuperQ

LGTM

zoedt · 2024-01-16T14:28:07Z

@TimRots @SuperQ thank you both!!!

* [CHANGE] Refactor netsnmp playback function to use an io.Reader #459 * [FEATURE] Support multiple security parameters for receiving SNMP V3 traps #457 * [ENHANCEMENT] netsnmp tests: tame overzealous file / dir permissions #458 Also update missing v1.37.0 changelog entries. Signed-off-by: SuperQ <superq@gmail.com>

…nmp#457) * First iteration of adding user security params map, assuming unique usernames * Log when new SP is added to map, add logger to map --------- Signed-off-by: zoe ✨ <9274242+zoedt@users.noreply.github.com>

zoedt added 4 commits December 7, 2023 15:24

First iteration of adding user security params map, assuming unique u…

96b2b25

…sernames Signed-off-by: zoe ✨ <9274242+zoedt@users.noreply.github.com>

First iteration of multiple users, attempt multiple sec params if mat…

03abdfa

…ching Signed-off-by: zoe ✨ <9274242+zoedt@users.noreply.github.com>

Add unmarshal trap test for multiple users

46e8ad6

Signed-off-by: zoe ✨ <9274242+zoedt@users.noreply.github.com>

Add mutex for multiple sec params, refactoring

303d12b

Signed-off-by: zoe ✨ <9274242+zoedt@users.noreply.github.com>

zoedt force-pushed the multiple-users branch from 80c09f5 to 303d12b Compare December 7, 2023 20:24

Hipska suggested changes Dec 8, 2023

View reviewed changes

zoedt force-pushed the multiple-users branch from 88fdf45 to 87778a7 Compare December 8, 2023 17:03

Fix errors, clean up based on feedback

1a07075

Signed-off-by: zoe ✨ <9274242+zoedt@users.noreply.github.com>

zoedt force-pushed the multiple-users branch from 87778a7 to 1a07075 Compare December 8, 2023 17:13

Hipska reviewed Dec 8, 2023

View reviewed changes

trap.go Outdated Show resolved Hide resolved

trap.go Outdated Show resolved Hide resolved

trap.go Outdated Show resolved Hide resolved

trap.go Outdated Show resolved Hide resolved

Hipska reviewed Dec 11, 2023

View reviewed changes

examples/trapserver_v3/main.go Outdated Show resolved Hide resolved

zoedt marked this pull request as ready for review December 12, 2023 21:13

Init sec params when add to table, refactor passing version, sec para…

5bc0a02

…ms, clearer names Signed-off-by: zoe ✨ <9274242+zoedt@users.noreply.github.com>

zoedt force-pushed the multiple-users branch from c67261b to 5bc0a02 Compare December 13, 2023 19:19

TimRots reviewed Dec 13, 2023

View reviewed changes

Hipska suggested changes Dec 14, 2023

View reviewed changes

trap.go Outdated Show resolved Hide resolved

trap.go Show resolved Hide resolved

marshal.go Outdated Show resolved Hide resolved

v3_map.go Outdated Show resolved Hide resolved

v3_map.go Outdated Show resolved Hide resolved

Remove unneeded packet, copyright docs and reorder version fn return …

83a06ce

…params Signed-off-by: zoe ✨ <9274242+zoedt@users.noreply.github.com>

zoedt force-pushed the multiple-users branch from a1ad732 to 83a06ce Compare December 18, 2023 15:31

Hipska approved these changes Dec 18, 2023

View reviewed changes

zoedt requested a review from TimRots December 18, 2023 17:06

zoedt mentioned this pull request Dec 18, 2023

[snmp/traps] Update to populate multiple V3 users in gosnmp trap listener DataDog/datadog-agent#21635

Merged

10 tasks

Log when new SP is added to map, add logger to map

5bacf5d

Signed-off-by: zoe ✨ <9274242+zoedt@users.noreply.github.com>

zoedt force-pushed the multiple-users branch from 8c2ad88 to 5bacf5d Compare January 12, 2024 18:38

TimRots approved these changes Jan 15, 2024

View reviewed changes

TimRots requested a review from SuperQ January 15, 2024 11:50

SuperQ approved these changes Jan 15, 2024

View reviewed changes

SuperQ merged commit db0c093 into gosnmp:master Jan 15, 2024

zoedt deleted the multiple-users branch January 16, 2024 15:56

zoedt mentioned this pull request Jan 16, 2024

[snmp/traps] Update gosnmp to latest version DataDog/datadog-agent#22088

Merged

10 tasks

SuperQ mentioned this pull request Aug 7, 2024

Release v1.38.0 #476

Merged

Support multiple security parameters for receiving SNMP V3 traps #457

Support multiple security parameters for receiving SNMP V3 traps #457

Uh oh!

Conversation

zoedt commented Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes:

Example steps

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Hipska left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zoedt commented Dec 12, 2023

Uh oh!

TimRots left a comment

Choose a reason for hiding this comment

Uh oh!

TimRots Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

zoedt Dec 14, 2023

Choose a reason for hiding this comment

Uh oh!

zoedt Dec 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoedt Dec 14, 2023

Choose a reason for hiding this comment

Uh oh!

TimRots Jan 10, 2024

Choose a reason for hiding this comment

Uh oh!

Hipska Jan 11, 2024

Choose a reason for hiding this comment

Uh oh!

zoedt Jan 12, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zoedt commented Dec 18, 2023

Question:

More background + defining the terms I’m using because I’m not sure if I’m using them correctly:

Engine ID

Uh oh!

zoedt commented Jan 3, 2024

Uh oh!

SuperQ left a comment

Choose a reason for hiding this comment

Uh oh!

zoedt commented Jan 16, 2024

Uh oh!

Uh oh!

zoedt commented Dec 7, 2023 •

edited

Loading

zoedt Dec 14, 2023 •

edited

Loading