diff --git a/util/src/main/java/google/registry/util/CertificateChecker.java b/util/src/main/java/google/registry/util/CertificateChecker.java index 155d18e08..e94911069 100644 --- a/util/src/main/java/google/registry/util/CertificateChecker.java +++ b/util/src/main/java/google/registry/util/CertificateChecker.java @@ -14,8 +14,11 @@ package google.registry.util; -import com.google.auto.value.AutoValue; +import static com.google.common.base.Preconditions.checkArgument; +import static google.registry.util.DateTimeUtils.START_OF_TIME; + import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableSortedMap; import java.security.PublicKey; import java.security.cert.X509Certificate; import java.security.interfaces.RSAPublicKey; @@ -26,95 +29,96 @@ import org.joda.time.Days; /** An utility to check that a given certificate meets our requirements */ public class CertificateChecker { - private final int maxValidityDays; + private final ImmutableSortedMap maxValidityLengthSchedule; private final int daysToExpiration; private final int minimumRsaKeyLength; - public final CertificateViolation certificateExpiredViolation; - public final CertificateViolation certificateNotYetValidViolation; - public final CertificateViolation certificateValidityLengthViolation; - public final CertificateViolation certificateOldValidityLengthValidViolation; - public final CertificateViolation certificateRsaKeyLengthViolation; - public final CertificateViolation certificateAlgorithmViolation; + private final Clock clock; - public CertificateChecker(int maxValidityDays, int daysToExpiration, int minimumRsaKeyLength) { - this.maxValidityDays = maxValidityDays; + /** + * Constructs a CertificateChecker instance with the specified configuration parameters. + * + *

The max validity length schedule is a sorted map of {@link DateTime} to {@link Integer} + * entries representing a maximum validity period for certificates issued on or after that date. + * The first entry must have a key of {@link DateTimeUtils#START_OF_TIME}, such that every + * possible date has an applicable max validity period. Since security requirements tighten over + * time, the max validity periods will be decreasing as the date increases. + * + *

The validity length schedule used by all major Web browsers as of 2020Q4 would be + * represented as: + * + * 

+   *   ImmutableSortedMap.of(
+   *     START_OF_TIME, 825,
+   *     DateTime.parse("2020-09-01T00:00:00Z"), 398
+   *   );
+   *
+ */ + // TODO(sarahbot): Inject this. + public CertificateChecker( + ImmutableSortedMap maxValidityLengthSchedule, + int daysToExpiration, + int minimumRsaKeyLength, + Clock clock) { + checkArgument( + maxValidityLengthSchedule.containsKey(START_OF_TIME), + "Max validity length schedule must contain an entry for START_OF_TIME"); + this.maxValidityLengthSchedule = maxValidityLengthSchedule; this.daysToExpiration = daysToExpiration; this.minimumRsaKeyLength = minimumRsaKeyLength; - this.certificateExpiredViolation = - CertificateViolation.create("Expired Certificate", "This certificate is expired."); - this.certificateNotYetValidViolation = - CertificateViolation.create( - "Not Yet Valid", "This certificate's start date has not yet passed."); - this.certificateOldValidityLengthValidViolation = - CertificateViolation.create( - "Validity Period Too Long", - String.format( - "The certificate's validity length must be less than or equal to %d days, or %d" - + " days if issued prior to 2020-09-01.", - maxValidityDays, 825)); - this.certificateValidityLengthViolation = - CertificateViolation.create( - "Validity Period Too Long", - String.format( - "The certificate must have a validity length of less than %d days.", - maxValidityDays)); - this.certificateRsaKeyLengthViolation = - CertificateViolation.create( - "RSA Key Length Too Long", - String.format("The minimum RSA key length is %d.", minimumRsaKeyLength)); - this.certificateAlgorithmViolation = - CertificateViolation.create( - "Certificate Algorithm Not Allowed", "Only RSA and ECDSA keys are accepted."); + this.clock = clock; } /** * Checks a certificate for violations and returns a list of all the violations the certificate * has. */ - public ImmutableSet checkCertificate( - X509Certificate certificate, Date now) { + public ImmutableSet checkCertificate(X509Certificate certificate) { ImmutableSet.Builder violations = new ImmutableSet.Builder<>(); - // Check Expiration + // Check if currently in validity period + Date now = clock.nowUtc().toDate(); if (certificate.getNotAfter().before(now)) { - violations.add(certificateExpiredViolation); + violations.add(CertificateViolation.EXPIRED); } else if (certificate.getNotBefore().after(now)) { - violations.add(certificateNotYetValidViolation); - } - int validityLength = getValidityLengthInDays(certificate); - if (validityLength > maxValidityDays) { - if (new DateTime(certificate.getNotBefore()) - .isBefore(DateTime.parse("2020-09-01T00:00:00Z"))) { - if (validityLength > 825) { - violations.add(certificateOldValidityLengthValidViolation); - } - } else { - violations.add(certificateValidityLengthViolation); - } + violations.add(CertificateViolation.NOT_YET_VALID); } - // Check Key Strengths + // Check validity period length + int maxValidityDays = + maxValidityLengthSchedule.floorEntry(new DateTime(certificate.getNotBefore())).getValue(); + if (getValidityLengthInDays(certificate) > maxValidityDays) { + violations.add(CertificateViolation.VALIDITY_LENGTH_TOO_LONG); + } + + // Check key strengths PublicKey key = certificate.getPublicKey(); if (key.getAlgorithm().equals("RSA")) { RSAPublicKey rsaPublicKey = (RSAPublicKey) key; if (rsaPublicKey.getModulus().bitLength() < minimumRsaKeyLength) { - violations.add(certificateRsaKeyLengthViolation); + violations.add(CertificateViolation.RSA_KEY_LENGTH_TOO_SHORT); } } else if (key.getAlgorithm().equals("EC")) { // TODO(sarahbot): Add verification of ECDSA curves } else { - violations.add(certificateAlgorithmViolation); + violations.add(CertificateViolation.ALGORITHM_CONSTRAINED); } return violations.build(); } - /** Returns true if the certificate is nearing expiration. */ - public boolean isNearingExpiration(X509Certificate certificate, Date now) { + /** + * Returns whether the certificate is nearing expiration. + * + *

Note that this is all that it checks. The certificate itself may well be expired or + * not yet valid and this message will still return false. So you definitely want to pair a call + * to this method with a call to {@link #checkCertificate} to determine other issues with the + * certificate that may be occurring. + */ + public boolean isNearingExpiration(X509Certificate certificate) { Date nearingExpirationDate = DateTime.parse(certificate.getNotAfter().toInstant().toString()) .minusDays(daysToExpiration) .toDate(); - return now.after(nearingExpirationDate); + return clock.nowUtc().toDate().after(nearingExpirationDate); } private static int getValidityLengthInDays(X509Certificate certificate) { @@ -122,20 +126,52 @@ public class CertificateChecker { DateTime end = DateTime.parse(certificate.getNotAfter().toInstant().toString()); return Days.daysBetween(start.withTimeAtStartOfDay(), end.withTimeAtStartOfDay()).getDays(); } -} -/** - * The type of violation a certificate has based on the certificate requirements - * (go/registry-proxy-security). - */ -@AutoValue -abstract class CertificateViolation { + private String getViolationDisplayMessage(CertificateViolation certificateViolation) { + // Yes, we'd rather do this as an instance method on the CertificateViolation enum itself, but + // we can't because we need access to configuration (injected as instance variables) which you + // can't get in a static enum context. + switch (certificateViolation) { + case EXPIRED: + return "Certificate is expired."; + case NOT_YET_VALID: + return "Certificate start date is in the future."; + case ALGORITHM_CONSTRAINED: + return "Certificate key algorithm must be RSA or ECDSA."; + case RSA_KEY_LENGTH_TOO_SHORT: + return String.format( + "RSA key length is too short; the minimum allowed length is %d bits.", + this.minimumRsaKeyLength); + case VALIDITY_LENGTH_TOO_LONG: + return String.format( + "Certificate validity period is too long; it must be less than or equal to %d days.", + this.maxValidityLengthSchedule.lastEntry().getValue()); + default: + throw new IllegalArgumentException( + String.format( + "Unknown CertificateViolation enum value: %s", certificateViolation.name())); + } + } - public abstract String name(); + /** + * The type of violation a certificate has based on the certificate requirements + * (go/registry-proxy-security). + */ + public enum CertificateViolation { + EXPIRED, + NOT_YET_VALID, + VALIDITY_LENGTH_TOO_LONG, + RSA_KEY_LENGTH_TOO_SHORT, + ALGORITHM_CONSTRAINED; - public abstract String displayMessage(); - - public static CertificateViolation create(String name, String displayMessage) { - return new AutoValue_CertificateViolation(name, displayMessage); + /** + * Gets a suitable end-user-facing display message for this particular certificate violation. + * + *

Note that the {@link CertificateChecker} instance must be passed in because it contains + * configuration values (e.g. minimum RSA key length) that go into the error message text. + */ + public String getDisplayMessage(CertificateChecker certificateChecker) { + return certificateChecker.getViolationDisplayMessage(this); + } } } diff --git a/util/src/main/java/google/registry/util/SelfSignedCaCertificate.java b/util/src/main/java/google/registry/util/SelfSignedCaCertificate.java index cabf67338..cc0d630ab 100644 --- a/util/src/main/java/google/registry/util/SelfSignedCaCertificate.java +++ b/util/src/main/java/google/registry/util/SelfSignedCaCertificate.java @@ -37,6 +37,7 @@ import org.bouncycastle.cert.jcajce.JcaX509v3CertificateBuilder; import org.bouncycastle.jce.provider.BouncyCastleProvider; import org.bouncycastle.operator.ContentSigner; import org.bouncycastle.operator.jcajce.JcaContentSignerBuilder; +import org.joda.time.DateTime; /** A self-signed certificate authority (CA) cert for use in tests. */ // TODO(weiminyu): make this class test-only. Requires refactor in proxy and prober. @@ -83,11 +84,22 @@ public class SelfSignedCaCertificate { return create(keyGen.generateKeyPair(), fqdn, from, to); } + public static SelfSignedCaCertificate create(String fqdn, DateTime from, DateTime to) + throws Exception { + return create(keyGen.generateKeyPair(), fqdn, from.toDate(), to.toDate()); + } + public static SelfSignedCaCertificate create(KeyPair keyPair, String fqdn, Date from, Date to) throws Exception { return new SelfSignedCaCertificate(keyPair.getPrivate(), createCaCert(keyPair, fqdn, from, to)); } + public static SelfSignedCaCertificate create( + KeyPair keyPair, String fqdn, DateTime from, DateTime to) throws Exception { + return new SelfSignedCaCertificate( + keyPair.getPrivate(), createCaCert(keyPair, fqdn, from.toDate(), to.toDate())); + } + static KeyPairGenerator createKeyPairGenerator() { try { KeyPairGenerator keyGen = KeyPairGenerator.getInstance("RSA", PROVIDER); diff --git a/util/src/test/java/google/registry/util/CertificateCheckerTest.java b/util/src/test/java/google/registry/util/CertificateCheckerTest.java index 7d1f060cf..4704e7bb2 100644 --- a/util/src/test/java/google/registry/util/CertificateCheckerTest.java +++ b/util/src/test/java/google/registry/util/CertificateCheckerTest.java @@ -15,9 +15,15 @@ package google.registry.util; import static com.google.common.truth.Truth.assertThat; -import static org.joda.time.DateTimeZone.UTC; +import static google.registry.util.CertificateChecker.CertificateViolation.ALGORITHM_CONSTRAINED; +import static google.registry.util.CertificateChecker.CertificateViolation.EXPIRED; +import static google.registry.util.CertificateChecker.CertificateViolation.NOT_YET_VALID; +import static google.registry.util.CertificateChecker.CertificateViolation.RSA_KEY_LENGTH_TOO_SHORT; +import static google.registry.util.CertificateChecker.CertificateViolation.VALIDITY_LENGTH_TOO_LONG; +import static google.registry.util.DateTimeUtils.START_OF_TIME; -import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableSortedMap; +import google.registry.testing.FakeClock; import java.security.KeyPairGenerator; import java.security.SecureRandom; import java.security.cert.X509Certificate; @@ -26,26 +32,33 @@ import org.joda.time.DateTime; import org.junit.jupiter.api.Test; /** Unit tests for {@link CertificateChecker} */ -public class CertificateCheckerTest { +class CertificateCheckerTest { private static final String SSL_HOST = "www.example.tld"; - private static CertificateChecker certificateChecker = new CertificateChecker(398, 30, 2048); + private FakeClock fakeClock = new FakeClock(); + private CertificateChecker certificateChecker = + new CertificateChecker( + ImmutableSortedMap.of(START_OF_TIME, 825, DateTime.parse("2020-09-01T00:00:00Z"), 398), + 30, + 2048, + fakeClock); @Test - void test_compliantCertificate() throws Exception { + void test_checkCertificate_compliantCertPasses() throws Exception { + fakeClock.setTo(DateTime.parse("2020-10-01T00:00:00Z")); X509Certificate certificate = SelfSignedCaCertificate.create( SSL_HOST, - DateTime.now(UTC).minusDays(5).toDate(), - DateTime.now(UTC).plusDays(80).toDate()) + DateTime.parse("2020-09-02T00:00:00Z"), + DateTime.parse("2021-10-01T00:00:00Z")) .cert(); - assertThat(certificateChecker.checkCertificate(certificate, DateTime.now(UTC).toDate())) - .isEqualTo(ImmutableSet.of()); + assertThat(certificateChecker.checkCertificate(certificate)).isEmpty(); } @Test - void test_certificateWithSeveralIssues() throws Exception { + void test_checkCertificate_severalViolations() throws Exception { + fakeClock.setTo(DateTime.parse("2010-01-01T00:00:00Z")); KeyPairGenerator keyGen = KeyPairGenerator.getInstance("RSA", new BouncyCastleProvider()); keyGen.initialize(1024, new SecureRandom()); @@ -53,104 +66,79 @@ public class CertificateCheckerTest { SelfSignedCaCertificate.create( keyGen.generateKeyPair(), SSL_HOST, - DateTime.now(UTC).plusDays(5).toDate(), - DateTime.now(UTC).plusDays(1000).toDate()) + DateTime.parse("2010-04-01T00:00:00Z"), + DateTime.parse("2014-07-01T00:00:00Z")) .cert(); - ImmutableSet violations = - certificateChecker.checkCertificate(certificate, DateTime.now(UTC).toDate()); - assertThat(violations).hasSize(3); - assertThat(violations) - .isEqualTo( - ImmutableSet.of( - certificateChecker.certificateNotYetValidViolation, - certificateChecker.certificateValidityLengthViolation, - certificateChecker.certificateRsaKeyLengthViolation)); - ; + assertThat(certificateChecker.checkCertificate(certificate)) + .containsExactly(NOT_YET_VALID, VALIDITY_LENGTH_TOO_LONG, RSA_KEY_LENGTH_TOO_SHORT); } @Test - void test_expiredCertificate() throws Exception { + void test_checkCertificate_expiredCertificate() throws Exception { + fakeClock.setTo(DateTime.parse("2014-01-01T00:00:00Z")); X509Certificate certificate = SelfSignedCaCertificate.create( SSL_HOST, - DateTime.now(UTC).minusDays(50).toDate(), - DateTime.now(UTC).minusDays(10).toDate()) + DateTime.parse("2010-04-01T00:00:00Z"), + DateTime.parse("2012-07-01T00:00:00Z")) .cert(); - ImmutableSet violations = - certificateChecker.checkCertificate(certificate, DateTime.now(UTC).toDate()); - assertThat(violations).containsExactly(certificateChecker.certificateExpiredViolation); + assertThat(certificateChecker.checkCertificate(certificate)).containsExactly(EXPIRED); } @Test - void test_notYetValid() throws Exception { + void test_checkCertificate_notYetValid() throws Exception { + fakeClock.setTo(DateTime.parse("2010-01-01T00:00:00Z")); X509Certificate certificate = SelfSignedCaCertificate.create( SSL_HOST, - DateTime.now(UTC).plusDays(10).toDate(), - DateTime.now(UTC).plusDays(50).toDate()) + DateTime.parse("2010-04-01T00:00:00Z"), + DateTime.parse("2012-07-01T00:00:00Z")) .cert(); - ImmutableSet violations = - certificateChecker.checkCertificate(certificate, DateTime.now(UTC).toDate()); - assertThat(violations).containsExactly(certificateChecker.certificateNotYetValidViolation); + assertThat(certificateChecker.checkCertificate(certificate)).containsExactly(NOT_YET_VALID); } @Test - void test_checkValidityLength() throws Exception { + void test_checkCertificate_validityLengthWayTooLong() throws Exception { + fakeClock.setTo(DateTime.parse("2020-10-01T00:00:00Z")); X509Certificate certificate = SelfSignedCaCertificate.create( SSL_HOST, - DateTime.now(UTC).minusDays(10).toDate(), - DateTime.now(UTC).plusDays(1000).toDate()) + DateTime.parse("2016-04-01T00:00:00Z"), + DateTime.parse("2021-07-01T00:00:00Z")) .cert(); - ImmutableSet violations = - certificateChecker.checkCertificate(certificate, DateTime.now(UTC).toDate()); - assertThat(violations).containsExactly(certificateChecker.certificateValidityLengthViolation); - - certificate = - SelfSignedCaCertificate.create( - SSL_HOST, - DateTime.parse("2020-08-01T00:00:00Z").toDate(), - DateTime.parse("2023-11-01T00:00:00Z").toDate()) - .cert(); - violations = certificateChecker.checkCertificate(certificate, DateTime.now(UTC).toDate()); - assertThat(violations) - .containsExactly(certificateChecker.certificateOldValidityLengthValidViolation); - - certificate = - SelfSignedCaCertificate.create( - SSL_HOST, - DateTime.parse("2020-08-01T00:00:00Z").toDate(), - DateTime.parse("2021-11-01T00:00:00Z").toDate()) - .cert(); - violations = certificateChecker.checkCertificate(certificate, DateTime.now(UTC).toDate()); - assertThat(violations).isEmpty(); + assertThat(certificateChecker.checkCertificate(certificate)) + .containsExactly(VALIDITY_LENGTH_TOO_LONG); } @Test - void test_nearingExpiration() throws Exception { + void test_checkCertificate_olderValidityLengthIssuedAfterCutoff() throws Exception { + fakeClock.setTo(DateTime.parse("2020-10-01T00:00:00Z")); X509Certificate certificate = SelfSignedCaCertificate.create( SSL_HOST, - DateTime.now(UTC).minusDays(50).toDate(), - DateTime.now(UTC).plusDays(10).toDate()) + DateTime.parse("2020-09-02T00:00:00Z"), + DateTime.parse("2022-10-01T00:00:00Z")) .cert(); - assertThat(certificateChecker.isNearingExpiration(certificate, DateTime.now(UTC).toDate())) - .isTrue(); - - certificate = - SelfSignedCaCertificate.create( - SSL_HOST, - DateTime.now(UTC).minusDays(50).toDate(), - DateTime.now(UTC).plusDays(100).toDate()) - .cert(); - assertThat(certificateChecker.isNearingExpiration(certificate, DateTime.now(UTC).toDate())) - .isFalse(); + assertThat(certificateChecker.checkCertificate(certificate)) + .containsExactly(VALIDITY_LENGTH_TOO_LONG); } @Test - void test_checkRsaKeyLength() throws Exception { - // Key length too low + void test_checkCertificate_olderValidityLengthIssuedBeforeCutoff() throws Exception { + fakeClock.setTo(DateTime.parse("2020-10-01T00:00:00Z")); + X509Certificate certificate = + SelfSignedCaCertificate.create( + SSL_HOST, + DateTime.parse("2019-09-01T00:00:00Z"), + DateTime.parse("2021-10-01T00:00:00Z")) + .cert(); + assertThat(certificateChecker.checkCertificate(certificate)).isEmpty(); + } + + @Test + void test_checkCertificate_rsaKeyLengthTooShort() throws Exception { + fakeClock.setTo(DateTime.parse("2020-10-01T00:00:00Z")); KeyPairGenerator keyGen = KeyPairGenerator.getInstance("RSA", new BouncyCastleProvider()); keyGen.initialize(1024, new SecureRandom()); @@ -158,27 +146,83 @@ public class CertificateCheckerTest { SelfSignedCaCertificate.create( keyGen.generateKeyPair(), SSL_HOST, - DateTime.now(UTC).minusDays(5).toDate(), - DateTime.now(UTC).plusDays(100).toDate()) + DateTime.parse("2020-09-02T00:00:00Z"), + DateTime.parse("2021-10-01T00:00:00Z")) .cert(); - ImmutableSet violations = - certificateChecker.checkCertificate(certificate, DateTime.now(UTC).toDate()); - assertThat(violations).containsExactly(certificateChecker.certificateRsaKeyLengthViolation); + assertThat(certificateChecker.checkCertificate(certificate)) + .containsExactly(RSA_KEY_LENGTH_TOO_SHORT); + } - // Key length higher than required - keyGen = KeyPairGenerator.getInstance("RSA", new BouncyCastleProvider()); + @Test + void test_checkCertificate_rsaKeyLengthLongerThanRequired() throws Exception { + fakeClock.setTo(DateTime.parse("2020-10-01T00:00:00Z")); + KeyPairGenerator keyGen = KeyPairGenerator.getInstance("RSA", new BouncyCastleProvider()); keyGen.initialize(4096, new SecureRandom()); - certificate = + X509Certificate certificate = SelfSignedCaCertificate.create( keyGen.generateKeyPair(), SSL_HOST, - DateTime.now(UTC).minusDays(5).toDate(), - DateTime.now(UTC).plusDays(100).toDate()) + DateTime.parse("2020-09-02T00:00:00Z"), + DateTime.parse("2021-10-01T00:00:00Z")) .cert(); - assertThat(certificateChecker.checkCertificate(certificate, DateTime.now(UTC).toDate())) - .isEqualTo(ImmutableSet.of()); + assertThat(certificateChecker.checkCertificate(certificate)).isEmpty(); + } + + @Test + void test_checkCertificate_invalidAlgorithm() throws Exception { + fakeClock.setTo(DateTime.parse("2020-10-01T00:00:00Z")); + KeyPairGenerator keyGen = KeyPairGenerator.getInstance("DSA", new BouncyCastleProvider()); + keyGen.initialize(2048, new SecureRandom()); + + X509Certificate certificate = + SelfSignedCaCertificate.create( + keyGen.generateKeyPair(), + SSL_HOST, + DateTime.parse("2020-09-02T00:00:00Z"), + DateTime.parse("2021-10-01T00:00:00Z")) + .cert(); + + assertThat(certificateChecker.checkCertificate(certificate)) + .containsExactly(ALGORITHM_CONSTRAINED); + } + + @Test + void test_isNearingExpiration_yesItIs() throws Exception { + fakeClock.setTo(DateTime.parse("2021-09-20T00:00:00Z")); + X509Certificate certificate = + SelfSignedCaCertificate.create( + SSL_HOST, + DateTime.parse("2020-09-02T00:00:00Z"), + DateTime.parse("2021-10-01T00:00:00Z")) + .cert(); + assertThat(certificateChecker.isNearingExpiration(certificate)).isTrue(); + } + + @Test + void test_isNearingExpiration_noItsNot() throws Exception { + fakeClock.setTo(DateTime.parse("2021-07-20T00:00:00Z")); + X509Certificate certificate = + SelfSignedCaCertificate.create( + SSL_HOST, + DateTime.parse("2020-09-02T00:00:00Z"), + DateTime.parse("2021-10-01T00:00:00Z")) + .cert(); + assertThat(certificateChecker.isNearingExpiration(certificate)).isFalse(); + } + + @Test + void test_CertificateViolation_RsaKeyLengthDisplayMessageFormatsCorrectly() { + assertThat(RSA_KEY_LENGTH_TOO_SHORT.getDisplayMessage(certificateChecker)) + .isEqualTo("RSA key length is too short; the minimum allowed length is 2048 bits."); + } + + @Test + void test_CertificateViolation_validityLengthDisplayMessageFormatsCorrectly() { + assertThat(VALIDITY_LENGTH_TOO_LONG.getDisplayMessage(certificateChecker)) + .isEqualTo( + "Certificate validity period is too long; it must be less than or equal to 398 days."); } }