new(nok8s): json schema validation for tracingpolicies - #5322
Conversation
42edb96 to
964380b
Compare
So, this is failing because in the
For now, i implement 1, but am open to switch to 2 if requested. EDIT: in the end, i chose to follow what we already do for crds: |
1554289 to
24206d3
Compare
✅ Deploy Preview for tetragon ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
24206d3 to
0d2788f
Compare
|
Example output from feeding a wrong syntax policy: In main, same policy: Policy file (note apiVersion: cilium.io/v1alpha1
kind: TracingPolicy
metadata:
name: "protection-policy-cve-2024-21626"
spec:
lsmhoooks:
- hook: "bprm_check_security"
args:
- index: 0 # struct linux_binprm *bprm
type: "path"
resolve: "mm.owner.fs.pwd"
selectors:
- matchArgs:
- index: 0
operator: "Prefix"
values:
- "/sys/fs/cgroup"
matchActions:
- action: Post
message: cve-2024-21626 |
0d2788f to
76dae24
Compare
There was a problem hiding this comment.
I confirmed that this came from the vendor's repo, and it's not something that we created by accident. (I guess our CI would catch it, if it came from us).
|
It might be worth doing an explicit validation for drift between the JSON schema and the CRDs in CI. |
This should never happen since |
76dae24 to
0834ad5
Compare
kkourt
left a comment
There was a problem hiding this comment.
Note: x-kubernetes-validations cel expressions are not supported; we would need to extend the json schema validation and manually support them; it's not super easy to do, but it would allow us to cleanup some code.
Is this something that we plan to do?
Because if we can't do it, then I think the best option would be to do just duplicate the validation logic in go since we are using CEL expressions extensively in our spec.
I think we might want to commit to do that in the future; unfortunately, it is a bit complex and would take quite a bit of time.
I am not sure, it quickly becomes risky to rely upon the double validation (sooner or later we would forget to add it) and also why pay the perf penalty (for validation) in k8s builds when k8s already validates for us? |
| github.com/prometheus/client_model v0.6.2 | ||
| github.com/prometheus/common v0.70.0 | ||
| github.com/prometheus/procfs v0.21.1 | ||
| github.com/santhosh-tekuri/jsonschema/v6 v6.0.2 |
There was a problem hiding this comment.
how much do we trust this new dep?
There was a problem hiding this comment.
Seems widely used and updated. But can't really say :/
There was a problem hiding this comment.
I'm also concerned about that. It seems to be widely used, but most of the projects that use it in the dependency graph seem to have 0 stars/forks: https://github.com/santhosh-tekuri/jsonschema/network/dependents
Some notable exceptions:
- https://github.com/apache/doris
- https://github.com/redpanda-data/console
- https://github.com/elastic/go-elasticsearch
Would https://github.com/google/jsonschema-go fit our purposes?
There was a problem hiding this comment.
One thought that came to mind is to do the validation after dropping privileges/capabilities. This is not easy, and AFAICT the best way to achieve it would be by executing a binary (it can even be tetragon).
There was a problem hiding this comment.
Would https://github.com/google/jsonschema-go fit our purposes?
After a quick look, i don't think it supports registering custom vocabulary and validate in a single step like we do with jsonschema (very elegantly).
And i'd really love to avoid the huge "take all x-kubernetes-validations keyword values in the json schema, then for each build the context (ie: which strucutre they are attached), and finally manually execute all cel programs with their own context after the json schema validation".
It becomes a real mess :) And it was the thing i thought we had to do (and that's why i said "it could be done but it's really hard) before discovering about the custom vocabulary feat of https://github.com/santhosh-tekuri/jsonschema.
IMO the 3 "notable exceptions" you provided are pretty big.
There was a problem hiding this comment.
This is not easy, and AFAICT the best way to achieve it would be by executing a binary (it can even be tetragon).
😱
| kubeconform: ## Validate Helm chart using kubeconform | ||
| kubeconform: | ||
| mkdir -p $(JSON_SCHEMAS)/ $(SCHEMAS_CACHE)/ | ||
| $(PYTHON) /bin/bash -c "pip install pyyaml && python /code/install/kubernetes/openapi2jsonschema.py /code/$(CRDS_RELATIVE_DIR)/*" | ||
| mv $(ROOT_DIR)/install/kubernetes/*-cilium.io.json $(JSON_SCHEMAS)/ | ||
| kubeconform: $(TETRAGON_CHART)/schemas ## Validate Helm chart using kubeconform |
There was a problem hiding this comment.
could you explain a little bit in the git commit log the why of these changes? I'm not super familiar on why we were doing all this before
There was a problem hiding this comment.
I'm not super familiar on why we were doing all this before
I'm not either :) i just split the schemas generation code from being part of the kubeconform target to its own.
(I just restored the SCHEMAS_CACHE thing because it's still useful).
I expanded the commit message to be more precise :)
Also, note that i tested make kubeconform and it works fine.
0834ad5 to
ec01176
Compare
a3f1776 to
f76b9ad
Compare
|
Moving to draft to fix CEL stuff. |
6033959 to
a6b9a67
Compare
|
Fixed CEL stuff. |
50a4218 to
9d1c7d1
Compare
9d1c7d1 to
262eca7
Compare
We already generated schemas as part of the `kubeconform` make target; move the schemas generation to their own target, and let `kubeconform` depend upon the new target. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
262eca7 to
616ed9e
Compare
Also, run `make generate` to generate them. Next commit will use the schemas through `go:embed` to validate policies. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Prior to this change, feeding a wrong-syntax policy spec to nok8s build would've accepted it just fine. After this change, syntax errors are caught and correctly thrown. Note that `x-kubernetes-validations` CEL expressions validation will come in the follow up commit. New dependency: github.com/santhosh-tekuri/jsonschema/v6. Impact of the change: * Binary size: * main: `49532K` * HEAD: `50752K` * Running memory: * main: `70252K` * HEAD: `73168` Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
616ed9e to
6681e99
Compare
mtardy
left a comment
There was a problem hiding this comment.
thanks I think overall lgtm!
Never a big fan of adding new deps to the projects, especially ones that belong to one individual author but I if the feature is worth it!
We register a `vocabulary` for the `jsonschema.Compiler` that basically maps each `"x-kubernetes-validations"` entry to a function that loads the CEL `rule` and `message`, compiles the rule and store the resulting CEL program. The resulting `celSchema` will then validate all stored CEL programs during the jsonschema validation. `k8s_xvalidation_test` tests are not `k8s` builds specific anymore. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Before this change, we would've tried to evaluate `!return` even when the rule did not include any `return` statement, leading to a warning in the new nok8s `celSchema.Validate()`. Consequently, bumped `CustomResourceDefinitionSchemaVersion`. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
The case is already covered by `tracingpolicy/k8s_xvalidation_test` now. Also, cleaned up unused `build.K8sEnabled()` helper. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
6681e99 to
237b5ac
Compare
|
@mtardy fixed your suggestions, PTAL 🙏 |
Description
Since in nok8s builds we can't rely upon kubernetes validation, we were lacking any sort of validation for the tracing policy yaml object.
This patch introduces a validation based upon the json schema of the crds that were previously generated and used only in the
kubeconformmake target.New dependency: github.com/santhosh-tekuri/jsonschema/v6; only used in nok8s build.
Size change:
~800K.
Most of the line changes come from:
pkg/tracingpolicy/schemas/json schemasChangelog