Skip to content

search pubic keys of recipients on WKD #1201

New issue
New issue
Closed
#1399
Closed
search pubic keys of recipients on WKD#1201
#1399
Assignees
DenBond7
Milestone
1.2.0: Enterprise features

Description

@tomholub
tomholub
opened on Apr 28, 2021

part of #1298

trust it more than results on Attester (search first on WKD, then on Attester if WKD doesn't return public key)

  1. port Wkd class from browser extension done
  2. add tests for it independently of the rest of the codebase, merge the class unused done
  3. another PR to start using this class during public key lookups - this issue

The WKD class must be unaffected by disallow_attester_search_for_domains

Activity

  1. tomholub
    added this to the later milestone on Apr 28, 2021
  2. tomholub
    modified the milestones: later, soon on Jun 21, 2021
  3. tomholub
    modified the milestones: soon, 1.2.0: More OrgRules on Aug 10, 2021
  4. tomholub
    assigned
    DenBond7
    on Aug 10, 2021

  5. DenBond7 commented on Aug 10, 2021

    @DenBond7
    DenBond7
    on Aug 10, 2021
    Collaborator

    @tomholub Could you show me the logic that you use for the web extension? Where you use the WKD client to fetch pub keys.

  6. tomholub commented on Aug 10, 2021

    @tomholub
    tomholub
    on Aug 10, 2021
    CollaboratorAuthor

    Every single time when public key is pulled from external sources, it would use a general PubLookup class, which has lookupEmail(email: string) method on it.

    It's implemented here: https://github.com/FlowCrypt/flowcrypt-browser/blob/master/extension/js/common/api/pub-lookup.ts

    When the method is called:

    1. it will try pulling from WKD. If WKD returns any result, then the PubLookup method itself will return that result. If not, it will proceed to step 2
    2. try pulling from Attester. If attester returns any result, then the PubLookup method itself will return that result. Else, proceed to step 3
    3. return empty result

    In the code I linked above, there is also a third pubkey source internalSks which we will not be implementing on Android.

  7. tomholub commented on Aug 10, 2021

    @tomholub
    tomholub
    on Aug 10, 2021
    CollaboratorAuthor

    Inside the Attester class, disallow_attester_search_for_domains would be used so that it returns empty in case searching is disabled on that domain (or completely), meaning this setting does not affect searching on WKD.

  8. tomholub commented on Aug 10, 2021

    @tomholub
    tomholub
    on Aug 10, 2021
    CollaboratorAuthor

    I recommend to open above code in vscode so that you can explore it (with ctrl+click) and get a better understanding of where it is used (right click + see usages) or how it fits together.

  9. DenBond7 commented on Aug 10, 2021

    @DenBond7
    DenBond7
    on Aug 10, 2021
    Collaborator

    As I understand it relates to search by email only (for fingerprints we use attester). Right?

    I'd like to remind you what logic we use:

    1. User types user@example.test
    2. We search by email via attester
    3. User sends a message. We already have a pub key for user@example.test in the cache
    4. User creates a new message again
    5. User types user@example.test
    6. As we already have a pub key for user@example.test in the cache we make a request by fingerprint(via attester) to fetch the latest pub key to keep keys fresh

  10. tomholub commented on Aug 10, 2021

    @tomholub
    tomholub
    on Aug 10, 2021
    CollaboratorAuthor

    As we already have a pub key for user@example.test in the cache we make a request by fingerprint(via attester) to fetch the latest pub key to keep keys fresh

    The browser extension logic is here:

    https://github.com/FlowCrypt/flowcrypt-browser/blob/ecf83dda1cb272a1096321ab4ff244ca567364ca/extension/chrome/elements/compose-modules/compose-storage-module.ts#L141

    1. This should be using PubLookup as well - a general place to search for external public keys

    2. PubLookup.getFingerprint(fingerprint: string) should only try Attester, because WKD does not support pulling by fingerprint. However, disallow_attester_search_for_domains is set to ["*"] then attester should always return an empty result, even when searching by fingerprint. I'm not sure if I highlighted this before. Is this already implemented or should we first add that?

    3. That means, with disallow_attester_search_for_domains: ["*"] in client configuration, that keys will never get updated this way.

    I'll file an issue for 3) separately, as we still need a solution for WKD, and it will work by re-fetching by email. I thought we already did that on browser extension - will investigate, and then describe the logic on that issue too.

  11. DenBond7 commented on Aug 10, 2021

    @DenBond7
    DenBond7
    on Aug 10, 2021
    Collaborator

    However, disallow_attester_search_for_domains is set to ["*"] then attester should always return an empty result, even when searching by fingerprint. I'm not sure if I highlighted this before. Is this already implemented or should we first add that?

    It's already true for now = if disallow_attester_search_for_domains is set to ["*"] then attester always return an empty result

  12. tomholub commented on Aug 10, 2021

    @tomholub
    tomholub
    on Aug 10, 2021
    CollaboratorAuthor

    It's already true for now = if disallow_attester_search_for_domains is set to ["*"] then attester always return an empty result

    Even when calling by fingerprint? Just checking :-)

  13. DenBond7 commented on Aug 10, 2021

    @DenBond7
    DenBond7
    on Aug 10, 2021
    Collaborator

    It's already true for now = if disallow_attester_search_for_domains is set to ["*"] then attester always return an empty result

    Even when calling by fingerprint? Just checking :-)

    Yup :)

    flowcrypt-android/FlowCrypt/src/main/java/com/flowcrypt/email/api/retrofit/FlowcryptApiRepository.kt

    Line 113 in 4771ba0

    } else if (orgRules?.disallowLookupOnAttester() == true) {

  14. 15 remaining items

  15. DenBond7
    added 3 commits that reference this issue on Aug 11, 2021

    Renamed CreateMessageActivityTestTest to CreateMessageActivityTest.| #…

    cfdc190

    Refactored code to use PubLookup.| #1201

    3b75bd9

    Added CreateMessageActivityWkdTest(not completed). Refactored code.| #…

    ac073eb

  16. DenBond7 commented on Aug 12, 2021

    @DenBond7
    DenBond7
    on Aug 12, 2021
    Collaborator

    Just for history. To be able to test WKD need to execute the following

    adb root
adb shell "echo 1 > /proc/sys/net/ipv4/ip_forward"
adb shell "iptables -t nat -A PREROUTING -s 127.0.0.1 -p tcp --dport 443 -j REDIRECT --to 1212"
adb shell "iptables -t nat -A OUTPUT -s 127.0.0.1 -p tcp --dport 443 -j REDIRECT --to 1212"

    We run a mock web server on https://localhost:1212. For testing we need to route all traffic for localhost to localhost:1212

  17. DenBond7
    added 9 commits that reference this issue on Aug 12, 2021

    Added more tests. Refactored code.| #1201

    9ed061d

    Refactored code.| #1201

    4e9af72

    Added one more test. Refactored code.| #1201

    75bf2c0

    Modified ci-wait-for-emulator.sh to route all traffic for localhost t…

    1664798

    Modified PubLookup to get first matching key by 'usableForEncryption'…

    7ba8e44

    Switched to use "platforms;android-30" fro tests on CI.| #1201

    aa0d7fc

    Modified existed tests.| #1201

    81a102c

    Restored WkdClientTest with a few changes.| #1201

    7976139

    Refactored code.| #1201

    a7ed316
  18. tomholub
    closed this as completedin #1399on Aug 16, 2021
  19. tomholub
    added a commit that references this issue on Aug 16, 2021

    issue #1201 Integrated WKD in the app (#1399)

    b97d345
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions