-
Notifications
You must be signed in to change notification settings - Fork 594
HDDS-4897. [SCM HA Security] Create SCM Cert Client and change DefaultCA to allow selfsigned and intermediary #2041
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
75550e6 to
3d3d1d0
Compare
3d3d1d0 to
86019cf
Compare
xiaoyuyao
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bharatviswa504 for working on this and the offline discussions. The PR LGTM overall, a few comments added inline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to overlap with most of the existing DefaultCAServer#generateSelfSignedCA, can we dedup the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it is completely different. As DefaultCAServer uses its own SelfSignedCertificate builder and persist, whereas this new method uses generate CSR and persist.
Where as taken care of dedup between getRootCASignedSCMCert and this method.
...rc/main/java/org/apache/hadoop/hdds/security/x509/certificate/authority/DefaultCAServer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some comments to highlight this is only used for Sub-CA?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep the root ca subject scm@host to be backward compatible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sub scm can have scm-sub@host
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this sub scm cert should have subject like scm-sub@host.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we start with the DefaultCAProfile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this PR code is integrated, when 2 CA Servers are started will change this. It will come in next in PR. As to make current code work, used DefaultProfile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we combine the exception handling if the logic are the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we check the result from RPC response before attempting further operations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already ClientSideTranslator has performed this. So, check if it has a certificate should be enough.
private SCMSecurityResponse handleError(SCMSecurityResponse resp)
throws SCMSecurityException {
if (resp.getStatus() != SCMSecurityProtocolProtos.Status.OK) {
throw new SCMSecurityException(resp.getMessage(),
SCMSecurityException.ErrorCode.values()[resp.getStatus().ordinal()]);
}
return resp;
}
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/HASecurityUtils.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate the logic behind this? Are we assume only the root CA start with the DefaultCAProfile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea here is DefaultCAProfile when used by CA Server it will issue only CA Certificate.
And RootCA Server right now starts with DefaultCAProfile on fresh installed clusters, where as on upgraded cluster from non-HA rootCAServer will be started like before with DefaultProfile. The integration of DefaultCAServer and starting 2 CA will be taken care in further Jiras.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bq. My idea here is DefaultCAProfile when used by CA Server it will issue only CA Certificate.
Should we rename to RootCAProfile in this case?
Also, when Primary starts two DefaultCA server in the follow up JIRA, one with RootCAProfile and one without?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, when Primary starts two DefaultCA server in the follow up JIRA, one with RootCAProfile and one without?
Yes.
|
Thank You @xiaoyuyao for the review. I have addressed/replied to the review comments. |
…tCA to allow selfsigned and intermediary.
32b7bc0 to
f361292
Compare
xiaoyuyao
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bharatviswa504 for the update. Just few questions inline. Otherwise, LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bq. My idea here is DefaultCAProfile when used by CA Server it will issue only CA Certificate.
Should we rename to RootCAProfile in this case?
Also, when Primary starts two DefaultCA server in the follow up JIRA, one with RootCAProfile and one without?
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/HASecurityUtils.java
Outdated
Show resolved
Hide resolved
|
Thank You @xiaoyuyao for the review |
What changes were proposed in this pull request?
This PR implements
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4897
How was this patch tested?
Added tests