[GitHub] [ozone] xiaoyuyao commented on a change in pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.

[GitBox <gi...@apache.org>]

xiaoyuyao commented on a change in pull request #1958:
URL: https://github.com/apache/ozone/pull/1958#discussion_r582477137



##########
File path: hadoop-hdds/interface-server/src/main/proto/ScmServerSecurityProtocol.proto
##########
@@ -49,6 +49,8 @@ message SCMSecurityRequest {
     optional SCMGetCertificateRequestProto getCertificateRequest = 5;
     optional SCMGetCACertificateRequestProto getCACertificateRequest = 6;
     optional SCMListCertificateRequestProto listCertificateRequest = 7;
+    optional SCMGeneratePeerSCMCertRequestProto

Review comment:
       the root ca scm also need to call this, can we remove the Peer here?

##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/protocol/SCMSecurityProtocol.java
##########
@@ -62,6 +63,18 @@ String getDataNodeCertificate(
   String getOMCertificate(OzoneManagerDetailsProto omDetails,
       String certSignReq) throws IOException;
 
+
+  /**
+   * Get SCM signed certificate for OM.
+   *
+   * @param scmNodeDetails       - DataNode Details.

Review comment:
       NIT: DataNode Details should be scm node details.

##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMCertStore.java
##########
@@ -93,6 +114,8 @@ public void revokeCertificate(BigInteger serialID) throws IOException {
                scmMetadataStore.getStore().initBatchOperation();) {
         scmMetadataStore.getRevokedCertsTable()
             .putWithBatch(batch, serialID, cert);
+        scmMetadataStore.getValidSCMCertsTable().deleteWithBatch(batch,

Review comment:
       we only need this if the revoked cert is a scm cert. 

##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/metadata/SCMDBDefinition.java
##########
@@ -105,6 +114,6 @@ public String getLocationConfigKey() {
   @Override
   public DBColumnFamilyDefinition[] getColumnFamilies() {
     return new DBColumnFamilyDefinition[] {DELETED_BLOCKS, VALID_CERTS,
-        REVOKED_CERTS, PIPELINES, CONTAINERS, TRANSACTIONINFO};
+        VALID_SCM_CERTS, REVOKED_CERTS, PIPELINES, CONTAINERS, TRANSACTIONINFO};

Review comment:
       we might also need a REVOKED_SCM_CERTS with the CRL work given we are putting cert of different roles into different table with this change. 

##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/authority/CertificateServer.java
##########
@@ -82,7 +82,7 @@ X509Certificate getCertificate(String certSerialId)
    */
   Future<X509CertificateHolder> requestCertificate(
       PKCS10CertificationRequest csr,
-      CertificateApprover.ApprovalType type)
+      CertificateApprover.ApprovalType type, boolean scmCert)

Review comment:
       can we use a enum for role instead with om/scm/dn for now, that might help query certificate by role 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org