[Backend] Follow-up to handle path exclusions in the gem or the secret detection service

Overview

While working on Phase 1 to add ability to manage and use SD Exclusions when scanning for secrets using secret push protection, we initially had to handle path-based exclusions in a post-scanning step (read more on this here), this was due to a couple of limitations we had when using ListBlobs()/ListAllBlobs() RPCs to retrieve committed changes for a new git push operation:

• Having to scan entire files as Gitaly didn't provide RPCs to get only diffs.
• Not having associated file paths available until we call GetTreeEntries() RPC later in the process (post-scanning).

In #469161 (closed), we worked to re-implement our scanning mechanism to drop ListBlobs()/ListAllBlobs() + GetTreeEntries() in favour of a combination of DiffBlobsRequest() + ListAllCommits()/FindChangedPath(), which allowed us to retrieve associated file paths of committed changes before we start scanning for secrets (pre-scanning). The aim of this issue is to pass retrieved file paths to the scanning interface (whether the gem or the secret detection service) and have it handle the exclusions entirely by itself to improve separation of concerns.

Proposal

• Update the push check to pass file paths retrieved from FindChangedPath() to scanning interfaces (along with exclusions).
• Update the gem to handle path exclusions by itself.
• Update the secret detection service to handle path exclusions by itself.

Original Discussion

The following discussion from !166511 (merged) should be addressed:

• @vshushlin started a discussion:

I left a few comments, but my biggest concern is the separation of concerns of the lib code and the gem .

Right now:

• some of the scanning exclustions are implemented in the gem, and some in the lib
• so we pass exclustion to the gem, as well as have code handling on time directly
• we also post-process secrets finding, and alter the result that gem returns

That makes it super hard to understand, write tests for etc. Everything is coupled way too tightly.

I understand that you've done that because you don't have some information (file paths) on the gem level, but I think we should either:

1. pass this information to the gem, and handle all exclusions there
2. post-process all exclusions on the library level

I would vote for the second option, as it's much more straightforward to implement right now, and you can open a follow-up issue to move this logic to the gem completely later.

added gitlab-org#14878 (closed) as parent epic

changed the description

changed milestone to %17.6

added backend devopssecure featureconsolidation groupsecret detection secret detection exclusions secret push protection sectionsec typefeature workflowblocked labels

added Category:Secret Detection label

marked this issue as blocked by #469161 (closed)

changed the description

mentioned in merge request !166511 (merged)

changed title from [Backend] Follow-up to handle file path exclusions in the gem to [Backend] Follow-up to handle path exclusions in the gem or the secret detection service

changed the description

changed milestone to %Backlog

added workflowready for development label and removed workflowblocked label

I'm moving this to the backlog. It's no longer blocked by #469161 (closed) but we still have a number of important issues to take care of first, such as #488499 (closed), before we can get to this one.

@ahmed.hemdan @smeadzinger I'm going to move this issue to the parent epic since we're technically done with Phase 1 and have released it successfully. I wasn't sure if there was a better way we could track follow up issues, so I'll keep it in the parent epic for now. Let me know if you have any other thoughts/ideas on this!

I think it makes sense to have this moved into the parent epic since we're done with Phase 1. As pointed out in the issue description, this was blocked by #469161 (closed) but given all the necessary follow-up work for diff scanning (mostly in #488499 (closed) and beyond), I consider it to be still blocked (that's why I moved it back to the %Backlog).

I will mark it as blocked by #488499 (closed) for now.

@ahmed.hemdan Now that Refactor DiffBlobs support out of secret detect... (#488499 - closed) is closed and stable, should we revisit this issue and get it prioritized? Is there anything we need to update in the proposal?

@eurie tagging you in as well since you've been making a number of changes on how we handle the exclusion logic.

CC @abellucci - Possible typemaintenance task for %17.8 or %17.9

@amarpatel I'll defer to @ahmed.hemdan for a more in-depth response since he's more familiar with the problem, however I'll add:

1. We added path exclusions to the SDS protocol in this MR
2. I think the second checkbox can be dropped since the 3rd (adding the feature to the SDS) will address it once the SDS gem is integrated shortly.
3. Do we need another checkbox for removing the existing post-scanning logic?

Now that Refactor DiffBlobs support out of secret detect... (#488499 - closed) is closed and stable, should we revisit this issue and get it prioritized? Is there anything we need to update in the proposal?

@amarpatel Well, we could get this prioritized but I wonder if we should instead focus first on the more pressing stuff like finalizing the work on diff scanning, and updating our montoring runbook/dashboard + the design document.

Below is a list of issues I think we should do before we handle path exclusions in the secret detection service:

• #423992 (closed) – Complete work to distinguish between user-local-to-remote pushes vs. automated workflows:
  • Identify RPCs that invoke access checks.
  • Update Rails monolith to ensure RPC calls to Gitaly populate the gitaly-client-context-bin with the RPC name.
  • Update secrets push check to skip scanning based the RPC name in gitaly-client-context-bin parameter.
• #491282 – Complete the work to switch WebIDE to use diff scanning:
  • Update secrets push check to remove protocol checks in favour of using gitaly-client-context-bin.
• #480092 (closed) – Remove the spp_scan_diffs feature flag and make diff scanning the default behaviour:
  • Remove entire file scanning behaviour completely from the secrets push check.
• #477389 – Drop use of RPCs that are no longer used:
  • Update secret push check to fully depend on ListAllCommits() and FindChangedPaths().
  • Remove usage of ListBlobs(), ListAllBlobs(), and GetTreeEntries() RPCs.
• #481883 (closed) – Kick off the work to refactor the secrets push check:
  • Update the proposal to reflect all the changes above.
  • Work on breaking down the class into multiple classes and enhance testing and logging.
• No issue, but still have to be done after all the above is complete:
  • Update the secret push protection monitoring runbook to reflect changes above.
  • Update the secret push protection dashboard to reflect changes above.
  • Update the platform-wide SD design document to reflect changes above.

@eurie and @serenafang Could you possibly verify the list above and let me know if something else is missing?

1. We added path exclusions to the SDS protocol in this MR
2. I think the second checkbox can be dropped since the 3rd (adding the feature to the SDS) will address it once the SDS gem is integrated shortly.
3. Do we need another checkbox for removing the existing post-scanning logic?

@eurie Good points. I think if are going to handle path exclusions in SDS (which was the ultimate goal of this issue), it makes sense to add another checkbox above to remove existing post-scanning logic that applies the path-based exclusions.

Yes, 2nd checkbox can be dropped since adding the feature to SDS is enough once SDS gem is integrated. It's goes without saying that handling exclusions in SDS requires that we have the service fully functional and not running passive mode first.

@ahmed.hemdan That list complete to the best of my knowledge.

removed from epic &14878 (closed)

added gitlab-org#14315 as parent epic

marked this issue as blocked by #488499 (closed)

added workflowblocked label and removed workflowready for development label

mentioned in merge request !177627 (merged)