Add way to customize environmentCheck Actions QL#22173
Conversation
michaelnebel
left a comment
There was a problem hiding this comment.
Thank you very much for doing this! It is highly appreciated!
Would it be possible to add some tests as well?
Note, that for testing the Yaml alternative it is possible to add an .ext.yml file next to the .ql test file - it just needs to have the same base name as the test file. That is, if there is a test named test.ql then adding test.ext.yml makes it possible to add tuples to the extensible predicates that only applies for test.ql.
| EnvironmentCheck() { | ||
| exists(string selected | | ||
| selectDeploymentEnvironmentDataModel(selected) and | ||
| if selected = "EnvironmentCheckMaD" |
There was a problem hiding this comment.
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
enabledDeploymentEnvironmentDataModelOR - By extending the abstract class
CustomEnvEnable.
On top of that there is an extensible predicateselectDeploymentEnvironmentDataModelused 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?
There was a problem hiding this comment.
the idea was in case there are more complicated cases for the scenarios where environment checks are only relevant in certain scenarios
but those cases are abstract currently so for simplicity in the modelling I will go with just MaD for now. we can keep in mind if a concrete case arises that custom QL is at least possible
thanks!
@michaelnebel thank you so much for the review! addressed most, except the testing request, which I agree with but I am having a bit of trouble envisioning how to test this other than add a new test that just checks the enabled control checks... I dont see such a lib test currently. the reason it is a bit tough is bc I think we want the default behaviour to still be "add all envs as sanitizers" and the specific behaviour to be "enable just specific ones" which means all the security queries would still have the same result regardless of whether the mechanism is active or not... toggling it on and off wont affect the test result and therefore in my mind its a bit of a bad unit test ideology any better ideas very welcome! |
Add implementation for technique to customize
EnvironmentCheckin actions queriesCurrently there is no way to customize Actions query "sanitizers" ie
ControlChecks. This PR adds a sample mechanism that uses 1) a specifier in MaD that designates which technique to use (the default is no customization) and 2) a mechanism that fills the definition of EnvironmentCheck with either i) a QL custom definition or ii) a MaD custom definition.The purpose of this is to allow for checks to be selectively turned off or narrowed based on real info related to deployment environments in repos (this would need to be externally supplied) .
This PR is similar to and borrows ideas from this PR.
The intention of this work would be to potentially apply this style to any check type or other sanitizer types in other languages, if the mechanisms used here are found to be suitable.
One important consideration of this work is that for Actions there is currently no Customizations.qll file. Either this PR should also add one , or it should be carefully considered if the current
actions.qllusage for a similar intended edit location is alright to do (probably the former would be more ideal).