fapifta commented on code in PR #4231:
URL: https://github.com/apache/ozone/pull/4231#discussion_r1103379009
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/CertificateClientTest.java:
##########
@@ -264,7 +279,8 @@ public void renewKey() throws Exception {
"CN=OzoneMaster", keyPair, 30, "SHA256withRSA");
keyPair = newKeyPair;
- x509Certificate = newCert;
+ CertificateFactory fact = CertificateFactory.getInstance("X.509");
Review Comment:
This and the next line repeatedly appear in two forms:
Based on java.security.cert.CertificateFactory this way:
```
CertificateFactory fact = CertificateFactory.getInstance("X.509");
certPath = fact.generateCertPath(ImmutableList.of(someCert));
```
and
Based on the BouncyCastle Certificate factory this way:
```
CertificateFactory factory = new CertificateFactory();
return factory.engineGenerateCertPath(updatedList);
```
We should I believe extract this to a method and use the same
CertificateFactory for our purposes. I am not sure what is the difference
though, so if it is justified I am fine with using both approach in parallel,
if this is the case, please let me know the justification please. This same
code appears in production and test code as well, similarly to
getTargetCertificate/firstCertificateFrom. We might put these into
CertificateCodec or a utility class besides the CertificateCodec, so we can
cover all usages of these two methods all over the code.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -800,9 +826,13 @@ public synchronized InitResponse init() throws
CertificateException {
return handleCase(init);
}
+ private X509Certificate getTargetCertificate(CertPath certificatePath) {
Review Comment:
This method should be utilized in more places within this class, there are a
couple occurrences where the same logic is there, that can be replaced with the
method call.
Also what do you think about calling it firstCertificateFrom?
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -292,22 +291,31 @@ public PublicKey getPublicKey() {
try {
publicKey = keyCodec.readPublicKey();
} catch (InvalidKeySpecException | NoSuchAlgorithmException
- | IOException e) {
+ | IOException e) {
Review Comment:
I don't think this indentation change is useful. There is no clear rule on
this I could find quickly, but based on what we usually do with function
parameters, I think the right indentation is 4 spaces here instead of 8.
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/certificate/authority/TestDefaultCAServer.java:
##########
@@ -197,11 +200,14 @@ public void testRequestCertificate() throws IOException,
testCA.init(new SecurityConfig(conf),
SELF_SIGNED_CA);
- Future<X509CertificateHolder> holder = testCA.requestCertificate(csrString,
- CertificateApprover.ApprovalType.TESTING_AUTOMATIC, SCM);
+ Future<CertPath> holder = testCA.requestCertificate(
+ csrString, CertificateApprover.ApprovalType.TESTING_AUTOMATIC, SCM);
// Right now our calls are synchronous. Eventually this will have to wait.
assertTrue(holder.isDone());
- assertNotNull(holder.get());
+ //Test that the cert path returned contains the CA certificate in proper
+ // place
+ assertEquals(holder.get().getCertificates().get(1),
Review Comment:
Shouldn't we assert as well that the returned certificate at the first place
matches the data that we specified in the CSR?
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/certificate/utils/TestCertificateCodec.java:
##########
@@ -33,27 +35,30 @@
import java.nio.file.Path;
import java.security.NoSuchAlgorithmException;
import java.security.NoSuchProviderException;
+import java.security.cert.CertPath;
+import java.security.cert.Certificate;
import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
import java.time.LocalDateTime;
import static org.apache.hadoop.hdds.HddsConfigKeys.OZONE_METADATA_DIRS;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
/**
* Tests the Certificate codecs.
*/
public class TestCertificateCodec {
- private static OzoneConfiguration conf = new OzoneConfiguration();
+ private static final OzoneConfiguration CONF = new OzoneConfiguration();
Review Comment:
I am not sure if this is the right direction...
If you look at the usage of the conf instance, at the moment, none of the
tests use it for anything, but that might change later, and in that case having
a static final reference may have unintended side effects.
As the conf is not needed in any static context, I would move it to the
instance level by removing the static keyword instead of making it final, and
would create a new instance for every test in the init method marked with the
@BeforeEach annotation to run before all tests. What do you think about this
approach?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/HASecurityUtils.java:
##########
@@ -192,24 +192,26 @@ private static void
getPrimarySCMSelfSignedCert(CertificateClient client,
PKCS10CertificationRequest csr = generateCSR(client, scmStorageConfig,
config, scmAddress);
- X509CertificateHolder subSCMCertHolder = rootCAServer.
+ CertPath subSCMCertHolderList = rootCAServer.
requestCertificate(csr, KERBEROS_TRUSTED, SCM).get();
- X509CertificateHolder rootCACertificateHolder =
- rootCAServer.getCACertificate();
+ CertPath rootCACertificatePath =
+ rootCAServer.getCaCertPath();
String pemEncodedCert =
- CertificateCodec.getPEMEncodedString(subSCMCertHolder);
-
+ CertificateCodec.getPEMEncodedString(subSCMCertHolderList);
+
Review Comment:
Can we remove these spaces please?
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/certificate/authority/TestDefaultCAServer.java:
##########
@@ -197,11 +200,14 @@ public void testRequestCertificate() throws IOException,
testCA.init(new SecurityConfig(conf),
SELF_SIGNED_CA);
- Future<X509CertificateHolder> holder = testCA.requestCertificate(csrString,
- CertificateApprover.ApprovalType.TESTING_AUTOMATIC, SCM);
+ Future<CertPath> holder = testCA.requestCertificate(
+ csrString, CertificateApprover.ApprovalType.TESTING_AUTOMATIC, SCM);
// Right now our calls are synchronous. Eventually this will have to wait.
assertTrue(holder.isDone());
- assertNotNull(holder.get());
+ //Test that the cert path returned contains the CA certificate in proper
+ // place
+ assertEquals(holder.get().getCertificates().get(1),
Review Comment:
Shouldn't we assert as well that the returned certificate at the first place
matches the data that we specified in the CSR?
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/CertificateClient.java:
##########
@@ -365,7 +379,17 @@ default void assertValidKeysAndCertificate() throws
OzoneSecurityException {
/**
* Register a receiver that will be called after the certificate renewed.
+ *
* @param receiver
*/
void registerNotificationReceiver(CertificateNotification receiver);
+
+ /**
+ * Type for specifying the type of the certificate to be stored.
+ */
+ enum CertType {
Review Comment:
What if we add a fileNamePrefix to the enum, and a getter for that, so we
can use polymorphism later on in the storeCertificate method, where we
currently have an if statement to determine the prefix?
On the other hand, the name of this enum and its values are a bit
problematic I believe. There is an other CertType enum used in the
CertificateStore class that is used by the CA server to store the certificates
it signs, I see a potential ambiguity between the two, and so that made me
think of a better name for this.
The real role of this seems to be to provide a ternary logic construct to
decide if a certificate is CA and if CA then it is a ROOT_CA certificate. So an
idea might be to change this to something like CAType, and have values like
SUBORDINATE, ROOT, and NONE.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -601,58 +621,64 @@ public X509Certificate queryCertificate(String query) {
* Stores the Certificate for this client. Don't use this api to add trusted
* certificates of others.
*
- * @param pemEncodedCert - pem encoded X509 Certificate
- * @param force - override any existing file
+ * @param pemEncodedCert - pem encoded X509 Certificate
* @throws CertificateException - on Error.
- *
*/
@Override
- public void storeCertificate(String pemEncodedCert, boolean force)
+ public void storeCertificate(String pemEncodedCert)
throws CertificateException {
- this.storeCertificate(pemEncodedCert, force, false);
+ this.storeCertificate(pemEncodedCert, false);
}
/**
* Stores the Certificate for this client. Don't use this api to add trusted
* certificates of others.
*
- * @param pemEncodedCert - pem encoded X509 Certificate
- * @param force - override any existing file
- * @param caCert - Is CA certificate.
+ * @param pemEncodedCert - pem encoded X509 Certificate
+ * @param caCert - Is CA certificate.
* @throws CertificateException - on Error.
- *
*/
@Override
- public void storeCertificate(String pemEncodedCert, boolean force,
+ public void storeCertificate(String pemEncodedCert,
Review Comment:
Shouldn't we use the CertType here as well to define if the certificate to
be stored is a CA cert or not?
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/certificate/utils/TestCertificateCodec.java:
##########
@@ -104,6 +94,50 @@ public void testGetPEMEncodedString()
assertEquals(firstCert, secondCert);
}
+ /**
+ * Test when converting a certificate path to pem encoded string and back
+ * we get back the same certificates.
+ */
+ @Test
+ public void testGetPemEncodedStringFromCertPath() throws IOException,
+ NoSuchAlgorithmException, NoSuchProviderException, CertificateException {
+ X509CertificateHolder certHolder1 = generateTestCert();
+ X509CertificateHolder certHolder2 = generateTestCert();
+ X509Certificate cert1 = CertificateCodec.getX509Certificate(certHolder1);
+ X509Certificate cert2 = CertificateCodec.getX509Certificate(certHolder2);
+ CertificateFactory certificateFactory = new CertificateFactory();
+ CertPath pathToEncode =
+ certificateFactory.engineGenerateCertPath(ImmutableList.of(cert1,
+ cert2));
+ String encodedPath = CertificateCodec.getPEMEncodedString(pathToEncode);
Review Comment:
Just a thought, in these tests, not just this one, in all of these tests, we
are testing the CertificateCodec class in a way that we generate a certificate
object, use the codec to re-write the certificate into PEM format, and then we
are reading back the PEM format string as a certificate object, and finally we
are checking for equality.
There is a potential flaw here, namely if the codec itself is able to
generate a PEM format string, and read it back, but nothing else can read that
PEM format string.
Our codec relies on underlying libraries that we do not want to test, but I
think we should at least have one test that ensures that the PEM file we are
writing out is readable with the standard openssl tool.
Please let me know if you agree, and if so, let's create a note/ticket for
us to review these tests, and handle this later on in a separate PR.
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/certificate/client/TestDefaultCertificateClient.java:
##########
@@ -506,6 +506,7 @@ public void testRenewAndStoreKeyAndCertificate() throws
Exception {
.newBuilder().setResponseCode(SCMSecurityProtocolProtos
.SCMGetCertResponseProto.ResponseCode.success)
.setX509Certificate(pemCert)
+ .setX509Certificate(pemCert)
Review Comment:
I think this one is either a typo or an accident... :) Can you please take a
look what was your intent here?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/security/OMCertificateClient.java:
##########
@@ -162,26 +164,26 @@ public CertificateSignRequest.Builder
getCSRBuilder(KeyPair keyPair)
@Override
public String signAndStoreCertificate(PKCS10CertificationRequest request,
- Path certPath) throws CertificateException {
+ Path certificatePath) throws CertificateException {
try {
SCMGetCertResponseProto response = getScmSecureClient()
.getOMCertChain(omInfo, getEncodedString(request));
String pemEncodedCert = response.getX509Certificate();
CertificateCodec certCodec = new CertificateCodec(
- getSecurityConfig(), certPath);
+ getSecurityConfig(), certificatePath);
// Store SCM CA certificate.
if (response.hasX509CACertificate()) {
String pemEncodedRootCert = response.getX509CACertificate();
storeCertificate(pemEncodedRootCert,
- true, true, false, certCodec, false);
- storeCertificate(pemEncodedCert, true, false, false, certCodec, false);
+ CA, certCodec, false);
+ storeCertificate(pemEncodedCert, INTERMEDIATE, certCodec, false);
// Store Root CA certificate if available.
if (response.hasX509RootCACertificate()) {
storeCertificate(response.getX509RootCACertificate(),
- true, false, true, certCodec, false);
+ CertType.ROOT_CA, certCodec, false);
Review Comment:
This one I believe also happens in a few places, where you have statically
imported the INTERMEDIATE CertType, while later you reference the ROOT_CA type
with the classname classified. Can you please unify these as well, and either
use CertType.INTERMEDIATE similarly to CertType.ROOT_CA, or import statically
the ROOT_CA value as well?
--
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]
