agents: accept egress deny-all (none / empty allowlist) in manifest validation by sanpj2292 · Pull Request #2031 · digitalocean/doctl

sanpj2292 · 2026-10-09T15:02:33Z

Summary

Client-side manifest validation (validateAgentManifest) rejected egress: none, so deny-all egress could not be expressed from the CLI. This change:

Accepts the flat scalar egress: none (alongside unrestricted).
Keeps present-but-empty allowlists (egress: [], allow_hosts: [], allow_ips: [], legacy allow: [] / allowIps: []) valid.
Rejects deny-all combined with legacy unrestricted, vpc_uuid or subnet_uuid, using harness-api's wording.
Rejects a legacy denyAll key with a hint (harness-api has no such key; the hint is only friendlier than "unknown field").
Matches harness-api on legacy edge cases: egress scalars and lists are flat-only, legacy unrestricted must be a bool, and none / unrestricted match exactly.
Updates doctl agents help text.

Server-only checks (not in validate): deny-all with URL-bound credentials, and legacy unrestricted with a non-empty allowlist.

Draft: do not merge until the matching server-side change is deployed, otherwise none would validate here but not be honoured by the API.

Testing

go test ./commands/ -run 'ValidateAgentManifest|Agents' passes.
go vet ./commands/ and gofmt are clean.
TestRegistryLogout fails on main without this change.

DO-rrao · 2026-11-04T09:02:24Z

@@ -87,6 +87,7 @@ harness: opencode
 ` + "```\n\n" + `**Egress** (optional). Omitting ` + "`egress`" + ` leaves destinations unrestricted. Exactly one form:

 - ` + "`egress: unrestricted`" + `


Please address before merge:

Wrong legacy egress hints: Validation hints tell users to write egress: none for legacy manifests, which fails API decoding. Legacy manifests should suggest allow: [] (to block all) or unrestricted: true. egress: none is only correct for flat manifests. Also, you'll need to update the tests that currently lock this flat wording onto the legacy cases.

Wrong flat deny-all hints: The deny-all hint incorrectly suggests allow: [] for flat manifests, which is the wrong key and will just fail the next validation check. Flat manifests should suggest egress: none or allow_hosts: [].

Dangerous empty allowlist + VPC error: When an empty allowlist is combined with a vpc_uuid, doctl currently implies the user should drop the VPC. This is dangerous because it can trick users into accidentally turning an open VPC attachment into a deny-all rule. Update this to mirror the server's exact error message for both flat and legacy formats: "ambiguous: drop the empty list for VPC with no ACL, or drop vpc_uuid for deny-all."

All three points were valid and are fixed in c684c11 (branch also rebased on main).

Legacy hints: null egress/allowlist now says write allow: [] to block all, or unrestricted: true / omit it (server wording); egress: none is only suggested for flat. Tests no longer lock flat wording onto legacy cases.

Flat denyAll hint now says write egress: none (or allow_hosts: []) to block all egress; the legacy hint keeps allow: [].

Empty allowlist + vpc_uuid/subnet_uuid now returns the server's exact message: "an empty allow_hosts/allow_ips with vpc_uuid is ambiguous: drop the empty list for VPC with no ACL, or drop vpc_uuid for deny-all", for both formats. Other combinations (e.g. legacy unrestricted) keep the 'cannot be combined' text.

vhrahulkumar

Nice, this closes the client/server gap for deny-all. A few things to check: the description and the code disagree on denyAll, and a few cases where validate can say OK while the API would 400. Details inline.

vhrahulkumar · 2026-11-04T16:15:28Z

 		if _, ok := known[k]; ok {
 			continue
 		}
