Add support for CIDR ranges in ignore_hosts setting. #5099
Conversation
Signed-off-by: shikharj05 <[email protected]>
Signed-off-by: shikharj05 <[email protected]>
shikharj05
requested review from
cwperks,
DarshitChanpura,
derek-ho,
nibix,
peternied,
RyanL1997,
reta and
willyborankin
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5099 +/- ##
=======================================
Coverage 71.55% 71.56%
=======================================
Files 336 336
Lines 22649 22667 +18
Branches 3603 3604 +1
=======================================
+ Hits 16206 16221 +15
- Misses 4641 4643 +2
- Partials 1802 1803 +1
|
|
Thank you for this PR @shikharj05. We will also want to update the documentation accordingly.
^ Looks like these cases are already being handled as well |
src/main/java/org/opensearch/security/auth/limiting/AbstractRateLimiter.java
Show resolved
Hide resolved
|
|
||
| import org.opensearch.security.support.WildcardMatcher; | ||
| import org.opensearch.security.user.AuthCredentials; | ||
|
|
||
| public interface AuthFailureListener { | ||
| List<String> getIgnoreHosts(); |
There was a problem hiding this comment.
There may be some overlap between this and the WildcardMatcher down below. Do you think we should remove the one below on favor of a single List<String> getIgnoreHosts(); which covers both static IPs and CIDR blocks?
Ack, will create a separate PR for docs. |
| @@ -66,6 +77,16 @@ public WildcardMatcher getIgnoreHostsMatcher() { | |||
| return hostMatcher; | |||
| } | |||
|
|
|||
| @Override | |||
| public SubnetUtils.SubnetInfo getSubnetForCidr(String cidr) { | |||
| return SUBNET_CACHE.computeIfAbsent(cidr, pattern -> { | |||
There was a problem hiding this comment.
Why do we need a cache here? Can't the SubnetUtils instances be created upon initialization of an instance of this class (or a subclass)?
| @@ -46,6 +54,7 @@ public final class SecurityUtils { | |||
| static final Pattern ENVBC_PATTERN = Pattern.compile("\\$\\{envbc" + ENV_PATTERN_SUFFIX); | |||
| static final Pattern ENVBASE64_PATTERN = Pattern.compile("\\$\\{envbase64" + ENV_PATTERN_SUFFIX); | |||
| public static Locale EN_Locale = forEN(); | |||
| private static final Map<String, SubnetInfo> cidrCache = new ConcurrentHashMap<>(); | |||
There was a problem hiding this comment.
Why do we need a cache on top on the cache in AbstractRateLimiter?
| } | ||
| AuthFailureListener authFailureListener = (AuthFailureListener) clientBlockRegistry; | ||
| String hostAddress = address.getHostAddress(); | ||
| return authFailureListener.getIgnoreHosts().parallelStream().filter(pattern -> pattern.indexOf('/') != -1).anyMatch(pattern -> { |
There was a problem hiding this comment.
Is using a parallelStream really justified here? AFAIK, it should be rather used in situations where the per-item operation clearly outweighs the cost of the thread management. I have doubts whether CIDR matching is really so heavy.
See: https://www.baeldung.com/java-when-to-use-parallel-stream#1-the-overhead
Description
This change adds support to specify CIDR ranges in ignore_hosts settings.
While ignore_hosts currently supports specifying IP addresses and hostnames, it would be good to support adding CIDR ranges as well. For example, see comment here- [Feature Request] Request to Document Behaviour Change in Unauthenticated Request Handling in OpenSearch 2.11.0 or later #4927 (comment)
Adding support for CIDR ranges in
ignore_hostsIssues Resolved
#4927
Is this a backport? If so, please add backport PR # and/or commits #, and remove
backport-failedlabel from the original PR.Do these changes introduce new permission(s) to be displayed in the static dropdown on the front-end? If so, please open a draft PR in the security dashboards plugin and link the draft PR here
Testing
[Please provide details of testing done: unit testing, integration testing and manual testing]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.