Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions actions/ql/lib/actions.qll
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
import codeql.actions.Ast

abstract class CustomEnvEnable extends Environment { }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing QL doc.

16 changes: 16 additions & 0 deletions actions/ql/lib/codeql/actions/config/Config.qll
Original file line number Diff line number Diff line change
Expand Up @@ -164,3 +164,19 @@ predicate untrustedGhCommandDataModel(string cmd_regex, string flag) {
predicate actionsPermissionsDataModel(string action, string permission) {
Extensions::actionsPermissionsDataModel(action, permission)
}

/**
* MaD models for deployment environments
* Fields:
* - name: deployment environment name, e.g. `Public CI`
*/
predicate enabledDeploymentEnvironmentDataModel(string name) {
Extensions::enabledDeploymentEnvironmentDataModel(name)
}

/**
* `EnvironmentCheck` implementation model.
*/
predicate selectDeploymentEnvironmentDataModel(string selected) {
Extensions::selectDeploymentEnvironmentDataModel(selected)
}
12 changes: 12 additions & 0 deletions actions/ql/lib/codeql/actions/config/ConfigExtensions.qll
Original file line number Diff line number Diff line change
Expand Up @@ -88,3 +88,15 @@ extensible predicate untrustedGhCommandDataModel(string cmd_regex, string flag);
* - see https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/controlling-permissions-for-github_token for documentation of token permissions.
*/
extensible predicate actionsPermissionsDataModel(string action, string permission);

/**
* Holds for deployment environments that exist for a given repository.
* Requires this to be externally supplied but once done can be used to
* toggle precision of whether that suffices or not as a control check.
*/
extensible predicate enabledDeploymentEnvironmentDataModel(string name);

/**
* Selects which deployment environments model to use to implement `EnvironmentCheck`.
*/
extensible predicate selectDeploymentEnvironmentDataModel(string mechanism);
12 changes: 12 additions & 0 deletions actions/ql/lib/codeql/actions/security/ControlChecks.qll
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,18 @@ abstract class LabelCheck extends ControlCheck {
}

class EnvironmentCheck extends ControlCheck instanceof Environment {
EnvironmentCheck() {
exists(string selected |
selectDeploymentEnvironmentDataModel(selected) and
if selected = "EnvironmentCheckMaD"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for looking into this!
I am not as such familiar with the actions QL implementation, so bear with me 😄

As far as I understand the idea of the pull request is to make it configurable which Environment instances that are added as ControlChecks. The PR implements support for this in two ways

  • Either by supplying environment names as tuples in enabledDeploymentEnvironmentDataModel OR
  • By extending the abstract class CustomEnvEnable.
    On top of that there is an extensible predicate selectDeploymentEnvironmentDataModel used to decide, which of the above strategies to use (if any).

A couple of questions:

  • Would it suffice to only have the MaD variant? The Yaml files are loaded dynamically and only supporting a single way of doing it will make it easier to maintain.
  • What is the reason for also having the abstract class way of doing it?

then enabledDeploymentEnvironmentDataModel(this.(Environment).getName())
else
if selected = "EnvironmentCheckCustomQL"
then this instanceof CustomEnvEnable
else this instanceof Environment
)
}

// Environment checks are not effective against any mutable attacks
// they do actually protect against untrusted code execution (sha)
override predicate protectsCategoryAndEvent(string category, string event) {
Expand Down
9 changes: 9 additions & 0 deletions actions/ql/lib/ext/config/customize_checks.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
extensions:
- addsTo:
pack: codeql/actions-all
extensible: selectDeploymentEnvironmentDataModel
data:
# The possible values this can take are:
# EnvironmentCheckCustomQL - for a custom QL implementation for the sufficient EnvironmentCheck.
# EnvironmentCheckMaD - for a custom MaD list of deployment environments that are relevant to a database.
- [""]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With the current logic in EnvironmentCheck, the extensible predicate selectDeploymentEnvironmentDataModel needs to contain at least one "dummy" tuple - in this case "" - otherwise no Environment instances will be added to ControlCheck.
Perhaps, consider letting keeping the extensible predicate selectDeploymentEnvironmentDataModel empty as default (the logic in EnvironmentCheck needs to be adjusted accordingly).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comment is only relevant in case we need to keep both the Yaml and abstract class option.
Otherwise, we can consider assuming that if enabledDeploymentEnvironmentDataModel contains any tuples, then it is exactly those environments that we would like to add to ControlCheck (this removes the need for selectDeploymentEnvironmentDataModel)

6 changes: 6 additions & 0 deletions actions/ql/lib/ext/config/deployment_environment.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
extensions:
- addsTo:
pack: codeql/actions-all
extensible: enabledDeploymentEnvironmentDataModel
data:
- [""]
Comment on lines +5 to +6

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
data:
- [""]
data: []

Loading