Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

search pubic keys of recipients on WKD #1201

Closed
tomholub opened this issue Apr 28, 2021 · 24 comments · Fixed by #1399
Closed

search pubic keys of recipients on WKD #1201

tomholub opened this issue Apr 28, 2021 · 24 comments · Fixed by #1399
Assignees

Comments

@tomholub
Copy link
Collaborator

tomholub commented 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

@DenBond7
Copy link
Collaborator

DenBond7 commented Aug 10, 2021

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

@tomholub
Copy link
Collaborator Author

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.

@tomholub
Copy link
Collaborator Author

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.

@tomholub
Copy link
Collaborator Author

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.

@DenBond7
Copy link
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 [email protected]
  2. We search by email via attester
  3. User sends a message. We already have a pub key for [email protected] in the cache
  4. User creates a new message again
  5. User types [email protected]
  6. As we already have a pub key for [email protected] in the cache we make a request by fingerprint(via attester) to fetch the latest pub key to keep keys fresh

@tomholub
Copy link
Collaborator Author

As we already have a pub key for [email protected] 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.

@DenBond7
Copy link
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

@tomholub
Copy link
Collaborator Author

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 :-)

@DenBond7
Copy link
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 :)

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

@tomholub
Copy link
Collaborator Author

Ok, then you can follow #1201 (comment) - let me know and I'll be happy to clarify further.

@DenBond7
Copy link
Collaborator

Btw, does the WKD server always return a single pub key?

@tomholub
Copy link
Collaborator Author

Definitely not - it's an array of public keys, may be zero, one or more than one. Good question.

@DenBond7
Copy link
Collaborator

For now, our logic works just with a single key. Should I use the first matched?

@tomholub
Copy link
Collaborator Author

Ok but that's only a temporary solution.

All public keys returned from WKD should be processed, they should all be inserted or updated in local storage (comparing them by fingerprint), and when sending out a message, instead of choosing the best fitting public key like we did before, we should encrypt for all valid pubkeys that we have for that person. I'll file another issue.

@DenBond7
Copy link
Collaborator

All public keys returned from WKD should be processed, they should all be inserted or updated in local storage (comparing them by fingerprint), and when sending out a message, instead of choosing the best fitting public key like we did before, we should encrypt for all valid pubkeys that we have for that person. I'll file another issue.

in that case, we should firstly done the following #1188

@DenBond7
Copy link
Collaborator

One more question. As I understand WKD returns keys in RAW format. Is it possible to receive armored keys here or should I always expect RAW keys?

@tomholub
Copy link
Collaborator Author

They always must come in raw. If they cannot be parsed as raw, then the response from WKD is wrong.

@DenBond7
Copy link
Collaborator

They always must come in raw. If they cannot be parsed as raw, then the response from WKD is wrong.

Should I check that? Why I ask. PGPainless can parse keys from different sources. In most cases, I don't care what exactly source do I have. if PGPainless can't recognize keys I will receive nothing.

One more question. Should I check that the input contains only pub keys? (throw errors or filter only pub keys, etc.)

@tomholub
Copy link
Collaborator Author

Should I check that? Why I ask. PGPainless can parse keys from different sources. In most cases, I don't care what exactly source do I have. if PGPainless can't recognize keys I will receive nothing.

I guess you can be a bit slack here - if PGPainless parses a public key, then consider it valid.

One more question. Should I check that the input contains only pub keys? (throw errors or filter only pub keys, etc.)

Here we should not be slack. I would throw if a private key was found there.

@tomholub
Copy link
Collaborator Author

Do you intend on running your own WKD for testing? I don't recommend that - WKD can be trivially mocked. To mock it, you can get the right URL here https://metacode.biz/openpgp/web-key-directory for a particular email, and then serve the binary key on that URL.

@DenBond7
Copy link
Collaborator

Do you intend on running your own WKD for testing?

No, I don't. That was posted for the history. WKD logic is new for me. I've attached the info that can be useful to refresh info in my mind in the future.

@DenBond7
Copy link
Collaborator

DenBond7 commented Aug 11, 2021

Need to test the following

  • wkd advanced. has policy file && no result (wkd_advanced_no_result@localhost)
  • wkd advanced. has policy file && has single pub key(wkd_advanced_pub@localhost)
  • wkd advanced skipped. wkd direct. no policy file (wkd_direct_no_policy@localhost)
  • wkd advanced skipped. wkd direct. has policy file && no result(wkd_direct_no_result@localhost)
  • wkd advanced skipped. wkd direct. has policy file && has single pub key(wkd_direct_pub@localhost)
  • wkd advanced timeout. wkd direct available(wkd_advanced_timeout@localhost)
  • wkd advanced timeout. wkd direct timeout(wkd_advanced_direct_timeout@localhost)
  • wkd returns private key(wkd_prv@localhost)

https://stackoverflow.com/questions/28170004/how-to-do-local-port-forwarding-with-iptables

@DenBond7
Copy link
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

DenBond7 added a commit that referenced this issue Aug 12, 2021
DenBond7 added a commit that referenced this issue Aug 12, 2021
DenBond7 added a commit that referenced this issue Aug 13, 2021
DenBond7 added a commit that referenced this issue Aug 16, 2021
tomholub pushed a commit that referenced this issue Aug 16, 2021
* Integrated WKD support in the app. Temporary disabled some tests.| #1201

* Renamed CreateMessageActivityTestTest to CreateMessageActivityTest.| #1201

* Refactored code to use PubLookup.| #1201

* Added CreateMessageActivityWkdTest(not completed). Refactored code.| #1201

* Added more tests. Refactored code.| #1201

* Refactored code.| #1201

* Added one more test. Refactored code.| #1201

* Modified ci-wait-for-emulator.sh to route all traffic for localhost to localhost:1212. | #1201

* Modified PubLookup to get first matching key by 'usableForEncryption'. | #1201

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

* Modified existed tests.| #1201

* Restored WkdClientTest with a few changes.| #1201

* updated comment, renamed variable

* updated comment

* Refactored code.| #1201

Co-authored-by: Tom J <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants