Build v1.11.2 #3
Conversation
WalkthroughThe changes update multiple components of the project. The Changes
Sequence Diagram(s)sequenceDiagram
participant Build as Build Process
participant DF as Dockerfile Script
participant PD as Patches Directory
participant GA as Git Apply
participant EC as NGINX Config Copier
Build->>DF: Start build
DF->>PD: Copy files from ./patches
DF->>GA: Apply all *.diff files in current context
DF->>EC: Copy ./etc/nginx to /etc/nginx (set ownership www-data)
DF->>Build: Build complete with new label
sequenceDiagram
participant TP as TCPProxy (Handle Method)
participant Conn as Connection
participant Log as Logger
TP->>Conn: Read minimum of 5 bytes (TLS header)
Conn-->>TP: Return header data
TP->>Conn: Calculate expected ClientHello size and read full message
Conn-->>TP: Return complete TLS message or error
alt Successful Read
TP->>TP: Process ClientHello message
else Error Occurs
TP->>Log: Log error with context
end
sequenceDiagram
participant Auth as Auth Module
participant VF as File Path Validator
participant Err as Error Handler
Auth->>VF: Construct password file path
VF-->>Auth: Return validation result (check for traversal characters)
alt Valid Path
Auth->>Auth: Continue processing auth configuration
else Invalid Path
Auth->>Err: Return error for invalid file path
end
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
images/controller/Dockerfile (1)
33-33: Include version locking for third-party librariesInstalling
lua-iconv 7-3is fine, but consider specifying explicit pinned versions or adding checks to confirm compatibility with the rest of the system. This helps avoid unexpected breaks when new releases appear.images/controller/patches/13068.diff (1)
53-73: Temporarily disabling template testingCommenting out the template test prevents potential security issues. However, consider tracking the re-enablement in a follow-up task or issue to ensure future thorough testing in a sandboxed environment.
images/controller/patches/11843.diff (2)
38-40: Ensure proper handling of non-TLS connections.Allocating a buffer of size
tlsHeaderLength + tlsMaxMessageLengthis sufficient for valid TLS records, but consider rejecting or handling connections that do not meet the expected handshake content type indata[0]. This helps avoid misinterpretation when the incoming traffic is not actually TLS.
46-48: Validate content type before reading additional bytes.Currently, the code infers the ClientHello length from
data[3]anddata[4], but does not check whetherdata[0]equals a valid TLS record type (e.g., 22 for handshake). Including a sanity check ondata[0]may improve security by rejecting unexpected record types early.images/controller/etc/nginx/template/nginx.tmpl (2)
153-168: Use caution with ModSecurity performance overhead.Enabling
modsecurity on;globally is powerful for security, but can introduce substantial performance overhead. Employ tight rules sets, confirm your rules are up to date, and consider excluding paths that do not require inspection to avoid performance bottlenecks.
1115-1120: Handle trailing slashes in rewrite logic.The snippet that trims a trailing slash (lines 646-649) before redirecting can create edge cases for routes expecting a trailing slash (e.g., linting tools, older internal references). Consider appending the slash if it’s semantically required by the service.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)images/controller/Dockerfile(2 hunks)images/controller/etc/nginx/template/nginx.tmpl(1 hunks)images/controller/patches/11843.diff(1 hunks)images/controller/patches/13068.diff(1 hunks)images/controller/patches/nginx-tmpl.patch(0 hunks)
💤 Files with no reviewable changes (1)
- images/controller/patches/nginx-tmpl.patch
🔇 Additional comments (12)
.gitignore (1)
1-2: Looks Good!Adding
tmpto.gitignoreis a standard practice to avoid clutter from temporary files.images/controller/Dockerfile (3)
8-9: Comprehensive patch application strategyCopying patches into the build context and applying all
.difffiles simplifies the build process, but ensure the patch files won't unintentionally overwrite each other or conflict. This approach looks good given the existing environment.
38-39: File copying changesCopying the controller binary and the entire
etc/nginxdirectory is straightforward. Just confirm that no sensitive files inetc/nginxare accidentally exposed or overwritten.Please ensure that only safe, necessary config files are included in
etc/nginx. If you want me to verify by script, let me know.
41-41: Metadata enhancementAdding the
org.opencontainers.image.sourcelabel is a great step for traceability and maintainability.images/controller/patches/13068.diff (2)
1-53: Secure password file namingThe updated logic using
filepath.Joinand validating normalized path components prevents path traversal attacks, which is crucial for security. Ensure logging calls in case of errors do not leak any sensitive filename details.
74-92: Refined quote handling in directivesRemoving quotes and using
strconv.Quoteensures correct quoting behavior. Double check that no special characters in the upstream hosts or URIs cause injection-like issues.images/controller/patches/11843.diff (2)
9-12: Good addition of constants for clarity.Defining
tlsHeaderLengthandtlsMaxMessageLengthas named constants improves readability and encapsulates critical TLS record parameters in a single location.
40-45: Draw attention to partial reads and incomplete headers.Using
io.ReadAtLeast(conn, data, tlsHeaderLength)ensures that the TLS header bytes are read before proceeding. However, make sure to handle scenarios where partial data is received or the TLS header is malformed (e.g., truncated, zero-length, etc.). Consider adding more robust validation of the received header fields to avoid potential index errors later.images/controller/etc/nginx/template/nginx.tmpl (4)
309-317: Validate performance impact with AIO and threaded I/O.Using
aio threads;and enablingtcp_nopush on; tcp_nodelay on;can significantly impact performance, especially under heavy load. Verify that these settings match your intended workload profile and hardware resources.
449-456: Confirm upgrade logic for WebSocket connections.Mapping
$http_upgradeto$connection_upgradeis standard for WebSocket traffic. However, ensure that ancillary protocols (e.g., HTTP/2 or HTTP/3 upgrades) are not hindered by these settings. In particular, confirm there's no conflict with other modules or used protocols.
621-673: Examine redirect logic for downstream invariants.The per-server redirect (lines 622-673) sets up a 302 to
$redirect.To. Confirm that rewriting paths and ports aligns with any custom logic in your environment (e.g., SSO logins, path-based routing). Mismatched or static port mappings may break behind proxies that override ports.
1539-1547: Consider upstream fallback logic.Using
proxy_intercept_errors on;combined with custom error pages is powerful. However, confirm that a mismatch in error handling from the upstream does not inadvertently mask legitimate client or upstream errors. Regularly test your custom error pages for correctness and user experience.
| + bLength, err := io.ReadAtLeast(conn, data[length:], tlsHeaderLength+clientHelloLength-length) | ||
| + if err != nil { | ||
| + klog.V(4).ErrorS(err, fmt.Sprintf("Error reading ClientHello of length %d from the connection", clientHelloLength)) | ||
| + return | ||
| + } | ||
| + length += bLength | ||
| + |
There was a problem hiding this comment.
Check correctness of subsequent read length.
clientHelloLength might be smaller than the actual handshake message if a malicious client spoofs an intentionally small length in the TLS record. Safely handling any mismatch in the second read (e.g., returning an error if fewer bytes arrive than expected) helps harden the handshake process.
| } | ||
|
|
||
| proxy_cache auth_cache; | ||
|
|
||
| {{- range $dur := $externalAuth.AuthCacheDuration }} | ||
| proxy_cache_valid {{ $dur }}; | ||
| {{- end }} | ||
|
|
||
| proxy_cache_key "$cache_key"; | ||
| {{ end }} | ||
|
|
||
| # ngx_auth_request module overrides variables in the parent request, | ||
| # therefore we have to explicitly set this variable again so that when the parent request | ||
| # resumes it has the correct value set for this variable so that Lua can pick backend correctly | ||
| set $proxy_upstream_name {{ buildUpstreamName $location | quote }}; | ||
|
|
||
| proxy_pass_request_body off; | ||
| proxy_set_header Content-Length ""; | ||
| proxy_set_header X-Forwarded-Proto ""; | ||
| proxy_set_header X-Request-ID $req_id; | ||
|
|
||
| {{ if $externalAuth.Method }} | ||
| proxy_method {{ $externalAuth.Method }}; | ||
| proxy_set_header X-Original-URI $request_uri; | ||
| proxy_set_header X-Scheme $pass_access_scheme; | ||
| {{ end }} | ||
|
|
||
| proxy_set_header Host {{ $externalAuth.Host }}; | ||
| proxy_set_header X-Original-URL $scheme://$http_host$request_uri; | ||
| proxy_set_header X-Original-Method $request_method; | ||
| proxy_set_header X-Sent-From "nginx-ingress-controller"; | ||
| proxy_set_header X-Real-IP $remote_addr; | ||
| {{ if and $all.Cfg.UseForwardedHeaders $all.Cfg.ComputeFullForwardedFor }} | ||
| proxy_set_header X-Forwarded-For $full_x_forwarded_for; | ||
| {{ else }} | ||
| proxy_set_header X-Forwarded-For $remote_addr; | ||
| {{ end }} | ||
|
|
||
| {{ if $externalAuth.RequestRedirect }} | ||
| proxy_set_header X-Auth-Request-Redirect {{ $externalAuth.RequestRedirect }}; | ||
| {{ else }} | ||
| proxy_set_header X-Auth-Request-Redirect $request_uri; | ||
| {{ end }} | ||
|
|
||
| {{ if not (contains $externalAuth.AuthSnippet "proxy_connect_timeout") }} | ||
| proxy_connect_timeout 15s; | ||
| {{ end }} | ||
|
|
||
| {{ if $externalAuth.AuthCacheKey }} | ||
| proxy_buffering "on"; | ||
| {{ else }} | ||
| proxy_buffering {{ $location.Proxy.ProxyBuffering }}; | ||
| {{ end }} | ||
| proxy_buffer_size {{ $location.Proxy.BufferSize }}; | ||
| proxy_buffers {{ $location.Proxy.BuffersNumber }} {{ $location.Proxy.BufferSize }}; | ||
| proxy_request_buffering {{ $location.Proxy.RequestBuffering }}; | ||
|
|
||
| proxy_ssl_server_name on; | ||
| proxy_pass_request_headers on; | ||
| {{ if isValidByteSize $location.Proxy.BodySize true }} | ||
| client_max_body_size {{ $location.Proxy.BodySize }}; | ||
| {{ end }} | ||
| {{ if isValidByteSize $location.ClientBodyBufferSize false }} | ||
| client_body_buffer_size {{ $location.ClientBodyBufferSize }}; | ||
| {{ end }} | ||
|
|
||
| # Pass the extracted client certificate to the auth provider | ||
| {{ if not (empty $server.CertificateAuth.CAFileName) }} | ||
| {{ if $server.CertificateAuth.PassCertToUpstream }} | ||
| proxy_set_header ssl-client-cert $ssl_client_escaped_cert; | ||
| {{ end }} | ||
| proxy_set_header ssl-client-verify $ssl_client_verify; | ||
| proxy_set_header ssl-client-subject-dn $ssl_client_s_dn; | ||
| proxy_set_header ssl-client-issuer-dn $ssl_client_i_dn; | ||
| {{ end }} | ||
|
|
||
| {{- range $line := buildAuthProxySetHeaders $externalAuth.ProxySetHeaders}} | ||
| {{ $line }} | ||
| {{- end }} | ||
|
|
||
| {{ if not (empty $externalAuth.AuthSnippet) }} | ||
| {{ $externalAuth.AuthSnippet }} | ||
| {{ end }} | ||
|
|
||
| {{ if and (eq $applyAuthUpstream true) (eq $applyGlobalAuth false) }} | ||
| {{ $authUpstreamName := buildAuthUpstreamName $location $server.Hostname }} | ||
| # The target is an upstream with HTTP keepalive, that is why the | ||
| # Connection header is cleared and the HTTP version is set to 1.1 as | ||
| # the Nginx documentation suggests: | ||
| # http://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive | ||
| proxy_http_version 1.1; | ||
| proxy_set_header Connection ""; | ||
| set $target {{ changeHostPort $externalAuth.URL $authUpstreamName }}; | ||
| {{ else }} | ||
| proxy_http_version {{ $location.Proxy.ProxyHTTPVersion }}; | ||
| set $target {{ $externalAuth.URL | quote }}; | ||
| {{ end }} | ||
| proxy_pass $target; | ||
| } | ||
| {{ end }} |
There was a problem hiding this comment.
Check for potential double-escape or injection with external auth.
When sending $request_uri and passing X-Original-Method, X-Original-URL, and other headers to the external auth endpoint, carefully ensure they are sanitized or validated. This helps prevent header smuggling or injection attacks if the external auth system does not sanitize inputs.
Signed-off-by: Timofei Larkin <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
images/controller/etc/nginx/template/nginx.tmpl (2)
203-252: Consolidate duplicated GeoIP2 City database configurationsSimilar to the Country database configurations, there's duplication between GeoLite2-City and GeoIP2-City databases with identical variable declarations.
Consider consolidating these duplicated blocks:
- {{ if eq $file "GeoLite2-City.mmdb" }} - geoip2 /etc/ingress-controller/geoip/GeoLite2-City.mmdb { - {{ if (gt $cfg.GeoIP2AutoReloadMinutes 0) }} - auto_reload {{ $cfg.GeoIP2AutoReloadMinutes }}m; - {{ end }} - $geoip2_city_country_code source=$remote_addr country iso_code; - # ... rest of variables - } - {{ end }} - - {{ if eq $file "GeoIP2-City.mmdb" }} - geoip2 /etc/ingress-controller/geoip/GeoIP2-City.mmdb { - {{ if (gt $cfg.GeoIP2AutoReloadMinutes 0) }} - auto_reload {{ $cfg.GeoIP2AutoReloadMinutes }}m; - {{ end }} - $geoip2_city_country_code source=$remote_addr country iso_code; - # ... rest of variables - } - {{ end }} + {{ if or (eq $file "GeoLite2-City.mmdb") (eq $file "GeoIP2-City.mmdb") }} + geoip2 /etc/ingress-controller/geoip/{{ $file }} { + {{ if (gt $cfg.GeoIP2AutoReloadMinutes 0) }} + auto_reload {{ $cfg.GeoIP2AutoReloadMinutes }}m; + {{ end }} + $geoip2_city_country_code source=$remote_addr country iso_code; + # ... rest of variables + } + {{ end }}
255-272: Consolidate duplicated GeoIP2 ASN database configurationsThere's duplication between GeoLite2-ASN and GeoIP2-ASN database configurations with identical variable declarations.
Consider consolidating these duplicated blocks:
- {{ if eq $file "GeoLite2-ASN.mmdb" }} - geoip2 /etc/ingress-controller/geoip/GeoLite2-ASN.mmdb { - {{ if (gt $cfg.GeoIP2AutoReloadMinutes 0) }} - auto_reload {{ $cfg.GeoIP2AutoReloadMinutes }}m; - {{ end }} - $geoip2_asn source=$remote_addr autonomous_system_number; - $geoip2_org source=$remote_addr autonomous_system_organization; - } - {{ end }} - - {{ if eq $file "GeoIP2-ASN.mmdb" }} - geoip2 /etc/ingress-controller/geoip/GeoIP2-ASN.mmdb { - {{ if (gt $cfg.GeoIP2AutoReloadMinutes 0) }} - auto_reload {{ $cfg.GeoIP2AutoReloadMinutes }}m; - {{ end }} - $geoip2_asn source=$remote_addr autonomous_system_number; - $geoip2_org source=$remote_addr autonomous_system_organization; - } - {{ end }} + {{ if or (eq $file "GeoLite2-ASN.mmdb") (eq $file "GeoIP2-ASN.mmdb") }} + geoip2 /etc/ingress-controller/geoip/{{ $file }} { + {{ if (gt $cfg.GeoIP2AutoReloadMinutes 0) }} + auto_reload {{ $cfg.GeoIP2AutoReloadMinutes }}m; + {{ end }} + $geoip2_asn source=$remote_addr autonomous_system_number; + $geoip2_org source=$remote_addr autonomous_system_organization; + } + {{ end }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
images/controller/Dockerfile(2 hunks)images/controller/PROVENANCE.md(1 hunks)images/controller/etc/nginx/template/nginx.tmpl(1 hunks)images/controller/patches/13068.diff(1 hunks)images/controller/patches/nginx-tmpl.patch(0 hunks)
💤 Files with no reviewable changes (1)
- images/controller/patches/nginx-tmpl.patch
✅ Files skipped from review due to trivial changes (1)
- images/controller/PROVENANCE.md
🚧 Files skipped from review as they are similar to previous changes (2)
- images/controller/patches/13068.diff
- images/controller/Dockerfile
🔇 Additional comments (7)
images/controller/etc/nginx/template/nginx.tmpl (7)
174-200: Consolidate repeated GeoIP database configurationsThis code contains nearly identical blocks for "GeoLite2-Country.mmdb" and "GeoIP2-Country.mmdb" databases with the same variable declarations. Similar duplications exist for City and ASN database configurations as well.
Consider consolidating these duplicated blocks to improve maintainability:
- {{ if eq $file "GeoLite2-Country.mmdb" }} - geoip2 /etc/ingress-controller/geoip/GeoLite2-Country.mmdb { - {{ if (gt $cfg.GeoIP2AutoReloadMinutes 0) }} - auto_reload {{ $cfg.GeoIP2AutoReloadMinutes }}m; - {{ end }} - $geoip2_country_code source=$remote_addr country iso_code; - $geoip2_country_name source=$remote_addr country names en; - $geoip2_country_geoname_id source=$remote_addr country geoname_id; - $geoip2_continent_code source=$remote_addr continent code; - $geoip2_continent_name source=$remote_addr continent names en; - $geoip2_continent_geoname_id source=$remote_addr continent geoname_id; - } - {{ end }} - - {{ if eq $file "GeoIP2-Country.mmdb" }} - geoip2 /etc/ingress-controller/geoip/GeoIP2-Country.mmdb { - {{ if (gt $cfg.GeoIP2AutoReloadMinutes 0) }} - auto_reload {{ $cfg.GeoIP2AutoReloadMinutes }}m; - {{ end }} - $geoip2_country_code source=$remote_addr country iso_code; - $geoip2_country_name source=$remote_addr country names en; - $geoip2_country_geoname_id source=$remote_addr country geoname_id; - $geoip2_continent_code source=$remote_addr continent code; - $geoip2_continent_name source=$remote_addr continent names en; - $geoip2_continent_geoname_id source=$remote_addr continent geoname_id; - } - {{ end }} + {{ if or (eq $file "GeoLite2-Country.mmdb") (eq $file "GeoIP2-Country.mmdb") }} + geoip2 /etc/ingress-controller/geoip/{{ $file }} { + {{ if (gt $cfg.GeoIP2AutoReloadMinutes 0) }} + auto_reload {{ $cfg.GeoIP2AutoReloadMinutes }}m; + {{ end }} + $geoip2_country_code source=$remote_addr country iso_code; + $geoip2_country_name source=$remote_addr country names en; + $geoip2_country_geoname_id source=$remote_addr country geoname_id; + $geoip2_continent_code source=$remote_addr continent code; + $geoip2_continent_name source=$remote_addr continent names en; + $geoip2_continent_geoname_id source=$remote_addr continent geoname_id; + } + {{ end }}Apply similar consolidation for the City and ASN database configurations as well.
1121-1244: Check for potential double-escape or injection with external auth.When sending
$request_uriand passingX-Original-Method,X-Original-URL, and other headers to the external auth endpoint, carefully ensure they are sanitized or validated. This helps prevent header smuggling or injection attacks if the external auth system does not sanitize inputs.
1-13: LGTM - Variable declarations and basic configurationThe variable declarations at the top of the template and the basic NGINX configuration settings (checksum, pid, pcre_jit) are well-structured.
18-37: LGTM - Conditional module loadingThe conditional module loading based on configuration flags is a good practice for optimizing resource usage - only loading modules when they are needed.
39-63: LGTM - Worker and event configurationThe worker process configuration and events block are well-configured with appropriate parameters for tuning performance.
74-139: LGTM - Lua initialization with proper error handlingThe Lua initialization block has proper error handling using pcall and comprehensive error reporting.
549-573: LGTM - Dynamic upstream balancer configurationThe upstream balancer configuration with Lua integration is well-designed for dynamic backend management.
Summary by CodeRabbit