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 #1970

Closed
dfarrell07 opened this issue Aug 18, 2022 · 8 comments
Closed

Add CodeQL variant analysis scanning #1970

dfarrell07 opened this issue Aug 18, 2022 · 8 comments
Assignees
Labels
automation wontfix This will not be worked on

Comments

@dfarrell07
Copy link
Member

dfarrell07 commented Aug 18, 2022

As originally discussed on #794, CodeQL is a different type of scanner than the others we run and identifies new issues.

Variant analysis is the process of using a known security vulnerability as a seed to find similar problems in your code. It’s a technique that security engineers use to identify potential vulnerabilities, and ensure these threats are properly fixed across multiple codebases.
Querying code using CodeQL is the most efficient way to perform variant analysis. You can use the standard CodeQL queries to identify seed vulnerabilities, or find new vulnerabilities by writing your own custom CodeQL queries. Then, develop or iterate over the query to automatically find logical variants of the same bug that could be missed using traditional manual techniques.

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

See also: https://www.youtube.com/watch?v=5beYejYfhjY

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

The issues it found are already being fixed in various PRs, but we should likely also add CodeQL linting to keep out similar issues.

Depends on #1969
Depends on stolostron/submariner-addon#474
Depends on submariner-io/subctl#224
Depends on submariner-io/submariner-operator#2212
Depends on submariner-io/submariner-operator#2213
Depends on submariner-io/submariner-operator#2219
Depends on submariner-io/lighthouse#866
Depends on submariner-io/admiral#411
Depends on submariner-io/cloud-prepare#372
Depends on submariner-io/coastguard#81
Depends on submariner-io/subctl#244
Depends on #1994
Depends on submariner-io/submariner-operator#2259

Original issues:

lgmt_codeql_scan_subadd
lgmt_codeql_scan_subop
lgmt_codeql_scan_subctl
lgmt_codeql_scan_subm

Originally posted by @dfarrell07 in #794 (comment)

@dfarrell07 dfarrell07 self-assigned this Aug 18, 2022
@dfarrell07
Copy link
Member Author

@skitt do you agree this is useful, something we should implement?

@skitt
Copy link
Member

skitt commented Aug 25, 2022

This does seem worthwhile, especially since it’s being integrated into GitHub code scanning.

dfarrell07 added a commit to dfarrell07/submariner-operator that referenced this issue Aug 25, 2022
This is a different type of static analysis than any others we run.
It identifies new issues that are missed by our other tools.

Relates-to: submariner-io/submariner#1970
Signed-off-by: Daniel Farrell <[email protected]>
dfarrell07 added a commit to dfarrell07/submariner-operator that referenced this issue Aug 25, 2022
This is a different type of static analysis than any others we run.
It identifies new issues that are missed by our other tools.
The company that built it was bought by GitHub and the tool is being
integrated into GitHub's security workflow.

Relates-to: submariner-io/submariner#1970
Signed-off-by: Daniel Farrell <[email protected]>
dfarrell07 added a commit to dfarrell07/submariner-operator that referenced this issue Aug 26, 2022
This is a different type of static analysis than others we run.
It identifies new issues that are missed by our other tools.
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
one to report results on merges.

Relates-to: submariner-io/submariner#1970
Signed-off-by: Daniel Farrell <[email protected]>
dfarrell07 added a commit to dfarrell07/submariner-operator that referenced this issue Aug 26, 2022
This is a different type of static analysis than others we run.

It identified new issues (already fixed) that our other tool 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/submariner#1970
Signed-off-by: Daniel Farrell <[email protected]>
dfarrell07 added a commit to dfarrell07/submariner-operator that referenced this issue Aug 26, 2022
This is a different type of static analysis than others we run.

It identified new issues (already fixed) that our other tool 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/submariner#1970
Signed-off-by: Daniel Farrell <[email protected]>
tpantelis pushed a commit to submariner-io/submariner-operator that referenced this issue Aug 26, 2022
This is a different type of static analysis than others we run.

It identified new issues (already fixed) that our other tool 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/submariner#1970
Signed-off-by: Daniel Farrell <[email protected]>
@dfarrell07
Copy link
Member Author

I thought there might be an issue where we were building twice, but it turns out init doesn't build for golang despite very confusing docs.

dfarrell07 added a commit to dfarrell07/submariner-operator that referenced this issue Aug 30, 2022
This job was called vulnerability variant analysis during most of
development. It should have been reordered when the name was shortened.

Relates-to: submariner-io/submariner#1970
Signed-off-by: Daniel Farrell <[email protected]>
dfarrell07 added a commit to dfarrell07/lighthouse that referenced this issue Aug 30, 2022
This is a different type of static analysis than others we run.

