KAFKA-6750: Add listener name to authentication context (KIP-282) by mimaison · Pull Request #4829 · apache/kafka

mimaison · 2018-04-05T16:35:37Z

Co-authored-by: Edoardo Comar ecomar@uk.ibm.com
Co-authored-by: Mickael Maison mickael.maison@gmail.com

This KIP adds the String value of the ListenerName used for the connection to the AuthenticationContext.

This allows PrincipalBuilders to retrieve this value and build different Principal based on the source of the connection. This is especially interesting in deployments where inter-broker traffic is on a different network/listener than user traffic or when the same protocol is used by several listeners.

The change in itself is mostly "plumbing" as the listener name needs to be passed from ChannelBuilders all the way down to all classes implementing AuthenticationContext.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

…text Co-authored-by: Edoardo Comar <ecomar@uk.ibm.com> Co-authored-by: Mickael Maison <mickael.maison@gmail.com>

interface

ijuma · 2018-05-21T18:59:54Z

Thanks for the PR. FYI, the text in italics in the PR is meant to be replaced with your own message. ;)

mimaison · 2018-05-23T08:45:22Z

Thanks @ijuma I updated the description accordingly 😄

ijuma · 2018-05-24T21:35:20Z

core/src/test/scala/unit/kafka/server/ListenerAndPrincipalBuilderTest.scala

