Thanks for your responses, Zixuan. I think it might make sense to eventually deprecate the AuthenticationTLS class, if only because I think it can be confusing to give users two ways to configure the same thing. However, that is a minor detail. For now, we'll need to support both.
Thanks, Michael On Thu, May 12, 2022 at 4:43 AM Zixuan Liu <node...@gmail.com> wrote: > > You can see the code in the implementation part, this will be consistent > with the actual document. > > Zixuan Liu <node...@gmail.com> 于2022年5月12日周四 17:03写道： > > > 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. > >> > >> > >> > >> > >> > > >> > >
