refresh keys from EKM except removed keys #4477

rrrooommmaaa · 2022-06-06T17:16:50Z

This PR sends a query to EKM for private keys on content scripts initialization

close #2602

Tests (delete all except exactly one):

Does not need tests (refactor only, docs or internal changes)
Difficult to test (explain why)
Not worth testing
Tests will be added later (issue #...)
Tests added or updated

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

addresses the issue it closes (if any)
code is readable and understandable
is accompanied with tests, or tests are not needed
is free of vulnerabilities
is documented clearly and usefully, or doesn't need documentation

test/source/mock/key-manager/key-manager-endpoints.ts

…m-ekm

…cript

…m-ekm

…ome authorization

…m-ekm

tooling/build-types-and-manifests.ts

lgtm-com · 2022-07-03T11:17:05Z

This pull request introduces 1 alert when merging 39228cb into 0a6bf7c - view on LGTM.com

new alerts:

1 for Incomplete regular expression for hostnames

…m-ekm

test/source/tests/setup.ts

tomholub

early review

extension/chrome/settings/setup/setup-key-manager-autogen.ts

extension/js/common/helpers.ts

rrrooommmaaa · 2022-07-11T12:31:07Z

tooling/bundle-content-scripts.ts

-  getFilesInDir(`${sourceDir}/common/core/crypto/smime`, /\.js$/, false),
-  getFilesInDir(`${sourceDir}/common/api/shared`, /\.js$/, false),
-  getFilesInDir(`${sourceDir}/common/api/key-server`, /\.js$/, false),
+  getFilesInDir(`${sourceDir}/js/common/platform`, /\.js$/, false),


I'm a bit baffled about how and when the directory got shifted.

Likely by one of the linked files importing some additional dependency that is outside /js folder. Is that possible?

Could likely find out by looking at the sourceDir directory at the moment it gets built, and see what other content is there outside of the js folder.

tomholub

From code, this looks correctly implemented, nevertheless I have some comments for improvements, for code clarity and also functionality. Thanks!

extension/js/common/helpers.ts

extension/js/background_page/background_page.ts

extension/js/content_scripts/webmail/setup-webmail-content-script.ts

…m-ekm

…key with valid lastModified

tomholub

I forgot to submit my review earlier, so here are some unsubmitted comments from before. From the look of it, I could probably merge it before these are resolved. I'll take one more look and potentially merge it now.

tomholub · 2022-07-28T18:24:29Z

extension/js/common/helpers.ts

+  if (passphrase === undefined && !existingKeys.length) {
+    return { needPassphrase: false, updateCount: 0 }; // return success as we can't possibly validate a passphrase
+    // this can only happen on misconfiguration
+    // todo: or should we throw?
+  }


Is this a situation in which the user has no keys set up on their account?

In this situation, we could allow any pass phrase to be used, as long as it has sufficient strength, like during setup. We could in fact probably just send that user to the setup screen where all the necessary UI is already present. What do you think? (it's exactly what we do on iOS anyway)

Yes, exactly, no keys set up is a misconfiguration.
Send the user from gmail page to setup? By changing window.location? Or opening another tab? I can try it.

You could try another tab, if no local keys & some remote keys exist now. That may work ok without upsetting the user.

tomholub · 2022-07-28T18:34:27Z

extension/js/content_scripts/webmail/setup-webmail-content-script.ts

+          const { privateKeys } = await keyManager.getPrivateKeys(idToken);
+          if (privateKeys.length) {
+            await processKeysFromEkm(acctEmail, privateKeys.map(entry => entry.decryptedPrivateKey), factory, ppEvent);
+          }


We should do the update in all cases, including when EKM returns 0 private keys. In that situation, all users keys should be removed from the browser extension. (they no longer have access to them).

It may cause the extension to misbehave, which we can improve later. But anyways, their keys should indeed be removed.

Oh, I was adhering to this specs: // stage 4 - modified key gets updated, removed key does not get removed in FlowCrypt/flowcrypt-ios#841

I wasn't very clear in the issue definition - sorry. I've tried to also point to FlowCrypt/flowcrypt-ios#1559 but may have been confusing. Anyway, I've filed an issue for it now at #4596

tomholub · 2022-07-28T18:35:41Z

extension/js/common/helpers.ts

+  return result;
+};
+
+export const processAndStoreKeysFromEkmLocally = async (


I haven't seen any code doing removals of private keys. When EKM no longer returns a particular key, the browser extension should forget the key too. Unless the the local version of that private key is revoked, in that case the extension should not forget the fact that it was revoked (and I suppose could keep the revoked private key too, if that's the simplest way to implement it).

Right, there aren't any removals, I was adhering to this specs: // stage 4 - modified key gets updated, removed key does not get removed in FlowCrypt/flowcrypt-ios#841
As for revocations, we are storing revoked keys' fingerprints in a separate table revocations forever (and optionally the revoked private keys there too), so no need to worry.

rrrooommmaaa added 2 commits June 6, 2022 17:13

wire EKM for live gmail test

7973cb7

fix tooling scripts

26155d2

tomholub reviewed Jun 6, 2022

View reviewed changes

test/source/mock/key-manager/key-manager-endpoints.ts Outdated Show resolved Hide resolved

rrrooommmaaa added 2 commits June 12, 2022 14:54

Merge remote-tracking branch 'origin/master' into issue-2602-pull-fro…

5c38087

…m-ekm

finished merge and refactoring

bc092e7

rrrooommmaaa force-pushed the issue-2602-pull-from-ekm branch from 36d3f0e to bc092e7 Compare June 13, 2022 06:53

rrrooommmaaa added 7 commits June 18, 2022 16:36

support and implementation of mocked test for setup-webmail-content-s…

b8ed25f

…cript

Added base private key for auto-updates

049a790

fix

068155d

Merge remote-tracking branch 'origin/master' into issue-2602-pull-fro…

b16f99a

…m-ekm

removed live test

2de153b

reverted debug constants

6cc4d64

tidying up

f49b12a

rrrooommmaaa force-pushed the issue-2602-pull-from-ekm branch from da83c05 to f49b12a Compare June 28, 2022 12:41

rrrooommmaaa added 4 commits June 28, 2022 15:03

use https://gmail.localhost:8001/gmail to avoid interference with Chr…

78421b5

…ome authorization

Renamed the object for test KM's updating key

e9f2d5f

Merge remote-tracking branch 'origin/master' into issue-2602-pull-fro…

f6bd98a

…m-ekm

wip

39228cb

github-advanced-security bot found potential problems Jul 3, 2022

View reviewed changes

tooling/build-types-and-manifests.ts Fixed Show fixed Hide fixed

rrrooommmaaa added 8 commits July 3, 2022 14:17

fixed regexes

7a4fe50

Displaying 'Account keys updated' toast

c2fef29

Merge remote-tracking branch 'origin/master' into issue-2602-pull-fro…

23001b9

…m-ekm

more thorough test control

aef6a30

more test cases

c87b391

Merge remote-tracking branch 'origin/master' into issue-2602-pull-fro…

408b9c7

…m-ekm

More test cases and EKM failure test

1bd33a3

Merge remote-tracking branch 'origin/master' into issue-2602-pull-fro…

d17e970

…m-ekm

rrrooommmaaa commented Jul 8, 2022

View reviewed changes

test/source/tests/setup.ts Outdated Show resolved Hide resolved

tomholub reviewed Jul 9, 2022

View reviewed changes

extension/chrome/settings/setup/setup-key-manager-autogen.ts Outdated Show resolved Hide resolved

extension/js/common/helpers.ts Outdated Show resolved Hide resolved

extension/js/common/helpers.ts Outdated Show resolved Hide resolved

extension/js/common/helpers.ts Show resolved Hide resolved

rrrooommmaaa added 4 commits July 9, 2022 14:24

Merge branch 'master' into issue-2602-pull-from-ekm

1c475ee

refactorings and better test setup

46a5872

lint fix

33adeb7

added test for EKM down

2296737

rrrooommmaaa commented Jul 11, 2022

View reviewed changes

niceties

b0d8316

rrrooommmaaa marked this pull request as ready for review July 11, 2022 12:38

tomholub reviewed Jul 15, 2022

View reviewed changes

Roman Shevchenko added 2 commits July 16, 2022 10:22

Merge remote-tracking branch 'origin/master' into issue-2602-pull-fro…

7529734

…m-ekm

refactoring

dbd86b3

rrrooommmaaa mentioned this pull request Jul 16, 2022

Refactor own contact's name update #4545

Open

Roman Shevchenko added 3 commits July 16, 2022 12:24

updated comments

e40a414

renamed options to ppOptions

b110e9f

allow overwriting oldKey with undefined lastModified property with a …

1acca8f

…key with valid lastModified

tomholub reviewed Aug 10, 2022

View reviewed changes

tomholub previously approved these changes Aug 10, 2022

View reviewed changes

Merge branch 'master' into issue-2602-pull-from-ekm

71e9943

tomholub dismissed their stale review via 71e9943 August 10, 2022 14:29

tomholub mentioned this pull request Aug 10, 2022

add support for removing private keys during EKM sync (and possibly recovering when syncing from 0 keys) #4596

Closed

fix

d1f00af

tomholub approved these changes Aug 10, 2022

View reviewed changes

tomholub enabled auto-merge (squash) August 10, 2022 14:35

tomholub changed the title ~~refresh keys from EKM~~ refresh keys from EKM except removed keys Aug 10, 2022

tomholub merged commit 415f80b into master Aug 10, 2022

tomholub deleted the issue-2602-pull-from-ekm branch August 10, 2022 14:46

refresh keys from EKM except removed keys #4477

refresh keys from EKM except removed keys #4477

Uh oh!

Conversation

rrrooommmaaa commented Jun 6, 2022 • edited by tomholub Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To be filled by reviewers

Uh oh!

Uh oh!

Uh oh!

lgtm-com bot commented Jul 3, 2022

Uh oh!

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomholub left a comment • 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!

tomholub Aug 10, 2022 • 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!

rrrooommmaaa Aug 10, 2022 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rrrooommmaaa commented Jun 6, 2022 •

edited by tomholub

Loading

tomholub left a comment •

edited

Loading

tomholub Aug 10, 2022 •

edited

Loading

rrrooommmaaa Aug 10, 2022 •

edited

Loading