Skip to content

fix(proxy): include OPA deny reason in CONNECT 403 response#2363

Merged
johntmyers merged 2 commits into
NVIDIA:mainfrom
zanetworker:fix/proxy-connect-deny-reason
Jul 21, 2026
Merged

fix(proxy): include OPA deny reason in CONNECT 403 response#2363
johntmyers merged 2 commits into
NVIDIA:mainfrom
zanetworker:fix/proxy-connect-deny-reason

Conversation

@zanetworker

Copy link
Copy Markdown
Contributor

Summary

When a CONNECT request is denied by OPA policy, the 403 response used a generic "not permitted by policy" message for both "endpoint not in policy" and "endpoint matched but binary didn't match." Users had no way to distinguish the two without reading supervisor logs, making binary identity the #1 debugging friction when setting up agents.

Related Issue

Fixes #2355

Changes

  • Added build_json_error_response_with_reason function that includes an optional reason field in the JSON body
  • Updated the CONNECT deny response to forward the OPA deny_reason (which already contains detailed binary mismatch info)
  • Empty reason is omitted for backward compatibility

Before:

{"detail":"CONNECT host:443 not permitted by policy","error":"policy_denied"}

After (binary mismatch):

{"detail":"CONNECT host:443 not permitted by policy","error":"policy_denied","reason":"binary '/usr/bin/python3.11' not allowed in policy 'allow_xxx' (ancestors: [...])"}

Testing

Bug reproduced live against a running sandbox:

# python3 is not in the aiplatform binary allowlist
openshell sandbox exec --name my-sandbox -- python3 -c "..."
# Current response: 403 with no reason (confirmed)
# After fix: 403 with reason field showing binary mismatch
  • mise run pre-commit passes
  • Unit tests added: policy_deny_response_includes_reason, policy_deny_response_omits_empty_reason
  • Bug reproduced live against local Docker sandbox

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (not applicable — response format addition only)

When a CONNECT request was denied by OPA policy, the 403 response
used a generic "not permitted by policy" message for both "endpoint
not in policy" and "endpoint matched but binary didn't match." Users
had no way to distinguish the two without reading supervisor logs.

The OPA policy already computes a detailed deny_reason (e.g.,
"binary '/usr/bin/node' not allowed in policy 'X'") but the proxy
was not including it in the HTTP response.

Now the CONNECT deny response includes a "reason" field with the
OPA deny reason when available. When the reason is empty, the field
is omitted for backward compatibility.

Fixes NVIDIA#2355

Signed-off-by: Adel Zaalouk <azaalouk@redhat.com>
@copy-pr-bot

copy-pr-bot Bot commented Jul 20, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@johntmyers johntmyers self-assigned this Jul 20, 2026
@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

PR Review Status

Validation: This PR is project-valid because it is a small, concentrated proxy bug fix linked to #2355 and addresses a user-visible sandbox policy debugging gap.
Head SHA: e333f14edf9e928e4ff1c5d408dcf1f9bf82c8dc

Review findings:

  • Blocking: this changes the user-visible CONNECT denial JSON shape, and docs/observability/logging.mdx currently documents the denied CONNECT body with only error and detail. Please update that page to include the optional reason field and explain that it carries the OPA deny reason when available.
  • Non-blocking: the independent code review found the implementation correct, but suggested adding an end-to-end-ish CONNECT policy-denial test that exercises handle_tcp_connection and asserts a non-empty OPA deny reason reaches the wire response, instead of only testing the response helper.

Docs: missing for a direct sandbox/proxy behavior change documented in docs/observability/logging.mdx.

Next state: gator:in-review

@johntmyers johntmyers added the gator:in-review Gator is reviewing or awaiting PR review feedback label Jul 20, 2026
@johntmyers

Copy link
Copy Markdown
Collaborator

/ok to test e333f14

@johntmyers johntmyers added the test:e2e Requires end-to-end coverage label Jul 21, 2026
@github-actions

Copy link
Copy Markdown

Label test:e2e applied for e333f14. Open Branch E2E Checks, find the run for commit e333f14, and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

…sponse

The proxy now includes a reason field in the JSON body of denied
CONNECT responses when the policy engine provides a specific denial
cause. Update the Proxy Error Responses section to show the field
and describe when it is present vs omitted.

Signed-off-by: Adel Zaalouk <azaalouk@redhat.com>
@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

PR Review Status

Validation: This PR is project-valid because it is a small, concentrated proxy bug fix linked to #2355 and addresses a user-visible sandbox policy debugging gap.
Head SHA: 785d231546ad98fee9ef0e92424c7263191bc84a

Review findings:

  • No blocking findings remain. The previous docs blocker is resolved by the docs/observability/logging.mdx update.
  • Non-blocking: the independent review still suggests adding an integration-style CONNECT policy-denial regression test that exercises the proxy path and asserts a non-empty OPA deny reason reaches the wire response, rather than only testing the response helper.

Docs: updated for the direct sandbox/proxy behavior change; no navigation update is needed for this existing docs page.

Next state: gator:watch-pipeline

@johntmyers johntmyers added gator:watch-pipeline Gator is monitoring PR CI/CD status and removed gator:in-review Gator is reviewing or awaiting PR review feedback labels Jul 21, 2026
@johntmyers

Copy link
Copy Markdown
Collaborator

/ok to test 785d231

@johntmyers johntmyers added gator:approval-needed Gator completed review; maintainer approval needed and removed gator:watch-pipeline Gator is monitoring PR CI/CD status labels Jul 21, 2026
@johntmyers
johntmyers added this pull request to the merge queue Jul 21, 2026
@johntmyers johntmyers added gator:merge-ready gator:watch-pipeline Gator is monitoring PR CI/CD status and removed gator:approval-needed Gator completed review; maintainer approval needed gator:merge-ready labels Jul 21, 2026
Merged via the queue into NVIDIA:main with commit 472e23f Jul 21, 2026
57 of 61 checks passed
@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

Monitoring Complete

Monitoring is complete because this PR has merged.

Final status: latest gator state was gator:watch-pipeline; review feedback was resolved, test:e2e was applied, maintainer approval was present, and the PR merged at head 785d231546ad98fee9ef0e92424c7263191bc84a.

I removed the active gator:* label because there is nothing left for gator to monitor on this PR.

@johntmyers johntmyers removed the gator:watch-pipeline Gator is monitoring PR CI/CD status label Jul 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(proxy): CONNECT deny response hides binary mismatch reason

2 participants