Support RFC 7239: Forwarded HTTP Extension #55

yadij · 2017-08-31T17:08:51Z

This adds the delete and transparent options as configurable for
this extension header in accordance with the RFC. Unlike XFF
these operations are enabled in all builds and transparent
operation is default. For now adding values still requires
custom request_header_add config.

rousskov

I am sorry, but I do not think this PR should be merged. Here are some of the high-level problems I see, in no particular order:

The PR adds no new functionality. The "delete" action is already available via request_header_access. The "transparent" action is the default Squid behavior.
The PR places configuration handling code in src/http/. As you know, I do not think that is a good design because configuration code eventually makes protocol-focused libraries like libhttp depend on "a lot of high-level stuff", causing linking problems and complicating understanding of the overall code architecture/library boundaries.
Initial support for Forwarded should IMHO protect the header from being TRACEd back to the client. See RFC 7239 section 8.2.
No justification provided for adding a new configuration directive instead of (renaming and) enhancing the old and popular forwarded_for directive which controls very similar functionality. I do not know what the best approach here is, but my knee jerk reaction would be to reuse and enhance the existing directive (which the admins have already used to express their desires regarding client information forwarding) instead of adding a new competing directive.

Let's discuss (on squid-dev, IRC?) how to add support for Forwarded before rushing to coding initial support.

yadij · 2017-09-01T16:55:32Z

For 1 - this is the initial step to make the simple bits configurable and preview the design direction.

For 2 - there is no better design in Squid today and creating a fourth Squid-wide config parser is out of scope here.

For 3 - The broken response behaviour is already being caused by the existing header registration. Not added by anything here. Though true it is not being completely fixed by this PR either.

For 4 - I went with a new directive because this is more of an alternative feature admin have to migrate to selectively when all their software chain supports it than an evolutionary upgrade to XFF. The changed default action, integration of client anonymity, per-parameter configuration requirements and auto-converting of received XFF are all subtly different or cant be passed to legacy XFF software down the chain.
The final version of the new directive might be able to absorb the old directive, but admin will probably need them different to handle the headers independently.

I will l try to be on IRC a bit more the next few days and we can go over design plans.

rousskov · 2017-09-01T17:08:44Z

For the record, I disagree with the first three of the above four arguments. For item 4, more discussion/specifics are needed for me to finalize my opinion. Let's discuss on IRC.

rousskov

This is a partial review. I left a few code-specific change requests as well, but I recommend starting with addressing higher-level design issues (that will naturally impact code).

rousskov · 2024-01-30T21:41:26Z

doc/release-notes/release-7.sgml.in

+	<p>Option <em>delete</em> removes both Forwarded and
+	   X-Forwarded-For headers from traffic.
+	<p>Option <em>transparent</em> relays any Forwarded and
+	   X-Forwarded-For headers without changes.
+	<p>Option <em>truncate</em> removes any existing
+	   Forwarded and X-Forwarded-For headers, then adds
+	   a new X-Forwarded-For header with Client IP.
+	<p>Deprecated Option <em>off</em> removes Forwarded headers and
+	   appends 'unknown' to X-Forwarded-For.
+	<p>Deprecated Option <em>on</em> (the default) removes Forwarded
+	   headers and appends Client IP to X-Forwarded-For.


Please do not repeat squid.conf.documented documentation here, so that we do not have to keep both snippets in sync. Release notes are meant for summarizing important changes and detailing upgrade caveats, not for documenting new directive details.

Suggested change

Option delete removes both Forwarded and

X-Forwarded-For headers from traffic.

Option transparent relays any Forwarded and

X-Forwarded-For headers without changes.

Option truncate removes any existing

Forwarded and X-Forwarded-For headers, then adds

a new X-Forwarded-For header with Client IP.

Deprecated Option off removes Forwarded headers and

appends 'unknown' to X-Forwarded-For.

Deprecated Option on (the default) removes Forwarded

headers and appends Client IP to X-Forwarded-For.

This change request and the above suggestion do not imply that these release notes should never talk about deprecated "off" and "on" settings. IMHO, the best place to talk about that is in the "Replaced by ..." text further below, where we should discuss how folks are supposed to upgrade their existing configurations.

These are the changes to previous Squid UI behaviour that are new in this PR, as required for UI change documentation.

These are the changes to previous Squid UI behaviour that are new in this PR, as required for UI change documentation.

These problematic paragraphs are documenting a lot more than changes and making it difficult to figure out what has actually changed. They are focused on documenting the details of how things work and needlessly repeat directive documentation. There is no requirement to repeat squid.conf.documented documentation in release notes. Release notes should flag/summarize changes and, where not obvious, explain their motivation, effects, and/or upgrade steps, all without duplicating directive documentation. Directive documentation lives elsewhere.

src/cache_cf.cc

rousskov · 2024-01-31T14:42:18Z

doc/release-notes/release-7.sgml.in

+
+   request_header_add Forwarded "for=%>a" !ipv6 !internal
+   request_header_add Forwarded "for=[%>a]" ipv6 !internal
+</verb>


Asking admins to use two rules (one for IPv4 and one for IPv6) to add a single XFF/Forwarded header is not acceptable IMO.

Several changes¹ can address this concern, but I believe that the overall best way forward here (that addresses this and several other concerns -- I stopped explicitly flagging them at some point) is to go back to the current official design where a dedicated directive (other than generic request_header_add) is responsible for forming sent client IP values (when they are sent). In this context, Squid should treat all sent XFF and Forwarded header fields as a single configurable entity rather than a hodgepodge of items assembled in semi-independent ways, especially since items order matters and correct formatting of individual items is tricky (including being dependent on difficult XFF upgrade decisions).

For example (just to illustrate the concept, I am not insisting on any specific option set or spelling at this time):

forward_client_addresses from_received_headers=<on|off> from_client_connection=<on|off> upgrade_xff=<on|off> format=... ...

This PR may not have to implement all the possible/useful options, of course, but it has to create the right framework for follow up PRs while implementing the necessary minimum.

Footnotes

For example, a prerequisite PR can introduce new %code formatting flag(s) that will result in %>a (when used with those new flag(s)) expanding to a quoted bracketed string for IPv6 addresses and unquoted string for IPv4 addresses. ↩

This is not a new requirement, we just have not documented it until now.

Recall that you previously required this PR scope reduction: - to omit the logic for adding any Forwarded header automatically.

Alex: Asking admins to use two rules (one for IPv4 and one for IPv6) to add a single XFF/Forwarded header is not acceptable IMO.

Amos: This is not a new requirement, we just have not documented it until now.

I disagree that this requirement is old. To quote the original change request, "... current official design where a dedicated directive (other than generic request_header_add) is responsible for forming sent client IP values (when they are sent)". Official code does not have this requirement. It is new.

doc/release-notes/release-7.sgml.in

rousskov · 2024-01-31T15:07:19Z

doc/release-notes/release-7.sgml.in

+   Unlike <em>X-Forwarded-For</em> the default is not to expose client IP in the
+   <em>Forwarded</em>.


Headers themselves do not have defaults. We need to explicitly say (somewhere in these release notes) that we are changing default Squid behavior (from adding an XFF header field with the client IP address to not adding an XFF or Forwarded header field with the client IP address, even though the received XFF/Forwarded fields are still forwarded by default).

I'm not sure what you are talking about here.

I have re-worded anyway in dfcb005, please check that the new text is okay.

Alex: We need to explicitly say (somewhere in these release notes) that we are changing default Squid behavior (from adding an XFF header field with the client IP address to not adding an XFF or Forwarded header field with the client IP address, even though the received XFF/Forwarded fields are still forwarded by default).

Amos: please check that the new text is okay.

It is not: I still do not see the corresponding explicit statement about this change in release notes. The current notes can be interpreted to imply that, but many readers will miss that important implication. We need to explicitly say that we are changing the default (from ... to ...). Explicitly highlighting that change is materially/significantly different from documenting the new default (as current notes do in "The default Squid behaviour is now ...").

rousskov · 2024-01-31T15:09:33Z

doc/release-notes/release-7.sgml.in

 	<ref id="mgr" name="section"> for details.

+	<tag>forwarded_for</tag>
+	<p>Replaced by <em>http_header_forwarded</em> directive.


AFAICT, the current PR does not "replace" the old directive. Both new and old directives are supported in PR code.

As far as admin can see the cache manager report and config parser messages, will no longer display the old directive names (except the parser to highlight that the directive has changed).

IMO this wording is better since the behaviour and defaults have changed. If it was just a directive rename I would have used "Renamed to/from" in the respective sections and not documented all the behavioural differences.

Let's be explicit about it instead of hoping that the reader will know what we meant. For example: "Deprecated and superseded by ...".

rousskov · 2024-01-31T15:19:11Z

src/cf.data.pre

 DOC_END

-NAME: forwarded_for
+NAME: http_header_forwarded forwarded_for


I am not yet convinced that we need to rename this directive. Why is renaming better than keeping the current name? Please share your cost/benefit analysis.

FWIW, if we do end up renaming, I am pretty sure that http_header_forwarded is not a good name. Something like forward_client_addresses would be a lot better IMO.

Well this directive is more about the "HTTP header Forwarded:" than about one (of many) possible values that can be sent in that header.

Well this directive is more about the "HTTP header Forwarded:" than about one (of many) possible values that can be sent in that header.

In current PR code, this directive is more about the old X-Forwarded-For header than it is about Forwarded header. It controls X-Forwarded-For header generation. It does not control Forwarded header generation. Its only effect on Forwarded header is to prevent its forwarding (in all non-default cases). This PR does not change the scope of this directive; it deprecates old "actions", adds two new "actions", and changes the default, with all of that focusing on that old X-Forwarded-For header. In summary, the offered justification feels invalid, and I am still not convinced we need to rename this directive.

Perhaps renaming becomes a good idea when this directive does more, but then we should agree on that future before renaming, so that we do not have to rename twice.

Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>

rousskov

This is another incomplete review to facilitate progress; a lot of work remains here, including addressing several old change requests.

rousskov · 2026-01-02T15:17:07Z

src/cf.data.pre

+
+	transparent
+		Squid will send all Forwarded or X-Forwarded-For
+		headers received.


Adding "without changes" describes what happens to received headers. It is a good addition, but it does not fully address this change request concern because this change request also applies to headers generated by Squid (in addition to received headers).

rousskov · 2026-01-02T15:30:39Z

doc/release-notes/release-7.sgml.in

+	<p>Option <em>delete</em> removes both Forwarded and
+	   X-Forwarded-For headers from traffic.
+	<p>Option <em>transparent</em> relays any Forwarded and
+	   X-Forwarded-For headers without changes.
+	<p>Option <em>truncate</em> removes any existing
+	   Forwarded and X-Forwarded-For headers, then adds
+	   a new X-Forwarded-For header with Client IP.
+	<p>Deprecated Option <em>off</em> removes Forwarded headers and
+	   appends 'unknown' to X-Forwarded-For.
+	<p>Deprecated Option <em>on</em> (the default) removes Forwarded
+	   headers and appends Client IP to X-Forwarded-For.


These are the changes to previous Squid UI behaviour that are new in this PR, as required for UI change documentation.

These problematic paragraphs are documenting a lot more than changes and making it difficult to figure out what has actually changed. They are focused on documenting the details of how things work and needlessly repeat directive documentation. There is no requirement to repeat squid.conf.documented documentation in release notes. Release notes should flag/summarize changes and, where not obvious, explain their motivation, effects, and/or upgrade steps, all without duplicating directive documentation. Directive documentation lives elsewhere.

src/cache_cf.cc

rousskov · 2026-01-02T16:09:19Z

src/http.cc

+        auto strFwd = request.header.getList(Http::HdrType::X_FORWARDED_FOR);
+
+        // if we cannot double strFwd size, then it grew past 50% of the limit
+        if (!strFwd.canGrowBy(strFwd.size())) {


Please address the essence of this change request rather than ignoring it because you disagree with informal code name/label used by the change request! I will rephrase using your name/label:

Please preserve official "String crashes" prevention logic when moving that official code here. Changing that tricky logic is outside this PR scope AFAICT.

rousskov · 2026-01-02T16:19:58Z

src/http.cc

+        // send what was received, with 'unknown' appended
+        auto strFwd = request.header.getList(Http::HdrType::X_FORWARDED_FOR);
+
+        // if we cannot double strFwd size, then it grew past 50% of the limit


Ah, tricky indeed - its not duplicated even if the one line and comments are.

This change request applies to all duplicated code in this area, including that "one line and comment" code. In current PR code, the following code appears at least twice (i.e. it is duplicated):

// if we cannot double strFwd size, then it grew past 50% of the limit if (!strFwd.canGrowBy(strFwd.size())) { // There is probably a forwarding loop with Via detection disabled. // If we do nothing, String will assert on overflow soon. // TODO: Terminate all transactions with huge XFF? strFwd = "error"; static FadingCounter warnedCount; if (warnedCount.count(1) < 100) debugs(11, DBG_IMPORTANT, "WARNING: likely forwarding loop with " << request.effectiveRequestUri()); }

rousskov · 2026-01-02T22:13:10Z

src/http/ExtForwarded.cc

+        debugs(3, DBG_PARSE_NOTE(1), "WARNING: UPGRADE: default action 'transparent' does not need to be configured.");
+        mode = Mode::fwdTransparent;
+    }
+}


Please reject invalid actions instead of leaving mode uninitialized.

rousskov · 2026-01-02T22:14:03Z

src/http/ExtForwarded.cc

+        debugs(3, DBG_PARSE_NOTE(1), "WARNING: UPGRADE: default action 'transparent' does not need to be configured.");
+        mode = Mode::fwdTransparent;
+    }
+}


Please reject multiple actions instead of ignoring all but the first one.

rousskov · 2026-01-02T22:18:09Z

src/http/ExtForwarded.cc

+        os << " transparent";
+        break;
+
+    // deprecated modes.


Please adjust PR metadata and code to use the same term for the directive parameter. I currently see "mode", "action", "option", and "operation". That is way too many inconsistencies! I can suggest settling on "action".

