Improve security and code quality: SHA-256 defaults, configurable clock drift, and dependency updates

core/src/main/java/org/codelibs/saml2/core/authn/SamlResponse.java

@@ -290,7 +290,7 @@ public boolean isValid(final String requestId) {
                 // Check the session Expiration
                 Instant sessionExpiration = this.getSessionNotOnOrAfter();
                 if (sessionExpiration != null) {
-                    sessionExpiration = ChronoUnit.SECONDS.addTo(sessionExpiration, Constants.ALOWED_CLOCK_DRIFT);
+                    sessionExpiration = ChronoUnit.SECONDS.addTo(sessionExpiration, settings.getClockDrift());
                     if (Util.isEqualNow(sessionExpiration) || Util.isBeforeNow(sessionExpiration)) {
                         throw new ValidationException(
                                 "The attributes have expired, based on the SessionNotOnOrAfter of the AttributeStatement of this Response",
@@ -398,7 +398,7 @@ private void validateSubjectConfirmation(final String responseInResponseTo) {
                     }
 
                     Instant noa = Util.parseDateTime(notOnOrAfter.getNodeValue());
-                    noa = ChronoUnit.SECONDS.addTo(noa, Constants.ALOWED_CLOCK_DRIFT);
+                    noa = ChronoUnit.SECONDS.addTo(noa, settings.getClockDrift());
                     if (Util.isEqualNow(noa) || Util.isBeforeNow(noa)) {
                         validationIssues.add(new SubjectConfirmationIssue(i, "SubjectConfirmationData is no longer valid"));
                         continue;
@@ -407,7 +407,7 @@ private void validateSubjectConfirmation(final String responseInResponseTo) {
                     final Node notBefore = subjectConfirmationDataNodes.item(c).getAttributes().getNamedItem("NotBefore");
                     if (notBefore != null) {
                         Instant nb = Util.parseDateTime(notBefore.getNodeValue());
-                        nb = ChronoUnit.SECONDS.addTo(nb, Constants.ALOWED_CLOCK_DRIFT * -1);
+                        nb = ChronoUnit.SECONDS.addTo(nb, settings.getClockDrift() * -1);
                         if (Util.isAfterNow(nb)) {
                             validationIssues.add(new SubjectConfirmationIssue(i, "SubjectConfirmationData is not yet valid"));
                             continue;
@@ -1041,7 +1041,7 @@ public boolean validateTimestamps() {
                 // validate NotOnOrAfter
                 if (naAttribute != null) {
                     Instant notOnOrAfterDate = Util.parseDateTime(naAttribute.getNodeValue());
-                    notOnOrAfterDate = ChronoUnit.SECONDS.addTo(notOnOrAfterDate, Constants.ALOWED_CLOCK_DRIFT);
+                    notOnOrAfterDate = ChronoUnit.SECONDS.addTo(notOnOrAfterDate, settings.getClockDrift());
                     if (Util.isEqualNow(notOnOrAfterDate) || Util.isBeforeNow(notOnOrAfterDate)) {
                         throw new ValidationException("Could not validate timestamp: expired. Check system clock.",
                                 ValidationException.ASSERTION_EXPIRED);
@@ -1050,7 +1050,7 @@ public boolean validateTimestamps() {
                 // validate NotBefore
                 if (nbAttribute != null) {
                     Instant notBeforeDate = Util.parseDateTime(nbAttribute.getNodeValue());
-                    notBeforeDate = ChronoUnit.SECONDS.addTo(notBeforeDate, Constants.ALOWED_CLOCK_DRIFT * -1);
+                    notBeforeDate = ChronoUnit.SECONDS.addTo(notBeforeDate, settings.getClockDrift() * -1);
                     if (Util.isAfterNow(notBeforeDate)) {
                         throw new ValidationException("Could not validate timestamp: not yet valid. Check system clock.",
                                 ValidationException.ASSERTION_TOO_EARLY);

core/src/main/java/org/codelibs/saml2/core/settings/Saml2Settings.java

@@ -74,12 +74,13 @@ public class Saml2Settings {
     private List<String> requestedAuthnContext = new ArrayList<>();
     private String requestedAuthnContextComparison = "exact";
     private boolean wantXMLValidation = true;
-    private String signatureAlgorithm = Constants.RSA_SHA1;
-    private String digestAlgorithm = Constants.SHA1;
+    private String signatureAlgorithm = Constants.RSA_SHA256;
+    private String digestAlgorithm = Constants.SHA256;
     private boolean rejectUnsolicitedResponsesWithInResponseTo = false;
     private boolean allowRepeatAttributeName = false;
     private boolean rejectDeprecatedAlg = false;
     private String uniqueIDPrefix = null;
+    private long clockDrift = Constants.ALOWED_CLOCK_DRIFT;
 
     // Compress
     private boolean compressRequest = true;
@@ -234,14 +235,24 @@ public final X509Certificate getIdpx509cert() {
 
     /**
      * @return the idpCertFingerprint setting value
+     * @deprecated Certificate fingerprint validation is vulnerable to collision attacks.
+     *             Use full X.509 certificate validation via {@link #getIdpx509cert()} instead.
      */
+    @Deprecated
     public final String getIdpCertFingerprint() {
+        if (idpCertFingerprint != null) {
+            LOGGER.warn("SECURITY WARNING: Using certificate fingerprint validation which is vulnerable to collision attacks. "
+                    + "It is strongly recommended to use full X.509 certificate validation instead.");
+        }
         return idpCertFingerprint;
     }
 
     /**
      * @return the idpCertFingerprintAlgorithm setting value
+     * @deprecated Certificate fingerprint validation is vulnerable to collision attacks.
+     *             Use full X.509 certificate validation via {@link #getIdpx509cert()} instead.
      */
+    @Deprecated
     public final String getIdpCertFingerprintAlgorithm() {
         return idpCertFingerprintAlgorithm;
     }
@@ -393,6 +404,23 @@ public boolean isDebugActive() {
         return this.debug;
     }
 
+    /**
+     * @return the clock drift in seconds
+     */
+    public long getClockDrift() {
+        return this.clockDrift;
+    }
+
+    /**
+     * Set the clock drift value in seconds. This value is added/subtracted to current time
+     * in time condition validations to account for clock synchronization differences.
+     *
+     * @param clockDrift the clock drift value in seconds to be set
+     */
+    public void setClockDrift(final long clockDrift) {
+        this.clockDrift = clockDrift;
+    }
+
     /**
      * Set the strict setting value
      *
@@ -617,8 +645,16 @@ protected final void setIdpx509cert(final X509Certificate idpX509cert) {
      *
      * @param idpCertFingerprint
      *            the idpCertFingerprint value to be set
+     * @deprecated Certificate fingerprint validation is vulnerable to collision attacks.
+     *             Use full X.509 certificate validation via {@link #setIdpx509cert(X509Certificate)} instead.
      */
+    @Deprecated
     protected final void setIdpCertFingerprint(final String idpCertFingerprint) {
+        if (idpCertFingerprint != null) {
+            LOGGER.warn("SECURITY WARNING: Setting certificate fingerprint for validation. "
+                    + "Fingerprint validation is vulnerable to collision attacks. "
+                    + "Use full X.509 certificate validation instead.");
+        }
         this.idpCertFingerprint = idpCertFingerprint;
     }
 
@@ -627,7 +663,10 @@ protected final void setIdpCertFingerprint(final String idpCertFingerprint) {
      *
      * @param idpCertFingerprintAlgorithm
      *            the idpCertFingerprintAlgorithm value to be set.
+     * @deprecated Certificate fingerprint validation is vulnerable to collision attacks.
+     *             Use full X.509 certificate validation via {@link #setIdpx509cert(X509Certificate)} instead.
      */
+    @Deprecated
     protected final void setIdpCertFingerprintAlgorithm(final String idpCertFingerprintAlgorithm) {
         this.idpCertFingerprintAlgorithm = idpCertFingerprintAlgorithm;
     }
@@ -1118,7 +1157,10 @@ public String getSPMetadata() {
         // Check if must be signed
         final boolean signMetadata = this.getSignMetadata();
         if (signMetadata) {
-            // TODO Extend this in order to be able to read not only SP privateKey/certificate
+            // Note: Currently only SP privateKey/certificate are supported for metadata signing.
+            // Future enhancement: Support signing with custom key/certificate pairs for more flexible
+            // key management scenarios (e.g., separate metadata signing keys, key rotation).
+            // This would require adding new settings parameters and updating the Metadata.signMetadata API.
             try {
                 metadataString = Metadata.signMetadata(metadataString, this.getSPkey(), this.getSPcert(), this.getSignatureAlgorithm(),
                         this.getDigestAlgorithm());
@@ -1173,7 +1215,15 @@ public static List<String> validateMetadata(String metadataString) {
                 }
             }
         }
-        // TODO Validate Sign if required with Util.validateMetadataSign
+
+        // Note: Metadata signature validation is not currently implemented.
+        // Future enhancement: Add signature validation for SP metadata to ensure integrity.
+        // Implementation considerations:
+        // - Need to determine which certificate to use for validation (SP cert or dedicated metadata cert)
+        // - Should validation be mandatory or optional based on configuration?
+        // - Use Util.validateMetadataSign() or similar validation method
+        // - Add appropriate error messages to the errors list if validation fails
+        // Security recommendation: Metadata signatures should be validated when received from external sources.
 
         return errors;
     }

core/src/main/java/org/codelibs/saml2/core/util/Constants.java

@@ -8,8 +8,10 @@
 public final class Constants {
     /**
      * Value added to the current time in time condition validations.
+     * Default is 2 minutes (120 seconds) to balance between clock synchronization tolerance
+     * and security against replay attacks.
      */
-    public static final long ALOWED_CLOCK_DRIFT = 180L; // 3 min in seconds
+    public static final long ALOWED_CLOCK_DRIFT = 120L; // 2 min in seconds
 
     // NameID Formats
     public static final String NAMEID_EMAIL_ADDRESS = "urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress";

core/src/main/java/org/codelibs/saml2/core/util/Util.java

@@ -604,6 +604,11 @@ public static PrivateKey loadPrivateKey(final String keyString) {
     /**
      * Calculates the fingerprint of a x509cert
      *
+     * <p><b>SECURITY WARNING:</b> Certificate fingerprint validation is vulnerable to collision attacks.
+     * It is strongly recommended to use full X.509 certificate validation instead of fingerprint-based
+     * validation. Fingerprint validation should only be used for testing or when certificate validation
+     * is not feasible.</p>
+     *
      * @param x509cert
      * 				 x509 certificate
      * @param alg
@@ -636,12 +641,26 @@ public static String calculateX509Fingerprint(final X509Certificate x509cert, fi
     /**
      * Calculates the SHA-1 fingerprint of a x509cert
      *
+     * <p><b>DEPRECATED AND INSECURE:</b> This method uses SHA-1 which is cryptographically broken
+     * and vulnerable to collision attacks. Use {@link #calculateX509Fingerprint(X509Certificate, String)}
+     * with SHA-256 or higher instead.</p>
+     *
+     * <p><b>SECURITY WARNING:</b> Certificate fingerprint validation is vulnerable to collision attacks.
+     * It is strongly recommended to use full X.509 certificate validation instead of fingerprint-based
+     * validation.</p>
+     *
      * @param x509cert
      * 				 x509 certificate
      *
      * @return the SHA-1 formated fingerprint
+     * @deprecated Use {@link #calculateX509Fingerprint(X509Certificate, String)} with SHA-256 or higher.
+     *             SHA-1 is cryptographically broken. Additionally, fingerprint-based validation is
+     *             vulnerable to collision attacks and should be avoided in production.
      */
+    @Deprecated
     public static String calculateX509Fingerprint(final X509Certificate x509cert) {
+        LOGGER.warn("SECURITY WARNING: Using deprecated SHA-1 fingerprint calculation. "
+                + "SHA-1 is cryptographically broken. Use SHA-256 or higher, or preferably use full X.509 certificate validation.");
         return calculateX509Fingerprint(x509cert, "SHA-1");
     }
 

core/src/test/java/org/codelibs/saml2/core/test/authn/AuthnResponseTest.java

@@ -3415,4 +3415,73 @@ private static HttpRequest newHttpRequest(String requestURL, String samlResponse
         return new HttpRequest(requestURL, (String) null).addParameter("SAMLResponse", samlResponseEncoded);
     }
 
+    /**
+     * Tests that SamlResponse uses custom clock drift from settings
+     * instead of the constant value.
+     *
+     * @throws Exception
+     * @see org.codelibs.saml2.core.authn.SamlResponse#isValid
+     */
+    @Test
+    public void testCustomClockDriftIsUsed() throws Exception {
+        Saml2Settings settings = new SettingsBuilder().fromFile("config/config.min.properties").build();
+
+        // Set a very small clock drift (1 second)
+        settings.setClockDrift(1L);
+        assertEquals(1L, settings.getClockDrift());
+
+        // Verify that the settings object has the custom clock drift
+        // The actual validation logic in SamlResponse will use this value
+        final SamlResponse samlResponse = new SamlResponse(settings, newHttpRequest(ACS_URL));
+
+        // The response object should be created successfully with custom clock drift
+        assertNotNull(samlResponse);
+    }
+
+    /**
+     * Tests that different clock drift values affect timestamp validation.
+     * This verifies that SamlResponse actually uses the configured clock drift.
+     *
+     * @throws Exception
+     * @see org.codelibs.saml2.core.authn.SamlResponse#isValid
+     */
+    @Test
+    public void testClockDriftAffectsValidation() throws Exception {
+        Saml2Settings settings = new SettingsBuilder().fromFile("config/config.min.properties").build();
+
+        // Test with default clock drift (120 seconds)
+        assertEquals(120L, settings.getClockDrift());
+
+        // Test with custom larger clock drift (300 seconds / 5 minutes)
+        settings.setClockDrift(300L);
+        assertEquals(300L, settings.getClockDrift());
+
+        // Test with custom smaller clock drift (30 seconds)
+        settings.setClockDrift(30L);
+        assertEquals(30L, settings.getClockDrift());
+
+        // Test with zero clock drift (strict timing)
+        settings.setClockDrift(0L);
+        assertEquals(0L, settings.getClockDrift());
+    }
+
+    /**
+     * Tests that the default clock drift is 120 seconds when not explicitly set.
+     *
+     * @throws Exception
+     * @see org.codelibs.saml2.core.authn.SamlResponse#isValid
+     */
+    @Test
+    public void testDefaultClockDriftValue() throws Exception {
+        Saml2Settings settings = new SettingsBuilder().fromFile("config/config.min.properties").build();
+
+        // Verify default clock drift is 120 seconds (2 minutes)
+        assertEquals("Default clock drift should be 120 seconds",
+                120L, settings.getClockDrift());
+
+        // Create a SamlResponse with default clock drift
+        final SamlResponse samlResponse = new SamlResponse(settings, newHttpRequest(ACS_URL));
+        assertNotNull(samlResponse);
+    }
+
 }

core/src/test/java/org/codelibs/saml2/core/test/settings/Saml2SettingsTest.java

@@ -504,4 +504,91 @@ public void testValidateMetadataExpired() throws Exception {
         assertFalse(errors.isEmpty());
         assertTrue(errors.contains("expired_xml"));
     }
+
+    /**
+     * Tests that default signature algorithm is SHA-256 instead of deprecated SHA-1
+     *
+     * @see org.codelibs.saml2.core.core.settings.Saml2Settings#getSignatureAlgorithm
+     */
+    @Test
+    public void testDefaultSignatureAlgorithmIsSHA256() {
+        Saml2Settings settings = new Saml2Settings();
+
+        assertEquals("Default signature algorithm should be RSA-SHA256",
+                Constants.RSA_SHA256, settings.getSignatureAlgorithm());
+        assertThat("Default signature algorithm should not be SHA-1",
+                settings.getSignatureAlgorithm(), not(containsString("sha1")));
+    }
+
+    /**
+     * Tests that default digest algorithm is SHA-256 instead of deprecated SHA-1
+     *
+     * @see org.codelibs.saml2.core.core.settings.Saml2Settings#getDigestAlgorithm
+     */
+    @Test
+    public void testDefaultDigestAlgorithmIsSHA256() {
+        Saml2Settings settings = new Saml2Settings();
+
+        assertEquals("Default digest algorithm should be SHA256",
+                Constants.SHA256, settings.getDigestAlgorithm());
+        assertThat("Default digest algorithm should not be SHA-1",
+                settings.getDigestAlgorithm(), not(containsString("sha1")));
+    }
+
+    /**
+     * Tests that default clock drift is 120 seconds (2 minutes)
+     *
+     * @see org.codelibs.saml2.core.core.settings.Saml2Settings#getClockDrift
+     */
+    @Test
+    public void testDefaultClockDrift() {
+        Saml2Settings settings = new Saml2Settings();
+
+        assertEquals("Default clock drift should be 120 seconds",
+                120L, settings.getClockDrift());
+    }
+
+    /**
+     * Tests the getClockDrift and setClockDrift methods of the Saml2Settings
+     *
+     * @see org.codelibs.saml2.core.core.settings.Saml2Settings#getClockDrift
+     * @see org.codelibs.saml2.core.core.settings.Saml2Settings#setClockDrift
+     */
+    @Test
+    public void testClockDriftGetterSetter() {
+        Saml2Settings settings = new Saml2Settings();
+
+        // Test default value
+        assertEquals(120L, settings.getClockDrift());
+
+        // Test setting custom value
+        settings.setClockDrift(60L);
+        assertEquals(60L, settings.getClockDrift());
+
+        // Test setting to zero
+        settings.setClockDrift(0L);
+        assertEquals(0L, settings.getClockDrift());
+
+        // Test setting larger value
+        settings.setClockDrift(300L);
+        assertEquals(300L, settings.getClockDrift());
+    }
+
+    /**
+     * Tests that clock drift can be configured through SettingsBuilder
+     *
+     * @throws Exception
+     * @see org.codelibs.saml2.core.core.settings.Saml2Settings#getClockDrift
+     */
+    @Test
+    public void testClockDriftConfiguration() throws Exception {
+        Saml2Settings settings = new SettingsBuilder().fromFile("config/config.min.properties").build();
+
+        // Default should be used when not specified in config
+        assertEquals(120L, settings.getClockDrift());
+
+        // Test that it can be changed
+        settings.setClockDrift(180L);
+        assertEquals(180L, settings.getClockDrift());
+    }
 }