OCPBUGS-72408: block image import from cluster IP addresses #591
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
WalkthroughAdds DNS-resolution helpers and IP-prefix checks to image import validation and request transport: resolves registry hosts, blocks loopback/link-local and cluster/service/pod CIDRs, validates ImageStreamImport targets pre-contact, and enforces restrictions via a RestrictedTransport used during imports. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (5)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (4)pkg/image/apiserver/registry/imagestreamimport/transport.go (1)
pkg/image/apiserver/registry/imagestreamimport/rest.go (3)
pkg/image/apis/image/validation/validation.go (1)
pkg/image/apiserver/registry/imagestreamimport/transport_test.go (2)
🔇 Additional comments (11)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ricardomaraschini The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/retest |
ricardomaraschini
force-pushed
the
block-image-import-by-ips
branch
from
a325b3c to
dd7a0d8
Compare
before accepting an image stream import object we need to validate it does not point to the cluster itself (pod or service cidr) as well as validate it does not try to reach localhost or link-local addresses. this commit adds a validation to the api that resolves the registry address and if it lands in one of these places a "forbidden" error is returned to the client.
ricardomaraschini
force-pushed
the
block-image-import-by-ips
branch
from
dd7a0d8 to
96d4e39
Compare
ricardomaraschini
changed the title
openshift-ci-robot
added
jira/severity-important
|
@ricardomaraschini: This pull request references Jira Issue OCPBUGS-72408, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-bug
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/assign @flavianmissi |
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/image/apis/image/validation/validation.go (1)
814-814: Fix typo in comment.Line 814 has a typo: "otherwhise" should be "otherwise".
📝 Proposed fix
- // either we have a name or an IP address here. if we manage to parse - // it as an IP then use it as so otherwhise we need to do a dns lookup. + // either we have a name or an IP address here. if we manage to parse + // it as an IP then use it as so otherwise we need to do a dns lookup.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
pkg/image/apis/image/validation/validation.gopkg/image/apis/image/validation/validation_test.gopkg/image/apiserver/registry/imagestreamimport/rest.gopkg/image/apiserver/registry/imagestreamimport/rest_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/image/apis/image/validation/validation.gopkg/image/apiserver/registry/imagestreamimport/rest.gopkg/image/apis/image/validation/validation_test.gopkg/image/apiserver/registry/imagestreamimport/rest_test.go
🧬 Code graph analysis (4)
pkg/image/apis/image/validation/validation.go (1)
pkg/image/apis/image/types.go (2)
DockerImageReference(518-518)ImageStreamImport(574-584)
pkg/image/apiserver/registry/imagestreamimport/rest.go (1)
pkg/image/apis/image/validation/validation.go (1)
ValidateImageStreamImportDisallowedHosts(858-892)
pkg/image/apis/image/validation/validation_test.go (2)
pkg/image/apis/image/types.go (5)
DockerImageReference(518-518)ImageStreamImport(574-584)ImageStreamImportSpec(587-596)RepositoryImportSpec(609-616)ImageImportSpec(630-637)pkg/image/apis/image/validation/validation.go (1)
ValidateImageStreamImportDisallowedHosts(858-892)
pkg/image/apiserver/registry/imagestreamimport/rest_test.go (1)
pkg/image/apiserver/registry/imagestreamimport/rest.go (1)
REST(52-67)
🔇 Additional comments (7)
pkg/image/apiserver/registry/imagestreamimport/rest_test.go (2)
601-660: LGTM! Well-designed test fakes.The fake implementations provide a focused test double for the Network config client. The approach of returning "not implemented" for unused methods while fully implementing only the
Getmethod is appropriate for these unit tests.
661-802: LGTM! Comprehensive test coverage.The test cases cover the essential scenarios for CIDR extraction:
- Valid IPv4 and IPv6 service and pod CIDRs
- Partial configurations (only service or only cluster CIDRs)
- Empty configurations
- Error handling when network config is unavailable
- Invalid CIDR parsing
pkg/image/apis/image/validation/validation.go (1)
855-892: LGTM! Solid validation implementation.The function correctly validates both repository and per-image imports, leveraging the
shouldContactRegistryhelper to enforce security policies. The error messages clearly indicate which imports are blocked and why.pkg/image/apiserver/registry/imagestreamimport/rest.go (2)
201-230: LGTM! Secure fail-closed implementation.The method correctly retrieves both service and pod CIDRs from the cluster Network configuration. The fail-closed behavior (returning an error when the Network config is unavailable) is the right security posture, as it prevents imports when the system can't determine which addresses should be blocked.
349-358: LGTM! Proper security validation integration.The validation is correctly placed in the request flow after authorization checks but before the actual import operation. The fail-closed behavior ensures that imports are rejected if the security policy cannot be determined, which is the correct security posture.
pkg/image/apis/image/validation/validation_test.go (2)
2058-2232: LGTM! Excellent test coverage.The test suite comprehensively validates the registry security checks:
- Loopback addresses (IPv4, IPv6, IPv4-mapped IPv6)
- Link-local addresses (including metadata endpoint)
- Custom CIDR blocks
- DNS resolution scenarios
- Multiple IPs from a single hostname
The custom resolver ensures deterministic test behavior while exercising the DNS resolution code path.
2234-2454: LGTM! Thorough validation testing.The tests comprehensively cover the ImageStreamImport validation scenarios:
- Repository vs. individual image imports
- Multiple images with mixed valid/invalid registries
- Custom CIDR restrictions
- Non-DockerImage kinds (correctly ignored)
- IP addresses with and without ports
- Both repository and image blocked simultaneously
The temporary override of
defaultIPLookupwith proper cleanup is a good testing pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/image/apiserver/registry/imagestreamimport/rest.go (1)
349-409: Critical: auth-fallback retry bypasses restricted transportsThe retry path (401 fallback) builds
importCtxwithr.transport/r.insecureTransport(unrestricted), so a request that passes initial validation can still reach blocked IP ranges during the retry, defeating the “transport-level” enforcement.Proposed fix
transport := NewRestrictedTransport(r.transport, blockedCIDRs) itransport := NewRestrictedTransport(r.insecureTransport, blockedCIDRs) @@ if importFailed { importCtx := importer.NewStaticCredentialsContext( - r.transport, r.insecureTransport, nil, + transport, itransport, nil, ) imports := r.importFn(importCtx, v2regConf) if err := imports.Import(ctx, isi, stream); err != nil { return nil, kapierrors.NewInternalError(err) } }
🤖 Fix all issues with AI agents
In @pkg/image/apiserver/registry/imagestreamimport/transport.go:
- Around line 14-41: RoundTrip must guard against nil inputs to avoid panics: at
the start of RestrictedTransport.RoundTrip check for nil req and nil req.URL and
return a descriptive error (instead of panicking), and ensure r and r.wrapped
are non-nil before calling r.wrapped.RoundTrip (either return an error or fall
back to http.DefaultTransport); also update NewRestrictedTransport to
document/handle a nil wrapped (e.g., set wrapped to http.DefaultTransport when
nil) so RestrictedTransport.wrapped is never nil.
🧹 Nitpick comments (3)
pkg/image/apiserver/registry/imagestreamimport/transport_test.go (1)
16-44: MakemockRoundTripperresponses more HTTP-client compatible (setResponse.Request)For configured responses (especially redirects), setting
resp.Request = reqmakes the mock closer to real transports and reduces brittleness if the stdlib behavior changes.Proposed tweak
func (m *mockRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { @@ resp := m.responses[m.callCount] m.callCount++ + if resp.Request == nil { + resp.Request = req + } + if resp.Body == nil { + resp.Body = http.NoBody + } return resp, nil }Also applies to: 65-68, 365-399
pkg/image/apis/image/validation/validation_test.go (1)
2058-2459: GlobalDefaultIPLookupoverrides are not parallel-safe; considert.Cleanupand order-insensitive assertionsIf any future tests in this package call
t.Parallel(), these global overrides can race. Also,TestValidateImageStreamImportDisallowedHostscurrently assumes a stable error order.pkg/image/apis/image/validation/validation.go (1)
818-858: Optional: include resolved IP/host in “not allowed” error to aid debuggingRight now the extra-CIDR block returns
import from <cidr> not allowed; including the resolved IP (and/or host) would make support cases easier without changing behavior.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
pkg/image/apis/image/validation/validation.gopkg/image/apis/image/validation/validation_test.gopkg/image/apiserver/registry/imagestreamimport/rest.gopkg/image/apiserver/registry/imagestreamimport/transport.gopkg/image/apiserver/registry/imagestreamimport/transport_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/image/apiserver/registry/imagestreamimport/rest.gopkg/image/apis/image/validation/validation.gopkg/image/apis/image/validation/validation_test.gopkg/image/apiserver/registry/imagestreamimport/transport_test.gopkg/image/apiserver/registry/imagestreamimport/transport.go
🧬 Code graph analysis (2)
pkg/image/apis/image/validation/validation.go (1)
pkg/image/apis/image/types.go (2)
DockerImageReference(518-518)ImageStreamImport(574-584)
pkg/image/apiserver/registry/imagestreamimport/transport_test.go (2)
pkg/image/apis/image/validation/validation.go (1)
IPLookup(790-790)pkg/image/apiserver/registry/imagestreamimport/transport.go (1)
NewRestrictedTransport(36-41)
🔇 Additional comments (2)
pkg/image/apis/image/validation/validation.go (1)
788-898: Core disallowed-host validation + fail-closed DNS behavior looks correctThe split between (1) validating registry hosts and (2) enforcing again at transport level aligns well with the TOCTOU risk described in the PR.
pkg/image/apiserver/registry/imagestreamimport/rest.go (1)
78-105: Signature change is properly wired through call sitesThe sole call site at
./pkg/image/apiserver/apiserver.go:271has been updated to pass all 13 parameters including the newconfigV1Clientparameter (passed asconfigV1Client.ConfigV1()). The parameter ordering matches the new function signature.
ricardomaraschini
force-pushed
the
block-image-import-by-ips
branch
from
d487af1 to
78a758d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/image/apiserver/registry/imagestreamimport/transport_test.go (1)
16-43: LGTM! Consider enhancing the error message for better debugging.The mock implementation is correct and follows standard testing patterns. The logic properly handles configured responses, errors, and default cases.
💡 Optional: Make the error message more informative
Consider including the call count in the error message to help with debugging:
if len(m.responses) > 0 { if m.callCount >= len(m.responses) { - return nil, errors.New("mock: unexpected call to RoundTrip") + return nil, fmt.Errorf("mock: unexpected call to RoundTrip (call %d, but only %d responses configured)", m.callCount+1, len(m.responses)) } resp := m.responses[m.callCount] m.callCount++
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
pkg/image/apis/image/validation/validation.gopkg/image/apis/image/validation/validation_test.gopkg/image/apiserver/registry/imagestreamimport/rest.gopkg/image/apiserver/registry/imagestreamimport/transport.gopkg/image/apiserver/registry/imagestreamimport/transport_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/image/apiserver/registry/imagestreamimport/transport.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/image/apiserver/registry/imagestreamimport/transport_test.gopkg/image/apis/image/validation/validation_test.gopkg/image/apis/image/validation/validation.gopkg/image/apiserver/registry/imagestreamimport/rest.go
🧬 Code graph analysis (3)
pkg/image/apiserver/registry/imagestreamimport/transport_test.go (2)
pkg/image/apis/image/validation/validation.go (1)
IPLookup(790-790)pkg/image/apiserver/registry/imagestreamimport/transport.go (1)
NewRestrictedTransport(46-51)
pkg/image/apis/image/validation/validation_test.go (2)
pkg/image/apis/image/validation/validation.go (1)
ValidateImageStreamImportDisallowedHosts(864-898)pkg/image/apis/image/types.go (5)
DockerImageReference(518-518)ImageStreamImport(574-584)ImageStreamImportSpec(587-596)RepositoryImportSpec(609-616)ImageImportSpec(630-637)
pkg/image/apis/image/validation/validation.go (1)
pkg/image/apis/image/types.go (2)
DockerImageReference(518-518)ImageStreamImport(574-584)
🔇 Additional comments (16)
pkg/image/apiserver/registry/imagestreamimport/transport_test.go (3)
45-63: LGTM!The mock DNS resolver implementation is clean and appropriate for testing. It properly validates test data and returns meaningful errors for unknown hosts.
65-81: LGTM!Good test setup with proper cleanup of global state. The use of
deferto restore the originaldefaultIPLookupensures test isolation.
82-400: Excellent test coverage!The test cases are comprehensive and cover a wide range of scenarios including:
- Loopback addresses (IPv4, IPv6, IPv4-mapped)
- Link-local addresses
- Custom CIDR blocking
- Multiple IP resolution
- DNS errors
- Redirect chains
- Error propagation
The test structure is clean and maintainable with good use of table-driven testing.
pkg/image/apis/image/validation/validation_test.go (3)
7-7: LGTM!The new imports are necessary and appropriate for the added functionality:
net/netipfor CIDR prefix handlingimagereffor parsing Docker image referencesAlso applies to: 19-19
2058-2237: LGTM!The test function is well-structured with:
- Proper cleanup of global state via defer
- Comprehensive mock DNS setup with different hostnames
- Good coverage of edge cases (loopback, link-local, CIDRs)
- Clear test case naming and organization
- Appropriate error message validation
The test effectively validates the
shouldContactRegistryfunction's behavior across various scenarios.
2239-2459: LGTM!The test provides comprehensive coverage of
ValidateImageStreamImportDisallowedHosts:
- Both repository and image imports are tested
- Multiple images with mixed safe/unsafe hosts
- Custom CIDR restrictions
- Non-DockerImage kinds (correctly ignored)
- IP addresses with and without ports
The error validation properly checks both the error count and message content, ensuring the validation function works correctly.
pkg/image/apiserver/registry/imagestreamimport/rest.go (5)
7-7: LGTM!The addition of
net/netipimport andnetCfgV1Clientfield are necessary for the new CIDR-based blocking functionality.Also applies to: 66-66
349-359: LGTM! Excellent fail-closed security posture.The code correctly retrieves blocked CIDRs and validates the import request before proceeding. The fail-closed behavior (returning an error if network configuration cannot be retrieved) is the right security choice, preventing imports when the system cannot determine what should be blocked.
378-382: LGTM!The restricted transports are correctly created and used for both secure and insecure connections. This ensures that all image import HTTP requests respect the CIDR blocking rules.
201-230: The review comment suggests adding error handling when bothServiceNetworkandClusterNetworkare empty. However, the existing test suite includes an explicit test case titled "empty network config" that expects this scenario to succeed without error, returning an empty prefix list. This behavior is intentional and correct—empty network configurations are valid in OpenShift clusters. The suggestion to return an error when both slices are empty would contradict the tested and designed behavior.Likely an incorrect or invalid review comment.
87-87: No action needed. TheNewRESTfunction signature already usesconfigV1Client configclientv1.ConfigV1Interface(line 87), and the single caller inpkg/image/apiserver/apiserver.go(line 284) correctly passesconfigV1Client.ConfigV1(). BothimageCfgV1ClientandnetCfgV1Clientfields are properly initialized from the same parameter. The design is sound and all callers are updated.Likely an incorrect or invalid review comment.
pkg/image/apis/image/validation/validation.go (5)
6-6: LGTM!The new imports are necessary for the added validation functionality:
errorsfor error creationnet/netipfor IP prefix/CIDR handlingAlso applies to: 9-9
788-794: LGTM!Excellent design pattern for testability. The
IPLookuptype andDefaultIPLookupvariable allow tests to inject mock DNS resolvers while production code uses the real resolver.
796-813: LGTM!The function correctly handles registry validation:
- Allows empty registry (uses default)
- Properly extracts host from "host:port" format
- Falls back to the whole string if no port is present
- Delegates to
ShouldContactHostfor the actual validation
815-859: LGTM! Robust IP validation with proper IPv4-mapped IPv6 handling.The function correctly:
- Parses direct IP addresses or resolves hostnames via DNS
- Blocks loopback addresses
- Blocks link-local addresses
- Checks against custom CIDR blocks
- Unmaps IPv4-mapped IPv6 addresses (line 850) before CIDR checks, which is crucial for correct comparison
The error messages are clear and indicate the reason for blocking.
861-898: LGTM!The validation function is well-implemented:
- Validates both repository and individual image imports
- Only processes DockerImage kinds (correctly ignoring ImageStreamTag, etc.)
- Provides clear error messages indicating which import was blocked and why
- Uses appropriate field error types (Forbidden for blocked imports)
- Logs blocked imports at debug level for troubleshooting
The function integrates cleanly with the Kubernetes validation framework.
more than blocking image stream import creations when they point to ips we know shouldn't be contacted we also need to block at the registry client level. this adds an extra layer of security on the unlikely case the dns entries change between the api validation and the actual import.
ricardomaraschini
force-pushed
the
block-image-import-by-ips
branch
from
78a758d to
0298033
Compare
|
@ricardomaraschini: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
This PR adds validation to block image imports from
localhost,link-localaddresses (169.254.0.0/16), and cluster service/pod CIDR ranges. The implementation fetches service and pod CIDRs from the cluster Network configuration, performs DNS resolution to validate hostnames, and fails closed if the network config is unavailable.