diff --git a/core/src/main/java/google/registry/config/RegistryConfig.java b/core/src/main/java/google/registry/config/RegistryConfig.java index 3807129e1..b9749f715 100644 --- a/core/src/main/java/google/registry/config/RegistryConfig.java +++ b/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 provideAllowedEcdsaCurves(RegistryConfigSettings config) { + return ImmutableSet.copyOf(config.sslCertificateValidation.allowedEcdsaCurves); + } } /** Returns the App Engine project ID, which is based off the environment name. */ diff --git a/core/src/main/java/google/registry/config/RegistryConfigSettings.java b/core/src/main/java/google/registry/config/RegistryConfigSettings.java index 88a38eb29..47db8bfb1 100644 --- a/core/src/main/java/google/registry/config/RegistryConfigSettings.java +++ b/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 maxValidityDaysSchedule; public int expirationWarningDays; public int minimumRsaKeyLength; + public Set allowedEcdsaCurves; } } diff --git a/core/src/main/java/google/registry/config/files/default-config.yaml b/core/src/main/java/google/registry/config/files/default-config.yaml index ed51c345a..ba8d0f951 100644 --- a/core/src/main/java/google/registry/config/files/default-config.yaml +++ b/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 diff --git a/core/src/main/java/google/registry/flows/certs/CertificateChecker.java b/core/src/main/java/google/registry/flows/certs/CertificateChecker.java index a9b5b72c9..e90145243 100644 --- a/core/src/main/java/google/registry/flows/certs/CertificateChecker.java +++ b/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 allowedEcdsaCurves; /** * Constructs a CertificateChecker instance with the specified configuration parameters. @@ -65,16 +70,18 @@ public class CertificateChecker { @Inject public CertificateChecker( @Config("maxValidityDaysSchedule") - ImmutableSortedMap maxValidityLengthSchedule, - @Config("expirationWarningDays") int daysToExpiration, + ImmutableSortedMap maxValidityDaysSchedule, + @Config("expirationWarningDays") int expirationWarningDays, @Config("minimumRsaKeyLength") int minimumRsaKeyLength, + @Config("allowedEcdsaCurves") ImmutableSet 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.daysToExpiration = daysToExpiration; + this.maxValidityLengthSchedule = maxValidityDaysSchedule; + 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 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. diff --git a/core/src/test/java/google/registry/flows/certs/CertificateCheckerTest.java b/core/src/test/java/google/registry/flows/certs/CertificateCheckerTest.java index 7e468f22c..f48b8a648 100644 --- a/core/src/test/java/google/registry/flows/certs/CertificateCheckerTest.java +++ b/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(); + } } diff --git a/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java b/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java index 0152140f0..34d415d55 100644 --- a/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java +++ b/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 ImmutableSortedMap.of(START_OF_TIME, 825, DateTime.parse("2020-09-01T00:00:00Z"), 398), 30, 2048, + ImmutableSet.of("secp256r1", "secp384r1"), fakeClock); } diff --git a/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java b/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java index 29934fcca..0e5c19e42 100644 --- a/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java +++ b/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java @@ -57,6 +57,7 @@ class UpdateRegistrarCommandTest extends CommandTestCase ImmutableSortedMap.of(START_OF_TIME, 825, DateTime.parse("2020-09-01T00:00:00Z"), 398), 30, 2048, + ImmutableSet.of("secp256r1", "secp384r1"), fakeClock); } diff --git a/core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java b/core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java index c0cc05796..404794a1e 100644 --- a/core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java +++ b/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");