Add --metrics-omit-secret-labels to skip per-SealedSecret labels on condition_info#1972
Conversation
The sealed_secrets_controller_condition_info gauge carries one time series per SealedSecret with namespace and name labels, served on the unauthenticated :8081 endpoint. The default Helm chart ships with networkPolicy.enabled: false, so any pod in the cluster can scrape :8081 and enumerate every SealedSecret name and namespace -- a cluster-wide inventory of where credentials live, regardless of the caller's RBAC on the sealedsecrets resource. For operators who treat that inventory as sensitive (multi-tenant clusters, descriptive names like database-credentials, payment-api-key), add a controller flag that opts out of emitting the per-secret labels entirely. Other metrics (unseal_requests_total, unseal_errors_total, http_*) are unaffected. Default is off so behaviour is unchanged for existing deployments. The flag wires through Flags.MetricsOmitSecretLabels into a package level toggle that ObserveCondition and UnregisterCondition consult. Discussed in GHSA-mx74-8fcw-qjrm. Signed-off-by: 0xVijay <53905346+0xVijay@users.noreply.github.com>
|
This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution. |
|
Due to the lack of activity in the last 7 days since it was marked as "stale", we proceed to close this Pull Request. Do not hesitate to reopen it later if necessary. |
|
Could you sign off the patches, please?. DCO is failing |
adaf504 to
eeca1a9
Compare
|
done @alvneiayu |
|
This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution. |
|
Due to the lack of activity in the last 7 days since it was marked as "stale", we proceed to close this Pull Request. Do not hesitate to reopen it later if necessary. |
2 participants
What this does
Adds a new controller flag
--metrics-omit-secret-labels(defaultfalse). When enabled,sealed_secrets_controller_condition_infois no longer updated, so the metrics endpoint stops emitting one time series per SealedSecret with thenamespace/namelabels. All other metrics (unseal_requests_total,unseal_errors_total,http_*) are unaffected.Why
The
condition_infogauge carries{namespace, name, condition, instance}labels and is served on:8081, which has no auth middleware. The default Helm chart ships withnetworkPolicy.enabled: false, so any pod in the cluster can hit the endpoint and dump a full inventory of SealedSecret names and namespaces, regardless of whether the caller has any RBAC onsealedsecrets.For operators who use descriptive names (
database-credentials,payment-api-key,stripe-prod-key) or run multi-tenant clusters, that inventory itself is sensitive — it tells an attacker exactly which namespaces store which credentials. Even with a NetworkPolicy in place, anything authorised to scrape the metrics port can read it.Discussed in advisory GHSA-mx74-8fcw-qjrm. @alvneiayu closed it as an insecure-config issue rather than a vuln, but agreed there's value in an option to restrict the information surfaced by the endpoint and suggested sending a PR.
Design
A package-level toggle in
metrics.go(metricsOmitSecretLabels) set bySetMetricsOmitSecretLabels(...), called fromcontroller.MainbeforeregisterMetrics.ObserveCondition/UnregisterConditionearly-return when the toggle is on. Operators who want the labels back simply omit the flag — default behaviour is unchanged.I considered a few alternatives (hash the labels, drop only
nameand keepnamespace, etc.) but settled on the simplest restrict option for the first cut. Happy to switch to a--metrics-secret-labels-mode={full,namespace-only,none}style flag if you'd prefer that shape.Test
Added
TestObserveConditionRespectsOmitSecretLabels— sets the flag, callsObserveCondition, verifies the gauge has zero series.Notes
false— no behaviour change unless the flag is set.Flagsstruct field padding when adding the new field. That's whypkg/controller/main.goshows a larger diff than the actual logic change.