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

Added support to allow to track the resolution of DNS requests. #721

Merged
merged 16 commits into from
Sep 14, 2020

Conversation

turban1988
Copy link
Contributor

This pull request offers the option to log the resolution of DNS requests.
This allows one to track and detect, for example, CNAME cloaking or to easily determine the possible IP address used by Firefox (e.g., to detect violations to the GDPR or CCPA).

The implementation uses the browser.dns interface provided by Firefox and writes results provided by the interface into the dns_resolution table in the SQLite database.

@vringar vringar self-requested a review August 7, 2020 22:06
@vringar
Copy link
Contributor

vringar commented Aug 14, 2020

Hey,
thank you for sharing this code with us! And sorry for making you wait so long on a response.
We see the value in this instrument and we want to merge and maintain it, however there are a couple of things that keep us from merging the PR in its current state.

These things are:

  • Lack of tests
    We currently attempt to have at least a couple of basic tests for each instrument by having them write to SQLite and verify that the expected records are there
  • The arrow/parquet schema needs to be updated as well

Copy link
Contributor

@vringar vringar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you change the file permissions on so many files from 644 -> 755?

@vringar
Copy link
Contributor

vringar commented Aug 14, 2020

Please let us know if you want to make necessary changes yourself or if we should consider this code to be provided as is and any changes we want should be made by ourselves

@turban1988
Copy link
Contributor Author

Aside from the test cases, I made the required changes (see the new commits).

As for the tests, I am not sure how to design them as I am not very familiar with the OpenWPM test structure and the fact that DNS resolution is not always deterministic. One way could be to set up a local DNS server which produces deterministic outcomes. However, I am not sure if that will be too much overhead for the test.

@vringar
Copy link
Contributor

vringar commented Aug 17, 2020

Thank you for making these changes.

We unfortunately don't have a good test strategy but seeing as we are already spinning up a webserver and localstack, I don't think adding a local DNS resolver would be too much overhead.

I'll look around for some good solutions over the week and get back to you if I find something appropriate

Note: localtest.me always resolves to 127.0.0.1
If you want to test different domains consider using dnsmasq in combination with etc/host to achieve consistent results. However we should set this up as a test fixture, which I have never done before, which might delay the feature

@vringar
Copy link
Contributor

vringar commented Sep 11, 2020

Hey @turban1988 ,
I've added a test and created a PR against your repository.
I know this isn't good testing by any means but it's mostly aimed at helping us spot breakage early when updating Firefox and for that it should be good enough.

@vringar vringar self-requested a review September 14, 2020 08:43
@vringar vringar merged commit e6e4d46 into openwpm:master Sep 14, 2020
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 this pull request may close these issues.

2 participants