fix(push): harden SSRF (CGNAT/benchmark + create-time URL reject) - #394
Open
SashaMIT wants to merge 3 commits into
Open
fix(push): harden SSRF (CGNAT/benchmark + create-time URL reject)#394SashaMIT wants to merge 3 commits into
SashaMIT wants to merge 3 commits into
Conversation
net.IP.IsPrivate covers RFC 1918/4193 only. Also reject RFC 6598 100.64/10 and RFC 2544 198.18/15 so push webhooks cannot target those non-global spaces (aligned with Python ipaddress non-global floor).
Fail closed on non-http(s) and private/loopback literals when storing push configs (CreateTaskPushConfig and SendMessage embed). Dial-time SSRF remains; StoreConfig.AllowPrivateNetworks opts out for trusted internal use. Parity with a2a-python/js create-time guards.
SashaMIT
changed the title
| return err | ||
| } | ||
| scheme := strings.ToLower(u.Scheme) | ||
| if scheme != "http" && scheme != "https" { |
Member
There was a problem hiding this comment.
this should not be done. nothing in the spec says pushes must be done over http(s). In an internal deployment I might have topic://name push configs for automatic mq event publishing. http push sender should just ignore non-http configs
Author
There was a problem hiding this comment.
You're right. The spec does not require HTTP, and topic:// is a real internal config.
Create/save now keeps non-http(s) URLs. HTTPPushSender skips them instead of POSTing. Private/loopback reject still applies to http(s) only.
448c527
Create/save no longer requires http(s). The spec does not, and internal deployments use schemes like topic:// for MQ. HTTPPushSender skips those URLs instead of POSTing. Private/loopback reject still applies to http(s) only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
isBlockedIP(parity with Pythonipaddress.is_private).CreateTaskPushConfigand SendMessage embed). Dial-time guard remains for DNS rebinding.push.StoreConfig.AllowPrivateNetworksopts out for trusted internal deployments (paired with senderAllowPrivateNetworks).Threat model: a caller who can create/embed push configs can already choose the webhook URL; dial-time already blocks private POSTs. This is fail-fast write-path + scheme reject (parity with a2a-python create-time / a2a-js#645), not a new authority class.
Test plan
go test ./a2asrv/push/ ./a2asrv/ -count=1AllowPrivateNetworkson the store