Skip to content
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

Log an error on SNMPv3 requests where authentication fails (#7929) #8215

Closed
wants to merge 1 commit into from

Conversation

DouglasHeriot
Copy link

gosnmp does not return any error when an SNMP get request fails due to authentication.
The SNMP agent will return a "report" PDU instead of a "get-response" PDU in this case.

A check has been added to verify the packet's PDUType is not "Report"

I have not yet added documentation or unit tests.

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

…a#7929)

gosnmp does not return any error when an SNMP get request fails due to authentication.
The SNMP agent will return a "report" PDU instead of a "get-response" PDU in this case.

A check has been added to verify the packet's PDUType is not "Report"
@sjwang90 sjwang90 added area/snmp feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Nov 24, 2020
@Hipska
Copy link
Contributor

Hipska commented Dec 17, 2020

@sjwang90 or @ssoroka Can you link issue #7929 to this PR?
@DouglasHeriot are you still planning to add more commits?

@sjwang90 sjwang90 linked an issue Dec 17, 2020 that may be closed by this pull request
@Hipska Hipska self-assigned this Dec 18, 2020
@Hipska
Copy link
Contributor

Hipska commented Jan 4, 2021

Hello @DouglasHeriot, Are you still planning to add more commits? Since this is still a PR in Draft state, it will not be considered for merging.

If there is no response soon, the PR will be closed as we are trying to clean up the back-log.

@DouglasHeriot
Copy link
Author

As per discussion in #7929 I think the pull request #8588 that updates the version of gosnmp should fix this issue. I'll reopen this if required.

@Hipska
Copy link
Contributor

Hipska commented Jan 18, 2021

Thanks for your response, but shouldn’t there still be some logging if credentials are not correct?

@henriknoerr
Copy link

see #7929 - Issue still happens, and yes I think a log message would indeed be expected in telegraf.log

@Hipska Hipska reopened this Feb 15, 2021
@Hipska
Copy link
Contributor

Hipska commented Feb 15, 2021

Issue is not fixed, but this PR will.

Copy link
Contributor

@Hipska Hipska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the code and would suggest this change for more meaningful error message.

@@ -415,6 +415,8 @@ func (t Table) Build(gs snmpConnection, walk bool) (*RTable, error) {
// index, and being added on the same row.
if pkt, err := gs.Get([]string{oid}); err != nil {
return nil, fmt.Errorf("performing get on field %s: %w", f.Name, err)
} else if pkt != nil && pkt.PDUType == gosnmp.Report {
return nil, fmt.Errorf("received SNMP report instead of response for field %s: check authentication?", f.Name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return nil, fmt.Errorf("received SNMP report instead of response for field %s: check authentication?", f.Name)
_, _, oidText, _, _ := SnmpTranslate(pkt.Variables[0].Name)
return nil, fmt.Errorf("recieved %s report", strings.TrimSuffix(oidText, ".0"))

@Hipska
Copy link
Contributor

Hipska commented Feb 19, 2021

@DouglasHeriot Are you willing to pick this up? Otherwise I will create my own PR. I want this to get resolved ASAP.

@Hipska
Copy link
Contributor

Hipska commented Feb 24, 2021

@DouglasHeriot?

@Hipska Hipska mentioned this pull request Mar 1, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/snmp feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Random integers in SNMP Input Plugin OID output
4 participants