One late addition that I just discovered is for any pulsar CLI argument that is zero arity, we'll need to make a breaking change to make it have arity 1. For example, the --tls-enable-hostname-verification param here [0] currently works such that it is disabled when the parameter is missing and enabled when it is present. Because the case when it is missing will become true, that will mean there is no way to disable the configuration, which is problematic.
My proposed solution is to make --tls-enable-hostname-verification take an argument (make it arity 1). The primary issue is that this breaks current users that have hard coded scripts that enable hostname verification with the param. Please let me know if you see a way around this breaking change to our CLI tools. Thanks, Michael [0] https://github.com/apache/pulsar/blob/3efdc9af3bede77b018146caf1cd11730701ba4f/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/PulsarAdminTool.java#L84-L86 On Tue, Jun 6, 2023 at 9:19 AM ZhangJian He <shoot...@gmail.com> wrote: > > +1. default security is reasonable. > > Thanks > ZhangJian He > > > On Tue, 6 Jun 2023 at 13:40, Michael Marshall <mmarsh...@apache.org> wrote: > > > > Should these changes be released in a minor version? > > > > I think it is reasonable to make this change in 3.1.0. The change will > > be covered in the release notes. > > > > > For the Golang client, I think we should introduce a > > > `NewDefaultConfig` method to create the config instead of directly new > > > config. > > > > Makes sense, thanks. > > > > - Michael > > > > On Sat, Jun 3, 2023 at 9:37 AM Zixuan Liu <node...@gmail.com> wrote: > > > > > > +1 > > > > > > Default to enable the TLS hostname verification is important anywhere, > > > which protected our application. > > > > > > Should these changes be released in a minor version? > > > > > > For the Golang client, I think we should introduce a > > > `NewDefaultConfig` method to create the config instead of directly new > > > config. > > > > > > Thanks, > > > Zixuan > > > > > > Michael Marshall <mmarsh...@apache.org> 于2023年6月3日周六 05:39写道： > > > > > > > > I put together PRs for the python and the C++ clients: > > > > > > > > https://github.com/apache/pulsar-client-python/pull/128 > > > > https://github.com/apache/pulsar-client-cpp/pull/278 > > > > > > > > I am not sure the right way to change the default for the go client > > > > because go uses the zero value for structs, and we have the variable > > > > named "TLSValidateHostname". Would it perhaps make sense to ignore > > > > that variable and instead use "TLSHostnameVerificationDisabled"? Then, > > > > false would be the secure default. However, it introduces a double > > > > negative, which might make it harder for users to understand. > > > > > > > > I plan to start the vote next week. > > > > > > > > Thanks, > > > > Michael > > > > > > > > > > > > On Wed, May 31, 2023 at 11:08 PM Michael Marshall < > > mmarsh...@apache.org> wrote: > > > > > > > > > > Hi Pulsar Community, > > > > > > > > > > I am writing to start the discussion on PIP 273 to enable hostname > > > > > verification by default. > > > > > > > > > > PR with PIP contents: https://github.com/apache/pulsar/pull/20453 > > > > > > > > > > I copy the content below (except for the associated svg of the pulsar > > > > > network diagram). > > > > > > > > > > I look forward to your feedback. > > > > > > > > > > Thanks, > > > > > Michael > > > > > > > > > > ---- > > > > > > > > > > # PIP 273: Enable hostname verification by default > > > > > > > > > > # Background knowledge > > > > > > > > > > When using TLS to secure a network connection from client to server, > > > > > hostname verification is the step where the client verifies that the > > > > > server's certificate contains the expected Subject Alternative Name > > or > > > > > the Common Name. This step is essential for preventing man in the > > > > > middle attacks where a malicious server presents a certificate that > > is > > > > > cryptographically valid but was intended for a different hostname > > than > > > > > the one the client is trying to connect to. > > > > > > > > > > [RFC 2818](https://datatracker.ietf.org/doc/html/rfc2818#section-3.1 > > ) > > > > > provides additional details on the hostname verification process as a > > > > > part of the TLS handshake and why a client must verify the identity > > of > > > > > the server. > > > > > > > > > > It is very helpful to understand the network topology of a Pulsar > > > > > Cluster when looking to enable hostname verification. Here is a > > > > > diagram I created. It is possibly incomplete: > > > > > > > > > > (see PIP PR to see diagram) > > > > > > > > > > # Motivation > > > > > > > > > > The primary motivation is to improve the security of Pulsar by making > > > > > it secure by default for hostname verification. > > > > > > > > > > Pulsar currently disables hostname verification by default, which led > > > > > to some security issues in the past. For example, > > > > > [CVE-2022-33682]( > > https://github.com/apache/pulsar/wiki/CVE-2022-33682) > > > > > happened because there was no way to enable hostname verification > > > > > within the broker or the proxy. If hostname verification had been > > > > > enabled by default, the lack of configurability would not have been a > > > > > security concern but a usability concern. > > > > > > > > > > # Goals > > > > > > > > > > ## In Scope > > > > > > > > > > * Enable hostname verification by default for all clients in the > > > > > Pulsar ecosystem. > > > > > * Make it possible to configure hostname verification per > > > > > geo-replicated cluster. > > > > > * Consolidate the name of the hostname verification configuration > > > > > option across all server components. > > > > > > > > > > ## Out of Scope > > > > > > > > > > * This PIP does not seek to enable TLS by default, which is relevant > > > > > because hostname verification only takes affect when TLS is enabled. > > > > > * Client configuration names will not be renamed because that would > > > > > introduce binary incompatibility. > > > > > * Tiered storage clients > > > > > * ETCd client configuration -- it enables hostname verification by > > > > > default already > > > > > * Zookeeper client configuration -- it enables hostname verification > > > > > by default already > > > > > * Function worker's gRPC client to function instances -- TLS is not > > > > > configurable for this connection > > > > > * Function state storage to bookkeeper -- TLS is not configurable for > > > > > this connection > > > > > * Pulsar SQL worker's hostname verification configuration name, which > > > > > is `pulsar.tls-hostname-verification-enable` > > > > > > > > > > # High Level Design > > > > > > > > > > * Enable hostname verification by default for all (official) clients > > > > > in the Pulsar ecosystem. > > > > > * Enable hostname verification by default for all server components > > in > > > > > the Pulsar ecosystem. > > > > > * Make it possible to configure hostname verification per > > > > > geo-replicated cluster. > > > > > * Consolidate names by using `tlsHostnameVerificationEnabled` for all > > > > > server components. > > > > > > > > > > # Detailed Design > > > > > > > > > > ## Design & Implementation Details > > > > > > > > > > ### Impacted configurations > > > > > > > > > > * The official Pulsar clients will be impacted. > > > > > * The broker's internal pulsar admin client, pulsar client, and > > > > > replication clients > > > > > * The broker's bookkeeper client and dlog bookkeeper client > > > > > * Autorecovery's bookkeeper client > > > > > * Bookkeeper's bookkeeper client > > > > > * Proxy's pulsar client and pulsar admin client (used for brokers and > > > > > function workers) > > > > > * WebSocket Proxy's pulsar client > > > > > * Function Worker's dlog bookkeeper client, pulsar client (used for > > > > > broker), and pulsar admin client (used for broker and for other > > > > > function workers) > > > > > * Function instance pulsar client and pulsar admin client (including > > > > > the function worker client used to download the function) > > > > > > > > > > ## Public-facing Changes > > > > > > > > > > All changes in this PIP are public facing. > > > > > > > > > > ### Public API > > > > > > > > > > ### Binary protocol > > > > > > > > > > N/A > > > > > > > > > > ### Configuration > > > > > > > > > > There are two name changes. For function worker config, I propose we > > > > > replace `tlsEnableHostnameVerification` with > > > > > `tlsHostnameVerificationEnabled` to match the other server > > components. > > > > > For the client.conf config which is used by CLI tools, I propose we > > > > > change `tlsEnableHostnameVerification` to > > > > > `tlsHostnameVerificationEnabled`. > > > > > > > > > > ### CLI > > > > > > > > > > ### Metrics > > > > > > > > > > N/A > > > > > > > > > > # Monitoring > > > > > > > > > > N/A > > > > > > > > > > # Security Considerations > > > > > > > > > > This PIP will improve Pulsar's security by making it secure by > > default > > > > > for hostname verification. > > > > > > > > > > # Backward & Forward Compatibility > > > > > > > > > > ## Revert > > > > > > > > > > Users and operators can opt out of these changes by setting the > > > > > following configurations to `false`: > > > > > > > > > > | Component | Configuration | > > > > > > > |----------------------------------|----------------------------------------------------------------------------------| > > > > > | Pulsar Client | `tlsHostnameVerificationEnabled=false` or > > > > > `enableTlsHostnameVerification(false)` | > > > > > | Pulsar Admin Client | `tlsHostnameVerificationEnabled=false` or > > > > > `enableTlsHostnameVerification(false)` | > > > > > | Broker | `tlsHostnameVerificationEnabled=false` | > > > > > | Broker's BK Client | > > `bookkeeper_tlsHostnameVerificationEnabled=false` | > > > > > | Bookkeeper | `tlsHostnameVerificationEnabled=false` | > > > > > | Autorecovery | `tlsHostnameVerificationEnabled=false` | > > > > > | Proxy | `tlsHostnameVerificationEnabled=false` | > > > > > | WebSocket Proxy | `tlsHostnameVerificationEnabled=false` | > > > > > | Function Worker | `tlsHostnameVerificationEnabled: false` | > > > > > | Function Worker's BK/DLog Client | > > > > > `bookkeeper_tlsHostnameVerificationEnabled=false` | > > > > > | Function Instance | TODO | > > > > > > > > > > ## Upgrade > > > > > > > > > > Ensure your certificates will correctly pass hostname verification. > > > > > > > > > > # Alternatives > > > > > > > > > > The primary alternative is to make no changes. > > > > > > > > > > # General Notes > > > > > > > > > > Here are the current draft PRs for implementing this PIP: > > > > > > > > > > * https://github.com/apache/pulsar/pull/20268 > > > > > * https://github.com/apache/pulsar/pull/20269 > >
