[jira] [Comment Edited] (HDDS-102) SCM CA: SCM CA server signs certificate for approved CSR
[
https://issues.apache.org/jira/browse/HDDS-102?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16725271#comment-16725271
]
Xiaoyu Yao edited comment on HDDS-102 at 12/19/18 6:55 PM:
---
Thanks [~anu] for the update. Patch v4 LGTM. I just have few minor comments, +1
after that being fixed.
CertificateSignRequest.java
Line 105: StringWriter#close() is no-op, so we could using the following
instead.
{code:java}
StringWriter str=new StringWriter();
try (JcaPEMWriterpemWriter= new JcaPEMWriter(str)){
pemWriter.writeObject(pemObject);
}
return str.toString();
{code}
MockApprover.java
Line 23/33: unused imports
TestDefaultProfile.java
Line 56-57/61: unused imports
was (Author: xyao):
Thanks [~anu] for the update. Patch v4 LGTM. I just have few minor comments, +1
after that being fixed.
CertificateSignRequest.java
Line 105: StringWriter#close() is no-op, so we could using the following
instead.
{code}
StringWriter str=*new* StringWriter();
*try* (JcaPEMWriterpemWriter=*new*JcaPEMWriter(str)){
pemWriter.writeObject(pemObject);
}
*return* str.toString();
{code}
MockApprover.java
Line 23/33: unused imports
TestDefaultProfile.java
Line 56-57/61: unused imports
> SCM CA: SCM CA server signs certificate for approved CSR
>
>
> Key: HDDS-102
> URL: https://issues.apache.org/jira/browse/HDDS-102
> Project: Hadoop Distributed Data Store
> Issue Type: Sub-task
>Reporter: Xiaoyu Yao
>Assignee: Anu Engineer
>Priority: Major
> Attachments: HDDS-102-HDDS-4.001.patch, HDDS-102-HDDS-4.001.patch,
> HDDS-102-HDDS-4.002.patch, HDDS-102-HDDS-4.003.patch,
> HDDS-102-HDDS-4.004.patch
>
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
-
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
[jira] [Comment Edited] (HDDS-102) SCM CA: SCM CA server signs certificate for approved CSR
[
https://issues.apache.org/jira/browse/HDDS-102?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16724516#comment-16724516
]
Anu Engineer edited comment on HDDS-102 at 12/18/18 10:25 PM:
--
The latest patch addresses all review comments. Please see the below for
details.
Thanks for the reviews and comments.[~xyao] [~ajayydv]
{quote}bq. Line 57-67: I think we could move getGeneralNames() and
isSupportedGeneralName() into the implementation.
{quote}
The reason we have left these functions is that it allows a CA client interface
to query the PKI if a specific extension is supported. It is a future-looking
interface and not needed in the current patch. Please let me know if you want
me to remove these.
{quote}Line 84: NIT: getExtensions() -> getSupportedExtensions()
{quote}
Fixed.
{quote}Line 91: NIT: the name could be simpified into isSupported(Extension
extension), also could be potentially consolidated into
{quote}
As I mentioned earlier, these will be useful when we write the client code to
check if we are doing the right stuff in relation to that Profile.
{quote}Line 105: can validateExtendedKeyUsage covered by validateExtension()?
{quote}
>From a user reading code and reviewing it does make sense to keep
>validateExtendedKeyusage into validating extension. But from an X509
>perspective, these are very different.
{quote}Line 111: this can be moved within try-with-resource and 113 can be
removed.
{quote}
Fixed.
{quote}Line 107: can we move ApprovalType enum to CertificateApprover interface
as CertificateApprover#type?
{quote}
Fixed.
{quote}Line 93: need to check null for returned array from
attribute.getAttributeValues() to avoid NPE.
{quote}
Fixed.
{quote}Line 102: NIT: the comment is incomplete.
{quote}
Fixed.
{quote}Line 107: NIT: maybe rename it to getExtensionList to be consistent with
getExtensionsList.
{quote}
Since it is just one letter difference, I thought it would be more confusing to
readers and hence my naming choice.
{quote}Line 206: do we need to reread CA public/private key from file for each
CSR? This may slow down the perf of the CA server.
{quote}
It is a conscious design decision. A certificate issue operation is sporadic,
and reading that information from a disk file is not a very long operation. If
and when people take memory dumps, if we have reference to the private key, it
is guaranteed to be in the memory dump of the JVM. With this approach, the
probability of the key being part of every memory dump is reduced. So we read
the key, use it and release it. There is still a possibility that key can be
part of the memory dump, but this is just a way to mitigate it. Performance
should not be an issue here since this code is executed only when a new
datanode or Ozone Manager is added to Ozone.
{quote}Line 207/208: can we use DateTime#toDate() instead of
java.sql.Date.valueOf?
{quote}
Here are the options to fix this,
1. Write this conversion our selves.
2. Use an external library
3. Call into Standard Lib, but the function that does this job is under SQL
namespace. Live with it.
Let me know what you prefer and I will make changes accordingly.
{quote}HDDS_X509_DEFAULT_DURATION is confusing with
HDDS_X509_CERT_DURATION_DEFAULT.
{quote}
Fixed.
{quote}L213/258 Specify the security provider as well? (i.e BC)
{quote}
As far as I know, BC does not have a version that can be swapped out to do this.
{quote}L238 readPublicKey: Shall we read public key first time form file and
than cache it for further purposes?
{quote}
See my earlier comment; this code is executed so rarely, that caching these
values make no sense. SCM CA should not be in the business of issuing
certificates with high throughput.
{quote}Method sign Shall we add documentation to ensure users call
approver#validate before it.
{quote}
Fixed.
{quote}L76: Possible typo "The last, and method which never"
{quote}
Fixed.
{quote}L78 "CSR is the base" perhaps "is" should be replaced with "if"?
{quote}
fixed.
{quote}L168 Shall we validate the received certificate? (signature etc)
{quote}
I am not sure how to validate a certificate. Are you proposing that we mimic an
application that uses the certificate. There are lots of fields to verify here
if we decided to do a deep validation. I am going to skip that for now.
{quote}Add a TODO for unimplemented test cases?
{quote}
Fixed.
was (Author: anu):
bq. Line 57-67: I think we could move getGeneralNames() and
isSupportedGeneralName() into the implementation.
The reason we have left these functions is that it allows a CA client interface
to query the PKI if a specific extension is supported. It is a future-looking
interface and not needed in the current patch. Please let me know if you want
me to remove these.
bq. Line 84: NIT: getExtensions() -> getSupportedExtensions()
Fixed.
bq. Line 91: NIT: the name could be simpified into isSupported(Exte
[jira] [Comment Edited] (HDDS-102) SCM CA: SCM CA server signs certificate for approved CSR
[ https://issues.apache.org/jira/browse/HDDS-102?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16723463#comment-16723463 ] Ajay Kumar edited comment on HDDS-102 at 12/17/18 11:25 PM: [~anu] thanks for the important patch. LGTM. Some additional comments from what [~xyao] has already mentioned: KeyCodec L213/258 Specify the security provider as well? (i.e BC) L238 readPublicKey: Shall we read public key first time form file and than cache it for further purposes? DefaultApprover Method sign Shall we add documentation to ensure users call approver#validate before it. DefaultCAServer L139: Typo "configureable" L62 should be package-info L76: Possible typo "The last, and method which never" L78 "CSR is the base" perhaps "is" should be replaced with "if"? TestDefaultCAServer Unused imports L168 Shall we validate the received certificate? (signature etc) TestDefaultProfile Add a TODO for unimplemented test cases? was (Author: ajayydv): [~anu] thanks for the important patch. LGTM. Some additional comments from what [~xyao] has already mentioned: KeyCodec L213/258 Specify the security provider as well? (i.e BC) L238 readPublicKey: Shall we read public key first time form file and than cache it for further purposes? DefaultApprover Method sign Shall we add documentation to ensure users call approver#validate before it or we can refactor it to call validate internally to ensure we always sign a valida csr. DefaultCAServer L139: Typo "configureable" L62 should be package-info L76: Possible typo "The last, and method which never" L78 "CSR is the base" perhaps "is" should be replaced with "if"? TestDefaultCAServer Unused imports L168 Shall we validate the received certificate? (signature etc) TestDefaultProfile Add a TODO for unimplemented test cases? > SCM CA: SCM CA server signs certificate for approved CSR > > > Key: HDDS-102 > URL: https://issues.apache.org/jira/browse/HDDS-102 > Project: Hadoop Distributed Data Store > Issue Type: Sub-task >Reporter: Xiaoyu Yao >Assignee: Anu Engineer >Priority: Major > Attachments: HDDS-102-HDDS-4.001.patch, HDDS-102-HDDS-4.001.patch, > HDDS-102-HDDS-4.002.patch > > -- This message was sent by Atlassian JIRA (v7.6.3#76005) - To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
[jira] [Comment Edited] (HDDS-102) SCM CA: SCM CA server signs certificate for approved CSR
[ https://issues.apache.org/jira/browse/HDDS-102?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16723463#comment-16723463 ] Ajay Kumar edited comment on HDDS-102 at 12/17/18 11:26 PM: [~anu] thanks for the important patch. LGTM. Some additional comments from what [~xyao] has already mentioned: KeyCodec L213/258 Specify the security provider as well? (i.e BC) L238 readPublicKey: Shall we read public key first time form file and than cache it for further purposes? DefaultApprover Method sign Shall we add documentation to ensure users call approver#validate before it. DefaultCAServer L139: Typo "configureable" package-info L76: Possible typo "The last, and method which never" L78 "CSR is the base" perhaps "is" should be replaced with "if"? TestDefaultCAServer Unused imports L168 Shall we validate the received certificate? (signature etc) TestDefaultProfile Add a TODO for unimplemented test cases? was (Author: ajayydv): [~anu] thanks for the important patch. LGTM. Some additional comments from what [~xyao] has already mentioned: KeyCodec L213/258 Specify the security provider as well? (i.e BC) L238 readPublicKey: Shall we read public key first time form file and than cache it for further purposes? DefaultApprover Method sign Shall we add documentation to ensure users call approver#validate before it. DefaultCAServer L139: Typo "configureable" L62 should be package-info L76: Possible typo "The last, and method which never" L78 "CSR is the base" perhaps "is" should be replaced with "if"? TestDefaultCAServer Unused imports L168 Shall we validate the received certificate? (signature etc) TestDefaultProfile Add a TODO for unimplemented test cases? > SCM CA: SCM CA server signs certificate for approved CSR > > > Key: HDDS-102 > URL: https://issues.apache.org/jira/browse/HDDS-102 > Project: Hadoop Distributed Data Store > Issue Type: Sub-task >Reporter: Xiaoyu Yao >Assignee: Anu Engineer >Priority: Major > Attachments: HDDS-102-HDDS-4.001.patch, HDDS-102-HDDS-4.001.patch, > HDDS-102-HDDS-4.002.patch > > -- This message was sent by Atlassian JIRA (v7.6.3#76005) - To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
