Extract early config validation from runServeCmd into testable helpers

[raju249]

Extracts early config-validation logic out of runServeCmd and puts it on the relevant config types in server/config/, following the existing pattern used by ConditionalAccessConfig.Validate(initFatal) and AndroidAgentConfig.Validate(initFatal). (First commit on this branch did the extraction into a separate file in cmd/fleet/; reshaped per review.)

runServeCmd is now a series of config.X.Validate(initFatal) calls:

• config.Logging.Validate(initFatal) — OTEL logs requires tracing enabled
• config.Osquery.Validate(initFatal) — host_identifier must be one of provided, instance, uuid, hostname
• config.Server.NormalizeURLPrefix() + config.Server.ValidateURLPrefix(initFatal) — Normalize mutates, ValidateURLPrefix is pure
• config.Server.Validate(initFatal) — private_key vs private_key_arn mutex check (called before Secrets Manager retrieval so a misconfig fails fast without paying for an external lookup)
• config.Server.ValidatePrivateKeyLength(initFatal) — minimum 32 bytes (called after Secrets Manager retrieval so an SM-provided short key is also caught)

The private-key checks are split into two methods rather than folded into one because the XOR check has to fire before the SM call, and SM retrieval populates PrivateKey — so a single Validate called twice would false-positive the XOR check post-SM whenever the user originally configured only private_key_arn. Open to feedback if a different split is preferred.

Tests live in server/config/config_test.go next to the existing config Validate tests, structured as one smoke case plus error branches per the existing convention.

Behavior is preserved: runServeCmd still calls initFatal at the same points with the same descriptions.

Broader plan

Issue #33370 calls for moving logic out of serve.go ("should only contain critical config and dependency injection logic"). This PR is one slice. Follow-ups, each in their own small PR:

• Extract more config validations (Apple APNs/SCEP both-or-neither, etc.)
• Use the initFatal injection from Inject initFatal as a package-level var (#33370) #45343 to cover runtime failure paths (datastore init, Redis init, MDM init)
• Larger extractions (license init, MDM wiring, mailer init)

Related issue: Refs #33370

Checklist for submitter

• Added/updated automated tests
• Input data is properly validated (validators added, no SQL/JS/shell paths involved)
• Changes file: not applicable — internal refactor with no user-visible behavior change

Summary by CodeRabbit

• Bug Fixes & Improvements

  • Centralized and strengthened startup configuration validation.
  • Enforced mutual exclusivity for private key sources and minimum private-key length.
  • Added URL-prefix normalization (ensure leading slash, trim trailing slash) and validation.
  • Ensured OTEL logging requires tracing when enabled.
  • Restricted osquery host identifier to supported values.

• Tests

  • Added tests covering validation rules and URL-prefix normalization/validation.

[claude]

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

This PR refactors configuration validation in Fleet's server startup sequence. Validation logic previously embedded in cmd/fleet/serve.go has been moved into dedicated methods on config types: ServerConfig, OsqueryConfig, and LoggingConfig. New methods enforce private key mutual exclusivity, minimum key length, URL prefix normalization and validation, osquery host identifier allowlist, and OTEL logging consistency. The serve startup flow now delegates validation to these config methods instead of performing inline checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 35.29% 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 clearly and specifically summarizes the main change: extracting config validation logic from runServeCmd into reusable helpers.
Description check	✅ Passed	The description covers the extraction approach, lists all validation methods, explains the private-key check split, references the broader plan, confirms tests are added, and correctly notes the changes file is not applicable.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.}

[raju249]

@getvictor - I did some more refactoring of serve.go file. Can you please take a review?

Thanks! 🙏 🙇

[codecov]

Codecov Report

❌ Patch coverage is 85.71429% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.76%. Comparing base (c19df6d) to head (cec08d7).
⚠️ Report is 189 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/fleet/serve.go	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #45583      +/-   ##
==========================================
+ Coverage   66.74%   66.76%   +0.01%     
==========================================
  Files        2740     2747       +7     
  Lines      219163   219761     +598     
  Branches    10947    10947              
==========================================
+ Hits       146283   146720     +437     
- Misses      59649    59769     +120     
- Partials    13231    13272      +41     

Flag	Coverage Δ
backend	68.61% <85.71%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[getvictor]

Thanks for picking this up. The pattern you're after already exists in server/config/:

• ConditionalAccessConfig.Validate(initFatal)
• AndroidAgentConfig.Validate(initFatal)
• S3Config.ValidateCloudFrontURL(initFatal)

Each one lives next to the config struct it validates and is unit tested in config_test.go (see TestConditionalAccessConfigValidate, TestAndroidAgentConfigValidate). runServeCmd already calls config.ConditionalAccess.Validate(initFatal), which is the shape we want for the rest.

Could you reshape this PR to put the validators on the relevant config types instead of in a new cmd/fleet/serve_validation.go? Concretely:

