Encrypted Flow - refactor encryption, use shared local datastore based on Google store, add debugging #61

holachuy · 2018-01-17T21:49:06Z

Encrypted Flow related major fixes #3

Refactored encryption to support secret, public, and private key encryption and decryption and handle errors
Use shared local datastore based on Google store for now, since the encrypted flow runs multiple processes (api and worker) that have to talk to the same DB
Add debugging throughout the encrypted flow process
Add uncaught exception handlers to catch, display, and exit on error

… and api are running in different process

…into local-datastore

holachuy · 2018-01-18T17:10:46Z

This is ready for review.

I did a pull with a rebase to merge the latest and it forced me to do several merges. Sorry for the noise.

seehamrun · 2018-01-18T16:09:54Z

config/environments/local/settings/common.yaml

@@ -1,10 +1,11 @@
 env: LOCAL
 cloud: LOCAL
+encryptedFlow: true


Do we want to turn this on by default for local dev? If so, we should update the documentation on how run locally.

Reverting. Yes, I thought in a follow up I'll enable and do the documentation, maybe once ported, unsure yet.

seehamrun · 2018-01-18T17:05:36Z

portability-core/src/main/java/org/dataportabilityproject/cloud/google/GoogleJobDataCache.java

@@ -35,7 +35,7 @@
 import java.util.concurrent.TimeUnit;
 import org.dataportabilityproject.cloud.interfaces.JobDataCache;



Nit: while we're in here, can we add a class comment ?

seehamrun · 2018-01-18T17:06:02Z

...ore/src/main/java/org/dataportabilityproject/cloud/google/GooglePersistentKeyValueStore.java

@@ -35,18 +37,13 @@
 import java.util.Map.Entry;
 import org.dataportabilityproject.cloud.interfaces.PersistentKeyValueStore;



Nit: class comment

seehamrun · 2018-01-18T17:06:58Z

portability-core/src/main/java/org/dataportabilityproject/cloud/local/LocalCloudFactory.java

 import com.google.common.base.Supplier;
 import com.google.common.base.Suppliers;
 import com.google.common.cache.CacheBuilder;
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
+<<<<<<< HEAD


Stray diff markers

seehamrun · 2018-01-18T17:07:43Z

portability-core/src/main/java/org/dataportabilityproject/job/CrypterFactory.java

@@ -0,0 +1,38 @@
+package org.dataportabilityproject.job;


License agreement missing

seehamrun · 2018-01-18T17:09:38Z

portability-core/src/main/java/org/dataportabilityproject/job/CrypterImpl.java

@@ -35,14 +36,19 @@ public String encrypt(String data) {
      byte[] encrypted = cipher.doFinal(data.getBytes(Charsets.UTF_8));
      return BaseEncoding.base64Url().encode(encrypted);
    } catch (BadPaddingException e) {
+      logger.error("BadPaddingException for data, length: {}", data.length(), e);


Out of curiosity, since we don't do anything different for these, (aside from the name of the exception), can we put them all in one catch?

Good point. Done.

seehamrun · 2018-01-18T17:10:20Z

portability-core/src/main/java/org/dataportabilityproject/job/CrypterImpl.java

@@ -61,14 +67,19 @@ public String decrypt(String encrypted) {
      System.arraycopy(decrypted, 8, data, 0, data.length);
      return new String(data, Charsets.UTF_8);
    } catch (BadPaddingException e) {
+      logger.error("BadPaddingException for data, length: {}", encrypted.length(), e);


Same here - can we put these in one catch?

seehamrun · 2018-01-18T17:23:52Z

.../src/main/java/org/dataportabilityproject/serviceProviders/google/GoogleServiceProvider.java

@@ -19,8 +19,11 @@
 import com.google.api.services.calendar.CalendarScopes;


Looks like these changes were introduced from my merge.

… etc

holachuy

I pushed the latest based on initial feedback, PTAL

rtannenbaum · 2018-01-18T20:04:40Z

portability-api/src/main/java/org/dataportabilityproject/webapp/CryptoHelper.java

@@ -53,15 +52,15 @@
   * Serialize and encrypt the given {@code authData} with the session key.


worker instance public key, not session key?

Done. Added more documentation and refactored to remove the public key encryption and just have the StartCopyHandler use it directly.

rtannenbaum · 2018-01-18T20:07:49Z

portability-core/src/main/java/org/dataportabilityproject/job/CrypterFactory.java

+/** Constructs instances of the {@link Crypter} for different key types. */
+public class CrypterFactory {
+
+  public static Crypter createCrypterForSecretKey(SecretKey key) {


Can you document that SecretKey is for symmetric keys?

rtannenbaum · 2018-01-18T20:11:47Z

portability-api/src/main/java/org/dataportabilityproject/webapp/PortabilityApiMain.java

 import com.google.inject.Guice;
 import com.google.inject.Injector;

 public class PortabilityApiMain {
  public static void main(String args[]) throws Exception {
+    Thread.setDefaultUncaughtExceptionHandler(UncaughtExceptionHandlers.systemExit());


do we want to do this in prod? won't it disrupt other jobs being handled by this API instance, especially if they are in the API polling stage?

Good point. I was debugging but in prod, we certainly don't want to exit. Added a warning message instead.

rtannenbaum · 2018-01-18T20:18:36Z

portability-api/src/main/java/org/dataportabilityproject/webapp/StartCopyHandler.java

-        cryptoHelper.encryptAuthData(publicKey, importAuthCookie));
+    logger.debug("Found publicKey: {}", publicKey.getEncoded().length);
+
+    String encryptedExportAuthData = cryptoHelper.encryptAuthData(publicKey, exportAuthCookie);


at this point is it the whole cookie or just credential that we're encrypting?

Good point, I clarified the variable name. It's just the cookie value, not the whole cookie.

rtannenbaum · 2018-01-18T20:21:46Z

portability-core/src/main/java/org/dataportabilityproject/cloud/local/LocalCloudFactory.java

-      Suppliers.memoize(InMemoryPersistentKeyValueStore::new);
+  // Google DataStore emulator is used for local development
+  private static final Datastore datastore = DatastoreOptions.newBuilder()
+      .setHost("http://localhost:8081")


should all ports be moved to yaml so we avoid conflicts?

rtannenbaum · 2018-01-18T20:23:06Z

portability-core/src/main/java/org/dataportabilityproject/cloud/local/LocalCloudFactory.java

@@ -46,21 +61,35 @@ public JobDataCache load(String service) throws Exception {
        }
      });

-  private static final Supplier<PersistentKeyValueStore> KEY_VALUE_SUPPLIER =
-      Suppliers.memoize(InMemoryPersistentKeyValueStore::new);
+  // Google DataStore emulator is used for local development


Nice find on the local data store! Does the code here check if it's running and print an error if not?

The database client code gives an error on startup if one isn't running.

rtannenbaum · 2018-01-18T20:30:21Z

portability-worker/src/main/java/org/dataportabilityproject/worker/WorkerImpl.java

 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;

 final class WorkerImpl {
-  private final Logger logger = LoggerFactory.getLogger(WorkerImpl.class);
+  private static final Logger logger = LoggerFactory.getLogger(WorkerImpl.class);
+  private static final Gson GSON = new Gson();


Do you think we should switch to Jackson since we are using that elsewhere?

I added a TODO for now to use the Jackson-based AuthData when it's checked in.

…ion by calling it directly from the StartCopyHandler.

…er to the api

holachuy

PTAL, all comments have been addressed. Thank you for the thorough review!

seehamrun · 2018-01-22T15:19:37Z

portability-worker/src/main/java/org/dataportabilityproject/worker/WorkerImpl.java

        if(true) {
-          logger.info("Exiting before copy, job: {}", job);
-          // TODO: Enable worker-based copying
+          logger.warn("Exiting before copy, job: {}", job);


Sorry if I missed this, could you explain why we cant enable this yet? iiuc, this causes us to skip the use of PortabilityCopier below right?

Done. It was mostly for debugging easily.

holachuy added 10 commits January 17, 2018 11:48

Use local google datastore emulator and add logging

15c0aac

DO NOT CHECKIN - setting encrypted flow to true

e78f949

Use local google datastore emulator for local development when worker…

f17a081

… and api are running in different process

use in memory datastore

ff07edd

debug and fixes

56bffb3

debugging in crypto classes

9ddadc3

rename crypto classes

aba8693

revert GoogleCloudFactory to master

a269d5e

encryption cleanup

d1ea77a

revert to master JWTTokenManager

5287865

holachuy requested review from seehamrun and rtannenbaum January 17, 2018 21:49

holachuy added 18 commits January 18, 2018 10:01

Use local google datastore emulator and add logging

f5db80b

DO NOT CHECKIN - setting encrypted flow to true

501db14

Use local google datastore emulator for local development when worker…

c9ff328

… and api are running in different process

use in memory datastore

74d2f76

debug and fixes

c416eaa

debugging in crypto classes

27d6ca3

rename crypto classes

10951ef

revert GoogleCloudFactory to master

2b97017

encryption cleanup

591bf7c

revert to master JWTTokenManager

20b82a6

Use local google datastore emulator and add logging

80c6bb7

Use local google datastore emulator for local development when worker…

55b446a

… and api are running in different process

debug and fixes

2a8e086

debugging in crypto classes

4d8ba01

rename crypto classes

626a2e5

revert GoogleCloudFactory to master

882c63c

encryption cleanup

3771187

revert to master JWTTokenManager

b682a3a

Merge branch 'local-datastore' of github.com:google/data-portability …

d6311d2

…into local-datastore

seehamrun requested changes Jan 18, 2018

View reviewed changes

holachuy and others added 2 commits January 18, 2018 13:48

Merge branch 'master' into local-datastore

7daa078

initial review changes: missing header, consolidate catch statements,…

0b2f32f

… etc

holachuy commented Jan 18, 2018

View reviewed changes

rtannenbaum reviewed Jan 18, 2018

View reviewed changes

holachuy added 4 commits January 19, 2018 11:46

Simplify naming of CrypterFactry creation factory and remove indirect…

7df0a0d

…ion by calling it directly from the StartCopyHandler.

document names better and add a non-existing uncaught exception handl…

4dfef9e

…er to the api

Document CrypterFactory better

e1a361e

add more documentation

a48357f

holachuy commented Jan 19, 2018

View reviewed changes

Merge branch 'master' into local-datastore

ae8d24a

rtannenbaum approved these changes Jan 20, 2018

View reviewed changes

seehamrun approved these changes Jan 22, 2018

View reviewed changes

holachuy merged commit 6663333 into master Jan 22, 2018

holachuy deleted the local-datastore branch January 25, 2018 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encrypted Flow - refactor encryption, use shared local datastore based on Google store, add debugging #61

Encrypted Flow - refactor encryption, use shared local datastore based on Google store, add debugging #61

holachuy commented Jan 17, 2018

holachuy commented Jan 18, 2018 •

edited

seehamrun Jan 18, 2018

holachuy Jan 18, 2018

seehamrun Jan 18, 2018

holachuy Jan 18, 2018

seehamrun Jan 18, 2018

holachuy Jan 18, 2018

seehamrun Jan 18, 2018

holachuy Jan 18, 2018

seehamrun Jan 18, 2018

holachuy Jan 18, 2018

seehamrun Jan 18, 2018

holachuy Jan 18, 2018

seehamrun Jan 18, 2018

holachuy Jan 18, 2018

seehamrun Jan 18, 2018

holachuy left a comment

rtannenbaum Jan 18, 2018

holachuy Jan 19, 2018 •

edited

rtannenbaum Jan 18, 2018

holachuy Jan 19, 2018

rtannenbaum Jan 18, 2018

holachuy Jan 19, 2018

rtannenbaum Jan 18, 2018

holachuy Jan 19, 2018

rtannenbaum Jan 18, 2018

holachuy Jan 19, 2018

rtannenbaum Jan 18, 2018

holachuy Jan 19, 2018

rtannenbaum Jan 18, 2018

holachuy Jan 19, 2018

holachuy left a comment

seehamrun Jan 22, 2018

holachuy Jan 22, 2018

		@@ -35,7 +35,7 @@
		import java.util.concurrent.TimeUnit;
		import org.dataportabilityproject.cloud.interfaces.JobDataCache;

		@@ -35,18 +37,13 @@
		import java.util.Map.Entry;
		import org.dataportabilityproject.cloud.interfaces.PersistentKeyValueStore;

		@@ -19,8 +19,11 @@
		import com.google.api.services.calendar.CalendarScopes;

		@@ -53,15 +52,15 @@
		* Serialize and encrypt the given {@code authData} with the session key.

Encrypted Flow - refactor encryption, use shared local datastore based on Google store, add debugging #61

Encrypted Flow - refactor encryption, use shared local datastore based on Google store, add debugging #61

Conversation

holachuy commented Jan 17, 2018

holachuy commented Jan 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holachuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holachuy Jan 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holachuy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holachuy commented Jan 18, 2018 •

edited

holachuy Jan 19, 2018 •

edited