[INS-345] Add New Relic Insights Query Key detector#4781
[INS-345] Add New Relic Insights Query Key detector#4781mustansir14 wants to merge 8 commits intotrufflesecurity:mainfrom
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| uniqueAccountIDMatches[match[1]] = struct{}{} | ||
| } | ||
|
|
||
| for _, keyMatch := range keyMatches { |
There was a problem hiding this comment.
Key matches not deduplicated unlike other multi-part detectors
Medium Severity
Account ID matches are deduplicated into uniqueAccountIDMatches, but keyMatches is iterated directly from FindAllStringSubmatch without deduplication. Other multi-part detectors in the codebase (e.g., adobeio, airbrakeprojectkey, airship) consistently deduplicate both parts into maps before the nested loops. This inconsistency means duplicate keys in scanned data produce duplicate results and redundant verification API calls.
There was a problem hiding this comment.
This is intended. The idea is to report multiple results if the key appears in multiple places, so that the user can remove it from all of those places.
Account ID matches are deduplicated because they are not the primary credential.
| regionUrls := map[string]string{ | ||
| "us": fmt.Sprintf("https://insights-api.newrelic.com/v1/accounts/%s/query?nrql=SELECT%%201", accountID), | ||
| "eu": fmt.Sprintf("https://insights-api.eu.newrelic.com/v1/accounts/%s/query?nrql=SELECT%%201", accountID), | ||
| } |
There was a problem hiding this comment.
(optional) suggestion: use net/url insted of fmt.Sprintf()
There was a problem hiding this comment.
I think that would unnecessarily complicate the code. We prefer net/url when we are concatenating user inputted URLs. Here we are simply inserting the account ID (which also comes via a strict regex so it does not have any chances of being malformed) into a URL.
| res, err := client.Do(req) | ||
| if err != nil { | ||
| return false, nil, fmt.Errorf("error making request: %w", err) | ||
| } |
There was a problem hiding this comment.
If the US request times out or fails at the network level, you bail out entirely without trying the EU region endpoint. You probably want to try the next region instead?
There was a problem hiding this comment.
Hmm good catch. I agree. I'll just extract the inside of the loop into a separate method, which should also fix the deferred response body cleanup issue by bugbot.
| results = append(results, s1) | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
No results emitted when account ID is absent
High Severity
When no account ID is found in the data chunk, uniqueAccountIDMatches is empty, so the inner for accountID := range uniqueAccountIDMatches loop never executes. This means the detector silently produces zero results even when valid NRIQ- keys are present. The PR description states the key is the actual credential and account ID is only needed for verification. Other multi-part detectors in this codebase (e.g., Atlassian v2) handle this by inserting an empty string entry into the map when no secondary matches are found, ensuring unverified results are still reported.
There was a problem hiding this comment.
Account ID is required for verification. The atlassian detector you mentioned captures the organization_id only to pass it into AnalysisInfo. It's not needed for verification
There was a problem hiding this comment.
So, the key is useless without an account ID? It can't be used to access any sort of data on its own?
There was a problem hiding this comment.
Yes, all endpoints that it can access require an account ID
| Redacted: keyResMatch[:8] + "...", | ||
| } | ||
|
|
||
| if verify && accountIDResMatch != "" { |
There was a problem hiding this comment.
in what scenarios, accountIDResMatch can be empty at this point?
There was a problem hiding this comment.
It cannot. Good catch. I'll remove the check
2 participants
Description:
This PR adds the New Relic Insights Query Key Detector.
Regex:
Key:
\b(NRIQ-[a-zA-Z0-9-_]{25})Account ID:
detectors.PrefixRegex([]string{"relic", "account", "id"}) + '\b(\d{4,10})\b'The key is the actual credential but account ID is required for verification because the verification endpoint requires an account ID in the path, and there is no other deterministic way to verify the credential without specifying the valid account ID (invalid or malformed account IDs return the same response as an invalid key)
Verification:
For verification, we use the Insights Query API with a simple select query:
https://insights-api.newrelic.com/v1/accounts/[account_id]/query?nrql=SELECT%%201.We send a GET request. A response code of
200means the key is valid.401means it is an invalid/rotated key.Note: For EU region keys, the host should be
insights-api.eu.newrelic.comCorpora Test:
The detector does not appear in the list.
Checklist:
make test-community)?make lintthis requires golangci-lint)?Note
Medium Risk
Adds a new secret detector with live HTTP verification against New Relic’s API, which can affect scan behavior (extra network calls) and introduce false positives/negatives if the regex or verification semantics are off.
Overview
Adds a new
NewRelicInsightsQueryKeydetector that matchesNRIQ-...keys and associated New Relic account IDs, emitting combinedRawV2values for multipart correlation.Implements optional verification by calling the legacy Insights Query API against both US and EU endpoints (setting
X-Query-Key) and records the verified region inExtraData.Wires the detector into the default detector list and registers a new protobuf
DetectorTypeenum value (NewRelicInsightsQueryKey = 1045), with unit + integration tests and a benchmark for the new scanner.Reviewed by Cursor Bugbot for commit a767a92. Bugbot is set up for automated code reviews on this repo. Configure here.