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

Handle lists with and without spaces in XFF #3610

Merged
merged 5 commits into from Jun 13, 2018

Conversation

Projects
None yet
3 participants
@rgs1
Copy link
Contributor

rgs1 commented Jun 12, 2018

Currently, Utility::getLastAddressFromXFF() only handles lists that
use a comma plus a space as the separator.

Per https://tools.ietf.org/html/rfc7239#section-7.1, spaces are allowed
around commas — so we should handle that too.

This fixes #3607.

Signed-off-by: Raul Gutierrez Segales rgs@pinterest.com

Handle lists with and without spaces in XFF
Currently, `Utility::getLastAddressFromXFF()` only handles lists that
use a comma plus a space as the separator.

Per https://tools.ietf.org/html/rfc7239#section-7.1, spaces are allowed
around commas — so we should handle that too.

This fixes #3607.

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@mattklein123

This comment has been minimized.

Copy link
Member

mattklein123 commented Jun 12, 2018

@rgs1 we are going to merge the revert to try to get master into a good state after some issues. Can you potentially re-apply #3609 in this PR and then @alyssawilk can help review?

@rgs1

This comment has been minimized.

Copy link
Contributor

rgs1 commented Jun 12, 2018

@mattklein123 sure, but do we really need to revert #3609? it's generating valid XFF headers...

@mattklein123

This comment has been minimized.

Copy link
Member

mattklein123 commented Jun 12, 2018

It is, but when Envoys are paired together, it will no longer work correctly, and I'm sure this will break people in a mesh configuration (including Lyft). I think it's better to revert and reapply as a set.

@rgs1

This comment has been minimized.

Copy link
Contributor

rgs1 commented Jun 12, 2018

@mattklein123 ah true

@rgs1

This comment has been minimized.

Copy link
Contributor

rgs1 commented Jun 12, 2018

i'll add #3609 in a follow-up commit

@alyssawilk

This comment has been minimized.

Copy link
Contributor

alyssawilk commented Jun 12, 2018

Having the utility fix in the same PR as the change to headers doesn't actually help if you don't roll everything out at once.

I'd honestly be inclined to roll back #3609, check this in, and wait [a month | a full release] before reapplying given there's 0 urgency on removing the whitespace. Alternately we could call it out much more clearly in the release notes that you must replace your upstream Envoys (which allow whitespace and no-whitespace) before replacing downstream Envoys (which change how the header looks) but that seems more dicey.

@alyssawilk
Copy link
Contributor

alyssawilk left a comment

Either way this LGTM (modulo nit) - thanks for both debugging and sorting out a fix so quickly!

@@ -303,6 +303,10 @@ Utility::getLastAddressFromXFF(const Http::HeaderMap& request_headers, uint32_t
xff_string = xff_string.substr(last_comma + seperator.size());
}

// ignore the whitespace, since they are allowed in HTTP lists (see rfc7239)

This comment has been minimized.

@alyssawilk

alyssawilk Jun 12, 2018

Contributor

nit: ignore -> Ignore

Fix comment.
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@mattklein123

This comment has been minimized.

Copy link
Member

mattklein123 commented Jun 12, 2018

I'd honestly be inclined to roll back #3609, check this in, and wait [a month | a full release] before reapplying given there's 0 urgency on removing the whitespace. Alternately we could call it out much more clearly in the release notes that you must replace your upstream Envoys (which allow whitespace and no-whitespace) before replacing downstream Envoys (which change how the header looks) but that seems more dicey.

This is a good point. Sorry, didn't think about this very much when I mentioned to roll it together. Agre let's apply this first then wait a month or so for the other one. Can we open a tracking issue for that?

@mattklein123
Copy link
Member

mattklein123 left a comment

Nice, thank you!

@@ -303,6 +303,10 @@ Utility::getLastAddressFromXFF(const Http::HeaderMap& request_headers, uint32_t
xff_string = xff_string.substr(last_comma + seperator.size());
}

