[I] [fix][misc] PIP-478: webServiceTls{Ciphers,Protocols} and the proxy lookup leg's brokerClientTls{Ciphers,Protocols} are not applied [pulsar]

["lhotari (via GitHub)" <gi...@apache.org>]

lhotari opened a new issue, #26386:
URL: https://github.com/apache/pulsar/issues/26386

   ### Motivation
   
   Two components accept a TLS cipher/protocol restriction that never reaches the connection it names.
   
   **1. The broker's web listener ignores `webServiceTlsCiphers` / `webServiceTlsProtocols`.**
   
   `WebService.createTlsFactoryWebServer` passes the *binary-listener* keys to Jetty:
   
   ```java
   JettyTlsFactory.createReloadingServerFactory(this.tlsFactory, TlsPurpose.WEB, pulsar.getExecutor(),
           config.getWebServiceTlsProvider(), config.isTlsRequireTrustedClientCertOnConnect(),
           config.isTlsAllowInsecureConnection(), config.getTlsCiphers(), config.getTlsProtocols());
   ```
   
   The three sibling components pass the web keys at the same call site — `pulsar-proxy` `WebServer`,
   `pulsar-websocket` `ProxyServer` and functions `WorkerServer` all pass
   `config.getWebServiceTlsCiphers(), config.getWebServiceTlsProtocols()`. Those arguments are what reach
   the engine via `JettyTlsFactory.applyServerConfig` → `setIncludeCipherSuites` / `setIncludeProtocols`.
   
   The composed policy cannot compensate. `DefaultBrokerTlsFactory.webPolicy` does resolve
   `firstNonEmpty(getWebServiceTlsProtocols(), getTlsProtocols())`, but the `WEB` purpose is only acquired
   as `SSLContext` and `SSLParameters`, and `FileBasedTlsFactory.isSupported` is
   
   ```java
   return instanceClass == SslContext.class || instanceClass == SSLContext.class;
   ```
   
   so the `SSLParameters` companion resolves to `Optional.empty()`, the baseline is null, and the policy's
   protocol/cipher lists reach nothing. That half of `webPolicy` is dead code.
   
   `DefaultBrokerTlsFactory`'s class javadoc nonetheless states the opposite:
   
   > The web listener has its own provider/protocol/cipher keys, and they take precedence over the
   > binary-listener ones when set — webServiceTlsProvider, webServiceTlsProtocols, webServiceTlsCiphers
   
   Both keys are shipped in `conf/broker.conf`.
   
   **2. The proxy's binary lookup leg ignores `brokerClientTlsCiphers` / `brokerClientTlsProtocols`.**
   
   The proxy has three outbound legs. The direct binary leg and the admin HTTP leg build from
   `ProxyTlsFactories.brokerClientPolicy(config)`, which sets `.protocols(...)` and `.ciphers(...)` from
   those keys. The lookup leg gets its TLS from `ProxyConnection.createClientConfiguration`, which sets the
   trust/cert/key paths, the keystore fields, the insecure and hostname-verification flags, the three
   provider pins and the factory selection — but never calls `setTlsCiphers` or `setTlsProtocols`.
   `ClientTlsFactorySupport.clientDefaultPolicy` then reads the untouched `new TreeSet<>()` defaults.
   
   `ProxyConfiguration.brokerClientTlsCiphers` is a real field whose `@FieldContext` says it is "used by the
   Pulsar proxy to authenticate with Pulsar brokers". The sibling component does propagate it —
   `PulsarService` does `conf.setTlsCiphers(getConfiguration().getBrokerClientTlsCiphers())` for the broker's
   own outbound client.
   
   ### Impact
   
   An operator who narrows the negotiated protocol or cipher set for these connections gets the restriction
   applied to some of them and silently not to others: on the broker, the REST/admin listener keeps
   negotiating from the framework default floor while the binary listener is restricted; on the proxy, the
   partitioned-metadata / get-topics / get-schema lookups negotiate unrestricted while the data and admin
   connections are restricted. The same `broker.conf` applied to a proxy or worker *does* restrict, so the
   difference is invisible in a mixed deployment and in any config-driven review.
   
   ### Note on history
   
   Both are pre-existing rather than introduced by PIP-478 (`git show bb4768d4902~1` shows the same wiring
   in `WebService` and in `ProxyConnection.createClientConfiguration`). What PIP-478 added is the
   `webPolicy` resolution and the javadoc asserting a precedence the code does not implement, and the
   provider pins next to the missing cipher/protocol pair on the proxy leg.
   
   ### Suggested fix
   
   - `WebService`: pass `config.getWebServiceTlsCiphers()` / `getWebServiceTlsProtocols()`, falling back to
     the binary keys when empty (matching `webPolicy`'s `firstNonEmpty`); or, if the binary-key behaviour
     must be preserved, delete the dead resolution from `webPolicy` and correct its javadoc.
   - `ProxyConnection.createClientConfiguration`: add `setTlsCiphers` / `setTlsProtocols` from the
     `brokerClientTls*` keys, mirroring `PulsarService`.
   - Pin both with assertions on the resulting configuration.
   
   ---
   
   Found in a final-state review of the PIP-478 series (#26321).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org