This is an automated email from the ASF dual-hosted git repository. coheigea pushed a commit to branch coheigea/WSS-721 in repository https://gitbox.apache.org/repos/asf/ws-wss4j.git
commit 00995d220df9aa5b6cc1a04661d2b3a5a052f80a Author: Colm O hEigeartaigh <[email protected]> AuthorDate: Tue Oct 13 07:43:43 2026 +0100 WSS-721 - Search the truststore before the keystore when verifying trust --- .../org/apache/wss4j/common/crypto/Merlin.java | 80 +++--- .../org/apache/wss4j/common/crypto/MerlinAKI.java | 46 ++-- .../wss4j/common/crypto/MerlinTrustTest.java | 280 +++++++++++++++++++++ 3 files changed, 347 insertions(+), 59 deletions(-) diff --git a/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/Merlin.java b/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/Merlin.java index 6483ff4da..0ae67eda8 100644 --- a/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/Merlin.java +++ b/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/Merlin.java @@ -760,33 +760,20 @@ public class Merlin extends CryptoBase { Collection<Pattern> subjectCertConstraints ) throws WSSecurityException { // - // FIRST step - Search the keystore for the transmitted certificate + // FIRST step - Search the truststore and keystore for the transmitted certificate // - if (certs.length == 1 && !enableRevocation) { - String issuerString = certs[0].getIssuerX500Principal().getName(); - BigInteger issuerSerial = certs[0].getSerialNumber(); - - CryptoType cryptoType = new CryptoType(CryptoType.TYPE.ISSUER_SERIAL); - cryptoType.setIssuerSerial(issuerString, issuerSerial); - X509Certificate[] foundCerts = getX509Certificates(cryptoType); - - // - // If a certificate has been found, the certificates must be compared - // to ensure against phony DNs (compare encoded form including signature) - // - if (foundCerts != null && foundCerts.length > 0 && foundCerts[0] != null && foundCerts[0].equals(certs[0])) { - try { - certs[0].checkValidity(); - } catch (CertificateExpiredException | CertificateNotYetValidException e) { - throw new WSSecurityException( - WSSecurityException.ErrorCode.FAILED_CHECK, e, "invalidCert" - ); - } - LOG.debug( - "Direct trust for certificate with {}", certs[0].getSubjectX500Principal().getName() + if (certs.length == 1 && !enableRevocation && isDirectlyTrusted(certs[0])) { + try { + certs[0].checkValidity(); + } catch (CertificateExpiredException | CertificateNotYetValidException e) { + throw new WSSecurityException( + WSSecurityException.ErrorCode.FAILED_CHECK, e, "invalidCert" ); - return; } + LOG.debug( + "Direct trust for certificate with {}", certs[0].getSubjectX500Principal().getName() + ); + return; } // @@ -799,13 +786,13 @@ public class Merlin extends CryptoBase { Object subject = convertSubjectToPrincipal(issuerString); - if (keystore != null) { - foundIssuingCertChains = getCertificates(subject, keystore, false); + if (truststore != null) { + foundIssuingCertChains = getCertificates(subject, truststore, true); } - //If we can't find the issuer in the keystore then look at the truststore - if ((foundIssuingCertChains == null || foundIssuingCertChains.isEmpty()) && truststore != null) { - foundIssuingCertChains = getCertificates(subject, truststore, true); + //If we can't find the issuer in the truststore then look at the keystore + if ((foundIssuingCertChains == null || foundIssuingCertChains.isEmpty()) && keystore != null) { + foundIssuingCertChains = getCertificates(subject, keystore, false); } if (foundIssuingCertChains == null || foundIssuingCertChains.isEmpty() @@ -901,6 +888,41 @@ public class Merlin extends CryptoBase { } } + /** + * Check whether the given certificate is directly trusted, i.e. whether the exact same + * certificate is stored in the truststore or the keystore. The truststore is searched + * first, as that is where trusted certificates are normally stored, so that the keystore + * does not need to be searched in the common case (WSS-721). + * + * @param cert The certificate to check + * @return true if the certificate was found in the truststore or keystore + * @throws WSSecurityException + */ + protected boolean isDirectlyTrusted(X509Certificate cert) throws WSSecurityException { + Object issuerName = convertSubjectToPrincipal(cert.getIssuerX500Principal().getName()); + BigInteger serialNumber = cert.getSerialNumber(); + + // + // If a certificate has been found, the certificates must be compared + // to ensure against phony DNs (compare encoded form including signature) + // + if (truststore != null) { + Certificate[] foundCerts = getCertificates(issuerName, serialNumber, truststore, true); + if (foundCerts.length > 0 && cert.equals(foundCerts[0])) { + return true; + } + } + + if (keystore != null) { + Certificate[] foundCerts = getCertificates(issuerName, serialNumber, keystore, false); + if (foundCerts.length > 0 && cert.equals(foundCerts[0])) { + return true; + } + } + + return false; + } + @Override public void verifyTrust(X509Certificate[] certs, boolean enableRevocation, Collection<Pattern> subjectCertConstraints, diff --git a/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/MerlinAKI.java b/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/MerlinAKI.java index 549f64189..90a1782cb 100644 --- a/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/MerlinAKI.java +++ b/ws-security-common/src/main/java/org/apache/wss4j/common/crypto/MerlinAKI.java @@ -20,7 +20,6 @@ package org.apache.wss4j.common.crypto; import java.io.IOException; -import java.math.BigInteger; import java.security.InvalidAlgorithmParameterException; import java.security.KeyStore; import java.security.KeyStoreException; @@ -87,33 +86,20 @@ public class MerlinAKI extends Merlin { Collection<Pattern> subjectCertConstraints ) throws WSSecurityException { // - // FIRST step - Search the keystore for the transmitted certificate + // FIRST step - Search the truststore and keystore for the transmitted certificate // - if (certs.length == 1 && !enableRevocation) { - String issuerString = certs[0].getIssuerX500Principal().getName(); - BigInteger issuerSerial = certs[0].getSerialNumber(); - - CryptoType cryptoType = new CryptoType(CryptoType.TYPE.ISSUER_SERIAL); - cryptoType.setIssuerSerial(issuerString, issuerSerial); - X509Certificate[] foundCerts = getX509Certificates(cryptoType); - - // - // If a certificate has been found, the certificates must be compared - // to ensure against phony DNs (compare encoded form including signature) - // - if (foundCerts != null && foundCerts.length > 0 && foundCerts[0] != null && foundCerts[0].equals(certs[0])) { - try { - certs[0].checkValidity(); - } catch (CertificateExpiredException | CertificateNotYetValidException e) { - throw new WSSecurityException( - WSSecurityException.ErrorCode.FAILED_CHECK, e, "invalidCert" - ); - } - LOG.debug( - "Direct trust for certificate with {}", certs[0].getSubjectX500Principal().getName() + if (certs.length == 1 && !enableRevocation && isDirectlyTrusted(certs[0])) { + try { + certs[0].checkValidity(); + } catch (CertificateExpiredException | CertificateNotYetValidException e) { + throw new WSSecurityException( + WSSecurityException.ErrorCode.FAILED_CHECK, e, "invalidCert" ); - return; } + LOG.debug( + "Direct trust for certificate with {}", certs[0].getSubjectX500Principal().getName() + ); + return; } // @@ -220,13 +206,13 @@ public class MerlinAKI extends Merlin { } Certificate[] certs = null; - if (keystore != null) { - certs = getCertificates(keyIdentifierBytes, keystore); + if (truststore != null) { + certs = getCertificates(keyIdentifierBytes, truststore); } - //If we can't find the issuer in the keystore then look at the truststore - if ((certs == null || certs.length == 0) && truststore != null) { - certs = getCertificates(keyIdentifierBytes, truststore); + //If we can't find the issuer in the truststore then look at the keystore + if ((certs == null || certs.length == 0) && keystore != null) { + certs = getCertificates(keyIdentifierBytes, keystore); } if (certs == null || certs.length == 0) { diff --git a/ws-security-common/src/test/java/org/apache/wss4j/common/crypto/MerlinTrustTest.java b/ws-security-common/src/test/java/org/apache/wss4j/common/crypto/MerlinTrustTest.java new file mode 100644 index 000000000..b4b2dc93a --- /dev/null +++ b/ws-security-common/src/test/java/org/apache/wss4j/common/crypto/MerlinTrustTest.java @@ -0,0 +1,280 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.wss4j.common.crypto; + +import java.io.InputStream; +import java.io.OutputStream; +import java.security.Key; +import java.security.KeyStore; +import java.security.KeyStoreException; +import java.security.KeyStoreSpi; +import java.security.cert.Certificate; +import java.security.cert.X509Certificate; +import java.util.Collections; +import java.util.Date; +import java.util.Enumeration; +import java.util.concurrent.atomic.AtomicInteger; + +import org.apache.wss4j.common.ext.WSSecurityException; +import org.apache.wss4j.common.util.Loader; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +/** + * Tests for the order in which Merlin searches the truststore and keystore when verifying + * trust in a certificate (WSS-721). + */ +public class MerlinTrustTest { + + private static X509Certificate wss40Cert; + + @BeforeAll + public static void setup() throws Exception { + WSProviderConfig.init(); + wss40Cert = (X509Certificate)loadKeyStore("keys/wss40.jks").getCertificate("wss40"); + } + + /** + * A certificate that is in the truststore is directly trusted without searching the keystore. + */ + @Test + public void testDirectTrustSearchesTruststoreFirst() throws Exception { + verifyDirectTrustSearchesTruststoreFirst(new Merlin()); + } + + @Test + public void testDirectTrustSearchesTruststoreFirstAKI() throws Exception { + verifyDirectTrustSearchesTruststoreFirst(new MerlinAKI()); + } + + private void verifyDirectTrustSearchesTruststoreFirst(Merlin crypto) throws Exception { + KeyStore truststore = createEmptyKeyStore(); + truststore.setCertificateEntry("wss40", wss40Cert); + + AtomicInteger keystoreSearches = new AtomicInteger(); + crypto.setKeyStore(countingKeyStore(loadKeyStore("keys/wss40_server.jks"), keystoreSearches)); + crypto.setTrustStore(truststore); + + crypto.verifyTrust(new X509Certificate[] {wss40Cert}, false, null, null); + assertEquals(0, keystoreSearches.get()); + } + + /** + * The issuer of a certificate is looked up in the truststore before the keystore. The keystore + * is only searched once, for the certificate itself, and not again for its issuer. + */ + @Test + public void testIssuerSearchesTruststoreFirst() throws Exception { + AtomicInteger keystoreSearches = new AtomicInteger(); + Merlin crypto = new Merlin(); + crypto.setKeyStore(countingKeyStore(loadKeyStore("keys/wss40_server.jks"), keystoreSearches)); + crypto.setTrustStore(loadKeyStore("keys/wss40CA.jks")); + + crypto.verifyTrust(new X509Certificate[] {wss40Cert}, false, null, null); + assertEquals(1, keystoreSearches.get()); + } + + /** + * A certificate that is only in the keystore is still directly trusted. + */ + @Test + public void testDirectTrustInKeystore() throws Exception { + Merlin crypto = new Merlin(); + crypto.setKeyStore(loadKeyStore("keys/wss40.jks")); + crypto.setTrustStore(createEmptyKeyStore()); + + crypto.verifyTrust(new X509Certificate[] {wss40Cert}, false, null, null); + } + + /** + * A certificate in neither the truststore nor the keystore, whose issuer is only in the keystore, + * is not trusted when a truststore is configured. + */ + @Test + public void testNotTrusted() throws Exception { + Merlin crypto = new Merlin(); + crypto.setKeyStore(loadKeyStore("keys/wss40_server.jks")); + crypto.setTrustStore(createEmptyKeyStore()); + + assertThrows(WSSecurityException.class, + () -> crypto.verifyTrust(new X509Certificate[] {wss40Cert}, false, null, null)); + } + + private static KeyStore loadKeyStore(String path) throws Exception { + KeyStore keyStore = KeyStore.getInstance("JKS"); + ClassLoader loader = Loader.getClassLoader(MerlinTrustTest.class); + try (InputStream input = Merlin.loadInputStream(loader, path)) { + keyStore.load(input, "security".toCharArray()); + } + return keyStore; + } + + private static KeyStore createEmptyKeyStore() throws Exception { + KeyStore keyStore = KeyStore.getInstance("JKS"); + keyStore.load(null, null); + return keyStore; + } + + /** + * Wrap a KeyStore so that each enumeration of its aliases is counted. + */ + private static KeyStore countingKeyStore(KeyStore delegate, AtomicInteger aliasesCount) throws Exception { + KeyStore keyStore = new KeyStore(new CountingKeyStoreSpi(delegate, aliasesCount), + delegate.getProvider(), delegate.getType()) { }; + keyStore.load(null, null); + return keyStore; + } + + private static final class CountingKeyStoreSpi extends KeyStoreSpi { + + private final KeyStore delegate; + private final AtomicInteger aliasesCount; + + CountingKeyStoreSpi(KeyStore delegate, AtomicInteger aliasesCount) { + this.delegate = delegate; + this.aliasesCount = aliasesCount; + } + + @Override + public Enumeration<String> engineAliases() { + aliasesCount.incrementAndGet(); + try { + return delegate.aliases(); + } catch (KeyStoreException e) { + return Collections.emptyEnumeration(); + } + } + + @Override + public Key engineGetKey(String alias, char[] password) { + try { + return delegate.getKey(alias, password); + } catch (Exception e) { + return null; + } + } + + @Override + public Certificate[] engineGetCertificateChain(String alias) { + try { + return delegate.getCertificateChain(alias); + } catch (KeyStoreException e) { + return null; + } + } + + @Override + public Certificate engineGetCertificate(String alias) { + try { + return delegate.getCertificate(alias); + } catch (KeyStoreException e) { + return null; + } + } + + @Override + public Date engineGetCreationDate(String alias) { + try { + return delegate.getCreationDate(alias); + } catch (KeyStoreException e) { + return null; + } + } + + @Override + public void engineSetKeyEntry(String alias, Key key, char[] password, Certificate[] chain) + throws KeyStoreException { + delegate.setKeyEntry(alias, key, password, chain); + } + + @Override + public void engineSetKeyEntry(String alias, byte[] key, Certificate[] chain) throws KeyStoreException { + delegate.setKeyEntry(alias, key, chain); + } + + @Override + public void engineSetCertificateEntry(String alias, Certificate cert) throws KeyStoreException { + delegate.setCertificateEntry(alias, cert); + } + + @Override + public void engineDeleteEntry(String alias) throws KeyStoreException { + delegate.deleteEntry(alias); + } + + @Override + public boolean engineContainsAlias(String alias) { + try { + return delegate.containsAlias(alias); + } catch (KeyStoreException e) { + return false; + } + } + + @Override + public int engineSize() { + try { + return delegate.size(); + } catch (KeyStoreException e) { + return 0; + } + } + + @Override + public boolean engineIsKeyEntry(String alias) { + try { + return delegate.isKeyEntry(alias); + } catch (KeyStoreException e) { + return false; + } + } + + @Override + public boolean engineIsCertificateEntry(String alias) { + try { + return delegate.isCertificateEntry(alias); + } catch (KeyStoreException e) { + return false; + } + } + + @Override + public String engineGetCertificateAlias(Certificate cert) { + try { + return delegate.getCertificateAlias(cert); + } catch (KeyStoreException e) { + return null; + } + } + + @Override + public void engineStore(OutputStream stream, char[] password) { + throw new UnsupportedOperationException(); + } + + @Override + public void engineLoad(InputStream stream, char[] password) { + // The delegate is already loaded + } + } +}
