Skip to content

listener: validate source IP addresses#4466

Closed
cmluciano wants to merge 2 commits intoenvoyproxy:masterfrom
cmluciano:cml/sourceipmatch
Closed

listener: validate source IP addresses#4466
cmluciano wants to merge 2 commits intoenvoyproxy:masterfrom
cmluciano:cml/sourceipmatch

Conversation

@cmluciano
Copy link
Copy Markdown
Member

Signed-off-by: Christopher M. Luciano cmluciano@us.ibm.com

Description: listener: validate source IP addresses
Risk Level: Low
Testing: new tests added & bazel test //test/server:listener_manager_impl_test
Docs Changes: WIP
Release Notes: WIP
Fixes #4457

Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>
@cmluciano
Copy link
Copy Markdown
Member Author

cc @junr03

still working on adding more tests to verify that this is working correctly

@htuch htuch requested a review from junr03 September 20, 2018 03:05
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Is this just the very first step towards adding source IP matching? Seems almost too small for a PR, but then again, I never say no to small PRs :)

std::vector<std::string> source_ips;
for (const auto& source_ip : filter_chain_match.source_prefix_ranges()) {
const auto& cidr_range = Network::Address::CidrRange::create(source_ip);
source_ips.push_back(cidr_range.asString());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why are we saving these to a vector?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is carryover from how it is implemented in prefix_ranges . Is there a more suitable type that I should go with?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess it's hard to see how things will be used without a fuller PR, doesn't seem unreasonable..

@cmluciano
Copy link
Copy Markdown
Member Author

cmluciano commented Sep 20, 2018

@htuch Yes I just started by searching for where we are currently using prefix_ranges and prepending source_prefix_ranges to a new function.

Update: I found the other pieces that need changed and am working on them now.

@junr03
Copy link
Copy Markdown
Member

junr03 commented Sep 24, 2018

@cmluciano 👌 lmk when the fuller PR is up and I will review. Thanks!

Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>
@cmluciano
Copy link
Copy Markdown
Member Author

closing for now so that I don't generate unnecessary builds/spam

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