Skip to content

Commit 4254782

Browse files
bbeaudreaultbbeaudreault
committed
Review fixes, simplify test config a bit
1 parent 49bca7d commit 4254782

5 files changed

Lines changed: 72 additions & 100 deletions

File tree

‎hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseTrustManager.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ public class HBaseTrustManager extends X509ExtendedTrustManager {
4949
private final HBaseHostnameVerifier hostnameVerifier;
5050

5151
/**
52-
* Instantiate a new ZKTrustManager.
52+
* Instantiate a new HBaseTrustManager.
5353
* @param x509ExtendedTrustManager The trustmanager to use for
5454
* checkClientTrusted/checkServerTrusted logic
5555
* @param serverHostnameVerificationEnabled If true, this TrustManager should verify hostnames of

‎hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/X509Util.java‎

Lines changed: 25 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -80,13 +80,13 @@ public final class X509Util {
8080
private static final String TLS_ENABLED_PROTOCOLS = CONFIG_PREFIX + "enabledProtocols";
8181
private static final String TLS_CIPHER_SUITES = CONFIG_PREFIX + "ciphersuites";
8282

83-
public static final String TLS_CONFIG_VERIFY_CLIENT_HOSTNAMES =
84-
CONFIG_PREFIX + "verify.client.hostnames";
85-
public static final String TLS_CONFIG_VERIFY_SERVER_HOSTNAMES =
86-
CONFIG_PREFIX + "verify.server.hostnames";
87-
8883
public static final String TLS_CONFIG_CLIENT_AUTH_MODE = CONFIG_PREFIX + "client.auth.mode";
8984

85+
public static final String HBASE_SERVER_NETTY_TLS_VERIFY_CLIENT_HOSTNAME =
86+
"hbase.server.netty.tls.verify.client.hostname";
87+
public static final String HBASE_CLIENT_NETTY_TLS_VERIFY_SERVER_HOSTNAME =
88+
"hbase.client.netty.tls.verify.server.hostname";
89+
9090
public static final String HBASE_CLIENT_NETTY_TLS_ENABLED = "hbase.client.netty.tls.enabled";
9191
public static final String HBASE_SERVER_NETTY_TLS_ENABLED = "hbase.server.netty.tls.enabled";
9292

@@ -210,13 +210,14 @@ public static SslContext createSslContextForClient(Configuration config)
210210
boolean sslCrlEnabled = config.getBoolean(TLS_CONFIG_CLR, false);
211211
boolean sslOcspEnabled = config.getBoolean(TLS_CONFIG_OCSP, false);
212212

213-
boolean verifyServerHostnames = config.getBoolean(TLS_CONFIG_VERIFY_SERVER_HOSTNAMES, true);
213+
boolean verifyServerHostname =
214+
config.getBoolean(HBASE_CLIENT_NETTY_TLS_VERIFY_SERVER_HOSTNAME, true);
214215

215216
if (trustStoreLocation.isEmpty()) {
216217
LOG.warn(TLS_CONFIG_TRUSTSTORE_LOCATION + " not specified");
217218
} else {
218219
sslContextBuilder.trustManager(createTrustManager(trustStoreLocation, trustStorePassword,
219-
trustStoreType, sslCrlEnabled, sslOcspEnabled, verifyServerHostnames, false));
220+
trustStoreType, sslCrlEnabled, sslOcspEnabled, verifyServerHostname, false));
220221
}
221222

222223
sslContextBuilder.enableOcsp(sslOcspEnabled);
@@ -252,13 +253,14 @@ public static SslContext createSslContextForServer(Configuration config)
252253
boolean sslOcspEnabled = config.getBoolean(TLS_CONFIG_OCSP, false);
253254

254255
ClientAuth clientAuth = ClientAuth.fromPropertyValue(config.get(TLS_CONFIG_CLIENT_AUTH_MODE));
255-
boolean verifyClientHostnames = config.getBoolean(TLS_CONFIG_VERIFY_CLIENT_HOSTNAMES, true);
256+
boolean verifyClientHostname =
257+
config.getBoolean(HBASE_SERVER_NETTY_TLS_VERIFY_CLIENT_HOSTNAME, true);
256258

257259
if (trustStoreLocation.isEmpty()) {
258260
LOG.warn(TLS_CONFIG_TRUSTSTORE_LOCATION + " not specified");
259261
} else {
260262
sslContextBuilder.trustManager(createTrustManager(trustStoreLocation, trustStorePassword,
261-
trustStoreType, sslCrlEnabled, sslOcspEnabled, false, verifyClientHostnames));
263+
trustStoreType, sslCrlEnabled, sslOcspEnabled, false, verifyClientHostname));
262264
}
263265

264266
sslContextBuilder.enableOcsp(sslOcspEnabled);
@@ -317,22 +319,22 @@ static X509KeyManager createKeyManager(String keyStoreLocation, String keyStoreP
317319
/**
318320
* Creates a trust manager by loading the trust store from the given file of the given type,
319321
* optionally decrypting it using the given password.
320-
* @param trustStoreLocation the location of the trust store file.
321-
* @param trustStorePassword optional password to decrypt the trust store (only applies to JKS
322-
* trust stores). If empty, assumes the trust store is not encrypted.
323-
* @param trustStoreType must be JKS, PEM, PKCS12, BCFKS or null. If null, attempts to
324-
* autodetect the trust store type from the file extension (e.g. .jks
325-
* / .pem).
326-
* @param crlEnabled enable CRL (certificate revocation list) checks.
327-
* @param ocspEnabled enable OCSP (online certificate status protocol) checks.
328-
* @param verifyClientHostnames if true, client hostname must match name in certificate
329-
* @param verifyServerHostnames if true, server hostname must match name in certificate
322+
* @param trustStoreLocation the location of the trust store file.
323+
* @param trustStorePassword optional password to decrypt the trust store (only applies to JKS
324+
* trust stores). If empty, assumes the trust store is not encrypted.
325+
* @param trustStoreType must be JKS, PEM, PKCS12, BCFKS or null. If null, attempts to
326+
* autodetect the trust store type from the file extension (e.g. .jks
327+
* / .pem).
328+
* @param crlEnabled enable CRL (certificate revocation list) checks.
329+
* @param ocspEnabled enable OCSP (online certificate status protocol) checks.
330+
* @param verifyClientHostname if true, client hostname must match name in certificate
331+
* @param verifyServerHostname if true, server hostname must match name in certificate
330332
* @return the trust manager.
331333
* @throws TrustManagerException if something goes wrong.
332334
*/
333335
static X509TrustManager createTrustManager(String trustStoreLocation, String trustStorePassword,
334-
String trustStoreType, boolean crlEnabled, boolean ocspEnabled, boolean verifyServerHostnames,
335-
boolean verifyClientHostnames) throws TrustManagerException {
336+
String trustStoreType, boolean crlEnabled, boolean ocspEnabled, boolean verifyServerHostname,
337+
boolean verifyClientHostname) throws TrustManagerException {
336338

337339
if (trustStorePassword == null) {
338340
trustStorePassword = "";
@@ -370,8 +372,8 @@ static X509TrustManager createTrustManager(String trustStoreLocation, String tru
370372

371373
for (final TrustManager tm : tmf.getTrustManagers()) {
372374
if (tm instanceof X509ExtendedTrustManager) {
373-
return new HBaseTrustManager((X509ExtendedTrustManager) tm, verifyServerHostnames,
374-
verifyClientHostnames);
375+
return new HBaseTrustManager((X509ExtendedTrustManager) tm, verifyServerHostname,
376+
verifyClientHostname);
375377
}
376378
}
377379
throw new TrustManagerException("Couldn't find X509TrustManager");

‎hbase-server/src/test/java/org/apache/hadoop/hbase/security/AbstractTestMutualTls.java‎

Lines changed: 10 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -86,12 +86,14 @@ public abstract class AbstractTestMutualTls {
8686

8787
@Parameterized.Parameter(2)
8888
public String keyPassword;
89-
9089
@Parameterized.Parameter(3)
91-
public boolean validateHostnames;
90+
public boolean expectSuccess;
9291

9392
@Parameterized.Parameter(4)
94-
public TestCase testCase;
93+
public boolean validateHostnames;
94+
95+
@Parameterized.Parameter(5)
96+
public CertConfig certConfig;
9597

9698
public enum CertConfig {
9799
// For no cert, we literally pass no certificate to the server. It's possible (assuming server
@@ -110,30 +112,6 @@ public enum CertConfig {
110112
VERIFIABLE_CERT_WITH_BAD_HOST
111113
}
112114

113-
public static class TestCase {
114-
public final CertConfig certConfig;
115-
public final boolean expectSuccessWithVerifyHost;
116-
public final boolean expectSuccessWithoutVerifyHost;
117-
118-
public TestCase(CertConfig certConfig, boolean expectSuccess) {
119-
this(certConfig, expectSuccess, expectSuccess);
120-
}
121-
122-
public TestCase(CertConfig certConfig, boolean expectSuccessWithVerifyHost,
123-
boolean expectSuccessWithoutVerifyHost) {
124-
this.certConfig = certConfig;
125-
this.expectSuccessWithVerifyHost = expectSuccessWithVerifyHost;
126-
this.expectSuccessWithoutVerifyHost = expectSuccessWithoutVerifyHost;
127-
}
128-
129-
@Override
130-
public String toString() {
131-
return "TestCase{" + "certConfig=" + certConfig + ", expectSuccessWithVerifyHost="
132-
+ expectSuccessWithVerifyHost + ", expectSuccessWithoutVerifyHost="
133-
+ expectSuccessWithoutVerifyHost + '}';
134-
}
135-
}
136-
137115
@BeforeClass
138116
public static void setUpBeforeClass() throws IOException {
139117
UTIL = new HBaseCommonTestingUtil();
@@ -159,11 +137,6 @@ public static void cleanUp() {
159137
UTIL.cleanupTestDir();
160138
}
161139

162-
public enum PeerType {
163-
CLIENT,
164-
SERVER
165-
}
166-
167140
protected abstract void initialize(Configuration serverConf, Configuration clientConf)
168141
throws IOException, GeneralSecurityException, OperatorCreationException;
169142

@@ -188,7 +161,7 @@ public void setUp() throws Exception {
188161

189162
protected void handleCertConfig(Configuration confToSet)
190163
throws GeneralSecurityException, IOException, OperatorCreationException {
191-
switch (testCase.certConfig) {
164+
switch (certConfig) {
192165
case NO_CLIENT_CERT:
193166
// clearing out the keystore location will cause no cert to be sent.
194167
confToSet.set(X509Util.TLS_CONFIG_KEYSTORE_LOCATION, "");
@@ -234,20 +207,15 @@ public void tearDown() throws IOException {
234207

235208
@Test
236209
public void testClientAuth() throws Exception {
237-
if (shouldThrow()) {
238-
ServiceException se = assertThrows(ServiceException.class, this::submitRequest);
239-
assertThat(se.getCause(), instanceOf(SSLHandshakeException.class));
240-
} else {
210+
if (expectSuccess) {
241211
// we expect no exception, so if one is thrown the test will fail
242212
submitRequest();
213+
} else {
214+
ServiceException se = assertThrows(ServiceException.class, this::submitRequest);
215+
assertThat(se.getCause(), instanceOf(SSLHandshakeException.class));
243216
}
244217
}
245218

246-
private boolean shouldThrow() {
247-
return validateHostnames && !testCase.expectSuccessWithVerifyHost
248-
|| !testCase.expectSuccessWithoutVerifyHost;
249-
}
250-
251219
private void submitRequest() throws ServiceException {
252220
stub.echo(null, TestProtos.EchoRequestProto.newBuilder().setMessage("hello world").build());
253221
}

‎hbase-server/src/test/java/org/apache/hadoop/hbase/security/TestMutualTlsClientSide.java‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -56,14 +56,14 @@ public static List<Object[]> data() {
5656
// we want to run with and without validating hostnames. we encode the expected success
5757
// criteria in the TestCase config. See below.
5858
for (boolean validateServerHostnames : new Boolean[] { true, false }) {
59-
// fail in all cases except "good cert/bad host" when host verification is disabled
60-
params.add(new Object[] { caKeyType, certKeyType, keyPassword, validateServerHostnames,
61-
new TestCase(CertConfig.NON_VERIFIABLE_CERT, false) });
62-
params.add(new Object[] { caKeyType, certKeyType, keyPassword, validateServerHostnames,
63-
new TestCase(CertConfig.VERIFIABLE_CERT_WITH_BAD_HOST, false, true) });
64-
// additionally ensure that it succeeds when a good cert is presented
65-
params.add(new Object[] { caKeyType, certKeyType, keyPassword, validateServerHostnames,
66-
new TestCase(CertConfig.GOOD_CERT, true) });
59+
// fail for non-verifiable certs or certs with bad hostnames when validateServerHostname
60+
// is true. otherwise succeed.
61+
params.add(new Object[] { caKeyType, certKeyType, keyPassword, false,
62+
validateServerHostnames, CertConfig.NON_VERIFIABLE_CERT });
63+
params.add(new Object[] { caKeyType, certKeyType, keyPassword, !validateServerHostnames,
64+
validateServerHostnames, CertConfig.VERIFIABLE_CERT_WITH_BAD_HOST });
65+
params.add(new Object[] { caKeyType, certKeyType, keyPassword, true,
66+
validateServerHostnames, CertConfig.GOOD_CERT });
6767
}
6868
}
6969
}
@@ -75,7 +75,8 @@ public static List<Object[]> data() {
7575
protected void initialize(Configuration serverConf, Configuration clientConf)
7676
throws IOException, GeneralSecurityException, OperatorCreationException {
7777
// client verifies server hostname, and injects bad certs into server conf
78-
clientConf.setBoolean(X509Util.TLS_CONFIG_VERIFY_SERVER_HOSTNAMES, validateHostnames);
78+
clientConf.setBoolean(X509Util.HBASE_CLIENT_NETTY_TLS_VERIFY_SERVER_HOSTNAME,
79+
validateHostnames);
7980
handleCertConfig(serverConf);
8081
}
8182
}

‎hbase-server/src/test/java/org/apache/hadoop/hbase/security/TestMutualTlsServerSide.java‎

Lines changed: 26 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ public class TestMutualTlsServerSide extends AbstractTestMutualTls {
4646
@ClassRule
4747
public static final HBaseClassTestRule CLASS_RULE =
4848
HBaseClassTestRule.forClass(TestMutualTlsServerSide.class);
49-
@Parameterized.Parameter(5)
49+
@Parameterized.Parameter(6)
5050
public X509Util.ClientAuth clientAuthMode;
5151

5252
@Parameterized.Parameters(
@@ -62,39 +62,39 @@ public static List<Object[]> data() {
6262
for (boolean validateClientHostnames : new Boolean[] { true, false }) {
6363
// ClientAuth.NONE should succeed in all cases, because it never requests the
6464
// certificate for verification
65-
params.add(new Object[] { caKeyType, certKeyType, keyPassword, validateClientHostnames,
66-
new TestCase(CertConfig.NO_CLIENT_CERT, true), X509Util.ClientAuth.NONE });
67-
params.add(new Object[] { caKeyType, certKeyType, keyPassword, validateClientHostnames,
68-
new TestCase(CertConfig.NON_VERIFIABLE_CERT, true), X509Util.ClientAuth.NONE });
69-
params.add(new Object[] { caKeyType, certKeyType, keyPassword, validateClientHostnames,
70-
new TestCase(CertConfig.VERIFIABLE_CERT_WITH_BAD_HOST, true),
71-
X509Util.ClientAuth.NONE });
65+
params.add(new Object[] { caKeyType, certKeyType, keyPassword, true,
66+
validateClientHostnames, CertConfig.NO_CLIENT_CERT, X509Util.ClientAuth.NONE });
67+
params.add(new Object[] { caKeyType, certKeyType, keyPassword, true,
68+
validateClientHostnames, CertConfig.NON_VERIFIABLE_CERT, X509Util.ClientAuth.NONE });
69+
params.add(
70+
new Object[] { caKeyType, certKeyType, keyPassword, true, validateClientHostnames,
71+
CertConfig.VERIFIABLE_CERT_WITH_BAD_HOST, X509Util.ClientAuth.NONE });
7272

7373
// ClientAuth.WANT should succeed if no cert, but if the cert is provided it is
74-
// validated. So should fail on
75-
// bad cert or good cert with bad host when host verification is enabled
76-
params.add(new Object[] { caKeyType, certKeyType, keyPassword, validateClientHostnames,
77-
new TestCase(CertConfig.NO_CLIENT_CERT, true), X509Util.ClientAuth.WANT });
78-
params.add(new Object[] { caKeyType, certKeyType, keyPassword, validateClientHostnames,
79-
new TestCase(CertConfig.NON_VERIFIABLE_CERT, false), X509Util.ClientAuth.WANT });
80-
params.add(new Object[] { caKeyType, certKeyType, keyPassword, validateClientHostnames,
81-
new TestCase(CertConfig.VERIFIABLE_CERT_WITH_BAD_HOST, false, true),
74+
// validated. So should fail on bad cert or good cert with bad host when host
75+
// verification is enabled
76+
params.add(new Object[] { caKeyType, certKeyType, keyPassword, true,
77+
validateClientHostnames, CertConfig.NO_CLIENT_CERT, X509Util.ClientAuth.WANT });
78+
params.add(new Object[] { caKeyType, certKeyType, keyPassword, false,
79+
validateClientHostnames, CertConfig.NON_VERIFIABLE_CERT, X509Util.ClientAuth.WANT });
80+
params.add(new Object[] { caKeyType, certKeyType, keyPassword, !validateClientHostnames,
81+
validateClientHostnames, CertConfig.VERIFIABLE_CERT_WITH_BAD_HOST,
8282
X509Util.ClientAuth.WANT });
8383

8484
// ClientAuth.NEED is most restrictive, failing in all cases except "good cert/bad host"
8585
// when host verification is disabled
86-
params.add(new Object[] { caKeyType, certKeyType, keyPassword, validateClientHostnames,
87-
new TestCase(CertConfig.NO_CLIENT_CERT, false), X509Util.ClientAuth.NEED });
88-
params.add(new Object[] { caKeyType, certKeyType, keyPassword, validateClientHostnames,
89-
new TestCase(CertConfig.NON_VERIFIABLE_CERT, false), X509Util.ClientAuth.NEED });
90-
params.add(new Object[] { caKeyType, certKeyType, keyPassword, validateClientHostnames,
91-
new TestCase(CertConfig.VERIFIABLE_CERT_WITH_BAD_HOST, false, true),
86+
params.add(new Object[] { caKeyType, certKeyType, keyPassword, false,
87+
validateClientHostnames, CertConfig.NO_CLIENT_CERT, X509Util.ClientAuth.NEED });
88+
params.add(new Object[] { caKeyType, certKeyType, keyPassword, false,
89+
validateClientHostnames, CertConfig.NON_VERIFIABLE_CERT, X509Util.ClientAuth.NEED });
90+
params.add(new Object[] { caKeyType, certKeyType, keyPassword, !validateClientHostnames,
91+
validateClientHostnames, CertConfig.VERIFIABLE_CERT_WITH_BAD_HOST,
9292
X509Util.ClientAuth.NEED });
9393

9494
// additionally ensure that all modes succeed when a good cert is presented
9595
for (X509Util.ClientAuth mode : X509Util.ClientAuth.values()) {
96-
params.add(new Object[] { caKeyType, certKeyType, keyPassword,
97-
validateClientHostnames, new TestCase(CertConfig.GOOD_CERT, true), mode });
96+
params.add(new Object[] { caKeyType, certKeyType, keyPassword, true,
97+
validateClientHostnames, CertConfig.GOOD_CERT, mode });
9898
}
9999
}
100100
}
@@ -109,7 +109,8 @@ protected void initialize(Configuration serverConf, Configuration clientConf)
109109
// server enables client auth mode and verifies client host names
110110
// inject bad certs into client side
111111
serverConf.set(X509Util.TLS_CONFIG_CLIENT_AUTH_MODE, clientAuthMode.name());
112-
serverConf.setBoolean(X509Util.TLS_CONFIG_VERIFY_CLIENT_HOSTNAMES, validateHostnames);
112+
serverConf.setBoolean(X509Util.HBASE_SERVER_NETTY_TLS_VERIFY_CLIENT_HOSTNAME,
113+
validateHostnames);
113114
handleCertConfig(clientConf);
114115
}
115116
}

0 commit comments

Comments
 (0)