pjfanning commented on code in PR #8703:
URL: https://github.com/apache/hadoop/pull/8703#discussion_r4400908509
##########
hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/CertificateUtil.java:
##########
@@ -57,7 +86,7 @@ public static RSAPublicKey parseRSAPublicKey(String pem)
throws ServletException
} else {
message = "CertificateException - PEM may be corrupt";
}
- throw new ServletException(message, ce);
+ throw new CertificateException(message, ce);
}
return (RSAPublicKey) key;
Review Comment:
This was already the case, but since `toRSAPublicKey` is new API documented
as reporting parse failures via `CertificateException`: a valid non-RSA
certificate (EC/DSA) gets through `generateCertificate` and then throws an
unchecked `ClassCastException` here. Suggest:
```java
if (!(key instanceof RSAPublicKey)) {
throw new CertificateException("Public key is not an RSA key: " +
key.getAlgorithm());
}
return (RSAPublicKey) key;
```
##########
hadoop-common-project/hadoop-auth/src/test/java/org/apache/hadoop/security/authentication/util/TestCertificateUtil.java:
##########
@@ -87,12 +89,62 @@ public void testValidPEM() throws Exception {
+
"9aTyR+HGHCfvwoCegc9rAVw/DLaRriSO/jnEXzYK6XLVKH+hx5UXrJ7Oyc7JjZUc3g9kCWORThCX"
+ "Mzc1xA==";
try {
- RSAPublicKey pk = CertificateUtil.parseRSAPublicKey(pem);
+ RSAPublicKey pk = CertificateUtil.toRSAPublicKey(pem);
assertNotNull(pk);
assertEquals("RSA", pk.getAlgorithm());
+ } catch (CertificateException se) {
+ fail("Should not have thrown CertificateException");
+ }
+ }
+
+ /**
+ * The deprecated entry point still reports a ServletException, so callers
+ * written against it keep compiling and keep catching what they caught
+ * before.
+ */
+ @Test
+ @SuppressWarnings("deprecation")
+ public void testDeprecatedEntryPointStillThrowsServletException() {
+ String pem = "not a certificate";
+ try {
+ CertificateUtil.parseRSAPublicKey(pem);
+ fail("Should not have thrown ServletException");
Review Comment:
The failure message reads the wrong way round: this `fail` runs when
`ServletException` was *not* thrown, so it should be something like `"Should
have thrown ServletException"`. Same at line 128.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
