Convert CertificateViolation into an enum (#829)

* Convert CertificateViolation into an enum

This ends up being nicer to deal with from callsites than class instances, while
still permitting full configurability of all parameters. There are various other
changes/fixes as well.
This commit is contained in:
Ben McIlwain 2020-10-07 22:19:36 -04:00 committed by GitHub
parent 151a2afb14
commit d052da4864
3 changed files with 248 additions and 156 deletions

View file

@ -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<DateTime, Integer> 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.
*
* <p>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.
*
* <p>The validity length schedule used by all major Web browsers as of 2020Q4 would be
* represented as:
*
* <pre>
* ImmutableSortedMap.of(
* START_OF_TIME, 825,
* DateTime.parse("2020-09-01T00:00:00Z"), 398
* );
* </pre>
*/
// TODO(sarahbot): Inject this.
public CertificateChecker(
ImmutableSortedMap<DateTime, Integer> 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<CertificateViolation> checkCertificate(
X509Certificate certificate, Date now) {
public ImmutableSet<CertificateViolation> checkCertificate(X509Certificate certificate) {
ImmutableSet.Builder<CertificateViolation> 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.
*
* <p>Note that this is <i>all</i> 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.
*
* <p>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);
}
}
}

View file

@ -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);

View file

@ -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<CertificateViolation> 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<CertificateViolation> 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<CertificateViolation> 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<CertificateViolation> 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<CertificateViolation> 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.");
}
}