Add ECDSA key validation to Certificate Checker (#855)

* Add ecdsa key validation

* Add some comments

* fix merge conflicts

* change variable names

* Separate tests

* separate curve tests

core/src/main/java/google/registry/config/RegistryConfig.java

@@ -1377,6 +1377,12 @@ public final class RegistryConfig {
     public static int provideMinimumRsaKeyLength(RegistryConfigSettings config) {
       return config.sslCertificateValidation.minimumRsaKeyLength;
     }
+
+    @Provides
+    @Config("allowedEcdsaCurves")
+    public static ImmutableSet<String> provideAllowedEcdsaCurves(RegistryConfigSettings config) {
+      return ImmutableSet.copyOf(config.sslCertificateValidation.allowedEcdsaCurves);
+    }
   }
 
   /** Returns the App Engine project ID, which is based off the environment name. */

core/src/main/java/google/registry/config/RegistryConfigSettings.java

@@ -16,6 +16,7 @@ package google.registry.config;
 
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 /** The POJO that YAML config files are deserialized into. */
 public class RegistryConfigSettings {
@@ -226,5 +227,6 @@ public class RegistryConfigSettings {
     public Map<String, Integer> maxValidityDaysSchedule;
     public int expirationWarningDays;
     public int minimumRsaKeyLength;
+    public Set<String> allowedEcdsaCurves;
   }
 }

core/src/main/java/google/registry/config/files/default-config.yaml

@@ -458,5 +458,9 @@ sslCertificateValidation:
   # The number of days before a certificate expires that indicates the
   # certificate is nearing expiration and warnings should be sent.
   expirationWarningDays: 30
-  # The minimum number of bits an RSA key must contain
+  # The minimum number of bits an RSA key must contain.
   minimumRsaKeyLength: 2048
+  # The ECDSA curves that are allowed for public keys.
+  allowedEcdsaCurves:
+    - secp256r1
+    - secp384r1

core/src/main/java/google/registry/flows/certs/CertificateChecker.java

@@ -28,10 +28,14 @@ import java.security.PublicKey;
 import java.security.cert.CertificateException;
 import java.security.cert.CertificateFactory;
 import java.security.cert.X509Certificate;
+import java.security.interfaces.ECPublicKey;
 import java.security.interfaces.RSAPublicKey;
 import java.util.Date;
 import java.util.stream.Collectors;
 import javax.inject.Inject;
+import org.bouncycastle.jcajce.provider.asymmetric.util.EC5Util;
+import org.bouncycastle.jce.ECNamedCurveTable;
+import org.bouncycastle.jce.spec.ECNamedCurveParameterSpec;
 import org.joda.time.DateTime;
 import org.joda.time.Days;
 
@@ -42,6 +46,7 @@ public class CertificateChecker {
   private final int daysToExpiration;
   private final int minimumRsaKeyLength;
   private final Clock clock;
+  private final ImmutableSet<String> allowedEcdsaCurves;
 
   /**
    * Constructs a CertificateChecker instance with the specified configuration parameters.
@@ -65,16 +70,18 @@ public class CertificateChecker {
   @Inject
   public CertificateChecker(
       @Config("maxValidityDaysSchedule")
-          ImmutableSortedMap<DateTime, Integer> maxValidityLengthSchedule,
+          ImmutableSortedMap<DateTime, Integer> maxValidityDaysSchedule,
-      @Config("expirationWarningDays") int daysToExpiration,
+      @Config("expirationWarningDays") int expirationWarningDays,
       @Config("minimumRsaKeyLength") int minimumRsaKeyLength,
+      @Config("allowedEcdsaCurves") ImmutableSet<String> allowedEcdsaCurves,
       Clock clock) {
     checkArgument(
-        maxValidityLengthSchedule.containsKey(START_OF_TIME),
+        maxValidityDaysSchedule.containsKey(START_OF_TIME),
         "Max validity length schedule must contain an entry for START_OF_TIME");
-    this.maxValidityLengthSchedule = maxValidityLengthSchedule;
+    this.maxValidityLengthSchedule = maxValidityDaysSchedule;
-    this.daysToExpiration = daysToExpiration;
+    this.daysToExpiration = expirationWarningDays;
     this.minimumRsaKeyLength = minimumRsaKeyLength;
+    this.allowedEcdsaCurves = allowedEcdsaCurves;
     this.clock = clock;
   }
 
@@ -123,7 +130,9 @@ public class CertificateChecker {
         violations.add(CertificateViolation.RSA_KEY_LENGTH_TOO_SHORT);
       }
     } else if (key.getAlgorithm().equals("EC")) {
-      // TODO(sarahbot): Add verification of ECDSA curves
+      if (!checkCurveName(key, allowedEcdsaCurves)) {
+        violations.add(CertificateViolation.INVALID_ECDSA_CURVE);
+      }
     } else {
       violations.add(CertificateViolation.ALGORITHM_CONSTRAINED);
     }
@@ -171,6 +180,33 @@ public class CertificateChecker {
     return Days.daysBetween(start.withTimeAtStartOfDay(), end.withTimeAtStartOfDay()).getDays();
   }
 
+  /** Checks if the curve used for a public key is in the list of acceptable curves. */
+  private static boolean checkCurveName(PublicKey key, ImmutableSet<String> allowedEcdsaCurves) {
+    org.bouncycastle.jce.spec.ECParameterSpec params;
+    // These 2 different instances of PublicKey need to be handled separately since their OIDs are
+    // encoded differently. More details on this can be found at
+    // https://stackoverflow.com/questions/49895713/how-to-find-the-matching-curve-name-from-an-ecpublickey.
+    if (key instanceof ECPublicKey) {
+      ECPublicKey ecKey = (ECPublicKey) key;
+      params = EC5Util.convertSpec(ecKey.getParams(), false);
+    } else if (key instanceof org.bouncycastle.jce.interfaces.ECPublicKey) {
+      org.bouncycastle.jce.interfaces.ECPublicKey ecKey =
+          (org.bouncycastle.jce.interfaces.ECPublicKey) key;
+      params = ecKey.getParameters();
+    } else {
+      throw new IllegalArgumentException("Unrecognized instance of PublicKey.");
+    }
+    return allowedEcdsaCurves.stream()
+        .anyMatch(
+            curve -> {
+              ECNamedCurveParameterSpec cParams = ECNamedCurveTable.getParameterSpec(curve);
+              return cParams.getN().equals(params.getN())
+                  && cParams.getH().equals(params.getH())
+                  && cParams.getCurve().equals(params.getCurve())
+                  && cParams.getG().equals(params.getG());
+            });
+  }
+
   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
@@ -190,6 +226,9 @@ public class CertificateChecker {
         return String.format(
             "Certificate validity period is too long; it must be less than or equal to %d days.",
             this.maxValidityLengthSchedule.lastEntry().getValue());
+      case INVALID_ECDSA_CURVE:
+        return String.format(
+            "The ECDSA key must use one of these algorithms: %s", allowedEcdsaCurves);
       default:
         throw new IllegalArgumentException(
             String.format(
@@ -206,7 +245,8 @@ public class CertificateChecker {
     NOT_YET_VALID,
     VALIDITY_LENGTH_TOO_LONG,
     RSA_KEY_LENGTH_TOO_SHORT,
-    ALGORITHM_CONSTRAINED;
+    ALGORITHM_CONSTRAINED,
+    INVALID_ECDSA_CURVE;
 
     /**
      * Gets a suitable end-user-facing display message for this particular certificate violation.

core/src/test/java/google/registry/flows/certs/CertificateCheckerTest.java

@@ -17,6 +17,7 @@ package google.registry.flows.certs;
 import static com.google.common.truth.Truth.assertThat;
 import static google.registry.flows.certs.CertificateChecker.CertificateViolation.ALGORITHM_CONSTRAINED;
 import static google.registry.flows.certs.CertificateChecker.CertificateViolation.EXPIRED;
+import static google.registry.flows.certs.CertificateChecker.CertificateViolation.INVALID_ECDSA_CURVE;
 import static google.registry.flows.certs.CertificateChecker.CertificateViolation.NOT_YET_VALID;
 import static google.registry.flows.certs.CertificateChecker.CertificateViolation.RSA_KEY_LENGTH_TOO_SHORT;
 import static google.registry.flows.certs.CertificateChecker.CertificateViolation.VALIDITY_LENGTH_TOO_LONG;
@@ -25,12 +26,16 @@ import static google.registry.testing.CertificateSamples.SAMPLE_CERT3;
 import static google.registry.util.DateTimeUtils.START_OF_TIME;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedMap;
 import google.registry.testing.FakeClock;
 import google.registry.util.SelfSignedCaCertificate;
+import java.security.AlgorithmParameters;
 import java.security.KeyPairGenerator;
 import java.security.SecureRandom;
 import java.security.cert.X509Certificate;
+import java.security.spec.ECGenParameterSpec;
+import java.security.spec.ECParameterSpec;
 import org.bouncycastle.jce.provider.BouncyCastleProvider;
 import org.joda.time.DateTime;
 import org.junit.jupiter.api.Test;
@@ -46,6 +51,7 @@ class CertificateCheckerTest {
           ImmutableSortedMap.of(START_OF_TIME, 825, DateTime.parse("2020-09-01T00:00:00Z"), 398),
           30,
           2048,
+          ImmutableSet.of("secp256r1", "secp384r1"),
           fakeClock);
 
   @Test
@@ -247,4 +253,62 @@ class CertificateCheckerTest {
         .isEqualTo(
             "Certificate validity period is too long; it must be less than or equal to 398 days.");
   }
+
+  @Test
+  void test_checkCurveName_invalidCurve_returnsViolation() throws Exception {
+    fakeClock.setTo(DateTime.parse("2020-10-01T00:00:00Z"));
+    // Invalid curve
+    KeyPairGenerator keyGen = KeyPairGenerator.getInstance("EC");
+    AlgorithmParameters apParam = AlgorithmParameters.getInstance("EC");
+    apParam.init(new ECGenParameterSpec("secp128r1"));
+    ECParameterSpec spec = apParam.getParameterSpec(ECParameterSpec.class);
+    keyGen.initialize(spec, 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(INVALID_ECDSA_CURVE);
+  }
+
+  @Test
+  void test_checkCurveName_p256Curve_returnsNoViolations() throws Exception {
+    fakeClock.setTo(DateTime.parse("2020-10-01T00:00:00Z"));
+    // valid P-256 curve
+    KeyPairGenerator keyGen = KeyPairGenerator.getInstance("EC");
+    AlgorithmParameters apParam = AlgorithmParameters.getInstance("EC");
+    apParam.init(new ECGenParameterSpec("secp256r1"));
+    ECParameterSpec spec = apParam.getParameterSpec(ECParameterSpec.class);
+    keyGen.initialize(spec, 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)).isEmpty();
+  }
+
+  @Test
+  void test_checkCurveName_p384Curve_returnsNoViolations() throws Exception {
+    fakeClock.setTo(DateTime.parse("2020-10-01T00:00:00Z"));
+    // valid P-384 curve
+    KeyPairGenerator keyGen = KeyPairGenerator.getInstance("EC");
+    AlgorithmParameters apParam = AlgorithmParameters.getInstance("EC");
+    apParam.init(new ECGenParameterSpec("secp384r1"));
+    ECParameterSpec spec = apParam.getParameterSpec(ECParameterSpec.class);
+    keyGen.initialize(spec, 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)).isEmpty();
+  }
 }

core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java

@@ -34,6 +34,7 @@ import static org.mockito.Mockito.when;
 
 import com.beust.jcommander.ParameterException;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.collect.Range;
 import com.google.common.net.MediaType;
@@ -61,6 +62,7 @@ class CreateRegistrarCommandTest extends CommandTestCase<CreateRegistrarCommand>
             ImmutableSortedMap.of(START_OF_TIME, 825, DateTime.parse("2020-09-01T00:00:00Z"), 398),
             30,
             2048,
+            ImmutableSet.of("secp256r1", "secp384r1"),
             fakeClock);
   }
 

core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java

@@ -57,6 +57,7 @@ class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarCommand>
             ImmutableSortedMap.of(START_OF_TIME, 825, DateTime.parse("2020-09-01T00:00:00Z"), 398),
             30,
             2048,
+            ImmutableSet.of("secp256r1", "secp384r1"),
             fakeClock);
   }
 

core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java

@@ -31,6 +31,7 @@ import static org.mockito.Mockito.when;
 import com.google.appengine.api.users.User;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSetMultimap;
 import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.truth.Truth;
@@ -123,6 +124,7 @@ public abstract class RegistrarSettingsActionTestCase {
             ImmutableSortedMap.of(START_OF_TIME, 825, DateTime.parse("2020-09-01T00:00:00Z"), 398),
             30,
             2048,
+            ImmutableSet.of("secp256r1", "secp384r1"),
             clock);
     inject.setStaticField(Ofy.class, "clock", clock);
     when(req.getMethod()).thenReturn("POST");