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

Scan only PR commits for Gitleaks instead of whole codebase #2504

Merged
merged 30 commits into from
May 13, 2023
Merged

Scan only PR commits for Gitleaks instead of whole codebase #2504

merged 30 commits into from
May 13, 2023

Conversation

DariuszPorowski
Copy link
Contributor

@DariuszPorowski DariuszPorowski commented Mar 31, 2023

Fixes #2487

Proposed Changes

  1. --redact flag for gitleaks should be default to prevent exposing detected secrets to logs
  2. common functions consolidated into utils, based on review comment: Scan only PR commits for Gitleaks instead of whole codebase #2504 (comment)
  3. CI env use case, based on discussion: AzureCommentReporter vars reflect official Azure DevOps naming #2510 (comment)
  4. Missed doc fixes in [documentation] runner - doc fix for env list of values #2449 (fixes Not possible to pass array of values for envs with local runner (cli) #2448)
  5. updated url to new gitleaks home: https://github.com/zricethezav/gitleaks -> https://github.com/gitleaks/gitleaks

github

ado

gitlab

Readiness Checklist

Author/Contributor

  • Add entry to the CHANGELOG listing the change and linking to the corresponding issue (if appropriate)
  • If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

  • Label as breaking if this is a large fundamental change
  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

@echoix
Copy link
Collaborator

echoix commented Mar 31, 2023

For the pyproject.toml, you can keep it for another PR, it's already in the plans to switch to it, but neither of us had time to learn about it yet! If you are used to it, we'll gladly accept a first implementation 😄
When building, there is a warning saying pip version 23 will enforce it, ie something like

DEPRECATION: xxxxxxx is being installed using the legacy 'setup.py install' method, because it does not have a 'pyproject.toml' and the 'wheel' package is not installed. pip 23.1 will enforce this behaviour change. A possible replacement is to enable the '--use-pep517' option. Discussion can be found at pypa/pip#8559

So we'll need to go to it a way or another.

@DariuszPorowski
Copy link
Contributor Author

@echoix that's the reason I put the note in the 1st line NOTE: pyproject.toml and poetry is just for my dev env, will remove on ready PR - so do not worry about these files for now :)
I can prapare PR with pyproject.toml and poetry in the future, poetry with venv changed my life ;)

@Kurt-von-Laven
Copy link
Collaborator

Yeah, Poetry is great!

@DariuszPorowski
Copy link
Contributor Author

@nvuillam +/- this PR is ready, minor cleanup, and changes left, but have Q around REPOSITORY_GITLEAKS_PR_COMMITS_SCAN. Currently, it is false but happy to put true as default because then validate all code base = false will does the job without any extra envs. But this is a kind of braking change and may impact current users' setup.
So, I'm happy to hear feedback and design decisions.

@nvuillam
Copy link
Member

nvuillam commented Apr 4, 2023

@DariuszPorowski it looks great, i'll check that tomorrow, thanks for the PR:)

@nvuillam
Copy link
Member

nvuillam commented Apr 4, 2023

Cc @Kurt-von-Laven @bdovaz @echoix

@DariuszPorowski
Copy link
Contributor Author

@nvuillam no rush, take your time. Tomorrow I am off so will continue on Thursday.

@DariuszPorowski
Copy link
Contributor Author

@nvuillam just following up :)

@nvuillam
Copy link
Member

@DariuszPorowski you have many failing test cases ^^

@DariuszPorowski
Copy link
Contributor Author

@nvuillam +/- this PR is ready, minor cleanup, and changes left, but have Q around REPOSITORY_GITLEAKS_PR_COMMITS_SCAN. Currently, it is false but happy to put true as default because then validate all code base = false will does the job without any extra envs. But this is a kind of braking change and may impact current users' setup.
So, I'm happy to hear feedback and design decisions.

@nvuillam actually follow up was on this question not on the code hehe ;)

@nvuillam
Copy link
Member

@DariuszPorowski if it breaks existing config of ML using gitleaks and VALIDATE_ALL_CODEBASE=false , i'd prefer to stay deactivated by default, to avoid a breaking change like u said ^^

@DariuszPorowski DariuszPorowski changed the title [WIP] Scan only PR commits for Gitleaks instead of whole codebase Scan only PR commits for Gitleaks instead of whole codebase Apr 30, 2023
@DariuszPorowski DariuszPorowski marked this pull request as ready for review April 30, 2023 04:59
@DariuszPorowski
Copy link
Contributor Author

@nvuillam I'm sorry it took so long but the last month has been hectic at work. PR ready for review :)

@nvuillam
Copy link
Member

nvuillam commented Apr 30, 2023

@DariuszPorowski same here, I understand 🤣

Your PR looks great but I have some doubts about the documentation... it seems you directly updated generated markdown file https://github.com/oxsecurity/megalinter/blob/cff15a01ef2d29d904b97b268df8b54225429b4a/docs/descriptors/repository_gitleaks.md , so the next time the doc will be rebuilt, it will be overwritten

If you want "free text" doc to be added, you need to update linter_description property in the YML descriptor file :)

You can verify that by yourself using bash build.sh --doc locally

@nvuillam
Copy link
Member

@DariuszPorowski Same remark about custom variables: if you add them in the descriptor, they will be automatically added to the doc & json schema ^^

image

@DariuszPorowski
Copy link
Contributor Author

@nvuillam Oh gosh, the last changes to te main were painful to integrate with this PR :D hope now is good, docs generated automatically

@nvuillam
Copy link
Member

nvuillam commented May 2, 2023

@DariuszPorowski i can assure you they were even harder to implement 😅

But it's for the better good: with next version you'll just have to trust MegaLinter core code with your env variables (including secrets defined in CI/CD), no need anymore to trust the dozens of embedded linters 😎

@nvuillam
Copy link
Member

@DariuszPorowski plz can u merge main in your branch ? :)

@DariuszPorowski
Copy link
Contributor Author

Sure, will do over the weekend

@nvuillam
Copy link
Member

I did it :)

@nvuillam nvuillam merged commit 7ff24de into oxsecurity:main May 13, 2023
Copy link
Member

@nvuillam nvuillam left a comment

Choose a reason for hiding this comment

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

@DariuszPorowski thanks for this great PR :)

@DariuszPorowski DariuszPorowski deleted the 2487-gitleaks-pr branch May 14, 2023 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants