KAFKA-3722 : The PlaintextChannelBuilder should always use the DefaultPrincipalBuilder

[MayureshGharat]

Consider this scenario :

1. We have a Kafka Broker running on PlainText and SSL port simultaneously.

2. We try to plugin a custom principal builder using the config "principal.builder.class" for the request coming over the SSL port.

3. The ChannelBuilders.createPrincipalBuilder(configs) first checks if a config "principal.builder.class" is specified in the passed in configs and tries to use that even when it is building the instance of PrincipalBuilder for the PlainText port, when that custom principal class is only menat for SSL port.

IMO, having a DefaultPrincipalBuilder for PlainText port should be fine.

[ijuma]

What is the issue that you're having with the current approach. Can you not return the received KafkaPrincipal.ANONYMOUS for the PLAINTEXT case (which is what DefaultPrincipalBuilder would do)? It seems more regular to me that the principal.builder.class is always used (it's a not ideal that we don't use it for SASL too).

[rajinisivaram]

At the moment, you can override the principal for PLAINTEXT, for instance, set principal based on IP. Or set a different principal for PLAINTEXT from an unauthenticated SSL client. Feels like a useful config even for PLAINTEXT.

[MayureshGharat]

@ijuma @rajinisivaram
For PlainText we don't supply any PrincipalBuilder. For SSL we want to supply a principalBuilder using the property "principal.builder.class".

1. Now consider we have a broker running on these 2 ports and supply that custom principalBuilder class using that config.
2. The interbroker communication is using PlainText. I am using a single broker cluster for testing.
3. Now we issue a produce request on the SSL port of the broker.
4. The controller tries to build a channel for plaintext with this broker for the new topic instructions.
5. PlainText tries to use the principal builder specified in the "principal.builder.class" config which was meant only for SSL port since the code path is same "ChannelBuilders.createPrincipalBuilder(configs)".
6. In the custom principal Builder if we are trying to do some cert checks or down conversion of transportLayer to SSLTransportLayer so that we can use its functionality we get runtime exception.

The basic idea is the PlainText channel should not be using the PrincipalBuilder meant for other types of channels. We should probably have separate builders and authorizers for each type of channel if there is a valid use case here. For my usecase having a DefaultPrincipalBuilder for PlainText would suffice. But now that I think more, having a clear separation for those configs might be a good idea as a broker can run on multiple types of ports in future.
I would be happy to start this discussion in Opensource email list and implement the patch if you guys agree.

[rajinisivaram]

@MayureshGharat Isn't the code in your custom principal builder in the method buildPrincipal(TransportLayer transportLayer, Authenticator authenticator)? If it is, you could simply check the type of transportLayer and have different code paths for PLAINTEXT and SSL. At the moment, you would need check if transportLayer is an instance of PlaintextTransportLayer or SslTransportLayer. It may be neater to add a method to return security protocol from TransportLayer.

[MayureshGharat]

@rajinisivaram Yeah right now I can have an "instanceOf" check or catch the downcasting exception and do the handling. Having a method in TransportLayer to return the type might be a good idea. But as the type of ports increase it means adding more checks to buildPrincipal right? Isn't it better to have separate configs for them instead? What do you think? That will be more cleaner IMO, though it increases the 2 configs (Authorizer and PrincipalBuilderClass) for each port type.

[rajinisivaram]

@MayureshGharat Personally I prefer smaller number of configs, especially since it can be made to work. But I do see your point about separate configs being neater. It will be good to see what others think.

[ijuma]

I am personally in favour of less configs too. It may make sense to pass the security protocol as a parameter to buildPrincipal to make it clearer that this is something that needs to be handled.

[MayureshGharat]

@ijuma I think we have 4 options from the above discussions.

1. Doing instanceOf check.
2. TransportLayer should expose api for telling the protocol.
3. buildPrincipal() should take in a new param for the type of protocol and document this in the javadoc on how to use it?
4. Add extra configs for each type of channel.
   Should I start a discussion on mailing list for this or do we agree on one of this?
   I am +1 for 3) and 4)

[rajinisivaram]

@MayureshGharat @ijuma Isn't 3) a breaking change of a public interface? I imagine 3) and 4) need a KIP since they change externals.

[ijuma]

@rajinisivaram If changing the existing method, yes. But we could introduce an overloaded one with additional parameter and an empty implementation. And have the existing one call that by default. This means that existing users that override the existing method would continue to work and new users could override the new method. This would require default methods that only exist in Java 8 though.

So, given that, it's unlikely that this would be accepted. So, adding a method to TransportLayer along with a documentation update in buildPrincipal telling people about it is probably the likely answer.

@MayureshGharat, it makes sense to start a mailing list thread about this to get more opinions.

[MayureshGharat]

@ijuma Sounds good. Will start a mailing list discussion on this.

[hachikuji]

Closing since the issue was fixed with KIP-189 and the JIRA was resolved.