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

Change Reserved to unmodified HTTP header-field #2466

Merged

Conversation

matyasmarkovics
Copy link
Contributor

Propagate unmodified header fields in the previously Reserved and
undefined 4th element of the http_header tuple.

Why do we need this new feature?

While Section 4.2 of RFC 2616 for HTTP/1.1 states
that Field names are case-insensitive, some non-compliant
web services may rely on peculiar casing of Field names.
Such web-services cannot be accessed through a Proxy-server,
that is implemented in Erlang/OTP, certainly not over HTTPS.

Despite the RFC, even the IANA lists a number of permanently
registered headers
in all-caps or mixed-cased format,
e.g.: ALPN, HTTP2-Settings, WWW-Authenticate, etc.
This leads to confusion and to service implementations,
that only accept headers formatted the same way.
A production issue was experienced using the common, drafted,
but not yet standard header: DNT.

There are a number of HTTP servers implemented in Erlang
that could be used in a Proxy implementation. However,
there is none that would respect header-casing.

  • cowboy, httpd - their header parser lower-cases the fields
  • yaws - emulators' internal HTTP header parsing via ssl/inet
  • mochiweb, elli - both use erlang:decode_packet/3

As a side note: An HTTPS-Proxy requires CONNECT method support.
Therefor cowboy is not a real option despite its popularity.

With this proposal yaws, mochiweb and elli could all be patched
to respect casing of header fields and propagate the original
header format to the application level modules.
The DNT header issue was experienced using a patched mochiweb.

Risks or uncertain artifacts?

These changes are minimalistic and backward compatible,
given application developers respected the documentation.
All above mentioned http servers do so, they ignore the 4th
element of http_header tuple. It was verified in the master
branches of the project repositories.

How did you solve it?

The proposal is that packet_parser.c should also propagate
unmodified fields to both inet_drv.c and erl_bif_port.c,
where these would be put in the 4th place of the http_header
tuple. Its recognised, that this element was Reserved
for future and/or internal use. Hopefully you'd agree, that
sending the original header fields is a productive use-case
to free up the reservation.

Adding a new httph_cs option, cs for case-sensitive, was also
considered. I got the impression thought, that these these
packet decoders exit for compatibility reasons only and
the OTP-Team is not keen on adding new options.
That said I'm happy to add a httph_cs and http_bin_cs,
if that is what is need for the merge to happen.

PS: A related Rejected PR from a colleague.

Propagate unmodified header fields in the previously Reserved and
undefined 4th element of the http_header tuple.

*Why do we need this new feature?*

While Section 4.2 of RFC 2616 for HTTP/1.1 states
that Field names are case-insensitive, some non-compliant
web services may rely on peculiar casing of Field names.
Such web-services cannot be accessed through a Proxy-server,
that is implemented in Erlang/OTP, certainly not over HTTPS.

Despite the RFC, even the IANA lists a number of permanently
registered headers in all-caps or mixed-cased format,
e.g.: ALPN, HTTP2-Settings, WWW-Authenticate, etc.
This leads to confusion and to service implementations,
that only accept headers formatted the same way.
A production issue was experienced using the common, drafted,
but not yet standard header: DNT.

There are a number of HTTP servers implemented in Erlang
that could be used in a Proxy implementation. However,
there is none that would respect header-casing.
* cowboy, httpd - their header parser lower-cases the fields
* yaws - emulators' internal HTTP header parsing via ssl/inet
* mochiweb, elli - both use erlang:decode_packet/3

As a side note: An HTTPS-Proxy requires CONNECT method support.
Therefor cowboy is not a real option despite its popularity.

With this proposal yaws, mochiweb and elli could all be patched
to respect casing of header fields and propagate the original
header format to the application level modules.
The DNT header issue was experienced using a patched mochiweb.

*Risks or uncertain artifacts?*

These changes are minimalistic and backward compatible,
given application developers respected the documentation.
All above mentioned http servers do so, they ignore the 4th
element of http_header tuple. It was verified in the master
branches of the project repositories.

*How did you solve it?*

The proposal is that packet_parser.c should also propagate
unmodified fields to both inet_drv.c and erl_bif_port.c,
where these would be put in the 4th place of the http_header
tuple. Its recognised, that this element was Reserved
for future and/or internal use. Hopefully you'd agree, that
sending the original header fields is a productive use-case
to free up the reservation.

Adding a new httph_cs option, cs for case-sensitive, was also
considered. I got the impression thought, that these these
packet decoders exit for compatibility reasons only and
the OTP-Team is not keen on adding new options.
@CLAassistant
Copy link

CLAassistant commented Nov 25, 2019

CLA assistant check
All committers have signed the CLA.

@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Nov 25, 2019
@garazdawi
Copy link
Contributor

Yes, this is a much better approach. It just needs some updates to the docs and should be good.

