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/24 08:20:24 UTC
[GitHub] [ozone] bharatviswa504 opened a new pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
bharatviswa504 opened a new pull request #1958:
URL: https://github.com/apache/ozone/pull/1958
## What changes were proposed in this pull request?
This Jira is to implement generatePeerSCMCertificate which will be called during bootStrap and also during init by primary SCM. This is similar to how OM and DN gets certificate.
During implementation as the validCerTable key is SerialId which is BigInteger, created a new table validScmCertsTable to store scm certs. This is being done so that we can implement querySCMCertificates which can return all SCM certs.
Also, I see there is a TODO for list Certs based on role. Not sure if there is a way in the current code to return certs from the table. If there is any approach, will rework this PR based on review comments.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4861
## How was this patch tested?
----------------------------------------------------------------
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
[GitHub] [ozone] bharatviswa504 commented on pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #1958:
URL: https://github.com/apache/ozone/pull/1958#issuecomment-786477528
@xiaoyuyao I have addressed review comments.
----------------------------------------------------------------
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
[GitHub] [ozone] adoroszlai commented on a change in pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [ozone] bshashikant commented on a change in pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
Posted by GitBox <gi...@apache.org>.
bshashikant commented on a change in pull request #1958:
URL: https://github.com/apache/ozone/pull/1958#discussion_r583383502
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/authority/DefaultCAServer.java
##########
@@ -252,23 +253,34 @@ private KeyPair getCAKeys() throws IOException {
}
private X509CertificateHolder signAndStoreCertificate(LocalDate beginDate,
- LocalDate endDate, PKCS10CertificationRequest csr) throws IOException,
+ LocalDate endDate, PKCS10CertificationRequest csr, NodeType nodeType)
+ 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 (nodeType.equals(NodeType.SCM)) {
+ // If scm cert store in scm cert table and valid cert table.
Review comment:
Can u fix the comments?
----------------------------------------------------------------
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
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1958:
URL: https://github.com/apache/ozone/pull/1958#discussion_r582678942
##########
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:
I don't see why it will be an incompatible change. For now, I followed ur suggestion
----------------------------------------------------------------
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
[GitHub] [ozone] bharatviswa504 merged pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 merged pull request #1958:
URL: https://github.com/apache/ozone/pull/1958
----------------------------------------------------------------
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
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1958:
URL: https://github.com/apache/ozone/pull/1958#discussion_r583399093
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/authority/DefaultCAServer.java
##########
@@ -252,23 +253,34 @@ private KeyPair getCAKeys() throws IOException {
}
private X509CertificateHolder signAndStoreCertificate(LocalDate beginDate,
- LocalDate endDate, PKCS10CertificationRequest csr) throws IOException,
+ LocalDate endDate, PKCS10CertificationRequest csr, NodeType nodeType)
+ 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 (nodeType.equals(NodeType.SCM)) {
+ // If scm cert store in scm cert table and valid cert table.
Review comment:
Done
----------------------------------------------------------------
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
[GitHub] [ozone] adoroszlai commented on a change in pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #1958:
URL: https://github.com/apache/ozone/pull/1958#discussion_r582665288
##########
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:
Can we name it `get...` now, and rename with the rest? I know it's a bit more work, but there are some benefits:
1. They are consistent with one another both before and after the rename.
2. If renaming should turn out to be not possible (eg. because it might introduce incompatibility) or not desired (eg. someone might argue that `get` reflects the operation more closely), then we are not stuck with inconsistent names.
----------------------------------------------------------------
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
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1958:
URL: https://github.com/apache/ozone/pull/1958#discussion_r582644190
##########
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:
Yes for now in this PR will only address for SCM, I can open a new Jira to do for other roles. So, that it will help during list with role type.
----------------------------------------------------------------
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
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1958:
URL: https://github.com/apache/ozone/pull/1958#discussion_r582644646
##########
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:
Done
----------------------------------------------------------------
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
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1958:
URL: https://github.com/apache/ozone/pull/1958#discussion_r582634132
##########
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:
Done.
----------------------------------------------------------------
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
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1958:
URL: https://github.com/apache/ozone/pull/1958#discussion_r582688696
##########
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:
Removed it
----------------------------------------------------------------
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
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1958:
URL: https://github.com/apache/ozone/pull/1958#discussion_r582688545
##########
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:
Done
##########
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:
Done
----------------------------------------------------------------
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
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1958:
URL: https://github.com/apache/ozone/pull/1958#discussion_r583585617
##########
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:
Yes, might be for list for role SCM for certtype RevokedCerts we might need this. Figured out during implementation of listCertificates. As for security work this is not required, will open a new Jira to fix this later.
----------------------------------------------------------------
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
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1958:
URL: https://github.com/apache/ozone/pull/1958#discussion_r582631795
##########
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:
I was debugging some issue yesterday, by mistake, the code came in this commit.
Will remove it
----------------------------------------------------------------
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
[GitHub] [ozone] bharatviswa504 commented on pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #1958:
URL: https://github.com/apache/ozone/pull/1958#issuecomment-786766349
Thank You everyone for the review.
----------------------------------------------------------------
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
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1958:
URL: https://github.com/apache/ozone/pull/1958#discussion_r583585617
##########
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:
Yes, might be for list for role SCM for certtype RevokedCerts we might need this. Figured out during implementation of listCertificates #1969. As for security work listCerts for certype revoke is not required, I will open a new Jira to fix this later.
----------------------------------------------------------------
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
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1958:
URL: https://github.com/apache/ozone/pull/1958#discussion_r582688884
##########
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:
Added now
----------------------------------------------------------------
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
[GitHub] [ozone] xiaoyuyao commented on a change in pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
Posted by 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
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1958:
URL: https://github.com/apache/ozone/pull/1958#discussion_r582655691
##########
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:
Done
----------------------------------------------------------------
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
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1958:
URL: https://github.com/apache/ozone/pull/1958#discussion_r582653903
##########
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:
Right now added scmCert to validCertsTable, so that revoke will remove from both for Scmcerts.
I think once we have a separate table for each role, we can add 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.
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
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1958:
URL: https://github.com/apache/ozone/pull/1958#discussion_r583585617
##########
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:
Yes, might be for list for role SCM for certtype RevokedCerts we might need this. Figured out during implementation of listCertificates #1958 1969. As for security work listCerts for certype revoke is not required, I will open a new Jira to fix this later.
----------------------------------------------------------------
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
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1958:
URL: https://github.com/apache/ozone/pull/1958#discussion_r582633966
##########
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:
Yes, the other APIs should also start with generate, the correct naming is to start with generate. I will use generate for this new API. I can open a new Jira to change for others.
----------------------------------------------------------------
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
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1958: HDDS-4861. [SCM HA Security] Implement generate SCM certificate.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1958:
URL: https://github.com/apache/ozone/pull/1958#discussion_r582646261
##########
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:
SCM Certs are stored in scmCertTable because we want to queryCerts for scm Role. This will come in help during SCM Startup, where each SCM need to get all SCM certs of its quorum.
And also added to fall back table validCertTables because to not to break list API, where it is listing all certs.
----------------------------------------------------------------
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