[
https://issues.apache.org/jira/browse/HADOOP-12668?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15152744#comment-15152744
]
Zhe Zhang commented on HADOOP-12668:
------------------------------------
Thanks for the new patch [~SINGHVJD]. The main code change looks good. A few
minors on the test code:
# {{PrefferedCipherSSLSocketFactory}} is a good idea to handle different
enabled ciphers on local test machines. Thanks for the thoughtful work. However
it shouldn't be in the {{test}} package instead of {{main}}. Since it's only
used by {{TestSSLHttpServer}} we should actually make it an inner class.
# Ideally we should verify 2 things: 1) when the set of enabled ciphers is not
fully covered by the set of excluded ciphers, SSL handshake should work; 2)
when the set of enabled ciphers is fully covered by excluded ciphers, handshake
should fail. We can add a separate {{enabledCiphers}} which doesn't overlap
with {{excludeCiphers}}:
{code}
private static final String enabledCiphers = "XXXX";
...
PrefferedCipherSSLSocketFactory testPreferredCipherSSLSocketFactory
= new PrefferedCipherSSLSocketFactory(sslSocketFactory,
enabledCiphers.split(","));
// Try handshake, should work
PrefferedCipherSSLSocketFactory testPreferredCipherSSLSocketFactory
= new PrefferedCipherSSLSocketFactory(sslSocketFactory,
excludeCiphers.split(","));
// Try handshake again, should fail
{code}
This is optional though. Up to you whether to add it. I tried the
{{enabledCiphers}} idea locally, saw expected behavior.
# A better way to assert a test failure is to add {{fail}} in the {{try}}
segment:
{code}
try {
InputStream in = conn.getInputStream();
ByteArrayOutputStream out = new ByteArrayOutputStream();
IOUtils.copyBytes(in, out, 1024);
fail("SSLHandshake shouldn't pass xxxx");
} catch (SSLHandshakeException ex) {
LOG.info("No Ciphers in common, expected behavior", ex);
}
{code}
# {{testExcludedCiphers}} has some unused local variables: {{excludedCiphers}},
{{response}}
# Typo "succeded"
+1 after addressing the above
> Modify HDFS embeded jetty server logic in HttpServer2.java to exclude weak
> Ciphers through ssl-server.conf
> ----------------------------------------------------------------------------------------------------------
>
> Key: HADOOP-12668
> URL: https://issues.apache.org/jira/browse/HADOOP-12668
> Project: Hadoop Common
> Issue Type: Improvement
> Components: security
> Affects Versions: 2.7.1
> Reporter: Vijay Singh
> Assignee: Vijay Singh
> Priority: Critical
> Labels: common, ha, hadoop, hdfs, security
> Attachments: Hadoop-12668.006.patch, Hadoop-12668.007.patch,
> Hadoop-12668.008.patch, Hadoop-12668.009.patch, test.log
>
> Original Estimate: 24h
> Remaining Estimate: 24h
>
> Currently Embeded jetty Server used across all hadoop services is configured
> through ssl-server.xml file from their respective configuration section.
> However, the SSL/TLS protocol being used for this jetty servers can be
> downgraded to weak cipher suites. This code changes aims to add following
> functionality:
> 1) Add logic in hadoop common (HttpServer2.java and associated interfaces) to
> spawn jetty servers with ability to exclude weak cipher suites. I propose we
> make this though ssl-server.xml and hence each service can choose to disable
> specific ciphers.
> 2) Modify DFSUtil.java used by HDFS code to supply new parameter
> ssl.server.exclude.cipher.list for hadoop-common code, so it can exclude the
> ciphers supplied through this key.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
