Hi Michael, Thanks for your feedback!
> I notice that the PIP doesn't mention documentation. Since we're adding another way to configure mTLS, please make sure to document the recommended way that users should take advantage of this feature and how this feature relates to the existing AuthenticationTLS feature. Good idea, let me add a simple document that how to use TLS transport and TLS authentication. > We are removing the client's need to use the AuthenticationTLS class to perform TLS authentication of clients by the server. We don't remove the use of the AuthenticationTLS. > If a user wants to use TLS certificates for authorization, they can still put roles in their client certificates and continue to use the AuthenticationProviderTLS class to map a TLS certificate to a role on the server side. You are right, the users still can use the AuthenticationTLS to perform the TLS transport and TLS authentication. Currently, the AuthenticationTLS includes TLS transport and TLS authentication, if the user only uses the TLS transport, not use the TLS authentication, it is confusing, so I want to add a TLS transport config in `ClientBuilder`. Thanks, Zixuan Michael Marshall <mmarsh...@apache.org> 于2022年5月12日周四 01:51写道： > I agree that the current state of this feature is a bit confusing, and > I think the proposed changes make sense. I notice that the PIP doesn't > mention documentation. Since we're adding another way to configure > mTLS, please make sure to document the recommended way that users > should take advantage of this feature and how this feature relates to the > existing AuthenticationTLS feature. > > In order to make sure I understand the feature correctly, can you > confirm that the following is correct? > > We are removing the client's need to use the AuthenticationTLS class > to perform TLS authentication of clients by the server. If a user > wants to use TLS certificates for authorization, they can still put > roles in their client certificates and continue to use the > AuthenticationProviderTLS class to map a TLS certificate to a role on > the server side. > > Thanks, > Michael > > > > > > > On Mon, May 9, 2022 at 12:58 AM Yunze Xu <y...@streamnative.io.invalid> > wrote: > > > > Thanks for your clarification. Let’s continue maintaining these configs > in > > `ClientBuilder`. > > > > Thanks, > > Yunze > > > > > > > > > > > 2022年5月9日 13:54，Zixuan Liu <node...@gmail.com> 写道： > > > > > > Hi Yunze, > > > > > > Thanks for your suggestion, your idea is great, but we have the > > > `tlsProtocols()` and `tlsCiphers()` in `ClientBuilder`, so I use this > style. > > > > > > Thanks, > > > Zixuan > > > > > > Yunze Xu <y...@streamnative.io.invalid> 于2022年5月9日周一 13:31写道： > > > > > >> It totally LGTM. I have a suggestion that it might be better to > configure a > > >> class like `TlsConfiguration` instead of multiple TLS related configs > > >> added to > > >> `ClientBuilder`. > > >> > > >> Thanks, > > >> Yunze > > >> > > >> > > >> > > >> > > >>> 2022年4月24日 14:15，Zixuan Liu <node...@gmail.com> 写道： > > >>> > > >>> Hi Pulsar community, > > >>> > > >>> I open a https://github.com/apache/pulsar/issues/15289 for Split > client > > >> TLS > > >>> transport encryption from authentication. > > >>> > > >>> Let me know what you think. > > >>> > > >>> Thanks, > > >>> Zixuan > > >>> > > >>> ------ > > >>> > > >>> Motivation > > >>> > > >>> The client supports TLS transport encryption and TLS authentication, > this > > >>> code so like: > > >>> > > >>> PulsarClient client = PulsarClient.builder() > > >>> .serviceUrl("pulsar+ssl://localhost:6651") > > >>> .tlsTrustCertsFilePath("/path/to/cacert.pem") > > >>> .authentication(AuthenticationTls.class.getName(), > > >> authParams) > > >>> .build() > > >>> > > >>> This causes an issue that cannot use other authentication with TLS > > >>> transport encryption, and also made our confusion if we use TLS > transport > > >>> encryption by setting authentication. > > >>> Goal > > >>> > > >>> Split client TLS transport encryption from authentication is used to > > >>> support TLS transport encryption with any authentication. > > >>> API Changes > > >>> > > >>> - Add new methods in org.apache.pulsar.client.api.ClientBuilder > > >>> > > >>> public interface ClientBuilder extends Serializable, Cloneable { > > >>> /** * Set the path to the TLS key file. * * @param > > >>> tlsKeyFilePath * @return the client builder instance */ > > >>> ClientBuilder tlsKeyFilePath(String tlsKeyFilePath); > > >>> > > >>> /** * Set the path to the TLS certificate file. * * > > >>> @param tlsCertificateFilePath * @return the client builder > > >>> instance */ > > >>> ClientBuilder tlsCertificateFilePath(String > tlsCertificateFilePath); > > >>> } > > >>> > > >>> ImplementationTLS transport encryption > > >>> > > >>> We can call the tlsKeyFilePath(), tlsCertificateFilePath() and > > >>> tlsTrustCertsFilePath() to configurate the TLS transport encryption, > the > > >>> code so like: > > >>> > > >>> PulsarClient client = PulsarClient.builder() > > >>> .serviceUrl("pulsar+ssl://my-host:6650") > > >>> .tlsTrustCertsFilePath("/path/to/cacert.pem") > > >>> .tlsKeyFilePath("/path/to/client-key.pem") > > >>> .tlsCertificateFilePath("/path/to/client-cert.pem") > > >>> .build(); > > >>> > > >>> TLS transport encryption with any authentication > > >>> > > >>> We can call the tlsKeyFilePath(), tlsCertificateFilePath(), > > >>> tlsTrustCertsFilePath() and authentication() to configurate the TLS > > >>> transport encryption with any authentication, the code so like: > > >>> > > >>> PulsarClient client = PulsarClient.builder() > > >>> .serviceUrl("pulsar+ssl://my-host:6650") > > >>> .tlsTrustCertsFilePath("/path/to/cacert.pem") > > >>> .tlsKeyFilePath("/path/to/client-key.pem") > > >>> .tlsCertificateFilePath("/path/to/client-cert.pem") > > >>> .authentication(AuthenticationTls.class.getName() /* > > >>> AuthenticationToken.class.getName()*/, authParams) > > >>> .builder() > > >>> > > >>> For AuthenticationTls, we need to do check the authParams, when the > > >>> authParams is empty, we need to read TLS config from ClientBuilder, > > >>> otherwise read from the authParams > > >>> Compatibility > > >>> > > >>> None. > > >> > > >> > > >
