Skip to content
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

Forwarded HTTP header #1350 #1629

Merged
merged 5 commits into from
Jul 5, 2022

Conversation

const-t
Copy link
Contributor

@const-t const-t commented May 17, 2022

What has been implemented:

  • Parsing of "Forwarded" header. "for=" parameter parsed, but
    not used now. Only xff continues to be used. Also "Forwarded"
    not modified when forwarding to backend.
  • Choosing the right host by using keyword "host" in HTTPTables.
    • Highest priority has "URI", then "Host" and "Forwarded"
  • Added tests for validity of parsing and choosing host.

Copy link
Contributor

@krizhanovsky krizhanovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@const-t thank you for your contribution! Quite a solid work, especially in the most complicated part, the parser.

May I ask you to make several improvements and simplifications along with the coding style cleanups? Some of the comments might need a discussion, especially the parser DSL macros.

Also I'd love to hear from you by e-mail ak @ tempesta-tech.com or a call https://calendly.com/tempesta-tech/60min : if you might be interested in job opportunities in our team or any other collaboration, then let's talk!

fw/http_match.c Show resolved Hide resolved
fw/http_match.c Outdated Show resolved Hide resolved
fw/http_match.c Show resolved Hide resolved
fw/http_match.c Show resolved Hide resolved
fw/http_match.c Outdated Show resolved Hide resolved
fw/http_parser.c Show resolved Hide resolved
fw/http_parser.c Outdated Show resolved Hide resolved
fw/http_parser.c Outdated Show resolved Hide resolved
fw/http_parser.c Outdated Show resolved Hide resolved
fw/http_parser.c Outdated Show resolved Hide resolved
Copy link
Contributor

@krizhanovsky krizhanovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still several minor changes are required. Please make them in well-splitted and -documented patches for quicker review

fw/http_limits.c Outdated Show resolved Hide resolved
fw/http_limits.c Show resolved Hide resolved
fw/http_limits.c Outdated Show resolved Hide resolved
fw/http_limits.c Outdated Show resolved Hide resolved
fw/http_match.c Outdated Show resolved Hide resolved
fw/http_parser.c Outdated Show resolved Hide resolved
fw/http_parser.c Outdated Show resolved Hide resolved
fw/http_parser.c Outdated Show resolved Hide resolved
fw/http_parser.c Outdated Show resolved Hide resolved
fw/http_parser.c Show resolved Hide resolved
Copy link
Contributor

@krizhanovsky krizhanovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just please squash at least all coding style cleanup changes and maybe the iterations on the parsing function.

@pale-emperor pale-emperor reopened this Jul 4, 2022
@const-t const-t force-pushed the ct-1350-forwarded-hdr branch 3 times, most recently from 082e551 to 18ee508 Compare July 5, 2022 06:55
const-t and others added 5 commits July 5, 2022 10:53
What has been implemented:
 - Parsing of "Forwarded" header. "for=" parameter parsed, but
   not used now. Only xff continues to be used. Also "Forwarded"
   not modified when forwarding to backend.
 - Choosing the right host by using keyword "host" in HTTPTables.
   - Highest priority has "URI", then "Host" and "Forwarded"
 - Added tests for validity of parsing and choosing host.
 - Added matching host of forwarded by http_host_required.
 -http_limits: Removed garbage and simplified conditions
 -http_match: Removed line breaks
 -http_parser: Removed unnecessary line breaks. Fixed misaligned \
 -http_parser: __(h2_)req_parse_forwarded() comments cleaned and added
__builtin_uaddl_overflow() in __parse_ulong() might write larger than 2 bytes
to port before "if" checks the limit. At the moment, thank to data alignment,
we have extra 2 bytes in the union, but this might change and we'll get
a vulnerability.
  -Edited large comment about __msg_field_open and explicit fixup.
  -Added macroses to __(h2)_req_parse_forwarded that allows to use
   __msg_field_open in forwarded parsing. Manual manipulation with
   "p" pointer replaced by these macroses.
@const-t const-t merged commit cf86049 into tempesta-tech:master Jul 5, 2022
@const-t const-t deleted the ct-1350-forwarded-hdr branch July 5, 2022 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants