Skip to content

fix(security): block Azure platform callback target - #147

Open
jason-allen-oneal wants to merge 1 commit into
openclaw:mainfrom
jason-allen-oneal:security/callback-azure-platform-vip
Open

fix(security): block Azure platform callback target#147
jason-allen-oneal wants to merge 1 commit into
openclaw:mainfrom
jason-allen-oneal:security/callback-azure-platform-vip

Conversation

@jason-allen-oneal

@jason-allen-oneal jason-allen-oneal commented Aug 3, 2026

Copy link
Copy Markdown

What Problem This Solves

Follow-up to #145. The callback address classifier still treated
168.63.129.16 as a public destination. Azure reserves that public-looking
virtual IP for platform services inside every virtual network, so callbacks
must not be allowed to reach it from the ClickClack host.

What Changed

  • Block 168.63.129.16/32 in the shared callback egress policy.
  • Cover literal classification and rejection before any socket dial.
  • Verify both registered slash-command and event-subscription delivery paths
    fail closed for the platform VIP.

Verification

  • go test ./... -count=1
  • go vet ./...
  • pnpm fmt:go:check
  • Focused classifier, pre-dial, slash-delivery, and event-delivery regressions

Real behavior proof

Behavior addressed: A registered slash-command or event-subscription
callback must reject Azure's 168.63.129.16 platform VIP before opening an
outbound connection.

Real environment tested: PR HEAD
b3f58ec245128b7056d55e0d3b8c2fe5052612b3, built with Go 1.26.5 and run as
the real ClickClack API with SQLite and development bootstrap enabled. The API
ran at 168.63.129.17 and a Python HTTP sentinel ran at 168.63.129.16 on an
isolated Docker bridge.

Exact steps run after this patch:

git rev-parse HEAD
proof_dir=$(mktemp -d)
mkdir -p "$proof_dir/data"
docker run --rm -v "$PWD:/src:ro" -v "$proof_dir:/out" -w /src \
  golang:1.26.5 go build -buildvcs=false -trimpath \
  -o /out/clickclack ./apps/api/cmd/clickclack

docker network create --driver bridge --internal \
  --subnet 168.63.129.0/24 --gateway 168.63.129.1 cc-pr147-proof
docker run -d --name cc-pr147-sentinel --network cc-pr147-proof \
  --ip 168.63.129.16 golang:1.26.5 python3 -u -c $'import http.server\nclass Handler(http.server.BaseHTTPRequestHandler):\n    def do_GET(self):\n        self.send_response(200); self.end_headers()\n    def do_POST(self):\n        self.send_response(204); self.end_headers()\nclass Server(http.server.ThreadingHTTPServer):\n    def get_request(self):\n        sock, addr = super().get_request()\n        print(f"TCP_ACCEPT {addr[0]}:{addr[1]}", flush=True)\n        return sock, addr\nServer(("0.0.0.0", 80), Handler).serve_forever()'
docker run -d --name cc-pr147-api --network cc-pr147-proof \
  --ip 168.63.129.17 -v "$proof_dir/clickclack:/usr/local/bin/clickclack:ro" \
  -v "$proof_dir/data:/data" golang:1.26.5 \
  /usr/local/bin/clickclack serve --addr 0.0.0.0:8080 \
  --data /data --dev-bootstrap=true
docker inspect -f '{{.Name}} {{range .NetworkSettings.Networks}}{{.IPAddress}}{{end}}' \
  cc-pr147-api cc-pr147-sentinel

api=http://127.0.0.1:8080
wsp=$(docker exec cc-pr147-api curl -fsS "$api/api/workspaces" | jq -r '.workspaces[0].id')
chn=$(docker exec cc-pr147-api curl -fsS "$api/api/workspaces/$wsp/channels" | jq -r '.channels[0].id')
bot_json=$(docker exec cc-pr147-api curl -fsS -X POST "$api/api/workspaces/$wsp/bots" \
  -H 'Content-Type: application/json' \
  --data '{"display_name":"PR 147 Proof Bot","handle":"pr-147-proof","token_name":"proof","scopes":["bot:read"]}')