// Ignore the whitespace, since they are allowed in HTTP lists (see RFC7239#section-7.1).
xff_string = StringUtil::ltrim(xff_string);
xff_string = StringUtil::rtrim(xff_string);

This comment has been minimized.

@mattklein123

mattklein123 Jun 12, 2018

Member

Sorry, just noticed. Do we have a test that covers the need for rtrim?

This comment has been minimized.

@rgs1

rgs1 Jun 12, 2018

Contributor

@mattklein123 no... only ltrim(). Lemme modify the case I added so that it has space at both sides...

This comment has been minimized.

@rgs1

rgs1 Jun 12, 2018

Contributor

@mattklein123 is it fine if I use this test string which should exercise ltrim(), rtrim() and no trim():

"192.0.2.10, 192.0.2.1 ,10.0.0.1"

Or you rather have it broken up in subtests, and be explicit about what path i am exercising?

This comment has been minimized.

@rgs1

rgs1 Jun 12, 2018

Contributor

i guess the above string with comments near each EXPECT call should be clear enough... Let me push what I have and lmk what you think.

This comment has been minimized.

@rgs1

rgs1 Jun 12, 2018

Contributor

@mattklein123 hold it, my comments are in the wrong place, fixing...

This comment has been minimized.

@rgs1

rgs1 Jun 12, 2018

Contributor

@mattklein123 should be good now, sorry about that!

another question

Test rtrim() too.
Also, comment about which space trimming case is being tested.

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@mattklein123
Copy link
Member

mattklein123 left a comment

great, thanks!

@mattklein123 mattklein123 self-assigned this Jun 12, 2018

Fix comments.
Comments were referring to the wrong test case.

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

// No space trimming.
ret = Utility::getLastAddressFromXFF(request_headers, 2);
EXPECT_EQ(first_address, ret.address_->ip()->addressAsString());

This comment has been minimized.

@alyssawilk

alyssawilk Jun 13, 2018

Contributor

I think this test would have passed with the previous PR, because we assumed "X, Y" The problem is X,Y with no whitespace on either side.

Can we add one more address in this test with no whitespace before the comma on either side?

Test address with no space on the left or right.
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@rgs1

This comment has been minimized.

Copy link
Contributor

rgs1 commented Jun 13, 2018

@alyssawilk added — thanks!

@rgs1

This comment has been minimized.

Copy link
Contributor

rgs1 commented Jun 13, 2018

@alyssawilk hmm, do we also need an integration test to check that xff is handled correctly from envoy to envoy (e.g.: something that would have caught the problem with 035a816)?

@alyssawilk

This comment has been minimized.

Copy link
Contributor

alyssawilk commented Jun 13, 2018

I'd love that, but I don't think we have the ability to set up multi-Envoy integration tests. It's definitely on my list though! I think the best we can do today is an integration test for A,B,C (no whitespace) if you're up for it.

@mattklein123

This comment has been minimized.

Copy link
Member

mattklein123 commented Jun 13, 2018

Ah thanks @alyssawilk nice catch on the test.

@mattklein123 mattklein123 merged commit 9d3a4eb into envoyproxy:master Jun 13, 2018

12 checks passed

DCO All commits have a DCO sign-off from the author
Details
ci/circleci: api Your tests passed on CircleCI!
Details
ci/circleci: asan Your tests passed on CircleCI!
Details
ci/circleci: build_image Your tests passed on CircleCI!
Details
ci/circleci: coverage Your tests passed on CircleCI!
Details
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: filter_example_mirror Your tests passed on CircleCI!
Details
ci/circleci: format Your tests passed on CircleCI!
Details
ci/circleci: ipv6_tests Your tests passed on CircleCI!
Details
ci/circleci: mac Your tests passed on CircleCI!
Details
ci/circleci: release Your tests passed on CircleCI!
Details
ci/circleci: tsan Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment