fix: require a serviceAuth entry for every serviceDomains host by planger · Pull Request #59 · eclipse-enclave/enclave

planger · 2026-08-26T12:05:36Z

What it does

Closes a fail-open gap in extension spec validation.

network.serviceDomains maps a host to a service id; network.serviceAuth.<id> defines how the gateway injects that credential. buildSecrets attaches an HTTP release rule only to ids present in both credentials.sources and network.serviceAuth, and EffectiveResolver unions the release hosts into the effective allowlist. A serviceDomains entry whose id had no serviceAuth entry was therefore inert in two security-relevant ways at once:

its hosts dropped out of the effective allowlist, and
the credential was injected as a raw env value instead of a proxy-swapped placeholder,

while enclave validate-extensions still reported ok. Deleting a single serviceAuth line from a spec was enough to trigger both, with no signal.

validateServiceAuthMappings now rejects that at load and names the remedy (add the serviceAuth entry, or list the hosts under network.allowedDomains if they only need to be reachable). The reverse direction — a serviceAuth entry with no hosts from either source — was already rejected by normalizeHosts, so the serviceDomains ↔ serviceAuth mapping is now required in both directions.

One related fix: ListFeatures swallowed every spec load error, so for a mixin the new load failure would have degraded to "feature silently missing from the built image". It now warns (on stderr, so shell completion stays intact) before skipping.

All built-in specs already satisfy the rule; no extension changes were needed.

How to test

make build && make test && make lint. The regression test is TestValidateServiceAuthMappings/serviceDomains_id_without_serviceAuth_fails.

End-to-end, with a user-authored feature spec:

TMP=$(mktemp -d)
mkdir -p $TMP/config/enclave/extensions/features/broken-feat
sed '/github-enterprise-token: { headerName/d' extensions/features/github-cli/spec.yaml \
  | sed 's/^name: github-cli/name: broken-feat/' \
  > $TMP/config/enclave/extensions/features/broken-feat/spec.yaml
XDG_CONFIG_HOME=$TMP/config ./bin/enclave validate-extensions   # before: "ok"; now: exits 1 naming the host and service
XDG_CONFIG_HOME=$TMP/config ./bin/enclave features              # now warns instead of silently omitting the feature

Follow-ups

The ListFeatures skip-warning has no unit test: specWarn is called directly rather than injected, and threading a sink through an exported function with nine call sites seemed like more churn than a log line justifies. Verified manually as above.
ListFeatures runs several times per enclave run, so the warning repeats. warnReservedFields already behaves that way, so this is the existing sink's behavior rather than something new here.

Breaking changes

This PR introduces breaking changes and has been coordinated with maintainers.

A spec that was previously accepted and silently inert is now a load error. No built-in spec is affected; a third-party sbx kit that uses serviceDomains purely for host scoping would need those hosts moved to network.allowedDomains.

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the contribution guidelines.

A spec.yaml mapping a host to a service id under network.serviceDomains without a matching network.serviceAuth entry loaded without complaint, but buildSecrets attaches an HTTP release rule only to ids present in both credentials.sources and network.serviceAuth. The mapping was therefore inert in two security-relevant ways: the hosts dropped out of the release hosts unioned into the effective allowlist, and the credential was injected as a raw env value instead of a proxy-swapped placeholder — while validate-extensions still reported ok. Reject it at load with a message naming the remedy. The reverse direction is already covered by normalizeHosts, so the mapping is now required both ways. ListFeatures swallowed every spec load error, which for a mixin would have turned this new load failure into a feature silently missing from the built image; it now warns before skipping.

EclipseSourceAI

Note

Autonomous AI review.

This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. A thumbs-down reaction on a comment marks it as rejected for follow-up reviews. Noting why in a reply helps, since replies are read too.

Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.

To get an updated review after pushing changes, a maintainer may re-request a review from this account.

Running in Eclipse Enclave, submitted via review-guard-mcp

Adds one validation rule to validateServiceAuthMappings: a network.serviceDomains host whose service id has no network.serviceAuth entry is now a load error. The reasoning checks out against the code. buildSecrets only attaches an HTTP release rule for ids present in both credentials.sources and network.serviceAuth, and serviceDomains has no other consumer, so such an entry really is inert in both directions the description claims (hosts never reach the effective allowlist, credential goes into the container as a raw env value). All built-in specs already satisfy the rule, and the docs updates in docs/extensions/README.md and adding-a-tool.md match the implemented behavior.

The ListFeatures change is a sensible companion: without it a mixin hitting the new error would have degraded to "feature silently missing from the image".

Points for a maintainer:

The golden conformance test does not cover the feature side of this. A built-in feature spec that fails the new rule is skipped by ListFeatures and make test stays green, while the equivalent break in a tool spec fails loudly. Verified both locally.
Whether the hard load error is the right call for third-party sbx kits that use serviceDomains for host scoping only. The description documents the break but leaves the breaking-changes checkbox unchecked.
The reverse direction is enforced only indirectly, via a generic hosts must contain at least one domain pattern error that never names serviceDomains.

Scope is tight, no drive-by changes, and the new test sits with the existing subtests.

EclipseSourceAI · 2026-08-28T17:49:39Z

+			// A feature dropped here is silently missing from the built image,
+			// so surface why. A directory without a spec is not an error.
+			if !errors.Is(err, os.ErrNotExist) {
+				specWarn(fmt.Sprintf("feature %q: %v; skipping", name, err))


Warning is good, but make test still can't catch this for a built-in feature. I deleted the github-enterprise-token line from extensions/features/github-cli/spec.yaml and TestExtensionSurfaceGolden stayed green: the feature just drops out of ListFeatures and nothing asserts its golden (features leg). Doing the same edit to extensions/tools/codex/spec.yaml fails loudly, because the tools leg enumerates names and loads each one (tools leg). Making the features leg iterate listExtensionNames + LoadFeatureExtension would close that.

Fixed in 35152b5. The features leg of TestExtensionSurfaceGolden now enumerates spec names and loads each one, so a spec that fails to load fails the test instead of dropping out of the snapshot set. I pulled the name-plus-hasSpecFile filter out of ListTools into listSpecNames and used it on both legs; ListFeatures uses it too, which filters spec-less directories before the load and lets the os.ErrNotExist carve-out around the new warning go away.

Verified with your reproduction. Deleting the github-enterprise-token line from extensions/features/github-cli/spec.yaml now gives:

--- FAIL: TestExtensionSurfaceGolden conformance_test.go:71: LoadFeatureExtension(github-cli): .../extensions/features/github-cli/spec.yaml: network.serviceDomains["*.ghe.com"] references service "github-enterprise-token" with no matching network.serviceAuth entry (add one, or list the hosts under network.allowedDomains instead)

EclipseSourceAI · 2026-08-28T17:49:41Z

 			return fmt.Errorf("%s: network.serviceDomains[%q] references service %q with no matching credentials.sources entry", specPath, host, id)
 		}