+import org.junit.Assert._
+import org.junit.{Before, Test}
+
+class ListenerAndPrincipalBuilderTest extends BaseRequestTest {


Do we need a new test for this? These tests are expensive since they involve starting several brokers. Ideally, we'd update one of the existing tests to also check the listener name.

We had a look at other tests extending BaseRequestTest and this does not really fit anywhere. We tried merging it into KafkaMetricReporterExceptionHandlingTest (one test checking both logics) but it really looked awful. As each test needs its own setup, the overall logic made little sense even with comments.

Yes the test takes 5 secs but we think it's clearer to keep it separated.

FYI we pushed the experiment into a branch: https://github.com/mimaison/kafka/blob/listener-name-with-miscellaneous-test/core/src/test/scala/unit/kafka/server/KafkaMiscellaneousRequestTest.scala

@ijuma As the deadline is today, can you have a look?

What about PlaintextEndToEndAuthorizationTest?

ijuma · 2018-05-29T18:05:49Z

This is a small feature, so we have an extra week.

ijuma

Thanks for the PR, left a few comments.

ijuma · 2018-06-01T07:00:34Z

clients/src/main/java/org/apache/kafka/common/network/PlaintextChannelBuilder.java

 public class PlaintextChannelBuilder implements ChannelBuilder {
    private static final Logger log = LoggerFactory.getLogger(PlaintextChannelBuilder.class);
    private Map<String, ?> configs;
+    private ListenerName listenerName;


Should be final.

ijuma · 2018-06-01T07:04:50Z

clients/src/test/java/org/apache/kafka/common/network/ChannelBuildersTest.java

 public class ChannelBuildersTest {

    @Test
-    @SuppressWarnings("deprecation")


Why did we remove this?

This test does not use any deprecated classes, so it's unecessary

I see, OldPrincipalBuilder extends the deprecated class but it's not deprecated itself.

ijuma · 2018-06-01T07:05:09Z

clients/src/test/java/org/apache/kafka/common/network/SslTransportLayerTest.java

        } catch (KafkaException e) {
            // Expected exception
+        } finally {
+            channelBuilder.close();


Can we use try with resources?

ijuma · 2018-06-01T07:11:20Z

core/src/test/scala/unit/kafka/server/ListenerAndPrincipalBuilderTest.scala

+import org.junit.Assert._
+import org.junit.{Before, Test}
+
+class ListenerAndPrincipalBuilderTest extends BaseRequestTest {


What about PlaintextEndToEndAuthorizationTest?

ijuma · 2018-06-02T00:30:39Z

clients/src/main/java/org/apache/kafka/common/network/PlaintextChannelBuilder.java

            InetAddress clientAddress = transportLayer.socketChannel().socket().getInetAddress();
-            return principalBuilder.build(new PlaintextAuthenticationContext(clientAddress));
+            // listenerName should only be null in Client mode
+            String listenerNameStr = listenerName != null ? listenerName.value() : null;


This seems a bit brittle. Maybe the field should be Optional<ListenerName>?

We should never be calling principal() in client mode.

We should probably not have the null check then, better fail fast. Same for the other case.

ijuma · 2018-06-02T00:33:21Z

clients/src/main/java/org/apache/kafka/common/security/auth/AuthenticationContext.java

+    /**
+     * Name of the listener used for the connection
+     */
+    String listenerName();


Can this be null? Maybe we should return Optional<String>. We would need to update the KIP too in that case.

AuthenticationContext is only used on the server-side, so listener name should never be null.

mimaison · 2018-06-04T13:09:01Z

Regarding listenerName being null. This can only happen on the Client side. The Client side only uses the default principal builder (even though the Client side is not using the Principal) and that does not use the new listener field.

The listenerName field will always be set on the server side where a custom PrincipalBuilder can try to access this field.

We don't have to check for null in SaslChannelBuilder as it has a Client and a Server side Authenticator so we can only pass the listener when on server.

In my opinion, having an Optional when it's always going to be set when needed is not the ideal solution. Maybe we can make this clearer in comments. What do you think ?

ijuma · 2018-06-04T22:19:55Z

@mimaison, AuthenticationContext is used in the client and the server. The fact that the listenerName is only used in the broker is even more reason to make it Optional. We can then document that it's always defined in the broker and never defined in the client. The fact that we currently have multiple places in the code with the following line is worrying:

// listenerName should only be null in Client mode
+            String listenerNameStr = listenerName != null ? listenerName.value() : null;

Another option would be to have a ServerAuthenticationContext subclass. But not sure if that's easy to add.

Am I missing something? Also cc @rajinisivaram in case she has some thoughts.

rajinisivaram · 2018-06-05T08:28:27Z

@ijuma There are a few internal methods where listenerName is passed through which are used both by client and server and these need null checks (could even be IllegalStateException, but maybe easier to leave as null checks for now). But the public interface AuthenticationContext is only for use on the server-side to build the principal. So I think it makes sense to leave listenerName as a String rather than make it Optional.

ijuma · 2018-06-05T09:31:34Z

Synced with @rajinisivaram offline and AuthenticationContext is callable from both server and client, but we only access KafkaChannel.principal() on the server. It would be good to improve the code so that KafkaChannel.principal() works in both cases (i.e. if the principal builder is only meant to be called on the server, then the client path should return the principal associated with the connection without going through the principal builder (this is what happens in the SaslClientAuthenticator case as @mimaison said). In any case, for another PR. :)

ijuma

A few minor comments and then I think this is ready to be merged.

ijuma · 2018-06-05T09:32:11Z

clients/src/main/java/org/apache/kafka/common/network/PlaintextChannelBuilder.java

 public class PlaintextChannelBuilder implements ChannelBuilder {
    private static final Logger log = LoggerFactory.getLogger(PlaintextChannelBuilder.class);
    private Map<String, ?> configs;
+    private final ListenerName listenerName;


static fields first, then final, then mutable.

ijuma · 2018-06-05T09:33:53Z

clients/src/main/java/org/apache/kafka/common/network/PlaintextChannelBuilder.java

+
+    /**
+     * Constructs a plaintext channel builder. ListenerName is provided only
+     * for server channel builder and will be null for client channel builder.


It's actually a bit more complicated. It's non-null whenever it's instantiated in the broker and null otherwise. In the broker case, we create both server and client channel builders, but it's set in both (for sending and receiving requests respectively).

ijuma · 2018-06-05T09:35:13Z

clients/src/main/java/org/apache/kafka/common/network/PlaintextChannelBuilder.java

            InetAddress clientAddress = transportLayer.socketChannel().socket().getInetAddress();
-            return principalBuilder.build(new PlaintextAuthenticationContext(clientAddress));
+            // listenerName should only be null in Client mode
+            String listenerNameStr = listenerName != null ? listenerName.value() : null;


We should probably not have the null check then, better fail fast. Same for the other case.

ijuma · 2018-06-05T09:36:06Z

clients/src/main/java/org/apache/kafka/common/security/auth/AuthenticationContext.java

 * An object representing contextual information from the authentication session. See
- * {@link SaslAuthenticationContext} and {@link SslAuthenticationContext}.
+ * {@link PlaintextAuthenticationContext}, {@link SaslAuthenticationContext}
+ * and {@link SslAuthenticationContext}.


Maybe we should mention that this class is only used in the broker.

ijuma · 2018-06-05T09:40:53Z

clients/src/main/java/org/apache/kafka/common/security/auth/SslAuthenticationContext.java

 package org.apache.kafka.common.security.auth;

 import javax.net.ssl.SSLSession;
+


Is this intentional?

ijuma · 2018-06-05T09:42:06Z

...ts/src/test/java/org/apache/kafka/common/security/auth/DefaultKafkaPrincipalBuilderTest.java

                transportLayer, oldPrincipalBuilder, null);

-        KafkaPrincipal principal = builder.build(new PlaintextAuthenticationContext(InetAddress.getLocalHost()));
+        KafkaPrincipal principal = builder.build(new PlaintextAuthenticationContext(InetAddress.getLocalHost(), SecurityProtocol.PLAINTEXT.name()));


A few long lines in this class. The goal is that they should fit within the GitHub window.

ijuma · 2018-06-05T09:42:31Z

core/src/test/scala/integration/kafka/api/EndToEndAuthorizationTest.scala

  }

-  private def sendRecords(numRecords: Int, tp: TopicPartition) {
+  protected def sendRecords(numRecords: Int, tp: TopicPartition) {


protected final?

ijuma · 2018-06-05T09:45:16Z

core/src/test/scala/integration/kafka/api/PlaintextEndToEndAuthorizationTest.scala

 // which we derive corresponding principals
 object PlaintextEndToEndAuthorizationTest {
+  var clientListenerName: String = ""
+  var serverListenerName: String = ""


private and volatile? Also, it seems like initialising them as None would be better.

ijuma · 2018-06-05T09:46:35Z

core/src/test/scala/integration/kafka/api/PlaintextEndToEndAuthorizationTest.scala

+      fail("Should have thrown a TopicAuthorizationException")
+    } catch {
+      case tae: TopicAuthorizationException => //expected
+    }


Use intercept[TopicAuthorizationException](sendRecords(1, tp)

mimaison · 2018-06-05T10:23:28Z

@ijuma Thanks for the feedback! I've addressed all your comments.

ijuma · 2018-06-05T10:29:56Z

core/src/test/scala/integration/kafka/api/PlaintextEndToEndAuthorizationTest.scala

+    intercept[TopicAuthorizationException](sendRecords(1, tp))
+
+    assertEquals("CLIENT", PlaintextEndToEndAuthorizationTest.clientListenerName.get)
+    assertEquals("SERVER", PlaintextEndToEndAuthorizationTest.serverListenerName.get)


The right way to do this is assertEquals(Some("SERVER"), PlaintextEndToEndAuthorizationTest.serverListenerName) so that one gets a reasonable error if the Option is None.

Good point, fixed

ijuma

LGTM, thanks.

…grained-acl-create-topics * apache-github/trunk: KAFKA-5588: Remove deprecated --new-consumer tools option (apache#5097) MINOR: Fix for the location of the trogdor.sh executable file in the documentation. (apache#5040) KAFKA-6997: Exclude test-sources.jar when $INCLUDE_TEST_JARS is FALSE MINOR: docs should point to latest version (apache#5132) KAFKA-6981: Move the error handling configuration properties into the ConnectorConfig and SinkConnectorConfig classes (KIP-298) [KAFKA-6730] Simplify State Store Recovery (apache#5013) MINOR: Rename package `internal` to `internals` for consistency (apache#5137) KAFKA-6704: InvalidStateStoreException from IQ when StreamThread closes store (apache#4801) MINOR: Add missing configs for resilience settings MINOR: Add regression tests for KTable mapValues and filter (apache#5134) KAFKA-6750: Add listener name to authentication context (KIP-282) (apache#4829) KAFKA-3665: Enable TLS hostname verification by default (KIP-294) (apache#4956) KAFKA-6938: Add documentation for accessing Headers on Kafka Streams Processor API (apache#5128) KAFKA-6813: return to double-counting for count topology names (apache#5075) KAFKA-5919; Adding checks on "version" field for tools using it MINOR: Remove deprecated KafkaStreams constructors in docs (apache#5118)

…refix * apache-github/trunk: KAFKA-6726: Fine Grained ACL for CreateTopics (KIP-277) (apache#4795) KAFKA-5588: Remove deprecated --new-consumer tools option (apache#5097) MINOR: Fix for the location of the trogdor.sh executable file in the documentation. (apache#5040) KAFKA-6997: Exclude test-sources.jar when $INCLUDE_TEST_JARS is FALSE MINOR: docs should point to latest version (apache#5132) KAFKA-6981: Move the error handling configuration properties into the ConnectorConfig and SinkConnectorConfig classes (KIP-298) [KAFKA-6730] Simplify State Store Recovery (apache#5013) MINOR: Rename package `internal` to `internals` for consistency (apache#5137) KAFKA-6704: InvalidStateStoreException from IQ when StreamThread closes store (apache#4801) MINOR: Add missing configs for resilience settings MINOR: Add regression tests for KTable mapValues and filter (apache#5134) KAFKA-6750: Add listener name to authentication context (KIP-282) (apache#4829) KAFKA-3665: Enable TLS hostname verification by default (KIP-294) (apache#4956) KAFKA-6938: Add documentation for accessing Headers on Kafka Streams Processor API (apache#5128) KAFKA-6813: return to double-counting for count topology names (apache#5075) KAFKA-5919; Adding checks on "version" field for tools using it MINOR: Remove deprecated KafkaStreams constructors in docs (apache#5118)

…ache#4829) PrincipalBuilder implementations can now take the listener into account when creating the Principal. This is especially interesting in deployments where inter-broker traffic is on a different listener than client traffic or when the same protocol is used by multiple listeners. The change in itself is mostly "plumbing" as the listener name needs to be passed from ChannelBuilders all the way down to all classes implementing AuthenticationContext. Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com>, Ismael Juma <ismael@juma.me.uk> Co-authored-by: Edoardo Comar <ecomar@uk.ibm.com> Co-authored-by: Mickael Maison <mickael.maison@gmail.com>

mimaison force-pushed the listener-name branch 2 times, most recently from cabe8e1 to ed2d367 Compare April 18, 2018 10:19

KAFKA-6750 - KIP-282: Add the listener name to the authentication con…

9c519d7

…text Co-authored-by: Edoardo Comar <ecomar@uk.ibm.com> Co-authored-by: Mickael Maison <mickael.maison@gmail.com>

edoardocomar force-pushed the listener-name branch from ed2d367 to 9c519d7 Compare April 30, 2018 12:45

Changed listenerName type to be a String on AuthenticationContext public

70bd84e

interface

ijuma self-requested a review May 21, 2018 18:59

ijuma added this to the 2.0.0 milestone May 21, 2018

ijuma self-assigned this May 21, 2018

ijuma reviewed May 24, 2018

View reviewed changes

ijuma reviewed Jun 1, 2018

View reviewed changes

Updates following review

bada354

ijuma reviewed Jun 2, 2018

View reviewed changes

Added in comment to testListenerName

d069e0a

ijuma reviewed Jun 5, 2018

View reviewed changes

ijuma changed the title ~~KAFKA-6750 - KIP-282: Add the listener name to the authentication con…~~ KAFKA-6750: Add the listener name to the authentication context (KIP-282) Jun 5, 2018

ijuma changed the title ~~KAFKA-6750: Add the listener name to the authentication context (KIP-282)~~ KAFKA-6750: Add listener name to authentication context (KIP-282) Jun 5, 2018

Addressed latest comments

4dd4203

ijuma reviewed Jun 5, 2018

View reviewed changes

Fixed asserts on Option

132d4df

ijuma approved these changes Jun 5, 2018

View reviewed changes

ijuma merged commit 8a166f8 into apache:trunk Jun 5, 2018

mimaison deleted the listener-name branch May 15, 2020 23:05

		package org.apache.kafka.common.security.auth;

		import javax.net.ssl.SSLSession;

Comments

Conversation

mimaison commented Apr 5, 2018 • edited by ijuma Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

ijuma commented May 21, 2018

Uh oh!

mimaison commented May 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented May 29, 2018

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma Jun 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mimaison commented Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ijuma commented Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajinisivaram commented Jun 5, 2018

Uh oh!

ijuma commented Jun 5, 2018

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mimaison commented Apr 5, 2018 •

edited by ijuma

Loading

ijuma Jun 2, 2018 •

edited

Loading

mimaison commented Jun 4, 2018 •

edited

Loading

ijuma commented Jun 4, 2018 •

edited

Loading