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

Convert trusted actions list to data extension #18435

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

felickz
Copy link
Contributor

@felickz felickz commented Jan 7, 2025

The actions/unpinned-tag query can be noisy under certain circumstances (ex: many internally developed actions) where unpinned actions is an acceptable risk. Today, there is no capability to modify the trusted owners (currently hardcoded to github, actions, advanced-security).

This change will make the query extensible so that a extension pack (which supports default setup at scale) could define additional trusted Actions owners. Ex:

extensions:
  - addsTo: 
      pack: codeql/actions-all
      extensible: trustedActionsOwnerDataModel 
    data:
      - ["octodemo"]

Which would expand the default set:

extensions:
- addsTo:
pack: codeql/actions-all
extensible: trustedActionsOwnerDataModel
data:
- ["actions"]
- ["github"]
- ["advanced-security"]

Pull Request checklist

All query authors

Internal query authors only

  • Autofixes generated based on these changes are valid, only needed if this PR makes significant changes to .ql, .qll, or .qhelp files. See the documentation (internal access required).
  • Changes are validated at scale (internal access required).
  • Adding a new query? Consider also adding the query to autofix.

@github-actions github-actions bot added the Actions Analysis of GitHub Actions label Jan 7, 2025
@felickz felickz marked this pull request as ready for review January 7, 2025 20:57
@Copilot Copilot bot review requested due to automatic review settings January 7, 2025 20:57
@felickz felickz requested a review from a team as a code owner January 7, 2025 20:57

Choose a reason for hiding this comment

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

Copilot reviewed 3 out of 5 changed files in this pull request and generated no comments.

Files not reviewed (2)
  • actions/ql/lib/codeql/actions/security/UseOfUnversionedImmutableAction.qll: Language not supported
  • actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql: Language not supported
Comments suppressed due to low confidence (1)

actions/ql/lib/codeql/actions/security/owner/trusted/trusted_actions_owner.model.yml:2

  • There is an extra space after the colon. It should be consistent with the rest of the file.
  - addsTo: 

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed repo to nwo to be precise in the naming used here

Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

Thanks for making this change. Could you also update the associated qhelp to describe to users how that can add their own data extension to customize this query?

- use existing data extensions config and yml folder
- rename from trustedActionsOwner to trustedActionsOwnerDataModel
- update related predicates
@felickz
Copy link
Contributor Author

felickz commented Jan 8, 2025

Thanks for making this change. Could you also update the associated qhelp to describe to users how that can add their own data extension to customize this query?

I actually had this same thought when creating that extension. Would make the feature easily discoverable! I took a stab at it:

### Configuration
If there is an Action publisher that you trust, you can include the owner name/organization in a data extension model pack to add it to the allow list for this query. Adding owners to this list will prevent security alerts when using unpinned tags for Actions published by that owner.
#### Example
To allow any Action from the publisher `octodemo`, such as `octodemo/3rd-party-action`, follow these steps:
1. Create a data extension file `/models/trusted-owner.model.yml` with the following content:
```yaml
extensions:
- addsTo:
pack: codeql/actions-all
extensible: trustedActionsOwnerDataModel
data:
- ["octodemo"]
```
2. Create a model pack file `/codeql-pack.yml` with the following content:
```yaml
name: my-org/actions-extensions-model-pack
version: 0.0.0
library: true
extensionTargets:
codeql/actions-all: '*'
dataExtensions:
- models/**/*.yml
```
3. Ensure that the model pack is included in your CodeQL analysis.
By following these steps, you will add `octodemo` to the list of trusted Action publishers, and the query will no longer generate security alerts for unpinned tags from this publisher.

- models/**/*.yml
```

3. Ensure that the model pack is included in your CodeQL analysis.
Copy link
Contributor

Choose a reason for hiding this comment

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

This step is not possible for default setup. I believe it's possible to just place the extensions file in .github/codeql/extensions in the current repository and have it automatically included in the run.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is possible to setup model packs with org level setup: Extending CodeQL coverage with CodeQL model packs in default setup ... added to references link at the bottom of the .md

I struggle with how verbose to make this guidance in the query markdown, feels like it would be better documented out in the (yet to be released) https://codeql.github.com/docs/codeql-language-guides/codeql-for-actions/

I will take a stab at adding some of the additional options here as a hint in the right direction

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm...that might be a better use of space to link out to the language guide (when it is available). I am a little concerned that if we load up the qhelp with lots of information around configuration, then autofix will get confused.

Presumably, the language docs are released along with the CLI release. So, timing would work out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point about autofix, I will shift gears and move these docs in that direction and consider deep linking via reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Actions Analysis of GitHub Actions documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants