HDDS-10819. Respect ssl.server.include.cipher.list and ssl.enabled.protocols in HttpServer2#10111
Conversation
…ols to HttpServer2 setup. Generated-by: Claude - Opus 4.6
adoroszlai
left a comment
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @fapifta for the patch.
| .includeCiphers( | ||
| sslConf.get("ssl.server.include.cipher.list")); |
There was a problem hiding this comment.
nit: add before .excludeCiphers to reduce change (due to moving around ;)
Also, I think these can use constants SSLFactory.SSL_SERVER_EXCLUDE_CIPHER_LIST and SSLFactory.SSL_SERVER_INCLUDE_CIPHER_LIST, since HttpServer2 already does.
There was a problem hiding this comment.
I have changed this method, reformatted the code and use SSLFactory constant for every string referenced there.
| } | ||
| } | ||
| if (null != excludeCiphers && !excludeCiphers.isEmpty()) { | ||
| if (excludeCiphers != null && !excludeCiphers.isEmpty()) { |
There was a problem hiding this comment.
nit: unnecessary change
There was a problem hiding this comment.
I respectfully disagree, this is the only condition which uses the yoda style, I think this is a better way as it conforms with the rest of the surrounding code.
| if (!enabledProtocols.equals( | ||
| SSLFactory.SSL_ENABLED_PROTOCOLS_DEFAULT)) { | ||
| String[] jettyExcludedProtocols = | ||
| sslContextFactory.getExcludeProtocols(); | ||
| String[] enabledProtocolsArray = | ||
| StringUtils.getTrimmedStrings(enabledProtocols); | ||
| List<String> enabledProtocolsList = | ||
| Arrays.asList(enabledProtocolsArray); | ||
|
|
||
| List<String> resetExcludedProtocols = new ArrayList<>(); | ||
| for (String jettyExcludedProtocol : jettyExcludedProtocols) { | ||
| if (!enabledProtocolsList.contains(jettyExcludedProtocol)) { | ||
| resetExcludedProtocols.add(jettyExcludedProtocol); | ||
| } else { | ||
| LOG.debug("Removed {} from exclude protocol list", | ||
| jettyExcludedProtocol); | ||
| } | ||
| } | ||
|
|
||
| sslContextFactory.setExcludeProtocols( | ||
| resetExcludedProtocols.toArray(new String[0])); | ||
| LOG.info("Reset exclude protocol list: {}", | ||
| resetExcludedProtocols); |
There was a problem hiding this comment.
nit: line wraps are unnecesary
There was a problem hiding this comment.
reformatted the code, and also rewritten the method based on the below suggestion to use removall
| List<String> resetExcludedProtocols = new ArrayList<>(); | ||
| for (String jettyExcludedProtocol : jettyExcludedProtocols) { | ||
| if (!enabledProtocolsList.contains(jettyExcludedProtocol)) { | ||
| resetExcludedProtocols.add(jettyExcludedProtocol); | ||
| } else { | ||
| LOG.debug("Removed {} from exclude protocol list", | ||
| jettyExcludedProtocol); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can be simplified?
List<String> resetExcludedProtocols = new ArrayList<>(jettyExcludedProtocols);
resetExcludedProtocols.removeAll(enabledProtocolsList);
adoroszlai
changed the title
adoroszlai
left a comment
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @fapifta for updating the patch.
With this change we have a separate property to define ozone ssl protocols, a fallback to hadoop.ssl.enabled.protocols, and then to the Hadoop default which is TLSv1.2.
|
After re-checking the PR, I realized that the original way introduces the new parameter for the enabled protocols as |
|
Thanks @fapifta for the patch. |
2 participants
What changes were proposed in this pull request?
Add ssl.server.include.cipher.list and ssl.enabled.protocols to HttpServer2 setup.
Please describe your PR in detail:
Port TLS protocol version control and cipher suite inclusion support from Hadoop's HttpServer2 to Ozone's forked copy, aligning with upstream changes from HADOOP-15169 and HADOOP-19546. Currently, Ozone's web endpoints (OM, SCM, Datanode, S3Gateway, Recon, HttpFS, Freon) only support excluding cipher suites via ssl.server.exclude.cipher.list and have no mechanism to restrict TLS protocol versions — both features that Hadoop's HttpServer2 already provides. This change adds the includeCiphers builder field and method to HttpServer2, ports the setEnabledProtocols() method that honors the hadoop.ssl.enabled.protocols configuration key, wires the new ssl.server.include.cipher.list config through BaseHttpServer.loadSslConfToHttpServerBuilder(), and adds comprehensive SSL tests verifying that cipher exclusion/inclusion and protocol restrictions are actually enforced at the TLS handshake level — a test coverage gap that exists today. All required constants (SSLFactory.SSL_SERVER_INCLUDE_CIPHER_LIST, SSLFactory.SSL_ENABLED_PROTOCOLS_KEY) are already available in Ozone's existing Hadoop 3.4.3 dependency, so no version bump is needed.
The changeset is generated by Claude - Opus 4.6
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10819
How was this patch tested?
JUnit tests are added.