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

Disable CodeQL from pipeline level and add it in job level #2698

Merged
merged 5 commits into from
Jan 28, 2025

Conversation

maglims
Copy link
Contributor

@maglims maglims commented Jan 24, 2025

For more information about how to contribute to this repo, visit this page.

Description

What is codeql, and what the persistent issues it has currently?
From what I understand in a briefly way, it is static analysis tool, runs on source codes for all kind of languages, and also traces the build process during to build, to capture any security vulnerability issues. It creates a DB during its run, which I believe records the scan result, and the DB will be uploaded in the pipeline to be source of different reporting/dashboards.
CodeQL currently has some issues arm64 macos image, which has been known for a while and in the back of codeQL team. This is not super clear what is the issues to me, and there is an existing ICM on codeql

When it runs in 1ES template?
CodeQL is injected task if the pipeline is on 1ES template. From our observations, 1ES always injects the run at the beginning of each task, i.e, before it runs any other tasks like tool installing. When the tool is installing, it may be interferred with codeql, causing the failure (Not exactly know the reason)

How manual run is different from codeql auto-injection task?
There is no difference between auto-injected codeQL and manual codeQL task themselves. The only difference is with manual, we/our team can decide when to inject. The workaround is to inject after the tool setup has completed, to let codeql have less interruptions with the build process. A recommended order from CodeQL team:
image

What is our solution here?
Disable from pipeline level, and then add manual task at each job level. Please note, Codeql doesn't support job level disable as of today, so in order to disable, we have to do it at pipeline level.

Will we receive the S360 alerts?
Given we are disabling the CodeQL at pipeline/repo level. Will we receive S360 alerts?
The answer should be No. We have confirmed with CodeQL team. As long as DB is uploaded, S360 will have some results to process, meeting the requirement. It doesn’t track where the DB is created (from 1ES template, or manual run doesn't matter)

Main changes in the PR:

  1. Disable CodeQL from the pipeline
  2. Manually add the task at job level

Validation

Validation performed:

  1. <Step 1>
  2. <Step 2>

Unit Tests added:

Unit tests are required for all changes. If no unit tests were added as part of this change, please explain why they aren't necessary.

<Yes/No>

End-to-end tests added:

<Yes/No>

Additional Requirements

Change file added:

Ensure the change file meets the formatting requirements.

<Yes/No>

Related PRs:

Remove this section if n/a

Next/remaining steps:

List the next or remaining steps in implementing the overall feature in subsequent PRs (or is the feature 100% complete after this?).

Remove this section if n/a

  • Item 1
  • Item 2

Screenshots:

Remove this section if n/a

Before After
< image1 > < image2 >

@maglims maglims requested a review from a team as a code owner January 24, 2025 22:39
@maglims maglims changed the title [Draft] Don't review Disable CodeQL from pipeline level and add it in job level Jan 27, 2025
@@ -87,6 +87,13 @@ steps:
script: 'nohup pnpm start-test-app --server-options-cert $(localhostSSLCertificate.secureFilePath) --server-options-key $(localhostSSLKey.secureFilePath) &'
workingDirectory: '$(ClientSdkProjectDirectory)'

- task: CodeQL3000Init@0
inputs:
${{ if eq(variables['Build.SourceBranch'], 'refs/heads/main') }}:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to codeQL, it should only run default branch (master or main)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mind setting it explicitly but I think codeQL by default checks branch. That is to say, in your branch, codeQL would not be triggered (because it isn't running on main) maybe and this inputs setting can be saved?

Also I cannot think of other way we can do something except checking this in and then monitor main branch to see if it fails or not. Hopefully it is not.

Copy link
Contributor

Choose a reason for hiding this comment

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

CodeQL will by default check for changes to main. Using the inputs tag here is to add additional branches. I would recommend modifying this to look for both main and release/* branches instead of just main.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I think you guys have a point and I also observed the skipping of run on non-default branch. Let me remove it since it doesn't seem doing anything better.

@noahdarveau-MSFT
Copy link
Contributor

To clarify my understanding, we will still be running codeQL, just at a later point in the pipeline? Will this have any impacts on its scanning capabilities?

codeql:
compiled:
enabled: false
justificationForDisabling: 'CodeQL has some know issues with arm64 macos. Disabling auto-injection and using manual task'
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: known

@maglims
Copy link
Contributor Author

maglims commented Jan 28, 2025

To clarify my understanding, we will still be running codeQL, just at a later point in the pipeline? Will this have any impacts on its scanning capabilities?

@noahdarveau-MSFT , this is a really good question, and also the core of changes. I have added some details in the PR description, let me see if I can add more details.

  1. Yes, we are still running CodeQL. As you can see, "CodeQL initialize" is the 1ES template injected task, which has been skipped now (Unfortunatly we cannot only skip this only for iOS. That is also why you see that we are adding it back to each job/repo that needs CodeQL scan)
    image.

Down there there is "CodeQL3000Init@0" task, which is the manual task we are adding now, to ensure the coverage.

  1. What is the difference between "CodeQL Initialize" and "CodeQL3000Init@0"?
    The tasks themselves are no difference on what they do, i.e, they do the same scan/job. With "CodeQL3000Init@0" task, we can decide where to put it or when we want to run the job. So as you can see, right now, we are putting it after tooling setup (specially you can see that it is running after "Setup pnpm"). That is also the reason why manual task should be able to fix the issue, because 'setup pnpm' already finishes and there is no way codeQL can mess around with it.

  2. Do we lose any scanning capability by running it in later stage?
    We are adding the tasks per CodeQL's suggestions. So there should be no concern from their point of view. Here is what CodeQL suggests how to run codeQL manually, i.e, it just needs to cover compile/build phase.

image

This ensures the scan coverage, without interferring other stages.

As a comparsion of how auto-injected task works, 1ES template always injects at very beginning of the job, which we don't have control. So you can image the order is something like below. So any tasks/tool setting up between "CodeQL init" and "CodeQL Finalize", can be interfered by CodeQL in an unexpected way. This is also the reason why we see intermittent failure.
Job:
steps:
- CodeQL Init

  • install dotnet
  • install python
  • run script
    - setup pnpm
  • other non-compile tasks
  • compile task
  • other non-compile tasks
    - - CodeQL Finalize

@maglims maglims merged commit 17d7889 into main Jan 28, 2025
37 checks passed
@maglims maglims deleted the magli/AddressCodeQLIssue branch January 28, 2025 22:51
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.

4 participants