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 variant analysis scanning #2041

Merged
merged 3 commits into from
Sep 12, 2023
Merged

Conversation

dfarrell07
Copy link
Member

This is a different type of static analysis than others we run.

Variant analysis is the process of using a known security
vulnerability as a seed to find similar problems in your code.

https://codeql.github.com/docs/codeql-overview/about-codeql/

CodeQL doesn't only do variant analysis for security issues, it also has semantic queries/rules for other types of issues.

https://github.com/github/codeql/tree/main/go/ql/src

It identified new issues (already fixed) that our other tools missed.

The company that built it was bought by GitHub and the tool is being integrated into GitHub's security workflow.

Add one unprivileged version of the job to gate PRs and one privileged version on-merge to report results.

Relates-to: #1970
Signed-off-by: Daniel Farrell [email protected]

@submariner-bot
Copy link
Contributor

🤖 Created branch: z_pr2041/dfarrell07/codeql3
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

@dfarrell07
Copy link
Member Author

dfarrell07 commented Sep 29, 2022

There's still an issue with reproducing negative results: #1970 (comment)

But checks are clearly running, and I think we're using the GHA as we should be. Maybe it's okay to merge?

@skitt
Copy link
Member

skitt commented Sep 30, 2022

Could you try adding a code change that this should flag, just to check whether it finds issues?

@dfarrell07
Copy link
Member Author

Could you try adding a code change that this should flag, just to check whether it finds issues?

Yeah, that's what I was doing to try to reproduce negative results. One example (there are others linked in the tracking issue): dfarrell07#54

@dfarrell07
Copy link
Member Author

dfarrell07 commented Oct 3, 2022

Could you try adding a code change that this should flag, just to check whether it finds issues?

Yeah, that's what I was doing to try to reproduce negative results. One example (there are others linked in the tracking issue): dfarrell07#54

Better example based on exactly this change: dfarrell07#56

@skitt
Copy link
Member

skitt commented Oct 17, 2022

Better example based on exactly this change: dfarrell07#56

Where are the results supposed to show up? The check says “No new or fixed alerts” which suggests it didn’t find anything amiss.

@dfarrell07
Copy link
Member Author

Better example based on exactly this change: dfarrell07#56

Where are the results supposed to show up? The check says “No new or fixed alerts” which suggests it didn’t find anything amiss.

Right, that's the issue, we fixed all the issues flagged by CodeQL (run through LGTM.com) before I was able to verify the negative case. I the linked example I revert a fix and it still passes.

@stale
Copy link

stale bot commented Nov 12, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Nov 12, 2022
@dfarrell07 dfarrell07 removed the wontfix This will not be worked on label Nov 15, 2022
@tpantelis
Copy link
Contributor

Is this PR still relevant?

@dfarrell07
Copy link
Member Author

Is this PR still relevant?

I'd still like to merge it as I think it's helpful, I just can't show it's actually catching issues. So to be explicit, I'd like to merge it although it's a bit below the quality standard I typically strive for.

@stale
Copy link

stale bot commented Dec 31, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Dec 31, 2022
@dfarrell07 dfarrell07 removed the wontfix This will not be worked on label Jan 3, 2023
@stale
Copy link

stale bot commented Jan 21, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Jan 21, 2023
@dfarrell07 dfarrell07 removed the wontfix This will not be worked on label Jan 24, 2023
@tpantelis
Copy link
Contributor

I'd still like to merge it as I think it's helpful, I just can't show it's actually catching issues. So to be explicit, I'd like to merge it although it's a bit below the quality standard I typically strive for.

I see that submariner-operator is already running this tool. But I'm not clear on where we can view or use the results...

@dfarrell07
Copy link
Member Author

I'd still like to merge it as I think it's helpful, I just can't show it's actually catching issues. So to be explicit, I'd like to merge it although it's a bit below the quality standard I typically strive for.

I see that submariner-operator is already running this tool. But I'm not clear on where we can view or use the results...

It publishes SARIF reports that GitHub automatically consumes and shows in the Security tab.

https://github.com/submariner-io/submariner-operator/security/code-scanning?query=branch%3Adevel+tool%3ACodeQL+

This is a different type of static analysis than others we run.

> Variant analysis is the process of using a known security
vulnerability as a seed to find similar problems in your code.

https://codeql.github.com/docs/codeql-overview/about-codeql/

CodeQL doesn't only do variant analysis for security issues, it also has
semantic queries/rules for other types of issues.

https://github.com/github/codeql/tree/main/go/ql/src

It identified new issues (already fixed) that our other tools missed.

The company that built it was bought by GitHub and the tool is being
integrated into GitHub's security workflow.

Add one unprivileged version of the job to gate PRs and one privileged
version on-merge to report results.

Relates-to: submariner-io#1970
Signed-off-by: Daniel Farrell <[email protected]>
@stale
Copy link

stale bot commented Mar 18, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Mar 18, 2023
@dfarrell07 dfarrell07 removed the wontfix This will not be worked on label Mar 21, 2023
@stale
Copy link

stale bot commented Jun 10, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Jun 10, 2023
@maayanf24 maayanf24 removed the wontfix This will not be worked on label Jun 13, 2023
@stale
Copy link

stale bot commented Aug 12, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Aug 12, 2023
@stale stale bot removed the wontfix This will not be worked on label Aug 16, 2023
@submariner-bot submariner-bot added the ready-to-test When a PR is ready for full E2E testing label Sep 12, 2023
@tpantelis tpantelis enabled auto-merge (rebase) September 12, 2023 13:49
@tpantelis tpantelis merged commit 2e1770c into submariner-io:devel Sep 12, 2023
@submariner-bot
Copy link
Contributor

🤖 Closed branches: [z_pr2041/dfarrell07/codeql3]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation ready-to-test When a PR is ready for full E2E testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants