Move Secret Detection script logic into the analyzer

Description

Secret Detection is unlike the other analyzers in that it scans git history by inspecting the output of git log -p ... instead of analyzing whole files/projects. Determining what parts of a git history to scan is not a straight forward task. Currently, the vendored template uses this script to prepare the git repo and git ranges.

There are 4 scenarios we should cover to make sure if secrets do get pushed, they are caught immediately.

1. Push events

1 or more commits can be associated with a push event. Example of this would be if you are working on a feature branch and commit 5 changes to that branch then push the branch. The Secret Detection job that gets triggered by the push event should scan only those 5 commits. The commits associated with the MR can be retrieved by using CI_COMMIT_BEFORE_SHA and CI_COMMIT_SHA.

2. Merge events

Merge events can have multiple push events associated with the event. This means retrieving the git of commits is not as straight forward and requires some form of git log targetBranch..CIBranch to determine the commit range.

3. Full History Scans

Some users may want to scan the entire history of their repo on all branches. This is gitleaks default behavior and is supported by the SECRET_DETECTION_HISTORIC_SCAN variable.

4. NoGit Scans

Users may want to scan the current state of their repo for secrets. This means treating repos as plain directories and scanning the contents of files for secrets. This is the current default behavior if no options are passed to the secret detection scanner.

The git depth is set to 20 for shallow clones in Gitlab pipelines. Secret Detection overrides this and instead fetches the entire history of $CI_DEFAULT_BRANCH and $CI_COMMIT_REF_NAME. This can cause some performance issues for large repos as fetching the full history of a repo is an expensive task. !78321 (merged) introduces some changes that make --depth dynamic when fetching. This should give us full coverage so that all commits being pushed to Gitlab are scanned and this should greatly reduce the time of secret detection jobs for large repos since we are only fetching what we need.

@plafoucriere and @dsearles brought up some good points when reviewing gitlab-com/www-gitlab-com!96577 (closed) saying we should move the script logic from the vendored template to the Secret Detection analyzer. I think this would be a worthwhile change for the following reasons:

1. Error handling: if an error pops up during a git fetch or git log we can handle the errors and log a useful message which would help with maintaining and debugging the analyzer. This would also help customers and support.
2. All Secret Detection logic tied to one release operation. Instead of updating the Secret Detection analyzer AND vendored template, we would just have to update the analyzer. This would eliminate the dance between vendored template changes and analyzer releases that we are currently doing.

Proposal

Move the vendored template script logic into the Secret Detection analyzer

changed milestone to %Backlog

added Category:Secret Detection devopssecure featureenhancement groupstatic analysis sectionsec labels

set weight to 5

added typefeature label

mentioned in merge request gitlab-com/www-gitlab-com!96577 (closed)

mentioned in issue #350213 (closed)

@zrice Thanks for creating this issue! As per our discussion, I mentioned that I did something similar recently, you can find the code here: https://gitlab.com/gitlab-com/gl-security/engineering-and-research/gib/-/blob/5b2ff61ea170db37b253e40a47da6dae7eff7c4b/runCompliance.go. Please feel free if you have any question of course.

added [deprecated] Accepting merge requests label

mentioned in issue #350656 (closed)

@twoodham maybe we could prioritize this issue as it relates to some of the issue you pinged me about:

• !72689 (closed)
• !76346 (closed)
• #328843 (closed)

It would be hugely beneficial to Secret Detection productivity as testing changes could happen all in the analyzer rather than in a bash script.

@connorgilbert - FYI on this issue.

@zrice - this feels like a breaking change to announce in %14.8 and execute in %15.0. Am I reading it correctly?

If removing the ability to override script in the vendored template for the secret detection job is considered a breaking change, then yes.

edit: I think I agree, it feels like a breaking change, but I figured I would bring it up since I'm working on issues right now that would benefit from this change.

@twoodham @zrice if there's bandwidth for it in 14.8, the more deprecations the merrier

On a more serious note, the motivation behind the deprecations push is to be able to accelerate progress moving forward, and this sounds like it fits the bill. We obviously don't want to cause unnecessary work for users, but it sounds to me like:

• This will give a better experience overall
• We don't expect people to make a lot of overrides that we'd now be disallowing

Based on what I'm reading here, my assessment would be that it would fit in the &7201 (closed) "best-case items" group.

If removing the ability to override script in the vendored template for the secret detection job is considered a breaking change, this yes. This does feel like it's breaking, in that we'd be either ignoring a customization or failing to run with a customized script block. (Right?)

@connorgilbert I think you hit the nail on the head!

• This will give a better experience overall

I believe this would be the case. It would reduce the complexity of the Secret Detection product in that state would be managed solely within the analyzer. I will try to create some sort of state diagram that describes all the different states Secret Detection can find itself in.

• We don't expect people to make a lot of overrides that we'd now be disallowing

I think that is the case. It's not common to override the script section in the vendored template.

WIP flowchart for the curious:

Secret_Detection_Flowchart.pdf!

