### Overview This epic is focused on building and delivering a first iteration (MVC) for [GitLab-native pre-receive secret detection](https://gitlab.com/groups/gitlab-org/-/epics/11439 "GitLab-native pre-receive secret detection scanning"). The aim here is to define and determine the work needed to ship the MVC, and put an implementation plan to action. Please see [this section](https://gitlab.com/gitlab-org/gitlab/-/issues/422574#problem "Perform secret detection for highest risk content pre-receive") for more information on the problem this MVC is solving. ### Scope Please check [this section](https://gitlab.com/groups/gitlab-org/-/epics/11439#mvc-scope "GitLab-native pre-receive secret detection scanning") from the parent epic, and note the following: * We have [decided](https://docs.google.com/document/d/195BM7tAU9iquFkjTROtpuLIlbjfDVKzphZ21ZyzfQlk/edit#bookmark=id.aijtk3i8nlee) (internal only) to put the feature behind a flag instead of an instance-level setting for the MVC. * A [configurable timeout](https://docs.google.com/document/d/195BM7tAU9iquFkjTROtpuLIlbjfDVKzphZ21ZyzfQlk/edit#bookmark=id.bazkitgokypw) (internal only) or a circuit breaker will likely be part of the Ruby gem (see [below](https://gitlab.com/groups/gitlab-org/-/epics/11587#one-build-a-ruby-gem-codename-shush-to-perform-secrets-regex-matching-on-git-blobs "[MVC] Build first iteration of pre-receive secret detection")) performing scans. Please also note the following, they're not part of the scope per se, but might come in handy to be delivered along with the MVC: * **Auditability**: having audit events created whenever a git push is blocked for failing secrets scanning. ### Proposal As a result of the spike work done in: * https://gitlab.com/gitlab-org/gitlab/-/issues/423832+ * https://gitlab.com/gitlab-org/gitlab/-/issues/423834+ And based on the discussions from: * https://gitlab.com/gitlab-org/gitlab/-/issues/422574+ (and the [parent epic](https://gitlab.com/groups/gitlab-org/-/epics/11439 "GitLab-native pre-receive secret detection scanning")) * [Secret Detection Research Spikes Review – Meeting Agenda](https://docs.google.com/document/d/195BM7tAU9iquFkjTROtpuLIlbjfDVKzphZ21ZyzfQlk) (internal only) We have identified the following two steps to be mandatory for shipping the MVC. These can be thought of as workstreams: 1. [Build a Ruby gem (gitlab-secret_detection) to perform secrets regex matching on git blobs.](https://gitlab.com/groups/gitlab-org/-/epics/11587#one-build-a-ruby-gem-codename-shush-to-perform-secrets-regex-matching-on-git-blobs "[MVC] Build first iteration of pre-receive secret detection") 2. [Create a push check to run secret detection scans on pushed code.](https://gitlab.com/groups/gitlab-org/-/epics/11587#two-create-a-push-check-to-run-secrets-detection-scans-on-pushed-code "[MVC] Build first iteration of pre-receive secret detection") We also have a number of additional topics to tackle after the primary ones above: 3. [Telemetry](https://gitlab.com/groups/gitlab-org/-/epics/11587#three-telemetry "[MVC] Build first iteration of pre-receive secret detection") 4. [Performance monitoring/profiling](https://gitlab.com/groups/gitlab-org/-/epics/11587#four-performance-monitoringprofiling "[MVC] Build first iteration of pre-receive secret detection") 5. [Production readiness](https://gitlab.com/groups/gitlab-org/-/epics/11587#five-production-readiness "[MVC] Build first iteration of pre-receive secret detection") Please read below for detailed information on the decisions and spike findings behind this proposal. --- #### :one: Build a Ruby gem (gitlab-secret_detection) to perform secrets regex matching on git blobs. To perform regex matching on git blobs that may include secrets, we plan to create a small Ruby library/gem that will be included as a dependency in GitLab main codebase (`gitlab-org/gitlab`). This dependency will accept one or more git blobs, match them against a defined ruleset of regular expressions (based on [`gitleaks.toml`](https://gitlab.com/gitlab-org/security-products/analyzers/secrets/-/blob/master/gitleaks.toml) used by [secrets analyzer](https://gitlab.com/gitlab-org/security-products/analyzers/secrets)), and return scan results. **Why a gem and not directly in `gitlab-org/gitlab`'s `/lib` folder?** Well, a few things: having this included directly in `gitlab-org/gitlab` defeats the purpose of [continuous secret detection](https://gitlab.com/gitlab-org/gitlab/-/issues/413274 "Secret Detection continuous scanning"), which aims to increase secret detection [coverage](https://about.gitlab.com/direction/secure/static-analysis/secret-detection/#1-year-plan) outside of CI/CD pipelines-context. Additionally, the aim here is to build a self-contained library – similar to [Hush](https://gitlab.com/gitlab-org/secure/pocs/hush) (internal only), or [this POC](https://gitlab.com/gitlab-org/secure/pocs/secret-detection-go-poc) (internal only) extracted from it – that accept code in the form of blobs, and return scan results. Moreover, not using a gem will increase tight coupling and will not make it easy to re-use it elsewhere if the code is only part of `gitlab-org/gitlab` codebase. For more on this point, please make sure to read on the [advantages of using gems](https://docs.gitlab.com/ee/development/gems.html#advantages-of-using-gems), and the pros/cons of having it inside the [same repository](https://docs.gitlab.com/ee/development/gems.html#in-the-same-repo) vs. [external repository](https://docs.gitlab.com/ee/development/gems.html#in-the-external-repo). **Shall we use the `gitleaks.toml` file from `secrets` analyzer?** Probably yes, the `secrets` analyzer is likely to continue to be maintained in the long term, and we should also avoid maintaining two separate set of rules (one in the analyzer and one in the gem). Plus, using the same configuration means we achieve some level of parity between our CI-based product offering and this new feature. Finally, our [benchmarking spike](https://gitlab.com/gitlab-org/gitlab/-/issues/423832 "Spike: Benchmark regex performance between Ruby and Go") based its [results](https://gitlab.com/gitlab-org/gitlab/-/issues/423832#note_1578076768 "Spike: Benchmark regex performance between Ruby and Go") on [matching blobs against the entire `gitleaks.toml` ruleset](https://gitlab.com/gitlab-org/gitlab/-/issues/423832#note_1571210682 "Spike: Benchmark regex performance between Ruby and Go"), therefore, I believe it only make sense to use the same configuration for our implementation. Please read more on this topic from the README of our [Secret Detection Go POC](https://gitlab.com/gitlab-org/secure/pocs/secret-detection-go-poc#ruleset) (internal only). **Ruby vs. Go** As explained above, we [decided](https://docs.google.com/document/d/195BM7tAU9iquFkjTROtpuLIlbjfDVKzphZ21ZyzfQlk/edit#bookmark=id.aijtk3i8nlee) (internal only) to use Ruby instead of Go, despite Go having a relatively better performance (check [spike](https://gitlab.com/gitlab-org/gitlab/-/issues/423832 "Spike: Benchmark regex performance between Ruby and Go") results: [1](https://gitlab.com/gitlab-org/gitlab/-/issues/423832#note_1578076768 "Spike: Benchmark regex performance between Ruby and Go"), [2](https://gitlab.com/gitlab-org/gitlab/-/issues/423832#note_1579850558 "Spike: Benchmark regex performance between Ruby and Go")). On the other hand, invoking a Go binary from Rails would have required more work to be done, to name a few: * Evaluate execution cost (if running as a daemon). * Figure out communication mode (gRPC vs. shell pipe). * Ensure [security isolation](https://gitlab.com/gitlab-org/gitlab/-/issues/413274#note_1550184207 "Secret Detection continuous scanning") (internal only). * Hardware resource allocation, and interference with the Rails application resources. Additionally, there was an initial investment involved in making the Go binary official within the GitLab project (e.g. binary distribution, determine release cycles, making it secure, etc). We have also [agreed](https://docs.google.com/document/d/195BM7tAU9iquFkjTROtpuLIlbjfDVKzphZ21ZyzfQlk/edit#bookmark=id.b745m8kfzktj) (internal only) to evaluate the Ruby-based approach, release it gradually behind a feature flag, and monitor it aggressively, while keeping the Go-binary approach on the table in case the former did not have satisfactory results. **Ruby's built-in regex matching vs. `re2` gem** While evaluating Ruby vs. Go in [our benchmarking spike](https://gitlab.com/gitlab-org/gitlab/-/issues/423832 "Spike: Benchmark regex performance between Ruby and Go"), @vbhat161 noticed that `gitlab-org/gitlab` project [already uses](https://gitlab.com/gitlab-org/gitlab/-/blob/06f13ecb38c31fcb82c96d98e7ac905f057d76d4/Gemfile#L256) a Ruby wrapper of the popular [`re2`](https://github.com/google/re2) C++ library. So, together with @craigmsmith they benchmarked `re2.match` and `RE2::set` from the `re2` gem against Ruby's built-in regex library [`Regexp`](https://docs.ruby-lang.org/en/3.2/Regexp.html), and the results where highly in favour of using [precompiled](https://mudge.name/re2/RE2/Set.html#compile-instance_method) `RE2::Set` vs. built-in `Regexp` and `re2.match` method. Therefore, we should use [`RE2::Set`](https://mudge.name/re2/RE2/Set.html) and make sure it's precompiled with the regular expressions patterns loaded from `gitleaks.toml` file, before we match them against git blobs inside the gem. Please have a look at the [code they used for benchmarking](https://gitlab.com/craigmsmith/regex-performance) for example, and the results present in [the README](https://gitlab.com/craigmsmith/regex-performance/-/blob/main/README.md?ref_type=heads) for more information. **Configurable timeout** We want to make sure we're not blocking pushes in case regex matching is taking too long or there's a failure, therefore, it's important to have a circuit breaker in the gem (configurable via its interface) that stop execution and return early if execution times out. **Other considerations** Please check [this section](https://gitlab.com/gitlab-org/gitlab/-/issues/422574#secret-scanner-likely-based-on-httpsgitlabcomgitlab-orggitlab-issues413274 "Perform secret detection for highest risk content pre-receive") for some other considerations to keep in mind when building the gem. --- #### :two: Create a push check to run secrets detection scans on pushed code. To detect secrets before git pushes, we [considered](https://gitlab.com/gitlab-org/gitlab/-/issues/422574#other-discussed-solutions "Perform secret detection for highest risk content pre-receive") a number of approaches, namely: * Customer-built server-side hooks. * Using client-side pre-commit hooks. * Using [push checks](https://docs.gitlab.com/ee/development/internal_api/internal_api_allowed.html#push-checks) (invoked within the `/internal/allowed` API endpoint from a `pre-recieve` git hook in `gitaly`). And we have agreed to move forward with the push checks approach. Please refer to the [rationale section](https://gitlab.com/gitlab-org/gitlab/-/issues/422574#rationale "Perform secret detection for highest risk content pre-receive") to learn more. With that in mind, the plan is to create a new push check based on the work done in our earlier POCs: * https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131414+ * https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131583+ Such that: * We introduce a new push rule (e.g. `EE::GitLab::Checks::PushRules::SecretsCheck`). * We update `EE::Gitlab::Checks::PushRuleCheck` to run `SecretsCheck` along with other checks. * We enumerate through new blobs fetched from `gitaly` in the new push check. * We pass the blobs to `gitlab-secret_detection` gem for regex matching and: * If a secret is found, we raise a `::Gitlab::GitAccess::ForbiddenError` and display an error message with the failure. * If no secrets are matched, check passes successfully, and the push continues. There are a number of considerations to keep in mind though while developing the push check: **Enumerating through blobs** To enumerate through the blobs of a certain git push, we have two approaches: 1. `git-rev-list(1)-base` approach (via [`Gitlab::Git::Repository#new_blobs` method](https://gitlab.com/gitlab-org/gitlab/-/blob/2b2d4ba9b58886396751d0bef8de3dc895fb3897/lib/gitlab/git/repository.rb#L418-430)) 2. `quarantine-directory-based` approach (via [`Gitlab::GitalyClient::BlobService#list_all_blobs` method](https://gitlab.com/gitlab-org/gitlab/-/blob/2b2d4ba9b58886396751d0bef8de3dc895fb3897/lib/gitlab/gitaly_client/blob_service.rb#L25-49)) The latter is the most efficient way to fetch blobs when a quarantine directory is available. A quarantine directory is where `git` stages new objects before applying the reference updates. The presence of the `GIT_OBJECT_DIRECTORY_RELATIVE` environment variable indicates that a quarantine directory is used. However, the former comes in handy when a quarantine directory isn't available, as `list_all_blobs` would then fetch all blobs of the repository instead of the new blobs. In such case, we need to fall back to using `new_blobs` (or its underlying method [`list_blobs`](https://gitlab.com/gitlab-org/gitlab/-/blob/0b6f2264777032c564e047547ea6f677c0af95cc/lib/gitlab/gitaly_client/blob_service.rb#L51-69)) while passing the new revisions to the method to retrieve their corresponding blobs. Please read more on this topic in [this thread](https://gitlab.com/gitlab-org/gitlab/-/issues/423834#note_1556350434 "Spike: Add a push check that performs secret detection") and [this comment](https://gitlab.com/gitlab-org/gitlab/-/issues/422574#note_1526604961 "Perform secret detection for highest risk content pre-receive"). Also, check [`Gitlab::Checks::FileSizeCheck::HookEnvironmentAwareAnyOversizedBlobs`](https://gitlab.com/gitlab-org/gitlab/-/blob/0b6f2264777032c564e047547ea6f677c0af95cc/lib/gitlab/checks/file_size_check/hook_environment_aware_any_oversized_blobs.rb#L15-25) to see an example for how it works. **Limit scans to certain file sizes and types** As part of the [requirements](https://gitlab.com/gitlab-org/gitlab/-/issues/422574#secret-scanner-likely-based-on-httpsgitlabcomgitlab-orggitlab-issues413274 "Perform secret detection for highest risk content pre-receive"), and to ensure secret detection isn't blocking `git` pushes, we have to consider limiting our scanning to certain files sizes and types. Since secrets are often included in configuration files, that tend to be small in size, it would be advisable to limit to scanning to file size below a certain threshold, this can be done by utilising either one of: * [`Gitlab::Checks::FileSizeCheck::AnyOversizedBlobs`](https://gitlab.com/gitlab-org/gitlab/-/blob/0b6f2264777032c564e047547ea6f677c0af95cc/lib/gitlab/checks/file_size_check/any_oversized_blobs.rb) * [`Gitlab::Checks::FileSizeCheck::HookEnvironmentAwareAnyOversizedBlobs`](https://gitlab.com/gitlab-org/gitlab/-/blob/0b6f2264777032c564e047547ea6f677c0af95cc/lib/gitlab/checks/file_size_check/hook_environment_aware_any_oversized_blobs.rb) So that we dimiss large blobs from being sent over to `gitlab-secret_detection` for scanning to begin with. Another thing to consider is skipping binary files from scanning, which include lots of high entropy strings anyway, and might lead to false positives. **Handling false positives** To ensure customers could skip pre-recieve secret detection, for example in cases where a developer need to commit a dummy secret that is used in tests, we decided to use a special commit message flag [similar to `[ci skip]`](https://docs.gitlab.com/ee/ci/pipelines/#skip-a-pipeline). This will happen as part of the push check, in which we will check for the presence of this flag (e.g. `[skip secret detection]`) in any of the commit messages for a certain `git` push. If the flag is present, the push check returns early, and no blobs will be sent to `gitlab-secret_detection` gem for scanning. Please check [this thread](https://gitlab.com/gitlab-org/gitlab/-/issues/423834#note_1553609088 "Spike: Add a push check that performs secret detection") and [related POC](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131583 "Draft: POC for skipping secrets detection push check if flag is present in commit message") for an example of how this might work. **Releasing the feature behind a feature flag** To ensure the gradual roll-out of this feature, we have agreed to put it behind a feature flag. Checking the flag will have to happen inside the push check, so that we only invoke `gitlab-secret_detection` gem if the flag is enabled for a [certain actor](https://docs.gitlab.com/ee/development/feature_flags/index.html#feature-actors) (e.g. a `project` or `group`). Please also check the process for [developing with feature flags](https://docs.gitlab.com/ee/development/feature_flags/index.html#feature-flags-in-gitlab-development) and [rolling them out](https://gitlab.com/gitlab-org/gitlab/-/blob/master/.gitlab/issue_templates/Feature%20Flag%20Roll%20Out.md). --- #### :three: Telemetry To better inform ourselves on what kind of telemetry is needed, we have to consider a few questions: * What kind of logs are needed? * Where is those logs stored, and how often they are rotated? * What metrics to track, and in which dashboards? --- #### :four: Performance monitoring/profiling Since this feature is going to be in the "hot path" for every `git` push, we have to understand how it will impact the overall performance of our systems, and what sort of monitoring and profiling we need to have in place. There are a number of things to especially measure and aggregate: * CPU load * Memory consumption * Latency And we likely should also spend a bit of time to find out the average commit size and average git push events/sec, among other things, in order to determine the thershold/maximum allowed commit/file/blob size. --- #### :five: Production readiness As a final step, we will likely need to ensure the production readiness of the MVC, that includes: * Perform any required code security checks (especially for the new Ruby gem). * Document the feature properly, and make sure support is aware/prepared to handle customers using it. ### Architecture Below is a sequence diagram showing an overview of the architecture for this MVC given all the components discussed in the proposal above. ```mermaid sequenceDiagram actor User User->>+Workhorse: git push Workhorse->>+Gitaly: tcp Gitaly->>+Gitaly: PostReceivePack Gitaly->>+Gitaly: PreReceiveHook Gitaly->>+Rails: grpc Note over Gitaly, Rails: invokes /internal/allowed endpoint Rails->>+Rails: EE::Gitlab::Checks::SecretsCheck Note over Rails: runs SecretsCheck break when special commit message flag is found Rails->>+Gitaly: push check skipped Gitaly->>+Workhorse: outcome of push Workhorse->>+User: outcome of push end Rails->>+Gitaly: ListBlobs or ListAllBlobs Note over Gitaly, Rails: if quarantine directory exists/not Gitaly->>+Rails: grpc Rails->>+gitlab-secret_detection: gitlab-secret_detection::Scan(blobs) alt no secret detected gitlab-secret_detection->>+gitlab-secret_detection: scan blob gitlab-secret_detection->>+Rails: success Rails->>+Gitaly: accept - no secret detected else scan timeout gitlab-secret_detection->>+gitlab-secret_detection: scan blob gitlab-secret_detection->>+Rails: fail - timeout Rails->>+Gitaly: accept - scan timeout else secret detected gitlab-secret_detection->>+gitlab-secret_detection: scan blob gitlab-secret_detection->>+Rails: fail - secret found Rails->>+Gitaly: reject - secret detected end Gitaly->>+Workhorse: outcome of push Workhorse->>+User: outcome of push ``` ### Implementation Plan See epics and issues below.