Enforce user's security level for received trap messages #75
Conversation
…ntegration-snmp into fix-traps-security-level # Conflicts: # CHANGELOG.md # VERSION
| Testing: AuthPriv
AuthNoPriv
NoAuthNoPriv
Everything appears to work as expected with one caveat: When there are issues with the format of the message - an incorrect privilege passphrase or protocol, for example - the event does not get created, and there is no logging to indicate that an event has been received, and has been dropped. This was super tricky to debug, and required me to attach a debugger to the Logstash process to figure out what was going on. |
| Tried with the logging library - logs are showing up in the log file: |
There was a problem hiding this comment.
Since the snmptrap effectively allows us to have zero or one USM's, this feels like a lot of overhead for a check that could be done in the CommandResponder#processPdu (that already contains logic for dropping non-matching events) in the existing SnmpClient#doTrap.
Is the idea that we want to be able to emit SNMPv3_USM_UNSUPPORTED_SECURITY_LEVEL, and allow SNMP4j to build a rejection response? From what I can see in a fork of SNMP4j, that is used as feedback when a payload includes a security_level that requires additional auth fields without actually providing said auth fields (e.g.,: >= AUTH_NOPRIV missing auth protocol, AUTH_PRIV missing privacy protocol), or a payload requests a level of auth that is not supported by the user (e.g., AUTH_PRIV but USM for the user has no auth or privacy protocols); it appears to be more an indication that the request is malformed than the contents of the payload being non-actionable.
In the contexts of an SNMP trap, I would view security_level as more of a post-receive filter than a pre-receive override to the USM.
Co-authored-by: Rye Biesemeyer <yaauie@users.noreply.github.com>
| I think that things may be a bit conflated here. In the But the While we could use it as a filter, the meaning isn't clear. Is it a minimum threshold? Or are we expecting an exact match? For example, both plugins allow a configuration that has
In both plugins, the In the In the
In this sense, we are expecting the I still think that we should be able to intercept it in the |
| Thank you Ry for digging into this, I saw that maintainer message as well while investigating it, and although I agree that it's not part of the USM responsibility to verify authorization, I don't think we should strictly apply that to us, given we're not providing a SNMP library, and there's no need to be compliant (in terms of responsibilities) with the RFC spec. As I mentioned, the idea of overriding the USM was providing a proper response to the clients, without having to implement a VACM system neither paying for their commercial version. Anyway, It doesn't really matter for traps, considering it's fire-and-forget, but it would be useful for supporting
I don't have a strong opinion on that, and I'm fine changing it as suggested considering we don't officially support INFORM messages. Regarding having a single security_name user in the USM, that's a limitation inherited from the old plugins (unfortunately), and it's not the reality of most SNMP environments where devices might have different credentials (security reasons). There's an open issue for extending that support, so users don't need to run a plugin per credential. I'll push the changes soon. Thanks! |
edmocosta changed the title edmocosta changed the title edmocosta force-pushed the fix-traps-security-level branch from 1d85c8f to fca421d Compare There was a problem hiding this comment.
I apologize for the delay in getting to a re-review.
I'm still concerned that we're not really defining what the security_level means in the context of the snmptrap plugin. It appears to default to noAuthNoPriv when security_name is specified, and it requires a precise match; even if trapped messages match provided auth_* and priv_* config they will be rejected.
I would expect the security_level config to act as a minimum. If a user configures security_name, auth_*, and priv_*, and sets security_level => authNoPriv, I would expect both authNoPriv and matching authPriv events to be emitted.
If we do intend to mandate an exact match, then I would expect not setting security_level equivalent to having no preference (not dropping events that correctly match the provided auth_* and/or priv_* params). Enforcing noAuthNoPriv when security_level is unspecified is a breaking change
| context 'when receiving a request from an unknown user' do | ||
| it 'should not process the message' do | ||
| queue = run_plugin_and_get_queue(plugin, timeout: 3) do | ||
| @trap_sender.send_trap_v3(target_address, "foo", auth_protocol, auth_pass, priv_protocol, priv_pass, 'noAuthNoPriv', {'1.1' => 'foo'}) |
There was a problem hiding this comment.
🤔 we have another test that uses the "correct" security_name with an "invalid security level", and this one has the same security level with an "incorrect" security_name.
We also had situations where providing noAuthNoPriv caused our own code to avoid sending the supplied auth and priv parameters; are we confident that they are supplied here, and that the test is passing for the right reasons?
There was a problem hiding this comment.
Dropped this test as unknown user shouldn't be a thing: #75 (comment)
| logger.debug("Received trap message from an unknown user: '{}'. Skipping", securityName); | ||
| return false; | ||
| } | ||
| if (!Objects.equals(userSecurityLevel, securityLevel)) { |
There was a problem hiding this comment.
Do we require security_level to be an exact match? If a trapped message is authPriv, and matches the auth_* and priv_* params, but security_level is either unspecified (resulting in an effective-default noAuthNoPriv) or authNoPriv, do we reject the trapped event?
This still feels weird to me, and isn't really specified in our docs. From how I read this code to behave I think our docs need a change like:
diff --git a/docs/input-snmptrap.asciidoc b/docs/input-snmptrap.asciidoc index 8874583..e341137 100644 --- a/docs/input-snmptrap.asciidoc +++ b/docs/input-snmptrap.asciidoc @@ -350,11 +350,13 @@ The `priv_protocol` option specifies the SNMPv3 privacy/encryption protocol. [id="plugins-{type}s-{plugin}-security_level"] ===== `security_level` - * Value can be any of: `noAuthNoPriv`, `authNoPriv`, `authPriv` - * There is no default value for this setting + * Value can be any of: + - `noAuthNoPriv`: trapped messages must be neither authenticated nor privatized + - `authNoPriv`: trapped messages must be authenticated according to <<plugins-{type}s-{plugin}-security_name>>/<<plugins-{type}s-{plugin}-auth_protocol>>/<<plugins-{type}s-{plugin}-auth_pass>> but must not be privatized + - `authPriv`: trapped messages must be both authenticated according to <<plugins-{type}s-{plugin}-security_name>>/<<plugins-{type}s-{plugin}-auth_protocol>>/<<plugins-{type}s-{plugin}-auth_pass>> and privatized according to <<plugins-{type}s-{plugin}-priv_protocol>>/<<plugins-{type}s-{plugin}-priv_pass>> + * The default value is `noAuthNoPriv`. -The `security_level` option specifies the SNMPv3 security level between -Authentication, No Privacy; Authentication, Privacy; or no Authentication, no Privacy. +The `security_level` option specifies the _precise_ SNMPv3 security level of trapped messages that will be accepted when <<plugins-{type}s-{plugin}-security_name>> is specified. [id="plugins-{type}s-{plugin}-security_name"] ===== `security_name` There was a problem hiding this comment.
Do we require security_level to be an exact match?
Not a requirement, I've changed it to accept higher security levels as well. I guess it also solves #75 (comment).
3 participants
This PR fixes the
snmptrapinput plugin to correctly enforce the configured user security level set by thesecurity_levelconfig. Received events that do not match the configured value are now dropped.