bot=$(jq -r '.bot.id' <<<"$bot_json")
install_json=$(docker exec cc-pr147-api curl -fsS -X POST \
  "$api/api/workspaces/$wsp/app-installations" -H 'Content-Type: application/json' \
  --data "{\"app_slug\":\"pr-147-proof\",\"display_name\":\"PR 147 Proof\",\"bot_user_id\":\"$bot\",\"config\":{\"default_channel_id\":\"$chn\"}}")
install=$(jq -r '.app_installation.id' <<<"$install_json")
slash_json=$(docker exec cc-pr147-api curl -fsS -X POST \
  "$api/api/workspaces/$wsp/slash-commands" -H 'Content-Type: application/json' \
  --data "{\"app_installation_id\":\"$install\",\"command\":\"/vip-proof\",\"description\":\"PR 147 runtime proof\",\"callback_url\":\"http://168.63.129.16/slash\",\"bot_user_id\":\"$bot\"}")
sub_json=$(docker exec cc-pr147-api curl -fsS -X POST \
  "$api/api/workspaces/$wsp/event-subscriptions" -H 'Content-Type: application/json' \
  --data "{\"app_installation_id\":\"$install\",\"event_types\":[\"message.created\"],\"callback_url\":\"http://168.63.129.16/events\"}")
sub=$(jq -r '.event_subscription.id' <<<"$sub_json")

# Positive reachability control from the actual API container.
docker exec cc-pr147-api curl -sS -o /dev/null -w '%{http_code}\n' \
  http://168.63.129.16/

# Exercise both registered callback workflows through the real API.
docker exec cc-pr147-api curl -sS -X POST "$api/api/hooks/slash/$chn" \
  -H 'Content-Type: application/x-www-form-urlencoded' \
  --data-urlencode 'command=/vip-proof' --data-urlencode 'text=runtime-boundary-check'
docker exec cc-pr147-api curl -fsS -X POST "$api/api/channels/$chn/messages" \
  -H 'Content-Type: application/json' \
  --data '{"body":"PR 147 event callback runtime boundary check"}'
docker exec cc-pr147-api curl -fsS \
  "$api/api/event-subscriptions/$sub/deliveries" | jq '.deliveries[0] | {response_status,error}'
docker logs cc-pr147-sentinel

docker run --rm -v "$PWD:/src:ro" -w /src golang:1.26.5 \
  go test -buildvcs=false ./apps/api/internal/httpapi \
  -run 'TestCallback(AddressPolicy|DialerRejectsAzurePlatformVIPBeforeDial|DeliveryBlocksAzurePlatformVIPForBothCallbackTypes)$' \
  -count=1 -v

The registration and invocation requests used ClickClack's real HTTP API. All
one-time signing secrets and bot credentials were excluded from the captured
output.

Evidence after fix:

HEAD
b3f58ec245128b7056d55e0d3b8c2fe5052612b3

NETWORK CONTROL
ClickClack API: 168.63.129.17
HTTP sentinel:  168.63.129.16
control_http=200 remote=168.63.129.16 connect=0.000259s total=0.001575s

SLASH CALLBACK
status=502
error=Post "http://168.63.129.16/slash": callback host resolves to a non-public address

EVENT TRIGGER
event_type=message.created

EVENT DELIVERY RECORD
response_status=0
error=Post "http://168.63.129.16/events": callback host resolves to a non-public address

ACCEPT-LEVEL SENTIN LOG AFTER BOTH CALLBACKS
"GET / HTTP/1.1" 200
TCP_ACCEPT 168.63.129.17:43316
# Exactly one TCP connection: the explicit control. Neither callback connected.

FOCUSED REGRESSIONS
--- PASS: TestCallbackAddressPolicy (0.00s)
--- PASS: TestCallbackDialerRejectsAzurePlatformVIPBeforeDial (0.00s)
--- PASS: TestCallbackDeliveryBlocksAzurePlatformVIPForBothCallbackTypes (0.00s)
PASS

