etcd: CodeQL Action fails blocking code review/merging

Appreciate your patience and willingness to try the workaround. I realise the workaround I suggested may not be enough to handle all situations, in particular workflows running on pull requests from forked repositories, which received a more limited set of permissions by default, even if you specify write in the permissions block. We have now rolled back the change in the CodeQL Action while we figure out a way to check on results without breaking such workflows. So you can continue to use v1 of the Action in your workflow, and should no longer see the error.

To learn more about the options for Actions token permissions, you may be in interested in https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token and https://docs.github.com/en/actions/security-guides/automatic-token-authentication#modifying-the-permissions-for-the-github_token. My understanding is that by keeping the Send write tokens to workflows from pull requests setting disabled in your repo settings, pull requests from forks will get read permission at most for any of the configured scopes (even if the permissions block sets it to write). The first column of the table in the docs describes the permissions for a workflow with no explicit permissions block: most scopes are set to read/write by default, so adding a permissions block to the workflow restricts the scopes further than the default to exactly the scopes you need.

There is also problem of false positives that we are unable to silence. CodeQL reports problems Log entries created from user input on data passed to zap which escapes the data when writing JSON.

Thanks for pointing these out too. Dismissing the alerts in the code scanning UI will ensure they don’t reappear. However the CodeQL team is interested in hearing about false positives and library coverage that affect our security-focused analysis. In this case it sounds like we could handle zap APIs as sanitisers for logging-related vulnerabilities. Could you please create an issue in https://github.com/github/codeql or https://github.com/github/codeql-go, so that the team that works on Go analysis can consider it?

Going forward, please don’t hesitate to file questions or issues in https://github.com/github/codeql and we will be very happy to help you out.

I see the warnings refreshed now in the security console. I will review them and dismiss when justified.

This solves one of the problems with CodeQL. @adityasharad please refrain from introducing a breaking changes without proper notification.

There is also problem of false positives that we are unable to silence. CodeQL reports problems Log entries created from user input on data passed to zap which escapes the data when writing JSON.

@adityasharad Do you have any recommendation on silencing those reports?

Greetings. I’m on the team at GitHub that maintains the CodeQL Action, and was directed here by our developer relations team, to whom I believe maintainers of this project reached out. Thanks for letting us know, and apologies for the inconvenience.

The timeline you describe (9 days ago) coincides with our update of v1 to v1.0.27, which includes a change to the default behaviour when uploading results, specifically checking on the status of the results while they’re being processed. The error (Resource not accessible by integration in the Waiting for processing to finish step) suggests that the default GITHUB_TOKEN being used in your Actions workflow doesn’t have permission to perform that check.

Could you please try one of the following and let me know if they resolve the problem with the failing workflow?

• Add an explicit permissions block to your Actions workflow updating the default permissions for the GITHUB_TOKEN. We recommend the following here:

  permissions:
    # required for all workflows
    security-events: write
    # only required for workflows in private repositories
    actions: read
    contents: read
  

  OR
• Turn off the default behaviour of waiting for results processing:

  - name: Perform CodeQL Analysis
    uses: github/codeql-action/analyze@v1
    # workaround: add the two lines below
    with:
      wait-for-processing: false
  

From https://codeql.github.com/codeql-query-help/csharp/cs-log-forging/#recommendation (note that this csharp documentation as there is no article for Go with recommendations on how to handle it) recommendation is to escape user input. Method of escaping depends on output we are generating, for example if we generated HTML document it should use HTML encoder.

In Etcd case user input is passed to zap logging library. In default configuration zap will write a JSON and properly escape values, so there is not immediate security threat. One case would be with new v3.6 feature where zap can be switched from JSON to traditional console logs. In that case characters like \n and \t might not be escaped, however I don’t see any impact here too.