+		if k == "denyAll" {


The PR description says this accepts legacy denyAll: true, but this branch rejects it, and TestValidateAgentManifest_EgressDenyAllFieldShape asserts the rejection. Which is intended? If the server doesn't accept denyAll, can we fix the description? Also, the generic "unknown field" path would already reject it, so this is only here for the hint text. That's fine, but worth saying in a comment.

Intended: harness-api rejects denyAll (agentspec.go keeps it only as a hint entry, "is not a manifest key"). The PR description was wrong; it is fixed. Added a comment in validateEgressObject saying the key is hint-only.

vhrahulkumar · 2026-11-04T16:15:28Z

-		// Explicit null is treated like omitted by the server; nothing to check.
+		out.Errors = append(out.Errors, fmt.Sprintf("%s is empty; %s", path, egressNullFix(legacy)))
 		return
 	case string:


The comments and tests say egress: none is flat-only, and the legacy fix text tells users to write allow: []. But this case string doesn't look at legacy, so spec.sandbox.egress: none passes validation in an envelope manifest. If harness-api's legacy decoder rejects it, validate says OK and create 400s. Can we confirm what legacy accepts and either gate none on !legacy or add a legacy test case documenting that it's accepted?

Confirmed against harness-api: a legacy spec.sandbox.egress: none fails decoding ("cannot unmarshal string into ... Egress"). The same holds for the unrestricted scalar and a host list in the legacy envelope. validateEgressValue now rejects all three for legacy manifests ("must be an object in the legacy envelope"). Tests: TestValidateAgentManifest_EgressLegacyRequiresObject.

vhrahulkumar · 2026-11-04T16:15:28Z

+	// VPC/subnet can conflict with it.
+	unrestricted := false
+	if legacy {
+		unrestricted, _ = m["unrestricted"].(bool)


validateEgressObject accepts unrestricted: "true" and "unrestricted" as strings, but this only honors a bool. So allow: [] plus unrestricted: "true" gets no conflict error. Could we share one parse helper (e.g. egressUnrestricted(m) bool) so both places agree?

harness-api decodes legacy unrestricted into a Go bool, so "true" is a 400 there. Rather than share a parser that accepts strings, legacy unrestricted is now bool-only in validateEgressObject, so both places agree. Test: TestValidateAgentManifest_EgressLegacyUnrestrictedMustBeBool.

vhrahulkumar · 2026-11-04T16:15:28Z

+	case len(vpcKeys) > 0 && !unrestricted:
+		// An empty list plus a VPC used to mean "VPC, no ACL". Use the server's
+		// wording so users are not nudged to drop the VPC and go deny-all.
+		out.Errors = append(out.Errors, fmt.Sprintf("%s: %s", path, egressEmptyListWithVPCMsg))


With allow_hosts: [] and only subnet_uuid (no vpc_uuid), the user sees "...with vpc_uuid is ambiguous... drop vpc_uuid for deny-all", then also "subnet_uuid requires vpc_uuid". The first message points at a key they didn't set. It's also flat-key wording for legacy manifests (allow / vpcUuid). Maybe skip this error when vpc_uuid is absent and let the "requires" error stand. Not covered by tests either.

Agreed. With only subnet_uuid set, the "ambiguous ... vpc_uuid" error is skipped and only "subnet_uuid requires vpc_uuid" is shown. If unrestricted is also set, the "cannot be combined with" error lists the key actually written. Test: TestValidateAgentManifest_EgressEmptyListSubnetOnly (flat and legacy). Note harness-api itself returns the "ambiguous" text for subnet-only, so the server error differs, but both fail validation.

vhrahulkumar · 2026-11-04T16:15:28Z

+// object is deny-all when an allowlist key is present but empty and the other
+// list names nothing. A missing or null key is not "present". Deny-all cannot
+// be combined with unrestricted, a non-empty allowlist, or a VPC/subnet.
+func validateEgressDenyAllCombination(m map[string]any, path string, legacy bool, out *agentManifestValidation) {


The comment says deny-all can't combine with "a non-empty allowlist", but a non-empty allowlist means it isn't deny-all (it's the IP-only or host-only case), so that clause is vacuous. The help text in agents_help.go says deny-all "cannot be combined with ... URL-bound credentials", which validate doesn't check. Could we say that part is enforced server-side only? Separately, legacy unrestricted: true plus a non-empty allow is still accepted client-side, while the OpenAPI text says that combination is invalid. Is that intended to stay a server-only check?

Fixed the comment (dropped the vacuous "non-empty allowlist" clause) and said which checks stay server-only: deny-all with URL-bound credentials, and legacy unrestricted with a non-empty allowlist (harness-api rejects it; validate does not, and that predates this PR). The help text now says the API rejects deny-all with URL-bound credentials and validate does not check it.

vhrahulkumar · 2026-11-04T16:15:28Z

-// subnet_uuid. Structural checks only — advisory wording (e.g. IP-only ACL) is
+// "unrestricted", "none" (deny all), host list, or object with allow_hosts /
+// allow_ips / vpc_uuid / subnet_uuid. An allowlist written present-but-empty
+// also means deny-all (MARSOHS-2118, VULN-5606). Structural checks only — advisory wording (e.g. IP-only ACL) is


Please drop VULN-5606 from this comment. doctl is public, and a VULN ID signals this is a security remediation. MARSOHS-* has precedent in the file, but the VULN ref adds nothing for readers. The line is also unwrapped against the surrounding comment style.

Dropped VULN-5606 and re-wrapped the comment.

vhrahulkumar · 2026-11-04T16:15:28Z

+// list names nothing. A missing or null key is not "present". Deny-all cannot
+// be combined with unrestricted, a non-empty allowlist, or a VPC/subnet.
+func validateEgressDenyAllCombination(m map[string]any, path string, legacy bool, out *agentManifestValidation) {
+	hostsKey, ipsKey, vpcKey, subnetKey := "allow_hosts", "allow_ips", "vpc_uuid", "subnet_uuid"


Nit: the hostsKey, ipsKey, vpcKey, subnetKey := ...; if legacy {...} block is now copy-pasted here and in validateEgressObject. We could pass the resolved keys in, or derive them from egressAliasFields.

Added egressKeysForFormat(legacy) (derived from egressAliasFields) and used it in both places.

vhrahulkumar · 2026-11-04T16:15:28Z

+	})
+}
+
+func TestValidateAgentManifest_EgressNullRejected(t *testing.T) {


Missing cases: legacy egress: none, unrestricted: "true" with allow: [], empty list with subnet_uuid only, none and None, and allow: [] with allowIps: null. In this test, branching on strings.HasPrefix(name, "legacy") (L632) is brittle. Could we put the expected fix text in the table, like the other new tests do?

Added the cases and moved the expected text into the table (no more HasPrefix(name, "legacy")): legacy egress: none (and the other legacy scalar/list forms), legacy unrestricted: "true", empty list with subnet_uuid only (flat and legacy), " none " and None (rejected: harness-api matches exactly, so the TrimSpace in doctl was removed), allow: [] with allowIps: null, and legacy unrestricted with subnet. All in b99de1d.

…alidation validateAgentManifest rejected 'egress: none' (scalar must be 'unrestricted') so users could not express deny-all from the CLI. Accept 'none' (flat) and 'denyAll: true' (legacy envelope), keep present-but-empty allowlists valid, and reject deny-all combined with unrestricted/allowlist/vpc/subnet keys, mirroring the server-side agentspec rules. Update help text.

Drop legacy denyAll acceptance (server rejects the key), reject null egress and allowlist keys, and extend tests.

… validate Legacy manifests were told to write `egress: none` (flat-only) and flat manifests were told to write `allow: []` (legacy-only). Hints now match the manifest format, mirroring harness-api's rejectNullEgress and denyAll text. An empty allowlist with vpc_uuid/subnet_uuid now reports harness-api's exact "ambiguous: drop the empty list for VPC with no ACL, or drop vpc_uuid for deny-all" message instead of implying the VPC should be dropped. Tests now assert the per-format wording.

… and edge cases Legacy envelope: reject egress scalars and lists, and a non-bool unrestricted. Scalars match exactly. subnet-only with an empty list gets the requires error. Share key lookup, drop the VULN ref, and add table-driven test cases.

vhrahulkumar

All my comments are addressed, thanks. One note for merge: the description still says not to merge until the matching server-side change is deployed, and the PR is no longer a draft, so please hold the merge until that ships.

sanpj2292 marked this pull request as ready for review November 3, 2026 13:24

sanpj2292 force-pushed the prao/MARSOHS-2120/fix.egress-deny-all branch from aa27f15 to 61033b7 Compare November 3, 2026 13:43

DO-rrao reviewed Nov 4, 2026

View reviewed changes

sanpj2292 force-pushed the prao/MARSOHS-2120/fix.egress-deny-all branch from 540ed39 to c684c11 Compare November 4, 2026 11:13

sanpj2292 requested a review from DO-rrao November 4, 2026 11:35

vhrahulkumar reviewed Nov 4, 2026

View reviewed changes

sanpj2292 and others added 5 commits November 4, 2026 22:17

agents: align deny-all validation with harness-api

bdbb26b

Drop legacy denyAll acceptance (server rejects the key), reject null egress and allowlist keys, and extend tests.

chore: gofmt agents_validate_test.go

0e28615

sanpj2292 force-pushed the prao/MARSOHS-2120/fix.egress-deny-all branch from b99de1d to 8410644 Compare November 4, 2026 16:48

vhrahulkumar approved these changes Nov 4, 2026

View reviewed changes

DO-rrao approved these changes Nov 4, 2026

View reviewed changes

DO-rrao merged commit 963718f into main Nov 4, 2026
6 checks passed

DO-rrao deleted the prao/MARSOHS-2120/fix.egress-deny-all branch November 4, 2026 17:12

		@@ -87,6 +87,7 @@ harness: opencode
		` + "```\n\n" + `Egress (optional). Omitting ` + "`egress`" + ` leaves destinations unrestricted. Exactly one form:

		- ` + "`egress: unrestricted`" + `

Conversation

sanpj2292 commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vhrahulkumar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vhrahulkumar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sanpj2292 commented Oct 9, 2026 •

edited

Loading