You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/02/04 22:00:56 UTC
[GitHub] [kafka] dpoldrugo opened a new pull request #10059: KAFKA-8562: SaslChannelBuilder - avoid (reverse) DNS lookup while building underlying SslTransportLayer
dpoldrugo opened a new pull request #10059:
URL: https://github.com/apache/kafka/pull/10059
Description:
As suggested by @omkreddy in this [comment](https://issues.apache.org/jira/browse/KAFKA-8562?focusedCommentId=16912437&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16912437), implemented avoiding (reverse) DNS lookup while building underlying SslTransportLayer.
How the problem manifested:
When clients or other brokers are connecting to a broker using SASL_SSL, a broker was doing (reverse) DNS lookup and if there is no PTR Record, the lookup could last several seconds, which in the end caused big latencies on several parts of the system... replication, consume requests and produce requests.
Here you can see a recorded sample:
<img width="1357" alt="KAFKA-8562 reverse DNS sampling" src="https://user-images.githubusercontent.com/1514332/106959147-9033a580-673a-11eb-9575-4b9fe986cb30.png">
Also, here is a Wireshark packet capture for DNS requests, and in this case you can see that it lasted more then 11 seconds:
When using PLAINTEXT or SSL, this problem doesn't manifest.
Solution:
In #2835 , @rajinisivaram already added a helper method `SslChannelBuilder.peerHost`, so I just moved it to a new class called `ChannelBuilderUtils` and used it in `SaslChannelBuilder.buildTransportLayer` method.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dpoldrugo edited a comment on pull request #10059: KAFKA-8562: SaslChannelBuilder - avoid (reverse) DNS lookup while building underlying SslTransportLayer
Posted by GitBox <gi...@apache.org>.
dpoldrugo edited a comment on pull request #10059:
URL: https://github.com/apache/kafka/pull/10059#issuecomment-791291261
@omkreddy well for as at Infobip it caused an incident which basically blocked all kafka traffic, just because of some clients didn't have PTR records and thus reverse dns blocked the networking thread pool of all kafka brokers. We are currently running a custom built kafka 2.5.0.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dpoldrugo commented on pull request #10059: KAFKA-8562: SaslChannelBuilder - avoid (reverse) DNS lookup while building underlying SslTransportLayer
Posted by GitBox <gi...@apache.org>.
dpoldrugo commented on pull request #10059:
URL: https://github.com/apache/kafka/pull/10059#issuecomment-785690250
Thanks @hachikuji and @omkreddy for you comments.
Changed the tests as you proposed:
https://github.com/apache/kafka/pull/10059/commits/c605ea41fb0b342e9bb17595941c0e658bb63360
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10059: KAFKA-8562: SaslChannelBuilder - avoid (reverse) DNS lookup while building underlying SslTransportLayer
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10059:
URL: https://github.com/apache/kafka/pull/10059#discussion_r582643189
##########
File path: clients/src/main/java/org/apache/kafka/common/network/ChannelBuilderUtils.java
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.kafka.common.network;
+
+import java.net.InetSocketAddress;
+import java.nio.channels.SelectionKey;
+import java.nio.channels.SocketChannel;
+
+public class ChannelBuilderUtils {
+
+ /**
+ * Returns host/IP address of remote host without reverse DNS lookup to be used as the host
+ * for creating SSL engine. This is used as a hint for session reuse strategy and also for
+ * hostname verification of server hostnames.
+ * <p>
+ * Scenarios:
+ * <ul>
+ * <li>Server-side
+ * <ul>
+ * <li>Server accepts connection from a client. Server knows only client IP
+ * address. We want to avoid reverse DNS lookup of the client IP address since the server
+ * does not verify or use client hostname. The IP address can be used directly.</li>
+ * </ul>
+ * </li>
+ * <li>Client-side
+ * <ul>
+ * <li>Client connects to server using hostname. No lookup is necessary
+ * and the hostname should be used to create the SSL engine. This hostname is validated
+ * against the hostname in SubjectAltName (dns) or CommonName in the certificate if
+ * hostname verification is enabled. Authentication fails if hostname does not match.</li>
+ * <li>Client connects to server using IP address, but certificate contains only
+ * SubjectAltName (dns). Use of reverse DNS lookup to determine hostname introduces
+ * a security vulnerability since authentication would be reliant on a secure DNS.
+ * Hence hostname verification should fail in this case.</li>
+ * <li>Client connects to server using IP address and certificate contains
+ * SubjectAltName (ipaddress). This could be used when Kafka is on a private network.
+ * If reverse DNS lookup is used, authentication would succeed using IP address if lookup
+ * fails and IP address is used, but authentication would fail if lookup succeeds and
+ * dns name is used. For consistency and to avoid dependency on a potentially insecure
+ * DNS, reverse DNS lookup should be avoided and the IP address specified by the client for
+ * connection should be used to create the SSL engine.</li>
+ * </ul></li>
+ * </ul>
+ */
+ static String peerHost(SelectionKey key) {
Review comment:
Instead of doing this, I think we can have the following 3 methods in `SslFactory`. Thoughts?
```java
public SSLEngine createSslEngine(Socket socket) {
return createSslEngine(peerHost(socket), socket.getPort());
}
/**
* Prefer `createSslEngine(Socket)` if a `Socket` instance is available. If using this overload,
* avoid reverse DNS resolution in the computation of `peerHost`.
*/
public SSLEngine createSslEngine(String peerHost, int peerPort) {
if (sslEngineFactory == null) {
throw new IllegalStateException("SslFactory has not been configured.");
}
if (mode == Mode.SERVER) {
return sslEngineFactory.createServerSslEngine(peerHost, peerPort);
} else {
return sslEngineFactory.createClientSslEngine(peerHost, peerPort, endpointIdentification);
}
}
/**
* Returns host/IP address of remote host without reverse DNS lookup to be used as the host
* for creating SSL engine. This is used as a hint for session reuse strategy and also for
* hostname verification of server hostnames.
* <p>
* Scenarios:
* <ul>
* <li>Server-side
* <ul>
* <li>Server accepts connection from a client. Server knows only client IP
* address. We want to avoid reverse DNS lookup of the client IP address since the server
* does not verify or use client hostname. The IP address can be used directly.</li>
* </ul>
* </li>
* <li>Client-side
* <ul>
* <li>Client connects to server using hostname. No lookup is necessary
* and the hostname should be used to create the SSL engine. This hostname is validated
* against the hostname in SubjectAltName (dns) or CommonName in the certificate if
* hostname verification is enabled. Authentication fails if hostname does not match.</li>
* <li>Client connects to server using IP address, but certificate contains only
* SubjectAltName (dns). Use of reverse DNS lookup to determine hostname introduces
* a security vulnerability since authentication would be reliant on a secure DNS.
* Hence hostname verification should fail in this case.</li>
* <li>Client connects to server using IP address and certificate contains
* SubjectAltName (ipaddress). This could be used when Kafka is on a private network.
* If reverse DNS lookup is used, authentication would succeed using IP address if lookup
* fails and IP address is used, but authentication would fail if lookup succeeds and
* dns name is used. For consistency and to avoid dependency on a potentially insecure
* DNS, reverse DNS lookup should be avoided and the IP address specified by the client for
* connection should be used to create the SSL engine.</li>
* </ul></li>
* </ul>
*/
private String peerHost(Socket socket) {
return new InetSocketAddress(socket.getInetAddress(), 0).getHostString();
}```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dpoldrugo commented on pull request #10059: KAFKA-8562: SaslChannelBuilder - avoid (reverse) DNS lookup while building underlying SslTransportLayer
Posted by GitBox <gi...@apache.org>.
dpoldrugo commented on pull request #10059:
URL: https://github.com/apache/kafka/pull/10059#issuecomment-786530625
@hachikuji @ijuma @omkreddy will you consider this pull request to be part of of 2.5.2 release?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji merged pull request #10059: KAFKA-8562: SaslChannelBuilder - avoid (reverse) DNS lookup while building underlying SslTransportLayer
Posted by GitBox <gi...@apache.org>.
hachikuji merged pull request #10059:
URL: https://github.com/apache/kafka/pull/10059
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on pull request #10059: KAFKA-8562: SaslChannelBuilder - avoid (reverse) DNS lookup while building underlying SslTransportLayer
Posted by GitBox <gi...@apache.org>.
hachikuji commented on pull request #10059:
URL: https://github.com/apache/kafka/pull/10059#issuecomment-785326212
I think the test failures are caused by this: https://github.com/apache/kafka/blob/trunk/clients/src/test/java/org/apache/kafka/common/security/authenticator/SaslAuthenticatorFailureDelayTest.java#L222. We are relying on the reverse DNS to map "127.0.0.1" to "localhost." If we use "localhost" directly, then the tests pass.
It seems reasonable to me to change the test case. I cannot think of a reason why this would change the reasoning that is documented under `ChannelBuilderUtils.peerHost`, which seems to apply just as well to SASL_SSL as SSL.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10059: KAFKA-8562: SaslChannelBuilder - avoid (reverse) DNS lookup while building underlying SslTransportLayer
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10059:
URL: https://github.com/apache/kafka/pull/10059#discussion_r582643189
##########
File path: clients/src/main/java/org/apache/kafka/common/network/ChannelBuilderUtils.java
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.kafka.common.network;
+
+import java.net.InetSocketAddress;
+import java.nio.channels.SelectionKey;
+import java.nio.channels.SocketChannel;
+
+public class ChannelBuilderUtils {
+
+ /**
+ * Returns host/IP address of remote host without reverse DNS lookup to be used as the host
+ * for creating SSL engine. This is used as a hint for session reuse strategy and also for
+ * hostname verification of server hostnames.
+ * <p>
+ * Scenarios:
+ * <ul>
+ * <li>Server-side
+ * <ul>
+ * <li>Server accepts connection from a client. Server knows only client IP
+ * address. We want to avoid reverse DNS lookup of the client IP address since the server
+ * does not verify or use client hostname. The IP address can be used directly.</li>
+ * </ul>
+ * </li>
+ * <li>Client-side
+ * <ul>
+ * <li>Client connects to server using hostname. No lookup is necessary
+ * and the hostname should be used to create the SSL engine. This hostname is validated
+ * against the hostname in SubjectAltName (dns) or CommonName in the certificate if
+ * hostname verification is enabled. Authentication fails if hostname does not match.</li>
+ * <li>Client connects to server using IP address, but certificate contains only
+ * SubjectAltName (dns). Use of reverse DNS lookup to determine hostname introduces
+ * a security vulnerability since authentication would be reliant on a secure DNS.
+ * Hence hostname verification should fail in this case.</li>
+ * <li>Client connects to server using IP address and certificate contains
+ * SubjectAltName (ipaddress). This could be used when Kafka is on a private network.
+ * If reverse DNS lookup is used, authentication would succeed using IP address if lookup
+ * fails and IP address is used, but authentication would fail if lookup succeeds and
+ * dns name is used. For consistency and to avoid dependency on a potentially insecure
+ * DNS, reverse DNS lookup should be avoided and the IP address specified by the client for
+ * connection should be used to create the SSL engine.</li>
+ * </ul></li>
+ * </ul>
+ */
+ static String peerHost(SelectionKey key) {
Review comment:
Instead of doing this, I think we can have the following 3 methods in `SslFactory`. Thoughts?
```java
public SSLEngine createSslEngine(Socket socket) {
return createSslEngine(peerHost(socket), socket.getPort());
}
/**
* Prefer `createSslEngine(Socket)` if a `Socket` instance is available. If using this overload,
* avoid reverse DNS resolution in the computation of `peerHost`.
*/
public SSLEngine createSslEngine(String peerHost, int peerPort) {
if (sslEngineFactory == null) {
throw new IllegalStateException("SslFactory has not been configured.");
}
if (mode == Mode.SERVER) {
return sslEngineFactory.createServerSslEngine(peerHost, peerPort);
} else {
return sslEngineFactory.createClientSslEngine(peerHost, peerPort, endpointIdentification);
}
}
/**
* Returns host/IP address of remote host without reverse DNS lookup to be used as the host
* for creating SSL engine. This is used as a hint for session reuse strategy and also for
* hostname verification of server hostnames.
* <p>
* Scenarios:
* <ul>
* <li>Server-side
* <ul>
* <li>Server accepts connection from a client. Server knows only client IP
* address. We want to avoid reverse DNS lookup of the client IP address since the server
* does not verify or use client hostname. The IP address can be used directly.</li>
* </ul>
* </li>
* <li>Client-side
* <ul>
* <li>Client connects to server using hostname. No lookup is necessary
* and the hostname should be used to create the SSL engine. This hostname is validated
* against the hostname in SubjectAltName (dns) or CommonName in the certificate if
* hostname verification is enabled. Authentication fails if hostname does not match.</li>
* <li>Client connects to server using IP address, but certificate contains only
* SubjectAltName (dns). Use of reverse DNS lookup to determine hostname introduces
* a security vulnerability since authentication would be reliant on a secure DNS.
* Hence hostname verification should fail in this case.</li>
* <li>Client connects to server using IP address and certificate contains
* SubjectAltName (ipaddress). This could be used when Kafka is on a private network.
* If reverse DNS lookup is used, authentication would succeed using IP address if lookup
* fails and IP address is used, but authentication would fail if lookup succeeds and
* dns name is used. For consistency and to avoid dependency on a potentially insecure
* DNS, reverse DNS lookup should be avoided and the IP address specified by the client for
* connection should be used to create the SSL engine.</li>
* </ul></li>
* </ul>
*/
private String peerHost(Socket socket) {
return new InetSocketAddress(socket.getInetAddress(), 0).getHostString();
}
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dpoldrugo commented on pull request #10059: KAFKA-8562: SaslChannelBuilder - avoid (reverse) DNS lookup while building underlying SslTransportLayer
Posted by GitBox <gi...@apache.org>.
dpoldrugo commented on pull request #10059:
URL: https://github.com/apache/kafka/pull/10059#issuecomment-778341744
Should this feature have it's own config so users configure it by themselves... to enable / disable reverse dns resolving for SASL_SSL ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on pull request #10059: KAFKA-8562: SaslChannelBuilder - avoid (reverse) DNS lookup while building underlying SslTransportLayer
Posted by GitBox <gi...@apache.org>.
hachikuji commented on pull request #10059:
URL: https://github.com/apache/kafka/pull/10059#issuecomment-785315990
Hmm.. I may have been too hasty with my approval. We need to investigate the test failures. I've triggered another build.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dpoldrugo commented on pull request #10059: KAFKA-8562: SaslChannelBuilder - avoid (reverse) DNS lookup while building underlying SslTransportLayer
Posted by GitBox <gi...@apache.org>.
dpoldrugo commented on pull request #10059:
URL: https://github.com/apache/kafka/pull/10059#issuecomment-791291261
@omkreddy well for as at Infobip it caused an incident which basically blocked all kafka traffic, just because of some clients didn't have PTR records and thus reverse dns blocked the networking thread pool of all kafka brokers.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] omkreddy edited a comment on pull request #10059: KAFKA-8562: SaslChannelBuilder - avoid (reverse) DNS lookup while building underlying SslTransportLayer
Posted by GitBox <gi...@apache.org>.
omkreddy edited a comment on pull request #10059:
URL: https://github.com/apache/kafka/pull/10059#issuecomment-785604829
@dpoldrugo Thanks for the PR. LGTM. As @hachikuji mentioned, we should update `SaslAuthenticatorFailureNoDelayTest`, `SaslAuthenticatorTest` tests. I think, we may not need config, as we are already avoiding DNS lookup for SSL.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] omkreddy commented on pull request #10059: KAFKA-8562: SaslChannelBuilder - avoid (reverse) DNS lookup while building underlying SslTransportLayer
Posted by GitBox <gi...@apache.org>.
omkreddy commented on pull request #10059:
URL: https://github.com/apache/kafka/pull/10059#issuecomment-785604829
@dpoldrugo Thanks for the PR. LGTM. As @hachikuji mentioned, we should update `SaslAuthenticatorFailureNoDelayTest`, `SaslAuthenticatorTest` tests. I think, we may not need config, as we are already avoiding DNS look up for SSL authentication.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dpoldrugo commented on pull request #10059: KAFKA-8562: SaslChannelBuilder - avoid (reverse) DNS lookup while building underlying SslTransportLayer
Posted by GitBox <gi...@apache.org>.
dpoldrugo commented on pull request #10059:
URL: https://github.com/apache/kafka/pull/10059#issuecomment-773663807
@ijuma, @omkreddy, @rajinisivaram could you take a look?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dpoldrugo commented on pull request #10059: KAFKA-8562: SaslChannelBuilder - avoid (reverse) DNS lookup while building underlying SslTransportLayer
Posted by GitBox <gi...@apache.org>.
dpoldrugo commented on pull request #10059:
URL: https://github.com/apache/kafka/pull/10059#issuecomment-773663807
@ijuma, @omkreddy, @rajinisivaram could you take a look?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] omkreddy commented on pull request #10059: KAFKA-8562: SaslChannelBuilder - avoid (reverse) DNS lookup while building underlying SslTransportLayer
Posted by GitBox <gi...@apache.org>.
omkreddy commented on pull request #10059:
URL: https://github.com/apache/kafka/pull/10059#issuecomment-787034333
@dpoldrugo Currently we don't have plan for 2.5.2 release. also we normally backport only critical/blocker fixes. not sure if this comes under this category.
https://cwiki.apache.org/confluence/display/KAFKA/Future+release+plan
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org