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

Revisiting dnslink support with X-Ipfs-Path #548

Closed
fiatjaf opened this issue Jul 31, 2018 · 6 comments · Fixed by #558
Closed

Revisiting dnslink support with X-Ipfs-Path #548

fiatjaf opened this issue Jul 31, 2018 · 6 comments · Fixed by #558
Assignees
Labels
kind/discussion Topical discussion; usually not changes to codebase

Comments

@fiatjaf
Copy link

fiatjaf commented Jul 31, 2018

Instead of doing TXT DNS request for every site when the "dnslink" option is set, why not listen for the .onHeadersReceived events (since these are already being listened for anyway) and look for the X-Ipfs-Path header?

@fiatjaf
Copy link
Author

fiatjaf commented Jul 31, 2018

Improved flow suggestion, after conversation with @lidel on IRC.

                   +---------------------------+
                   | is there a cached dnslink |
         +---------+ for the domain?           |
         |         +----------+----------------+
   yes   |                    |
         v                no  v

+--------------+        +------------------+
|load page from|        |load page normally|
|ipfs          |        +------+-----------+
++-------------+               |
 |                      +------+----+
 |                      |did it 404?|
 |                      +----+------+
 |  yes                      |
 |                           |
 |                           |   no
 |  +--------------+         |
 |  |look for a    |         |
 |  |dnslink in    |         |  +----------------------+
 +> |the TXT record|         +> |show page, but inspect|
    +--------------+            |the response headers  |
                                +-------+--------------+
                                        |
                                +-------+---------------+
                    +-----------+is there a X|Ipfs|Path?|
                    |           +-------+---------------+
                    |                   |
                    |                   |
                   yes               no v
                    |
                    v               +--------+
                                    |ok, done|
                 +-----------+      +--------+
                 |cache the  |
                 |dnslink for|
                 |the domain |
                 +-----------+

@lidel lidel added the kind/discussion Topical discussion; usually not changes to codebase label Jul 31, 2018
@lidel
Copy link
Member

lidel commented Jul 31, 2018

Thank you for sharing. It is an interesting idea, but I see two problems with it:

  1. AFAIK presence of X-Ipfs-Path header is not the same as dnslink. Header indicates the IPFS address of requested asset, without guarantees that anything else on the domain is IPFS-related. We can't use it as a global flag equal to dnslink, as there is a risk of breaking websites that choose to use IPFS only for some assets.

  2. webRequest.onHeadersReceived is just too late:

    • it relies on at least one HTTP request to succeed, which means
      • if HTTP server is down, website wont work, even tho it is available on IPFS
      • does not work for initial request(s). Those will be loaded over HTTP, following ones will get redirected to IPFS, which means mixing asset origins and introducing surface for breakage.

It just not reliable enough, I am afraid.

@lidel lidel closed this as completed Jul 31, 2018
@fiatjaf
Copy link
Author

fiatjaf commented Jul 31, 2018

Can we discuss this a little more before closing the issue?

My entire point here is that the optional usage of dnslink actually is a no-usage, even more if you consider the frightening warning in the options page. If we could come up with a solution that isn't as slow as the current dnslink stuff so that it could be enabled by default that would be good overall.


without guarantees that anything else on the domain is IPFS-related

Fair enough, but the presence of X-Ipfs-Path could then trigger a full DNS request (via ipfs name resolve?), which is much better than
a. doing that DNS request on every domain, which is known to slow down the browsers; and
b. not doing it at all, which is the default behavior I would bet 99% of the users leave unchanged.

if HTTP server is down, website wont work, even tho it is available on IPFS

If the HTTP server is down, currently the website won't work anyway, as the dnslink setting will be disabled;

mixing asset origins and introducing surface for breakage

If it doesn't work for initial requests, then it could check the dnslink and redirect all requests to the local IPFS daemon as soon as onHeadersReceived is received and the presence of X-Ipfs-Path is detected. This is much better than checking the dns link for all domains.

webRequest.onHeadersReceived is just too late

Well, in this other issue you've said that "we only need to read response headers before connection is aborted, the overhead is minimal and I believe it is worth it" when talking about onHeadersReceived, so I guess I can answer that the overhead is minimal 😛

@lidel
Copy link
Member

lidel commented Jul 31, 2018

(Revisiting after a nap, jump to the end for quick summary)

I am sorry, bit jet-lagged today. I really appreciate this discussion, thank you for your ideas!
Reopening and renaming this issue to better reflect its contents.

[..] the presence of X-Ipfs-Path could then trigger a full DNS request (via ipfs name resolve?),

