Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: git plugin - option to limit depth of historical scans #118

Merged
merged 10 commits into from
Jun 29, 2023

Conversation

tcdsv
Copy link
Contributor

@tcdsv tcdsv commented Jun 28, 2023

Closes #94

  • add the depth field to the GitPlugin struct
  • add the depth option to the git plugin command
  • create a function called buildScanOptions to generate a string of scanning options for the gitleaks GitLog function
  • by default, gitleaks GitLog function scans using --full-history and --all options (see: https://github.com/gitleaks/gitleaks/blob/master/detect/git/git.go#L44). The reason these options are embedded in buildScanOptions is to maintain this behavior
  • tested manually

Proposed Changes

  • feat: add --depth <number> option to git plugin command

Additional Considerations

  • GitLog --all option scans the entire repo (including all branches). users may prefer to scan only a specific branch instead of the entire repository.
  • Not directly related, but the current behavior of the git plugin is to skip deleted files (https://github.com/Checkmarx/2ms/blob/master/plugins/git.go#L48). In case there is an unnoticed leak in a deleted file, the secret will still exist in the git history and will be missed.

I submit this contribution under the Apache-2.0 license.

@baruchiro baruchiro self-requested a review June 28, 2023 08:20
Copy link
Member

@jossef jossef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please see comments below

plugins/git.go Outdated
func (p *GitPlugin) buildScanOptions() string {
options := ""
if p.Depth > 0 {
options = fmt.Sprintf("--full-history --all -n %d", p.Depth)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
options = fmt.Sprintf("--full-history --all -n %d", p.Depth)
options = fmt.Sprintf("-n %d", p.Depth)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only to scan current checked-out branch. can you explain --full-history in this context?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by default, gitleaks GitLog function scans using --full-history and --all options (see: https://github.com/gitleaks/gitleaks/blob/master/detect/git/git.go#L44). The reason these options are embedded in buildScanOptions is to maintain this behavior

From the PR description

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

additional info:
https://www.git-scm.com/docs/git-log#Documentation/git-log.txt-Defaultmode history is pruned without --full-history option

I think it makes sense to make --all optional for all usages of this plugin, it will be more consistent and less confusing from the user's perspective.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks for the info. I'm okay with --full-history

regarding --all:

we can have our own version of --all / --all-branches optional arg which will include all commits from all branches.
by default (if not provided) - without --all = only the currently checked-out branch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • made --all optional and updated buildScanOptions function
  • added scanAllBranches boolean field to GitPlugin struct
  • limited the scope of Depth field in GitPlugin struct by changing it to depth

Copy link
Contributor

@baruchiro baruchiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good, one more step and we will be there!

plugins/git.go Outdated

"github.com/gitleaks/go-gitdiff/gitdiff"
"github.com/rs/zerolog/log"
"github.com/spf13/cobra"
"github.com/zricethezav/gitleaks/v8/detect/git"
)

const (
argDepth = "depth"
argScanAllBranches = "all"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please call it all-branches, otherwise it is not clear all- what?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

plugins/git.go Outdated
},
}

flags := command.Flags()
flags.BoolVar(&p.scanAllBranches, argScanAllBranches, false, "scan all branches")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it will print default: false in the help message?

If not, please add [default: false] to the end of the description.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

plugins/git.go Outdated Show resolved Hide resolved
Copy link
Member

@jossef jossef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@baruchiro baruchiro merged commit 514e07a into Checkmarx:master Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

git plugin - option to limit depth of historical scans (default all commits)
3 participants