@garazdawi garazdawi added feature waiting waiting for changes/input from author labels Nov 25, 2019
@matyasmarkovics
Copy link
Contributor Author

Thanks for the feedback @garazdawi. Both ssl and inet app. docs refer back to the documentation of erlang:decode_packet/3, so I've amended that one only. Please let me know if you think updates needed elsewhere. I have looked but couldn't find other relevant places to update.

@matyasmarkovics
Copy link
Contributor Author

While I was searching through cowboy's commit history to create this PR's description, I've came across a comment about how decode_packet title-cases headers with double hyphens incorrectly: e.g.: Content--Length will be Content--length, so I've added a test-case for this. It's not necessarily an issue as I'm not aware of legitimate headers with such a format. However, maybe it's good to have a test, so that the casing behaviour is known.

@garazdawi
Copy link
Contributor

Please let me know if you think updates needed elsewhere.

erlang:decode_packet/3 was were I meant, so this is fine.

However, maybe it's good to have a test, so that the casing behaviour is known.

Agreed. Good initiative, thanks.

Putting it in testing over the weekend.

@garazdawi garazdawi added testing currently being tested, tag is used by OTP internal CI and removed waiting waiting for changes/input from author labels Nov 28, 2019
@garazdawi garazdawi removed the testing currently being tested, tag is used by OTP internal CI label Dec 6, 2019
@garazdawi garazdawi added the testing currently being tested, tag is used by OTP internal CI label Dec 6, 2019
@garazdawi garazdawi merged commit 8a7d17f into erlang:master Dec 9, 2019
@matyasmarkovics
Copy link
Contributor Author

Hey @garazdawi , thanks for merging this in master.

  • Am I correct in assuming that, these changes would be included in the next major release, ie.: OTP-23?
  • Do you see any chances to have the changes back-ported to OTP-22? If so, anything I can do to initiate that process? :)

@garazdawi
Copy link
Contributor

Do you see any chances to have the changes back-ported to OTP-22?

There will only be a couple of months difference between the 22.3 release and 23.0, so I think the risk outweighs the rewards. It is already too late for 22.2.

@matyasmarkovics
Copy link
Contributor Author

matyasmarkovics commented Dec 9, 2019

Do you see any chances to have the changes back-ported to OTP-22?

There will only be a couple of months difference between the 22.3 release and 23.0.

Good to hear that. Thanks!

tank-bohr added a commit to tank-bohr/bookish_spork that referenced this pull request Apr 3, 2021
- Lowercase method atoms
- Use binaries both for header names and values
- Introduce `raw_headers` which preserve headers case and order. [erlang/otp#2466](erlang/otp#2466) allows to keep headers case
- Some documentation updates
- Update deps
tank-bohr added a commit to tank-bohr/bookish_spork that referenced this pull request Apr 3, 2021
- Lowercase method atoms
- Use binaries both for header names and values
- Introduce `raw_headers` which preserve headers case and order. [erlang/otp#2466](erlang/otp#2466) allows to keep headers case
- Some documentation updates
- Update deps
tank-bohr added a commit to tank-bohr/bookish_spork that referenced this pull request Apr 3, 2021
- Lowercase method atoms
- Use binaries both for header names and values
- Introduce `raw_headers` which preserve headers case and order. [erlang/otp#2466](erlang/otp#2466) allows to keep headers case
- Some documentation updates
- Update deps
- Update CI to run tests inside docker container
tank-bohr added a commit to tank-bohr/bookish_spork that referenced this pull request Apr 3, 2021
- Lowercase method atoms
- Use binaries both for header names and values
- Introduce `raw_headers` which preserve headers case and order. [erlang/otp#2466](erlang/otp#2466) allows to keep headers case
- Some documentation updates
- Update deps
- Update CI to run tests inside docker container
tank-bohr added a commit to tank-bohr/bookish_spork that referenced this pull request Apr 3, 2021
- Lowercase method atoms
- Use binaries both for header names and values
- Introduce `raw_headers` which preserve headers case and order. [erlang/otp#2466](erlang/otp#2466) allows to keep headers case
- Some documentation updates
- Update deps
- Update CI to run tests inside docker container
tank-bohr added a commit to tank-bohr/bookish_spork that referenced this pull request Apr 3, 2021
- Lowercase method atoms
- Use binaries both for header names and values
- Introduce `raw_headers` which preserve headers case and order. [erlang/otp#2466](erlang/otp#2466) allows to keep headers case
- Some documentation updates
- Update deps
- Update CI to run tests inside docker container
tank-bohr added a commit to tank-bohr/bookish_spork that referenced this pull request Apr 3, 2021
- Lowercase method atoms
- Use binaries both for header names and values
- Introduce `raw_headers` which preserve headers case and order. [erlang/otp#2466](erlang/otp#2466) allows to keep headers case
- Some documentation updates
- Update deps
- Update CI to run tests inside docker container
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants