From ec50fc23d0d0c091d6f4c9a1a43ad9929e138385 Mon Sep 17 00:00:00 2001 From: Erik Schultink Date: Thu, 3 Oct 2024 17:16:33 -0700 Subject: [PATCH 1/8] drop zoom oauth refresh token parameter --- infra/modules/worklytics-connector-specs/main.tf | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/infra/modules/worklytics-connector-specs/main.tf b/infra/modules/worklytics-connector-specs/main.tf index f0d7efea9..91ecbf45f 100644 --- a/infra/modules/worklytics-connector-specs/main.tf +++ b/infra/modules/worklytics-connector-specs/main.tf @@ -989,13 +989,7 @@ EOT value_managed_by_tf : false description : "Short-lived Oauth access_token used by connector to retrieve Zoom data. Filled by Proxy instance." }, - { - name : "OAUTH_REFRESH_TOKEN" - writable : true - lockable : true - sensitive : true - value_managed_by_tf : false - }, # q: needed? per logic as of 9 June 2023, would be created + # used to have OAUTH_REFRESH_TOKEN; but as of Sept 2024, seeing that it's not being used ], reserved_concurrent_executions : null # 1 example_api_calls_user_to_impersonate : null From a888e6d3c7b3670ed882722273ee5486f896d521 Mon Sep 17 00:00:00 2001 From: Erik Schultink Date: Thu, 3 Oct 2024 17:18:03 -0700 Subject: [PATCH 2/8] zoom client id isn't sensitive --- infra/modules/worklytics-connector-specs/main.tf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/infra/modules/worklytics-connector-specs/main.tf b/infra/modules/worklytics-connector-specs/main.tf index 91ecbf45f..f19b25953 100644 --- a/infra/modules/worklytics-connector-specs/main.tf +++ b/infra/modules/worklytics-connector-specs/main.tf @@ -978,7 +978,7 @@ EOT { name : "ACCOUNT_ID" writable : false - sensitive : true + sensitive : false value_managed_by_tf : false description : "Account ID of the Zoom tenant from which the Connector will retrieve Zoom data. Value should be obtained from your Zoom admin." }, From a227baf47fb6cba597b9b019570ba6951471a33a Mon Sep 17 00:00:00 2001 From: Erik Schultink Date: Thu, 3 Oct 2024 17:23:39 -0700 Subject: [PATCH 3/8] document that zoom renders this stuff in clear in web consoles --- infra/modules/worklytics-connector-specs/main.tf | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/infra/modules/worklytics-connector-specs/main.tf b/infra/modules/worklytics-connector-specs/main.tf index f19b25953..fbf385b8e 100644 --- a/infra/modules/worklytics-connector-specs/main.tf +++ b/infra/modules/worklytics-connector-specs/main.tf @@ -971,14 +971,14 @@ EOT { name : "CLIENT_ID" writable : false - sensitive : false + sensitive : false # zoom renders in clear in console value_managed_by_tf : false description : "Client ID of the Zoom 'Server-to-Server' OAuth App used by the Connector to retrieve Zoom data. Value should be obtained from your Zoom admin." }, { name : "ACCOUNT_ID" writable : false - sensitive : false + sensitive : false # zoom renders in clear in console value_managed_by_tf : false description : "Account ID of the Zoom tenant from which the Connector will retrieve Zoom data. Value should be obtained from your Zoom admin." }, From 0bf8dd0f1cde9ad92651115cc4f1586ff90097cd Mon Sep 17 00:00:00 2001 From: Erik Schultink Date: Thu, 3 Oct 2024 17:27:09 -0700 Subject: [PATCH 4/8] try to conditionally set parameter type for AWS; test thisgit status --- infra/modules/aws-ssm-secrets/main.tf | 29 ++++++++++++--------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/infra/modules/aws-ssm-secrets/main.tf b/infra/modules/aws-ssm-secrets/main.tf index ce4bf912b..5f941fcc9 100644 --- a/infra/modules/aws-ssm-secrets/main.tf +++ b/infra/modules/aws-ssm-secrets/main.tf @@ -13,21 +13,18 @@ locals { externally_managed_secrets = { for k, spec in var.secrets : k => spec if !(spec.value_managed_by_tf) } terraform_managed_secrets = { for k, spec in var.secrets : k => spec if spec.value_managed_by_tf } - - tf_management_description_appendix = "Value managed by a Terraform configuration; changes outside Terraform may be overwritten by subsequent 'terraform apply' runs" } # see: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/ssm_parameter resource "aws_ssm_parameter" "secret" { for_each = local.terraform_managed_secrets - name = "${local.path_prefix}${each.key}" - # Due https://github.com/hashicorp/terraform-provider-aws/issues/31267 - # all are added as secureString - type = "SecureString" - description = each.value.description - value = sensitive(coalesce(each.value.value, local.PLACEHOLDER_VALUE)) - key_id = coalesce(var.kms_key_id, "alias/aws/ssm") + name = "${local.path_prefix}${each.key}" + type = each.value.sensitive ? "SecureString" : "String" + description = each.value.description + value = each.value.sensitive ? sensitive(coalesce(each.value.value, local.PLACEHOLDER_VALUE)) : null + insecure_value = each.value.sensitive ? null : coalesce(each.value.value, local.PLACEHOLDER_VALUE) + key_id = coalesce(var.kms_key_id, "alias/aws/ssm") lifecycle { ignore_changes = [ @@ -41,17 +38,17 @@ resource "aws_ssm_parameter" "secret" { resource "aws_ssm_parameter" "secret_with_externally_managed_value" { for_each = local.externally_managed_secrets - name = "${local.path_prefix}${each.key}" - # Due https://github.com/hashicorp/terraform-provider-aws/issues/31267 - # all are added as secureString - type = "SecureString" - description = each.value.description - value = sensitive(coalesce(each.value.value, local.PLACEHOLDER_VALUE)) - key_id = coalesce(var.kms_key_id, "alias/aws/ssm") + name = "${local.path_prefix}${each.key}" + type = each.value.sensitive ? "SecureString" : "String" + description = each.value.description + value = each.value.sensitive ? sensitive(coalesce(each.value.value, local.PLACEHOLDER_VALUE)) : null + insecure_value = each.value.sensitive ? null : coalesce(each.value.value, local.PLACEHOLDER_VALUE) + key_id = coalesce(var.kms_key_id, "alias/aws/ssm") lifecycle { ignore_changes = [ value, # key difference here; we don't want to overwrite values filled by the external process + insecure_value, tags ] } From 1157e85d2f1b08b10e7b1c2859ae6dd1eca6f248 Mon Sep 17 00:00:00 2001 From: Erik Schultink Date: Fri, 4 Oct 2024 09:26:42 -0700 Subject: [PATCH 5/8] some zoom troubleshooting insights --- docs/sources/zoom/README.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/docs/sources/zoom/README.md b/docs/sources/zoom/README.md index d35547690..bd17b4068 100644 --- a/docs/sources/zoom/README.md +++ b/docs/sources/zoom/README.md @@ -81,3 +81,21 @@ sufficient, but as of May 2024 are no longer available for newly created Zoom ap Once the scopes are added, click on `Done` and then `Continue`. 6. Activate the app + +## Troubleshooting + +### Zoom API Error : 400 invalid client + +`{"reason":"Invalid client_id or client_secret","error":"invalid_client"}` + +Causes: + - extra chars in Client ID; or incorrect Client ID + +Confirm that the `Client ID` and `Client Secret` are correctly set in your secret store solution (AWS Parameter Store, Secrets Manager, or GCP Secret Manager). + +### Zoom API Error : 400 Bad Request + +`{"reason":"Bad Request","error":"invalid_request"}` + +Causes: + - extra chars in Account ID; or incorrect Account ID From 17c7a2851b1721e371e09cb5cf842e87ab5a6f00 Mon Sep 17 00:00:00 2001 From: Erik Schultink Date: Fri, 4 Oct 2024 09:32:03 -0700 Subject: [PATCH 6/8] add a bunch of trims to AccountCredentialsGrantTokenRequestBuilder --- ...untCredentialsGrantTokenRequestBuilder.java | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/java/core/src/main/java/co/worklytics/psoxy/gateway/impl/oauth/AccountCredentialsGrantTokenRequestBuilder.java b/java/core/src/main/java/co/worklytics/psoxy/gateway/impl/oauth/AccountCredentialsGrantTokenRequestBuilder.java index 1204e89c6..94cc1b353 100644 --- a/java/core/src/main/java/co/worklytics/psoxy/gateway/impl/oauth/AccountCredentialsGrantTokenRequestBuilder.java +++ b/java/core/src/main/java/co/worklytics/psoxy/gateway/impl/oauth/AccountCredentialsGrantTokenRequestBuilder.java @@ -8,6 +8,7 @@ import com.google.api.client.http.UrlEncodedContent; import lombok.Getter; import lombok.NoArgsConstructor; +import org.apache.commons.lang3.StringUtils; import javax.inject.Inject; import java.nio.charset.StandardCharsets; @@ -59,23 +60,26 @@ public enum ConfigProperty implements ConfigService.ConfigProperty { @Override public HttpContent buildPayload() { + + //trim to avoid copy-paste errors + //q : check for non-printable chars or anything like that + String accountId = StringUtils.trim(config.getConfigPropertyAsOptional(ConfigProperty.ACCOUNT_ID) + .orElseGet(() -> secretStore.getConfigPropertyOrError(ConfigProperty.ACCOUNT_ID))); + // The documentation doesn't say anything to use POST data, but passes everything in the URL // Tested manually and, for the moment, it is accepted as POST data Map data = new TreeMap<>(); data.put(PARAM_GRANT_TYPE, getGrantType()); - //q: move to config? cost-benefit, mainly - data.put(PARAM_ACCOUNT_ID, - config.getConfigPropertyAsOptional(ConfigProperty.ACCOUNT_ID) - .orElseGet(() -> secretStore.getConfigPropertyOrError(ConfigProperty.ACCOUNT_ID))); + data.put(PARAM_ACCOUNT_ID, accountId); return new UrlEncodedContent(data); } @Override public void addHeaders(HttpHeaders httpHeaders) { - String clientId = config.getConfigPropertyAsOptional(ConfigProperty.CLIENT_ID) - .orElseGet(() -> secretStore.getConfigPropertyOrError(ConfigProperty.CLIENT_ID)); + String clientId = StringUtils.trim(config.getConfigPropertyAsOptional(ConfigProperty.CLIENT_ID) + .orElseGet(() -> secretStore.getConfigPropertyOrError(ConfigProperty.CLIENT_ID))); - String clientSecret = secretStore.getConfigPropertyOrError(ConfigProperty.CLIENT_SECRET); + String clientSecret = StringUtils.trim(secretStore.getConfigPropertyOrError(ConfigProperty.CLIENT_SECRET)); String token = Base64.getEncoder() .encodeToString(String.join(":", clientId, clientSecret).getBytes(StandardCharsets.UTF_8)); httpHeaders.setAuthorization("Basic " + token); From 8b498ccab3bf74c431d2e9ed9a540f6fca6b2b95 Mon Sep 17 00:00:00 2001 From: Erik Schultink Date: Fri, 4 Oct 2024 10:17:56 -0700 Subject: [PATCH 7/8] cleanup error handling case --- .../psoxy/gateway/impl/CommonRequestHandler.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/java/core/src/main/java/co/worklytics/psoxy/gateway/impl/CommonRequestHandler.java b/java/core/src/main/java/co/worklytics/psoxy/gateway/impl/CommonRequestHandler.java index 14262422d..31bd2ced1 100644 --- a/java/core/src/main/java/co/worklytics/psoxy/gateway/impl/CommonRequestHandler.java +++ b/java/core/src/main/java/co/worklytics/psoxy/gateway/impl/CommonRequestHandler.java @@ -212,11 +212,10 @@ public HttpEventResponse handle(HttpEventRequest request) { sourceApiRequest = requestFactory.buildRequest(request.getHttpMethod(), new GenericUrl(targetForSourceApiRequest), content); } catch (IOException e) { builder.statusCode(HttpStatus.SC_INTERNAL_SERVER_ERROR); - builder.body("Failed to parse request; review logs"); + builder.body("Failed to parse response; review logs"); builder.header(ResponseHeader.ERROR.getHttpHeader(), ErrorCauses.CONNECTION_SETUP.name()); - log.log(Level.WARNING, e.getMessage(), e); + log.log(Level.WARNING, e.getMessage()); //something like "Error getting access token for service account: 401 Unauthorized POST https://oauth2.googleapis.com/token," - log.log(Level.WARNING, "Confirm oauth scopes set in config.yaml match those granted in data source"); return builder.build(); } catch (java.util.NoSuchElementException e) { // missing config, such as ACCESS_TOKEN @@ -225,8 +224,6 @@ public HttpEventResponse handle(HttpEventRequest request) { log.log(Level.WARNING, e.getMessage(), e); return builder.build(); } - - //TODO: what headers to forward??? populateHeadersFromSource(sourceApiRequest, request, targetForSourceApiRequest); //setup request From c6d53b8c7b28e3da9a9238b814b13d2ac6907891 Mon Sep 17 00:00:00 2001 From: Erik Schultink Date: Fri, 4 Oct 2024 10:48:22 -0700 Subject: [PATCH 8/8] change placeholder warning to info; was confusing people in logs --- .../co/worklytics/psoxy/aws/ParameterStoreConfigService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/impl/aws/src/main/java/co/worklytics/psoxy/aws/ParameterStoreConfigService.java b/java/impl/aws/src/main/java/co/worklytics/psoxy/aws/ParameterStoreConfigService.java index c23f247de..137a27bad 100644 --- a/java/impl/aws/src/main/java/co/worklytics/psoxy/aws/ParameterStoreConfigService.java +++ b/java/impl/aws/src/main/java/co/worklytics/psoxy/aws/ParameterStoreConfigService.java @@ -115,7 +115,7 @@ Optional getConfigPropertyAsOptional(ConfigProperty property, Function r; if (Objects.equals(parameterResponse.parameter().value(), PLACEHOLDER_VALUE)) { - log.warning("Found placeholder value for " + paramName + "; this is either a misconfiguration, or a value that proxy itself should later fill."); + log.info("Found placeholder value for " + paramName + "; this is either a misconfiguration, or a value that proxy itself should later fill."); r = Optional.empty(); } else { if (envVarsConfig.isDevelopment()) {