rousskov · 2026-01-02T22:24:36Z

src/http/ExtForwarded.cc

+    // XXX: Configuration::Component API cannot print the correct squid.conf text for these.
+    // XXX: should be printed with the deprecated 'forwarded_for ' directive name


Please remove these XXXs: Squid should not do extra work to remember and report deprecated directive names when directives get renamed. Backlogged changes allow Squid to report configuration as it was written by the admin, but not reporting that way is not a Configuration::Component API bug, and is not a bug at all.

rousskov · 2026-01-02T22:28:15Z

src/cf.data.pre

 COMMENT: on|off|transparent|truncate|delete
-TYPE: string
-DEFAULT: on
-LOC: opt_forwarded_for


This PR adds no new functionality with respect to Forwarded header¹, ignores RFC 7239 "Information Leak" section, and yet its title makes a "Support RFC 7239" claim. We should relax or remove that claim. Overall, this PR is not really about that RFC and that header AFAICT.

Footnotes

The "delete" action is already available via request_header_access. The "transparent" action is the old default Squid behavior for that Forwarded header. ↩

rousskov requested changes Sep 1, 2017

View reviewed changes

This comment was marked as resolved.

Sign in to view

rousskov added the S-waiting-for-author author action is expected (and usually required) label Sep 14, 2017

yadij force-pushed the v5-forwarded branch from 0fd5b26 to 3f3093a Compare September 21, 2017 14:11

squid-anubis added the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Feb 5, 2018

squid-anubis removed the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label May 11, 2018

yadij force-pushed the v5-forwarded branch from 3f3093a to b355f89 Compare May 11, 2018 09:03

squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Mar 20, 2019

squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Apr 11, 2019

squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Apr 26, 2019

squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Jun 3, 2019

yadij added the feature maintainer needs documentation updates for merge label Dec 23, 2019

squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Mar 15, 2020

yadij force-pushed the v5-forwarded branch from 53ae146 to c3128de Compare June 1, 2020 13:11

squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Jun 2, 2020

yadij removed the feature maintainer needs documentation updates for merge label Aug 9, 2020

squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Feb 14, 2021

squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Feb 28, 2021

yadij force-pushed the v5-forwarded branch 3 times, most recently from c20720d to 76e30a8 Compare January 26, 2024 21:18

yadij requested a review from rousskov January 26, 2024 21:20

yadij added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Jan 26, 2024

rousskov requested changes Jan 31, 2024

View reviewed changes

rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Jan 31, 2024

kinkie added the M-ignored-by-merge-bots https://github.com/measurement-factory/anubis/blob/master/README.md#pull-request-labels label Feb 14, 2024

yadij force-pushed the v5-forwarded branch from ab87f28 to 07f9c8f Compare March 14, 2025 19:47

RFC 7239 - Forwarded header initial support

da44a6d

yadij force-pushed the v5-forwarded branch from 07f9c8f to da44a6d Compare August 6, 2025 02:42

yadij and others added 5 commits August 6, 2025 19:10

Use 'Configuration API' required by reviewer

6938b1e

Update src/http.cc

4c25e09

Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>

Update src/cf.data.pre

4c78e1e

Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>

Apply suggestions from code review

cd7b5c9

Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>

Update release-7.sgml.in

dfcb005

yadij changed the title ~~RFC 7239 Forwarded HTTP Extension~~ Support RFC 7239: Forwarded HTTP Extension Aug 6, 2025

yadij added 4 commits August 6, 2025 20:58

Review requested changes

976da29

Preserve true transparency on 'transparent'

93ded7c

maintenance

61120fc

Template instantiations cannot be inlined

afd3e53

yadij requested a review from rousskov August 8, 2025 10:02

yadij mentioned this pull request Nov 18, 2025

Update for Nettle 4.0 base64 API #2296

Open

rousskov requested changes Jan 2, 2026

View reviewed changes

rousskov removed the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Jan 2, 2026

		Unlike <em>X-Forwarded-For</em> the default is not to expose client IP in the
		<em>Forwarded</em>.

		// XXX: Configuration::Component API cannot print the correct squid.conf text for these.
		// XXX: should be printed with the deprecated 'forwarded_for ' directive name

Support RFC 7239: Forwarded HTTP Extension #55

Are you sure you want to change the base?

Support RFC 7239: Forwarded HTTP Extension #55

Uh oh!

Conversation

yadij commented Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rousskov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadij commented Sep 1, 2017

Uh oh!

rousskov commented Sep 1, 2017

Uh oh!

This comment was marked as resolved.

Uh oh!

rousskov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rousskov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

yadij commented Aug 31, 2017 •

edited

Loading

rousskov left a comment •

edited

Loading