[
https://issues.apache.org/jira/browse/HDDS-1012?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16756409#comment-16756409
]
Ajay Kumar commented on HDDS-1012:
----------------------------------
[~anu] thanks for review.
{quote}1. BlockToken.java:92: This code should not throw if this needs to work.
Not part of this patch.
2. BlockToken.java: Line 99, if we allow this function to throw IOExecption,
then we can remove all kinds of try catch completely. In fact, this function
will have no code modification AFAIK.{quote}
Reverted all changes.
{quote}3. DefaultCertificateClient.javaE#init – Think that bit based coding is
very fragile and hard to understand. If you want to create a mapping like this,
please add an enum and then handle that in the switch case. It is hard to read,
case 0... especially when the good and bad cases are mixed all over in the bit
map. Please consider rewriting, so suggestions, break them into functions{quote}
Done. enums are more readable but refrained from it in last patch as they might
not convey the complete picture. With int based cases one can directly see the
corresponding case in truth table.
{quote}4. init: I don't think code path is possible at all.
{quote}
I think you mean the null check and corresponding throw , corrected in latest
patch.
{quote}5. It is very hard to understand how this code will be used if there is
no use of this code in the patch. Can we have a patch, which focuses on a use
case, so that we can understand the security issues in the context? I am not
able to see/understand how the init function will be used?{quote}
Not sure if i understand this suggestion completely. Like [HDDS-955], this
patch adds default implementation. Init function will be called by SCM
certificate clients(datanodes and ozonemanager) to initialize and bootstrap.
{quote}6. KeyCodec.java: My Editor complains that it is duplicate code, Can you
please take a look?{quote}
Two new api are added to store private and public key separately, existing one
stores keypair's.
{quote}7. OzoneConfigKeys.java: I think it is a security hole to create default
passwords in code. Most users will not know about this, and all you need one
bad guy to misuse this. We should just store normal .pem files in directories
protected by the file system. This hardcoded default password adds no extra
level security over the file system permissions. Once we move to that model,
the changes in SecurityConfig are superfluous too.{quote}
Agee, Had a todo to store it in KMS or pem file as you suggested. But since
current patch doesn't implement KeyStore/CertStore related functionality i have
removed it from current patch.
> Add Default CertificateClient implementation
> --------------------------------------------
>
> Key: HDDS-1012
> URL: https://issues.apache.org/jira/browse/HDDS-1012
> Project: Hadoop Distributed Data Store
> Issue Type: Sub-task
> Reporter: Ajay Kumar
> Assignee: Ajay Kumar
> Priority: Major
> Attachments: HDDS-1012.01.patch, HDDS-1012.02.patch,
> HDDS-1012.03.patch
>
>
> Add Default CertificateClient implementation
--
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
