From fd19299ae0ce1e51e203e78b34d33e4b50cf141b Mon Sep 17 00:00:00 2001 From: Jonathan Klabunde Tomer <125505367+jkt-signal@users.noreply.github.com> Date: Fri, 20 Oct 2023 09:09:22 -0700 Subject: [PATCH] Accept a captcha score threshold for challenges from the spam filter --- .../textsecuregcm/WhisperServerService.java | 12 +++- .../controllers/ChallengeController.java | 21 ++++++- .../limits/RateLimitChallengeManager.java | 6 +- .../spam/PushChallengeConfig.java | 44 ++++++++++++++ .../spam/PushChallengeConfigProvider.java | 60 +++++++++++++++++++ .../controllers/ChallengeControllerTest.java | 44 +++++++++++--- .../limits/RateLimitChallengeManagerTest.java | 33 +++++++--- 7 files changed, 198 insertions(+), 22 deletions(-) create mode 100644 service/src/main/java/org/whispersystems/textsecuregcm/spam/PushChallengeConfig.java create mode 100644 service/src/main/java/org/whispersystems/textsecuregcm/spam/PushChallengeConfigProvider.java diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java index f6422a236..d0099f104 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java @@ -167,6 +167,7 @@ import org.whispersystems.textsecuregcm.s3.PostPolicyGenerator; import org.whispersystems.textsecuregcm.securestorage.SecureStorageClient; import org.whispersystems.textsecuregcm.securevaluerecovery.SecureValueRecovery2Client; import org.whispersystems.textsecuregcm.spam.FilterSpam; +import org.whispersystems.textsecuregcm.spam.PushChallengeConfigProvider; import org.whispersystems.textsecuregcm.spam.RateLimitChallengeListener; import org.whispersystems.textsecuregcm.spam.ReportSpamTokenProvider; import org.whispersystems.textsecuregcm.spam.ScoreThresholdProvider; @@ -848,9 +849,14 @@ public class WhisperServerService extends Application webSocketEnvironment, WebSocketEnvironment provisioningEnvironment) { - environment.jersey().register(ScoreThresholdProvider.ScoreThresholdFeature.class); - webSocketEnvironment.jersey().register(ScoreThresholdProvider.ScoreThresholdFeature.class); - provisioningEnvironment.jersey().register(ScoreThresholdProvider.ScoreThresholdFeature.class); + List.of( + ScoreThresholdProvider.ScoreThresholdFeature.class, + PushChallengeConfigProvider.PushChallengeConfigFeature.class) + .forEach(feature -> { + environment.jersey().register(feature); + webSocketEnvironment.jersey().register(feature); + provisioningEnvironment.jersey().register(feature); + }); } private void registerExceptionMappers(Environment environment, diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ChallengeController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ChallengeController.java index c71933db6..c53d385d9 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ChallengeController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ChallengeController.java @@ -36,10 +36,15 @@ import org.whispersystems.textsecuregcm.entities.AnswerRecaptchaChallengeRequest import org.whispersystems.textsecuregcm.limits.RateLimitChallengeManager; import org.whispersystems.textsecuregcm.metrics.UserAgentTagUtil; import org.whispersystems.textsecuregcm.push.NotPushRegisteredException; +import org.whispersystems.textsecuregcm.spam.Extract; +import org.whispersystems.textsecuregcm.spam.FilterSpam; +import org.whispersystems.textsecuregcm.spam.PushChallengeConfig; +import org.whispersystems.textsecuregcm.spam.ScoreThreshold; import org.whispersystems.textsecuregcm.util.HeaderUtils; @Path("/v1/challenge") @Tag(name = "Challenge") +@FilterSpam public class ChallengeController { private final RateLimitChallengeManager rateLimitChallengeManager; @@ -74,7 +79,9 @@ public class ChallengeController { public Response handleChallengeResponse(@Auth final AuthenticatedAccount auth, @Valid final AnswerChallengeRequest answerRequest, @HeaderParam(HttpHeaders.X_FORWARDED_FOR) final String forwardedFor, - @HeaderParam(HttpHeaders.USER_AGENT) final String userAgent) throws RateLimitExceededException, IOException { + @HeaderParam(HttpHeaders.USER_AGENT) final String userAgent, + @Extract final ScoreThreshold captchaScoreThreshold, + @Extract final PushChallengeConfig pushChallengeConfig) throws RateLimitExceededException, IOException { Tags tags = Tags.of(UserAgentTagUtil.getPlatformTag(userAgent)); @@ -82,6 +89,9 @@ public class ChallengeController { if (answerRequest instanceof final AnswerPushChallengeRequest pushChallengeRequest) { tags = tags.and(CHALLENGE_TYPE_TAG, "push"); + if (!pushChallengeConfig.pushPermitted()) { + return Response.status(429).build(); + } rateLimitChallengeManager.answerPushChallenge(auth.getAccount(), pushChallengeRequest.getChallenge()); } else if (answerRequest instanceof AnswerRecaptchaChallengeRequest recaptchaChallengeRequest) { tags = tags.and(CHALLENGE_TYPE_TAG, "recaptcha"); @@ -91,7 +101,8 @@ public class ChallengeController { auth.getAccount(), recaptchaChallengeRequest.getCaptcha(), mostRecentProxy, - userAgent); + userAgent, + captchaScoreThreshold.getScoreThreshold()); if (!success) { return Response.status(428).build(); @@ -150,7 +161,11 @@ public class ChallengeController { @ApiResponse(responseCode = "429", description = "Too many attempts", headers = @Header( name = "Retry-After", description = "If present, an positive integer indicating the number of seconds before a subsequent attempt could succeed")) - public Response requestPushChallenge(@Auth final AuthenticatedAccount auth) { + public Response requestPushChallenge(@Auth final AuthenticatedAccount auth, + @Extract PushChallengeConfig pushChallengeConfig) { + if (!pushChallengeConfig.pushPermitted()) { + return Response.status(429).build(); + } try { rateLimitChallengeManager.sendPushChallenge(auth.getAccount()); return Response.status(200).build(); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManager.java index c2d2b4bda..26066b27d 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManager.java @@ -14,6 +14,8 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.Optional; + import org.whispersystems.textsecuregcm.captcha.Action; import org.whispersystems.textsecuregcm.captcha.CaptchaChecker; import org.whispersystems.textsecuregcm.controllers.RateLimitExceededException; @@ -64,12 +66,12 @@ public class RateLimitChallengeManager { } } - public boolean answerRecaptchaChallenge(final Account account, final String captcha, final String mostRecentProxyIp, final String userAgent) + public boolean answerRecaptchaChallenge(final Account account, final String captcha, final String mostRecentProxyIp, final String userAgent, final Optional scoreThreshold) throws RateLimitExceededException, IOException { rateLimiters.getRecaptchaChallengeAttemptLimiter().validate(account.getUuid()); - final boolean challengeSuccess = captchaChecker.verify(Action.CHALLENGE, captcha, mostRecentProxyIp).isValid(); + final boolean challengeSuccess = captchaChecker.verify(Action.CHALLENGE, captcha, mostRecentProxyIp).isValid(scoreThreshold); final Tags tags = Tags.of( Tag.of(SOURCE_COUNTRY_TAG_NAME, Util.getCountryCode(account.getNumber())), diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/spam/PushChallengeConfig.java b/service/src/main/java/org/whispersystems/textsecuregcm/spam/PushChallengeConfig.java new file mode 100644 index 000000000..e9e8edce8 --- /dev/null +++ b/service/src/main/java/org/whispersystems/textsecuregcm/spam/PushChallengeConfig.java @@ -0,0 +1,44 @@ +/* + * Copyright 2023 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.whispersystems.textsecuregcm.spam; + +import java.util.Optional; + +import org.glassfish.jersey.server.ContainerRequest; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * A PushChallengeConfig may be provided by an upstream request filter. If request contains a + * property for PROPERTY_NAME it can be forwarded to a downstream filter to indicate whether + * push-token challenges can be used in place of captchas when evaluating whether a request should + * be allowed to continue. + */ +public class PushChallengeConfig { + private static final Logger logger = LoggerFactory.getLogger(PushChallengeConfig.class); + + public static final String PROPERTY_NAME = "pushChallengePermitted"; + + /** + * A score threshold in the range [0, 1.0] + */ + private final boolean pushPermitted; + + /** + * Extract an optional score threshold parameter provided by an upstream request filter + */ + public PushChallengeConfig(final ContainerRequest containerRequest) { + this.pushPermitted = Optional + .ofNullable(containerRequest.getProperty(PROPERTY_NAME)) + .filter(obj -> obj instanceof Boolean) + .map(obj -> (Boolean) obj) + .orElse(true); // not a typo! true is the default + } + + public boolean pushPermitted() { + return pushPermitted; + } +} diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/spam/PushChallengeConfigProvider.java b/service/src/main/java/org/whispersystems/textsecuregcm/spam/PushChallengeConfigProvider.java new file mode 100644 index 000000000..65a598aca --- /dev/null +++ b/service/src/main/java/org/whispersystems/textsecuregcm/spam/PushChallengeConfigProvider.java @@ -0,0 +1,60 @@ +/* + * Copyright 2023 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.whispersystems.textsecuregcm.spam; + +import java.util.function.Function; +import javax.inject.Singleton; +import javax.ws.rs.core.Feature; +import javax.ws.rs.core.FeatureContext; +import org.glassfish.jersey.internal.inject.AbstractBinder; +import org.glassfish.jersey.server.ContainerRequest; +import org.glassfish.jersey.server.model.Parameter; +import org.glassfish.jersey.server.spi.internal.ValueParamProvider; + +/** + * Parses a {@link PushChallengeConfig} out of a {@link ContainerRequest} to provide to jersey resources. + * + * A request filter may enrich a ContainerRequest with a PushChallengeConfig by providing a float + * property with the name {@link PushChallengeConfig#PROPERTY_NAME}. This indicates whether push + * challenges may be considered when evaluating whether a request should proceed. + * + * A resource can consume a PushChallengeConfig with by annotating a PushChallengeConfig parameter with {@link Extract} + */ +public class PushChallengeConfigProvider implements ValueParamProvider { + + /** + * Configures the PushChallengeConfigProvider + */ + public static class PushChallengeConfigFeature implements Feature { + @Override + public boolean configure(FeatureContext context) { + context.register(new AbstractBinder() { + @Override + protected void configure() { + bind(PushChallengeConfigProvider.class) + .to(ValueParamProvider.class) + .in(Singleton.class); + } + }); + return true; + } + } + + @Override + public Function getValueProvider(final Parameter parameter) { + if (parameter.getRawType().equals(PushChallengeConfig.class) + && parameter.isAnnotationPresent(Extract.class)) { + return PushChallengeConfig::new; + } + return null; + + } + + @Override + public PriorityType getPriority() { + return Priority.HIGH; + } +} diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ChallengeControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ChallengeControllerTest.java index c91b9dfc3..924aa1e30 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ChallengeControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ChallengeControllerTest.java @@ -22,18 +22,29 @@ import io.dropwizard.testing.junit5.DropwizardExtensionsSupport; import io.dropwizard.testing.junit5.ResourceExtension; import java.io.IOException; import java.time.Duration; +import java.util.Optional; import java.util.Set; +import java.util.concurrent.atomic.AtomicReference; import javax.ws.rs.client.Entity; +import javax.ws.rs.container.ContainerRequestContext; +import javax.ws.rs.container.ContainerRequestFilter; +import javax.ws.rs.core.Feature; +import javax.ws.rs.core.FeatureContext; import javax.ws.rs.core.Response; import org.glassfish.jersey.test.grizzly.GrizzlyWebTestContainerFactory; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.whispersystems.textsecuregcm.auth.AuthenticatedAccount; import org.whispersystems.textsecuregcm.auth.DisabledPermittedAuthenticatedAccount; import org.whispersystems.textsecuregcm.limits.RateLimitChallengeManager; import org.whispersystems.textsecuregcm.mappers.RateLimitExceededExceptionMapper; import org.whispersystems.textsecuregcm.push.NotPushRegisteredException; +import org.whispersystems.textsecuregcm.spam.PushChallengeConfigProvider; +import org.whispersystems.textsecuregcm.spam.ScoreThreshold; +import org.whispersystems.textsecuregcm.spam.ScoreThresholdProvider; import org.whispersystems.textsecuregcm.tests.util.AuthHelper; import org.whispersystems.textsecuregcm.util.SystemMapper; @@ -44,10 +55,24 @@ class ChallengeControllerTest { private static final ChallengeController challengeController = new ChallengeController(rateLimitChallengeManager); + private static final AtomicReference scoreThreshold = new AtomicReference<>(); + private static final ResourceExtension EXTENSION = ResourceExtension.builder() .addProvider(AuthHelper.getAuthFilter()) .addProvider(new PolymorphicAuthValueFactoryProvider.Binder<>( Set.of(AuthenticatedAccount.class, DisabledPermittedAuthenticatedAccount.class))) + .addProvider(ScoreThresholdProvider.ScoreThresholdFeature.class) + .addProvider(PushChallengeConfigProvider.PushChallengeConfigFeature.class) + .addProvider(new Feature() { + public boolean configure(FeatureContext featureContext) { + featureContext.register(new ContainerRequestFilter() { + public void filter(ContainerRequestContext requestContext) { + requestContext.setProperty(ScoreThreshold.PROPERTY_NAME, scoreThreshold.get()); + } + }); + return true; + } + }) .setMapper(SystemMapper.jsonMapper()) .setTestContainerFactory(new GrizzlyWebTestContainerFactory()) .addResource(new RateLimitExceededExceptionMapper()) @@ -57,6 +82,7 @@ class ChallengeControllerTest { @AfterEach void teardown() { reset(rateLimitChallengeManager); + scoreThreshold.set(null); } @Test @@ -99,8 +125,9 @@ class ChallengeControllerTest { assertEquals(String.valueOf(retryAfter.toSeconds()), response.getHeaderString("Retry-After")); } - @Test - void testHandleRecaptcha() throws RateLimitExceededException, IOException { + @ParameterizedTest + @ValueSource(booleans = { true, false } ) + void testHandleRecaptcha(boolean hasThreshold) throws RateLimitExceededException, IOException { final String recaptchaChallengeJson = """ { "type": "recaptcha", @@ -109,9 +136,13 @@ class ChallengeControllerTest { } """; - when(rateLimitChallengeManager.answerRecaptchaChallenge(any(), any(), any(), any())) + when(rateLimitChallengeManager.answerRecaptchaChallenge(any(), any(), any(), any(), any())) .thenReturn(true); + + if (hasThreshold) { + scoreThreshold.set(Float.valueOf(0.5f)); + } final Response response = EXTENSION.target("/v1/challenge") .request() .header(HttpHeaders.X_FORWARDED_FOR, "10.0.0.1") @@ -120,7 +151,7 @@ class ChallengeControllerTest { assertEquals(200, response.getStatus()); - verify(rateLimitChallengeManager).answerRecaptchaChallenge(eq(AuthHelper.VALID_ACCOUNT), eq("The value of the solved captcha token"), eq("10.0.0.1"), anyString()); + verify(rateLimitChallengeManager).answerRecaptchaChallenge(eq(AuthHelper.VALID_ACCOUNT), eq("The value of the solved captcha token"), eq("10.0.0.1"), anyString(), eq(hasThreshold ? Optional.of(0.5f) : Optional.empty())); } @Test @@ -132,8 +163,7 @@ class ChallengeControllerTest { "captcha": "The value of the solved captcha token" } """; - - when(rateLimitChallengeManager.answerRecaptchaChallenge(eq(AuthHelper.VALID_ACCOUNT), eq("The value of the solved captcha token"), eq("10.0.0.1"), anyString())) + when(rateLimitChallengeManager.answerRecaptchaChallenge(eq(AuthHelper.VALID_ACCOUNT), eq("The value of the solved captcha token"), eq("10.0.0.1"), anyString(), any())) .thenReturn(false); final Response response = EXTENSION.target("/v1/challenge") @@ -157,7 +187,7 @@ class ChallengeControllerTest { final Duration retryAfter = Duration.ofMinutes(17); doThrow(new RateLimitExceededException(retryAfter, true)).when(rateLimitChallengeManager) - .answerRecaptchaChallenge(any(), any(), any(), any()); + .answerRecaptchaChallenge(any(), any(), any(), any(), any()); final Response response = EXTENSION.target("/v1/challenge") .request() diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManagerTest.java index 7665455e6..9113523c5 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/limits/RateLimitChallengeManagerTest.java @@ -13,9 +13,14 @@ import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; import java.io.IOException; +import java.util.Optional; import java.util.UUID; +import java.util.stream.Stream; + import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.ValueSource; import org.whispersystems.textsecuregcm.captcha.Action; import org.whispersystems.textsecuregcm.captcha.AssessmentResult; @@ -27,6 +32,8 @@ import org.whispersystems.textsecuregcm.storage.Account; class RateLimitChallengeManagerTest { + private static final float DEFAULT_SCORE_THRESHOLD = 0.1f; + private PushChallengeManager pushChallengeManager; private CaptchaChecker captchaChecker; private RateLimiters rateLimiters; @@ -71,27 +78,39 @@ class RateLimitChallengeManagerTest { } @ParameterizedTest - @ValueSource(booleans = {true, false}) - void answerRecaptchaChallenge(final boolean successfulChallenge) throws RateLimitExceededException, IOException { + @MethodSource + void answerRecaptchaChallenge(Optional scoreThreshold, float actualScore, boolean expectSuccess) + throws RateLimitExceededException, IOException { final Account account = mock(Account.class); when(account.getNumber()).thenReturn("+18005551234"); when(account.getUuid()).thenReturn(UUID.randomUUID()); when(captchaChecker.verify(eq(Action.CHALLENGE), any(), any())) - .thenReturn(successfulChallenge - ? AssessmentResult.alwaysValid() - : AssessmentResult.invalid()); + .thenReturn(AssessmentResult.fromScore(actualScore, DEFAULT_SCORE_THRESHOLD)); when(rateLimiters.getRecaptchaChallengeAttemptLimiter()).thenReturn(mock(RateLimiter.class)); when(rateLimiters.getRecaptchaChallengeSuccessLimiter()).thenReturn(mock(RateLimiter.class)); when(rateLimiters.getRateLimitResetLimiter()).thenReturn(mock(RateLimiter.class)); - rateLimitChallengeManager.answerRecaptchaChallenge(account, "captcha", "10.0.0.1", "Test User-Agent"); + rateLimitChallengeManager.answerRecaptchaChallenge(account, "captcha", "10.0.0.1", "Test User-Agent", scoreThreshold); - if (successfulChallenge) { + if (expectSuccess) { verify(rateLimitChallengeListener).handleRateLimitChallengeAnswered(account, ChallengeType.CAPTCHA); } else { verifyNoInteractions(rateLimitChallengeListener); } } + + private static Stream answerRecaptchaChallenge() { + return Stream.of( + Arguments.of(Optional.empty(), 0.5f, true), + Arguments.of(Optional.empty(), 0.1f, true), + Arguments.of(Optional.empty(), 0.0f, false), + Arguments.of(Optional.of(0.1f), 0.5f, true), + Arguments.of(Optional.of(0.1f), 0.1f, true), + Arguments.of(Optional.of(0.1f), 0.0f, false), + Arguments.of(Optional.of(0.3f), 0.5f, true), + Arguments.of(Optional.of(0.3f), 0.1f, false), + Arguments.of(Optional.of(0.3f), 0.0f, false)); + } }