Observed result after fix: The control request proves that
168.63.129.16:80 was reachable from the real ClickClack runtime. The slash
workflow returned the pre-dial policy error, the event workflow persisted the
same policy error with no HTTP response status, and the accept-level sentinel
recorded exactly the explicit control connection with no connection from
either callback. The focused pre-dial regression also passed with a dial
function that fails the test if called. Together, the real runtime and no-dial
evidence show both workflows reject the VIP before opening a callback
connection.

What was not tested: This disposable proof did not run inside an Azure VNet
or contact Azure platform services. It exercised the exact literal platform
VIP through both real callback workflows. Hostname resolution and TLS callback
behavior were not part of this /32 policy change.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Aug 3, 2026
@clawsweeper

clawsweeper Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed August 3, 2026, 3:22 PM ET / 19:22 UTC.

ClawSweeper review

What this changes

The PR blocks 168.63.129.16 from signed slash-command and event-subscription callback delivery, with literal classification, pre-dial, and both-workflow regression coverage.

Merge readiness

⚠️ Ready for maintainer review - 3 items remain

This PR remains necessary: current main does not block the Azure platform callback address, and the two-file patch cleanly extends the shared callback egress boundary with direct no-dial coverage. No patch defect was found; merge should wait only for maintainer confirmation that the intentional compatibility break is within the accepted strict callback policy.

Priority: P1
Reviewed head: b3f58ec245128b7056d55e0d3b8c2fe5052612b3
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦞 diamond lobster (5/6) This is a focused, proof-positive security follow-up with no discrete patch defect found; only the maintainer-owned compatibility decision remains.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (logs): The PR body provides redacted real-runtime Docker evidence: a control request reached the sentinel while both real callback workflows returned the policy error before the sentinel accepted a connection.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (logs): The PR body provides redacted real-runtime Docker evidence: a control request reached the sentinel while both real callback workflows returned the policy error before the sentinel accepted a connection.
Evidence reviewed 6 items Current main still permits this literal target: The current callback policy rejects non-public and enumerated special-purpose addresses, but its blocked-prefix list has no 168.63.129.16/32 entry. The dialer accepts a literal address only after this shared policy check, so current source leaves the reported target outside the deny list.
Both callback workflows use the shared policy path: Event-subscription and slash-command delivery both submit requests through callbackClient; changing the central address policy covers both paths without duplicating delivery behavior.
Patch is narrow and conflict-free against current main: The branch adds one /32 policy entry and 53 test lines. A three-way merge against current main produced merged results for both touched files with no conflict.
Findings None None.
Security None None.

How this fits together

ClickClack sends signed HTTP callbacks for slash commands and event subscriptions using a shared outbound client. That client parses or resolves a configured callback target, checks it against the public-destination policy, and only then opens a connection.

flowchart LR
  A[Registered callback URL] --> B[Shared callback client]
  B --> C[Literal parser or DNS lookup]
  C --> D[Public destination policy]
  D -->|allowed| E[Outbound HTTP connection]
  D -->|blocked| F[Callback delivery error]
  E --> G[Slash or event endpoint]
Loading

Decision needed

Question Recommendation
Should the accepted public-only callback policy also reject Azure’s provider-reserved virtual IP 168.63.129.16, intentionally breaking any existing callback configured to that address? Accept the stricter callback boundary: Approve blocking the Azure platform VIP as a callback target and merge the focused policy and regression changes.

Why: The patch is technically narrow and has direct runtime proof, but maintainers must decide whether this provider-specific public-looking address belongs inside the permanent callback security boundary despite the compatibility impact.

Before merge

  • Resolve merge risk (P1) - Merging deliberately causes existing callbacks configured to 168.63.129.16 to fail before connection; that upgrade-visible compatibility break needs explicit maintainer acceptance even though the code follows the existing strict-policy design.
  • Complete next step (P1) - The remaining action is explicit maintainer acceptance of the provider-specific compatibility break; no mechanical repair is needed.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus test delta production +3, tests +53 The small policy change is backed by targeted classifier, no-dial, and callback-workflow coverage.
Affected workflows 2 callback producers covered Both slash-command and event-subscription delivery use the shared callback client and receive direct regression coverage.

Merge-risk options

