-
Notifications
You must be signed in to change notification settings - Fork 520
ESO-165: Enhancement Proposal for External Secrets Operator Network Policy #1834
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
It feels rather dangerous for the operator to manage its own NP. If something ever goes wrong with the policy (for example, in an update) that prevented the operator from connecting, how would it recover? I think it's safer just to have the operator manage its operands and leave the operator unprotected until the OLM manifests can take NPs. |
5f8ee63
to
f906b59
Compare
@siddhibhor-56: This pull request references ESO-165 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
@siddhibhor-56: This pull request references ESO-165 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
except for couple of suggestions.
cc: @TrilokGeer @mytreya-rh for further reviews.
apiVersion: networking.k8s.io/v1 | ||
kind: NetworkPolicy | ||
metadata: | ||
name: allow-api-server-egress-for webhook |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name: allow-api-server-egress-for webhook | |
name: allow-api-server-egress-for-webhook |
// | ||
// +kubebuilder:validation:Optional | ||
// +optional | ||
NetworkPolicy []v1.NetworkPolicy `json:"networkPolicy,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have an ID to map the policy to help with reconciliation.
NetworkPolicy []v1.NetworkPolicy `json:"networkPolicy,omitempty"` | |
NetworkPolicy *v1.NetworkPolicy `json:"networkPolicy,omitempty"` | |
NetworkPolicies []NetworkPolicy `json:"networkPolicies,omitempty"` | |
} | |
type NetworkPolicy struct { | |
// name to assign to the created NetworkPolicy object. | |
// +required | |
Name string `json:"name,omitempty"` | |
NetworkPolicy *v1.NetworkPolicy `json:"networkPolicy,omitempty"` | |
} |
Hi @knobunc , As of now the network policy of the operand will be managed by the operator whereas the operator network policy will be managed by OLM bundle. |
kind: NetworkPolicy | ||
metadata: | ||
name: allow-operator-traffic | ||
namespace: external-secrets-operator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we are fixing the namespace of the operator here, right?
i think the user as of now has the option to install in a different namespace right?
Are we going to update the documentation that operator namespace should be fixed, or is it already in the docs?
Or shall we specify that if the namespace is changed, the operator network policies will not take effect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @bharath-b-rh and @siddhibhor-56 for pointing me to the kustomization
Actually, the namespaces, specifically for network policy, seemed to be patched here in OLM code, right?
/lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes correct ,
The namespace is not fixed, its user-configurable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
except for few nitpicks.
|
||
* **Allow Egress to API Server:** Permit egress to the API server on port 6443/TCP so it can inject CA data into other resources. | ||
* **Allow Ingress from Core Controller:** Permit ingress from the `external-secrets` controller pod for communication with the Bitwarden server. | ||
* Note: The Bitwarden server is an optional integration. It is only created if explicitly enabled by user configuration.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Note: The Bitwarden server is an optional integration. It is only created if explicitly enabled by user configuration.* | |
* Note: The Bitwarden server is an optional integration. It is only created if explicitly enabled by user configuration. User has to additionally create a allow NetworkPolicy to interact with the external `Bitwarden Secret Manager`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
* **For the `external-secrets-bitwarden-sever` pod (`app: external-secrets`):** | ||
|
||
* **Allow Egress to API Server:** Permit egress to the API server on port 6443/TCP so it can inject CA data into other resources. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* **Allow Egress to API Server:** Permit egress to the API server on port 6443/TCP so it can inject CA data into other resources. | |
* **Allow Egress to API Server:** Permit egress to the API server on port 6443/TCP so it can store the secrets fetched from external `Bitwarden Secrets Manager` into a Kubernetes Secret resource. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
- protocol: TCP | ||
port: 6443 # Required: Kubernetes API server | ||
ingress: | ||
# Optional: expose metrics (8443 and/or 8080) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Optional: expose metrics (8443 and/or 8080) | |
# Optional: expose metrics (8443 or 8080 based on user configuration) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
port: 6443 | ||
``` | ||
|
||
3. **Allow `external-secrets-webhook` Controller Traffic:** This policy allows the API Server Access for Outbound communication to Kubernetes API server (port 6443) for resource reconciliation and Webhook Admission Control Inbound traffic from API server to webhook (port 10250) for resource validation and mutation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3. **Allow `external-secrets-webhook` Controller Traffic:** This policy allows the API Server Access for Outbound communication to Kubernetes API server (port 6443) for resource reconciliation and Webhook Admission Control Inbound traffic from API server to webhook (port 10250) for resource validation and mutation. | |
3. **Allow `external-secrets-webhook` Controller Traffic:** This policy allows the API Server Access for Outbound communication to Kubernetes API server (port 6443) for resource reconciliation and Webhook Admission Control Inbound traffic from API server to webhook (port 10250) for resource validation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
port: 10250 | ||
``` | ||
|
||
4. **Allow `external-secrets-cert-controller` Traffic:** This policy allows the cert-controller API Server Access for outbound communication to Kubernetes API server (port 6443/TCP) for certificate lifecycle management. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4. **Allow `external-secrets-cert-controller` Traffic:** This policy allows the cert-controller API Server Access for outbound communication to Kubernetes API server (port 6443/TCP) for certificate lifecycle management. | |
4. Allow `external-secrets-cert-controller` Traffic:** This policy allows the cert-controller API Server Access for outbound communication to Kubernetes API server (port 6443/TCP) for certificate lifecycle management. |
port: 6443 | ||
``` | ||
|
||
5. **Allow `external-secrets-bitwarden-server` Traffic: This policy permits the Bitwarden server to communicate with the Kubernetes API server (port 6443/TCP) for secret synchronization and certificate validation, and to receive inbound traffic from the core controller for integration workflows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5. **Allow `external-secrets-bitwarden-server` Traffic: This policy permits the Bitwarden server to communicate with the Kubernetes API server (port 6443/TCP) for secret synchronization and certificate validation, and to receive inbound traffic from the core controller for integration workflows. | |
5. **Allow `external-secrets-bitwarden-server` Traffic: This policy permits the Bitwarden server to communicate with the Kubernetes API server (port 6443/TCP) for secret synchronization, and to receive inbound traffic from the core controller for integration workflows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
- protocol: TCP | ||
port: 6443 | ||
``` | ||
6. **User-Configurable Policies:** Users must configure additional policies via the API for external-secrets controller egress (to communicate with external providers). Example user configuration: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6. **User-Configurable Policies:** Users must configure additional policies via the API for external-secrets controller egress (to communicate with external providers). Example user configuration: | |
6. **User-Configurable Policies:** Users must configure additional policies via the `ExternalSecrets` custom resource to set `external-secrets` controller egress allow policy to communicate with external providers. Example user configuration: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bharath-b-rh The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
It'd be great to validate against some practices implemented similarly openshift/cluster-olm-operator#118 |
/lgtm |
Ya, Thank you for pointing it out @TrilokGeer necessary changes had been added in the EP regarding the "from" clause in metrics. |
/lgtm |
@siddhibhor-56: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This PR implements the proposal to secure the External Secrets Operator (ESO) by introducing NetworkPolicy resources for the operands. The objective is to minimize the attack surface and ensure all components adhere to the principle of least privilege.
The approach begins with a baseline “deny-all” NetworkPolicy for ingress and egress traffic. From this baseline, only the minimal and explicitly required network flows are allowed for ESO components to function correctly.
Key allowed flows include:
API Server Access: Egress to the Kubernetes API server is permitted so ESO can reconcile resources.
Metrics Scraping(if applicable): Ingress on the metrics endpoint is allowed to enable Prometheus monitoring.
Webhook Validation (if applicable): Ingress on the webhook port is allowed to support admission review requests from the API server.