carverauto/serviceradar

Fork

You've already forked serviceradar

Code Issues 190 Pull requests 16 Projects Releases 173 Packages Wiki Activity Actions

review: 1.0.17 #77

New issue

Closed

opened 2026-03-28 04:21:02 +00:00 by mfreeman451 · 1 comment

mfreeman451 commented

2026-03-28 04:21:02 +00:00

Owner

Copy link

Imported from GitHub.

Original GitHub issue: #211
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/issues/211
Original created: 2025-02-17T22:52:30Z

SNMP Checker Code Review Findings

Here's a list of findings from the recent code review. Please address these issues to improve the security, usability, and reliability of the SNMP checker.

Security

Critical: SNMP Community String: The default public community string is a major security risk. The configuration must require strong, unique community strings for each target. Document this prominently in the README.
High Priority: SNMP v3 Support: Implement SNMP v3 with proper authentication and encryption (USM).
Input Validation (Target Names, OIDs): Strengthen input validation for target names (use regular expressions) and OIDs (prevent injection).
Data Type Conversion Vulnerabilities: Ensure comprehensive error handling and logging in convertValue and helper functions. Consider fuzzing.
Denial-of-Service (DoS): Implement rate limiting and resource quotas to mitigate DoS risks. Add metrics on SNMP requests and errors.

Usability and Readability

#214
Clarity in Error Messages: Improve error messages (include OID name, target name, etc.).
Comments on Constants: Add comments to explain the purpose of constants.
Consider structured logging: Use a structured logging library like zap or logrus.

Potential Goroutine Problems

Error Channel Blocking: Increase the buffer size of errorChan or use a non-blocking send with error logging in SNMPCollector.
Collector Shutdown: Use a sync.WaitGroup in SNMPCollector.Stop to wait for the collect and handleErrors goroutines to exit.
Data Race on TargetStatus: The TargetStatus is updated by both the pollTarget and handleDataPoint functions. Protect with Mutexes.
Aggregator Reset: Protect reads/writes around Aggregator.Reset() to prevent race conditions.

Context Issues

#216
Context Timeout Configuration: Make defaultServiceStatusTimeout configurable.

Unbounded Growth

Time Series Data: Implement persistent storage or a more sophisticated in-memory caching mechanism with eviction policies for TimeSeriesData.
Target and OID Count: Consider strategies for horizontal scaling and resource management as the number of targets and OIDs increases.

Resource Leaks

SNMP Client Connections: Double-check that SNMP client connections are always closed properly (use defer).
Channel Closure: Double-check that all channels are eventually closed to prevent goroutine leaks.

Performance Issues

String Conversions: Use a sync.Pool to reuse string buffers in convertString.
OID Lookup: Use a map to store OID configurations in findOIDConfig.
Data Aggregation: Optimize the data structure used to store time series data for faster range queries.
Optimize SNMP GET requests: Consider using SNMP bulk GET requests.

Imported from GitHub. Original GitHub issue: #211 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/issues/211 Original created: 2025-02-17T22:52:30Z --- ## SNMP Checker Code Review Findings Here's a list of findings from the recent code review. Please address these issues to improve the security, usability, and reliability of the SNMP checker. ### Security - [ ] **Critical: SNMP Community String:** The default `public` community string is a major security risk. The configuration *must* require strong, unique community strings for each target. Document this prominently in the README. - [ ] **High Priority: SNMP v3 Support:** Implement SNMP v3 with proper authentication and encryption (USM). - [ ] **Input Validation (Target Names, OIDs):** Strengthen input validation for target names (use regular expressions) and OIDs (prevent injection). - [ ] **Data Type Conversion Vulnerabilities:** Ensure comprehensive error handling and logging in `convertValue` and helper functions. Consider fuzzing. - [x] **Denial-of-Service (DoS):** Implement rate limiting and resource quotas to mitigate DoS risks. Add metrics on SNMP requests and errors. ### Usability and Readability - [x] #214 - [ ] **Clarity in Error Messages:** Improve error messages (include OID name, target name, etc.). - [ ] **Comments on Constants:** Add comments to explain the purpose of constants. - [ ] **Consider structured logging:** Use a structured logging library like `zap` or `logrus`. ### Potential Goroutine Problems - [ ] **Error Channel Blocking:** Increase the buffer size of `errorChan` or use a non-blocking send with error logging in `SNMPCollector`. - [x] **Collector Shutdown:** Use a `sync.WaitGroup` in `SNMPCollector.Stop` to wait for the `collect` and `handleErrors` goroutines to exit. - [x] **Data Race on `TargetStatus`:** The `TargetStatus` is updated by both the `pollTarget` and `handleDataPoint` functions. Protect with Mutexes. - [x] **Aggregator Reset:** Protect reads/writes around `Aggregator.Reset()` to prevent race conditions. ### Context Issues - [x] #216 - [ ] **Context Timeout Configuration:** Make `defaultServiceStatusTimeout` configurable. ### Unbounded Growth - [ ] **Time Series Data:** Implement persistent storage or a more sophisticated in-memory caching mechanism with eviction policies for `TimeSeriesData`. - [ ] **Target and OID Count:** Consider strategies for horizontal scaling and resource management as the number of targets and OIDs increases. ### Resource Leaks - [ ] **SNMP Client Connections:** Double-check that SNMP client connections are always closed properly (use `defer`). - [ ] **Channel Closure:** Double-check that all channels are eventually closed to prevent goroutine leaks. ### Performance Issues - [ ] **String Conversions:** Use a `sync.Pool` to reuse string buffers in `convertString`. - [ ] **OID Lookup:** Use a map to store OID configurations in `findOIDConfig`. - [ ] **Data Aggregation:** Optimize the data structure used to store time series data for faster range queries. - [ ] **Optimize SNMP GET requests:** Consider using SNMP bulk GET requests.

mfreeman451 commented

2026-03-28 04:21:03 +00:00

Author

Owner

Copy link

Imported GitHub comment.

Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/issues/211#issuecomment-2670445644
Original created: 2025-02-20T04:38:26Z

closing, will run again for 1.0.18

Imported GitHub comment. Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/issues/211#issuecomment-2670445644 Original created: 2025-02-20T04:38:26Z --- closing, will run again for 1.0.18

mfreeman451 closed this issue

2026-03-28 04:21:03 +00:00

No Branch/Tag specified

staging

2406-feat-agent-fleet-management-secure-self-update-system

bug/k8s-helm-deployments

chore/k8s-arc-update

rust-fix

2371-analytics-stats-cards-should-abbreviate-numbers

chore/perl-cleanup

2942-featweb-ng-add-logs-tab-to-device-details-page

192-feat-tftp-server

mikemiles-dev/feature/netflow_collection

testing

dependabot/cargo/hostname-0.4.1

dependabot/cargo/redis-1.0.1

dependabot/cargo/bb8-0.9.0

dependabot/cargo/rcgen-0.14.6

dependabot/cargo/hyper-1.8.1

dependabot/cargo/hyper-util-0.1.19

dependabot/cargo/clap-4.5.51

dependabot/cargo/thiserror-2.0.17

dependabot/cargo/time-tz-2.0.0

dependabot/cargo/tonic-build-0.14.2

main

dependabot/npm_and_yarn/docs/mdast-util-to-hast-13.2.1

815-feat-support-win32-for-agentpoller

gh-pages

v1.2.5

v1.2.4

v1.2.3

v1.2.2

v1.2.1

v1.2.0

v1.1.2

v1.1.0

v1.0.92

v1.0.91

v1.0.90

v1.0.89

v1.0.88

v1.0.87

v1.0.86

v1.0.85

v1.0.84

v1.0.83

v1.0.82

v1.0.81

v1.0.78

v1.0.77

v1.0.76

v1.0.70

v1.0.69

v1.0.68

v1.0.67

v1.0.66

v1.0.65

v1.0.64

v1.0.63

v1.0.62

v1.0.61

v1.0.60

v1.0.59

v1.0.58

1.0.57

v1.0.56

v1.0.55

v1.0.54-pre5

