fix(aws): fail instead of falling back to 0.0.0.0/0 for security group#650
Conversation
When public IP auto-detection failed, SSH and K8s API ports were opened to the entire internet. Now fail with a clear error message asking the user to set the CIDR explicitly. Audit finding NVIDIA#34 (LOW). Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
There was a problem hiding this comment.
Pull request overview
This pull request aims to improve security by removing a dangerous fallback to 0.0.0.0/0 when public IP detection fails during AWS security group creation. Instead of silently falling back to allowing traffic from anywhere, the code now returns an error asking users to explicitly set ingressIpRanges.
Changes:
- Removed
0.0.0.0/0fallback when public IP detection fails in AWS security group creation - Changed from warning + fallback to immediate error return
- Updated comment to reflect that only auto-detected IP (not fallback) is added
| publicIP, err := utils.GetIPAddress() | ||
| if err != nil { | ||
| return fmt.Errorf("could not detect public IP for security group (set ingressCidr explicitly): %w", err) | ||
| } |
There was a problem hiding this comment.
This change breaks a valid use case where users provide explicit ingressIpRanges. According to the documentation (docs/guides/ip-detection.md and docs/commands/create.md), when users provide ingressIpRanges, the detected IP should be added in addition to those ranges. However, if IP detection fails (e.g., due to corporate firewall or proxy), the code will now error out even when the user has provided valid ingressIpRanges.
The fix should only fail when BOTH conditions are true: (1) IP detection fails AND (2) ingressIpRanges is empty. Otherwise, the code should continue with the user-provided ranges.
Suggested logic:
- Try to detect public IP
- If detection succeeds, add it to ipRanges
- If detection fails AND len(p.Spec.IngressIpRanges) == 0, return error
- If detection fails BUT user provided ingressIpRanges, log a warning and continue
| p.log.Warning("Could not detect public IP, using 0.0.0.0/0: %v", err) | ||
| publicIP, err := utils.GetIPAddress() | ||
| if err != nil { | ||
| return fmt.Errorf("could not detect public IP for security group (set ingressCidr explicitly): %w", err) |
There was a problem hiding this comment.
The error message references "ingressCidr" but the actual field name in the API is "ingressIpRanges" (as seen in api/holodeck/v1alpha1/types.go line 108). This inconsistency could confuse users. The error message should use the correct field name from the API spec.
| return fmt.Errorf("could not detect public IP for security group (set ingressCidr explicitly): %w", err) | |
| return fmt.Errorf("could not detect public IP for security group (set ingressIpRanges explicitly): %w", err) |
| } else { | ||
| p.log.Warning("Could not detect public IP, using 0.0.0.0/0: %v", err) | ||
| publicIP, err := utils.GetIPAddress() | ||
| if err != nil { |
There was a problem hiding this comment.
Missing cancelLoading call before returning error. Following the codebase pattern (seen throughout this file and in pkg/provider/aws/cluster.go:222, pkg/provider/aws/nlb.go:68, etc.), errors that occur during a loading operation should call cancelLoading(logger.ErrLoadingFailed) before returning. The loading spinner was started at line 326 but is not properly cancelled when IP detection fails.
| if err != nil { | |
| if err != nil { | |
| cancelLoading(logger.ErrLoadingFailed) |
| publicIP, err := utils.GetIPAddress() | ||
| if err != nil { | ||
| return fmt.Errorf("could not detect public IP for security group (set ingressCidr explicitly): %w", err) |
There was a problem hiding this comment.
The PR description references "Audit Finding #34 (LOW)" suggesting a security audit, but the linked PR #34 is about bumping golang.org/x/net dependency, not a security audit of this code. This appears to be an error in the PR description - either the wrong issue/PR was referenced, or the audit finding number is incorrect.
Pull Request Test Coverage Report for Build 21962394536Details
💛 - Coveralls |
Summary
0.0.0.0/0fallback when public IP detection failsingressCidrexplicitlyAudit Finding
0.0.0.0/0fallback inpkg/provider/aws/create.goChanges
pkg/provider/aws/create.go: Replace0.0.0.0/0fallback with error returnTest plan
gofmt— no formatting issuesgo build— compilesgo test ./pkg/...— all tests pass