Skip to content
New issue

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

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

Already on GitHub? # 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
tcdsv and others added 3 commits June 28, 2023 17:45
Co-authored-by: Baruch Odem (Rothkoff) <baruchiro@gmail.com>
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
# for free to join this conversation on GitHub. Already have an account? # 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