fix(query): Extend net_raw_capabilities_not_being_dropped k8s rule to cover further resource kinds

[Churro]

Proposed Changes

• Extend the rule to cover additional resource kinds, e.g., Deployment, DaemonSet, etc.
• Fold the check whether securityContext.capabilities.drop is defined into one rule
  • Even if securityContext or securityContext.capabilities are undefined, it is now immediately highlighted that securityContext.capabilities.drop is missing

I submit this contribution under the Apache-2.0 license.

[kicsbot]

Scan submitted to Checkmarx

[rafaela-soares]

Hi, @Churro!

First of all, sorry for the delay in reviewing some of your PRs. And thank you so much for your contributions to KICS!

Your approach (check whether securityContext.capabilities.drop is defined into one rule) is very impressive! 🚀

However, due to how the KICS searchKey is defined, the searchKey points wrongly with your approach. For example, take a look at the positive1.yaml.

In the master, there is a result in line 13. In this PR, the result points to line 18. That happens because KICS interprets "." in the searchKey as a "breaking line" and it will search for the key(s) in the file.

In this case, although there is no securityContext defined in the container named as payment2 (line 13) the searchKey is pointing to line 18 because the searchKey tells KICS to search for securityContext.capabilities.

As a solution for that, we built the function get_nested_values_info that receives two inputs: object and array_vals. This function verifies if the array_vals are nested in the object. It will return an object with two fields: valid (returns if the array_vals are nested in the object (array_vals should be sorted) and searchKey returns the searchKey possible.

For example:

object := {"elem1": {"elem2": "elem3"}}
array_vals := ["elem2", "elem3", "elem4"]
return_value := {"valid": false, "searchKey": "elem2.elem3"}

Can you apply the function get_nested_values_info, please? 😊 You can find more examples of the application here. (Please merge the branch with the master)

P.S. The other PRs with the same approach should also apply the function get_nested_values_info

[Churro]

Hi @rafaela-soares, I have now applied your suggestions and merged with master.

From an engineering perspective, however, I'd like to question the introduction of the get_nested_values_info function. It basically adds significant complexity to queries that is unrelated to the purpose of the rules themselves and only needed for kics output. Are you sure this is the best way tackle the searchKey problem?

My suggestion would be to refine the searchKey implementation to deliver the expectable output, rather than trying to workaround a deficiency in the matching strategy with adaptations to countless rules. Probably, it helps if I illustrate what I mean with two concrete examples:

1. searchKey matching
   For the sample snippet you posted, the searchKey output already included the container name, i.e.:

"searchKey": "metadata.name={{example}}.spec.containers.name={{payment2}}.securityContext.capabilities"
...
"searchKey": "metadata.name={{example}}.spec.containers.name={{payment3}}.securityContext.capabilities
...
"searchKey": "metadata.name={{example}}.spec.containers.name={{payment4}}.securityContext.capabilities"
...
"searchKey": "metadata.name={{example}}.spec.containers.name={{payment}}.securityContext.capabilities"

I would expect the searchKey implementation to understand that a search key which includes containers.name={{payment2}} cannot be located in container payment4. Instead, I would assume that searchKey always points to that line in the YAML with the greatest common denominator, i.e., if there is a container named payment2 but no securityContext.capabilities, it should still point to the line with name: payment2.

What do you think about it? I may be wrong but is it probably related to the fact that matching is based mainly on the Levensthein distance (helper.go), so as long as there is a longer "approximate" match in the search hierarchy, it is picked?

2. Runtime complexity
   By replacing object.get(...) with get_nested_values_info you switch from a runtime complexity of O(1) to O(n). This is because walk() traverses all branches of the document and doesn't stop when it finds array_vals.

The effect becomes visible if we take the sample snippet of your screenshot and profile it with my original query (eval time: 0,371931ms) and the one you suggested with get_nested_values_info (eval time: 1,191899ms).

$ opa eval -d query_me.rego -d ../../../libraries/common.rego -d ../../../libraries/k8s.rego -i sample.json \
--profile --format=pretty 'data.Cx'

+----------+----------+----------+---------------+
|   TIME   | NUM EVAL | NUM REDO |   LOCATION    |
+----------+----------+----------+---------------+
| 36.658µs | 20       | 20       | query.rego:43 |
| 33.66µs  | 20       | 20       | query.rego:40 |
| 32.229µs | 20       | 20       | query.rego:42 |
| 27.17µs  | 1        | 1        | query.rego:28 |
| 21.29µs  | 1        | 1        | data.Cx       |
| 20.599µs | 8        | 0        | query.rego:36 |
| 15.84µs  | 8        | 8        | query.rego:34 |
| 12.19µs  | 4        | 4        | query.rego:38 |
| 11.94µs  | 8        | 8        | query.rego:35 |
| 11.32µs  | 3        | 6        | query.rego:13 |
+----------+----------+----------+---------------+

$ opa eval -d query_nested.rego -d ../../../libraries/common.rego -d ../../../libraries/k8s.rego -i sample.json \
--profile --format=pretty 'data.Cx'

+-----------+----------+----------+------------------------------------+
|   TIME    | NUM EVAL | NUM REDO |              LOCATION              |
+-----------+----------+----------+------------------------------------+
| 442.537µs | 264      | 203      | ../../../libraries/common.rego:730 |
| 129.307µs | 78       | 132      | ../../../libraries/common.rego:729 |
| 109.327µs | 20       | 20       | query_nested.rego:42               |
| 64.598µs  | 28       | 28       | query_nested.rego:39               |
| 34.43µs   | 20       | 20       | query_nested.rego:41               |
| 32.68µs   | 17       | 18       | ../../../libraries/common.rego:29  |
| 29.488µs  | 8        | 8        | query_nested.rego:34               |
| 22.989µs  | 1        | 1        | data.Cx                            |
| 22.569µs  | 4        | 4        | ../../../libraries/common.rego:737 |
| 20.97µs   | 7        | 6        | ../../../libraries/common.rego:714 |
+-----------+----------+----------+------------------------------------+

This may be seem negligible with this tiny sample snippet but with large manifests and when run in combination with all other queries, it will make a difference. // Update: Ok, to be honest, not really 😄 - execution time for this rule raised from 230µs to 18ms for a manifest of 100kb. That's not noticeable in practice but is also not "nothing"...

[joaoReigota1]

Hi, @Churro thank you for the observation, you are correct.
Although the change is small in these queries it might be bigger in more complex ones.
To fix the limitations that the "searchKey" has, we added another property called searchLine
The searchLine makes use of the JSON path and should fix this issue.
Please refer to this query as an example of its usage: https://github.com/Checkmarx/kics/blob/master/assets/queries/k8s/psp_with_unrestricted_access_to_host_path/query.rego#L54

We want to add your pull requests to the next version that is releasing this Wednesday, as they are a major help in improving our Kubernetes queries, so please feel free to choose whether you want to keep the function "get_nested_values_info" or try to make use of the searchLine.
If you have any questions about the searchLine or any other topic related to KICS feel free to ask, we are happy to help 😊

Again, thank you for the help in improving our Kubernetes queries

[kicsbot]

Checkmarx SAST - Scan Summary & Details

Cx-SAST Summary

Total of 5 vulnerabilities
 0 High
 0 Medium
 5 Low
 0 Info

Violation Summary

No policy violation found

[Churro]

Hi @joaoReigota1, thank you for the hint on searchLine. I wasn't aware of this property 👍
I've now revised the rule and added this. As a result, the visual search lines correspond to the reported ones.

Please let me know in case something is still missing to get this ready for v1.5.4.

[joaoReigota1]

LGTM!

.