• LoggingConfig.Validate(initFatal) for the current validateOTELLoggingConfig, so future logging checks land in one place.
• OsqueryConfig.Validate(initFatal) for validateOsqueryHostIdentifier. The allowlist is short, so one positive smoke case plus a negative is plenty.
• ServerConfig.Validate(initFatal) folding in the private_key vs private_key_secret_arn mutex check and the minimum length check of 32 bytes.
• URL prefix: split into two methods on ServerConfig. NormalizeURLPrefix() mutates, ValidateURLPrefix(initFatal) is pure. Right now normalizeAndValidateServerURLPrefix hides a mutation behind a name that reads as a pure check. Call them in sequence from runServeCmd.

Then runServeCmd becomes a series of config.X.Validate(initFatal) calls: readable top to bottom, no new file in cmd/fleet/, and consistent with what's already in the repo.

Nit on the tests once they move: prefer one positive smoke case plus the error branches. Tables that enumerate every allowed enum value are mostly maintenance cost.

[raju249]

Thanks for the review @getvictor

I have updated the code and updated the PR description. Please take a look.

Thanks! 🙏 🙇

[getvictor]

/agentic_review

[getvictor]

@coderabbitai full review

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. Misleading private key error ✓ Resolved 🐞 Bug ◔ Observability

Description

ServerConfig.Validate reports "private_key_secret_arn" in its fatal error, but the actual config
key/YAML tag is "private_key_arn" (exposed as server.private_key_arn). This can mislead operators
during startup failures by pointing them to a non-existent setting.

Code

server/config/config.go[R177-184]

Evidence

The ServerConfig field is configured via private_key_arn/server.private_key_arn, but the newly
extracted validation still errors with private_key_secret_arn, which does not appear as a
supported config key anywhere in the repo docs or config manager registration.

server/config/config.go[113-131]
server/config/config.go[173-185]
server/config/config.go[1329-1342]
docs/Configuration/fleet-server-configuration.md[812-826]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ServerConfig.Validate` emits an init-fatal error string referencing `private_key_secret_arn`, but the real config setting is `private_key_arn` (`server.private_key_arn` in the Manager). This makes the startup error message point to a key that users cannot set.
### Issue Context
- `ServerConfig.PrivateKeySecretArn` is tagged `yaml:"private_key_arn"`.
- The config manager registers `server.private_key_arn`.
- Docs reference `server_private_key_arn` / `server_private_key` (operator-facing naming), and nowhere uses `private_key_secret_arn`.
### Fix Focus Areas
- server/config/config.go[173-185]
### Expected fix
- Update the error message to reference the real key name (e.g., `private_key_arn` or `server.private_key_arn`) and keep it consistent with docs/flag naming where possible.
- Optionally align the nearby comment to the same naming.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. URLPrefix '/' accepted ✓ Resolved 🐞 Bug ≡ Correctness

Description

ServerConfig.NormalizeURLPrefix can convert URLPrefix="/" into an empty string, and
ValidateURLPrefix then returns early, so an explicitly configured "/" avoids validation and is
treated as "no prefix". This contradicts the documented expectation that URL prefixes are path-like
(e.g. "/apps/fleet") and can hide a misconfiguration that previously would fail fast at startup.

Code

server/config/config.go[R200-215]

Evidence

NormalizeURLPrefix trims a trailing slash and can yield an empty string; ValidateURLPrefix
explicitly returns early on empty, so a configured "/" can avoid the regex check even though the
regex/comment and docs expect a non-empty path prefix.

server/config/config.go[173-220]
docs/Configuration/fleet-server-configuration.md[690-703]
server/config/config_test.go[951-982]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ServerConfig.NormalizeURLPrefix` trims a trailing slash and can turn an explicitly-configured `"/"` into `""`. Because `ValidateURLPrefix` treats an empty string as valid and returns early, a `server.url_prefix` value of `"/"` silently becomes "no prefix" and skips the regex validation.
## Issue Context
- The code documents `allowedURLPrefixRegexp` as matching a **non-empty** slash-prefixed path of segments.
- The configuration docs describe prefixes in the form `"/apps/fleet"` (no trailing slash). A raw `"/"` does not fit that form and should not be silently accepted.
## Fix Focus Areas
- server/config/config.go[197-220]
- server/config/config_test.go[951-982]
## Suggested fix
1. Ensure normalization never turns a non-empty, user-supplied prefix into the empty string without erroring.
- Minimal approach: in `NormalizeURLPrefix`, after `TrimSuffix`, if the result is empty, restore it back to `"/"` (or otherwise preserve a sentinel) so `ValidateURLPrefix` will reject it via the regex.
- Alternative: make `ValidateURLPrefix` explicitly reject `"/"` and/or validate the pre-normalized value.
2. Add a unit test covering `URLPrefix: "/"` and assert that validation calls `initFatal` (rejects the value).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[coderabbitai]

✅ Actions performed

Full review triggered.

[getvictor]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0)

1. URLPrefix '/' accepted 🐞 Bug ≡ Correctness

Description

ServerConfig.NormalizeURLPrefix can convert URLPrefix="/" into an empty string, and
ValidateURLPrefix then returns early, so an explicitly configured "/" avoids validation and is
treated as "no prefix". This contradicts the documented expectation that URL prefixes are path-like
(e.g. "/apps/fleet") and can hide a misconfiguration that previously would fail fast at startup.

Code

[server/config/config.go[R200-215]](https://github.com/fleetdm/fleet/pull/45583/files#diff-3c25189bbd67311458979d8b7dcacba95332e9afd3754e23966df0388287be9cR200-R215)

Evidence

NormalizeURLPrefix trims a trailing slash and can yield an empty string; ValidateURLPrefix
explicitly returns early on empty, so a configured "/" can avoid the regex check even though the
regex/comment and docs expect a non-empty path prefix.

[server/config/config.go[173-220]](https://github.com/fleetdm/fleet/blob/0b11fa5c78241f205515403fa12d11463020915d/server/config/config.go/#L173-L220)
[docs/Configuration/fleet-server-configuration.md[690-703]](https://github.com/fleetdm/fleet/blob/0b11fa5c78241f205515403fa12d11463020915d/docs/Configuration/fleet-server-configuration.md/#L690-L703)
[server/config/config_test.go[951-982]](https://github.com/fleetdm/fleet/blob/0b11fa5c78241f205515403fa12d11463020915d/server/config/config_test.go/#L951-L982)

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ServerConfig.NormalizeURLPrefix` trims a trailing slash and can turn an explicitly-configured `"/"` into `""`. Because `ValidateURLPrefix` treats an empty string as valid and returns early, a `server.url_prefix` value of `"/"` silently becomes "no prefix" and skips the regex validation.

## Issue Context
- The code documents `allowedURLPrefixRegexp` as matching a **non-empty** slash-prefixed path of segments.
- The configuration docs describe prefixes in the form `"/apps/fleet"` (no trailing slash). A raw `"/"` does not fit that form and should not be silently accepted.

## Fix Focus Areas
- server/config/config.go[197-220]
- server/config/config_test.go[951-982]

## Suggested fix
1. Ensure normalization never turns a non-empty, user-supplied prefix into the empty string without erroring.
  - Minimal approach: in `NormalizeURLPrefix`, after `TrimSuffix`, if the result is empty, restore it back to `"/"` (or otherwise preserve a sentinel) so `ValidateURLPrefix` will reject it via the regex.
  - Alternative: make `ValidateURLPrefix` explicitly reject `"/"` and/or validate the pre-normalized value.
2. Add a unit test covering `URLPrefix: "/"` and assert that validation calls `initFatal` (rejects the value).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@raju249 It looks like this issue was introduced by this PR. Please fix. Otherwise things look good.

[raju249]

@getvictor - Right. Fixed it. Can you take a fresh look, please?

[raju249]

/agentic_review

[qodo-free-for-open-source-projects]

Persistent review updated to latest commit 166fa9e

[raju249]

/agentic_review

[qodo-free-for-open-source-projects]

Persistent review updated to latest commit cec08d7

[coderabbitai]

🧹 Nitpick comments (1)

server/config/config.go (1)

190-195: 💤 Low value

Consider documenting the 32-byte minimum requirement.

The 32-byte minimum for private keys is hardcoded without explanation. Consider adding a package-level constant with a comment explaining the cryptographic rationale, or at least add an inline comment.

📝 Suggested improvement

+// minPrivateKeyLength is the minimum required length for server private keys
+// to ensure adequate cryptographic strength.
+const minPrivateKeyLength = 32
+
 func (s ServerConfig) ValidatePrivateKeyLength(initFatal func(err error, msg string)) {
-	if len(s.PrivateKey) > 0 && len(s.PrivateKey) < 32 {
+	if len(s.PrivateKey) > 0 && len(s.PrivateKey) < minPrivateKeyLength {
-		initFatal(errors.New("private key must be at least 32 bytes long"),
+		initFatal(fmt.Errorf("private key must be at least %d bytes long", minPrivateKeyLength),
 			"validate private key")
 	}
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@server/config/config.go` around lines 190 - 195, The hardcoded 32-byte
minimum in ServerConfig.ValidatePrivateKeyLength should be replaced by a named
package-level constant (e.g., MinPrivateKeyLength) with a comment explaining the
cryptographic rationale (why 32 bytes is required), and the
ValidatePrivateKeyLength method should reference that constant instead of the
literal; update any related tests or callers of ValidatePrivateKeyLength if they
assume the magic number.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@server/config/config.go`:
- Around line 190-195: The hardcoded 32-byte minimum in
ServerConfig.ValidatePrivateKeyLength should be replaced by a named
package-level constant (e.g., MinPrivateKeyLength) with a comment explaining the
cryptographic rationale (why 32 bytes is required), and the
ValidatePrivateKeyLength method should reference that constant instead of the
literal; update any related tests or callers of ValidatePrivateKeyLength if they
assume the magic number.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 193a9872-8558-420d-b36f-c3a40f130517

📥 Commits

Reviewing files that changed from the base of the PR and between 166fa9e and cec08d7.

📒 Files selected for processing (1)

• server/config/config.go

[getvictor]

Thank you for contributing

[raju249]

@getvictor - Would it be possible to merge this PR? Thanks for the approval!