Fix Redfish protocol and TLS certificate verification handling#739
Open
stefanhipfel wants to merge 3 commits intomainfrom
Open
Fix Redfish protocol and TLS certificate verification handling#739stefanhipfel wants to merge 3 commits intomainfrom
stefanhipfel wants to merge 3 commits intomainfrom
Conversation
Fixes #395 This commit addresses a security vulnerability where TLS certificate verification was hardcoded to be disabled (Insecure: true) in the Redfish BMC client, regardless of configuration. Changes: 1. BMC Package (bmc/redfish.go): - Added InsecureTLS bool field to Options struct - Updated newRedfishBaseBMCClient to use configurable TLS setting instead of hardcoded true 2. Command-Line Flags (cmd/main.go): - Added --protocol flag for explicit HTTP/HTTPS selection - Added --skip-cert-validation flag for TLS verification control - Deprecated --insecure flag with backward compatibility - Added clear warning messages for insecure configurations 3. Reconcilers (internal/controller/*): - Replaced Insecure bool field with DefaultProtocol and SkipCertValidation fields in all reconcilers: * EndpointReconciler * BMCReconciler * ServerReconciler * BIOSSettingsReconciler * BIOSVersionReconciler * BMCSettingsReconciler * BMCVersionReconciler * BMCUserReconciler 4. Utilities (internal/bmcutils/bmcutils.go): - Updated GetProtocolScheme to accept defaultScheme parameter - Updated GetBMCClientForServer with protocol and TLS parameters - Updated GetBMCClientFromBMC with protocol and TLS parameters - Updated CreateBMCClient to configure InsecureTLS option 5. Tests: - Updated suite_test.go with new field names - Updated server_controller_test.go with new field names - All tests passing Migration Guide: - Old: --insecure=false (HTTPS but no cert validation) - New: --protocol=https --skip-cert-validation=false (secure HTTPS) Backward Compatibility: - Existing --insecure flag continues to work with deprecation warnings - No breaking changes to existing deployments Security Improvements: - TLS certificate verification is now configurable - Users can enable secure HTTPS connections with proper cert validation - Clear warnings when running in insecure mode
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughThis PR replaces the legacy single Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/controller/suite_test.go (1)
164-168: Add at least one HTTPS + verification-enabled suite path.All shared controller fixtures here still use the legacy-equivalent combination of
httpplusSkipCertValidation: true, so CI never exercises the branch that actually verifies certificates. That leaves the security fix unpinned and won't catch regressions where protocol selection and cert validation get coupled again.Also applies to: 177-178, 190-191, 228-234, 244-250, 265-271, 280-286, 313-316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/suite_test.go` around lines 164 - 168, Tests currently only exercise HTTP with SkipCertValidation:true; add at least one suite that sets DefaultProtocol to the HTTPS scheme and SkipCertValidation to false so the certificate-verification branch is exercised. Locate the suite struct(s) that set Client, Scheme, MACPrefixes, DefaultProtocol (currently metalv1alpha1.HTTPProtocolScheme) and SkipCertValidation and add or modify a parallel test case where DefaultProtocol is the HTTPS constant (e.g., metalv1alpha1.HTTPSProtocolScheme) and SkipCertValidation is false; apply the same change to the other listed fixtures (the blocks around the referenced locations) so CI covers both HTTP+skip and HTTPS+verify paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/bmcuser_controller.go`:
- Around line 355-360: bmcConnectionTest currently discards the BMC client
returned by bmcutils.CreateBMCClient, leaking Redfish sessions; modify
bmcConnectionTest to call Logout() on the returned client after a successful
credential check (or defer a safe logout) so sessions are closed, ensuring you
check the client is non-nil before calling Logout; this prevents leaks when
updateEffectiveSecret invokes bmcConnectionTest multiple times.
---
Nitpick comments:
In `@internal/controller/suite_test.go`:
- Around line 164-168: Tests currently only exercise HTTP with
SkipCertValidation:true; add at least one suite that sets DefaultProtocol to the
HTTPS scheme and SkipCertValidation to false so the certificate-verification
branch is exercised. Locate the suite struct(s) that set Client, Scheme,
MACPrefixes, DefaultProtocol (currently metalv1alpha1.HTTPProtocolScheme) and
SkipCertValidation and add or modify a parallel test case where DefaultProtocol
is the HTTPS constant (e.g., metalv1alpha1.HTTPSProtocolScheme) and
SkipCertValidation is false; apply the same change to the other listed fixtures
(the blocks around the referenced locations) so CI covers both HTTP+skip and
HTTPS+verify paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ab78603-e34b-4035-8757-29c209d50e1f
📒 Files selected for processing (13)
bmc/redfish.gocmd/main.gointernal/bmcutils/bmcutils.gointernal/controller/biossettings_controller.gointernal/controller/biosversion_controller.gointernal/controller/bmc_controller.gointernal/controller/bmcsettings_controller.gointernal/controller/bmcuser_controller.gointernal/controller/bmcversion_controller.gointernal/controller/endpoint_controller.gointernal/controller/server_controller.gointernal/controller/server_controller_test.gointernal/controller/suite_test.go
Merged latest main branch (commit 7ef9f11) which included: - Simplified BMCSettings reconciler with renamed constants - Improved error handling and cleanup logic - GitHub Actions dependency updates Fixed CodeRabbit identified issues: - Fixed session leak in bmcConnectionTest() by properly calling defer bmcClient.Logout() after creating BMC client - Added documentation about test coverage limitation for HTTPS + certificate verification (mock server only supports HTTP) All tests passing (102 controller specs, composite coverage: 57.6%)
afritzler
approved these changes
Apr 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #395
This commit addresses a security vulnerability where TLS certificate verification was hardcoded to be disabled (Insecure: true) in the Redfish BMC client, regardless of configuration.
Summary by CodeRabbit
New Features
Deprecated
Improvements