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

Fix RabbitMQ detector not respecting timeout #3421

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

rgmz
Copy link
Contributor

@rgmz rgmz commented Oct 15, 2024

Description:

This adds a timeout to the RabbitMQ detector.

2024-10-15T18:28:56-04:00	error	trufflehog	a detector ignored the context timeout	{"detector_worker_id": "4ZclX", "detector": {"type":"RabbitMQ"}, "timeout": 10}

Checklist:

  • Tests passing (make test-community)?
  • Lint passing (make lint this requires golangci-lint)?

@rgmz rgmz force-pushed the fix/rabbitmq-timeout branch 2 times, most recently from 7ca4070 to ad2b550 Compare October 15, 2024 23:20
_ = conn.Close()
}
}()
return err == nil, err
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Future work would be to categorize the errors provided by this, differentiating between valid and invalid.

Such as:
Verification issue: lookup xcws0001: no such host
Verification issue: i/o timeout

Copy link
Contributor

@abmussani abmussani left a comment

Choose a reason for hiding this comment

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

@rgmz thanks for the fix. Instead of 10s constant timeout, Will it be a good idea to calculate the remaining time using context.Deadline() ? and use default 10s if there is no deadline define ?

@rgmz
Copy link
Contributor Author

rgmz commented Oct 24, 2024

Will it be a good idea to calculate the remaining time using context.Deadline() ? and use default 10s if there is no deadline define ?

I think that risks exacerbating #2991.

IMO, the problem is detectors taking an unreasonable amount of time or hanging. If something needs a bit more time than the deadline to be verified, I think that's acceptable.

@rgmz rgmz requested a review from a team as a code owner November 3, 2024 16:19
@ahrav ahrav merged commit 53d01ad into trufflesecurity:main Nov 7, 2024
13 checks passed
@rgmz rgmz deleted the fix/rabbitmq-timeout branch November 8, 2024 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants