feat(contrib/haproxy): HAProxy AAP support#3912
Conversation
3518fb7 to
049d56e
Compare
1c79257 to
122b92f
Compare
9483abb to
6db4230
Compare
469c32d to
9ee5626
Compare
e-n-0
changed the title
github-actions
Bot
added
the
apm:ecosystem
6c40e5f to
5fce2b7
Compare
f3ac150 to
2664229
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. About Codex in GitHubYour 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, or 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 fix this CI failure" or "@codex address that feedback". |
eec6d03 to
39c6213
Compare
39c6213 to
db0cd84
Compare
| func getStringValue(msg *message.Message, key string) string { | ||
| if val, exists := msg.KV.Get(key); exists { | ||
| if str, ok := val.(string); ok { | ||
| return str | ||
| } | ||
| } | ||
| return "" | ||
| } | ||
|
|
||
| func getIntValue(msg *message.Message, key string) int { | ||
| if val, exists := msg.KV.Get(key); exists { | ||
| if i, ok := val.(int); ok { | ||
| return i | ||
| } | ||
| if i64, ok := val.(int64); ok { | ||
| return int(i64) | ||
| } | ||
| } | ||
| return 0 | ||
| } | ||
|
|
||
| func getBytesArrayValue(msg *message.Message, key string) []byte { | ||
| if val, exists := msg.KV.Get(key); exists { | ||
| if bytes, ok := val.([]byte); ok { | ||
| return bytes | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func getBoolValue(msg *message.Message, key string) bool { | ||
| if val, exists := msg.KV.Get(key); exists { | ||
| if b, ok := val.(bool); ok { | ||
| return b | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| func getIPValue(msg *message.Message, key string) net.IP { | ||
| if val, exists := msg.KV.Get(key); exists { | ||
| if ip, ok := val.(net.IP); ok { | ||
| return ip | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
In the end is this Message type even usable alone. Maybe create a wrapper type and transform those into methods so it is more convenient
| s, ok := tracer.SpanFromContext(ctx) | ||
| if !ok { | ||
| return fmt.Errorf("failed to retreive the span from the context of the request") | ||
| } | ||
|
|
||
| timeout := getStringValue(requestContextData.msg, "timeout") | ||
| requestContextData.timeout = timeout | ||
|
|
||
| spanId := s.Context().SpanID() | ||
| spanIdStr := strconv.FormatUint(spanId, 10) | ||
| requestContextData.req.Actions.SetVar(action.ScopeTransaction, "span_id", spanIdStr) |
There was a problem hiding this comment.
Why do you need to get the span id here? Can you add a comment explaining why ?
There was a problem hiding this comment.
As the HAProxy SPO protocol is working by sending messages to the agent. However there is no way to link messages to a request (this doesn't work like envoy in a bi-directional stream). The answer found to link each message was to:
- Set the span id as variable inside the answer of the request headers message
- Store the current request context linked with this span id
- In HAProy, this variable is set in the session of the request, and the span id is sent back again on each other messages related to this request
- We retrieve the cached request state on each message based on the received span id
| authority := headers.Get("Host") | ||
| method := getStringValue(m.msg, "method") | ||
| path := getStringValue(m.msg, "path") | ||
| https := getBoolValue(m.msg, "https") |
There was a problem hiding this comment.
Same here about making these strings into documented constants
| contentLength := headers.Get("Content-Length") | ||
| if contentLength != "" { |
There was a problem hiding this comment.
| contentLength := headers.Get("Content-Length") | |
| if contentLength != "" { | |
| if contentLength := headers.Get("Content-Length"); contentLength != "" { |
Using the content length to know ahead of time if there will be a body is not exactly right I believe. Tons of streaming requests don't set the content-length header and still send a body. Either in the request or the response. cf. https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Content-Length
Although it can be used to know ahead of time if the body will be too big
There was a problem hiding this comment.
As we discussed, I removed the mandatory content length check to ask for the body. Now the body is always sent if the content type is a valid body we can parse, except if the content-length is detected as 0.
See f1c9872
…ment needed interfaces to work with the mp
a7c40ec to
d3ecca8
Compare
…d on the content-length header only
| requestContextData := &haproxyRequestContextData{req: req, msg: msg} | ||
|
|
||
| switch msg.Name { | ||
| case "http-request-headers-msg": |
There was a problem hiding this comment.
I guess those could also be set as constant since they don't seem to appear as-is in the SPOP protocal but rather seems to be arbitrarily set in the HAProxy config crumbs you created
| handler, mt, cleanup := setup() | ||
| defer cleanup() | ||
|
|
||
| end2EndStreamRequest(t, handler, "/", "GET", map[string]string{"User-Agent": "Chromium", "Content-Type": "application/json"}, map[string]string{}, false, false, `{ "name": "<script>alert(1)</script>" }`, "") |
There was a problem hiding this comment.
| end2EndStreamRequest(t, handler, "/", "GET", map[string]string{"User-Agent": "Chromium", "Content-Type": "application/json"}, map[string]string{}, false, false, `{ "name": "<script>alert(1)</script>" }`, "") | |
| end2EndStreamRequest(t, handler, "/", "GET", map[string]string{"User-Agent": "Chromium", "Content-Type": "application/json"}, map[string]string{}, false, false, `{ "payload": { "name": "<script>alert(1)</script>" } }`, "") |
There was a problem hiding this comment.
Please rebase on the fix we did yesterday and make sure this test pass
| // This test is failing because the external processor is waiting for a body to run the waf on the response headers | ||
| // This scenario can happen if the processor never receive the body (due to a timeout, error in the app backend, ...) | ||
| /*t.Run("blocking-event-on-response-headers-with-body-not-sent", func(t *testing.T) { | ||
| handler, mt, cleanup := setup() | ||
| defer cleanup() | ||
|
|
||
| spanId, bodyRequested, blockedAct := sendProcessingRequestHeaders(t, handler, map[string]string{"User-Agent": "Chromium", "Content-Type": "application/json"}, "GET", "/", 0) | ||
| require.False(t, bodyRequested) | ||
| require.Nil(t, blockedAct) | ||
|
|
||
| // Send a processing response headers with the information that it would be followed by a body, but don't send the body | ||
| bodyRequested, blockedAct = sendProcessingResponseHeaders(t, handler, map[string]string{"test": "match-response-header", "Content-Type": "application/json"}, "200", spanId, 256) | ||
|
|
||
| // Res should be an immediate response with the blocking event | ||
| require.Nil(t, blockedAct) | ||
| require.False(t, bodyRequested) | ||
|
|
||
| finished := mt.FinishedSpans() | ||
| require.Len(t, finished, 1) | ||
| checkForAppsecEvent(t, finished, map[string]int{"custom-001": 1, "headers-003": 1}) | ||
|
|
||
| // Check for tags | ||
| span := finished[0] | ||
| require.Equal(t, "true", span.Tag("appsec.event")) | ||
| require.Equal(t, "true", span.Tag("appsec.blocked")) | ||
| })*/ | ||
| } |
There was a problem hiding this comment.
Can you rather skip the test than comment it? Are you sure we can't make this test pass ?
There was a problem hiding this comment.
This test scenario is a bit special. In the test we don't send the response body even if that's requested by the processor. We are simulating an error on the HAProxy side where the body is never sent.
In this case when the timeout (of the cache) triggers, the request will be closed and mark as blocked in the trace, but this won't reflect the reality, as the response to the client would have been different (but we don't receive this data).
I can make it pass, but it will validate the wrong behavior
|
/merge --delay in 10 hours |
|
View all feedbacks in Devflow UI.
Pull Request scheduled to be added to the queue on Thu, 02 Oct 2025 08:33:12 UTC
The expected merge time in
|
2 participants
What does this PR do?
This PR implements an HAProxy SPOE messages handler that can be used to create full featured AAP HAProxy agent (coming part of #3913).
This PR uses the message_processor package implemented from #3860 (superseded by #3945 and #3951) and implements its own HAProxy message processor.
This implementation use the library
github.com/negasus/haproxy-spoe-gofor the communication between haproxy and the agent (implementing the SPOP).Motivation
New proxy integration supported for AAP: HAProxy
Reviewer's Checklist
./scripts/lint.shlocally.Unsure? Have a question? Request a review!