This is quite interesting apprach, but comes with some trade-offs.
Quick thoughts:

  • removes performance hit, no need to send 1 DNS lookup per every new host, we would do it only for clearly IPFS-enabled ones, but...
    • ..what if website owner runs go-ipfs behind ngnx that sanitizes headers and removes X-Ipfs-Path ?
      • Potential solution: we could add explicit note to our docs that X-Ipfs-Path header is mandatory for every asset on dnslink-ed website.
    • ..what if HTTP server is down? webRequest.onErrorOccurred does not allow for returning a redirect-on-error, which means HTTP failure will break page load.
      • Potential solution: always do dnslink lookup in onErrorOccurred and if dnslink is found display browser action popup with message "HTTP transport failed, would you like to retry with IPFS?" that can be clicked and opens IPNS version of website.

which is much better than:
a. doing that DNS request on every domain, which is known to slow down the browsers;

Yes, this is the pain point. Right now dnslink experiment enables DNS TXT lookups via HTTP API: http://127.0.0.1:5001/api/v0/dns/docs.ipfs.io
That being said, the note about performance impact does not hold that much these days, as long as you are running a localhost node. We are caching lookups, so potential slowdown happens only on first load.

I really hoped we would have a WebExtension API for running efficient DNS TXT lookups by now, that would remove performance factor entirely, but that issue is still open and won't happen any time soon.

I agree we could try alternative approach to dnslink before that API lands.

b. not doing it at all, which is the default behavior I would bet 99% of the users leave unchanged.
[..] If the HTTP server is down, currently the website won't work anyway, as the dnslink setting will be disabled

Note that dnslink experiment is off by default on purpose. I understand it is frustrating it is not on by default, but we want to get it right before we enable it for all users. That includes accounting for all edge cases, including HTTP server being down.

[..] it could check the dnslink and redirect all requests to the local IPFS daemon as soon as onHeadersReceived is received and the presence of X-Ipfs-Path is detected. This is much better than checking the dns link for all domains.

Agreed. 👍

The only missing pieces in this approach are:

  • making X-Ipfs-Path mandatory
  • handling "HTTP server down" case (potential solution mentioned earlier)

webRequest.onHeadersReceived is just too late
Well, in this other issue you've said that "we only need to read response headers before connection is aborted, the overhead is minimal and I believe it is worth it" when talking about onHeadersReceived, so I guess I can answer that the overhead is minimal 😛

Yes, overhead is not a problem here, it is smaller than HTTP-based DNS TXT lookup done by current dnslink implementation.

What i meant by "just too late", is that onHeadersReceived means that one initial HTTP request was sent, so:

  • we need to account for "HTTP server down" case
  • we need to admit there is a potential privacy leak that comes with initial request over legacy HTTP stack (but we can live with it until proper DNS lookup API lands).

TL;DR

Oh wow, I should make this shorter, but don't have time, so let me just summarize:

  • I am okay with introducing dnslink support based on webRequest.onHeadersReceived+X-Ipfs-Path
    • Initial request can be cancelled and redirected in onHeadersReceived, so no mixed-origin issues
    • It should be safe enough to ship companion with it enabled by default
  • If efficient dnslookup API lands in webextensions at some point in future, we can remove onHeadersReceived version
  • We probably should keep the original dnslink experiment as opt-in, for people who want to avoid sending that initial HTTP request or don't want to use X-Ipfs-Path

@lidel lidel reopened this Jul 31, 2018
@lidel lidel changed the title Watch for "X-Ipfs-Path" header on .onHeadersReceived? Revisiting dnslink support Jul 31, 2018
@lidel lidel added the UX label Jul 31, 2018
@lidel lidel self-assigned this Jul 31, 2018
@lidel
Copy link
Member

lidel commented Aug 7, 2018

Side question: should X-Ipfs-Path in onHeadersReceived be enough to cancel and redirect as a new request to local gateway, even if there is no dnslink?

This is a scenario when someone exposes /ipfs/{CID}/ of HTTP gateway as / using reverse proxy, blocking loading arbitrary CIDs, but does not mind using IPFS for loading the site itself.

My initial intuition is that it is okay: it would be just what we do right now with /ipfs/{CID}/foo paths, but instead of URL pathname we would inspect X-Ipfs-Path header. As an opt-out mechanizm website owner could always filter out X-Ipfs-Path.

Thoughts?

@lidel lidel changed the title Revisiting dnslink support Revisiting dnslink support with X-Ipfs-Path Aug 7, 2018
@lidel
Copy link
Member

lidel commented Aug 27, 2018

I just went with it and implemented DNSLink policy that takes advantage of X-Ipfs-Path and is enabled by default.

More details in PR #558

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/discussion Topical discussion; usually not changes to codebase
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants