ashishkumar50 commented on code in PR #4934:
URL: https://github.com/apache/ozone/pull/4934#discussion_r1234703987
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/protocol/SCMSecurityProtocol.java:
##########
@@ -164,11 +164,19 @@ long revokeCertificates(List<String> certIds, int reason,
long revocationTime)
* Get SCM signed certificate.
*
* @param nodeDetails - Node Details.
- * @param certSignReq - Certificate signing request.
+ * @param certSignReq - Certificate signing request.
* @return String - pem encoded SCM signed
- * certificate.
+ * certificate.
*/
String getCertificate(NodeDetailsProto nodeDetails,
String certSignReq) throws IOException;
+ /**
+ * Get all root CA certificates known to SCM.
+ *
+ * @return String - pem encoded concatanation of root CA certificates
Review Comment:
Typo correct to: `concatenation`
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/protocol/SCMSecurityProtocol.java:
##########
@@ -164,11 +164,19 @@ long revokeCertificates(List<String> certIds, int reason,
long revocationTime)
* Get SCM signed certificate.
*
* @param nodeDetails - Node Details.
- * @param certSignReq - Certificate signing request.
+ * @param certSignReq - Certificate signing request.
* @return String - pem encoded SCM signed
- * certificate.
+ * certificate.
*/
String getCertificate(NodeDetailsProto nodeDetails,
String certSignReq) throws IOException;
+ /**
+ * Get all root CA certificates known to SCM.
+ *
+ * @return String - pem encoded concatanation of root CA certificates
+ * @return
Review Comment:
Remove extra return from javadoc.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMSecurityProtocolServer.java:
##########
@@ -403,13 +415,20 @@ public List<String> listCACertificate() throws
IOException {
}
@Override
- public String getRootCACertificate() throws IOException {
- LOGGER.debug("Getting Root CA certificate.");
- if (storageContainerManager.getScmStorageConfig()
- .checkPrimarySCMIdInitialized()) {
- return CertificateCodec.getPEMEncodedString(rootCACertificate);
+ public synchronized String getRootCACertificate() throws IOException {
Review Comment:
Do we need synchronized here?, The old existing method is without
synchronized.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java:
##########
@@ -879,14 +880,20 @@ certificateStore, new DefaultCAProfile(),
SecretKeyManager secretKeyManager = secretKeyManagerService != null ?
secretKeyManagerService.getSecretKeyManager() : null;
+ X509Certificate rootCaCert = scmCertificateClient == null ? null :
+ scmCertificateClient.getRootCACertificate() != null ?
+ scmCertificateClient.getRootCACertificate() :
+ scmCertificateClient.getCACertificate();
+ ArrayList<X509Certificate> rootCaList = new ArrayList<>();
Review Comment:
```suggestion
List<X509Certificate> rootCaList = new ArrayList<>();
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMSecurityProtocolServer.java:
##########
@@ -403,13 +415,20 @@ public List<String> listCACertificate() throws
IOException {
}
@Override
- public String getRootCACertificate() throws IOException {
- LOGGER.debug("Getting Root CA certificate.");
- if (storageContainerManager.getScmStorageConfig()
- .checkPrimarySCMIdInitialized()) {
- return CertificateCodec.getPEMEncodedString(rootCACertificate);
+ public synchronized String getRootCACertificate() throws IOException {
+ Date lastCertDate = new Date(0);
+ X509Certificate lastCert = null;
+ for (X509Certificate cert : rootCACertificate) {
+ if (cert.getNotAfter().after(lastCertDate)) {
+ lastCertDate = cert.getNotAfter();
+ lastCert = cert;
+ }
}
- return null;
+ return CertificateCodec.getPEMEncodedString(lastCert);
Review Comment:
Before calling need to ensure `lastCert` is not NULL or else it will lead to
NPE in `CertificateCodec .getPEMEncodedString`
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMSecurityProtocolServer.java:
##########
@@ -232,6 +233,17 @@ private void validateSecretKeyStatus() throws
SCMSecretKeyException {
}
}
+ @Override
+ public synchronized List<String> getAllRootCaCertificates()
Review Comment:
Do we need synchronized here? May not be required.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMSecurityProtocolServer.java:
##########
@@ -232,6 +233,17 @@ private void validateSecretKeyStatus() throws
SCMSecretKeyException {
}
}
+ @Override
+ public synchronized List<String> getAllRootCaCertificates()
+ throws IOException {
+ ArrayList<String> pemEncodedList =
Review Comment:
```suggestion
List<String> pemEncodedList =
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMSecurityProtocolServer.java:
##########
@@ -403,13 +415,20 @@ public List<String> listCACertificate() throws
IOException {
}
@Override
- public String getRootCACertificate() throws IOException {
- LOGGER.debug("Getting Root CA certificate.");
- if (storageContainerManager.getScmStorageConfig()
- .checkPrimarySCMIdInitialized()) {
- return CertificateCodec.getPEMEncodedString(rootCACertificate);
+ public synchronized String getRootCACertificate() throws IOException {
+ Date lastCertDate = new Date(0);
+ X509Certificate lastCert = null;
+ for (X509Certificate cert : rootCACertificate) {
+ if (cert.getNotAfter().after(lastCertDate)) {
+ lastCertDate = cert.getNotAfter();
+ lastCert = cert;
+ }
}
- return null;
+ return CertificateCodec.getPEMEncodedString(lastCert);
+ }
+
+ public synchronized void addNewRootCa(X509Certificate rootCaCertToAdd) {
Review Comment:
I can see this is used only by test code, can you mark this with test
annotation. Also synchronized is not required.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java:
##########
@@ -879,14 +880,20 @@ certificateStore, new DefaultCAProfile(),
SecretKeyManager secretKeyManager = secretKeyManagerService != null ?
secretKeyManagerService.getSecretKeyManager() : null;
+ X509Certificate rootCaCert = scmCertificateClient == null ? null :
+ scmCertificateClient.getRootCACertificate() != null ?
+ scmCertificateClient.getRootCACertificate() :
+ scmCertificateClient.getCACertificate();
+ ArrayList<X509Certificate> rootCaList = new ArrayList<>();
+ rootCaList.add(rootCaCert);
Review Comment:
If `rootCaCert` is null based on above condition, in that case we should not
add into `rootCaList`.
--
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]
