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

Add CodeQL Security Scan #770

Merged
merged 5 commits into from
May 21, 2021
Merged

Add CodeQL Security Scan #770

merged 5 commits into from
May 21, 2021

Conversation

KKelvinLo
Copy link
Member

Motivation

This PR is a follow-up to issue open-telemetry/oteps#144

CodeQL is GitHub's static analysis engine which scans repos for security vulnerabilities. As the project grows and we near GA it might be useful to have a workflow which checks for security vulnerabilities to ensure that every incremental change is following best development practices. Also passing basic security checks will also make sure that there aren't any glaring issues for our users.

Changes

  • This PR adds CodeQL security checks to the repository
  • After every run the workflow will upload the results to GitHub.

Workflow Triggers

  • daily cron job at 1:30 am
  • workflow_dispatch (in the case that the maintainers want to trigger a manual security check)
  • CHANGELOG.md updated for non-trivial changes
  • [] Unit tests have been added
  • [] Changes in public API reviewed

@KKelvinLo KKelvinLo changed the title Add codeql Add CodeQL Security Scan May 20, 2021
@KKelvinLo KKelvinLo marked this pull request as ready for review May 20, 2021 01:07
@KKelvinLo KKelvinLo requested a review from a team May 20, 2021 01:07
@codecov
Copy link

codecov bot commented May 20, 2021

Codecov Report

Merging #770 (49b9d18) into main (af9cd15) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #770   +/-   ##
=======================================
  Coverage   95.95%   95.95%           
=======================================
  Files         176      176           
  Lines        7172     7172           
=======================================
  Hits         6882     6882           
  Misses        290      290           

rename workflow

update changelog

update format

update format
@maxgolov
Copy link
Contributor

@KKelvinLo - could you please take a look why the check hasn't been showing in the list of successful checks? You may want to adjust the template a bit for the results to show under the Security tab, maybe naming the step as CodeQL.

Template that's been missing the setup parts:
https://github.com/open-telemetry/opentelemetry-cpp/pull/654/files

@maxgolov
Copy link
Contributor

Maybe it's all good already. Perhaps it'd show the results after merge?... 🤷

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

Thank you for adding this. Will the CodeQL workflow fails if there are any security vulnerabilities, or it just uploads the result somewhere and we need to periodically keep checking it?

@@ -0,0 +1,36 @@
name: "CodeQL Code Scan"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
name: "CodeQL Code Scan"
name: "CodeQL"

That should show the code scan result under Security tab?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep it if is enabled in the security tab, the action will post to the alerts

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to click on that in the Security tab or do we need to rename it to CodeQL ?

@KKelvinLo
Copy link
Member Author

Thank you for adding this. Will the CodeQL workflow fails if there are any security vulnerabilities, or it just uploads the result somewhere and we need to periodically keep checking it?

This workflow will post the results under the security tab, the workflow will pass

@KKelvinLo
Copy link
Member Author

@KKelvinLo - could you please take a look why the check hasn't been showing in the list of successful checks? You may want to adjust the template a bit for the results to show under the Security tab, maybe naming the step as CodeQL.

Template that's been missing the setup parts:
https://github.com/open-telemetry/opentelemetry-cpp/pull/654/files

It should be good now - changed it from a cron schedule to on pull request and on push

@maxgolov
Copy link
Contributor

Code scanning results / CodeQL Timed out after 109m

It got crashed on recursive submodules?! 😀

I guess we need to exclude third_party directory.

@alolita
Copy link
Member

alolita commented May 20, 2021

😀 @KKelvinLo can you please exclude the third-party directories. Let's make sure the workflow is robust. Thanks!

Thanks @maxgolov @ThomsonTan @lalitb for reviewing and merging!

@KKelvinLo
Copy link
Member Author

KKelvinLo commented May 20, 2021

Code scanning results / CodeQL Timed out after 109m

It got crashed on recursive submodules?! 😀

I guess we need to exclude third_party directory.

After digging through GitHub Actions documentation it doesn't look like there's an explicit exclude directory option, so I added a bash command to remove the third_party directory after checking out the code.

@lalitb
Copy link
Member

lalitb commented May 21, 2021

Code scanning results / CodeQL Timed out after 109m

It got crashed on recursive submodules?! 😀
I guess we need to exclude third_party directory.

After digging through GitHub Actions documentation it doesn't look like there's an explicit exclude directory option, so I added a bash command to remove the third_party directory after checking out the code.

Thanks. Instead of removing the third-party folder later, we can also checkout the repo without submodules :). For now, can you rebase the branch with main, as that will allow us to merge the PR

@KKelvinLo
Copy link
Member Author

Rebased!

@lalitb lalitb merged commit b6bf10f into open-telemetry:main May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set up CodeQL GitHub action for OpenTelemetry C++ SDK repo
5 participants