diff --git a/demo/pom.xml b/demo/pom.xml index febee05..bfac6b7 100644 --- a/demo/pom.xml +++ b/demo/pom.xml @@ -1,5 +1,6 @@ - + oiosaml3-demo OIOSAML Demo for Java v3 4.0.0 diff --git a/idp/pom.xml b/idp/pom.xml index 7bf1573..fb8bd50 100644 --- a/idp/pom.xml +++ b/idp/pom.xml @@ -1,4 +1,5 @@ - + 4.0.0 dk.digst oiosaml3-idp @@ -9,6 +10,9 @@ 11 UTF-8 + + 1.18.46 @@ -96,6 +100,22 @@ + + org.apache.maven.plugins + maven-compiler-plugin + + + + + org.projectlombok + lombok + ${lombok.version} + + + + + org.springframework.boot spring-boot-maven-plugin diff --git a/integrationtest/pom.xml b/integrationtest/pom.xml index 3154086..faed05e 100644 --- a/integrationtest/pom.xml +++ b/integrationtest/pom.xml @@ -1,5 +1,6 @@ - + oiosaml3-integrationtest OIOSAML Integrationtest Java v3 4.0.0 diff --git a/oiosaml/pom.xml b/oiosaml/pom.xml index abdad86..b9f6a1a 100644 --- a/oiosaml/pom.xml +++ b/oiosaml/pom.xml @@ -1,6 +1,6 @@ + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd"> 4.0.0 dk.digst oiosaml3.java @@ -37,30 +37,29 @@ 1.8.0 false - + - - sign - - - - org.apache.maven.plugins - maven-gpg-plugin - 1.5 - - - sign-artifacts - verify - - sign - - - - - - - - + + sign + + + + org.apache.maven.plugins + maven-gpg-plugin + 1.5 + + + sign-artifacts + verify + + sign + + + + + + + @@ -87,7 +86,7 @@ true - + org.apache.maven.plugins maven-source-plugin @@ -101,7 +100,7 @@ - + org.apache.maven.plugins maven-javadoc-plugin @@ -175,6 +174,14 @@ 2.3.9 + + + org.bouncycastle + bcpkix-jdk15on + 1.59 + test + + org.junit.jupiter junit-jupiter-engine diff --git a/oiosaml/src/main/java/dk/gov/oio/saml/model/IdPMetadata.java b/oiosaml/src/main/java/dk/gov/oio/saml/model/IdPMetadata.java index 1c274b1..5d56368 100644 --- a/oiosaml/src/main/java/dk/gov/oio/saml/model/IdPMetadata.java +++ b/oiosaml/src/main/java/dk/gov/oio/saml/model/IdPMetadata.java @@ -179,22 +179,22 @@ private void doRevocationCheck() throws ExternalException, InternalException { if (lastCRLCheck == null || (lastUpdate != null && lastUpdate.isAfter(lastCRLCheck))) { try { // Encryption - Set validEncryptionCertificates = CRLChecker.checkCertificates(getAllX509CertificatesWithUsageType(UsageType.ENCRYPTION), getLastCRLCheck()); + Set validEncryptionCertificates = CRLChecker.checkCertificates(getAllX509CertificatesWithUsageType(UsageType.ENCRYPTION)); this.validEncryptionCertificates.clear(); if (validEncryptionCertificates != null) { this.validEncryptionCertificates.addAll(validEncryptionCertificates); } // Signing - Set validSigningCertificates = CRLChecker.checkCertificates(getAllX509CertificatesWithUsageType(UsageType.SIGNING), getLastCRLCheck()); + Set validSigningCertificates = CRLChecker.checkCertificates(getAllX509CertificatesWithUsageType(UsageType.SIGNING)); this.validSigningCertificates.clear(); if (validSigningCertificates != null) { this.validSigningCertificates.addAll(validSigningCertificates); } // Unspecified - Set validUnspecifiedCertificates = CRLChecker.checkCertificates(getAllX509CertificatesWithUsageType(UsageType.UNSPECIFIED), getLastCRLCheck()); - Set validNullCertificates = CRLChecker.checkCertificates(getAllX509CertificatesWithUsageType(null), getLastCRLCheck()); + Set validUnspecifiedCertificates = CRLChecker.checkCertificates(getAllX509CertificatesWithUsageType(UsageType.UNSPECIFIED)); + Set validNullCertificates = CRLChecker.checkCertificates(getAllX509CertificatesWithUsageType(null)); this.validUnspecifiedCertificates.clear(); if (validUnspecifiedCertificates != null) { diff --git a/oiosaml/src/main/java/dk/gov/oio/saml/service/CRLChecker.java b/oiosaml/src/main/java/dk/gov/oio/saml/service/CRLChecker.java index c42f518..6ea59f2 100644 --- a/oiosaml/src/main/java/dk/gov/oio/saml/service/CRLChecker.java +++ b/oiosaml/src/main/java/dk/gov/oio/saml/service/CRLChecker.java @@ -5,6 +5,8 @@ import java.net.URI; import java.net.URISyntaxException; import java.net.URL; +import java.net.URLConnection; +import java.security.GeneralSecurityException; import java.security.InvalidAlgorithmParameterException; import java.security.NoSuchAlgorithmException; import java.security.cert.CRLException; @@ -13,6 +15,7 @@ import java.security.cert.CertPathValidatorException; import java.security.cert.CertificateException; import java.security.cert.CertificateFactory; +import java.security.cert.CertPathValidatorException.BasicReason; import java.security.cert.PKIXParameters; import java.security.cert.PKIXRevocationChecker; import java.security.cert.TrustAnchor; @@ -21,13 +24,15 @@ import java.security.cert.X509Certificate; import java.util.ArrayList; import java.util.Collections; +import java.util.Date; import java.util.EnumSet; -import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -44,7 +49,6 @@ import org.bouncycastle.asn1.x509.GeneralNames; import org.bouncycastle.i18n.filter.UntrustedUrlInput; import org.bouncycastle.x509.extension.X509ExtensionUtil; -import org.joda.time.DateTime; import org.opensaml.core.config.InitializationException; import dk.gov.oio.saml.config.Configuration; @@ -59,17 +63,26 @@ public class CRLChecker { // (request base64-encoded into the URL path) for small requests, see sun.security.provider.certpath.OCSP private static final String JDK_OCSP_USE_GET_PROPERTY = "com.sun.security.ocsp.useget"; - private static Map certificateMap = new HashMap(); + // Revocation data is fetched from locations named by the certificate being checked, a slow or + // unresponsive endpoint must not hold the revocation check open + private static final int CONNECT_TIMEOUT_MILLIS = 10000; + private static final int READ_TIMEOUT_MILLIS = 10000; + + private static final Map issuingCACertificatesByUrl = new ConcurrentHashMap(); + + // Indexes of the keyCertSign and cRLSign bits in the KeyUsage extension, see RFC 5280 section 4.2.1.3 + private static final int KEY_CERT_SIGN = 5; + private static final int CRL_SIGN = 6; /** * Make the JDK OCSP client POST its requests instead of using the RFC 5019 GET form. - * + *

* JDK 12 and later default to GET for requests that fit in 255 characters, but the NemLog-in OCSP * responders answer HTTP 404 to that form. The resulting UNDETERMINED_REVOCATION_STATUS makes every * OCSP check fail, which silently degrades the SP to the CRL fallback, or drops the IdP certificates * entirely when CRL checking is disabled as well. POST is mandatory for responders (RFC 6960), so * forcing it is safe. Java 8 and 11 always POST and are unaffected. - * + *

* The JDK reads the property once, when sun.security.provider.certpath.OCSP is initialized, so this * must run before the first OCSP check in the JVM, hence the call from {@link OIOSAML3Service#init}. * An explicit setting made by the deployer (-Dcom.sun.security.ocsp.useget=...) always wins. @@ -90,9 +103,9 @@ static void configureOcspTransport(Configuration configuration) { log.info("Setting '{}' to false, so OCSP requests are sent using POST", JDK_OCSP_USE_GET_PROPERTY); } - public static Set checkCertificates(List x509Certificates, DateTime lastCRLCheck) throws ExternalException, InternalException, InitializationException { + public static Set checkCertificates(List x509Certificates) throws ExternalException, InternalException, InitializationException { Set result = new HashSet<>(); - if (x509Certificates == null || x509Certificates.size() == 0) { + if (x509Certificates == null || x509Certificates.isEmpty()) { return result; } @@ -101,8 +114,7 @@ public static Set checkCertificates(List x509C if (checkCertificate(certificate)) { result.add(certificate); log.debug("Certificate validated successfully: {}", certificate.getSubjectDN()); - } - else { + } else { log.warn("Certificate did not validate: {}", certificate.getSubjectDN()); } } @@ -112,35 +124,40 @@ public static Set checkCertificates(List x509C // OCSP first if configured, with fallback to CRL if configured private static boolean checkCertificate(X509Certificate certificate) { + if (!isWithinValidityPeriod(certificate)) { + log.warn("Certificate is outside its validity period: {}", certificate.getSubjectDN()); + return false; + } + boolean validated = false; Configuration config = OIOSAML3Service.getConfig(); if (config.isOcspCheckEnabled()) { try { validated = doOCSPCheck(certificate); - } - catch (Exception e) { + } catch (Exception e) { + if (!isRevocationDataUnavailable(e)) { + log.warn("Certificate rejected while validating it using OCSP: {}", certificate.getSubjectDN(), e); + return false; + } + log.warn("Unexpected error while validating certificate using OCSP.", e); if (config.isCRLCheckEnabled()) { try { validated = doCRLCheck(certificate); - } - catch (Exception ex) { + } catch (Exception ex) { log.warn("Unexpected error while validating certificate using CRL.", ex); - } + } } } - } - else if (config.isCRLCheckEnabled()) { + } else if (config.isCRLCheckEnabled()) { try { validated = doCRLCheck(certificate); - } - catch (Exception ex) { + } catch (Exception ex) { log.warn("Unexpected error while validating certificate using CRL.", ex); - } - } - else { + } + } else { log.warn("checkCertificate called, but both OCSP and CRL checking is disabled"); validated = true; } @@ -156,6 +173,8 @@ private static boolean doOCSPCheck(X509Certificate certificate) throws Certifica throw new RuntimeException("No OCSP access location could be found"); } + URI responder = toHttpUri(ocspServer); + // try to retrieve issuing OCES CA certificate X509Certificate issuer = getIssuingCertificate(certificate); if (issuer == null) { @@ -179,7 +198,7 @@ private static boolean doOCSPCheck(X509Certificate certificate) throws Certifica // "ocsp.enable"/"ocsp.responderURL" security properties, leaks the responder of the certificate // being checked into every other PKIX validation in the JVM, and races with concurrent checks. PKIXRevocationChecker revocationChecker = (PKIXRevocationChecker) cpv.getRevocationChecker(); - revocationChecker.setOcspResponder(new URI(ocspServer)); + revocationChecker.setOcspResponder(responder); // Fallback to CRL is handled by checkCertificate, according to the OIOSAML configuration revocationChecker.setOptions(EnumSet.of(PKIXRevocationChecker.Option.NO_FALLBACK)); @@ -192,13 +211,11 @@ private static boolean doOCSPCheck(X509Certificate certificate) throws Certifica log.debug("Certificate successfully validated during OCSP check."); revoked = false; - } - catch (CertPathValidatorException cpve) { - if (cpve.getMessage() != null && cpve.getMessage().contains("Certificate has been revoked")) { + } catch (CertPathValidatorException cpve) { + if (BasicReason.REVOKED == cpve.getReason()) { revoked = true; log.info("Certificate revoked, cert[{}] : {}", cpve.getIndex(), cpve.getMessage()); - } - else { + } else { log.warn("Validation failure, cert[{}] : {}", cpve.getIndex(), cpve.getMessage()); throw cpve; } @@ -207,6 +224,22 @@ private static boolean doOCSPCheck(X509Certificate certificate) throws Certifica return (!revoked); } + /** + * Whether the OCSP check failed because the revocation status could not be obtained, rather than because + * the certificate itself failed validation. Only the former is a reason to ask the CRL instead: the CRL + * says nothing about the validity of a certificate, so letting it answer for a rejected certificate + * would turn that rejection into an acceptance. + */ + private static boolean isRevocationDataUnavailable(Exception e) { + if (e instanceof CertPathValidatorException) { + return BasicReason.UNDETERMINED_REVOCATION_STATUS == ((CertPathValidatorException) e).getReason(); + } + + // Everything else is a failure, such as a certificate naming no responder + // or an issuer certificate that could not be fetched + return true; + } + private static X509Certificate getIssuingCertificate(X509Certificate certificate) { log.debug("Attempting to extract issuing ca certifcate from certificate {}", certificate.getSubjectDN()); @@ -217,17 +250,16 @@ private static X509Certificate getIssuingCertificate(X509Certificate certificate try (ASN1InputStream aIn = new ASN1InputStream(bytes)) { ASN1OctetString octs = (ASN1OctetString) aIn.readObject(); - + try (ASN1InputStream aIn2 = new ASN1InputStream(octs.getOctets())) { ASN1Primitive auth_info_acc = aIn2.readObject(); - + if (auth_info_acc != null) { authInfoAcc = AuthorityInformationAccess.getInstance(auth_info_acc); } } } - } - catch (Exception e) { + } catch (Exception e) { log.debug("Cannot extract access location of issuing ca.", e); return null; } @@ -236,35 +268,133 @@ private static X509Certificate getIssuingCertificate(X509Certificate certificate Iterator urlIt = issuingCaUrls.iterator(); while (urlIt.hasNext()) { Object caUrl = new UntrustedUrlInput(urlIt.next()); + String url = caUrl.toString(); + + // A cached certificate is only reused while it still is the issuer, so that a CA replaced at the + // same location is picked up instead of failing every check until the process restarts + X509Certificate cached = issuingCACertificatesByUrl.get(url); + if (cached != null && isIssuedBy(cached, certificate)) { + return cached; + } + + X509Certificate issuer = downloadCertificate(url); + if (issuer == null || !isIssuedBy(issuer, certificate)) { + return null; + } - return downloadCertificate(caUrl.toString()); + issuingCACertificatesByUrl.put(url, issuer); + + return issuer; } return null; } - - private static X509Certificate downloadCertificate(String url) { - if (certificateMap.containsKey(url)) { - return certificateMap.get(url); + + /** + * The AIA location is named by the certificate being checked, so the certificate downloaded from it can + * only serve as trust anchor or CRL signer once it is known to be a CA allowed to issue certificates, + * and to be the one that issued that certificate. + */ + private static boolean isIssuedBy(X509Certificate issuer, X509Certificate certificate) { + if (!certificate.getIssuerX500Principal().equals(issuer.getSubjectX500Principal())) { + log.warn("Certificate fetched from the issuer location of {} is issued to somebody else", certificate.getSubjectDN()); + return false; } - + + // Below zero means the basic constraints deny the certificate the CA role, or say nothing at all + if (issuer.getBasicConstraints() < 0) { + log.warn("Certificate fetched from the issuer location of {} is not a CA certificate", certificate.getSubjectDN()); + return false; + } + + if (!hasKeyUsage(issuer, KEY_CERT_SIGN)) { + log.warn("Certificate fetched from the issuer location of {} is not allowed to sign certificates", certificate.getSubjectDN()); + return false; + } + + if (!isWithinValidityPeriod(issuer)) { + log.warn("Certificate fetched from the issuer location of {} is outside its validity period", certificate.getSubjectDN()); + return false; + } + + try { + certificate.verify(issuer.getPublicKey()); + return true; + } catch (GeneralSecurityException e) { + log.warn("Certificate fetched from the issuer location of {} did not issue it", certificate.getSubjectDN(), e); + return false; + } + } + + /** + * Open a stream to a location named by certificate content. + * + *

Only http and https are allowed. The OCES CAs serve revocation data over plain http, which is fine + * because the data is signed, but the scheme has to be constrained so that a certificate cannot make the + * SP read from file:, jar: or any other protocol the JVM happens to support.

+ */ + private static InputStream openStream(String url) throws IOException { + URL location = new URL(url); + + String protocol = location.getProtocol().toLowerCase(Locale.ROOT); + if (!"http".equals(protocol) && !"https".equals(protocol)) { + throw new IOException(String.format("Refusing to fetch revocation data over '%s'", protocol)); + } + + URLConnection connection = location.openConnection(); + connection.setConnectTimeout(CONNECT_TIMEOUT_MILLIS); + connection.setReadTimeout(READ_TIMEOUT_MILLIS); + + return connection.getInputStream(); + } + + private static URI toHttpUri(String url) throws URISyntaxException { + URI uri = new URI(url); + + String scheme = (uri.getScheme() != null) ? uri.getScheme().toLowerCase(Locale.ROOT) : ""; + if (!"http".equals(scheme) && !"https".equals(scheme)) { + throw new RuntimeException(String.format("Refusing to use OCSP responder with scheme '%s'", scheme)); + } + + return uri; + } + + /** + * Whether the certificate is within its validity period, allowing for the configured clock skew the way + * the freshness check on a CRL does. + */ + private static boolean isWithinValidityPeriod(X509Certificate certificate) { + long clockSkewMillis = 1000L * 60 * OIOSAML3Service.getConfig().getClockSkew(); + long now = System.currentTimeMillis(); + + return certificate.getNotBefore().getTime() - clockSkewMillis <= now + && certificate.getNotAfter().getTime() + clockSkewMillis >= now; + } + + /** + * A key is only usable for the purposes its certificate grants it, and a certificate stating no key + * usage at all grants none of them. + */ + private static boolean hasKeyUsage(X509Certificate certificate, int keyUsageBit) { + boolean[] keyUsage = certificate.getKeyUsage(); + + return keyUsage != null && keyUsage.length > keyUsageBit && keyUsage[keyUsageBit]; + } + + private static X509Certificate downloadCertificate(String url) { try { CertificateFactory factory = CertificateFactory.getInstance("X.509"); - try (InputStream is = new URL(url).openStream()) { + try (InputStream is = openStream(url)) { X509Certificate certificate = (X509Certificate) factory.generateCertificate(is); if (certificate != null) { - certificateMap.put(url, certificate); - return certificate; } - + log.warn("Failed to parse certificate from {}", url); - } - catch (IOException ex) { + } catch (IOException ex) { log.warn("Failed to download intermediate CA certificate from {}", url); } - } - catch (CertificateException ex) { + } catch (CertificateException ex) { log.warn("Failed to generate certificate factory", ex); } @@ -280,17 +410,16 @@ private static String getOCSPUrl(X509Certificate certificate) { byte[] bytes = certificate.getExtensionValue(AUTH_INFO_ACCESS); try (ASN1InputStream aIn = new ASN1InputStream(bytes)) { ASN1OctetString octs = (ASN1OctetString) aIn.readObject(); - + try (ASN1InputStream aIn2 = new ASN1InputStream(octs.getOctets())) { ASN1Primitive auth_info_acc = aIn2.readObject(); - + if (auth_info_acc != null) { authInfoAcc = AuthorityInformationAccess.getInstance(auth_info_acc); } } } - } - catch (Exception e) { + } catch (Exception e) { log.debug("Cannot extract access location of OCSP responder.", e); return null; } @@ -345,27 +474,32 @@ private static List getOCSPUrls(AuthorityInformationAccess authInfoAcces return urls; } - private static boolean doCRLCheck(X509Certificate certificate) throws IOException, CertificateException, CRLException, InitializationException { - boolean revoked = true; + private static boolean doCRLCheck(X509Certificate certificate) throws IOException, GeneralSecurityException, InitializationException { + boolean revoked; String url = getCRLUrl(certificate); if (url == null) { throw new RuntimeException("No CRL url could be found"); } - URL u = new URL(url); - try (InputStream is = u.openStream()) { + X509Certificate issuer = getIssuingCertificate(certificate); + if (issuer == null) { + throw new RuntimeException("CA Certificate for CRL check could not be retrieved!"); + } + + try (InputStream is = openStream(url)) { CertificateFactory cf = CertificateFactory.getInstance("X.509"); X509CRL crl = (X509CRL) cf.generateCRL(is); log.debug("CRL for {}: {}", url, crl); + validateCRL(crl, certificate, issuer); + X509CRLEntry revokedCertificate = crl.getRevokedCertificate(certificate.getSerialNumber()); if (revokedCertificate != null) { log.warn("Certificate found in revocation list " + certificate.getSubjectDN()); revoked = true; - } - else { + } else { revoked = false; } } @@ -373,6 +507,38 @@ private static boolean doCRLCheck(X509Certificate certificate) throws IOExceptio return !revoked; } + /** + * A CRL is fetched from a location named by the certificate itself, so the absence of a serial number in + * it only means anything once the list is known to be signed by the issuing CA and to be current. + */ + private static void validateCRL(X509CRL crl, X509Certificate certificate, X509Certificate issuer) throws GeneralSecurityException { + if (!crl.getIssuerX500Principal().equals(certificate.getIssuerX500Principal())) { + throw new CRLException("CRL was not issued by the CA that issued the certificate"); + } + + // A CA that is not allowed to sign CRLs cannot vouch for this list however well the signature verifies + if (!hasKeyUsage(issuer, CRL_SIGN)) { + throw new CRLException("CRL issuer is not allowed to sign CRLs, its key usage does not include cRLSign"); + } + + crl.verify(issuer.getPublicKey()); + + long clockSkewMillis = 1000L * 60 * OIOSAML3Service.getConfig().getClockSkew(); + Date now = new Date(); + + Date thisUpdate = crl.getThisUpdate(); + if (thisUpdate == null || thisUpdate.getTime() - clockSkewMillis > now.getTime()) { + throw new CRLException("CRL is not valid yet, thisUpdate is " + thisUpdate); + } + + // RFC 5280 leaves nextUpdate optional, but a list that does not say when it is superseded cannot be + // told apart from one that was replaced long ago + Date nextUpdate = crl.getNextUpdate(); + if (nextUpdate == null || nextUpdate.getTime() + clockSkewMillis < now.getTime()) { + throw new CRLException("CRL is no longer current, nextUpdate is " + nextUpdate); + } + } + private static String getCRLUrl(X509Certificate certificate) throws IOException { log.debug("Attempting to extract distribution point from certificate {}", certificate.getSubjectDN()); diff --git a/oiosaml/src/test/java/dk/gov/oio/saml/service/CRLCheckerLocalTest.java b/oiosaml/src/test/java/dk/gov/oio/saml/service/CRLCheckerLocalTest.java new file mode 100644 index 0000000..8487eb5 --- /dev/null +++ b/oiosaml/src/test/java/dk/gov/oio/saml/service/CRLCheckerLocalTest.java @@ -0,0 +1,221 @@ +package dk.gov.oio.saml.service; + +import static org.mockserver.model.HttpRequest.request; +import static org.mockserver.model.HttpResponse.response; + +import java.math.BigInteger; +import java.security.KeyPair; +import java.security.cert.X509CRL; +import java.security.cert.X509Certificate; +import java.util.Collections; +import java.util.Date; +import java.util.List; +import java.util.Set; + +import org.bouncycastle.asn1.x509.BasicConstraints; +import org.bouncycastle.asn1.x509.KeyUsage; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.mockserver.client.MockServerClient; + +import dk.gov.oio.saml.util.TestPkiUtil; + +/** + * Revocation checking against a locally served CA, so the CRL can be tampered with. The tests against the + * live NemLog-in test CA are in {@link CRLCheckerTest}. + */ +public class CRLCheckerLocalTest extends BaseServiceTest { + private static final String CRL_PATH = "/test-ca/crl"; + private static final String CA_ISSUER_PATH = "/test-ca/cacert"; + private static final String OCSP_PATH = "/test-ca/ocsp"; + private static final BigInteger SERIAL_NUMBER = BigInteger.valueOf(4711); + private static final String CA_NAME = "OIOSAML test CA"; + private static final KeyUsage CA_KEY_USAGE = new KeyUsage(KeyUsage.keyCertSign | KeyUsage.cRLSign); + + private KeyPair caKeyPair; + private X509Certificate caCertificate; + private X509Certificate certificate; + + @BeforeEach + public void beforeEach(MockServerClient mockServer) throws Exception { + OIOSAML3Service.getConfig().setCRLCheckEnabled(true); + OIOSAML3Service.getConfig().setOcspCheckEnabled(false); + + caKeyPair = TestPkiUtil.generateKeyPair(); + caCertificate = TestPkiUtil.createCaCertificate(caKeyPair, CA_NAME); + certificate = TestPkiUtil.createCertificate(TestPkiUtil.generateKeyPair(), "OIOSAML test certificate", SERIAL_NUMBER, + caCertificate, caKeyPair, url(CRL_PATH), url(CA_ISSUER_PATH)); + + mockServer.reset(); + mockServer.when(request().withPath(CA_ISSUER_PATH)).respond(response().withBody(caCertificate.getEncoded())); + } + + @DisplayName("Test that a certificate absent from a valid CRL is accepted") + @Test + public void testAcceptCertificateAbsentFromCrl(MockServerClient mockServer) throws Exception { + serveCrl(mockServer, TestPkiUtil.createCrl(caCertificate, caKeyPair, TestPkiUtil.hoursFromNow(24))); + + Assertions.assertEquals(1, checkCertificate().size()); + } + + @DisplayName("Test that a certificate listed in a valid CRL is rejected") + @Test + public void testRejectCertificateListedInCrl(MockServerClient mockServer) throws Exception { + serveCrl(mockServer, TestPkiUtil.createCrl(caCertificate, caKeyPair, TestPkiUtil.hoursFromNow(24), SERIAL_NUMBER)); + + Assertions.assertEquals(0, checkCertificate().size()); + } + + @DisplayName("Test that a CRL signed by another key is not trusted") + @Test + public void testRejectCrlSignedByAnotherKey(MockServerClient mockServer) throws Exception { + // Same content as the accepted list above, only the signing key differs + serveCrl(mockServer, TestPkiUtil.createCrl(caCertificate, TestPkiUtil.generateKeyPair(), TestPkiUtil.hoursFromNow(24))); + + Assertions.assertEquals(0, checkCertificate().size()); + } + + @DisplayName("Test that a CRL that is no longer current is not trusted") + @Test + public void testRejectStaleCrl(MockServerClient mockServer) throws Exception { + serveCrl(mockServer, TestPkiUtil.createCrl(caCertificate, caKeyPair, TestPkiUtil.hoursFromNow(-12))); + + Assertions.assertEquals(0, checkCertificate().size()); + } + + @DisplayName("Test that a CRL without a nextUpdate is not trusted") + @Test + public void testRejectCrlWithoutNextUpdate(MockServerClient mockServer) throws Exception { + serveCrl(mockServer, TestPkiUtil.createCrl(caCertificate, caKeyPair, (Date) null)); + + Assertions.assertEquals(0, checkCertificate().size()); + } + + @DisplayName("Test that a CRL is not trusted when the key usage of the CA does not allow it to sign CRLs") + @Test + public void testRejectCrlFromCaWithoutCrlSignKeyUsage(MockServerClient mockServer) throws Exception { + serveCaAndCertificate(mockServer, TestPkiUtil.createCaCertificate(caKeyPair, CA_NAME, new KeyUsage(KeyUsage.keyCertSign), new BasicConstraints(0))); + serveCrl(mockServer, TestPkiUtil.createCrl(caCertificate, caKeyPair, TestPkiUtil.hoursFromNow(24))); + + Assertions.assertEquals(0, checkCertificate().size()); + } + + @DisplayName("Test that a CA whose key usage does not allow it to sign certificates is not trusted") + @Test + public void testRejectCaWithoutKeyCertSignKeyUsage(MockServerClient mockServer) throws Exception { + serveCaAndCertificate(mockServer, TestPkiUtil.createCaCertificate(caKeyPair, CA_NAME, new KeyUsage(KeyUsage.cRLSign), new BasicConstraints(0))); + serveCrl(mockServer, TestPkiUtil.createCrl(caCertificate, caKeyPair, TestPkiUtil.hoursFromNow(24))); + + Assertions.assertEquals(0, checkCertificate().size()); + } + + @DisplayName("Test that a CA whose basic constraints do not give it the CA role is not trusted") + @Test + public void testRejectCaWithoutBasicConstraints(MockServerClient mockServer) throws Exception { + serveCaAndCertificate(mockServer, TestPkiUtil.createCaCertificate(caKeyPair, CA_NAME, CA_KEY_USAGE, null)); + serveCrl(mockServer, TestPkiUtil.createCrl(caCertificate, caKeyPair, TestPkiUtil.hoursFromNow(24))); + + Assertions.assertEquals(0, checkCertificate().size()); + } + + @DisplayName("Test that a certificate that has expired is rejected") + @Test + public void testRejectExpiredCertificate(MockServerClient mockServer) throws Exception { + useCertificate(null, TestPkiUtil.hoursFromNow(-48), TestPkiUtil.hoursFromNow(-24)); + serveCrl(mockServer, TestPkiUtil.createCrl(caCertificate, caKeyPair, TestPkiUtil.hoursFromNow(24))); + + Assertions.assertEquals(0, checkCertificate().size()); + } + + @DisplayName("Test that a certificate that is not valid yet is rejected") + @Test + public void testRejectCertificateNotValidYet(MockServerClient mockServer) throws Exception { + useCertificate(null, TestPkiUtil.hoursFromNow(24), TestPkiUtil.hoursFromNow(48)); + serveCrl(mockServer, TestPkiUtil.createCrl(caCertificate, caKeyPair, TestPkiUtil.hoursFromNow(24))); + + Assertions.assertEquals(0, checkCertificate().size()); + } + + @DisplayName("Test that a CA that has expired is not trusted") + @Test + public void testRejectExpiredCa(MockServerClient mockServer) throws Exception { + serveCaAndCertificate(mockServer, TestPkiUtil.createCaCertificate(caKeyPair, CA_NAME, CA_KEY_USAGE, + new BasicConstraints(0), TestPkiUtil.hoursFromNow(-48), TestPkiUtil.hoursFromNow(-24))); + serveCrl(mockServer, TestPkiUtil.createCrl(caCertificate, caKeyPair, TestPkiUtil.hoursFromNow(24))); + + Assertions.assertEquals(0, checkCertificate().size()); + } + + @DisplayName("Test that the CRL answers when the OCSP responder cannot state a revocation status") + @Test + public void testFallBackToCrlWhenRevocationStatusIsUnavailable(MockServerClient mockServer) throws Exception { + OIOSAML3Service.getConfig().setOcspCheckEnabled(true); + useCertificate(url(OCSP_PATH), TestPkiUtil.hoursFromNow(-1), TestPkiUtil.hoursFromNow(24)); + + // Nothing is served at the OCSP location, so the responder cannot be reached + serveCrl(mockServer, TestPkiUtil.createCrl(caCertificate, caKeyPair, TestPkiUtil.hoursFromNow(24))); + + Assertions.assertEquals(1, checkCertificate().size()); + } + + @DisplayName("Test that the CRL does not answer for a certificate the OCSP path rejected") + @Test + public void testDoNotFallBackToCrlWhenTheCertificateWasRejected(MockServerClient mockServer) throws Exception { + OIOSAML3Service.getConfig().setOcspCheckEnabled(true); + + // Expired, but so recently that the clock skew still lets it past the validity check, so the OCSP + // path is reached and rejects it where the CRL, which says nothing about validity, would not + useCertificate(url(OCSP_PATH), TestPkiUtil.hoursFromNow(-24), TestPkiUtil.minutesFromNow(-2)); + serveCrl(mockServer, TestPkiUtil.createCrl(caCertificate, caKeyPair, TestPkiUtil.hoursFromNow(24))); + + Assertions.assertEquals(0, checkCertificate().size()); + } + + @DisplayName("Test that a certificate not issued by the CA at its issuer location is rejected") + @Test + public void testRejectCertificateNotIssuedByServedCa(MockServerClient mockServer) throws Exception { + // The CA certificate served at the issuer location belongs to a different CA + KeyPair otherCaKeyPair = TestPkiUtil.generateKeyPair(); + X509Certificate otherCaCertificate = TestPkiUtil.createCaCertificate(otherCaKeyPair, CA_NAME); + + mockServer.reset(); + mockServer.when(request().withPath(CA_ISSUER_PATH)).respond(response().withBody(otherCaCertificate.getEncoded())); + serveCrl(mockServer, TestPkiUtil.createCrl(otherCaCertificate, otherCaKeyPair, TestPkiUtil.hoursFromNow(24))); + + Assertions.assertEquals(0, checkCertificate().size()); + } + + /** + * Replaces the CA, and the certificate being checked with one issued by it, so a CA differing from the + * one set up for every test in a single respect can be put in its place. + */ + private void serveCaAndCertificate(MockServerClient mockServer, X509Certificate replacementCaCertificate) throws Exception { + caCertificate = replacementCaCertificate; + certificate = TestPkiUtil.createCertificate(TestPkiUtil.generateKeyPair(), "OIOSAML test certificate", SERIAL_NUMBER, + caCertificate, caKeyPair, url(CRL_PATH), url(CA_ISSUER_PATH)); + + mockServer.reset(); + mockServer.when(request().withPath(CA_ISSUER_PATH)).respond(response().withBody(caCertificate.getEncoded())); + } + + private void useCertificate(String ocspUrl, Date notBefore, Date notAfter) throws Exception { + certificate = TestPkiUtil.createCertificate(TestPkiUtil.generateKeyPair(), "OIOSAML test certificate", SERIAL_NUMBER, + caCertificate, caKeyPair, url(CRL_PATH), url(CA_ISSUER_PATH), ocspUrl, notBefore, notAfter); + } + + private void serveCrl(MockServerClient mockServer, X509CRL crl) throws Exception { + mockServer.when(request().withPath(CRL_PATH)).respond(response().withBody(crl.getEncoded())); + } + + private Set checkCertificate() throws Exception { + List certificates = Collections.singletonList(certificate); + + return CRLChecker.checkCertificates(certificates); + } + + private static String url(String path) { + return "http://localhost:8081" + path; + } +} diff --git a/oiosaml/src/test/java/dk/gov/oio/saml/service/CRLCheckerTest.java b/oiosaml/src/test/java/dk/gov/oio/saml/service/CRLCheckerTest.java index ab3848c..3dd2ef9 100644 --- a/oiosaml/src/test/java/dk/gov/oio/saml/service/CRLCheckerTest.java +++ b/oiosaml/src/test/java/dk/gov/oio/saml/service/CRLCheckerTest.java @@ -30,7 +30,7 @@ public class CRLCheckerTest extends BaseServiceTest { X509Certificate certificate = (X509Certificate) factory.generateCertificate(bis); List x509Certificates = Collections.singletonList(certificate); - Set validCertificates = CRLChecker.checkCertificates(x509Certificates, null); + Set validCertificates = CRLChecker.checkCertificates(x509Certificates); Assertions.assertEquals(1, validCertificates.size()); } @@ -47,7 +47,7 @@ public class CRLCheckerTest extends BaseServiceTest { X509Certificate certificate = (X509Certificate) factory.generateCertificate(bis); List x509Certificates = Collections.singletonList(certificate); - Set validCertificates = CRLChecker.checkCertificates(x509Certificates, null); + Set validCertificates = CRLChecker.checkCertificates(x509Certificates); Assertions.assertEquals(1, validCertificates.size()); } @@ -68,7 +68,7 @@ public class CRLCheckerTest extends BaseServiceTest { X509Certificate certificate = (X509Certificate) factory.generateCertificate(bis); List x509Certificates = Collections.singletonList(certificate); - Set validCertificates = CRLChecker.checkCertificates(x509Certificates, null); + Set validCertificates = CRLChecker.checkCertificates(x509Certificates); Assertions.assertNotNull(validCertificates); Assertions.assertEquals(0, validCertificates.size()); @@ -90,7 +90,7 @@ public class CRLCheckerTest extends BaseServiceTest { X509Certificate certificate = (X509Certificate) factory.generateCertificate(bis); List x509Certificates = Collections.singletonList(certificate); - Set validCertificates = CRLChecker.checkCertificates(x509Certificates, null); + Set validCertificates = CRLChecker.checkCertificates(x509Certificates); Assertions.assertNotNull(validCertificates); Assertions.assertEquals(0, validCertificates.size()); diff --git a/oiosaml/src/test/java/dk/gov/oio/saml/util/TestPkiUtil.java b/oiosaml/src/test/java/dk/gov/oio/saml/util/TestPkiUtil.java new file mode 100644 index 0000000..4c2b9b2 --- /dev/null +++ b/oiosaml/src/test/java/dk/gov/oio/saml/util/TestPkiUtil.java @@ -0,0 +1,150 @@ +package dk.gov.oio.saml.util; + +import java.math.BigInteger; +import java.security.KeyPair; +import java.security.KeyPairGenerator; +import java.security.cert.X509CRL; +import java.security.cert.X509Certificate; +import java.util.Date; + +import org.bouncycastle.asn1.x500.X500Name; +import org.bouncycastle.asn1.x509.AccessDescription; +import org.bouncycastle.asn1.x509.AuthorityInformationAccess; +import org.bouncycastle.asn1.x509.BasicConstraints; +import org.bouncycastle.asn1.x509.CRLDistPoint; +import org.bouncycastle.asn1.x509.DistributionPoint; +import org.bouncycastle.asn1.x509.DistributionPointName; +import org.bouncycastle.asn1.x509.Extension; +import org.bouncycastle.asn1.x509.GeneralName; +import org.bouncycastle.asn1.x509.GeneralNames; +import org.bouncycastle.asn1.x509.KeyUsage; +import org.bouncycastle.cert.X509v2CRLBuilder; +import org.bouncycastle.cert.jcajce.JcaX509CRLConverter; +import org.bouncycastle.cert.jcajce.JcaX509CertificateConverter; +import org.bouncycastle.cert.jcajce.JcaX509v3CertificateBuilder; +import org.bouncycastle.operator.ContentSigner; +import org.bouncycastle.operator.jcajce.JcaContentSignerBuilder; + +/** + * Mints a CA, certificates and CRLs so revocation checking can be tested against locally served, and locally + * tampered with, revocation data instead of a live CA. + */ +public class TestPkiUtil { + private static final String SIGNATURE_ALGORITHM = "SHA256withRSA"; + + public static KeyPair generateKeyPair() throws Exception { + KeyPairGenerator generator = KeyPairGenerator.getInstance("RSA"); + generator.initialize(2048); + + return generator.generateKeyPair(); + } + + /** + * Self signed CA certificate, the issuer of the certificates and CRLs below, holding the basic + * constraints and key usage a CA needs to issue both. + */ + public static X509Certificate createCaCertificate(KeyPair keyPair, String name) throws Exception { + return createCaCertificate(keyPair, name, new KeyUsage(KeyUsage.keyCertSign | KeyUsage.cRLSign), new BasicConstraints(0)); + } + + /** + * Self signed CA certificate with the given key usage and basic constraints, either of which is left out + * of the certificate when null, so a CA lacking what it takes to issue certificates or CRLs can be + * tested. + */ + public static X509Certificate createCaCertificate(KeyPair keyPair, String name, KeyUsage keyUsage, BasicConstraints basicConstraints) throws Exception { + return createCaCertificate(keyPair, name, keyUsage, basicConstraints, hoursFromNow(-1), hoursFromNow(24)); + } + + /** + * Self signed CA certificate valid in the given period, so a CA that has expired, or is not valid yet, + * can be tested. + */ + public static X509Certificate createCaCertificate(KeyPair keyPair, String name, KeyUsage keyUsage, BasicConstraints basicConstraints, Date notBefore, Date notAfter) throws Exception { + X500Name subject = new X500Name("CN=" + name); + + JcaX509v3CertificateBuilder builder = new JcaX509v3CertificateBuilder( + subject, BigInteger.ONE, notBefore, notAfter, subject, keyPair.getPublic()); + if (basicConstraints != null) { + builder.addExtension(Extension.basicConstraints, true, basicConstraints); + } + if (keyUsage != null) { + builder.addExtension(Extension.keyUsage, true, keyUsage); + } + + return convert(builder, keyPair); + } + + /** + * Certificate naming the given locations as its CRL distribution point and issuer access location, the + * way the OCES certificates do. + */ + public static X509Certificate createCertificate(KeyPair keyPair, String name, BigInteger serialNumber, X509Certificate caCertificate, KeyPair caKeyPair, String crlUrl, String caIssuerUrl) throws Exception { + return createCertificate(keyPair, name, serialNumber, caCertificate, caKeyPair, crlUrl, caIssuerUrl, null, hoursFromNow(-1), hoursFromNow(24)); + } + + /** + * Certificate naming an OCSP responder as well, and valid in the given period, so the OCSP path and a + * certificate that has expired, or is not valid yet, can be tested. No responder is named when the OCSP + * location is null. + */ + public static X509Certificate createCertificate(KeyPair keyPair, String name, BigInteger serialNumber, X509Certificate caCertificate, KeyPair caKeyPair, String crlUrl, String caIssuerUrl, String ocspUrl, Date notBefore, Date notAfter) throws Exception { + JcaX509v3CertificateBuilder builder = new JcaX509v3CertificateBuilder( + caCertificate, serialNumber, notBefore, notAfter, new X500Name("CN=" + name), keyPair.getPublic()); + + builder.addExtension(Extension.cRLDistributionPoints, false, new CRLDistPoint(new DistributionPoint[] { + new DistributionPoint(new DistributionPointName(new GeneralNames(uri(crlUrl))), null, null)})); + builder.addExtension(Extension.authorityInfoAccess, false, accessDescriptions(caIssuerUrl, ocspUrl)); + + return convert(builder, caKeyPair); + } + + private static AuthorityInformationAccess accessDescriptions(String caIssuerUrl, String ocspUrl) { + AccessDescription caIssuers = new AccessDescription(AccessDescription.id_ad_caIssuers, uri(caIssuerUrl)); + if (ocspUrl == null) { + return new AuthorityInformationAccess(caIssuers); + } + + return new AuthorityInformationAccess(new AccessDescription[] { + caIssuers, new AccessDescription(AccessDescription.id_ad_ocsp, uri(ocspUrl))}); + } + + /** + * CRL listing the given serial numbers as revoked. + * + * @param signingKeyPair key the list is signed with, which is not necessarily the CA key + * @param nextUpdate when the list is superseded, in the past for a list that is no longer current + */ + public static X509CRL createCrl(X509Certificate caCertificate, KeyPair signingKeyPair, Date nextUpdate, BigInteger... revokedSerialNumbers) throws Exception { + X509v2CRLBuilder builder = new X509v2CRLBuilder(new X500Name(caCertificate.getSubjectX500Principal().getName()), hoursFromNow(-1)); + if (nextUpdate != null) { + builder.setNextUpdate(nextUpdate); + } + + for (BigInteger serialNumber : revokedSerialNumbers) { + builder.addCRLEntry(serialNumber, hoursFromNow(-1), 0); + } + + ContentSigner signer = new JcaContentSignerBuilder(SIGNATURE_ALGORITHM).build(signingKeyPair.getPrivate()); + + return new JcaX509CRLConverter().getCRL(builder.build(signer)); + } + + public static Date hoursFromNow(int hours) { + return minutesFromNow(hours * 60); + } + + public static Date minutesFromNow(int minutes) { + return new Date(System.currentTimeMillis() + (minutes * 60L * 1000)); + } + + private static X509Certificate convert(JcaX509v3CertificateBuilder builder, KeyPair signingKeyPair) throws Exception { + ContentSigner signer = new JcaContentSignerBuilder(SIGNATURE_ALGORITHM).build(signingKeyPair.getPrivate()); + + return new JcaX509CertificateConverter().getCertificate(builder.build(signer)); + } + + private static GeneralName uri(String url) { + return new GeneralName(GeneralName.uniformResourceIdentifier, url); + } +} diff --git a/pom.xml b/pom.xml index 3093c83..a3ef499 100644 --- a/pom.xml +++ b/pom.xml @@ -1,4 +1,5 @@ - + 4.0.0 dk.digst oiosaml3-parent @@ -11,19 +12,19 @@ true - - - - org.apache.maven.plugins - maven-compiler-plugin - 3.8.1 - - - 8 - - + + + + org.apache.maven.plugins + maven-compiler-plugin + 3.8.1 + + + 8 + + org.apache.maven.plugins @@ -33,11 +34,11 @@ true true - - - + + + - + oiosaml demo integrationtest