It identified new issues (already fixed) that our other tool 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/submariner#1970
Signed-off-by: Daniel Farrell <[email protected]>
dfarrell07 added a commit to dfarrell07/admiral that referenced this issue Aug 30, 2022
This is a different type of static analysis than others we run.

It identified new issues (already fixed) that our other tool 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/submariner#1970
Signed-off-by: Daniel Farrell <[email protected]>
dfarrell07 added a commit to dfarrell07/admiral that referenced this issue Aug 30, 2022
This is a different type of static analysis than others we run.

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/submariner#1970
Signed-off-by: Daniel Farrell <[email protected]>
dfarrell07 added a commit to dfarrell07/cloud-prepare that referenced this issue Aug 30, 2022
This is a different type of static analysis than others we run.

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/submariner#1970
Signed-off-by: Daniel Farrell <[email protected]>
dfarrell07 added a commit to dfarrell07/coastguard that referenced this issue Aug 30, 2022
This is a different type of static analysis than others we run.

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/submariner#1970
Signed-off-by: Daniel Farrell <[email protected]>
dfarrell07 added a commit to dfarrell07/subctl that referenced this issue Aug 30, 2022
This is a different type of static analysis than others we run.

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/submariner#1970
Signed-off-by: Daniel Farrell <[email protected]>
dfarrell07 added a commit to dfarrell07/submariner that referenced this issue Aug 30, 2022
This is a different type of static analysis than others we run.

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]>
dfarrell07 added a commit to dfarrell07/subctl that referenced this issue Sep 1, 2022
This is a different type of static analysis than others we run.

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/submariner#1970
Signed-off-by: Daniel Farrell <[email protected]>
dfarrell07 added a commit to dfarrell07/subctl that referenced this issue Sep 1, 2022
This is a different type of static analysis than others we run.

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/submariner#1970
Signed-off-by: Daniel Farrell <[email protected]>
dfarrell07 added a commit to dfarrell07/submariner that referenced this issue Sep 1, 2022
This is a different type of static analysis than others we run.

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]>
@dfarrell07
Copy link
Member Author

I have the PRs for this on hold, in draft, while I'm trying to sort out some weirdness that was exposed by sending them around to the various repos.

In short, I haven't been able get the CodeQL builds to fail on code that contains errors (dfarrell07/submariner-operator#116, dfarrell07#53, dfarrell07#54) and it always fails on subctl even if it doesn't contain errors (dfarrell07/subctl#11, dfarrell07/subctl#14, dfarrell07/subctl#15).

dfarrell07 added a commit to dfarrell07/submariner-operator that referenced this issue Sep 22, 2022
This job was called vulnerability variant analysis during most of
development. It should have been reordered when the name was shortened.

Relates-to: submariner-io/submariner#1970

Signed-off-by: Daniel Farrell <[email protected]>
dfarrell07 added a commit to dfarrell07/submariner-operator that referenced this issue Sep 26, 2022
This job was called vulnerability variant analysis during most of
development. It should have been reordered when the name was shortened.

Relates-to: submariner-io/submariner#1970
Signed-off-by: Daniel Farrell <[email protected]>
dfarrell07 added a commit to dfarrell07/submariner-operator that referenced this issue Sep 26, 2022
This job was called vulnerability variant analysis during most of
development. It should have been reordered when the name was shortened.

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

Relates-to: submariner-io/submariner#1970
Signed-off-by: Daniel Farrell <[email protected]>
skitt pushed a commit to submariner-io/submariner-operator that referenced this issue Sep 27, 2022
This job was called vulnerability variant analysis during most of
development. It should have been reordered when the name was shortened.

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

Relates-to: submariner-io/submariner#1970
Signed-off-by: Daniel Farrell <[email protected]>
dfarrell07 added a commit to dfarrell07/submariner that referenced this issue Sep 29, 2022
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/

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]>
dfarrell07 added a commit to dfarrell07/submariner that referenced this issue Sep 29, 2022
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]>
@dfarrell07
Copy link
Member Author

Still trying to figure out why CodeQL doesn't flag the same issues flagged by LGTM.com using CodeQL.

One last test with all the tweaks learned from the Operator iterations, just to be super clear about the lack of negative results:

In dfarrell07@260afb3, I revert the commit that is identified by LGTM.com as having fixed an error flagged by CodeQL.

FIXED The first argument to 'errors.Wrap' is always nil

The CodeQL job doesn't flag the reverted code as an issue:

https://github.com/dfarrell07/submariner/actions/runs/3153159751/jobs/5129296659

I guess it's possible that the issue is caused by running on a fork, per this warning in the logs:

Warning: This run of the CodeQL Action does not have permission to access Code Scanning API endpoints. As a result, it will not be opted into any experimental features. This could be because the Action is running on a pull request from a fork. If not, please ensure the Action has the 'security-events: write' permission. Details: HttpError: Resource not accessible by integration

@dfarrell07
Copy link
Member Author

The really fun part is whatever's going on with subctl. Always fails with:

  [2022-09-29 20:31:24] [build-stderr] 2022/09/29 20:31:24 Trying build command make []
  [2022-09-29 20:31:24] [build-stdout] Downloading Makefile.dapper
  [2022-09-29 20:31:24] [build-stdout] make: 'Makefile.dapper' is up to date.
  [2022-09-29 20:31:26] [build-stderr] 2022/09/29 20:31:26 Dependencies are still not resolving after the build, continuing to install dependencies.
  [2022-09-29 20:31:26] [build-stderr] 2022/09/29 20:31:26 Installing dependencies using `go get -v ./...`.
  [2022-09-29 20:31:39] [build-stderr] package github.com/submariner-io/subctl/cmd
  [2022-09-29 20:31:39] [build-stderr] 	imports github.com/submariner-io/subctl/cmd/subctl
  [2022-09-29 20:31:39] [build-stderr] 	imports github.com/submariner-io/subctl/pkg/cloud/azure
  [2022-09-29 20:31:39] [build-stderr] 	imports github.com/submariner-io/cloud-prepare/pkg/azure
  [2022-09-29 20:31:39] [build-stderr] 	imports github.com/Azure/azure-sdk-for-go/sdk/azcore: build constraints exclude all Go files in /home/runner/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/[email protected]
  [2022-09-29 20:31:39] [build-stderr] package github.com/submariner-io/subctl/cmd
  [2022-09-29 20:31:39] [build-stderr] 	imports github.com/submariner-io/subctl/cmd/subctl
  [2022-09-29 20:31:39] [build-stderr] 	imports github.com/submariner-io/subctl/pkg/cloud/azure
  [2022-09-29 20:31:39] [build-stderr] 	imports github.com/Azure/azure-sdk-for-go/sdk/azidentity: build constraints exclude all Go files in /home/runner/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/[email protected]
  [2022-09-29 20:31:39] [build-stderr] 2022/09/29 20:31:39 Running /usr/bin/go failed, continuing anyway: exit status 1
  [2022-09-29 20:31:39] [build-stderr] 2022/09/29 20:31:39 Running extractor command '/opt/hostedtoolcache/CodeQL/0.0.0-20220908/x64/codeql/go/tools/linux64/go-extractor [./...]' from directory '/home/runner/work/subctl/subctl'.
  [2022-09-29 20:31:41] [build-stderr] 2022/09/29 20:31:41 Build flags: ''; patterns: './...'
  [2022-09-29 20:31:41] [build-stderr] 2022/09/29 20:31:41 Running packages.Load.
  [2022-09-29 20:31:50] [build-stderr] 2022/09/29 20:31:50 Error running go tooling: err: exit status 1: stderr: go build github.com/Azure/azure-sdk-for-go/sdk/azcore: build constraints exclude all Go files in /home/runner/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/[email protected]
  [2022-09-29 20:31:50] [build-stderr] go build github.com/Azure/azure-sdk-for-go/sdk/azidentity: build constraints exclude all Go files in /home/runner/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/[email protected]
  [2022-09-29 20:31:50] [build-stderr] 2022/09/29 20:31:50 Extraction failed: exit status 1
  Error: 9-29 20:31:50] [ERROR] Spawned process exited abnormally (code 1; tried to run: [/opt/hostedtoolcache/CodeQL/0.0.0-20220908/x64/codeql/tools/linux64/preload_tracer, /opt/hostedtoolcache/CodeQL/0.0.0-20220908/x64/codeql/go/tools/autobuild.sh])
  A fatal error occurred: Exit status 1 from command: [/opt/hostedtoolcache/CodeQL/0.0.0-20220908/x64/codeql/go/tools/autobuild.sh]
  Error: The process '/opt/hostedtoolcache/CodeQL/0.0.0-20220908/x64/codeql/codeql' failed with exit code 2

submariner-io/subctl#244

@github-actions github-actions bot removed the dependent label Nov 9, 2022
dfarrell07 added a commit to dfarrell07/submariner that referenced this issue Feb 13, 2023
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]>
@dfarrell07 dfarrell07 moved this to In Review in Submariner 0.16 May 9, 2023
tpantelis pushed a commit that referenced this issue Sep 12, 2023
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]>
@stale
Copy link

stale bot commented Sep 17, 2023

This issue has been automatically marked as stale because it has not had activity for 60 days. It will be closed if no further activity occurs. Please make a comment if this issue/pr is still valid. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Sep 17, 2023
@github-project-automation github-project-automation bot moved this from In Review to Done in Submariner 0.16 Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation wontfix This will not be worked on
Projects
No open projects
Status: Done
Development

No branches or pull requests

3 participants