improve error handling, cleanup config (#828)

* more error feedback about SdkClientExceptions in AWS SSM case

* avoid deprecated isDevelopment calls

* better error feedback when failing to connect to Source api

* refactorCommonRequestHandler, to make a bit clearer

* cleanup usage of deprecated method

* fix compile

* clean unused imports

* wrap url parsing in exception handling

* even more exception handling

java/core/src/main/java/co/worklytics/psoxy/ErrorCauses.java

@@ -24,6 +24,21 @@ public enum ErrorCauses {
      * request was not sent over HTTPS
      */
     HTTPS_REQUIRED,
+
+    /**
+     *  indicates failure to connect from proxy instance to source
+     */
+    CONNECTION_TO_SOURCE,
+
+    /**
+     *  failed to build target URL (eg, that of source) from request URL (requested from proxy)
+     */
+    FAILED_TO_BUILD_URL,
+
+    /**
+     *  failed to get configuration data; or misconfigured.
+     */
+    CONFIGURATION_FAILURE,
     ;
 
 }

java/core/src/main/java/co/worklytics/psoxy/PsoxyModule.java

@@ -26,7 +26,6 @@
 import com.google.api.client.http.HttpContent;
 import com.google.api.client.json.JsonFactory;
 import com.google.api.client.json.gson.GsonFactory;
-import com.google.common.base.Preconditions;
 import com.jayway.jsonpath.Configuration;
 import com.jayway.jsonpath.spi.json.JacksonJsonProvider;
 import com.jayway.jsonpath.spi.mapper.JacksonMappingProvider;
@@ -241,14 +240,14 @@ Base64UrlSha256HashPseudonymEncoder base64UrlSha256HashPseudonymEncoder() {
 
     @Provides
     @Singleton
-    JsonSchemaFilterUtils schemaRuleUtils(EnvVarsConfigService configService) {
+    JsonSchemaFilterUtils schemaRuleUtils(EnvVarsConfigService envVarsConfigService) {
         ObjectMapper objectMapper = new ObjectMapper()
                 .registerModule(new JavaTimeModule())
                 .configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false);
 
         //TODO: probably more proper to override with a 'development' module of some kind
         JsonSchemaFilterUtils.Options.OptionsBuilder options = JsonSchemaFilterUtils.Options.builder();
-        options.logRedactions(configService.isDevelopment());
+        options.logRedactions(envVarsConfigService.isDevelopment());
 
         return new JsonSchemaFilterUtils(objectMapper, options.build());
     }

java/core/src/main/java/co/worklytics/psoxy/gateway/ConfigService.java

@@ -67,15 +67,6 @@ default Optional<ConfigValueWithMetadata> getConfigPropertyWithMetadata(ConfigPr
             .map(value -> ConfigValueWithMetadata.builder().value(value).build());
     }
 
-    /**
-     * @deprecated use EnvVarsConfigService::isDevelopment
-     */
-    @Deprecated // use EnvVarsConfigService::isDevelopment
-    default boolean isDevelopment() {
-        return this.getConfigPropertyAsOptional(ProxyConfigProperty.IS_DEVELOPMENT_MODE)
-                .map(Boolean::parseBoolean).orElse(false);
-    }
-
 
     @Builder
     @Value

java/core/src/main/java/co/worklytics/psoxy/gateway/impl/CachingConfigServiceDecorator.java

@@ -95,8 +95,4 @@ public Optional<String> getConfigPropertyAsOptional(ConfigProperty property) {
         }
     }
 
-    @Override
-    public boolean isDevelopment() {
-        return delegate.isDevelopment();
-    }
 }

java/core/src/main/java/co/worklytics/psoxy/gateway/impl/CommonRequestHandler.java

@@ -21,8 +21,8 @@
 import com.google.common.collect.Sets;
 import lombok.NoArgsConstructor;
 import lombok.SneakyThrows;
+import lombok.Value;
 import lombok.extern.java.Log;
-import org.apache.commons.lang3.ObjectUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.hc.core5.http.HttpHeaders;
 import org.apache.http.HttpStatus;
@@ -31,6 +31,8 @@
 
 import javax.inject.Inject;
 import java.io.IOException;
+import java.net.ConnectException;
+import java.net.MalformedURLException;
 import java.net.URL;
 import java.util.Locale;
 import java.util.Objects;
@@ -147,39 +149,44 @@ public HttpEventResponse handle(HttpEventRequest request) {
                     .build();
         }
 
-
         Optional<HttpEventResponse> healthCheckResponse = healthCheckRequestHandler.handleIfHealthCheck(request);
         if (healthCheckResponse.isPresent()) {
             return healthCheckResponse.get();
         }
 
-        // parse requested URL, re-writing host/etc
-        String requestedTargetUrl = parseRequestedTarget(request);
-
-        // if requested target URL has tokenized components, reverse
-        String clearTargetUrl = reverseTokenizedUrlComponents(requestedTargetUrl);
-
-        boolean tokenizedURLReversed = ObjectUtils.notEqual(requestedTargetUrl, clearTargetUrl);
-
-        // Using original URL to check sanitized rules, as they should match the original URL. It could contain tokenized components.
-        // Examples:
-        // /v1/accounts/p~12adsfasdfasdf31
-        // /v1/accounts/12345
-        URL originalRequestedURL = new URL(requestedTargetUrl);
-        // And the URL to use for source request; it could contain the reversed tokenized components
-        URL targetForSourceApiRequest = new URL(clearTargetUrl);
+        RequestUrls requestUrls;
+        try {
+            requestUrls = buildRequestedUrls(request);
+        } catch (Throwable e) {
+            //really shouldn't happen ... parsing one url from another, so would be a bad bug in our canonicalization code for this to go wrong
+            log.log(Level.WARNING, "Error parsing  / building request URL", e);
+            return HttpEventResponse.builder()
+                    .statusCode(HttpStatus.SC_INTERNAL_SERVER_ERROR)
+                    .header(ResponseHeader.ERROR.getHttpHeader(), ErrorCauses.FAILED_TO_BUILD_URL.name())
+                    .body("Error parsing request URL")
+                    .build();
+        }
 
         // avoid logging clear URL outside of dev
-        URL toLog = envVarsConfigService.isDevelopment() ? targetForSourceApiRequest : originalRequestedURL;
+        URL toLog = envVarsConfigService.isDevelopment() ? requestUrls.getTarget() : requestUrls.getOriginal();
 
         boolean skipSanitization = skipSanitization(request);
 
         HttpEventResponse.HttpEventResponseBuilder builder = HttpEventResponse.builder();
 
-        this.sanitizer = loadSanitizerRules();
+        try {
+            this.sanitizer = loadSanitizerRules();
+        } catch (Throwable e) {
+            log.log(Level.SEVERE, "Error loading sanitizer rules", e);
+            return HttpEventResponse.builder()
+                    .statusCode(HttpStatus.SC_INTERNAL_SERVER_ERROR)
+                    .header(ResponseHeader.ERROR.getHttpHeader(), ErrorCauses.CONFIGURATION_FAILURE.name())
+                    .body("Error loading sanitizer rules")
+                    .build();
+        }
 
         //build log entry
-        String logEntry = String.format("%s %s TokenInUrlReversed=%b", request.getHttpMethod(), URLUtils.relativeURL(toLog), tokenizedURLReversed);
+        String logEntry = String.format("%s %s TokenInUrlDecrypted=%b", request.getHttpMethod(), URLUtils.relativeURL(toLog), requestUrls.hasDecryptedTokens());
         if (request.getClientIp().isPresent()) {
             // client IP is NOT available for direct AWS Lambda calls; only for API Gateway.
             // don't want to put blank/unknown in direct case, so log IP conditionally
@@ -188,7 +195,7 @@ public HttpEventResponse handle(HttpEventRequest request) {
 
         if (skipSanitization) {
             log.info(String.format("%s. Skipping sanitization.", logEntry));
-        } else if (sanitizer.isAllowed(request.getHttpMethod(), originalRequestedURL)) {
+        } else if (sanitizer.isAllowed(request.getHttpMethod(), requestUrls.getOriginal())) {
             log.info(String.format("%s. Rules allowed call.", logEntry));
         } else {
             builder.statusCode(HttpStatus.SC_FORBIDDEN);
@@ -209,7 +216,17 @@ public HttpEventResponse handle(HttpEventRequest request) {
                 content = new ByteArrayContent(contentType, request.getBody());
             }
 
-            sourceApiRequest = requestFactory.buildRequest(request.getHttpMethod(), new GenericUrl(targetForSourceApiRequest), content);
+            sourceApiRequest = requestFactory.buildRequest(request.getHttpMethod(), new GenericUrl(requestUrls.getTarget()), content);
+
+            //TODO: what headers to forward???
+            populateHeadersFromSource(sourceApiRequest, request, requestUrls.getTarget());
+
+            //setup request
+            sourceApiRequest
+                .setThrowExceptionOnExecuteError(false)
+                .setConnectTimeout(SOURCE_API_REQUEST_CONNECT_TIMEOUT_MILLISECONDS)
+                .setReadTimeout(SOURCE_API_REQUEST_READ_TIMEOUT);
+
         } catch (IOException e) {
             builder.statusCode(HttpStatus.SC_INTERNAL_SERVER_ERROR);
             builder.body("Failed to parse request; review logs");
@@ -226,21 +243,19 @@ public HttpEventResponse handle(HttpEventRequest request) {
             return builder.build();
         }
 
-        //TODO: what headers to forward???
-        populateHeadersFromSource(sourceApiRequest, request, targetForSourceApiRequest);
-
-        //setup request
-        sourceApiRequest
-                .setThrowExceptionOnExecuteError(false)
-                .setConnectTimeout(SOURCE_API_REQUEST_CONNECT_TIMEOUT_MILLISECONDS)
-                .setReadTimeout(SOURCE_API_REQUEST_READ_TIMEOUT);
-
-        //q: add exception handlers for IOExceptions / HTTP error responses, so those retries
-        // happen in proxy rather than on Worklytics-side?
-        com.google.api.client.http.HttpResponse sourceApiResponse = sourceApiRequest.execute();
-
-        // return response
-        builder.statusCode(sourceApiResponse.getStatusCode());
+        com.google.api.client.http.HttpResponse sourceApiResponse = null;
+        try {
+            //q: add exception handlers for IOExceptions / HTTP error responses, so those retries
+            // happen in proxy rather than on Worklytics-side?
+            sourceApiResponse = sourceApiRequest.execute();
+        } catch (ConnectException e) {
+            //connectivity problems
+            builder.statusCode(HttpStatus.SC_SERVICE_UNAVAILABLE);
+            builder.header(ResponseHeader.ERROR.getHttpHeader(), ErrorCauses.CONNECTION_TO_SOURCE.name());
+            builder.body("Error connecting to source API: " + e.getMessage());
+            log.log(Level.SEVERE, "Error connecting to source API: " + e.getMessage(), e);
+            return builder.build();
+        }
 
         try {
             // return response
@@ -261,7 +276,7 @@ public HttpEventResponse handle(HttpEventRequest request) {
                 } else {
                     RESTApiSanitizer sanitizerForRequest = getSanitizerForRequest(request);
 
-                    proxyResponseContent = sanitizerForRequest.sanitize(request.getHttpMethod(), originalRequestedURL, responseContent);
+                    proxyResponseContent = sanitizerForRequest.sanitize(request.getHttpMethod(), requestUrls.getOriginal(), responseContent);
                     String rulesSha = rulesUtils.sha(sanitizerForRequest.getRules());
                     builder.header(ResponseHeader.RULES_SHA.getHttpHeader(), rulesSha);
                     log.info("response sanitized with rule set " + rulesSha);
@@ -281,6 +296,24 @@ public HttpEventResponse handle(HttpEventRequest request) {
     }
 
 
+    @Value
+    static class RequestUrls {
+
+        /**
+         * original URL, as requested
+         */
+        URL original;
+
+        /**
+         * decrypted URL, if tokenized components were found
+         */
+        URL target;
+
+        boolean hasDecryptedTokens() {
+            return !original.equals(target);
+        }
+    }
+
     /**
      * encapsulates dynamically configuring Sanitizer based on request (to support some aspects of
      * its behavior being controlled via HTTP headers)
@@ -308,6 +341,27 @@ Optional<PseudonymImplementation> parsePseudonymImplementation(HttpEventRequest
                 .map(PseudonymImplementation::parseHttpHeaderValue);
     }
 
+    @VisibleForTesting
+    @SneakyThrows // MalformedURLException, but that really shouldn't happen!!
+    RequestUrls buildRequestedUrls(HttpEventRequest request) {
+
+        // parse requested URL, re-writing host/etc
+        String requestedTargetUrl = parseRequestedTarget(request);
+
+        // if requested target URL has tokenized components, reverse
+        String clearTargetUrl = reverseTokenizedUrlComponents(requestedTargetUrl);
+
+        // Using original URL to check sanitized rules, as they should match the original URL. It could contain tokenized components.
+        // Examples:
+        // /v1/accounts/p~12adsfasdfasdf31
+        // /v1/accounts/12345
+        URL originalRequestedURL = new URL(requestedTargetUrl);
+        // And the URL to use for source request; it could contain the reversed tokenized components
+        URL targetForSourceApiRequest = new URL(clearTargetUrl);
+
+        return new RequestUrls(originalRequestedURL, targetForSourceApiRequest);
+    }
+
     /**
      * side effects: modifies the responseBuilder, adding the headers to pass through
      *
@@ -388,7 +442,7 @@ HttpRequestFactory getRequestFactory(HttpEventRequest request) {
      * @return
      */
     private boolean skipSanitization(HttpEventRequest request) {
-        if (config.isDevelopment()) {
+        if (envVarsConfigService.isDevelopment()) {
             // caller requested to skip
             return request.getHeader(ControlHeader.SKIP_SANITIZER.getHttpHeader())
                     .map(Boolean::parseBoolean)
@@ -399,7 +453,7 @@ private boolean skipSanitization(HttpEventRequest request) {
     }
 
     private void logRequestIfVerbose(HttpEventRequest request) {
-        if (config.isDevelopment()) {
+        if (envVarsConfigService.isDevelopment()) {
             log.info(request.prettyPrint());
         }
     }
@@ -435,7 +489,7 @@ String reverseTokenizedUrlComponents(String encodedURL) {
     }
 
     private void logIfDevelopmentMode(Supplier<String> messageSupplier) {
-        if (config.isDevelopment()) {
+        if (envVarsConfigService.isDevelopment()) {
             log.info(messageSupplier.get());
         }
     }

java/core/src/main/java/co/worklytics/psoxy/gateway/impl/EnvVarsConfigService.java

@@ -1,6 +1,7 @@
 package co.worklytics.psoxy.gateway.impl;
 
 import co.worklytics.psoxy.gateway.ConfigService;
+import co.worklytics.psoxy.gateway.ProxyConfigProperty;
 import lombok.NoArgsConstructor;
 
 import javax.inject.Inject;
@@ -22,4 +23,11 @@ public String getConfigPropertyOrError(ConfigProperty property) {
     public Optional<String> getConfigPropertyAsOptional(ConfigProperty property) {
         return Optional.ofNullable(System.getenv(property.name()));
     }
+
+     public boolean isDevelopment() {
+        return this.getConfigPropertyAsOptional(ProxyConfigProperty.IS_DEVELOPMENT_MODE)
+            .map(Boolean::parseBoolean).orElse(false);
+    }
+
+
 }

java/core/src/test/java/co/worklytics/psoxy/gateway/impl/MemoryConfigService.java

@@ -31,9 +31,4 @@ public String getConfigPropertyOrError(@NonNull ConfigProperty property) {
     public Optional<String> getConfigPropertyAsOptional(@NonNull ConfigProperty property) {
         return Optional.ofNullable(map.get(property.name()));
     }
-
-    @Override
-    public boolean isDevelopment() {
-        return true;
-    }
 }

java/impl/aws/src/main/java/co/worklytics/psoxy/aws/ParameterStoreConfigService.java

@@ -1,10 +1,10 @@
 package co.worklytics.psoxy.aws;
 
-import co.worklytics.psoxy.gateway.ConfigService;
 import co.worklytics.psoxy.gateway.LockService;
 import co.worklytics.psoxy.gateway.SecretStore;
 import co.worklytics.psoxy.gateway.impl.EnvVarsConfigService;
 import co.worklytics.psoxy.utils.RandomNumberGenerator;
+import com.amazonaws.SdkClientException;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.util.concurrent.Uninterruptibles;
@@ -25,7 +25,6 @@
 import java.util.NoSuchElementException;
 import java.util.Objects;
 import java.util.Optional;
-import java.util.concurrent.TimeUnit;
 import java.util.function.Function;
 import java.util.logging.Level;
 
@@ -148,6 +147,10 @@ <T> Optional<T> getConfigPropertyAsOptional(ConfigProperty property, Function<Ge
                 log.log(Level.SEVERE, String.format("Throttling issues for key %s, rate limit reached most likely despite retries", paramName), e);
             }
             throw new IllegalStateException(String.format("failed to get config value: %s", paramName), e);
+        } catch (SdkClientException e) {
+            // transient IO related errors reading from SSM? can't reproduce exactly
+            log.log(Level.SEVERE, "Error reading configuration from SSM: " + e.getMessage(), e);
+            throw e;
         }
     }