Refactor example-e2e test #19

M4tteoP · 2022-10-03T11:00:40Z

Follows #12:

Merges the e2e and example logic providing a list of requests that can be both executed manually and run as e2e/inside the CI.
Adds some logic handling the response body intervention: response body is dropped if a deny action is triggered

This reverts commit bc55253.

README.md

example/e2e-example.sh

jcchavezs · 2022-10-03T19:31:55Z

example/envoy-config.yaml

@@ -3,23 +3,26 @@ static_resources:
    - address:
        socket_address:
          address: 0.0.0.0
-          port_value: 8001
+          port_value: 80


port 80 might not be the best choice for an example. One usually finds it occupied.

I was mainly taking into account the usage of the example via docker-compose. In that case we will use ports 8080 and 8081 respectively for envoy and httpbin. Is it better to take into account also a standalone usage of the envoy config file?

jcchavezs · 2022-10-03T19:32:16Z

example/envoy-config.yaml

      filter_chains:
        - filters:
            - name: envoy.filters.network.http_connection_manager
              typed_config:
                "@type": type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
                stat_prefix: ingress_http
                codec_type: auto
+                http_protocol_options:
+                  accept_http_10: true


Shall we add a comment on why this is needed?

I removed this option from the example and left a commit on the config for ftw.

M4tteoP · 2022-10-03T20:10:58Z

main.go

+		if err != nil {
+			proxywasm.LogCriticalf("failed to perform interruption on response body: %v", err)
+		}
+		// TODO(M4tteop): I'm looking for the Go version of StopIterationNoBuffer


Do you have any advice about it? I don't feel calling SendHttpResponse twice is really a safe option even if it is working as I wish (close the connection even if the client is still waiting for the body).

Update in order to give a bit more context: the WAF has not detected any malicious pattern up to the response body, so, in order to avoid leakage, I just want to drop the connection and send back an empty body.

Looking at https://github.com/proxy-wasm/proxy-wasm-cpp-sdk/blob/master/proxy_wasm_enums.h#L37-L42 and https://github.com/tetratelabs/proxy-wasm-go-sdk/blob/main/proxywasm/types/types.go#L22-L27 I guess there was a reason to not to mimic outputs and also an alternative to not to buffer the response body so maybe @mathetake can provide some context.

good q, I just wanted to simplify the API using the same status, rather than complex enums for each call back type (header, trailer, body/data, etc) as in C++.

I understand that you want to proactively drop the body, but in anyway the upcoming remaining body will be discarded immediately after the request fails, so do we REALLY want it?

mathetake · 2022-10-03T23:42:48Z

main.go

+			proxywasm.LogCriticalf("failed to perform interruption on response body: %v", err)
+		}
+		// TODO(M4tteop): I'm looking for the Go version of StopIterationNoBuffer
+		// It is formally not correct to invoke SendHttpResponse during OnHttpResponseBody, but it is correctly closing the connection.


what does this mean? connection of what? what if it is http2?

what I wanted to say is that with the current impl of envoy, it's impossible to make the request fail after you return Continue in OnHttpResponseHeader proxy-wasm/proxy-wasm-cpp-host#143

that's the reason why we had the internal fork of cpp-host to change this behavior done by Takaya

Thanks for confirming @mathetake. There are many proxy-wasm tasks we need to follow up on and finally have some budget for that so we'll try to make some progress there.

