-
Notifications
You must be signed in to change notification settings - Fork 69
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
Feature/im tests #79
Feature/im tests #79
Conversation
|
||
Ability to detect if the WhatsApp instant messaging platform is working, by | ||
checking if the DNS resolutions are consistent and if it's possible to | ||
establish a TCP session with the IPs of the endpoints. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd s/session/connection/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
|
||
This test will check if 3 services are working as they should: | ||
|
||
1. The whatsapp endpoints |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the whatsapp endpoints?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hostnames, IP:port combinations it connects to when using the app.
|
||
When the `--all-endpoints` option is **not** specified we will be testing one | ||
of this endpoints picked at random, while when that option is specified we | ||
will be testing all of them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be useful to specify a specific endpoint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
|
||
To check if they work properly we will first do a DNS A lookup to the | ||
endpoint in question. We then check if any of the returned IPs (we ignore | ||
anything that is not an IPv4 or IPv6 address) are part of the address space |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get exactly the purpose of the parenthesis and especially I wonder why you expect IPv6 when you query for A... perhaps we query for ANY?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I should actually drop the mention of IPv6 since we would never get this back. The point here was that if we get back a CNAME we would ignore it.
|
||
As a reference point to know if a certain IP is part of the WhatsApp network | ||
we use the list of IP blocks published by WhatsApp here: | ||
https://www.whatsapp.com/cidr.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this list fetched as part of the test or distributed as a OONI input?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hardcoded inside of the test code itself. There is mention is the source at which time it was retrieved. We don't download it dynamically because if whatsapp.com is blocked then we will not be able to fetch it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good. I'd specify that inside the documentation.
|
||
```json | ||
{ | ||
"whatsapp_endpoints_status": "blocked" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing comma at end of line
|
||
For a given IP address we consider the connection successful if either connecting to `IP:443` or `IP:5222` succeeds. | ||
|
||
If ALL of the connections succeed then we consider the endpoint to no be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/no/not/
WhatsApp web is the service by which users are able to use WhatsApp from a | ||
web browser on their computer. | ||
When using WhatsApp web users scan a QR code displayed in the browser from | ||
their phone hence authenticating the web app. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say "to authenticate the web app"
|
||
For the service to work a user needs to have whatsapp be working properly | ||
from their phone (it needs to be unblocked there) and if the "Keep me signed | ||
in" option in unticked their phone needs to be connected to the internet and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"in unticked" -> "is unticked"
"registration_server_status": "blocked" | "ok" | null, | ||
|
||
"registration_server_failure": "FAILURE STRING (since 0.5.0)", | ||
"registratison_server_failure": "FAILURE STRING (note: up until 0.4.0 this was the name of the key)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say "... the name of the key was incorrectly spelled"
|
||
* edge-mqtt.facebook.com (`edge`) | ||
|
||
* external.xx.fbcdn.net (`external_cdn`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is xx
meant to be a number or literally xx
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's literally xx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol
```json | ||
{ | ||
"facebook_$ENDPOINT_NAME_reachable": true, | ||
"facebook_tcp_blocking": true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhm, this key is probably only present if some endpoints are blocked, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, if they are not blocked, then it's set to false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll follow up with talking because it's gonna be faster :)
"facebook-stun-reachable": null, (up to version 0.3.0) | ||
|
||
"facebook-tcp-blocking": true | false | null, (up to version 0.3.0) | ||
"facebook-dns-blocking": true | false | null, (up to version 0.3.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
beware of the indent here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some tweaks need to be applied before we can merge; I've added comments inline.
* master: (44 commits) Feature/im tests (#79) Feature/cloud fronting (#54) Remove informed-consent and test-descriptions from the ooni-spec repo Add some notes on release procedure Correct typo: probe_city referred to as probe_ip Broken link in `inform-users-long.md` (#70) oonib.md: remove a lie (#68) [oonib specification] Updates to the specification (#67) [oonib specification] clarify the meaning of content key (#66) Fix whitespace and broken links in data policy Rename web-connectivity to be test number 17 Fix broken links Wrote a description for the web-connectivity test (#61) Add draft of web_connectivity test (#48) Added web_connectivity and edited contact on risks.md Edited the contact address on data-policy.md Wrote the description for http request tests & created a file for it (#59) Fix links Fix links Minor edit to risks.md ...
This reverts commit a7a2979.
No description provided.