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

Add support for CIDR notation in RemoteIpFilter #632

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fabien-chebel
Copy link

Context

Tomcat's RemoteIpFilter currently allows configuring trusted/internal proxies using regexp.

When integrating with reverse proxies with a large number of IP addresses, regexp configuration gets cumbersome.

Suggestion

I suggest adding support for IP ranges in CIDR notation to make it easier to setup the filter in these cases.

For backward compatibility, matching with masks is only performed when the trusted/internal proxies patterns are null.

Depending on the feedback I receive on this PR, I may add the same changes to Tomcat's RemoteIpValve.

# Context

Tomcat's `RemoteIpFilter` currently allows configuring trusted/internal proxies using regexp.

When integrating with reverse proxies with a large number of IP addresses, regexp configuration gets cumbersome.

# Suggestion

I suggest adding support for IP ranges in CIDR notation to make it easier to setup the filter in these cases.

For backward compatibility, matching with masks is only performed when the trusted/internal proxies patterns are null.

Depending on the feedback I receive on this PR, I may add the same changes to Tomcat's `RemoteIpValve`.
@rainerjung
Copy link
Contributor

I have not looked at your implementation suggestion, but did you notice there is:

https://tomcat.apache.org/tomcat-9.0-doc/config/filter.html#Remote_CIDR_Filter

@rainerjung
Copy link
Contributor

Ah, sorry I just realized what you plan. Please ignore my reference to RemoteCIDRFilter.

@markt-asf
Copy link
Contributor

This PR should use NetMaskSet as well.

@aooohan
Copy link
Member

aooohan commented Oct 26, 2023

Hi, If you want this PR to merge smoothly, please modify this change according to markt‘s comment.

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.

4 participants