+		if _, ok := doc.Network.ServiceAuth[id]; !ok {
+			return fmt.Errorf("%s: network.serviceDomains[%q] references service %q with no matching network.serviceAuth entry (add one, or list the hosts under network.allowedDomains instead)", specPath, host, id)


This flips a previously accepted (if inert) sbx kit spec into a hard load error, and for a kind: sandbox kit that means the tool stops running entirely, not just losing the header injection. AGENTS.md asks for breaking changes to external-consumer contracts to be coordinated and the breaking-changes box in the description is unchecked, so a maintainer should confirm hard fail over a load warning here.

Keeping the hard fail, deliberately.

A load warning would preserve exactly the behaviour this PR exists to remove. With no serviceAuth entry the secret gets no release rule, and auth_manager.go then puts the real token into the container environment as a raw value instead of a proxy-swapped placeholder, while the hosts stay off the effective allowlist. So the spec was already not doing what it asked for, and it was leaking the credential while doing it. A warning printed during enclave run would scroll past.

It is also the treatment the loader already gives this class of authoring mistake: unknown keys under UnmarshalStrict, validateProxyManaged, validateEntrypointArgv, and root commands.startup all fail at load rather than warn.

On blast radius: no built-in spec is affected, and for features ListFeatures warns and skips, so a broken kit degrades rather than blocking the session. A kind: sandbox kit does stop running, but its spec is authored by whoever invokes the tool, the remedy is one line, and the error names it. I am fine with that trade as maintainer.

Note that the reverse-direction check added in 35152b5 does not widen this: it only re-messages specs normalizeHosts already rejected.

EclipseSourceAI · 2026-08-28T17:49:42Z

 		if _, ok := sources[id]; !ok {
 			return fmt.Errorf("%s: network.serviceDomains[%q] references service %q with no matching credentials.sources entry", specPath, host, id)
 		}
+		if _, ok := doc.Network.ServiceAuth[id]; !ok {


The reverse direction is required but not nearly as legible. A serviceAuth entry with no hosts and no serviceDomains reference bottoms out in secrets["tok"].release.http: hosts must contain at least one domain pattern (here), which never mentions serviceDomains. Catching that case here too would make both directions point at the same remedy.

Fixed in 35152b5. validateServiceAuthMappings now also rejects a serviceAuth entry that gets no hosts from either source:

network.serviceAuth["github-enterprise-token"] has no hosts to release the credential to (add a hosts list, or map hosts to this service under network.serviceDomains)

Two follow-on adjustments. The check runs ahead of normalizeHosts, so TestLoadProfileRejectsSecretReleaseWithEmptyHosts now asserts the new message, and TestValidateAndNormalizeSecretConfigsEmptyReleaseHosts covers the normalizeHosts guard directly to keep it from going untested. The unknown-id checks for both maps also run before both pairing checks now, otherwise a typo like serviceDomains: { ghe.com: github-tokn } would report the hostless serviceAuth entry it happens to create rather than the typo itself.

…lden set Addresses AI review feedback on the serviceDomains/serviceAuth validation: - The reverse direction (a serviceAuth entry with no hosts from either serviceDomains or its own hosts list) bottomed out in normalizeHosts' generic "hosts must contain at least one domain pattern", which never named serviceDomains. It is now caught in validateServiceAuthMappings so both directions point at the same remedy. Unknown-id errors are checked before the pairing so a typo'd id still names itself. This moves the rejection ahead of normalizeHosts, so the empty-hosts guard is now covered directly in secrets_test.go. - TestExtensionSurfaceGolden's features leg went through ListFeatures, which skips specs it cannot load, so a broken built-in feature spec left the suite green. It now enumerates spec names and loads each one, matching the tools leg. The name enumeration ListTools already did is extracted into listSpecNames and shared with ListFeatures, which lets the spec-less-directory case drop out of the new skip warning.

EclipseSourceAI

Note

Autonomous AI review.

This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. A thumbs-down reaction on a comment marks it as rejected for follow-up reviews. Noting why in a reply helps, since replies are read too.

Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.

To get an updated review after pushing changes, a maintainer may re-request a review from this account.

Running in Eclipse Enclave, submitted via review-guard-mcp

Follow-up review. The only functional change since my last pass is 35152b5, which addresses both open points from my first review:

The reverse-direction gap (a serviceAuth entry with no hosts from either source) is now caught in validateServiceAuthMappings with a message naming serviceDomains, instead of bottoming out in the generic normalizeHosts error. Unknown-id checks run first so a typo'd id is reported at the right spot, verified with the new spec_map_test.go subtests.
The features leg of TestExtensionSurfaceGolden now enumerates spec names via the extracted listSpecNames and loads each one directly, so a built-in feature spec that fails to load now fails the test instead of silently dropping out of ListFeatures. Confirmed this actually catches the repro from my earlier comment.

I re-ran go build ./... and go test ./internal/config/... locally, both pass, and read through the new validation ordering and tests; no issues found in the pushed changes.

The remaining open thread on spec_map.go (hard load error vs. warning for kind: sandbox kits) got a detailed rebuttal from the author defending the hard fail. That reasoning holds up against the code, so I'm not re-raising it; it's a call for a maintainer to sign off on, not a defect.

These previous comments can be resolved as they are now handled:

I can't resolve them myself as I would need write permission on this repository.

planger marked this pull request as ready for review August 26, 2026 12:29

EclipseSourceAI reviewed Aug 28, 2026

View reviewed changes

planger requested a review from EclipseSourceAI August 31, 2026 14:31

EclipseSourceAI reviewed Aug 31, 2026

View reviewed changes

Navigation Menu

Uh oh!

fix: require a serviceAuth entry for every serviceDomains host - #59

fix: require a serviceAuth entry for every serviceDomains host#59
planger wants to merge 2 commits into
mainfrom
fix/service-domains-require-service-auth

planger commented Aug 26, 2026

Uh oh!

EclipseSourceAI left a comment

Uh oh!

EclipseSourceAI Aug 28, 2026

Uh oh!

planger Aug 31, 2026

Uh oh!

EclipseSourceAI Aug 28, 2026

Uh oh!

planger Aug 31, 2026

Uh oh!

EclipseSourceAI Aug 28, 2026

Uh oh!

planger Aug 31, 2026

Uh oh!

EclipseSourceAI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

planger commented Aug 26, 2026

What it does

How to test

Follow-ups

Breaking changes

Review checklist

Uh oh!

EclipseSourceAI left a comment

Choose a reason for hiding this comment

Uh oh!

EclipseSourceAI Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

planger Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

EclipseSourceAI Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

planger Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

EclipseSourceAI Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

planger Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

EclipseSourceAI left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants