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

Add support for DNSLink resolution #8068

Merged
merged 1 commit into from
Mar 10, 2021
Merged

Add support for DNSLink resolution #8068

merged 1 commit into from
Mar 10, 2021

Conversation

spylogsster
Copy link
Contributor

@spylogsster spylogsster commented Feb 24, 2021

Added an Open in IPFS option for domains that have a TXT record that is prefixed with dnslink=

Resolves brave/brave-browser#13609
Resolves brave/brave-browser#13611

Submitter Checklist:

  • Requested security review https://github.com/brave/security/issues/332
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

  • Navigate to brantly.eth.link and en.wikipedia-on-ipfs.org, on both you should see Open in IPFS option in address bar only if IPFS mode is local or gateway
  • Click by button will open ipns://[domain] url in new tab, if you have any path in url, it should be added as well
  • This should work for all domains that have dnslink= in the TXT dns record and x-ipfs-path HTTP header is present
  • This should work if page returned code different from OK 200 or failed due to network/DNS
  • This must not work in private windows or in Tor windows
  • Please note it doesnt work for me of I use Secure DNS "with your current service provider" as there are no available dns serviers. It may be same on your side

@spylogsster spylogsster requested a review from bbondy February 24, 2021 18:34
@spylogsster spylogsster self-assigned this Feb 24, 2021
@spylogsster spylogsster force-pushed the dnslink-fetcher branch 5 times, most recently from 167ab4c to e3157da Compare February 24, 2021 21:49
@diracdeltas
Copy link
Member

to avoid an extra DNS request (at least for users who haven't yet opted into IPFS), i wonder if it's possible to look up this TXT record in the DNS cache instead? maybe https://source.chromium.org/chromium/chromium/src/+/master:net/dns/host_cache.h?q=dns%20cache&ss=chromium ?

@spylogsster
Copy link
Contributor Author

spylogsster commented Feb 25, 2021

to avoid an extra DNS request (at least for users who haven't yet opted into IPFS), i wonder if it's possible to look up this TXT record in the DNS cache instead? maybe https://source.chromium.org/chromium/chromium/src/+/master:net/dns/host_cache.h?q=dns%20cache&ss=chromium ?

This cache should be used inside HostResolver, https://source.chromium.org/chromium/chromium/src/+/master:net/dns/host_resolver_manager.h;l=56?q=HostResolverManager&ss=chromium%2Fchromium%2Fsrc,

Added flag to allow cache usage in parameters and changed code to use one resolver instance per tab helper, but in my tests with Wireshark I see DNS requests for same sites anyway. I will check this logic, probably would be better to create a separate issue for this

@spylogsster spylogsster force-pushed the dnslink-fetcher branch 2 times, most recently from 0b1a194 to c5640d7 Compare February 25, 2021 17:09
@spylogsster spylogsster force-pushed the dnslink-fetcher branch 5 times, most recently from 7ff961f to 1a3e6c0 Compare February 25, 2021 20:38
@spylogsster spylogsster force-pushed the dnslink-fetcher branch 2 times, most recently from 204222b to 93f6d9f Compare February 26, 2021 08:36
@spylogsster spylogsster requested review from bridiver and a team as code owners March 1, 2021 10:56
#include "chrome/browser/ui/browser_finder.h"
#include "chrome/browser/ui/layout_constants.h"
#include "chrome/browser/ui/views/toolbar/toolbar_ink_drop_util.h"
#include "components/grit/brave_components_resources.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we're missing some deps here, too. This target has check_includes = false so you have to manually check unfortunately

@spylogsster spylogsster force-pushed the dnslink-fetcher branch 12 times, most recently from 368c74e to d004e22 Compare March 5, 2021 13:35
@spylogsster spylogsster requested a review from bridiver March 8, 2021 06:22
@spylogsster spylogsster force-pushed the dnslink-fetcher branch 2 times, most recently from f8e679f to 96182d7 Compare March 9, 2021 06:11
13609: Added an Open in IPFS option for domains that have a TXT record that is prefixed with dnslink=
13611: Add a setting to automatically redirect for DNSLink
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.

Add a setting to automatically redirect for DNSLink Support DNSLink resolution
4 participants