Hi @rclsilver ! can you also add this to webhook and certController as well? 😄

Also, change our tests definition to include an init Container and see it passing :)

As stated in Slack, my main concern is if someone can hijack a pipeline install where ESO is being installed from as a helm chart and they inject an initContainer with malicious code in it.

I guess, they could do that with other deployments, but if there aren't any, ESO is providing another attack vector for that operation.

Therefore, I'm on the fence with this modification. There are no guardrails around that unfortunately. Or rather, I don't know of any. :)

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

I just fixed the check-diff test and added a test for the helm chart.

Tell me if you want me to add the extraInitContainers for the other components.

As stated in Slack, my main concern is if someone can hijack a pipeline install where ESO is being installed from as a helm chart and they inject an initContainer with malicious code in it.

I guess, they could do that with other deployments, but if there aren't any, ESO is providing another attack vector for that operation.

Therefore, I'm on the fence with this modification. There are no guardrails around that unfortunately. Or rather, I don't know of any. :)

Yes, this is a valid concern. However, the best way to integrate a webhook provider today would be to make it available as a sidecar and make external-secrets do a connection via a unix socket to it. The best way to do that IMO is during installation time, right?

If we look at other similar projects:

• SecretStore CSI Driver does not allow: https://github.com/kubernetes-sigs/secrets-store-csi-driver/blob/main/charts/secrets-store-csi-driver/templates/secrets-store-csi-driver.yaml - Note: this is because providers communicate directly via hostpath, and interface directly with kubelet).

• Kyverno does allow: https://github.com/kyverno/kyverno/blob/main/charts/kyverno/templates/admission-controller/deployment.yaml#L93-L96

• Gatekeeper does not allow: https://github.com/open-policy-agent/gatekeeper/blob/master/charts/gatekeeper/templates/gatekeeper-controller-manager-deployment.yaml

• Cert Manager does not allow: https://artifacthub.io/packages/helm/cert-manager/cert-manager?modal=template&template=deployment.yaml

Which would mean we should probably remove this feature 🤔

Things we would need to remove thinking on securing the container on our own release:

• We already allow it with extraContainers: https://github.com/external-secrets/external-secrets/blob/ec12919d04d0aee48d5051a2a08b8002a3a07d6c/deploy/charts/external-secrets/templates/deployment.yaml#L116C1-L118C19

• We also already allow hostPaths to be set up, which means users could do this anyways on a worse security posture 🤔. And this one is needed sometimes just to make ESO work due to CNI overlay shenanigans.

IMO We should either support initContainers or remove extraContainers... whether this means people will need to fork off helm charts or not.

(and also, guide users how to install a in-pod-like webhook provider 😆 )

@gusfcarvalho I'm on the side of security hardening. Sorry @rclsilver. I can be overruled ofc.

@Skarlso Quoting from your previous comment:

and they inject an initContainer with malicious code in it.

I fail to see how an init container would have any impact on the operator itself. The init containers runs independently from the main container, whether they have restartPolicy: Always or not to act as real "sidecars". pid namespace can be shared between containers, but that's not the default, so the remaining communication channel is through local loopback, which is what we're doing in our current usecase as suggested by @gusfcarvalho to @rclsilver on slack.

IMO, the Helm chart that you provide has little to do with security hardening of the operator itself, it's just a "template" to deploy the operator's image and related Kube resources.

IMO We should either support initContainers or remove extraContainers... whether this means people will need to fork off helm charts or not.

I agree, it's either none or both, but as I said, the Helm chart itself is nothing else than a convenience for the community to deploy the operator. If you reject customization's values, the users will simply fork or write their own chart.

@gusfcarvalho

If we look at other similar projects:

I haven't done any research to prove or disprove that any of these projects had a valid reason in the past to allow the customization of containers or init containers in their charts, but I could find other examples of charts from OSS projects that allow customization of containers/initContainers (fluent-bit for example).
It may be related to the specific usage/needs of those projects to run sidecar containers.

Perhaps to clarify the discussion, @Skarlso, are you more concerned about the ability to add others containers to the operator's pod, or the ability to query another container locally through the webhook provider ?

Security wise, no one should use webhook providers, really :) they are on untrusted network bound perimeters. They are just an operational “must have” for a lot of use cases; but in any of those these use cases are sacrificing security for that (hopefully, in a conscious manner).

those projects are all on the security space, and most of them drop this support, so this is why they were chosen as benchmark.

I understand the positioning for security hardening and to be honest, we might really remove support for sidecars 🤔🤔.

Re: how could this be used for evil: the external-secret service account would be available for the attacker to freely steal all your secrets 🥲🥲🥲

adding this to be discussed in the next community meeting. Please join them @rclsilver @wI2L 😃

I agree, it's either none or both, but as I said, the Helm chart itself is nothing else than a convenience for the community to deploy the operator. If you reject customization's values, the users will simply fork or write their own chart.

if it were just a convenience, users would have no problem in forking it and maintaining their own. For me this is an argument to do breaking changes for the sake of what we think it’s a best convenient interface for users to start with 😂😂. We both know this isn’t really true and that people expect this helm chart deliverable as a part of the pipelines.

I fail to see how an init container would have any impact on the operator itself.

initContainer can be used to load a compromised image that executes during load potentially injecting malicious code.

It could cause dependency chain attacks, or exhaust resources, even prevent the main thing to run if it keeps crashing.

Also, since it can share a mount it can include a backdoor script into the main application.

@Skarlso Not saying that what you said is false per so, just far fetched. At this point, following your argument, the image.repository value is also a potential risk since it can be edited to pull a compromised operator's image.

What you said is also true for non-init containers, yet the chart allows to add extra containers.

My honest opinion on that matter is is that while those considerations are valid, this shouldn't be the chart's responsibility to prevent it, but a broader subject that involves CI/deploy workflows.

Anyway, this doesn't represent a blocker, as we said, users could build their own Helm chart if they disagree with the stance that you take about this subject.

However, we'd like to talk more about the Webhook provider (which is pretty handy in our case), in order to clarify what's you have in mind about this feature:

(and also, guide users how to install a in-pod-like webhook provider 😆 )
I understand the positioning for security hardening and to be honest, we might really remove support for sidecars 🤔🤔.

@gusfcarvalho @moolen Let's discuss this on next community meeting. I added it to the agenda ( 4th of June ).

Reading through this leaves me a bit puzzled;
We can already achieve the same behavior by adding a sleep to a sidecar container's command, its just wasteful.
How do the security concerns for init containers differ from the already existing extra sidecar containers?

@ItielOlenick The point here is to remove that option as well. :)

👍 my 5ct:

I think it's totally fine to make initContainers and extraContainers configurable. There are various valid reasons why users would need to do this. It makes it easier to customize the app. Users will do this anyway - one way or the other.

🤡 I found myself populating the /etc/hosts and/or /etc/resolv.conf via init-continer / sidecar the other day 🫠

I'd be happy to trade usability with a slight increase in attack surface. It's not our responsibility to harden the deployment pipeline of our users.

It's such a common practice in helm charts that seeing this as an issue is extremely odd IMO.
We need to add a custom credential process and this is the way we can do so.

It's such a common practice in helm charts that seeing this as an issue is extremely odd IMO.
We need to add a custom credential process and this is the way we can do so.

If you check the thread - while it is a common practice in helm charts, almost none of the security-related tooling charts expose that 😄

Users will do this anyway - one way or the other.

But I guess that's the point. Having this as a well known, provided upstream option for ill-intended users vs. not having as an option so users that need this would do it anyways.

I totally understand the point of exposing a very, very privileged service account to any other processes as a potential risk here, while for the use case that started this whole discussion (a webhook integration), there are other ways to bind it to external-secrets (like the good old socket + CSI).

IMO we document ☝️ socket + CSI method, and stop supporting other methods which are way easier to abuse.

I voiced my concerns. :) Other than that, I will follow whatever the maintainer majority is. :)

Hi,

If the webhook functionality were to be removed, what would be a realistic alternative for supporting similar integration use cases?

The main advantage of this provider is that it allows us to integrate internal systems without needing to modify ESO’s code directly, which is critical for our workflow.

Thanks in advance for your input.

Hi,

If the webhook functionality were to be removed, what would be a realistic alternative for supporting similar integration use cases?

The main advantage of this provider is that it allows us to integrate internal systems without needing to modify ESO’s code directly, which is critical for our workflow.

Thanks in advance for your input.

Its not disallowing webhooks - but disallowing them to be used with sidecar approach (favoring more secure modes, like mounting an unix socket).

In our case, if the running component exposes a Unix socket, we would need a way to ensure that this socket exists (most likely by sharing it through an emptyDir) before starting ESO. This is something that naturally happens when using sidecars, as lifecycle dependencies are implicitly managed.

I'm having a hard time understanding how exposing a Unix socket via emptyDir would be more secure than allowing sidecars.

I'm having a hard time understanding how exposing a Unix socket via emptyDir would be more secure than allowing sidecars.

Unix Sockets can be mounted as a CSI volume mount, effectively removing the need for anything running as a sidecar to external-secrets. This is how SPIRE and SecretStore CSI work.

For that to work, my guess is that we would need to provide some sort of webhook-csi-driver for users plus docs around it. At least thats what I planned to discuss on the next community meeting if I was able to join it 😅

It should be OK with this version

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud