> Is it possible to deprecate the option to configure the certificates in the AuthenticationTls class? I think it should be since the certs are now configured as top level configuration, and they need only be configured once.
We can't do that deprecate the option to configure the certificates in the AuthenticationTls class, if do this, we need to deal with the AuthenticationDataProvider, it is complex. When using `AuthenticationTls` and TLS config of `ClientBuilder`, we only use the `AuthenticationTls`, which can override the top level configuration, it is easier. I submitted https://github.com/apache/pulsar/pull/15634 for this PIP. --- PIP has some changes: ~For `AuthenticationTls`, we need to check the authParams, when the authParams is empty, we need to read TLS config from `ClientBuilder`, otherwise read from the authParams, the authParams can override the config from `ClientBuilder`.~ When using `AuthenticationTls` and TLS config of `ClientBuilder`, we only use the `AuthenticationTls`, which can override the top level configuration. **Sort priority:** `AuthenticationTls` > TLS config of `ClientBuilder`. --- Thanks, Zixuan Michael Marshall <mmarsh...@apache.org> 于2022年6月1日周三 12:54写道： > I know it's a bit late to respond, but just want to confirm that > Zixuan is correct that we cannot deprecate the `AuthenticationTls` > class because we rely on that class to set the authentication mode in > the pulsar protocol. That mode is then used by the ServerCnx for > authentication and authorization. > > Is it possible to deprecate the option to configure the certificates > in the AuthenticationTls class? I think it should be since the certs > are now configured as top level configuration, and they need only be > configured once. > > Thanks, > Michael > > On Sat, May 14, 2022 at 3:29 AM Zixuan Liu <node...@gmail.com> wrote: > > > > Hi Michael, It's not the same here. > > > > If you use AuthenticationTLS, which means you enable TLS authentication > and > > transport. > > > > ``` > > PulsarClient client = PulsarClient.builder() > > .serviceUrl("pulsar://my-host:6651") > > .tlsTrustCertsFilePath("/path/to/cacert.pem") > > .tlsKeyFilePath("/path/to/client-key.pem") > > .tlsCertificateFilePath("/path/to/client-cert.pem") > > .authentication(AuthenticationTls.class.getName()) // > > AuthenticationTls will uses the above certificate. > > .build(); > > ``` > > > > If you remove AuthenticationTLS, means we only use TLS transport. > > > > Thanks, > > Zixuan > > > > > > > > > > Michael Marshall <mmarsh...@apache.org> 于2022年5月14日周六 13:27写道： > > > > > 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. > > > > >> > >> > > > > >> > >> > > > > >> > > > > > >> > > > > > > > > >
