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

[ci] Add CredScan static security analysis #4590

Closed
wants to merge 1 commit into from

Conversation

shiyu1994
Copy link
Collaborator

Add CredScan static security analysis. This is a part of the original pull request #4585. We separate each task into a standalone PR for better reviewability.

@StrikerRUS
Copy link
Collaborator

Should the new job fail when it finds something wrong? Right now it finds passwords in our docker files but finishes successfully after that.

...
docker/README.md:docker/README.md(112,32): Error CSCAN-GENERAL0060: General Password: A potential secret was detected in 'README.md':(CSCAN-GENERAL0060 General Password) Validate file contains secrets, remove, roll credential, and use approved store. For additional information on secret remediation see https://aka.ms/credscan.
docker/gpu/README.md:docker/gpu/README.md(25,18): Error CSCAN-GENERAL0060: General Password: A potential secret was detected in 'README.md':(CSCAN-GENERAL0060 General Password) Validate file contains secrets, remove, roll credential, and use approved store. For additional information on secret remediation see https://aka.ms/credscan.
  Saving exported Sarif v2 file: D:\a\1\.gdn\a\Results.sarif
  Active results: 2
  Skipped results: 0
    Baselined results: 0
    Suppressed results: 0
    Results excluded by tool filters: 0
    Results below minimum severity: 0
    Results classified as Pass: 0

@StrikerRUS
Copy link
Collaborator

Also, could you please clarify why do you think LightGBM repo needs this analysis? It seems that leaving credentials in a source code is not something that can happen in our source code.

@shiyu1994
Copy link
Collaborator Author

These static security analysis are required by the open source policy of Microsoft. But we can choose not to add these analysis into the ci job of master branch. Instead, we may keep them in a separate branch, and re-open the PR to get the analysis results before each release. Since we only need the analysis results.

@StrikerRUS
Copy link
Collaborator

StrikerRUS commented Sep 25, 2021

These static security analysis are required by the open source policy of Microsoft.

Ah, I see. OK.

But we can choose not to add these analysis into the ci job of master branch.

I think it should depend on a required time for this job. If it's insignificant, we can run this job for each commit. Otherwise, we can add it to our optional checks triggered by comments similarly to how we regenerate configure file in the R-package or run long valgrind tests:

- name: Trigger R valgrind tests
if: github.event.comment.body == '/gha run r-valgrind'
run: |
$GITHUB_WORKSPACE/.ci/trigger_dispatch_run.sh \
"${{ github.event.issue.pull_request.url }}" \
"${{ github.event.comment.id }}" \
"gha_run_r_valgrind"
- name: Trigger R Solaris CRAN checks
if: github.event.comment.body == '/gha run r-solaris'
run: |
$GITHUB_WORKSPACE/.ci/trigger_dispatch_run.sh \
"${{ github.event.issue.pull_request.url }}" \
"${{ github.event.comment.id }}" \
"gha_run_r_solaris"
- name: Trigger update R configure
if: github.event.comment.body == '/gha run r-configure'
run: |
$GITHUB_WORKSPACE/.ci/trigger_dispatch_run.sh \
"${{ github.event.issue.pull_request.url }}" \
"${{ github.event.comment.id }}" \
"gha_run_r_configure"

And then add it to our release checklist (#4169 (comment)).
I believe it will be better than

keep them in a separate branch, and re-open the PR to get the analysis results before each release.

@shiyu1994
Copy link
Collaborator Author

@StrikerRUS Oh, thank you. Adding these checks as optional items is enough. That's a good suggestion. I will add it there.

@StrikerRUS
Copy link
Collaborator

@shiyu1994 I can help with adding new ChatOps command. But new job should be run at GitHub Actions, not at Azure Pipelines.

@jameslamb
Copy link
Collaborator

What is the status of this PR? @shiyu1994 is there something you need help with?

@shiyu1994
Copy link
Collaborator Author

@StrikerRUS @jameslamb Thanks, I'll move these new static analysis jobs to ci actions by updating this PR.

@jameslamb
Copy link
Collaborator

@shiyu1994 it's been a few months without activity on this PR. Can it be closed?

@shiyu1994
Copy link
Collaborator Author

Yes. I think it is not necessary to include this in the repo. I will manually do the static check when needed. Thanks!

Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants