markt-asf commented on code in PR #767:
URL: https://github.com/apache/tomcat/pull/767#discussion_r1828019091
##########
java/org/apache/catalina/filters/RateLimitFilter.java:
##########
@@ -63,6 +67,10 @@
* information that it has, e.g. allow more requests to certain users based on
roles, etc.
* </p>
* <p>
+ * The <code>exposeHeaders</code> allows output runtime information of rate
limiter via response header, is disabled by
+ * default, only for http-api or debugging purpose. Those runtime information
should not be accessible to the attackers.
+ * </p>
+ * <p>
Review Comment:
There are really two patches here. One to add the ExactRateLimiter and one
to add support for "RateLimit header fields for HTTP". I think it would be
better to do this as 2 PRs. There is a bit too much going on in this PR.
##########
java/org/apache/catalina/util/LocalStrings_zh_CN.properties:
##########
@@ -53,3 +53,4 @@ sessionIdGeneratorBase.randomAlgorithm=使用算法[{0}]初始化随机数生成
sessionIdGeneratorBase.randomProvider=使用程序提供的初始化随机数生成器异常[{0}]
timebucket.maintenance.error=定期维护时出现错误
+exactRateLimiter.maintenance.error=定期维护时出现错误
Review Comment:
Can you re-use `timebucket.maintenance.error` here?
##########
java/org/apache/catalina/util/TimeBucketCounter.java:
##########
@@ -97,23 +97,22 @@ public TimeBucketCounter(int bucketDuration,
ScheduledExecutorService executorSe
* Increments the counter for the passed identifier in the current time
bucket and returns the new value.
*
* @param identifier an identifier for which we want to maintain count,
e.g. IP Address
- *
* @return the count within the current time bucket
*/
public final int increment(String identifier) {
- String key = getCurrentBucketPrefix() + "-" + identifier;
+ String key = getBucketPrefix(System.currentTimeMillis()) + "-" +
identifier;
AtomicInteger ai = map.computeIfAbsent(key, v -> new AtomicInteger());
return ai.incrementAndGet();
}
/**
- * Calculates the current time bucket prefix by shifting bits for fast
division, e.g. shift 16 bits is the same as
+ * Calculates the time bucket prefix by shifting bits for fast division,
e.g. shift 16 bits is the same as
* dividing by 65,536 which is about 1:05m.
- *
+ * @param timestamp in millis
* @return The current bucket prefix.
*/
- public final int getCurrentBucketPrefix() {
- return (int) (System.currentTimeMillis() >> this.numBits);
+ public final int getBucketPrefix(long timestamp) {
+ return (int) (timestamp >> this.numBits);
Review Comment:
Why make this change? It appears to increase code duplication with no
obvious benefit.
##########
java/org/apache/catalina/util/ExactRateLimiter.java:
##########
@@ -0,0 +1,184 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.catalina.util;
+
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.juli.logging.Log;
+import org.apache.juli.logging.LogFactory;
+import org.apache.tomcat.util.res.StringManager;
+import org.apache.tomcat.util.threads.ScheduledThreadPoolExecutor;
+
+import jakarta.servlet.FilterConfig;
+
+/**
+ * An accurate fixed window limiter.
+ */
+public class ExactRateLimiter implements RateLimiter {
Review Comment:
It looks like there is scope for a common base class for this class and
`FastRateLimiter`
##########
java/org/apache/catalina/filters/RateLimitFilter.java:
##########
@@ -173,25 +213,49 @@ public void init(FilterConfig filterConfig) throws
ServletException {
rateLimiter.setRequests(bucketRequests);
rateLimiter.setFilterConfig(filterConfig);
+ if (policyName != null && !policyName.trim().isEmpty()) {
+ rateLimiter.setPolicyName(policyName.trim());
+ }
+
filterName = filterConfig.getFilterName();
log.info(sm.getString("rateLimitFilter.initialized", filterName,
Integer.valueOf(bucketRequests),
Integer.valueOf(bucketDuration),
Integer.valueOf(rateLimiter.getRequests()),
Integer.valueOf(rateLimiter.getDuration()), (!enforce ? "Not "
: "") + "enforcing"));
}
-
+
+ /**
+ * Returns the identifier from the servlet request, which used to work as
rate limit by.
+ * @param request
+ * @return identifier
+ */
+ protected String getLimitByIdentifier(ServletRequest request) {
+ return request.getRemoteAddr();
+ }
+
@Override
public void doFilter(ServletRequest request, ServletResponse response,
FilterChain chain)
throws IOException, ServletException {
- String ipAddr = request.getRemoteAddr();
- int reqCount = rateLimiter.increment(ipAddr);
+ String identifier = getLimitByIdentifier(request);
+ int reqCount = rateLimiter.increment(identifier);
Review Comment:
This looks like another, possibly separate, improvement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