v1.0.53

v1.0.53-pre19

v1.0.53-pre18

v1.0.53-pre17

v1.0.53-pre15

1.0.53-pre10

1.0.53-pre9

1.0.53-pre8

1.0.53-pre7

1.0.53-pre6

1.0.53-pre5

1.0.53-pre4

1.0.53-pre3

1.0.53-pre2

1.0.53-pre1

1.0.52

1.0.51

1.0.50

1.0.49

1.0.49-pre5

1.0.49-pre4

1.0.49-pre3

1.0.49-pre2

1.0.48

1.0.48-rc2

1.0.48-rc1

1.0.48-pre8

1.0.48-pre7

1.0.48-pre6

1.0.48-pre5

1.0.48-pre4

1.0.48-pre3

1.0.48-pre2

1.0.48-pre1

1.0.47

1.0.47-pre8

1.0.47-pre7

1.0.47-pre6

1.0.47-pre5

1.0.47-pre4

1.0.47-pre3

1.0.47-pre2

1.0.47-pre1

1.0.46

1.0.46-pre9

1.0.46-pre8

1.0.46-pre7

1.0.46-pre6

1.0.46-pre5

1.0.46-pre4

1.0.46-pre3

1.0.46-pre2

1.0.46-pre1

1.0.45

1.0.44

1.0.44-pre12

1.0.44-pre11

1.0.44-pre10

1.0.44-pre9

1.0.44-pre8

1.0.44-pre7

1.0.44-pre6

1.0.44-pre5

1.0.44-pre4

1.0.44-pre3

1.0.44-pre2

1.0.44-pre1

1.0.43

1.0.42

1.0.41

1.0.41-pre1

1.0.40

1.0.40-pre11

1.0.40-pre10

1.0.40-pre9

1.0.40-pre8

1.0.40-pre7

1.0.40-pre6

1.0.40-pre5

1.0.40-pre4

1.0.40-pre3

1.0.40-pre2

1.0.40-pre1

1.0.39

1.0.38

1.0.37

1.0.36

1.0.36-pre5

1.0.36-pre4

1.0.36-pre3

1.0.36-pre2

1.0.35

1.0.35-pre3

1.0.35-pre2

1.0.35-pre

1.0.34-pre3

1.0.34-pre2

1.0.34-pre1

1.0.33

1.0.33-pre2

1.0.33-pre

1.0.32

1.0.31

1.0.30

1.0.29

1.0.28

1.0.27

1.0.26

1.0.25

1.0.24

1.0.23

1.0.22

1.0.21

1.0.20

1.0.19

1.0.18

1.0.17

1.0.16

1.0.15

1.0.14

1.0.13

1.0.11

1.0.10

1.0.9

1.0.8

1.0.7

1.0.6

1.0.5

1.0.4

1.0.3

1.0.2

1.0.1

1.0.0

Labels

Clear labels

1week

2weeks

Failed compliance check

IP cameras

NATS

NATS JetStream

Possible security concern

Something isn't working

build

checkers

ci-cd

continuous integration-continuous deployments

cleanup

cnpg

cloud-native postgres

Pull requests that update a dependency file

device-management

documentation

Improvements or additions to documentation

duplicate

This issue or pull request already exists

dusk

ebpf

enhancement

New feature or request

Pull requests that update GitHub Actions code

Pull requests that update Go code

good first issue

Good for newcomers

help wanted

Extra attention is needed

invalid

This doesn't seem right

javascript

Pull requests that update Javascript code

Oracle Linux related issues

otel

opentelemetry logs, traces, metrics

plug-in

proton

timeplus proton streaming database

python

question

Further information is requested

Pull requests that update rust code

sdk

security

serviceradar-agent

serviceradar-agent-gateway

This will not be worked on

zen-engine

No labels

1week

2weeks

Failed compliance check

IP cameras

NATS

Possible security concern

serviceradar-agent-gateway

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

carverauto/serviceradar#77

Reference in a new issue

Repository

carverauto/serviceradar

Title

Body

No description provided.

Delete branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?

Rows
Columns