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:
- A push could potentially include many commits, and having the flag added to only one commit message is not very auditable.
- 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:
- Use push options to bypass scanning for the entire push.
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:
- 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.
- This would be implemented by updating
- 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
- Update
gitaly
to passpushOptions
as a param in the request to/internal/allowed
endpoint.- This will imitate what is being done in the
/internal/post_receive
request. - This will likely require a change to the
allowedRequest
andAllowedParams
types as well. - This may require a slight change in the
prereceive
hook too.
- This will imitate what is being done in the
- Add new push option to
lib/gitlab/push_options.rb
. - Check if an update to
/internal/allowed
endpoint is required to handle thepush_options
param passed fromgitaly
. - Update
/lib/api/helpers/internal
to passpush_options
param to access checkers. - Update
/lib/gitlab/git_access
to handle push_options param in all necessary methods. - Update
/lib/gitlab/checks/changes_access
class to receive and pass down thepush_options
param. - The
push_options
param will now be part ofchanges_access
object (available inee/lib/gitlab/checks/secrets_check.rb
).