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 2023/01/05 15:04:06 UTC
[GitHub] [ozone] ChenSammi opened a new pull request, #4145: HDDS-7590. Use keyManager and trustManager provided by keyStoreFactory in om grpc services
ChenSammi opened a new pull request, #4145:
URL: https://github.com/apache/ozone/pull/4145
https://issues.apache.org/jira/browse/HDDS-7590
--
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: issues-unsubscribe@ozone.apache.org
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] ChenSammi commented on a diff in pull request #4145: HDDS-7590. Use keyManager and trustManager provided by keyStoreFactory in om grpc services
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4145:
URL: https://github.com/apache/ozone/pull/4145#discussion_r1116528836
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/authority/DefaultCAServer.java:
##########
@@ -224,16 +227,20 @@ private KeyPair getCAKeys() throws IOException {
public Future<CertPath> requestCertificate(
PKCS10CertificationRequest csr,
CertificateApprover.ApprovalType approverType, NodeType role) {
- LocalDate beginDate = LocalDate.now().atStartOfDay().toLocalDate();
- LocalDateTime temp = LocalDateTime.of(beginDate, LocalTime.MIDNIGHT);
+ LocalDateTime beginDate;
+ if (!testSecureFlag) {
Review Comment:
@fapifta . Yeah, it seems there is no industry standard or common practice for the certificate start and end time rounding, then let's keep the same logic using LocalDateTime.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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
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] ChenSammi commented on a diff in pull request #4145: HDDS-7590. Use keyManager and trustManager provided by keyStoreFactory in om grpc services
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4145:
URL: https://github.com/apache/ozone/pull/4145#discussion_r1116525240
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMSecurityProtocolServer.java:
##########
@@ -421,9 +421,9 @@ public void stop() {
}
public void join() throws InterruptedException {
- LOGGER.trace("Join RPC server for SCMSecurityProtocolServer.");
+ LOGGER.info("Join RPC server for SCMSecurityProtocolServer.");
Review Comment:
This join() is called only once, right after stop(). The log level in stop is info. But it's trace in join. I think we should use the same level as stop.
--
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: issues-unsubscribe@ozone.apache.org
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] Galsza commented on a diff in pull request #4145: HDDS-7590. Use keyManager and trustManager provided by keyStoreFactory in om grpc services
Posted by "Galsza (via GitHub)" <gi...@apache.org>.
Galsza commented on code in PR #4145:
URL: https://github.com/apache/ozone/pull/4145#discussion_r1114411300
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -111,8 +112,10 @@ public abstract class DefaultCertificateClient implements CertificateClient {
private static final Random RANDOM = new SecureRandom();
- private static final String CERT_FILE_NAME_FORMAT = "%s.crt";
+ public static final String CERT_FILE_NAME_FORMAT = "%s.crt";
+ public static final String CA_CERT_PREFIX = "CA-";
private static final int CA_CERT_PREFIX_LEN = 3;
+ public static final String ROOT_CA_CERT_PREFIX = "ROOTCA-";
Review Comment:
On the master branch there is a CAType enum now that contains these values. I suggest using that enum, and redefining `CA_CERT_PREFIX_LEN` as `CAType.SUBORDINATE.getFileNamePrefix().length()` and `ROOT_CA_PREFIX_LEN` as `CAType.ROOT.getFileNamePrefix().length()` .
--
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: issues-unsubscribe@ozone.apache.org
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] kerneltime commented on pull request #4145: HDDS-7590. Use keyManager and trustManager provided by keyStoreFactory in om grpc services
Posted by GitBox <gi...@apache.org>.
kerneltime commented on PR #4145:
URL: https://github.com/apache/ozone/pull/4145#issuecomment-1375967658
@neils-dev
--
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: issues-unsubscribe@ozone.apache.org
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] fapifta merged pull request #4145: HDDS-7590. Use keyManager and trustManager provided by keyStoreFactory in om grpc services
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta merged PR #4145:
URL: https://github.com/apache/ozone/pull/4145
--
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: issues-unsubscribe@ozone.apache.org
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] ChenSammi commented on a diff in pull request #4145: HDDS-7590. Use keyManager and trustManager provided by keyStoreFactory in om grpc services
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4145:
URL: https://github.com/apache/ozone/pull/4145#discussion_r1115375040
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -111,8 +112,10 @@ public abstract class DefaultCertificateClient implements CertificateClient {
private static final Random RANDOM = new SecureRandom();
- private static final String CERT_FILE_NAME_FORMAT = "%s.crt";
+ public static final String CERT_FILE_NAME_FORMAT = "%s.crt";
+ public static final String CA_CERT_PREFIX = "CA-";
private static final int CA_CERT_PREFIX_LEN = 3;
+ public static final String ROOT_CA_CERT_PREFIX = "ROOTCA-";
Review Comment:
@Galsza , thanks for the review. testUgi is necessary to make sure OM can connect SCM regarding kerberos login and kerberos service authentication. testSecureFlag for DefaultCAServer is to support a more fine grained certificate lifetime. CAType is a good suggestion. I will refactor this part of code.
--
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: issues-unsubscribe@ozone.apache.org
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] fapifta commented on a diff in pull request #4145: HDDS-7590. Use keyManager and trustManager provided by keyStoreFactory in om grpc services
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta commented on code in PR #4145:
URL: https://github.com/apache/ozone/pull/4145#discussion_r1116310251
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMSecurityProtocolServer.java:
##########
@@ -421,9 +421,9 @@ public void stop() {
}
public void join() throws InterruptedException {
- LOGGER.trace("Join RPC server for SCMSecurityProtocolServer.");
+ LOGGER.info("Join RPC server for SCMSecurityProtocolServer.");
Review Comment:
I am a bit unsure about these two... do we really want/need to increase the log level for these messages?
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/authority/DefaultCAServer.java:
##########
@@ -224,16 +227,20 @@ private KeyPair getCAKeys() throws IOException {
public Future<CertPath> requestCertificate(
PKCS10CertificationRequest csr,
CertificateApprover.ApprovalType approverType, NodeType role) {
- LocalDate beginDate = LocalDate.now().atStartOfDay().toLocalDate();
- LocalDateTime temp = LocalDateTime.of(beginDate, LocalTime.MIDNIGHT);
+ LocalDateTime beginDate;
+ if (!testSecureFlag) {
Review Comment:
We had a discussion about the testSecureFlag today, let me summarize what I think in a few points:
- it would be beneficial to have the same code for production and tests
- it does not really matter from either production or test perspective if we date back the start of the validity period to the start of the day, it is only important for both that the certificate is valid once it is used
- there does not seem to be an industry standard that says a certificate validity period should start at midnight, in fact thinking about it, it is possibly just because of tooling, google for example has random timestamps for start of validity
Based on this I would suggest to use simply LocalDateTime.now() as the beginDate for all cases, and then calculate the endDate just by adding the duration.
--
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: issues-unsubscribe@ozone.apache.org
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] ChenSammi commented on a diff in pull request #4145: HDDS-7590. Use keyManager and trustManager provided by keyStoreFactory in om grpc services
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4145:
URL: https://github.com/apache/ozone/pull/4145#discussion_r1116525240
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMSecurityProtocolServer.java:
##########
@@ -421,9 +421,9 @@ public void stop() {
}
public void join() throws InterruptedException {
- LOGGER.trace("Join RPC server for SCMSecurityProtocolServer.");
+ LOGGER.info("Join RPC server for SCMSecurityProtocolServer.");
Review Comment:
This join() is called only once, right after stop(). The log level in stop is info. Why it's trace in join. I think we should use the same level as stop.
--
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: issues-unsubscribe@ozone.apache.org
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