@M4tteoP In the meantime, it would be good to separate PRs for refactoring the example and response body logic. If you want to do the latter first, then my recommendation is to move ProcessResponseHeaders to OnHttpResponseBody). I can't find any coreruleset rules targeting response headers and this fits my intuition that there wouldn't be much to match there (maybe response cookies? But there isn't even a variable for that like there is for request cookies). There are a few response body rules though, so if we remove interruption in response headers we can still sanitize the response body when either interrupts there. What do you think?

mathetake · 2022-10-03T23:49:24Z

main.go

+		err = proxywasm.ReplaceHttpResponseBody([]byte(``))
+		if err != nil {
+			proxywasm.LogCriticalf("failed to perform interruption on response body: %v", err)


replacing with an empty buffer doesn't release anything in the host - it's just taking another string_view on the same memory region so this is just doing nothing.

Suggested change

err = proxywasm.ReplaceHttpResponseBody([]byte(``))

if err != nil {

proxywasm.LogCriticalf("failed to perform interruption on response body: %v", err)

M4tteoP · 2022-10-04T16:09:53Z

The conversation about response body logic continues in #26

M4tteoP · 2022-10-06T12:26:51Z

If it sounds good, I propose to merge this PR addressing only the refactor of the e2e, temporary commenting tests about the response body. They will be added again as soon as we agree on a short-term solution #26.

e2e/Dockerfile

jcchavezs · 2022-10-06T18:54:48Z

example/docker-compose.yml

    volumes:
      - logs:/home/envoy/logs:ro
+  tests:


I think you don't need this container for the example, the only reason this container is here, if I recall correctly is because you can curl the envoy directly from CI? If that is the case I recommend you create another docker-compose file in the e2e folder to include the script. See https://stackoverflow.com/questions/55650342/import-docker-compose-file-in-another-compose-file

On top of that, here I find a circular dependency: e2e depends on example and example depends on e2e (see https://github.com/corazawaf/coraza-proxy-wasm/pull/19/files#diff-fe02bc0074dfa40a0e1b60098921a60d187efae125046c3f008a5cdfa62cf028R60)

jcchavezs · 2022-10-07T09:23:55Z

magefile.go

 func E2e() error {
-	return sh.RunV("docker-compose", "--file", "e2e/docker-compose.yml", "up", "--abort-on-container-exit")
+	return sh.RunV("docker-compose", "-f", "e2e/docker-compose.yml", "up", "--abort-on-container-exit", "tests")


Does this rely on the example (example/docker-compose.yml) to be up and running already? If that is the case I'd recommend you bootstrap services on both files but in any case the command should not require you to do a previous command. See https://docs.docker.com/compose/extends/

Another option is of course to launch the example before the e2e

mage e2e does not rely anymore on example/docker-compose.yml. I thought that a standalone and straightforward docker-compose was a cleaner alternative.
(Mainly had troubles with extends because of services with 'depends_on' cannot be extended).

M4tteoP added 6 commits September 30, 2022 12:48

first Readme version

d9707e4

adds example

a3db851

updates coraza, updates readme with example instructions

047efd7

go mod tidy

fba2979

adds readme-tests, CI and minor readme refactor

6e8a0d5

refactor example-e2e, adds response body logic

e5d04ac

M4tteoP requested review from anuraaga and jcchavezs as code owners October 3, 2022 11:00

M4tteoP mentioned this pull request Oct 3, 2022

Adds example #12

Closed

M4tteoP changed the title ~~Refactor e2e response body logic, response body logic~~ Refactor e2en, response body logic Oct 3, 2022

tmp rename

bc55253

M4tteoP changed the title ~~Refactor e2en, response body logic~~ Refactor example-e2e test, response body logic Oct 3, 2022

M4tteoP added 7 commits October 3, 2022 13:23

Revert "tmp rename"

fc02282

This reverts commit bc55253.

Merge branch 'main' into refactor_e2e_response_body_logic

e608dad

address conflicts

c19f0bd

address conflicts go.sum

ce6f252

fixing go.mod go.sum

2da78cb

tmp add e2e folder

aafeb91

Merge branch 'main' into refactor_e2e_response_body_logic

daba132

M4tteoP marked this pull request as draft October 3, 2022 12:46

jcchavezs reviewed Oct 3, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

jcchavezs reviewed Oct 3, 2022

View reviewed changes

example/e2e-example.sh Outdated Show resolved Hide resolved

jcchavezs reviewed Oct 3, 2022

View reviewed changes

M4tteoP commented Oct 3, 2022

View reviewed changes

relocating e2e script, minor readme

c61ed5c

mathetake reviewed Oct 3, 2022

View reviewed changes

This was referenced Oct 4, 2022

Intervention action at response body phase. #26

Open

Response body logic #27

Closed

M4tteoP added 7 commits October 6, 2022 11:01

addressing conflicts

69d2218

Merge branch 'main' into refactor_e2e_response_body_logic

bb5318f

wip, not addressing response body logic

9c61c31

excluding response body logic

01af699

lint

4365406

tweaking ports

21bd8e7

tweaking ports 2

5ede2b6

M4tteoP marked this pull request as ready for review October 6, 2022 12:28

M4tteoP changed the title ~~Refactor example-e2e test, response body logic~~ Refactor example-e2e test, ~~response body logic~~ Oct 6, 2022

M4tteoP changed the title ~~Refactor example-e2e test, ~~response body logic~~~~ Refactor example-e2e test Oct 6, 2022

jcchavezs reviewed Oct 6, 2022

View reviewed changes

e2e/Dockerfile Outdated Show resolved Hide resolved

jcchavezs reviewed Oct 6, 2022

View reviewed changes

e2e/Dockerfile Outdated Show resolved Hide resolved

jcchavezs reviewed Oct 6, 2022

View reviewed changes

e2e/Dockerfile Outdated Show resolved Hide resolved

jcchavezs reviewed Oct 6, 2022

View reviewed changes

M4tteoP added 3 commits October 7, 2022 01:13

splitting example and e2e docker compose

22606eb

e2e compose minimal file

e2ae164

adding context to docker compose

f5bf1e2

jcchavezs reviewed Oct 7, 2022

View reviewed changes

jcchavezs approved these changes Oct 7, 2022

View reviewed changes

jcchavezs merged commit 6d0de78 into corazawaf:main Oct 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor example-e2e test #19

Refactor example-e2e test #19

M4tteoP commented Oct 3, 2022 •

edited

Loading

jcchavezs Oct 3, 2022

M4tteoP Oct 3, 2022

jcchavezs Oct 3, 2022

M4tteoP Oct 6, 2022

M4tteoP Oct 3, 2022 •

edited

Loading

jcchavezs Oct 3, 2022

mathetake Oct 3, 2022

mathetake Oct 3, 2022 •

edited

Loading

mathetake Oct 3, 2022

mathetake Oct 3, 2022

anuraaga Oct 4, 2022

mathetake Oct 3, 2022

M4tteoP commented Oct 4, 2022 •

edited

Loading

M4tteoP commented Oct 6, 2022

jcchavezs Oct 6, 2022

jcchavezs Oct 7, 2022

jcchavezs Oct 7, 2022

M4tteoP Oct 7, 2022

	err = proxywasm.ReplaceHttpResponseBody([]byte(``))
	if err != nil {
	proxywasm.LogCriticalf("failed to perform interruption on response body: %v", err)

Refactor example-e2e test #19

Refactor example-e2e test #19

Conversation

M4tteoP commented Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M4tteoP Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathetake Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M4tteoP commented Oct 4, 2022 • edited Loading

M4tteoP commented Oct 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M4tteoP commented Oct 3, 2022 •

edited

Loading

M4tteoP Oct 3, 2022 •

edited

Loading

mathetake Oct 3, 2022 •

edited

Loading

M4tteoP commented Oct 4, 2022 •

edited

Loading