Skip to content
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

✨ (Blocked by #1475) - Ensure that metrics is protected with TLS for Prometheus integration. #1476

Conversation

camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented Nov 17, 2024

⚠️ This PR is blocked by #1475 ; the goal is to merge the second commit. After we merge (#1475) we can rebase and move forward within

This change replaces controllers-runtime's self-signed certificates for metrics with cert-manager-managed certificates, ensuring security. We should not use insecureSkipVerify: true. Also, it is not a good practice for production env relay on the certs self-signed generated by controller-runtime and/or the old kube-rbac-proxy according to sig-security.

Key updates:

  • Configured metricsServerOptions to use cert-manager-managed certificates (/var/metrics/certs).
  • Added ClusterIssuer (olmv1-metrics-ca) and Certificate resources for automated certificate management.
  • Updated ServiceMonitor to enable secure TLS scraping by Prometheus using certificates issued by olmv1-metrics-ca.
  • The deployment was pushed to mount metrics certificates as secrets.

Benefits:

  • Enhanced security with automated certificate lifecycle management.
  • Production-ready TLS setup for Prometheus metrics scraping.

…n/authorization

This commit removes the use of the kube-rbac-proxy image and replaces it with metrics authentication/authorization provided by controller-runtime. The kube-rbac-proxy image is deprecated and will no longer be maintained, which introduces risks to production environments. For more details, see: kubernetes-sigs/kubebuilder#3907

Key changes:
- Updated  to configure metrics server options with secure authentication/authorization using controller-runtime filters.
- Added support for disabling HTTP/2 by default to mitigate vulnerabilities (e.g., HTTP/2 Stream Cancellation CVE).
- Changed the default metrics endpoint to HTTPS (port 8443) and removed the kube-rbac-proxy container from deployment configurations.
- Updated RBAC files to include metrics-specific roles and bindings, ensuring secure access to metrics.

This aligns with best practices for security and simplifies the metrics setup by leveraging built-in capabilities of controller-runtime.
@camilamacedo86 camilamacedo86 requested a review from a team as a code owner November 17, 2024 11:26
Copy link

netlify bot commented Nov 17, 2024

Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit 32c69b4
🔍 Latest deploy log https://app.netlify.com/sites/olmv1/deploys/6739d51a6450500008e29b84
😎 Deploy Preview https://deploy-preview-1476--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@camilamacedo86 camilamacedo86 force-pushed the ensure-prometheus-protection branch from 5c8f432 to ea6d722 Compare November 17, 2024 11:26
@camilamacedo86 camilamacedo86 changed the title (Blocked by #1475) - Integrate cert-manager for production-grade metrics TLS and Prometheus integration. (Blocked by #1475) - Ensure that metrics is protected with TLS for Prometheus integration. Nov 17, 2024
@camilamacedo86 camilamacedo86 changed the title (Blocked by #1475) - Ensure that metrics is protected with TLS for Prometheus integration. ✨ (Blocked by #1475) - Ensure that metrics is protected with TLS for Prometheus integration. Nov 17, 2024
@camilamacedo86 camilamacedo86 force-pushed the ensure-prometheus-protection branch from ea6d722 to a3928ad Compare November 17, 2024 11:32
This change replaces controllers-runtime's self-signed certificates for metrics with cert-manager-managed certificates, ensuring security. We should not use `insecureSkipVerify: true`. Also, it is not a good practice for production env relay on the certs self-signed generated by controller-runtime and/or the old kube-rbac-proxy according to sig-security.

Key updates:
- Configured `metricsServerOptions` to use cert-manager-managed certificates (`/var/metrics/certs`).
- Added `ClusterIssuer` (`olmv1-metrics-ca`) and `Certificate` resources for automated certificate management.
- Updated `ServiceMonitor` to enable secure TLS scraping by Prometheus using certificates issued by `olmv1-metrics-ca`.
- The deployment was pushed to mount metrics certificates as secrets.

Benefits:
- Enhanced security with automated certificate lifecycle management.
- Production-ready TLS setup for Prometheus metrics scraping.
@camilamacedo86 camilamacedo86 force-pushed the ensure-prometheus-protection branch from a3928ad to 32c69b4 Compare November 17, 2024 11:35
Copy link

codecov bot commented Nov 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.96%. Comparing base (63ef902) to head (32c69b4).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1476      +/-   ##
==========================================
+ Coverage   74.73%   74.96%   +0.23%     
==========================================
  Files          42       42              
  Lines        3241     3272      +31     
==========================================
+ Hits         2422     2453      +31     
  Misses        646      646              
  Partials      173      173              
Flag Coverage Δ
e2e 52.87% <100.00%> (+0.54%) ⬆️
unit 56.66% <0.00%> (-0.55%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@camilamacedo86
Copy link
Contributor Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 19, 2024
@tmshort
Copy link
Contributor

tmshort commented Nov 19, 2024

This should use the existing olmv1-ca to sign the certificate. No need to add another.

This should add support to rotate the certificates via controller-runtime/pkg/certwatcher

@camilamacedo86
Copy link
Contributor Author

Hi @tmshort thank you for the feedback.

@tmshort
Copy link
Contributor

tmshort commented Nov 19, 2024

We also need to consider that when/if we merge catalogd/operator-controller into a single binary, that this may be a combined service.

catalogd already has support for certwatcher.

@camilamacedo86
Copy link
Contributor Author

Hi @tmshort

I will create a Brief and RFC to explain why it is required
Also, I will follow your suggestion that will simplify things
By last keep all in the same PR to add as example in the RFC

And yes, you are right. We need to do the same with catalogD

@camilamacedo86
Copy link
Contributor Author

Hi @tmshort

Thank you for your input. That was very, very helpful.
Since we could simplify for now, I will include all in #1475 so it will be easier to use it in the RFC.
But I will follow your input. !

Closing in favor of #1475

@camilamacedo86 camilamacedo86 deleted the ensure-prometheus-protection branch November 19, 2024 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants