-
Notifications
You must be signed in to change notification settings - Fork 634
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
Allow snmp_context to be passed as optional parameter to override the snmp.ContextName #845
Conversation
This could be used to e.g. scrape the full table on a router, DoSing
monitoring or even the whole machine.
This is why scrape config needs to be static.
I am unsure if gating this behind a "dangerous" flag or so would be
defensible, though my gut is no.
Sent by mobile; please excuse my brevity.
…On Tue, Feb 14, 2023, 19:01 dszarmach ***@***.***> wrote:
snmp?target=&module= will use existing c.Auth.ContextName from snmp.yml as
g.ContextName if defined (same as previous behavior)
snmp?target=&module=&snmp_context=<context_name> allows for adding or
overriding this value by passing it as a job param similar to target and
module values.
this allows for simple automated configuration of jobs to scrape
individual contexts without needing to define a new entry in snmp.yml for
each
@SuperQ <https://github.com/SuperQ> @RichiH <https://github.com/RichiH>
------------------------------
You can view, comment on, or merge this pull request online at:
#845
Commit Summary
- cc28509
<cc28509>
add snmp_context as optional param
- b7c7c00
<b7c7c00>
dont change config - just use the context name if passed
- 91d55de
<91d55de>
restore blank line
- bfcac79
<bfcac79>
remove context value from log entry
File Changes
(3 files <https://github.com/prometheus/snmp_exporter/pull/845/files>)
- *M* collector/collector.go
<https://github.com/prometheus/snmp_exporter/pull/845/files#diff-bacda4196256b80bde6a1f6a6cc19d08e3309a4418a179ac4a730135bf950825>
(11)
- *M* config/config.go
<https://github.com/prometheus/snmp_exporter/pull/845/files#diff-fe44f09c4d5977b5f5eaea29170b6a0748819c9d02271746a20d81a5f3efca17>
(9)
- *M* main.go
<https://github.com/prometheus/snmp_exporter/pull/845/files#diff-2873f79a86c0d8b3335cd7731b0ecf7dd4301eb19a82ef7a1cba7589b5252261>
(11)
Patch Links:
- https://github.com/prometheus/snmp_exporter/pull/845.patch
- https://github.com/prometheus/snmp_exporter/pull/845.diff
—
Reply to this email directly, view it on GitHub
<#845>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFYII6O3EJMVTBXIUECKM3WXPB7LANCNFSM6AAAAAAU34ABGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@RichiH wouldn't that only be a concern if someone was already re-using snmp v3 credentials across contexts improperly? A properly configured system aiming to do it securely would not have a single user with permissions to various contexts that were intended to be separated, so any attempt to redirect and re-use the credentials from "module_a,context_a" to "module_a,context_b" via snmp_exporter?snmp call would just generate an authentication failure, right? |
I must have misunderstood this PR on mobile, sorry! This seems fine, though it should probably be gated on being used with SNMPv3 only. The question is if the scrape should fail if there's a context for a v1/v2 auth block. @bastischubert offered to test the PR against v3 hardware, I currently don't have access to any. |
@dszarmach could you sign the DCO, please? |
eb10ef6
to
bcf99ca
Compare
Signed-off-by: prombot <[email protected]> Signed-off-by: Douglas <[email protected]>
Signed-off-by: Douglas <[email protected]>
Signed-off-by: Douglas <[email protected]>
Signed-off-by: Douglas <[email protected]>
Signed-off-by: Douglas <[email protected]>
Done DCO |
@bastischubert if you happen to be testing against cisco nxos for bgp: add a mapping of context name to vrf like: snmp-server context bgp_vrf_1 vrf bgp_vrf_1 then you should see the following behavior: snmp?module=bgp_v3&target=device_name = will return all bgp info not in a VRF I have >200 contexts running this way in a production environment for the past 2 months without issue. |
@dszarmach
using go build it produces a binary that works as expected @dszarmach: could you fix the formatting, after that i don't see a reason to not merge that PR |
Signed-off-by: Douglas <[email protected]>
done |
LGTM @RichiH could you or someone else from the team have also have a look (failing circleci test) and get this one merged? |
I don't think we actually want to do this. Passing arbitrary strings into SNMP is dangerous. We're refactoring the SNMP configuration in #859 so that SNMPv3 Context can be configured as part of the |
@SuperQ the reason I made this was a scale problem in addition to convenience. Having to create a new entry for every SNMP context for every module in snmp config file pushed me up against a failure eventually - something like "excessive aliasing" in the YML I think was the error. Without aliasing the snmp config file balloons to hundreds of megs and performance suffers. Could adding some safety checks on the passed context name before passing on to SNMP work here and address security concerns? |
Take a look at the linked PR, it's designed to eliminate that exact problem. By splitting the SNMP auth/community/etc from the walk module, it makes it much easier to configure. I'm also planning to eventually support multiple config files, so walks and auths can be built and shipped sparately. |
@SuperQ Interesting. I will keep an eye on that and once it is merged will attempt to refactor my config generation tool to use the new system. If it solves the problem I was trying to fix here, will move to the new method and close this PR. |
FYI, the config split is now merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be rebased against latest.
@@ -55,7 +55,7 @@ ifneq ($(shell which gotestsum),) | |||
endif | |||
endif | |||
|
|||
PROMU_VERSION ?= 0.13.0 | |||
PROMU_VERSION ?= 0.14.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not modify Makefile.common
in this PR. We automatically manage this file.
@@ -115,7 +115,7 @@ type ScrapeResults struct { | |||
retries uint64 | |||
} | |||
|
|||
func ScrapeTarget(ctx context.Context, target string, config *config.Module, logger log.Logger) (ScrapeResults, error) { | |||
func ScrapeTarget(ctx context.Context, target string, snmp_context string, config *config.Module, logger log.Logger) (ScrapeResults, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go style does not use underscores for varibale names. Please follow the Go style guide.
func ScrapeTarget(ctx context.Context, target string, snmp_context string, config *config.Module, logger log.Logger) (ScrapeResults, error) { | |
func ScrapeTarget(ctx context.Context, target string, snmpContext string, config *config.Module, logger log.Logger) (ScrapeResults, error) { |
Ping, are you still interested in this? Would you mind if someone else took over work on this feature? |
@SuperQ feel free to have someone else take a look - have not had time to confirm the config split change fully replaces the utility of this or not as it requires a rewrite of some other config generation code first to test. If this PR ends up merged I will not have to do any of that, so yes still interested the feature. |
I realized we actually implemented context name support when we refactored the Auth/Module split. I don't think this change is necessary now as controlling context can now be done from the vastly simplified config. |
snmp?target=hostname&module=module will use existing c.Auth.ContextName from snmp.yml as g.ContextName if defined (same as previous behavior)
snmp?target=hostname&module=module&snmp_context=context_name allows for adding or overriding this value by passing it as a job param similar to target and module values.
this allows for simple automated configuration of jobs to scrape individual contexts without needing to define a new entry in snmp.yml for each
@SuperQ @RichiH