dpol1 commented on code in PR #1942:
URL: https://github.com/apache/stormcrawler/pull/1942#discussion_r3413490371
##########
core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java:
##########
@@ -538,6 +547,36 @@ private byte[] toByteArray(
return arr;
}
+ /**
+ * Network interceptor blocking connections to IP addresses rejected by
the configured {@link
+ * IPFilterRules}. The IP address is only known once the connection has
been established, hence
+ * the filtering happens at the protocol level rather than by filtering
URLs.
+ */
+ static class HTTPFilterIPAddressInterceptor implements Interceptor {
+
+ private final IPFilterRules rules;
+
+ HTTPFilterIPAddressInterceptor(IPFilterRules rules) {
+ this.rules = rules;
+ }
+
+ @NotNull
+ @Override
+ public Response intercept(Interceptor.Chain chain) throws IOException {
+ final Connection connection =
Objects.requireNonNull(chain.connection());
+ final InetAddress address = connection.socket().getInetAddress();
Review Comment:
docs section is good. one case it doesn't cover: with a proxy configured the
filter checks the proxy IP, not the target, so it's effectively off for proxied
fetches.
maybe add a NOTE for that?
##########
core/src/test/java/org/apache/stormcrawler/protocol/okhttp/CIDRTest.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.stormcrawler.protocol.okhttp;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import com.google.common.net.InetAddresses;
+import java.net.InetAddress;
+import org.junit.jupiter.api.Test;
+
+class CIDRTest {
+
+ private static InetAddress ip(String address) {
+ return InetAddresses.forString(address);
+ }
+
+ @Test
+ void singleIPv4AddressMatchesOnlyItself() {
+ CIDR cidr = new CIDR("127.0.0.1");
+ assertTrue(cidr.contains(ip("127.0.0.1")));
+ assertFalse(cidr.contains(ip("127.0.0.2")));
Review Comment:
same root as the `/32` bug above — this only changes the last byte, so byte
0 never gets tested.
##########
core/src/main/java/org/apache/stormcrawler/protocol/okhttp/CIDR.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.stormcrawler.protocol.okhttp;
+
+import com.google.common.net.InetAddresses;
+import java.net.InetAddress;
+
+/**
+ * Parse a <a href=
"https://en.wikipedia.org/wiki/Classless_Inter-Domain_Routing";>CIDR</a> block
+ * notation and test whether an IP address is contained in the subnet range
defined by the CIDR.
+ */
+public class CIDR {
+
+ private final InetAddress addr;
+ private final int mask;
+
+ public CIDR(InetAddress address, int mask) {
+ this.addr = address;
+ this.mask = mask;
+ }
+
+ public CIDR(String cidr) throws IllegalArgumentException {
+ String ipStr = cidr;
+ int sep = cidr.indexOf('/');
+ if (sep > -1) {
+ ipStr = cidr.substring(0, sep);
+ }
+ addr = InetAddresses.forString(ipStr);
+ int parsedMask;
+ if (sep > -1) {
+ parsedMask = Integer.parseInt(cidr.substring(sep + 1));
+ } else {
+ parsedMask = addr.getAddress().length * 8;
+ }
+ if (cidr.indexOf(':') > -1 && addr.getAddress().length == 4) {
+ // IPv4-mapped IPv6 addresses are automatically converted to IPv4,
+ // need to shift the mask
+ parsedMask = Math.max(0, parsedMask - 96);
+ }
+ this.mask = parsedMask;
Review Comment:
Mask isn't range-checked, so `10.0.0.0/-1` ends up matching everything
(`remainingMaskBits <= 0` on the first byte). worth throwing here —
`IPFilterRules` already catches `IllegalArgumentException` and skips the rule:
```suggestion
int maxMask = addr.getAddress().length * 8;
if (parsedMask < 0 || parsedMask > maxMask) {
throw new IllegalArgumentException("Invalid CIDR mask /" +
parsedMask + " for " + ipStr);
}
this.mask = parsedMask;
##########
core/src/main/java/org/apache/stormcrawler/protocol/okhttp/CIDR.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.stormcrawler.protocol.okhttp;
+
+import com.google.common.net.InetAddresses;
+import java.net.InetAddress;
+
+/**
+ * Parse a <a href=
"https://en.wikipedia.org/wiki/Classless_Inter-Domain_Routing";>CIDR</a> block
+ * notation and test whether an IP address is contained in the subnet range
defined by the CIDR.
+ */
+public class CIDR {
+
+ private final InetAddress addr;
+ private final int mask;
+
+ public CIDR(InetAddress address, int mask) {
+ this.addr = address;
+ this.mask = mask;
+ }
+
+ public CIDR(String cidr) throws IllegalArgumentException {
+ String ipStr = cidr;
+ int sep = cidr.indexOf('/');
+ if (sep > -1) {
+ ipStr = cidr.substring(0, sep);
+ }
+ addr = InetAddresses.forString(ipStr);
+ int parsedMask;
+ if (sep > -1) {
+ parsedMask = Integer.parseInt(cidr.substring(sep + 1));
+ } else {
+ parsedMask = addr.getAddress().length * 8;
+ }
+ if (cidr.indexOf(':') > -1 && addr.getAddress().length == 4) {
+ // IPv4-mapped IPv6 addresses are automatically converted to IPv4,
+ // need to shift the mask
+ parsedMask = Math.max(0, parsedMask - 96);
+ }
+ this.mask = parsedMask;
+ }
+
+ public boolean contains(InetAddress address) {
+ byte[] addr0 = addr.getAddress();
+ byte[] addr1 = address.getAddress();
+ if (addr0.length != addr1.length) {
+ // not comparing IPv4 and IPv6 addresses
+ return false;
+ }
+ for (int i = 0; i < addr0.length; i++) {
+ int remainingMaskBits = mask - (i * 8);
+ if (remainingMaskBits <= 0) {
+ return true;
+ }
+ int m = ~(0xff >> remainingMaskBits); // mask for byte under cursor
Review Comment:
This line is very clever but `0xff >> remainingMaskBits` wraps once
`remainingMaskBits >= 32` (Java masks the shift count mod 32), so byte 0 is
left effectively unchecked for `/32`, `/128` and IPv6 prefixes ≥ 32.
`new CIDR("127.0.0.1").contains(ip("1.0.0.1"))` returns true. Keeping the
mask inside a single byte avoids both the wrap and the sign-extension:
```suggestion
// keep the mask within one byte so the shift can't wrap (Java
shifts mod 32)
int m = remainingMaskBits >= 8 ? 0xff : (0xff << (8 -
remainingMaskBits)) & 0xff;
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
