zydronium/google-nomulus
1
0
Fork 0
mirror of https://github.com/google/nomulus.git synced 2025-07-09 04:33:28 +02:00
Code Issues Projects Releases Packages Wiki Activity

Validate Certificate on Login (#919)

Browse source 
* Check certificate matches saved one on login

* Add tests

* refactoring

* fix warning messages
This commit is contained in:
sarahcaseybot 2021-01-19 17:06:26 -05:00 • committed by GitHub
parent 9b5805f145
commit 3b679058b0
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 148 additions and 40 deletions
Download patch file Download diff file Expand all files Collapse all files

60
core/src/main/java/google/registry/flows/TlsCredentials.java
View file

@ -42,6 +42,9 @@ import javax.servlet.http.HttpServletRequest;
* <dl>
* <dt>X-SSL-Certificate
* <dd>This field should contain a base64 encoded digest of the client's TLS certificate. It is
* used only if the validation of the full certificate fails.
* <dt>X-SSL-Full-Certificate
* <dd>This field should contain a base64 encoding of the client's TLS certificate. It is
* validated during an EPP login command against a known good value that is transmitted out of
* band.
* <dt>X-Forwarded-For
@ -55,15 +58,18 @@ public class TlsCredentials implements TransportCredentials {
private final boolean requireSslCertificates;
private final Optional<String> clientCertificateHash;
private final Optional<String> clientCertificate;
private final Optional<InetAddress> clientInetAddr;
@Inject
public TlsCredentials(
@Config("requireSslCertificates") boolean requireSslCertificates,
@Header("X-SSL-Certificate") Optional<String> clientCertificateHash,
@Header("X-SSL-Full-Certificate") Optional<String> clientCertificate,
@Header("X-Forwarded-For") Optional<String> clientAddress) {
this.requireSslCertificates = requireSslCertificates;
this.clientCertificateHash = clientCertificateHash;
this.clientCertificate = clientCertificate;
this.clientInetAddr = clientAddress.map(TlsCredentials::parseInetAddress);
}
@ -91,7 +97,7 @@ public class TlsCredentials implements TransportCredentials {
ImmutableList<CidrAddressBlock> ipAddressAllowList = registrar.getIpAddressAllowList();
if (ipAddressAllowList.isEmpty()) {
logger.atInfo().log(
"Skipping IP allow list check because %s doesn't have an IP allow list",
"Skipping IP allow list check because %s doesn't have an IP allow list.",
registrar.getClientId());
return;
}
@ -115,11 +121,46 @@ public class TlsCredentials implements TransportCredentials {
/**
* Verifies client SSL certificate is permitted to issue commands as {@code registrar}.
*
* @throws MissingRegistrarCertificateException if frontend didn't send certificate hash header
* @throws MissingRegistrarCertificateException if frontend didn't send certificate header
* @throws BadRegistrarCertificateException if registrar requires certificate and it didn't match
*/
@VisibleForTesting
void validateCertificate(Registrar registrar) throws AuthenticationErrorException {
// Check that certificate is present in registrar object
if (!registrar.getClientCertificate().isPresent()
&& !registrar.getFailoverClientCertificate().isPresent()) {
// Log an error and validate using certificate hash instead
// TODO(sarahbot): throw a RegistrarCertificateNotConfiguredException once hash is no longer
// used as failover
logger.atWarning().log(
"There is no certificate configured for registrar %s.", registrar.getClientId());
}
// Check that the request included the full certificate
else if (!clientCertificate.isPresent()) {
// Log an error and validate using certificate hash instead
// TODO(sarahbot): throw a MissingRegistrarCertificateException once hash is no longer used as
// failover
logger.atWarning().log(
"Request from registrar %s did not include X-SSL-Full-Certificate.",
registrar.getClientId());
} else {
// Check if the certificate is equal to the one on file for the registrar.
if (clientCertificate.equals(registrar.getClientCertificate())
|| clientCertificate.equals(registrar.getFailoverClientCertificate())) {
// successfully validated, return here since hash validation is not necessary
return;
}
// Log an error and validate using certificate hash instead
// TODO(sarahbot): throw a BadRegistrarCertificateException once hash is no longer used as
// failover
logger.atWarning().log("Non-matching certificate for registrar %s.", registrar.getClientId());
}
validateCertificateHash(registrar);
}
private void validateCertificateHash(Registrar registrar) throws AuthenticationErrorException {
// Check the certificate hash as a failover
// TODO(sarahbot): Remove hash checks once certificate checks are working.
if (!registrar.getClientCertificateHash().isPresent()
&& !registrar.getFailoverClientCertificateHash().isPresent()) {
if (requireSslCertificates) {
@ -130,14 +171,17 @@ public class TlsCredentials implements TransportCredentials {
return;
}
}
// Check that the request included the certificate hash
if (!clientCertificateHash.isPresent()) {
logger.atInfo().log("Request did not include X-SSL-Certificate");
logger.atInfo().log(
"Request from registrar %s did not include X-SSL-Certificate.", registrar.getClientId());
throw new MissingRegistrarCertificateException();
}
// Check if the certificate hash is equal to the one on file for the registrar.
if (!clientCertificateHash.equals(registrar.getClientCertificateHash())
&& !clientCertificateHash.equals(registrar.getFailoverClientCertificateHash())) {
logger.atWarning().log(
"bad certificate hash (%s) for %s, wanted either %s or %s",
"Non-matching certificate hash (%s) for %s, wanted either %s or %s.",
clientCertificateHash,
registrar.getClientId(),
registrar.getClientCertificateHash(),
@ -202,6 +246,14 @@ public class TlsCredentials implements TransportCredentials {
return extractOptionalHeader(req, "X-SSL-Certificate");
}
@Provides
@Header("X-SSL-Full-Certificate")
static Optional<String> provideClientCertificate(HttpServletRequest req) {
// Note: This header is actually required, we just want to handle its absence explicitly
// by throwing an EPP exception rather than a generic Bad Request exception.
return extractOptionalHeader(req, "X-SSL-Full_Certificate");
}
@Provides
@Header("X-Forwarded-For")
static Optional<String> provideForwardedFor(HttpServletRequest req) {

11
core/src/main/java/google/registry/tools/ValidateLoginCredentialsCommand.java
View file

@ -55,6 +55,7 @@ final class ValidateLoginCredentialsCommand implements CommandWithRemoteApi {
validateWith = PathParameter.InputFile.class)
private Path clientCertificatePath;
// TODO(sarahbot@): Remove this after hash fallback is removed
@Nullable
@Parameter(
names = {"-h", "--cert_hash"},
@ -72,15 +73,19 @@ final class ValidateLoginCredentialsCommand implements CommandWithRemoteApi {
checkArgument(
clientCertificatePath == null || isNullOrEmpty(clientCertificateHash),
"Can't specify both --cert_hash and --cert_file");
String clientCertificate = "";
if (clientCertificatePath != null) {
clientCertificateHash = getCertificateHash(
loadCertificate(new String(Files.readAllBytes(clientCertificatePath), US_ASCII)));
clientCertificate = new String(Files.readAllBytes(clientCertificatePath), US_ASCII);
clientCertificateHash = getCertificateHash(loadCertificate(clientCertificate));
}
Registrar registrar =
checkArgumentPresent(
Registrar.loadByClientId(clientId), "Registrar %s not found", clientId);
new TlsCredentials(
true, Optional.ofNullable(clientCertificateHash), Optional.ofNullable(clientIpAddress))
true,
Optional.ofNullable(clientCertificateHash),
Optional.ofNullable(clientCertificate),
Optional.ofNullable(clientIpAddress))
.validate(registrar, password);
checkState(
registrar.isLive(), "Registrar %s has non-live state: %s", clientId, registrar.getState());

27
core/src/test/java/google/registry/flows/EppLoginTlsTest.java
View file

@ -34,10 +34,13 @@ class EppLoginTlsTest extends EppTestCase {
final AppEngineExtension appEngine =
AppEngineExtension.builder().withDatastoreAndCloudSql().build();
void setClientCertificateHash(String clientCertificateHash) {
void setCredentials(String clientCertificateHash, String clientCertificate) {
setTransportCredentials(
new TlsCredentials(
true, Optional.ofNullable(clientCertificateHash), Optional.of("192.168.1.100:54321")));
true,
Optional.ofNullable(clientCertificateHash),
Optional.ofNullable(clientCertificate),
Optional.of("192.168.1.100:54321")));
}
@BeforeEach
@ -57,14 +60,14 @@ class EppLoginTlsTest extends EppTestCase {
@Test
void testLoginLogout() throws Exception {
setClientCertificateHash(CertificateSamples.SAMPLE_CERT_HASH);
setCredentials(CertificateSamples.SAMPLE_CERT_HASH, CertificateSamples.SAMPLE_CERT);
assertThatLoginSucceeds("NewRegistrar", "foo-BAR2");
assertThatLogoutSucceeds();
}
@Test
void testLogin_wrongPasswordFails() throws Exception {
setClientCertificateHash(CertificateSamples.SAMPLE_CERT_HASH);
setCredentials(CertificateSamples.SAMPLE_CERT_HASH, CertificateSamples.SAMPLE_CERT);
// For TLS login, we also check the epp xml password.
assertThatLogin("NewRegistrar", "incorrect")
.hasResponse(
@ -74,7 +77,7 @@ class EppLoginTlsTest extends EppTestCase {
@Test
void testMultiLogin() throws Exception {
setClientCertificateHash(CertificateSamples.SAMPLE_CERT_HASH);
setCredentials(CertificateSamples.SAMPLE_CERT_HASH, CertificateSamples.SAMPLE_CERT);
assertThatLoginSucceeds("NewRegistrar", "foo-BAR2");
assertThatLogoutSucceeds();
assertThatLoginSucceeds("NewRegistrar", "foo-BAR2");
@ -88,7 +91,7 @@ class EppLoginTlsTest extends EppTestCase {
@Test
void testNonAuthedLogin_fails() throws Exception {
setClientCertificateHash(CertificateSamples.SAMPLE_CERT_HASH);
setCredentials(CertificateSamples.SAMPLE_CERT_HASH, CertificateSamples.SAMPLE_CERT);
assertThatLogin("TheRegistrar", "password2")
.hasResponse(
"response_error.xml",
@ -98,7 +101,7 @@ class EppLoginTlsTest extends EppTestCase {
@Test
void testBadCertificate_failsBadCertificate2200() throws Exception {
setClientCertificateHash("laffo");
setCredentials("laffo", "cert");
assertThatLogin("NewRegistrar", "foo-BAR2")
.hasResponse(
"response_error.xml",
@ -108,7 +111,7 @@ class EppLoginTlsTest extends EppTestCase {
@Test
void testGfeDidntProvideClientCertificate_failsMissingCertificate2200() throws Exception {
setClientCertificateHash(null);
setCredentials(null, null);
assertThatLogin("NewRegistrar", "foo-BAR2")
.hasResponse(
"response_error.xml",
@ -117,7 +120,7 @@ class EppLoginTlsTest extends EppTestCase {
@Test
void testGoodPrimaryCertificate() throws Exception {
setClientCertificateHash(CertificateSamples.SAMPLE_CERT_HASH);
setCredentials(CertificateSamples.SAMPLE_CERT_HASH, CertificateSamples.SAMPLE_CERT);
DateTime now = DateTime.now(UTC);
persistResource(
loadRegistrar("NewRegistrar")
@ -130,7 +133,7 @@ class EppLoginTlsTest extends EppTestCase {
@Test
void testGoodFailoverCertificate() throws Exception {
setClientCertificateHash(CertificateSamples.SAMPLE_CERT2_HASH);
setCredentials(CertificateSamples.SAMPLE_CERT2_HASH, CertificateSamples.SAMPLE_CERT2);
DateTime now = DateTime.now(UTC);
persistResource(
loadRegistrar("NewRegistrar")
@ -143,7 +146,7 @@ class EppLoginTlsTest extends EppTestCase {
@Test
void testMissingPrimaryCertificateButHasFailover_usesFailover() throws Exception {
setClientCertificateHash(CertificateSamples.SAMPLE_CERT2_HASH);
setCredentials(CertificateSamples.SAMPLE_CERT2_HASH, CertificateSamples.SAMPLE_CERT2);
DateTime now = DateTime.now(UTC);
persistResource(
loadRegistrar("NewRegistrar")
@ -156,7 +159,7 @@ class EppLoginTlsTest extends EppTestCase {
@Test
void testRegistrarHasNoCertificatesOnFile_fails() throws Exception {
setClientCertificateHash("laffo");
setCredentials("laffo", "cert");
DateTime now = DateTime.now(UTC);
persistResource(
loadRegistrar("NewRegistrar")

3
core/src/test/java/google/registry/flows/FlowRunnerTest.java
View file

@ -138,7 +138,8 @@ class FlowRunnerTest {
@Test
void testRun_loggingStatement_tlsCredentials() throws Exception {
flowRunner.credentials =
new TlsCredentials(true, Optional.of("abc123def"), Optional.of("127.0.0.1"));
new TlsCredentials(
true, Optional.of("abc123def"), Optional.of("cert"), Optional.of("127.0.0.1"));
flowRunner.run(eppMetricBuilder);
assertThat(Splitter.on("\n\t").split(findFirstLogMessageByPrefix(handler, "EPP Command\n\t")))
.contains("TlsCredentials{clientCertificateHash=abc123def, clientAddress=/127.0.0.1}");

45
core/src/test/java/google/registry/flows/TlsCredentialsTest.java
View file

@ -26,6 +26,7 @@ import static org.mockito.Mockito.when;
import com.google.common.collect.ImmutableSet;
import google.registry.flows.TlsCredentials.BadRegistrarIpAddressException;
import google.registry.flows.TlsCredentials.MissingRegistrarCertificateException;
import google.registry.flows.TlsCredentials.RegistrarCertificateNotConfiguredException;
import google.registry.model.registrar.Registrar;
import google.registry.testing.AppEngineExtension;
import google.registry.util.CidrAddressBlock;
@ -50,8 +51,9 @@ final class TlsCredentialsTest {
}
@Test
void testClientCertificateHash_missing() {
TlsCredentials tls = new TlsCredentials(true, Optional.empty(), Optional.of("192.168.1.1"));
void testClientCertificateAndHash_missing() {
TlsCredentials tls =
new TlsCredentials(true, Optional.empty(), Optional.empty(), Optional.of("192.168.1.1"));
persistResource(
loadRegistrar("TheRegistrar")
.asBuilder()
@ -64,7 +66,8 @@ final class TlsCredentialsTest {
@Test
void test_missingIpAddress_doesntAllowAccess() {
TlsCredentials tls = new TlsCredentials(false, Optional.of("certHash"), Optional.empty());
TlsCredentials tls =
new TlsCredentials(false, Optional.of("certHash"), Optional.empty(), Optional.empty());
persistResource(
loadRegistrar("TheRegistrar")
.asBuilder()
@ -79,7 +82,41 @@ final class TlsCredentialsTest {
@Test
void test_validateCertificate_canBeConfiguredToBypassCertHashes() throws Exception {
TlsCredentials tls =
new TlsCredentials(false, Optional.of("certHash"), Optional.of("192.168.1.1"));
new TlsCredentials(
false, Optional.of("certHash"), Optional.of("cert"), Optional.of("192.168.1.1"));
persistResource(
loadRegistrar("TheRegistrar")
.asBuilder()
.setClientCertificate(null, DateTime.now(UTC))
.setFailoverClientCertificate(null, DateTime.now(UTC))
.build());
// This would throw a RegistrarCertificateNotConfiguredException if cert hashes were not
// bypassed
tls.validateCertificate(Registrar.loadByClientId("TheRegistrar").get());
}
void testProvideClientCertificate() {
HttpServletRequest req = mock(HttpServletRequest.class);
when(req.getHeader("X-SSL-Full-Certificate")).thenReturn("data");
assertThat(TlsCredentials.EppTlsModule.provideClientCertificate(req)).isEqualTo("data");
}
@Test
void testClientCertificate_notConfigured() {
TlsCredentials tls =
new TlsCredentials(
true, Optional.of("hash"), Optional.of(SAMPLE_CERT), Optional.of("192.168.1.1"));
persistResource(loadRegistrar("TheRegistrar").asBuilder().build());
assertThrows(
RegistrarCertificateNotConfiguredException.class,
() -> tls.validateCertificate(Registrar.loadByClientId("TheRegistrar").get()));
}
@Test
void test_validateCertificate_canBeConfiguredToBypassCerts() throws Exception {
TlsCredentials tls =
new TlsCredentials(
false, Optional.of("certHash"), Optional.of("cert"), Optional.of("192.168.1.1"));
persistResource(
loadRegistrar("TheRegistrar")
.asBuilder()

34
core/src/test/java/google/registry/flows/session/LoginFlowViaTlsTest.java
View file

@ -33,9 +33,11 @@ import org.junit.jupiter.api.Test;
/** Unit tests for {@link LoginFlow} when accessed via a TLS transport. */
public class LoginFlowViaTlsTest extends LoginFlowTestCase {
private static final Optional<String> GOOD_CERT =
private static final Optional<String> GOOD_CERT = Optional.of(CertificateSamples.SAMPLE_CERT);
private static final Optional<String> GOOD_CERT_HASH =
Optional.of(CertificateSamples.SAMPLE_CERT_HASH);
private static final Optional<String> BAD_CERT =
private static final Optional<String> BAD_CERT = Optional.of(CertificateSamples.SAMPLE_CERT2);
private static final Optional<String> BAD_CERT_HASH =
Optional.of(CertificateSamples.SAMPLE_CERT2_HASH);
private static final Optional<String> GOOD_IP = Optional.of("192.168.1.1");
private static final Optional<String> BAD_IP = Optional.of("1.1.1.1");
@ -53,7 +55,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase {
@Test
void testSuccess_withGoodCredentials() throws Exception {
persistResource(getRegistrarBuilder().build());
credentials = new TlsCredentials(true, GOOD_CERT, GOOD_IP);
credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, GOOD_IP);
doSuccessfulTest("login_valid.xml");
}
@ -64,7 +66,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase {
.setIpAddressAllowList(
ImmutableList.of(CidrAddressBlock.create("2001:db8:0:0:0:0:1:1/32")))
.build());
credentials = new TlsCredentials(true, GOOD_CERT, GOOD_IPV6);
credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, GOOD_IPV6);
doSuccessfulTest("login_valid.xml");
}
@ -75,7 +77,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase {
.setIpAddressAllowList(
ImmutableList.of(CidrAddressBlock.create("2001:db8:0:0:0:0:1:1/32")))
.build());
credentials = new TlsCredentials(true, GOOD_CERT, GOOD_IPV6);
credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, GOOD_IPV6);
doSuccessfulTest("login_valid.xml");
}
@ -85,21 +87,29 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase {
getRegistrarBuilder()
.setIpAddressAllowList(ImmutableList.of(CidrAddressBlock.create("192.168.1.255/24")))
.build());
credentials = new TlsCredentials(true, GOOD_CERT, GOOD_IP);
credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, GOOD_IP);
doSuccessfulTest("login_valid.xml");
}
@Test
void testFailure_incorrectClientCertificateHash() {
persistResource(getRegistrarBuilder().build());
credentials = new TlsCredentials(true, BAD_CERT, GOOD_IP);
credentials = new TlsCredentials(true, BAD_CERT_HASH, BAD_CERT, GOOD_IP);
doFailingTest("login_valid.xml", BadRegistrarCertificateException.class);
}
@Test
void testFailure_missingClientCertificateHash() {
// TODO(Sarahbot): This should fail once hash fallback is removed
void testSuccess_missingClientCertificate() throws Exception {
persistResource(getRegistrarBuilder().build());
credentials = new TlsCredentials(true, Optional.empty(), GOOD_IP);
credentials = new TlsCredentials(true, GOOD_CERT_HASH, Optional.empty(), GOOD_IP);
doSuccessfulTest("login_valid.xml");
}
@Test
void testFailure_missingClientCertificateAndHash() {
persistResource(getRegistrarBuilder().build());
credentials = new TlsCredentials(true, Optional.empty(), Optional.empty(), GOOD_IP);
doFailingTest("login_valid.xml", MissingRegistrarCertificateException.class);
}
@ -112,7 +122,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase {
CidrAddressBlock.create(InetAddresses.forString("192.168.1.1"), 32),
CidrAddressBlock.create(InetAddresses.forString("2001:db8::1"), 128)))
.build());
credentials = new TlsCredentials(true, GOOD_CERT, Optional.empty());
credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, Optional.empty());
doFailingTest("login_valid.xml", BadRegistrarIpAddressException.class);
}
@ -125,7 +135,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase {
CidrAddressBlock.create(InetAddresses.forString("192.168.1.1"), 32),
CidrAddressBlock.create(InetAddresses.forString("2001:db8::1"), 128)))
.build());
credentials = new TlsCredentials(true, GOOD_CERT, BAD_IP);
credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, BAD_IP);
doFailingTest("login_valid.xml", BadRegistrarIpAddressException.class);
}
@ -138,7 +148,7 @@ public class LoginFlowViaTlsTest extends LoginFlowTestCase {
CidrAddressBlock.create(InetAddresses.forString("192.168.1.1"), 32),
CidrAddressBlock.create(InetAddresses.forString("2001:db8::1"), 128)))
.build());
credentials = new TlsCredentials(true, GOOD_CERT, BAD_IPV6);
credentials = new TlsCredentials(true, GOOD_CERT_HASH, GOOD_CERT, BAD_IPV6);
doFailingTest("login_valid.xml", BadRegistrarIpAddressException.class);
}
}

8
core/src/test/java/google/registry/tools/ValidateLoginCredentialsCommandTest.java
View file

@ -64,7 +64,7 @@ class ValidateLoginCredentialsCommandTest extends CommandTestCase<ValidateLoginC
runCommand(
"--client=NewRegistrar",
"--password=" + PASSWORD,
"--cert_hash=" + CERT_HASH,
"--cert_file=" + getCertFilename(CertificateSamples.SAMPLE_CERT),
"--ip_address=" + CLIENT_IP);
}
@ -83,7 +83,7 @@ class ValidateLoginCredentialsCommandTest extends CommandTestCase<ValidateLoginC
runCommand(
"--client=NewRegistrar",
"--password=" + PASSWORD,
"--cert_hash=" + CERT_HASH,
"--cert_file=" + getCertFilename(CertificateSamples.SAMPLE_CERT),
"--ip_address=" + CLIENT_IP));
assertThat(thrown)
.hasMessageThat()
@ -91,7 +91,7 @@ class ValidateLoginCredentialsCommandTest extends CommandTestCase<ValidateLoginC
}
@Test
void testFailure_loginWithBadPassword() {
void testFailure_loginWithBadPassword() throws Exception {
EppException thrown =
assertThrows(
BadRegistrarPasswordException.class,
@ -99,7 +99,7 @@ class ValidateLoginCredentialsCommandTest extends CommandTestCase<ValidateLoginC
runCommand(
"--client=NewRegistrar",
"--password=" + new StringBuilder(PASSWORD).reverse(),
"--cert_hash=" + CERT_HASH,
"--cert_file=" + getCertFilename(CertificateSamples.SAMPLE_CERT),
"--ip_address=" + CLIENT_IP));
assertAboutEppExceptions().that(thrown).marshalsToXml();
}