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.

Sign up for GitHub

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

Jump to bottom

Handle custom detector response and include in extra data #3411

Merged
merged 8 commits into from
Oct 28, 2024
Merged

Handle custom detector response and include in extra data #3411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fb478d4
Handle custom detector response and include in extra data
kashifkhan0771 Oct 15, 2024
3662976
Added todo
kashifkhan0771 Oct 15, 2024
670eb3d
fixed panic
kashifkhan0771 Oct 15, 2024
6dc944a
simplicity is always good
kashifkhan0771 Oct 15, 2024
36c6519
limit the response to 200 chars
kashifkhan0771 Oct 22, 2024
949426f
results should print now in output
kashifkhan0771 Oct 23, 2024
707598b
Merge branch 'main' into update/custom-detector-resp
kashifkhan0771 Oct 28, 2024
b9bf00c
Merge branch 'main' into update/custom-detector-resp
kashifkhan0771 Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 26 additions & 8 deletions pkg/custom_detectors/custom_detectors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bytes"
"context"
"encoding/json"
"io"
"net/http"
"regexp"
"strings"
Expand Down Expand Up @@ -101,10 +102,6 @@ func (c *CustomRegexWebhook) FromData(ctx context.Context, verify bool, data []b
close(resultsCh)

for result := range resultsCh {
// NOTE: I don't believe this is being set anywhere else, hence the map assignment.
result.ExtraData = map[string]string{
"name": c.GetName(),
}
Comment on lines -104 to -107
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the reason the response result wasn't being printed! 😛 @zricethezav

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if the name is something we want to print. I can make the change to append it.

results = append(results, result)
}

Expand All @@ -129,6 +126,7 @@ func (c *CustomRegexWebhook) createResults(ctx context.Context, match map[string
DetectorType: detectorspb.DetectorType_CustomRegex,
DetectorName: c.GetName(),
Raw: []byte(raw),
ExtraData: map[string]string{},
}

if !verify {
Expand Down Expand Up @@ -166,14 +164,34 @@ func (c *CustomRegexWebhook) createResults(ctx context.Context, match map[string
}
req.Header.Add(key, strings.TrimLeft(value, "\t\n\v\f\r "))
}
res, err := httpClient.Do(req)
resp, err := httpClient.Do(req)
if err != nil {
continue
}
// TODO: Read response body.
res.Body.Close()
if res.StatusCode == http.StatusOK {
defer func() {
_, _ = io.Copy(io.Discard, resp.Body)
_ = resp.Body.Close()
}()

if resp.StatusCode == http.StatusOK {
// mark the result as verified
result.Verified = true

body, err := io.ReadAll(resp.Body)
Copy link
Contributor

Choose a reason for hiding this comment

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

@mcastorina I could not recall where we had a discussion about PII. I remember, we decided not to include PII in extraData. Will this needed to be care here as well ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a great point. Custom detector servers are user supplied (or at least user configured), so I don't think PII would be an issue here.. @zricethezav what are your thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

see https://github.com/trufflesecurity/trufflehog/pull/3411/files#r1809008021

if err != nil {
continue
}

// TODO: handle different content-type responses seperatly when implement custom detector configurations
responseStr := string(body)
// truncate to 200 characters if response length exceeds 200
if len(responseStr) > 200 {
responseStr = responseStr[:200]
}

// store the processed response in ExtraData
result.ExtraData["response"] = responseStr
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kashifkhan0771 Thought: storing the entire response could lead to some large outputs. This could be annoying for users who already are using custom detectors.

One option could be limit the response to 200 characters. I don't feel comfortable merging this PR as is.

@kashifkhan0771 additionally, I tried spinning up a verification server and testing this myself but found that the only thing that could printed in the output was the name of the customer verifier: "ExtraData":{"name":"HogTokenDetector"} when using the --json flag. A mock test might help here, although I have a suspicion there might be some other issues considering I'm not able to surface additional ExtraData past the name of the custom verifier in the output.

@CameronLonsdale what is your use case for ExtraData? How are you using TruffleHog to surface this data (what command and flags)?

Copy link

Choose a reason for hiding this comment

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

For me, I always run trufflehog with --json --only-verified. I wanted to surface data from my custom patterns that are the responses from verifying the credential against the API. So this would typically be a JSON blob which tells me info about the user which corresponds to the token I just verified.

I wrap trufflehog around custom code to sync results to Jira etc so hence why JSON. But I would expect the extraData to show up in CLI output aswell, so truncating it to be readable at the terminal makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zricethezav @CameronLonsdale I added the limit of 200 chars for response. I am not sure how to test the custom detector. If anyone of you can guide me how to do it. I can test locally to understand why it is not printing the extra data in output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, @mcastorina, for your guidance on running the custom detector. I successfully tested it and identified the issue that was preventing the results from printing. I've fixed it now. Please review my changes.


break
}
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/detectors/meraki/meraki.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,11 @@ import (
"context"
"encoding/json"
"fmt"
regexp "github.com/wasilibs/go-re2"
"io"
"net/http"

regexp "github.com/wasilibs/go-re2"

"github.com/trufflesecurity/trufflehog/v3/pkg/common"
"github.com/trufflesecurity/trufflehog/v3/pkg/detectors"
"github.com/trufflesecurity/trufflehog/v3/pkg/pb/detectorspb"
Expand Down
Loading