You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "ChenSammi (via GitHub)" <gi...@apache.org> on 2023/03/21 13:09:58 UTC
[GitHub] [ozone] ChenSammi opened a new pull request, #4442: HDDS-8178. CertificateClient and KeyStoresFactory support multiple Sub-CA certificates in the trust chain
ChenSammi opened a new pull request, #4442:
URL: https://github.com/apache/ozone/pull/4442
https://issues.apache.org/jira/browse/HDDS-8178
--
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 #4442: HDDS-8178. CertificateClient and KeyStoresFactory support multiple Sub-CA certificates in the trust chain
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta commented on code in PR #4442:
URL: https://github.com/apache/ozone/pull/4442#discussion_r1147644768
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -331,6 +332,35 @@ public synchronized X509Certificate getCACertificate() {
return firstCertificateFrom(caCertPath);
}
+ /**
+ * Return all certificates in this component's trust chain,
+ * the last one is the root CA certificate.
+ */
+ public synchronized List<X509Certificate> getTrustChain() {
+ CertPath path = getCertPath();
+ if (path == null || path.getCertificates() == null) {
+ return null;
+ }
+
+ List<X509Certificate> chain = new ArrayList<>();
+ // certificate bundle case
+ if (path.getCertificates().size() > 1) {
+ for (int i = 0; i < path.getCertificates().size(); i++) {
+ chain.add((X509Certificate) path.getCertificates().get(i));
+ }
+ } else {
+ // case before certificate bundle is supported
+ chain.add(getCertificate());
+ chain.add(getCACertificate());
Review Comment:
One thing I am not sure here...
Is it possible that we do not have a subordinate CA cert stored due to upgrading from a very old version or on a cluster without SCM HA? If so we might not want to add a null value to this list... however if the null value is possible, but does not cause any trouble, then let's just keep this piece simple.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/ssl/ReloadingX509KeyManager.java:
##########
@@ -176,7 +181,10 @@ private X509ExtendedKeyManager loadKeyManager(CertificateClient caClient)
}
currentPrivateKey = privateKey;
- currentCertId = cert.getSerialNumber().toString();
+ currentCertList.clear();
+ for (X509Certificate caCert: newCertList) {
Review Comment:
the name caCert is a little bit misleading here, as the newCertList should contain the service certificate the subordinate CA certs, and the root CA cert, can we just name it c or cert within the for loop?
--
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 #4442: HDDS-8178. CertificateClient and KeyStoresFactory support multiple Sub-CA certificates in the trust chain
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta merged PR #4442:
URL: https://github.com/apache/ozone/pull/4442
--
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] neils-dev commented on a diff in pull request #4442: HDDS-8178. CertificateClient and KeyStoresFactory support multiple Sub-CA certificates in the trust chain
Posted by "neils-dev (via GitHub)" <gi...@apache.org>.
neils-dev commented on code in PR #4442:
URL: https://github.com/apache/ozone/pull/4442#discussion_r1150003228
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -331,6 +332,38 @@ public synchronized X509Certificate getCACertificate() {
return firstCertificateFrom(caCertPath);
}
+ /**
+ * Return all certificates in this component's trust chain,
+ * the last one is the root CA certificate.
+ */
+ public synchronized List<X509Certificate> getTrustChain() {
Review Comment:
add Override annotation for implementation of interface
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/ssl/ReloadingX509KeyManager.java:
##########
@@ -64,7 +65,7 @@ public class ReloadingX509KeyManager extends X509ExtendedKeyManager {
* materials are changed.
*/
private PrivateKey currentPrivateKey;
- private String currentCertId;
+ private List<String> currentCertList = new ArrayList<>();
Review Comment:
Thanks @ChenSammi for the patch with changes to the keystore and truststore.
_Minor_: may read better if `currentCertList` _renamed_ to `currentCertIdsList` or similar to differentiate and emphasize list of serialIds from list of certificate objects.
--
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 pull request #4442: HDDS-8178. CertificateClient and KeyStoresFactory support multiple Sub-CA certificates in the trust chain
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #4442:
URL: https://github.com/apache/ozone/pull/4442#issuecomment-1482794893
The reverse way works too. The latest patch puts all certificates in the trust chain in KeyManager and root ca certificates in TrustManager. @fapifta
--
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 pull request #4442: HDDS-8178. CertificateClient and KeyStoresFactory support multiple Sub-CA certificates in the trust chain
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #4442:
URL: https://github.com/apache/ozone/pull/4442#issuecomment-1482435048
@fapifta , thanks for the review and suggestion. Will try it out.
--
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 pull request #4442: HDDS-8178. CertificateClient and KeyStoresFactory support multiple Sub-CA certificates in the trust chain
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta commented on PR #4442:
URL: https://github.com/apache/ozone/pull/4442#issuecomment-1482105701
Hi @ChenSammi,
thank you for working on this piece, and for the discussion we had on this.
So my understanding after our discussion is that there is a problem with the TLS authentication, and that is because the validity of the certificate provided by an other party can not be verified properly.
As we discussed the original intent with introducing certificate bundles was to simplify the trust setup, the idea is that we have just only the rootCA certificate in the TrustStores, with that we provide a certificate bundle for every service that contains the certificate chain up until the rootCA.
With that the KeyStore will be initialized with the chain of certificates up to the rootCA, and during connection setup, we provide this chain to the other party, so it will be able to verify the certificate validity up to our trust anchor the rootCA which it has in its TrustStore.
Your patch brings back the old way, where the TrustStore contains all the sub-CA certificates and the rootCA certificate, which in turn solves the problem but it conflicts with the intentions of the change to use a certificate bundle.
As we discussed the problem is that the ReloadingX509KeyManager#loadKeyManager method just gets the actual service's certificate and not the certificate bundle as it is, when it loads its own certificate and key material to the KeyStore to be used with the connection. Hence the chain is not send properly during setting up the connection, and that is why the other party can not verify the certificate against only the rootCA certificate in the TrustStore.
So the solution for the problem should be to load the whole bundle file in the KeyStore, and I believe that should solve the problem you face and would like to solve here.
Can you please check out this approach and let me know if my understanding and the proposed solution is working, and then update this PR or create a new one?
--
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 #4442: HDDS-8178. CertificateClient and KeyStoresFactory support multiple Sub-CA certificates in the trust chain
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #4442:
URL: https://github.com/apache/ozone/pull/4442#discussion_r1147760065
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -331,6 +332,35 @@ public synchronized X509Certificate getCACertificate() {
return firstCertificateFrom(caCertPath);
}
+ /**
+ * Return all certificates in this component's trust chain,
+ * the last one is the root CA certificate.
+ */
+ public synchronized List<X509Certificate> getTrustChain() {
+ CertPath path = getCertPath();
+ if (path == null || path.getCertificates() == null) {
+ return null;
+ }
+
+ List<X509Certificate> chain = new ArrayList<>();
+ // certificate bundle case
+ if (path.getCertificates().size() > 1) {
+ for (int i = 0; i < path.getCertificates().size(); i++) {
+ chain.add((X509Certificate) path.getCertificates().get(i));
+ }
+ } else {
+ // case before certificate bundle is supported
+ chain.add(getCertificate());
+ chain.add(getCACertificate());
Review Comment:
Although I think there is always CA certificate in the old version Ozone, but it's safer to check the CA certificate before add it to the list.
--
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