Remove client IP from HTTP boot logic

[maxmoehl]

Putting the client IP first in the list of client identifiers to check for boot configuration has the problem that the client is usually FeDHCP. If it uses an IP address of a management server for which a boot configuration exists only that boot configuration will be served for every single boot request.

Adjust the logic to ignore the clients IP address to rely exclusively on the X-Forwarded-For header to resolving the boot configuration.

Resolves: #290

Summary by CodeRabbit

• Bug Fixes
  • Updated client IP detection logic; client IP information may not be available in certain network configurations.

[coderabbitai]

Walkthrough

The HTTP boot handler's client IP extraction logic was modified to exclusively rely on the X-Forwarded-For header for client IP identification, removing direct RemoteAddr parsing. This prevents intermediate proxies like FeDHCP from being incorrectly matched as the client endpoint.

Changes

Cohort / File(s)	Summary
HTTP Boot Handler IP Logic server/bootserver.go	Removed net.SplitHostPort parsing of RemoteAddr and associated error handling; clientIPs is now populated only from X-Forwarded-For header. When the header is absent or empty, clientIPs remains empty.
Boot Server Tests server/bootserver_test.go	Updated test assertion for the /httpboot endpoint to expect empty ClientIPs in the default/no-match scenario, aligning with the new IP extraction behavior.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Remove client IP from HTTP boot logic' clearly and concisely describes the main change: removing client IP from HTTP boot logic to rely exclusively on X-Forwarded-For header.
Description check	✅ Passed	The description explains the problem (FeDHCP clients being NATed to management server IP), the solution (ignoring client IP and relying on X-Forwarded-For), and includes the resolved issue reference.
Linked Issues check	✅ Passed	The code changes directly address the requirement in issue #290: client IP (RemoteAddr) is removed from boot configuration lookup identifiers and only X-Forwarded-For header is used.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to the stated objective: removing RemoteAddr-based client IP extraction and using only X-Forwarded-For header for boot configuration resolution.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch maxmoehl/ignore-client-ip-httpboot

📝 Coding Plan

• Generate coding plan for human review comments

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

server/bootserver.go (1)

378-399: ⚠️ Potential issue | 🟠 Major

Use only the originating XFF client entry for matching.

At Line 389, iterating all X-Forwarded-For entries can still match an intermediate proxy/management IP and return the wrong boot config. This should match only the original client value (or a trusted-hop strategy), not every hop.

Proposed fix

-	if xff := r.Header.Get("X-Forwarded-For"); xff != "" {
-		for _, ip := range strings.Split(xff, ",") {
-			trimmedIP := strings.TrimSpace(ip)
-			if trimmedIP != "" {
-				clientIPs = append(clientIPs, trimmedIP)
-			}
-		}
+	if xff := strings.TrimSpace(r.Header.Get("X-Forwarded-For")); xff != "" {
+		// XFF format: client, proxy1, proxy2...
+		// Match only the originating client IP.
+		if first := strings.TrimSpace(strings.SplitN(xff, ",", 2)[0]); first != "" {
+			clientIPs = append(clientIPs, first)
+		}
 		log.Info("X-Forwarded-For Header Found in the Request", "method", r.Method, "path", r.URL.Path, "clientIPs", clientIPs)
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/bootserver.go` around lines 378 - 399, The code currently splits
X-Forwarded-For into clientIPs and iterates every hop when listing
HTTPBootConfig causing possible matches on intermediate proxies; change the XFF
handling in bootserver.go so you extract only the originating client IP (the
first non-empty left-most trimmed entry from X-Forwarded-For) and use that
single IP for the k8sClient.List call (and for logging) instead of iterating
clientIPs; if you need a trusted-hop strategy, replace the single-IP selection
with that logic but ensure only one IP is used for MatchingFields lookup in the
loop that calls k8sClient.List and for the "Found/Failed to list HTTPBootConfig"
logs.

🧹 Nitpick comments (1)

server/bootserver_test.go (1)

27-48: Add a direct regression test for XFF-based matching behavior.

This case (Line 44) validates the no-header default path, but it does not prove the fix path: RemoteAddr ignored and X-Forwarded-For client used. Please add a test with a non-matching RemoteAddr plus matching XFF value (ideally multi-hop XFF) to lock in the intended behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/bootserver_test.go` around lines 27 - 48, Add a new test It(...)
alongside the existing "delivers default httpboot..." case that sends an HTTP
request to testServerURL+"/httpboot" with a non-matching RemoteAddr scenario but
with an X-Forwarded-For header whose first (client) IP matches an existing
HTTPBootConfig; use a multi-hop XFF value (e.g. "clientIP, proxy1, proxy2") and
perform the request via http.NewRequest + client.Do, then decode into
httpBootResponse and assert the returned UKIURL equals the expected matching
config URL (not defaultUKIURL), that ClientIPs contains the parsed XFF entries,
and that SystemUUID is set when the matching HTTPBootConfig provides one;
reference httpBootResponse, testServerURL, defaultUKIURL and the HTTPBootConfig
matching behavior when implementing.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@server/bootserver.go`:
- Around line 378-399: The code currently splits X-Forwarded-For into clientIPs
and iterates every hop when listing HTTPBootConfig causing possible matches on
intermediate proxies; change the XFF handling in bootserver.go so you extract
only the originating client IP (the first non-empty left-most trimmed entry from
X-Forwarded-For) and use that single IP for the k8sClient.List call (and for
logging) instead of iterating clientIPs; if you need a trusted-hop strategy,
replace the single-IP selection with that logic but ensure only one IP is used
for MatchingFields lookup in the loop that calls k8sClient.List and for the
"Found/Failed to list HTTPBootConfig" logs.

---

Nitpick comments:
In `@server/bootserver_test.go`:
- Around line 27-48: Add a new test It(...) alongside the existing "delivers
default httpboot..." case that sends an HTTP request to
testServerURL+"/httpboot" with a non-matching RemoteAddr scenario but with an
X-Forwarded-For header whose first (client) IP matches an existing
HTTPBootConfig; use a multi-hop XFF value (e.g. "clientIP, proxy1, proxy2") and
perform the request via http.NewRequest + client.Do, then decode into
httpBootResponse and assert the returned UKIURL equals the expected matching
config URL (not defaultUKIURL), that ClientIPs contains the parsed XFF entries,
and that SystemUUID is set when the matching HTTPBootConfig provides one;
reference httpBootResponse, testServerURL, defaultUKIURL and the HTTPBootConfig
matching behavior when implementing.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1826cd84-dec7-42c0-a246-8c214914cbc9

📥 Commits

Reviewing files that changed from the base of the PR and between ebd31b0 and c030d5b.

📒 Files selected for processing (2)

• server/bootserver.go
• server/bootserver_test.go