Maintainer options:

  1. Confirm strict policy and merge (recommended)
    Accept the explicit break for callbacks targeting the Azure platform VIP, then land this bounded deny-list entry and its regression tests.
  2. Pause for callback-policy scope
    Keep the current behavior until maintainers decide whether provider-reserved public-looking targets should be enumerated in the shared policy.

Technical review

Best possible solution:

Adopt the Azure platform VIP as a prohibited callback destination, keep the focused pre-dial and two-workflow regressions, and record the intended fail-closed upgrade behavior in the PR discussion rather than adding a weaker fallback.

Do we have a high-confidence way to reproduce the issue?

Yes, with high confidence from source: current main sends literal targets through the shared address policy, whose blocked prefixes do not include this address. The PR adds direct literal, no-dial, slash, and event-delivery checks for the gap.

Is this the best way to solve the issue?

Yes technically: a single shared deny-list entry preserves identical behavior across both callback producers and the added tests verify rejection before dialing. Maintainer intent is still needed for the compatibility boundary, not for the implementation mechanics.

AGENTS.md: found, but no applicable review policy affected this item.

Codex review notes: model internal, reasoning high; reviewed against 601224ddee75.

Labels

Label justifications:

  • P1: The PR closes an outbound callback security gap in two active integration-delivery workflows.
  • merge-risk: 🚨 compatibility: Existing callback registrations using this address will fail before connection after upgrade.
  • merge-risk: 🚨 security-boundary: The diff expands the outbound callback destination deny policy for a provider-reserved address.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (logs): The PR body provides redacted real-runtime Docker evidence: a control request reached the sentinel while both real callback workflows returned the policy error before the sentinel accepted a connection.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides redacted real-runtime Docker evidence: a control request reached the sentinel while both real callback workflows returned the policy error before the sentinel accepted a connection.

Evidence

What I checked:

  • Current main still permits this literal target: The current callback policy rejects non-public and enumerated special-purpose addresses, but its blocked-prefix list has no 168.63.129.16/32 entry. The dialer accepts a literal address only after this shared policy check, so current source leaves the reported target outside the deny list. (apps/api/internal/httpapi/callback_http.go:17, 601224ddee75)
  • Both callback workflows use the shared policy path: Event-subscription and slash-command delivery both submit requests through callbackClient; changing the central address policy covers both paths without duplicating delivery behavior. (apps/api/internal/httpapi/features.go:1745, 601224ddee75)
  • Patch is narrow and conflict-free against current main: The branch adds one /32 policy entry and 53 test lines. A three-way merge against current main produced merged results for both touched files with no conflict. (apps/api/internal/httpapi/callback_http.go:20, b3f58ec24512)
  • Current callback boundary dates to the merged security work: Blame attributes the policy list and its special-purpose-range rationale to the merged callback-security change, which introduced the strict public-destination boundary this PR extends. (apps/api/internal/httpapi/callback_http.go:17, f62c1709f867)
  • Branch tests match the callback boundary: The proposed test additions classify the literal as blocked, fail a literal dial before DNS or socket activity, and ensure both slash and event callback delivery paths do not dial the address. (apps/api/internal/httpapi/integration_security_test.go:157, b3f58ec24512)
  • Repository policy was checked: The full repository policy only governs sqlc-generated SQL changes; this Go-only callback-policy and test patch does not enter that surface. (AGENTS.md:1, 601224ddee75)

Likely related people:

  • steipete: The merged security change introduced and currently owns the callback public-destination policy that this focused deny-list entry extends. (role: introduced the current callback security boundary; confidence: high; commits: f62c1709f867; files: apps/api/internal/httpapi/callback_http.go, apps/api/internal/httpapi/features.go, apps/api/internal/httpapi/integration_security_test.go)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (3 earlier review cycles)
  • reviewed 2026-08-03T15:37:08.180Z sha b3f58ec :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-03T19:12:50.154Z sha b3f58ec :: needs maintainer review before merge. :: none
  • reviewed 2026-08-03T19:17:57.143Z sha b3f58ec :: needs maintainer review before merge. :: none

Copy link
Copy Markdown
Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P1 Urgent regression or broken agent/channel workflow affecting real users now. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant