Refactor OcspResponseValidator.validateCertificateStatusUpdateTime(),…

… make OCSP response time validation parameters configurable WE2-868 Signed-off-by: Mart Somermaa <[email protected]>
web-eid · Apr 9, 2024 · 0ff0a73 · 0ff0a73
1 parent b938031
commit 0ff0a73
Show file tree

Hide file tree

Showing 10 changed files with 164 additions and 68 deletions.
diff --git a/src/main/java/eu/webeid/security/certificate/CertificateValidator.java b/src/main/java/eu/webeid/security/certificate/CertificateValidator.java
@@ -23,9 +23,9 @@
 package eu.webeid.security.certificate;
 
 import eu.webeid.security.exceptions.CertificateExpiredException;
+import eu.webeid.security.exceptions.CertificateNotTrustedException;
 import eu.webeid.security.exceptions.CertificateNotYetValidException;
 import eu.webeid.security.exceptions.JceException;
-import eu.webeid.security.exceptions.CertificateNotTrustedException;
 
 import java.security.GeneralSecurityException;
 import java.security.InvalidAlgorithmParameterException;
@@ -40,7 +40,6 @@
 import java.security.cert.X509CertSelector;
 import java.security.cert.X509Certificate;
 import java.util.Collection;
-import java.util.Collections;
 import java.util.Date;
 import java.util.Set;
 import java.util.stream.Collectors;
@@ -91,9 +90,8 @@ public static X509Certificate validateIsSignedByTrustedCA(X509Certificate certif
     }
 
     public static Set<TrustAnchor> buildTrustAnchorsFromCertificates(Collection<X509Certificate> certificates) {
-        return Collections.unmodifiableSet(certificates.stream()
-            .map(cert -> new TrustAnchor(cert, null))
-            .collect(Collectors.toSet()));
+        return certificates.stream()
+            .map(cert -> new TrustAnchor(cert, null)).collect(Collectors.toUnmodifiableSet());
     }
 
     public static CertStore buildCertStoreFromCertificates(Collection<X509Certificate> certificates) throws JceException {

diff --git a/src/main/java/eu/webeid/security/validator/AuthTokenValidationConfiguration.java b/src/main/java/eu/webeid/security/validator/AuthTokenValidationConfiguration.java
@@ -32,9 +32,10 @@
 import java.security.cert.X509Certificate;
 import java.time.Duration;
 import java.util.Collection;
-import java.util.Collections;
 import java.util.HashSet;
 import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
 
 import static eu.webeid.security.util.Collections.newHashSet;
 import static eu.webeid.security.util.DateAndTime.requirePositiveDuration;
@@ -48,6 +49,8 @@ public final class AuthTokenValidationConfiguration {
     private Collection<X509Certificate> trustedCACertificates = new HashSet<>();
     private boolean isUserCertificateRevocationCheckWithOcspEnabled = true;
     private Duration ocspRequestTimeout = Duration.ofSeconds(5);
+    private long allowedOcspResponseTimeSkewMillis = TimeUnit.MINUTES.toMillis(15);
+    private long maxOcspResponseThisUpdateAgeMillis = TimeUnit.MINUTES.toMillis(2);
     private DesignatedOcspServiceConfiguration designatedOcspServiceConfiguration;
     // Don't allow Estonian Mobile-ID policy by default.
     private Collection<ASN1ObjectIdentifier> disallowedSubjectCertificatePolicies = newHashSet(
@@ -63,12 +66,14 @@ public final class AuthTokenValidationConfiguration {
 
     private AuthTokenValidationConfiguration(AuthTokenValidationConfiguration other) {
         this.siteOrigin = other.siteOrigin;
-        this.trustedCACertificates = Collections.unmodifiableSet(new HashSet<>(other.trustedCACertificates));
+        this.trustedCACertificates = Set.copyOf(other.trustedCACertificates);
         this.isUserCertificateRevocationCheckWithOcspEnabled = other.isUserCertificateRevocationCheckWithOcspEnabled;
         this.ocspRequestTimeout = other.ocspRequestTimeout;
+        this.allowedOcspResponseTimeSkewMillis = other.allowedOcspResponseTimeSkewMillis;
+        this.maxOcspResponseThisUpdateAgeMillis = other.maxOcspResponseThisUpdateAgeMillis;
         this.designatedOcspServiceConfiguration = other.designatedOcspServiceConfiguration;
-        this.disallowedSubjectCertificatePolicies = Collections.unmodifiableSet(new HashSet<>(other.disallowedSubjectCertificatePolicies));
-        this.nonceDisabledOcspUrls = Collections.unmodifiableSet(new HashSet<>(other.nonceDisabledOcspUrls));
+        this.disallowedSubjectCertificatePolicies = Set.copyOf(other.disallowedSubjectCertificatePolicies);
+        this.nonceDisabledOcspUrls = Set.copyOf(other.nonceDisabledOcspUrls);
     }
 
     void setSiteOrigin(URI siteOrigin) {
@@ -99,6 +104,22 @@ void setOcspRequestTimeout(Duration ocspRequestTimeout) {
         this.ocspRequestTimeout = ocspRequestTimeout;
     }
 
+    public long getAllowedOcspResponseTimeSkewMillis() {
+        return allowedOcspResponseTimeSkewMillis;
+    }
+
+    public void setAllowedOcspResponseTimeSkewMillis(long allowedOcspResponseTimeSkewMillis) {
+        this.allowedOcspResponseTimeSkewMillis = allowedOcspResponseTimeSkewMillis;
+    }
+
+    public long getMaxOcspResponseThisUpdateAgeMillis() {
+        return maxOcspResponseThisUpdateAgeMillis;
+    }
+
+    public void setMaxOcspResponseThisUpdateAgeMillis(long maxOcspResponseThisUpdateAgeMillis) {
+        this.maxOcspResponseThisUpdateAgeMillis = maxOcspResponseThisUpdateAgeMillis;
+    }
+
     public DesignatedOcspServiceConfiguration getDesignatedOcspServiceConfiguration() {
         return designatedOcspServiceConfiguration;
     }
@@ -128,6 +149,12 @@ void validate() {
             throw new IllegalArgumentException("At least one trusted certificate authority must be provided");
         }
         requirePositiveDuration(ocspRequestTimeout, "OCSP request timeout");
+        if (allowedOcspResponseTimeSkewMillis <= 0) {
+            throw new IllegalArgumentException("Allowed OCSP response time-skew  must be greater than zero");
+        }
+        if (maxOcspResponseThisUpdateAgeMillis <= 0) {
+            throw new IllegalArgumentException("Max OCSP response thisUpdate age must be greater than zero");
+        }
     }
 
     AuthTokenValidationConfiguration copy() {
@@ -156,5 +183,4 @@ public static void validateIsOriginURL(URI uri) throws IllegalArgumentException
             throw new IllegalArgumentException("An URI syntax exception occurred");
         }
     }
-
 }
diff --git a/src/main/java/eu/webeid/security/validator/AuthTokenValidatorBuilder.java b/src/main/java/eu/webeid/security/validator/AuthTokenValidatorBuilder.java
@@ -127,6 +127,38 @@ public AuthTokenValidatorBuilder withOcspRequestTimeout(Duration ocspRequestTime
         return this;
     }
 
+    /**
+     * Sets the allowed time skew in milliseconds for OCSP response's thisUpdate and nextUpdate times.
+     * This value is used to tolerate slight discrepancies between the system clock and the OCSP responder's clock.
+     * <p>
+     * This is an optional configuration parameter, the default is 15 minutes.
+     *
+     * @param allowedTimeSkewMillis the allowed time skew in milliseconds
+     * @return the builder instance for method chaining.
+     */
+    // TODO: use Duration.
+    public AuthTokenValidatorBuilder withAllowedOcspResponseTimeSkewMilliseconds(long allowedTimeSkewMillis) {
+        configuration.setAllowedOcspResponseTimeSkewMillis(allowedTimeSkewMillis);
+        LOG.debug("Allowed OCSP response time skew set to {} milliseconds", allowedTimeSkewMillis);
+        return this;
+    }
+
+    /**
+     * Sets the maximum age in milliseconds of the OCSP response's thisUpdate time before it is considered too old.
+     * <p>
+     * This is an optional configuration parameter, the default is 2 minutes.
+     *
+     * @param maxThisUpdateAgeMillis the maximum age of the OCSP response's thisUpdate time in milliseconds
+     * @return the builder instance for method chaining.
+     */
+    // TODO: use Duration.
+    public AuthTokenValidatorBuilder withMaxOcspResponseThisUpdateAgeMilliseconds(long maxThisUpdateAgeMillis) {
+        configuration.setMaxOcspResponseThisUpdateAgeMillis(maxThisUpdateAgeMillis);
+        LOG.debug("Maximum OCSP response thisUpdate age set to {} milliseconds", maxThisUpdateAgeMillis);
+        return this;
+    }
+
+
     /**
      * Adds the given URLs to the list of OCSP URLs for which the nonce protocol extension will be disabled.
      * The OCSP URL is extracted from the user certificate and some OCSP services don't support the nonce extension.

diff --git a/src/main/java/eu/webeid/security/validator/AuthTokenValidatorImpl.java b/src/main/java/eu/webeid/security/validator/AuthTokenValidatorImpl.java
@@ -27,9 +27,9 @@
 import eu.webeid.security.authtoken.WebEidAuthToken;
 import eu.webeid.security.certificate.CertificateLoader;
 import eu.webeid.security.certificate.CertificateValidator;
-import eu.webeid.security.exceptions.JceException;
-import eu.webeid.security.exceptions.AuthTokenParseException;
 import eu.webeid.security.exceptions.AuthTokenException;
+import eu.webeid.security.exceptions.AuthTokenParseException;
+import eu.webeid.security.exceptions.JceException;
 import eu.webeid.security.validator.certvalidators.SubjectCertificateExpiryValidator;
 import eu.webeid.security.validator.certvalidators.SubjectCertificateNotRevokedValidator;
 import eu.webeid.security.validator.certvalidators.SubjectCertificatePolicyValidator;
@@ -64,10 +64,8 @@ final class AuthTokenValidatorImpl implements AuthTokenValidator {
     private final SubjectCertificateValidatorBatch simpleSubjectCertificateValidators;
     private final Set<TrustAnchor> trustedCACertificateAnchors;
     private final CertStore trustedCACertificateCertStore;
-    // OcspClient uses OkHttp internally.
-    // OkHttp performs best when a single OkHttpClient instance is created and reused for all HTTP calls.
-    // This is because each client holds its own connection pool and thread pools.
-    // Reusing connections and threads reduces latency and saves memory.
+    // OcspClient uses built-in HttpClient internally by default.
+    // A single HttpClient instance is reused for all HTTP calls to utilize connection and thread pools.
     private OcspClient ocspClient;
     private OcspServiceProvider ocspServiceProvider;
     private final AuthTokenSignatureValidator authTokenSignatureValidator;
@@ -186,7 +184,11 @@ private SubjectCertificateValidatorBatch getCertTrustValidators() {
         return SubjectCertificateValidatorBatch.createFrom(
             certTrustedValidator::validateCertificateTrusted
         ).addOptional(configuration.isUserCertificateRevocationCheckWithOcspEnabled(),
-            new SubjectCertificateNotRevokedValidator(certTrustedValidator, ocspClient, ocspServiceProvider)::validateCertificateNotRevoked
+            new SubjectCertificateNotRevokedValidator(certTrustedValidator,
+                ocspClient, ocspServiceProvider,
+                configuration.getAllowedOcspResponseTimeSkewMillis(),
+                configuration.getMaxOcspResponseThisUpdateAgeMillis()
+            )::validateCertificateNotRevoked
         );
     }
 

diff --git a/...va/eu/webeid/security/validator/certvalidators/SubjectCertificateNotRevokedValidator.java b/...va/eu/webeid/security/validator/certvalidators/SubjectCertificateNotRevokedValidator.java
@@ -63,17 +63,23 @@ public final class SubjectCertificateNotRevokedValidator {
     private final SubjectCertificateTrustedValidator trustValidator;
     private final OcspClient ocspClient;
     private final OcspServiceProvider ocspServiceProvider;
+    private final long allowedOcspResponseTimeSkewMillis;
+    private final long maxOcspResponseThisUpdateAgeMillis;
 
     static {
         Security.addProvider(new BouncyCastleProvider());
     }
 
     public SubjectCertificateNotRevokedValidator(SubjectCertificateTrustedValidator trustValidator,
                                                  OcspClient ocspClient,
-                                                 OcspServiceProvider ocspServiceProvider) {
+                                                 OcspServiceProvider ocspServiceProvider,
+                                                 long allowedOcspResponseTimeSkewMillis,
+                                                 long maxOcspResponseThisUpdateAgeMillis) {
         this.trustValidator = trustValidator;
         this.ocspClient = ocspClient;
         this.ocspServiceProvider = ocspServiceProvider;
+        this.allowedOcspResponseTimeSkewMillis = allowedOcspResponseTimeSkewMillis;
+        this.maxOcspResponseThisUpdateAgeMillis = maxOcspResponseThisUpdateAgeMillis;
     }
 
     /**
@@ -166,7 +172,7 @@ private void verifyOcspResponse(BasicOCSPResp basicResponse, OcspService ocspSer
         //      be available about the status of the certificate (nextUpdate) is
         //      greater than the current time.
 
-        OcspResponseValidator.validateCertificateStatusUpdateTime(certStatusResponse);
+        OcspResponseValidator.validateCertificateStatusUpdateTime(certStatusResponse, allowedOcspResponseTimeSkewMillis, maxOcspResponseThisUpdateAgeMillis);
 
         // Now we can accept the signed response as valid and validate the certificate status.
         OcspResponseValidator.validateSubjectCertificateStatus(certStatusResponse);

diff --git a/src/main/java/eu/webeid/security/validator/ocsp/OcspResponseValidator.java b/src/main/java/eu/webeid/security/validator/ocsp/OcspResponseValidator.java
@@ -44,7 +44,6 @@
 import java.util.Date;
 import java.util.Objects;
 import java.util.TimeZone;
-import java.util.concurrent.TimeUnit;
 
 public final class OcspResponseValidator {
 
@@ -54,8 +53,7 @@ public final class OcspResponseValidator {
      * https://oidref.com/1.3.6.1.5.5.7.3.9
      */
     private static final String OID_OCSP_SIGNING = "1.3.6.1.5.5.7.3.9";
-
-    static final long ALLOWED_TIME_SKEW_MILLIS = TimeUnit.MINUTES.toMillis(15);
+    private static final String ERROR_PREFIX = "Certificate status update time check failed: ";
 
     public static void validateHasSigningExtension(X509Certificate certificate) throws OCSPCertificateException {
         Objects.requireNonNull(certificate, "certificate");
@@ -78,7 +76,7 @@ public static void validateResponseSignature(BasicOCSPResp basicResponse, X509Ce
         }
     }
 
-    public static void validateCertificateStatusUpdateTime(SingleResp certStatusResponse) throws UserCertificateOCSPCheckFailedException {
+    public static void validateCertificateStatusUpdateTime(SingleResp certStatusResponse, long allowedTimeSkewMillis, long maxThisupdateAgeMillis) throws UserCertificateOCSPCheckFailedException {
         // From RFC 2560, https://www.ietf.org/rfc/rfc2560.txt:
         // 4.2.2.  Notes on OCSP Responses
         // 4.2.2.1.  Time
@@ -89,17 +87,33 @@ public static void validateCertificateStatusUpdateTime(SingleResp certStatusResp
         //   If nextUpdate is not set, the responder is indicating that newer
         //   revocation information is available all the time.
         final Date now = DateAndTime.DefaultClock.getInstance().now();
-        final Date notAllowedBefore = new Date(now.getTime() - ALLOWED_TIME_SKEW_MILLIS);
-        final Date notAllowedAfter = new Date(now.getTime() + ALLOWED_TIME_SKEW_MILLIS);
+        final Date earliestAcceptableTimeSkew = new Date(now.getTime() - allowedTimeSkewMillis);
+        final Date latestAcceptableTimeSkew = new Date(now.getTime() + allowedTimeSkewMillis);
+        final Date earliestAcceptableThisUpdateTime = new Date(now.getTime() - maxThisupdateAgeMillis);
+
         final Date thisUpdate = certStatusResponse.getThisUpdate();
-        final Date nextUpdate = certStatusResponse.getNextUpdate() != null ? certStatusResponse.getNextUpdate() : thisUpdate;
-        if (notAllowedAfter.before(thisUpdate) ||
-            notAllowedBefore.after(nextUpdate)) {
-            throw new UserCertificateOCSPCheckFailedException("Certificate status update time check failed: " +
-                "notAllowedBefore: " + toUtcString(notAllowedBefore) +
-                ", notAllowedAfter: " + toUtcString(notAllowedAfter) +
-                ", thisUpdate: " + toUtcString(thisUpdate) +
-                ", nextUpdate: " + toUtcString(certStatusResponse.getNextUpdate()));
+        if (thisUpdate.after(latestAcceptableTimeSkew)) {
+            throw new UserCertificateOCSPCheckFailedException(ERROR_PREFIX +
+                "thisUpdate '" + toUtcString(thisUpdate) + "' is too far in the future, " +
+                "latest allowed: '" + toUtcString(latestAcceptableTimeSkew) + "'");
+        }
+        if (thisUpdate.before(earliestAcceptableThisUpdateTime)) {
+            throw new UserCertificateOCSPCheckFailedException(ERROR_PREFIX +
+                "thisUpdate '" + toUtcString(thisUpdate) + "' is too old, " +
+                "earliest allowed: '" + toUtcString(earliestAcceptableThisUpdateTime) + "'");
+        }
+
+        final Date nextUpdate = certStatusResponse.getNextUpdate();
+        if (nextUpdate == null) {
+            return;
+        }
+        if (nextUpdate.before(earliestAcceptableTimeSkew)) {
+            throw new UserCertificateOCSPCheckFailedException(ERROR_PREFIX +
+                "nextUpdate '" + toUtcString(nextUpdate) + "' is in the past");
+        }
+        if (nextUpdate.before(thisUpdate)) {
+            throw new UserCertificateOCSPCheckFailedException(ERROR_PREFIX +
+                "nextUpdate '" + toUtcString(nextUpdate) + "' is before thisUpdate '" + toUtcString(thisUpdate) + "'");
         }
     }
 

diff --git a/src/test/java/eu/webeid/security/validator/AuthTokenValidatorBuilderTest.java b/src/test/java/eu/webeid/security/validator/AuthTokenValidatorBuilderTest.java
@@ -29,7 +29,11 @@
 
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
-class AuthTokenValidatorBuilderTest {
+public class AuthTokenValidatorBuilderTest {
+
+    // AuthTokenValidationConfiguration has a package-private constructor, but some tests need access to it outside its package.
+    // Provide a public accessor to it for these tests.
+    public static final AuthTokenValidationConfiguration CONFIGURATION = new AuthTokenValidationConfiguration();
 
     final AuthTokenValidatorBuilder builder = new AuthTokenValidatorBuilder();