fapifta commented on code in PR #4808:
URL: https://github.com/apache/ozone/pull/4808#discussion_r1212677121
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/CertificateClient.java:
##########
@@ -110,13 +111,15 @@ X509Certificate getCertificate(String certSerialId)
* Return all certificates in this component's trust chain,
* the last one is the root CA certificate.
*/
- List<X509Certificate> getTrustChain();
+ List<X509Certificate> getTrustChain() throws CertificateException;
/**
* Return the latest Root CA certificate known to the client.
* @return latest Root CA certificate known to the client.
*/
- X509Certificate getRootCACertificate();
+ X509Certificate getLatestRootCACertificate();
Review Comment:
Please add API doc for the new methods.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -865,6 +906,20 @@ public synchronized X509Certificate getRootCACertificate()
{
return null;
}
+ @Override
+ public Set<X509Certificate> getAllRootCaCerts() {
+ return rootCaCertificates.stream().
+ map(this::firstCertificateFrom)
Review Comment:
Is there a use case where we use any secondary certificate from CA
certificate files? Also is there a case when we have a CA certificate file with
more than one certificate? (I believe this we might have but just after SCM
decommission, but correct me if I am wrong.)
If we do not have, we can store the first certificate only in memory, and
then we do not need this mapping all the time, and we can return an
unmodifiableSet based on the stored certs simply. What do you think?
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -340,20 +349,52 @@ public synchronized List<X509Certificate> getTrustChain()
{
}
} else {
// case before certificate bundle is supported
- chain.add(getCertificate());
- X509Certificate cert = getCACertificate();
- if (cert != null) {
- chain.add(getCACertificate());
- }
- cert = getRootCACertificate();
- if (cert != null) {
- chain.add(cert);
+ X509Certificate lastInsertedCert = getCertificate();
+ chain.add(lastInsertedCert);
+ List<X509Certificate> caCertList = getCaCertList();
+ while (!isRootCa(lastInsertedCert)) {
+ Optional<X509Certificate> issuerOpt =
+ getIssuerForCert(lastInsertedCert, caCertList);
+ if (issuerOpt.isPresent()) {
+ X509Certificate issuer = issuerOpt.get();
+ chain.add(issuer);
+ lastInsertedCert = issuer;
+ } else {
+ throw new CertificateException("No issuer found for certificate: " +
+ lastInsertedCert);
+ }
}
+ //add self-signed certificate to the chain
+ chain.add(lastInsertedCert);
}
return chain;
}
+ private boolean isRootCa(X509Certificate cert) {
+ return
cert.getSubjectX500Principal().equals(cert.getIssuerX500Principal());
+ }
+
+ private Optional<X509Certificate> getIssuerForCert(X509Certificate cert,
+ Iterable<X509Certificate> issuerCerts) {
+ for (X509Certificate issuer : issuerCerts) {
+ if (cert.getIssuerX500Principal().equals(
+ issuer.getSubjectX500Principal())) {
+ return Optional.of(issuer);
+ }
+ }
+ return Optional.empty();
+ }
+
+ private List<X509Certificate> getCaCertList() throws IOException {
Review Comment:
We have OzoneSecurityUtil.convertToX509 to do the same thing, we might use
that here as well.
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/x509/certificate/client/CertificateClientTestImpl.java:
##########
@@ -95,6 +97,7 @@ public CertificateClientTestImpl(OzoneConfiguration conf,
boolean autoRenew)
throws Exception {
certificateMap = new ConcurrentHashMap<>();
securityConfig = new SecurityConfig(conf);
+ rootCerts = ConcurrentHashMap.newKeySet();
Review Comment:
Why we need a set backed by a concurrent hashmap here? Isn't the simple or
worst case the concurrent HashSet work?
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -340,20 +349,52 @@ public synchronized List<X509Certificate> getTrustChain()
{
}
} else {
// case before certificate bundle is supported
- chain.add(getCertificate());
- X509Certificate cert = getCACertificate();
- if (cert != null) {
- chain.add(getCACertificate());
- }
- cert = getRootCACertificate();
- if (cert != null) {
- chain.add(cert);
+ X509Certificate lastInsertedCert = getCertificate();
+ chain.add(lastInsertedCert);
+ List<X509Certificate> caCertList = getCaCertList();
+ while (!isRootCa(lastInsertedCert)) {
+ Optional<X509Certificate> issuerOpt =
+ getIssuerForCert(lastInsertedCert, caCertList);
+ if (issuerOpt.isPresent()) {
+ X509Certificate issuer = issuerOpt.get();
+ chain.add(issuer);
+ lastInsertedCert = issuer;
+ } else {
+ throw new CertificateException("No issuer found for certificate: " +
+ lastInsertedCert);
+ }
}
+ //add self-signed certificate to the chain
+ chain.add(lastInsertedCert);
}
return chain;
}
+ private boolean isRootCa(X509Certificate cert) {
+ return
cert.getSubjectX500Principal().equals(cert.getIssuerX500Principal());
Review Comment:
Theoretically it is possible to have a self-signed certificate that is not a
CA certificate, but in the system the only self-signed certificate is the
rootCA certificate we created, so I would not think it is a necessary check
until the premise is true.
On the other hand, your comment inspired an other observation regarding this
condition, if we have an externally configured CA certificate we use as the
rootCA certificate, then that is most likely a certificate which is signed by
an other higher level CA entity instead of a self-signed certificate, and we
will need to handle that scenario here as well.
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/security/ssl/TestReloadingX509TrustManager.java:
##########
@@ -51,16 +52,15 @@ public void testReload() throws Exception {
TrustManager tm =
caClient.getServerKeyStoresFactory().getTrustManagers()[0];
X509Certificate cert1 = caClient.getRootCACertificate();
- assertEquals(cert1,
- ((ReloadingX509TrustManager)tm).getAcceptedIssuers()[0]);
+ assertTrue(isCertAcceptedInTrustManager(cert1, tm));
caClient.renewRootCA();
caClient.renewKey();
X509Certificate cert2 = caClient.getRootCACertificate();
assertNotEquals(cert1, cert2);
-
- assertEquals(cert2,
- ((ReloadingX509TrustManager)tm).getAcceptedIssuers()[0]);
+
+ assertTrue(isCertAcceptedInTrustManager(cert1, tm));
Review Comment:
You can use MatcherAsserts here in the form of:
org.hamcrest.MatcherAssert.assertThat(tm.getAcceptedIssuers(),
org.hamcrest.Matchers.arrayContaining(cert1)
It is expressive, and you do not have to write your own assertion logic for
array contains.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
