Galsza commented on code in PR #4934:
URL: https://github.com/apache/ozone/pull/4934#discussion_r1234874291
##########
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:
Yes, since the list of the root cas can change. The old method didn't need
synch because the whole server could only ever use one root ca
##########
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:
Yes, in the end the rootCas might change once root ca rotation is
implemented and this is going to fail with ConcurrentModificationException if
we don't synchronize
##########
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:
Thanks, nice catch
##########
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:
This method is going to be used later once the root ca certificate rotation
is implemented. It's here because it makes sense as well as we can ensure that
the sync is properly placed.
##########
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:
Thank you, that was a nice catch, I've added functionality to handle that
--
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]
