[GitHub] [ozone] GlenGeng commented on a change in pull request #1662: HDDS-4507. Add SCM CA CLI to query certificate.

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

GlenGeng commented on a change in pull request #1662:
URL: https://github.com/apache/ozone/pull/1662#discussion_r537267042



##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMCertStore.java
##########
@@ -112,4 +117,39 @@ public X509Certificate getCertificateByID(BigInteger serialID,
       return scmMetadataStore.getRevokedCertsTable().get(serialID);
     }
   }
+
+  @Override
+  public List<X509Certificate> listCertificate(HddsProtos.NodeType role,
+      BigInteger startSerialID, int count, CertType certType)
+      throws IOException {
+    // TODO: Filter by role
+    List<? extends Table.KeyValue<BigInteger, X509Certificate>> certs;
+    if (startSerialID.longValue() == 0) {
+      startSerialID = null;
+    }
+    if (certType == CertType.VALID_CERTS) {
+      certs = scmMetadataStore.getValidCertsTable().getRangeKVs(
+          startSerialID, count);
+    } else {
+      certs = scmMetadataStore.getRevokedCertsTable().getRangeKVs(
+          startSerialID, count);
+    }
+    List<X509Certificate> results = new ArrayList<>(certs.size());
+    for (Table.KeyValue<BigInteger, X509Certificate> kv : certs) {
+      try {
+        X509Certificate cert = kv.getValue();
+        // TODO: filter certificate based on CN and specified role.
+        // This requires change of the approved subject CN format:
+        // Subject: O=CID-e66d4728-32bb-4282-9770-351a7e913f07,
+        // OU=9a7c4f86-c862-4067-b12c-e7bca51d3dfe, CN=root@98dba189d5f0
+
+        // The new format will look like below that are easier to filter.
+        // CN=FQDN/user=root/role=datanode/...
+        results.add(cert);
+      } catch (IOException e) {
+        LOG.error("Fail to get certificate from SCM metadata store", e);

Review comment:
       Show we throw out the `IOException` here? The caller may not be aware of the exception happened here from the returned `results`.

##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMSecurityProtocolServer.java
##########
@@ -192,6 +195,33 @@ public String getCACertificate() throws IOException {
     }
   }
 
+  /**
+   *
+   * @param role            - node role: OM/SCM/DN.
+   * @param startSerialId   - start certificate serial id.
+   * @param count           - max number of certificates returned in a batch.
+   * @param isRevoked       - whether list for revoked certs only.
+   * @return
+   * @throws IOException
+   */
+  @Override
+  public List<String> listCertificate(HddsProtos.NodeType role,
+      long startSerialId, int count, boolean isRevoked) throws IOException {
+    List<X509Certificate> certificates =
+        certificateServer.listCertificate(role, startSerialId, count,
+            isRevoked);
+    List<String> results = new ArrayList<>(certificates.size());
+    for (X509Certificate cert : certificates) {
+      try {
+        String certStr = CertificateCodec.getPEMEncodedString(cert);
+        results.add(certStr);
+      } catch (SCMSecurityException e) {
+        LOGGER.error("listCertificate fail to encode certificate.", e);

Review comment:
       ditto




----------------------------------------------------------------
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