connaisseur
connaisseur copied to clipboard
Refactor helm deployment
When touching the helm deployment the next time, please consider the following things:
-
The way how we allow configurations of Kubernetes resource settings differs from resource to resource. We have an own section
service
to configure the KubernetesService
, but configuration for the webhook is handled in thedeployment
section (which I would have assumed to cover only settings for the KubernetesDeployment
resource judging from the additional section for theService
resource. I'd suggest to decide for one option for consistency reasons. -
Why do we allow the
Service
type to be configurable? Did we ever test Connaisseur with another type thanClusterIP
? Are we sure that our security assumptions hold for all service types equally? -
In certain circumstances (with a high probability), Connaisseur deploys empty resources (e.g.
env
oralertconfig
secret, there may be more). Are we sure that we want that? -
Do we want to explicitly enable features by adding an extra yaml subfield
enabled: true
like we do for e.g.automaticChildApproval
? However we decide, I'd suggest to do it consistently - right now,detectionMode
behaves differently thanautomaticChildApproval
for example. -
Tying into the point above: How do we want to handle keys, that are commented out? Do we want to be able to achieve the same behaviour with explicit values? Right now, there is no way for e.g. alerting to be turned off if not having the config block commented out.
-
We once decided to allow no trust root in
notaryv1
validator instances for the preconfig to work. This results in inconsistent trust root validations. I'd like to question the decision again, maybe we should rethink that. -
[Naming] Alerting has a
templates
list. I'd suggest to rename it toreceivers
. -
[Naming] Should the
policy
section rather bepolicies
?
Consider deprecating default
for trust roots in light of #428
Consider dropping dockerhub official public key
See also https://github.com/sse-secure-systems/connaisseur/issues/383 - might become more relevant when users can configure their own side cars etc.
normalize naming either camel-case or snake case
- Check whether
reinvocationPolicy: Never
could lead to the scenario that another admission controller e.g. adds a sidecar container to a Pod after Connaisseur did its mutation, thus, leading to unverified containers in a deployment. If so, change default such that this cannot happen by default
improve hierarchy and summarize all features under a feature
key
Refactor version mgmt:
- Version should probably only be in Chart.yml, bc valid values.yml depends on which helm version it is, so should put it there -> 〰️ decided to default to Chart, but allow config in helm/values.yaml
- Split off tag from image? -> ✔️
- Single version instead of app and helm version?
- Check whether
reinvocationPolicy: Never
could lead to the scenario that another admission controller e.g. adds a sidecar container to a Pod after Connaisseur did its mutation, thus, leading to unverified containers in a deployment. If so, change default such that this cannot happen by default
Since we're not technically idempotent due to a change in image tag potentially resulting in a different policy being applied, changing the reinvocationPolicy by default seems risky. However, I agree that this is a potential vector to bypass validation if there's neglience in another mutating admission controller (an attacker controlling with direct access to admission controllers may also be able to forge Connaisseurs controller instead of hijacking another one)
@annekebr what do you think about documenting this as a limitation instead? Other option I'd see is if user could set a flag validateMutationSafe
or the like, which then employs a validating admission controller and whose documentation suggests using policies that lend themselves to idempotent execution through Connaisseur, e.g. policies don't specify human-readable tags and instead either *
or digests
[Naming] Should the policy section rather be policies?
Talked with @phbelitz and we both agreed that it is a single policy with multiple rules below it (which is also the notion in the explaining comments), so would not rename
Should we move to explicitly support only currently supported k8s versions? (while not purposefully bricking old ones of course)
E.g. since start of April the legacy tests fail since their runners were deprecated for good and I don't see that we should invest more time into fixing up these tests.
Afaik @phbelitz agrees. @xopham Thoughts?