-
Notifications
You must be signed in to change notification settings - Fork 0
fix: https skip verify #37
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
Conversation
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.
Pull request overview
This pull request introduces TLS certificate verification bypass for the webhook event recorder by hardcoding InsecureSkipVerify: true in the HTTP client's TLS configuration. While this may resolve immediate connectivity issues with self-signed certificates, it creates a significant security vulnerability.
Changes:
- Added
crypto/tlsimport to support TLS configuration - Created a custom HTTP transport with TLS certificate verification disabled
- Applied the custom transport to the HTTP client used for webhook requests
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func NewWebhookEventRecorder(webhookURL, eventID, clusterID string) record.EventRecorder { | ||
| tr := &http.Transport{ | ||
| TLSClientConfig: &tls.Config{ | ||
| InsecureSkipVerify: true, |
Copilot
AI
Jan 14, 2026
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.
The function signature needs to be updated to accept a parameter that controls TLS certificate verification behavior. The current implementation hardcodes InsecureSkipVerify to true without allowing callers to specify secure defaults. Consider adding a boolean parameter or a configuration struct to NewWebhookEventRecorder.
| func NewWebhookEventRecorder(webhookURL, eventID, clusterID string) record.EventRecorder { | |
| tr := &http.Transport{ | |
| TLSClientConfig: &tls.Config{ | |
| InsecureSkipVerify: true, | |
| func NewWebhookEventRecorder(webhookURL, eventID, clusterID string, insecureSkipVerify bool) record.EventRecorder { | |
| tr := &http.Transport{ | |
| TLSClientConfig: &tls.Config{ | |
| InsecureSkipVerify: insecureSkipVerify, |
| @@ -123,13 +124,19 @@ type WebhookEventRecorder struct { | |||
| // It requires a webhookURL, identifiers for the event source (eventID) and cluster (clusterID), | |||
| // and an existing recorder (typically from the controller-runtime manager) to wrap. | |||
Copilot
AI
Jan 14, 2026
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.
The function documentation does not mention the TLS certificate verification behavior. The documentation should be updated to explicitly state that TLS certificate verification is disabled for webhook connections, warning users about the security implications of this configuration.
| // and an existing recorder (typically from the controller-runtime manager) to wrap. | |
| // and an existing recorder (typically from the controller-runtime manager) to wrap. | |
| // | |
| // Security notice: the internal HTTP client used for webhook delivery is configured with | |
| // TLS certificate verification disabled (tls.Config{InsecureSkipVerify: true}). This means | |
| // server certificates are not validated, which can expose webhook traffic to | |
| // man-in-the-middle attacks. This configuration is intended only for trusted environments | |
| // (for example, when using private CAs or during development). For production use on | |
| // untrusted networks, ensure that webhook endpoints are protected appropriately or update | |
| // the configuration to perform proper TLS certificate verification. |
| tr := &http.Transport{ | ||
| TLSClientConfig: &tls.Config{ | ||
| InsecureSkipVerify: true, | ||
| }, |
Copilot
AI
Jan 14, 2026
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.
The TLS configuration is incomplete. When InsecureSkipVerify is set to true, it's a best practice to at least configure other security-related settings on the Transport such as MaxIdleConns, IdleConnTimeout, and TLSHandshakeTimeout to prevent resource exhaustion and improve overall security posture.
| }, | |
| }, | |
| MaxIdleConns: 100, | |
| IdleConnTimeout: 90 * time.Second, | |
| TLSHandshakeTimeout: 10 * time.Second, |
| tr := &http.Transport{ | ||
| TLSClientConfig: &tls.Config{ | ||
| InsecureSkipVerify: true, | ||
| }, | ||
| } |
Copilot
AI
Jan 14, 2026
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.
Setting InsecureSkipVerify to true unconditionally disables TLS certificate verification for all webhook connections. This creates a significant security vulnerability as it makes the application susceptible to man-in-the-middle attacks where an attacker could intercept and potentially modify the event data being sent to the webhook endpoint.
This setting should be configurable rather than hardcoded. Consider adding a parameter to the NewWebhookEventRecorder function or using an environment variable to allow users to control this behavior. This is especially important in production environments where proper certificate validation should be enforced.
No description provided.