Revamp secrets check bypass mechanism

Overview

As part of our effort to introduce the experimental version of pre-receive secret detection feature, we have made a decision to use a special commit flag, i.e. [skip secret detection], in order to skip scanning for an entire push, if the flag is present in any of the commits pushed.

However, this design has a number of limitations:

  1. A push could potentially include many commits, and having the flag added to only one commit message is not very auditable.
  2. A user would have to add the flag each time they're committing changes to a file with a false-positive secret, even if they did include it earlier (in a commit that is not part of the merge request where new changes are introduced).

To resolve these issues, we should consider to revamp the bypass mechanism in secrets push check to:

Please also read discussions (1, 2) below for more information.

Future options (beyond Beta)

We should consider implementing another mechanisms to bypass single results or lines beyond the Beta.

Proposed options include:

  1. A special comment (e.g. # gitleaks:allow or another flag of our choice) to bypass scanning for a single line. (This would work the same as pipeline-based secret detection. However, security teams have asked in the past to be able to disable this type of in-code override.)
    • This would be implemented by updating gitlab-secret_detection gem to skip scanning lines that has # gitleaks:allow comment or similar flag of our choice.
  2. Maintain a project- or organization-specific allowlist for values that are determined not to be true secrets. (This would have the downside of developers not being able to resolve the detection themselves, but many security teams prefer this type of centralized control.)

Original Discussion

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

  • @samihiltunen started a discussion: (+3 comments)

    So if any of the commits includes the skip flag, we'd skip the scan for the entire push? If so, I'd again nudge towards considering push options for this. If a flag in any of the commits skips scanning the rest of the commits, the commit messages are not really auditable as mentioned in this thread. A secret could be included in a commit if it happened to be bundled in a push with a commit that included the flag. The commit would have no indication that the secret scanning was skipped though.

    If that is not an issue, push options seem like a better fit given they don't require the user to modify commits. UX wise it's clearer that a push options applies to the entire push as opposed to a flag in any of the commits. This is more about the feature design though.

    If skipping scanning rest of the commits is desired, it would be good to add a test for the behavior that secrets are ignored in other commits as well.

Proposal

In a recent Looking forward discussion, a decision was made to focus our effort in Beta on the tasks mentioned below.

  • Introduce a new push option to bypass scanning for an entire push.
  • Update secrets push check to support the new push option in addition to the existing capability (special commit flag).
  • Update documentation to reflect the new capability (i.e. the push option can be used to skip an entire push).

Implementation

  1. Update gitaly to pass pushOptions as a param in the request to /internal/allowed endpoint.
    1. This will imitate what is being done in the /internal/post_receive request.
    2. This will likely require a change to the allowedRequest and AllowedParams types as well.
    3. This may require a slight change in the prereceive hook too.
  2. Add new push option to lib/gitlab/push_options.rb.
  3. Check if an update to /internal/allowed endpoint is required to handle the push_options param passed from gitaly.
  4. Update /lib/api/helpers/internal to pass push_options param to access checkers.
  5. Update /lib/gitlab/git_access to handle push_options param in all necessary methods.
  6. Update /lib/gitlab/checks/changes_access class to receive and pass down the push_options param.
  7. The push_options param will now be part of changes_access object (available in ee/lib/gitlab/checks/secrets_check.rb).
Edited by Ahmed Hemdan