You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2021/02/25 05:35:06 UTC
[GitHub] [ozone] adoroszlai commented on a change in pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
adoroszlai commented on a change in pull request #1958:
URL: https://github.com/apache/ozone/pull/1958#discussion_r582534634
##########
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.
Review comment:
```suggestion
* Get SCM signed certificate for SCM.
```
##########
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.
+ * @param certSignReq - Certificate signing request.
+ * @return String - pem encoded SCM signed
+ * certificate.
+ */
+ String generateSCMPeerCertificate(ScmNodeDetailsProto scmNodeDetails,
Review comment:
Nit: operations for generating OM and DN certs are called `get...`. I think it should match those. (Although I also think those might have better been `generate...` in the first place.)
##########
File path: hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/TestHddsUtils.java
##########
@@ -44,6 +46,8 @@
@Test
public void testGetHostName() {
+ UUID uii = UUID.randomUUID();
+ UUID.nameUUIDFromBytes(uii.toString().getBytes(StandardCharsets.UTF_8));
Review comment:
Can you please explain why this is needed? Result from `nameUUIDFromBytes` is ignored.
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/authority/DefaultCAServer.java
##########
@@ -252,23 +252,32 @@ private KeyPair getCAKeys() throws IOException {
}
private X509CertificateHolder signAndStoreCertificate(LocalDate beginDate,
- LocalDate endDate, PKCS10CertificationRequest csr) throws IOException,
+ LocalDate endDate, PKCS10CertificationRequest csr, boolean scmCert)
+ throws IOException,
OperatorCreationException, CertificateException {
X509CertificateHolder xcert = approver.sign(config,
getCAKeys().getPrivate(),
getCACertificate(), java.sql.Date.valueOf(beginDate),
java.sql.Date.valueOf(endDate), csr, scmID, clusterID);
- store.storeValidCertificate(xcert.getSerialNumber(),
- CertificateCodec.getX509Certificate(xcert));
+ if (scmCert) {
+ // If scm cert store in scm cert table and valid cert table.
+ // This is to help to return scm certs when get certs call.
+ store.storeValidScmCertificate(xcert.getSerialNumber(),
+ CertificateCodec.getX509Certificate(xcert));
Review comment:
Can you please explain why SCM certs need to be also stored in a separate table?
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMSecurityProtocolServer.java
##########
@@ -156,6 +159,44 @@ public String getOMCertificate(OzoneManagerDetailsProto omDetails,
}
}
+
+ /**
+ * Get SCM signed certificate for SCM peer Node.
+ *
+ * @param scmNodeDetails - SCM Node Details.
+ * @param certSignReq - Certificate signing request.
+ * @return String - SCM signed pem encoded certificate.
+ */
+ @Override
+ public String generateSCMPeerCertificate(ScmNodeDetailsProto scmNodeDetails,
+ String certSignReq) throws IOException {
+ Objects.requireNonNull(scmNodeDetails);
+ LOGGER.info("Processing CSR for scm {}, nodeId: {}",
+ scmNodeDetails.getHostName(), scmNodeDetails.getScmNodeId());
+
+ // Check clusterID
+ if (storageContainerManager.getClusterId().equals(
+ scmNodeDetails.getClusterId())) {
+ throw new IOException("SCM ClusterId mismatch. Peer SCM ClusterId, " +
+ scmNodeDetails.getClusterId() + "leader SCM ClusterId "
Review comment:
Isn't "leader" confusing here? If I understand correctly, this might precede Ratis leader election.
Also, a space is needed between clusterID and "leader" (or its replacement).
```suggestion
throw new IOException("SCM ClusterId mismatch. Peer SCM ClusterId " +
scmNodeDetails.getClusterId() + ", primary SCM ClusterId "
```
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/protocolPB/SCMSecurityProtocolClientSideTranslatorPB.java
##########
@@ -129,6 +131,26 @@ public String getOMCertificate(OzoneManagerDetailsProto omDetails,
return getOMCertChain(omDetails, certSignReq).getX509Certificate();
}
+ /**
+ * Get SCM signed certificate and root CA certificate for SCM Peer node.
+ *
+ * @param scmNodeDetails - SCM Node Details.
+ * @param certSignReq - Certificate signing request.
+ * @return String - pem encoded SCM signed
+ * certificate.
+ */
+ public String generateSCMPeerCertificate(ScmNodeDetailsProto scmNodeDetails,
+ String certSignReq) throws IOException {
+ SCMGeneratePeerSCMCertRequestProto request =
+ SCMGeneratePeerSCMCertRequestProto.newBuilder()
+ .setCSR(certSignReq)
+ .setScmDetails(scmNodeDetails)
+ .build();
+ return submitRequest(Type.GeneratePeerSCMCertificate,
+ builder -> builder.setGeneratePeerSCMCertificateRequest(request))
+ .getGetCertResponseProto().getX509CACertificate();
Review comment:
I think this should be:
```suggestion
.getGetCertResponseProto().getX509Certificate();
```
to return the generated certificate, not the CA one. Compare to server side:
https://github.com/apache/ozone/blob/82519e70317546617d7a52c9dfa30a42e3df917c/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/protocol/SCMSecurityProtocolServerSideTranslatorPB.java#L175-L176
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/protocol/SCMSecurityProtocolServerSideTranslatorPB.java
##########
@@ -146,6 +154,31 @@ public SCMGetCertResponseProto getDataNodeCertificate(
}
+ /**
+ * Get SCM signed certificate for DataNode.
Review comment:
```suggestion
* Get SCM signed certificate for SCM.
```
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/protocol/SCMSecurityProtocolServerSideTranslatorPB.java
##########
@@ -146,6 +154,31 @@ public SCMGetCertResponseProto getDataNodeCertificate(
}
+ /**
+ * Get SCM signed certificate for DataNode.
+ *
+ * @param request
+ * @return SCMGetDataNodeCertResponseProto.
Review comment:
Leftover reference to `DataNode`.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/protocol/SCMSecurityProtocolServerSideTranslatorPB.java
##########
@@ -146,6 +154,31 @@ public SCMGetCertResponseProto getDataNodeCertificate(
}
+ /**
+ * Get SCM signed certificate for DataNode.
+ *
+ * @param request
Review comment:
Description is missing.
##########
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?
@xiaoyuyao I assume this applies to all related code as well, right? Example:
https://github.com/apache/ozone/blob/82519e70317546617d7a52c9dfa30a42e3df917c/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/protocol/SCMSecurityProtocol.java#L75-L76
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMSecurityProtocolServer.java
##########
@@ -156,6 +159,44 @@ public String getOMCertificate(OzoneManagerDetailsProto omDetails,
}
}
+
+ /**
+ * Get SCM signed certificate for SCM peer Node.
+ *
+ * @param scmNodeDetails - SCM Node Details.
+ * @param certSignReq - Certificate signing request.
+ * @return String - SCM signed pem encoded certificate.
+ */
+ @Override
+ public String generateSCMPeerCertificate(ScmNodeDetailsProto scmNodeDetails,
+ String certSignReq) throws IOException {
+ Objects.requireNonNull(scmNodeDetails);
+ LOGGER.info("Processing CSR for scm {}, nodeId: {}",
+ scmNodeDetails.getHostName(), scmNodeDetails.getScmNodeId());
+
+ // Check clusterID
+ if (storageContainerManager.getClusterId().equals(
+ scmNodeDetails.getClusterId())) {
+ throw new IOException("SCM ClusterId mismatch. Peer SCM ClusterId, " +
+ scmNodeDetails.getClusterId() + "leader SCM ClusterId "
+ + storageContainerManager.getClusterId());
+ }
+
+
+ Future<X509CertificateHolder> future =
+ certificateServer.requestCertificate(certSignReq,
+ KERBEROS_TRUSTED, true);
+
+ try {
+ return CertificateCodec.getPEMEncodedString(future.get());
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new IOException("generateSCMPeerCertificate operation failed. ", e);
+ } catch (ExecutionException e) {
+ throw new IOException("generateSCMPeerCertificate operation failed. ", e);
+ }
Review comment:
Can you please extract this repeated block to a method? Now appears three times with slight differences.
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/authority/DefaultApprover.java
##########
@@ -88,7 +88,8 @@ public X509CertificateHolder sign(
Date validTill,
PKCS10CertificationRequest certificationRequest,
String scmId,
- String clusterId) throws IOException, OperatorCreationException {
+ String clusterId) throws IOException,
+ OperatorCreationException {
Review comment:
Nit: can we omit this formatting-only change?
----------------------------------------------------------------
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