[
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(Extension
extension), also could be potentially consolidated into
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.
bq. Line 105: can validateExtendedKeyUsage covered by validateExtension()?
>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.
bq. Line 111: this can be moved within try-with-resource and 113 can be removed.
Fixed.
bq. Line 107: can we move ApprovalType enum to CertificateApprover interface as
CertificateApprover#type?
Fixed.
bq. Line 93: need to check null for returned array from
attribute.getAttributeValues() to avoid NPE.
Fixed.
bq. Line 102: NIT: the comment is incomplete.
Fixed.
bq.Line 107: NIT: maybe rename it to getExtensionList to be consistent with
getExtensionsList.
Since it is just one letter difference, I thought it would be more confusing to
readers and hence my naming choice.
bq. 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.
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.
bq. Line 207/208: can we use DateTime#toDate() instead of java.sql.Date.valueOf?
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.
bq. HDDS_X509_DEFAULT_DURATION is confusing with
HDDS_X509_CERT_DURATION_DEFAULT.
Fixed.
bq. L213/258 Specify the security provider as well? (i.e BC)
As far as I know, BC does not have a version that can be swapped out to do this.
bq. L238 readPublicKey: Shall we read public key first time form file and than
cache it for further purposes?
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.
bq. Method sign Shall we add documentation to ensure users call
approver#validate before it.
Fixed.
bq. L76: Possible typo "The last, and method which never"
Fixed.
bq. L78 "CSR is the base" perhaps "is" should be replaced with "if"?
fixed.
bq. L168 Shall we validate the received certificate? (signature etc)
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.
bq. Add a TODO for unimplemented test cases?
Fixed.
> 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
>
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
