OCPBUGS-72408: block image import from cluster IP addresses#591
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
WalkthroughAdd DNS-based host resolution and IP-prefix enforcement for image imports: introduce an IPLookup abstraction and ShouldContactHost/ValidateImageStreamImportDisallowedHosts APIs; compute blocked and allowed CIDRs in imagestreamimport REST; enforce policies via a new RestrictedTransport used during import flows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing touches
Comment |
openshift-ci
Bot
added
the
approved
|
/retest |
ricardomaraschini
force-pushed
the
block-image-import-by-ips
branch
2 times, most recently
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.
ricardomaraschini
force-pushed
the
block-image-import-by-ips
branch
from
78a758d to
0298033
Compare
flavianmissi
left a comment
flavianmissi
left a comment
There was a problem hiding this comment.
Left a question/possible nit, but LGTM either way.
/lgtm
|
/retest |
|
/verified by @xiuwang |
|
/retest |
vendor the fake config v1 implementation so we can make the tests simpler on our side.
ricardomaraschini
force-pushed
the
block-image-import-by-ips
branch
from
a52486e to
86b1e56
Compare
ricardomaraschini
force-pushed
the
block-image-import-by-ips
branch
from
bdf0bc5 to
e8eb3b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/image/apis/image/validation/validation.go`:
- Around line 833-848: The loop over resolved IPs (variable ips) currently
returns nil as soon as an allowedPrefix.Contains(ntip) matches, short‑circuiting
validation across other IPs; change that early return to continue so that an
allowed IP only skips further checks for that particular ntip and the loop still
validates the remaining IPs (i.e., replace the return nil inside the
allowedPrefix.Contains(ntip) branch with continue), leaving the rest of the
validation logic intact.
🧹 Nitpick comments (1)
pkg/image/apis/image/validation/validation_test.go (1)
2199-2223: Add a mixed allowed+blocked multi‑IP case
This guards against allowlist short‑circuit if a hostname resolves to both allowed and blocked IPs.🧪 Proposed test case
{ name: "multiple resolved ips - one blocked", registry: "multipleips", blocked: []netip.Prefix{netip.MustParsePrefix("192.168.0.0/24")}, errorstr: "not allowed", }, + { + name: "multiple resolved ips - allowed and blocked", + registry: "multipleips", + blocked: []netip.Prefix{netip.MustParsePrefix("192.168.0.0/24")}, + allowed: []netip.Prefix{netip.MustParsePrefix("2.3.4.5/32")}, + errorstr: "not allowed", + }, { name: "allowed IP bypasses loopback", registry: "localmachine", allowed: []netip.Prefix{netip.MustParsePrefix("127.0.0.1/32")}, },
ricardomaraschini
force-pushed
the
block-image-import-by-ips
branch
from
e8eb3b8 to
1d3f7e3
Compare
ricardomaraschini
force-pushed
the
block-image-import-by-ips
branch
from
855fbe7 to
fc3c56b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/image/apis/image/validation/validation.go`:
- Around line 804-833: ref.Registry may contain bracketed IPv6 literals like
"[2001:db8::1]" which survive the net.SplitHostPort branch and cause
net.ParseIP(host) in ShouldContactHost to fail; update the flow to strip
surrounding brackets before parsing or DNS resolution. Specifically, in
ShouldContactHost (and/or right after extracting host from ref.Registry where
net.SplitHostPort is used) detect if host starts with '[' and ends with ']' and
remove those brackets before calling net.ParseIP(host) or resolve(ctx, host), so
IPv6 literals parse correctly and avoid unnecessary DNS lookups or false blocks.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/image/apiserver/registry/imagestreamimport/transport.go`:
- Around line 31-42: RoundTrip on RestrictedTransport can panic if the
shouldContactHost function is nil (e.g., when not created via
NewRestrictedTransport); update RoundTrip to guard against a nil
shouldContactHost before calling it: check r.shouldContactHost != nil and return
a clear error if it's nil (or fall back to a safe default decision) so the call
in RoundTrip (which currently invokes r.shouldContactHost(req.Context(), host,
r.blocked, r.allowed)) cannot dereference a nil function; ensure this check is
added near the existing nil checks for r.wrapped and req/URL and reference
RestrictedTransport.RoundTrip and NewRestrictedTransport in the change.
🧹 Nitpick comments (2)
pkg/image/apis/image/validation/validation.go (1)
801-816: Avoid brittle error‑string matching forSplitHostPort.
Relying onstrings.Contains(err.Error(), ...)is fragile; useerrors.Aswith*net.AddrErrorinstead.♻️ Proposed refactor
- if !strings.Contains(err.Error(), "missing port in address") { - return fmt.Errorf("invalid registry url: %w", err) - } + var addrErr *net.AddrError + if !errors.As(err, &addrErr) || addrErr.Err != "missing port in address" { + return fmt.Errorf("invalid registry url: %w", err) + }pkg/image/apiserver/registry/imagestreamimport/rest.go (1)
257-272: Prefer structuredSplitHostPorterror checks.
Useerrors.Aswith*net.AddrErrorinstead of string matching to detect the “missing port” case.♻️ Proposed refactor
- if !strings.Contains(err.Error(), "missing port in address") { - return nil, fmt.Errorf("invalid registry url: %w", err) - } + var addrErr *net.AddrError + if !errors.As(err, &addrErr) || addrErr.Err != "missing port in address" { + return nil, fmt.Errorf("invalid registry url: %w", err) + }-import ( - "context" - "fmt" - "net" - "net/http" - "net/netip" - "strings" - "time" -) +import ( + "context" + "errors" + "fmt" + "net" + "net/http" + "net/netip" + "strings" + "time" +)
ricardomaraschini
force-pushed
the
block-image-import-by-ips
branch
2 times, most recently
from
cb66b89 to
8586037
Compare
block image imports that point to cluster IP addresses (pod/service CIDR), localhost, or link-local addresses. this is implemented with two layers of : 1. api validation: before accepting an image stream import object, the registry address is resolved and validated. if it points to blocked addresses (cluster CIDRs, loopback, link-local), a "forbidden" error is returned. 2. http client validation: an additional check at the registry client level prevents contact even if dns entries change between api validation and the actual import operation. internal registry ips are allowed to bypass blocking.
ricardomaraschini
force-pushed
the
block-image-import-by-ips
branch
from
8586037 to
4ced69f
Compare
|
/lgtm |
|
@ricardomaraschini: all tests passed! 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. |
|
/approve |
|
/verified by @xiuwang Test again, no regression. |
openshift-ci-robot
added
the
verified
|
@xiuwang: This PR has been marked as verified by 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. |
| // properly parsing IPv6 addresses and strips out the brackets. | ||
| withPort := fmt.Sprintf("%s:0", imageConfig.Status.InternalRegistryHostname) | ||
| if hostname, _, err = net.SplitHostPort(withPort); err != nil { | ||
| return nil, fmt.Errorf("invalid registry url: %w", err) |
There was a problem hiding this comment.
This might be a confusing message with the :0. Either strip it out of the message or use 443, the actual default port.
There was a problem hiding this comment.
We only end here if the error is because of a missing port so this isn't ever expected to fail. The zero was to demonstrate intent: we do not care about the port.
That being said: setting this to 443 made the LLM reviewer here (code rabbit) ask me to use 0 instead.
|
|
||
| for _, blockedPrefix := range blocked { | ||
| if blockedPrefix.Contains(ntip) { | ||
| return fmt.Errorf("import from %s not allowed", blockedPrefix.String()) |
There was a problem hiding this comment.
Do not expose the blocklist to the user, make this a generic message.
| if ip := net.ParseIP(host); ip != nil { | ||
| ips = append(ips, ip) | ||
| } else { | ||
| ipAddrs, err := resolve(ctx, host) |
There was a problem hiding this comment.
network calls like this should have a timeout
There was a problem hiding this comment.
The whole image stream import is bound to the http request context, including going to the remote registries (network calls) and fetching the needed manifests.
This isn't introducing a new behavior, i.e. the image import process also relies on the http context without timeout. We could put a timeout here but I am not confident I can come up with a value. What do you think ?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flavianmissi, ingvagabund, ricardomaraschini, sanchezl 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
|
@ricardomaraschini: Jira Issue Verification Checks: Jira Issue OCPBUGS-72408 Jira Issue OCPBUGS-72408 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
6 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.