-
-
Notifications
You must be signed in to change notification settings - Fork 407
test: default to HTTP/1.1 protocol version for all tests instead of HTTP/1.0 #4043
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: default to HTTP/1.1 protocol version for all tests instead of HTTP/1.0 #4043
Conversation
…TTP/1.0 tests which explicitly test wrong or not allowed versions are not changed. (in REQUEST-920-PROTOCOL-ENFORCEMENT/920430.yaml and REQUEST-920-PROTOCOL-ENFORCEMENT/920280.yaml).
Hi @daum3ns, thanks for this PR - unfortunately there is failed test in case of both engines' regression test. Have you tested these modifications with Apache2 and Nginx before you sent this PR? |
i am currently looking into it, its the same test for both engines that fails.. |
Ok, I figured it out. In HTTP/1.0 an empty host header is allowed, and therefore the rule is expected to block the request in the test.. @airween What do you think? |
Yes, changing the protocol version in that case is a good idea. Probably you might add some extra comment here. |
I would rather change the way we deal with versions than simply changing the default. For most tests we expect them to work for any HTTP version and we should really test that that assumption is true. I propose to update go-ftw, and possibly the test schema, to run each test against multiple HTTP versions, unless a test defines a restriction. I also think we should transform the HTTP version restrictions into ranges, such that we can say:
|
I do not get the reasoning behind your proposal @theseion. Could you elaborate? |
Updating from HTTP/1.0 to HTTP/1.1 is fine. However:
Does that answer your question @dune73? |
Got you. As of this writing we do not know if the tests work with HTTP/1.1 and HTTP/2 etc. Your proposal expands the coverage across various protocols. That sounds good. Yet, I wonder if this is really necessary for every test. I see a lot of tests with |
I see two different discussions here. One related to this PR, which I think we will be able to merge without further changes first. Then we should open a new issue in go-ftw and handle the discussion here around http versions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Created issue here: coreruleset/go-ftw#480 |
tests which explicitly test wrong or not allowed versions are not changed. (in REQUEST-920-PROTOCOL-ENFORCEMENT/920430.yaml and REQUEST-920-PROTOCOL-ENFORCEMENT/920280.yaml).