connaisseur icon indicating copy to clipboard operation
connaisseur copied to clipboard

Refactor helm deployment

Open annekebr opened this issue 2 years ago • 6 comments

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 Kubernetes Service, but configuration for the webhook is handled in the deployment section (which I would have assumed to cover only settings for the Kubernetes Deployment resource judging from the additional section for the Service 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 than ClusterIP? 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 or alertconfig 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 than automaticChildApproval 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 to receivers.

  • [Naming] Should the policy section rather be policies?

annekebr avatar Jan 21 '22 15:01 annekebr

Consider deprecating default for trust roots in light of #428

xopham avatar Jan 21 '22 15:01 xopham

Consider dropping dockerhub official public key

xopham avatar Jan 21 '22 15:01 xopham

See also https://github.com/sse-secure-systems/connaisseur/issues/383 - might become more relevant when users can configure their own side cars etc.

annekebr avatar Jan 21 '22 15:01 annekebr

normalize naming either camel-case or snake case

xopham avatar Jan 21 '22 15:01 xopham

  • 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

annekebr avatar Feb 22 '22 18:02 annekebr

improve hierarchy and summarize all features under a feature key

xopham avatar Feb 22 '22 21:02 xopham

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?

Starkteetje avatar Mar 17 '23 09:03 Starkteetje

  • 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

Starkteetje avatar Mar 27 '23 07:03 Starkteetje

[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

Starkteetje avatar Apr 04 '23 11:04 Starkteetje

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?

Starkteetje avatar Apr 04 '23 14:04 Starkteetje