S106 init by eschultink · Pull Request #1 · Worklytics/psoxy

eschultink · 2021-10-05T14:11:01Z

Initial push of working version into psoxy github repo. Includes:

various project files, including setup for repo (Github) / IDE (intellij)
terraform code to setup infra (with example config for a personal dev project)
CI test suite to cover terraform code (just init, validate) and java code (basic tests of pseudonymization/redaction of examples)
working proxy deployments for gmail/google-chat, with example invocations via curl

… ...

…onment

eschultink · 2021-10-05T15:25:08Z

java/configs/gmail.yaml

@@ -0,0 +1,3 @@
+SOURCE: gmail
+TARGET_HOST: gmail.googleapis.com
+OAUTH_SCOPES: https://www.googleapis.com/auth/gmail.metadata


these files fill environment variables for the cloud function. in essence, they are a config.

davidfq · 2021-10-06T13:57:59Z

README.md

+## Development
+
+Can run locally via IntelliJ + maven, using run config:
+  - `psoxy - run gmail  (located in `.idea/runConfigurations`)


missing backtick

aperez-worklytics

No blockers, just some comments. Impressive work, congrats!

aperez-worklytics · 2021-10-05T18:21:44Z

java/src/main/java/co/worklytics/psoxy/PrebuiltSanitizerOptions.java

+public class PrebuiltSanitizerOptions {
+
+   static final Sanitizer.Options GMAIL_V1 = Sanitizer.Options.builder()
+         .pseudonymization(Pair.of("\\/gmail\\/v1\\/users\\/.*?\\/messages\\/.*",


As we discussed and maybe not for the PoC, but that will be better if parametrized in files

aperez-worklytics · 2021-10-05T18:22:40Z

java/src/main/java/co/worklytics/psoxy/Pseudonym.java

+ */
+@Builder
+@Value
+public class Pseudonym {


Again and not for now, but we could create a shared package for pseudonym thing to we can reuse code for creating hashes between psoxy and worklytics

yes, probably sooner than later

aperez-worklytics · 2021-10-05T18:26:11Z

java/src/main/java/co/worklytics/psoxy/Route.java

+import java.util.stream.Collectors;
+
+@Log
+public class Route implements HttpFunction {


Well, function is not actually routing as we are transforming the results, I'd rename it as Sanitize

🤔 well, it does something beyond sanitizing:

route the request to the source API (removes the host + cloud function name; adds the source API host)

add authentication for source, as needed

I think Route is ok, other names could be Transponder (transmit and responds, more or less) or ApiCallTranslator(probably more accurate than router)

aperez-worklytics · 2021-10-05T18:27:10Z

java/src/main/java/co/worklytics/psoxy/Route.java

+        com.google.api.client.http.HttpRequest sourceApiRequest =
+            requestFactory.buildGetRequest(targetUrl);
+
+        //TODO: what headers to forward???


Maybe that can be included as part of the config

aperez-worklytics · 2021-10-05T18:35:05Z

java/src/main/java/co/worklytics/psoxy/Route.java

+            .setConnectTimeout(SOURCE_API_REQUEST_CONNECT_TIMEOUT)
+            .setReadTimeout(SOURCE_API_REQUEST_READ_TIMEOUT);
+
+        //q: add exception handlers for IOExceptions / HTTP error responses, so those retries


Actually it should happen in both but with different purpose... proxy should check the request and then worklytics should check the connection against proxy as any other external endpoint.
Proxy should return the status of the request to the source as it is done in L132; if after consuming its retries source request still not working I'll return a 500 or 502 + header. In Worklytics with that code and with the header we may know that we should not retry the connection again, as something happened.

aperez-worklytics · 2021-10-05T19:10:08Z

java/src/main/java/co/worklytics/psoxy/impl/SanitizerImpl.java

+
+    Configuration jsonConfiguration;
+
+    public void initConfiguration() {


Mmm so better if this is the class constructor instead of an init method that should be called by someone

aperez-worklytics · 2021-10-06T15:06:09Z

java/src/main/java/co/worklytics/psoxy/Route.java

+        // return response
+        response.setStatusCode(sourceApiResponse.getStatusCode());
+
+        if (sourceApiResponse.getStatusCode() == HttpStatusCodes.STATUS_CODE_OK) {


Maybe better with Response.Status.Family.familyOf(sourceApiResponse.getStatusCode()).equals(Response.Status.Family.SUCCESSFUL); as it will cover all 2xx status

which pkg is this from? base java, or we need to add a dependency?

this? https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/Response.html

that requires us to bring in the jax-rs API. not sure that's better than just checking the 200-300 range.

I do question whether it's what we want, as 204 is conventionally 'No Content' right, so will our attempt to parse content crash?

aperez-worklytics · 2021-10-06T15:15:12Z

java/src/main/java/co/worklytics/psoxy/Route.java

+        } else {
+            //write error, which shouldn't contain PII, directly
+            //TODO: could run this through DLP to be extra safe
+            sourceApiResponse.getContent().transferTo(response.getOutputStream());


Add log here to catch info about why request was not successful; the customer could check the function logs to have more details

aperez-worklytics · 2021-10-06T15:15:41Z

java/src/main/java/co/worklytics/psoxy/Route.java

+
+
+    GenericUrl buildTarget(HttpRequest request) {
+        String targetUri = "https://"


As Jose mentioned yesterday, better with a uriBuilder

k, i'll improve a bit by using URL - but URLBuilder (java) would require us to parse the query string for the request, and then add it back via addParam - seems pointless

URIBuilder is apache http-client, which isn't currently a dependency of this repo

aperez-worklytics · 2021-10-06T15:25:07Z

java/src/main/java/co/worklytics/psoxy/impl/SanitizerImpl.java

+
+        Pseudonym.PseudonymBuilder builder = Pseudonym.builder();
+        String canonicalValue;
+        //q: this auto-detect a good idea? Or invert control and let caller specify with a header


At the moment it is, I think. Let's see how this evolve in the future; probably as you say we could specify what we want as a header - and if not specified use the default, which is the one implemented here

jlorper · 2021-10-10T17:40:12Z

infra/dev-personal/main.tf

+
+  project_id                   = var.project_id
+  connector_service_account_id = "psoxy-gmail-dwd"
+  display_name                 = "Psoxy Connector - GMail Dev Erik"


Use developer name as variable? Whenever there is "Erik" replace (maybe done in other PR)

jlorper · 2021-10-10T17:43:37Z

infra/modules/gcp-secret-to-cloud-function/main.tf

+
+# todo needed bc as of Sept 2021, no way to expose secret via Cloud Function Maven plugin or
+# terraform
+resource "local_file" "todo" {


Use https://www.terraform.io/docs/language/resources/provisioners/local-exec.html ?

jlorper · 2021-10-10T17:44:27Z

infra/modules/gcp/README.md

+"per connection" basis (connection in this context is defined as the abstract concept of ongoing
+data import from a data source to a Worklytics account).


maybe worth to do a glossary markdown file?

jlorper · 2021-10-10T17:47:06Z

java/pom.xml

+        <gcpProjectId>psoxy-dev-erik</gcpProjectId>
+        <sourceApi>gmail</sourceApi>
+        <!-- serviceAccount that function will run as. For Google Workspace use cases, should be the
+         one configured as OAuth Client and authorized for your instance. -->
+        <serviceAccount>psoxy-gmail-dwd@psoxy-dev-erik.iam.gserviceaccount.com</serviceAccount>


all "erik" related stuff ok for ease development, but should be wiped out and use parameters in the future

jlorper · 2021-10-10T17:53:53Z

java/src/main/java/co/worklytics/psoxy/Route.java

+    final int SOURCE_API_REQUEST_CONNECT_TIMEOUT = 30_000;
+    final int SOURCE_API_REQUEST_READ_TIMEOUT = 300_000;


Given the amount, I assume are millis. But good to document or set as suffix

jlorper · 2021-10-10T17:54:58Z

java/src/main/java/co/worklytics/psoxy/Route.java

+    void initSanitizer() {
+        //TODO: pull salt from Secret Manager
+        sanitizer = new SanitizerImpl(
+            PrebuiltSanitizerOptions.MAP.get(getRequiredConfigProperty(ConfigProperty.SOURCE))
+            .withPseudonymizationSalt("salt")
+        );
+    }


Probably done already, but remember the static initializer to provide global context

jlorper · 2021-10-10T17:55:57Z

java/src/main/java/co/worklytics/psoxy/Route.java

+        String path =
+            request.getPath()
+                .replace(System.getenv(RuntimeEnvironmentVariables.K_SERVICE.name()) + "/", "")
+            + request.getQuery().map(s -> "?" + s).orElse("");


use UriBuilder

jlorper

Some comments, sorry for the late review

eschultink added 30 commits September 21, 2021 13:53

initial docs

543f902

initial .gitignore for IntelliJ

d31c788

repo layout for infra

f22c890

tweak README

882b386

more intellij config files, that jetbrains claims should be committed…

7aaf760

… ...

terraform modules to support GCP cloud function as personal dev envir…

7e3bbce

…onment

local working HelloWorld

c2199d3

add an .editorconfig

42bf256

wip of GET pass through to target host, plus SA auth

c4bad8e

run config w env var

2b4a956

rename yaml example

f4bc06a

ignore backup tf state

11f4a06

add 'salt' to use for pseudonymization

5f6f01c

service account + key to support gmail connector tests in local

9d66110

SA email as an output

ae10cfc

activate needed APIs as part of DWD connectors

4da2f6b

fix run config ref to credential

aac5b50

roundtrip working for worklytics local

486befd

tweak request settings, so appropriate for proxy context

baf856c

initial end-to-end with pseudonymization, working locally and in test

c7432e6

infra cleanup

32e17c0

add google-chat case; deployment to gcp working

a0cd15b

fix authentication in the cloud

144e91a

add some comments

9515fa6

pass through domains for emails

9793d7e

hash emails based on canonical form

05786b1

pr template

f04d38b

CI to compile java code

14c352a

CI for the java code

493332b

add CI for infra

051c7f7

eschultink requested a review from aperez-worklytics October 5, 2021 14:11

eschultink added 3 commits October 5, 2021 07:12

add comments; exclude domain if null

e96cc3e

clean unused methods

d9f0ab1

cleanup imports

b381080

eschultink mentioned this pull request Oct 5, 2021

s107 : use salt value from secret manager #2

Merged

docs on salt

3268d0b

eschultink commented Oct 5, 2021

View reviewed changes

eschultink added 4 commits October 5, 2021 18:51

more intuitive value for header

bb75269

parameterize function name

12b2dbf

doc cleanup

e6d89a3

better GCP instructions

13e62ca

davidfq reviewed Oct 6, 2021

View reviewed changes

eschultink added 3 commits October 6, 2021 07:33

pseudonym that can be converted both to and from JSON

171c8b9

dismiss idea of doing anything with personal names from emails

b0eae36

clearer naming Pseudonym - PseudonymizedIdentity; add scope

9e650e6

aperez-worklytics approved these changes Oct 6, 2021

View reviewed changes

eschultink added 4 commits October 6, 2021 08:29

clarify scope/domain semantics

620bd14

cleanup comments

44c2a39

cr fixes

3145819

typo

b4b6cf3

eschultink merged commit 1259c53 into main Oct 6, 2021

eschultink deleted the s106-init branch October 6, 2021 16:58

jlorper reviewed Oct 10, 2021

View reviewed changes


		Configuration jsonConfiguration;

		public void initConfiguration() {



		GenericUrl buildTarget(HttpRequest request) {
		String targetUri = "https://"

		"per connection" basis (connection in this context is defined as the abstract concept of ongoing
		data import from a data source to a Worklytics account).

		final int SOURCE_API_REQUEST_CONNECT_TIMEOUT = 30_000;
		final int SOURCE_API_REQUEST_READ_TIMEOUT = 300_000;

Conversation

eschultink commented Oct 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aperez-worklytics left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlorper Oct 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlorper left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jlorper Oct 10, 2021 •

edited

Loading