Great, thanks! I edited it into the &7201 (closed) description and the plan sounds good. I'm planning to make a docs issue to make sure we end up with all the right deprecation announcements even for those things that are going to be executed (i.e., actually removed) in future releases.

mentioned in merge request gitlab-org/security-products/analyzers/secrets!130 (merged)

mentioned in epic &7201 (closed)

mentioned in merge request !79985 (closed)

As discussed in !79985 (closed), I think at a minimum we could simply extract all bash short of scripts from the template and we can throw a shellcheck job into out secrets CI. That could be a worthwhile change for %15.0 if we can manage it

This is pretty similar to the approach ~"group::configure" uses for their auto-deploy-image which powers Category:Auto DevOps

mentioned in issue #352565 (closed)

Depending on the outcome of !80326 (comment 846909678) we may not need to do this. It seems we cannot guarantee valid commit ranges in pipelines so an alternative would be just to scan the CI_COMMIT_SHA changes. This would greatly simplify the product, although leave potential for missed secrets.

mentioned in issue #327438 (closed)

mentioned in merge request !83037 (closed)

mentioned in merge request gitlab-org/security-products/analyzers/secrets!155 (closed)

assigned to @zrice

mentioned in merge request gitlab-org/security-products/analyzers/secrets!156 (merged)

Thanks for working on this @zrice! We've removed the Seeking community contributions label to avoid having multiple people working on the same issue.

removed [deprecated] Accepting merge requests label

@connorgilbert @theoretick @plafoucriere talking through this with @rossfuhrman earlier I think it makes more sense to remove all attempts at dynamically fetching the correct depth and instead rely on the user to set a proper GIT_DEPTH. If we want, we could set the default GIT_DEPTH in the secret vendored template could be rather large, like 500 or as that should handle most cases. If this is a big issue for users, we can introduce more git fetch operations into the analyzer.

I may be missing something, so I'll try here to state my assumptions and reasoning as well.

• I agree with the various comments I saw suggesting that we bias toward completeness over performance (e.g. gitlab-com/www-gitlab-com!96577 (comment 811077895)). I recognize that fetches could be costly, but it seems better to detect secrets somewhat slower than to silently drop them.
  • It's a better situation to suggest a workaround (like cutting GIT_DEPTH) if there is an observed performance issue, compared to dealing with a false negative.
• I am not sure how much we could "rely on the user to set a proper GIT_DEPTH" as, frankly, I expect most users to not ever think about that variable. (And I think they shouldn't have to either, honestly.) I would expect that the default we pick will be by far the most commonly used option.
• Based on those two considerations, I am on board with setting a high GIT_DEPTH. I would reconsider this if we had known serious performance issues with such a high value.
• I am persuaded by arguments to move this logic into the analyzer to the extent possible, since sync'ing the template and the analyzer versions seems hard to do in SaaS, let alone offline or once a user pins an analyzer version.
  • Side note, not blocking—this feels like it might also help us avoid issues like this #349299 (closed) where I think the CI template might not be handling CA certs the same as the analyzer.

My main concerns/questions would be:

• whether we're confident this is the right solution after some churn in the templates.
• if it is a breaking change, we should really not be springing it without the default required notice. We could do it off-cycle (before 16.0) but should give people notice if we expect there to be breaking customer impact. Do we?

@connorgilbert

I am not sure how much we could "rely on the user to set a proper GIT_DEPTH" as, frankly, I expect most users to not ever think about that variable. (And I think they shouldn't have to either, honestly.) I would expect that the default we pick will be by far the most commonly used option.

Fair point, and I agree. If a secret detection job failed due to a "commit/object does not exist" git failure, then we could present a log message instructing users to set a higher GIT_DEPTH.

I agree with the various comments I saw suggesting that we bias toward completeness over performance (e.g. gitlab-com/www-gitlab-com!96577 (comment 811077895)). I recognize that fetches could be costly, but it seems better to detect secrets somewhat slower than to silently drop them. I am persuaded by arguments to move this logic into the analyzer to the extent possible, since sync'ing the template and the analyzer versions seems hard to do in SaaS, let alone offline or once a user pins an analyzer version.

Given your concerns it may be best we just go with the original plan and implement dynamic fetching in the analyzer.

cc @rossfuhrman I'll update the MR again

Sorry for the delay, yesterday was all Zoom. By this I meant that 500 seemed like a high enough number that it sounded like a workable solution:

• Based on those two considerations, I am on board with setting a high GIT_DEPTH. I would reconsider this if we had known serious performance issues with such a high value.

I could've been clearer about that bottom-line, sorry.

I want to be clear that I trust the team's judgment in terms of which approaches seem most promising given the constraints we're under and the goals we have—happy to engage in more detail on what we plan to do for %15.0 (synchronously if that's most efficient) but if we want to leave that conversation for later milestones I'm also fine with that.

changed milestone to %15.0

added workflowin dev label

added workflowproduction label and removed workflowin dev label

closing this issue out as the work discussed has been completed

closed

mentioned in issue gitlab-org/quality/triage-reports#7679 (closed)