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

Allow detectors to customize cleaning logic #3233

Closed
wants to merge 19 commits into from
Closed

Conversation

rosecodym
Copy link
Collaborator

@rosecodym rosecodym commented Aug 19, 2024

Description:

We have identified some cases in which the results "cleaning" logic (the logic that eliminates superfluous results) should not run. In order to allow this, we need to expose the cleaning logic to the engine. This PR does so by doing these things:

  • Extending the Detector interface with a pair of methods to control cleaning logic
    • This is done via the creation of a new interface that is embedded
  • Creating a default implementation of this interface that can be embedded in detectors that don't need to customize this logic
  • Embedding this default implementation in every detector except aws and awssessionkey
    • This represents a change of behavior for opsgenie, twilio, and razorpay, but we have determined that those detectors' previous customization of their cleaning behavior was incorrect and should be removed
  • Invoking this new cleaning logic from the engine rather than individual detectors

Checklist:

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

@rosecodym rosecodym requested review from a team as code owners August 19, 2024 21:38
@rosecodym rosecodym marked this pull request as draft August 19, 2024 21:39
@rosecodym rosecodym marked this pull request as ready for review August 19, 2024 21:43
@rgmz
Copy link
Contributor

rgmz commented Aug 20, 2024

Nothing against this specific change, however, it feels like the number of detector-related interfaces and default implementations is getting out of hand (multi-part creds is another confusing one).

IMO, it would be cleaner ( 😎 ) to use the default CleanResults function unless the detector has a special CustomResultCleaner interface.

@rosecodym
Copy link
Collaborator Author

Nothing against this specific change, however, it feels like the number of detector-related interfaces and default implementations is getting out of hand (multi-part creds is another confusing one).

IMO, it would be cleaner ( 😎 ) to use the default CleanResults function unless the detector has a special CustomResultCleaner interface.

Heh, I will point out that when I did the false positive change that way @mcastorina suggested I instead use an embedded default logic provider (like I did here), which is why I did this. But I actually prefer that solution, so I'll take a pass at it.

@rgmz do you remember any details about that interface-smuggling related bug you found awhile ago? I want to ensure I don't trip over it again.

@mcastorina
Copy link
Collaborator

I'm not sure why I suggested that in the past, but I agree with @rgmz's suggestion to use interface smuggling here. I think it makes a lot of sense for detectors to have opt-in behavior for special handling, considering how many detectors we have.

@rosecodym
Copy link
Collaborator Author

#3235 is an implementation that uses interface smuggling

@rgmz
Copy link
Contributor

rgmz commented Aug 20, 2024

@rgmz do you remember any details about that interface-smuggling related bug you found awhile ago? I want to ensure I don't trip over it again.

The bug was in how the nested structs / interface matching works. #2960

rosecodym added a commit that referenced this pull request Aug 21, 2024
We have identified some cases in which the results "cleaning" logic (the logic that eliminates superfluous results) should not run. In order to allow this, we need to expose the cleaning logic to the engine. This PR does so by doing these things:

- Create a CustomResultsCleaner interface that can be implemented by detectors that want to use custom cleaning logic
- Implement this interface for the aws and awssessionkey detectors (and remove their previous invocation of their custom cleaning logic)
- Modify the engine to invoke this logic (conditionally)

This PR also removes the "custom" cleaning logic for the opsgenie, razorpay, and twilio detectors, because it was added erroneously.

This is an alternative implementation of #3233.
@rosecodym
Copy link
Collaborator Author

doing #3235 instead

@rosecodym rosecodym closed this Aug 21, 2024
@rosecodym rosecodym deleted the customize-cleaning branch August 21, 2024 13:42
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