Skip to content

fix(egress): require enforced credential destinations#1136

Merged
jwx0925 merged 5 commits into
opensandbox-group:mainfrom
hellomypastor:codex/fix-credential-egress-binding
Jun 30, 2026
Merged

fix(egress): require enforced credential destinations#1136
jwx0925 merged 5 commits into
opensandbox-group:mainfrom
hellomypastor:codex/fix-credential-egress-binding

Conversation

@hellomypastor

@hellomypastor hellomypastor commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Require Credential Vault to use dns+nft enforcement so credential-bound destinations cannot bypass network policy through direct IP connections.
  • Keep defaultAction: allow backward compatible, but emit security warnings and recommend defaultAction: deny.
  • Respect explicit deny rules when validating credential-bound destinations.
  • Update documentation, specifications, SDK fixtures, and tests for the hardened behavior.

Testing

  • Not run (explain why)
  • Unit tests
  • Integration tests
  • e2e / manual verification

Validation performed:

  • components/egress: go test ./...
  • Credential Vault trusted/untrusted proxy tests
  • Go E2E sources compilation check

Full E2E was not run because it requires a matching OpenSandbox server and Credential Vault target environment.

Breaking Changes

  • None
  • Yes (describe impact and migration path)

Credential Proxy now requires an explicit network policy and dns+nft enforcement. Existing deployments using DNS-only enforcement must migrate to:

[egress]
mode = "dns+nft"

defaultAction: allow remains accepted for backward compatibility, but is deprecated for Credential Vault usage and emits a security warning. defaultAction: deny with explicit destination allow rules is recommended.

Checklist

  • Linked Issue or clearly described motivation
  • Added/updated docs (if needed)
  • Added/updated tests (if needed)
  • Security impact considered
  • Backward compatibility considered

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a5cbcc7faf

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread components/egress/pkg/credentialvault/vault.go
Comment thread tests/go/credential_vault_e2e_test.go
Comment thread specs/sandbox-lifecycle.yml Outdated
@Pangjiping Pangjiping self-assigned this Jun 29, 2026
@Pangjiping Pangjiping added bug Something isn't working component/egress labels Jun 29, 2026
Pangjiping
Pangjiping previously approved these changes Jun 29, 2026

@Pangjiping Pangjiping left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Pangjiping

This comment was marked as duplicate.

Pangjiping

This comment was marked as duplicate.

@Pangjiping Pangjiping left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking Change concern: mandatory defaultAction: deny

Security hardening direction makes sense, but forcing defaultAction: deny as a hard constraint is a breaking change that's difficult to accept.

Issues

  1. All existing integrations break — every user currently running Credential Vault with defaultAction: allow gets a 400 on upgrade. The E2E fixture churn (all 6 SDKs touched) shows the blast radius.

  2. defaultAction: deny puts heavy burden on callers — users must enumerate every destination host, including credential-bound targets AND their IP addresses (the new targetIp rules in E2E are a direct example). Maintaining a complete allowlist is expensive for sandboxes that talk to multiple services.

Suggestion

  • Make defaultAction: deny recommended — guide users with docs and warning logs, don't hard-reject
  • If the security model truly requires deny-default, use a deprecation cycle: warn → reject

@github-actions github-actions Bot added component/server documentation Improvements or additions to documentation labels Jun 30, 2026
…l-egress-binding

# Conflicts:
#	components/egress/credential_vault_handler_test.go

@Pangjiping Pangjiping left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @jwx0925 ptal

@jwx0925 jwx0925 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jwx0925 jwx0925 merged commit c5f669a into opensandbox-group:main Jun 30, 2026
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working component/egress component/server documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants