-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat: gateway subdomains + http proxy mode #6096
Conversation
bb44c3c
to
5abd144
Compare
I'm trying to look at this by the lens of what potential user wants.
I think (A) should be "batteries included" and not require any configuration, (D) requires more advanced setup in DNS anyway, so it is ok for it to be an opt-in, and there is some vagueness about resolution rules when (B) and (C) happen to use the same hostname, which suggests there should be a way to make both configurable or simply do not allow that scenario on a single hostname. Having that as a starting point, some notes below (apologies, I failed to make this shorter). Simplify configuration of Gateway featuresI like the idea of having explicit list of hostnames that impacts the way requests are handled, but we should not hardcode any known Would it be more versatile and easier to reason about if we tweak ipfs/go-ipfs-config#30 and add three clear options to control gateway behavior? Something like (names are just placeholders):
Caveats:
Gateway HeuristicLocalhostIIUC there is a golden path for localhost:
Public GatewaysI feel I am missing some context on gateway symlinks mentioned here – what are use cases for that? So far I was under impression that mixing namespaces such as DNSLink website with Path Gateway is something we want to move away from. It makes security model unnecessarily hard to reason about and I did not see any use in production apart from That being said: If we allow Paths & DNSLink on the same hostnameWe use
If DNSLink always takes a priorityWe don't need
Personally I'd like to keep it simple and avoid mixing DNSLink with Path Gateway if possible, Configuration ExamplesDNSLink
Path Gateway
Subdomain Gateway
Thoughts? |
This comment has been minimized.
This comment has been minimized.
Unless I'm mistaken, I believe our proposals are mostly isomorphic. We might want a better name. Given:
This would be expressed in the current patch as:
The benefits of doing it this way are:
That's why I had a single list.
There's still an issue of hosting a DNSLink website and a subdomain gateway on the same hostname. In the original proposal, we'd redirect paths listed in
With symlink support, any DNSLink website could become a "gateway" by creating a symlink to /ipfs. The go-ipfs gateway would.
In general, we do. However, users may still want to have path-based gateways (possibly with javascript disabled via CSP) with a nice homepage. |
This should definitely be an option. |
Good point. |
Thinking about it a bit, I like the way you've split the gateway lists (path/subdomain). I'm just not entirely sure how to handle subdomain gateways + dnslink + redirects. |
|
@lidel LGTM! Go for it. |
My only issue with this format is that for most people the default config will be correct and so the host config for each host will just be an empty object. Maybe we could drop the word "Resolve" from "ResolvePaths", "ResolveDNSLink" etc.? Otherwise this LGTM! |
5abd144
to
6509e39
Compare
Initial success with Next steps:
Some fun screenshotsNote: Crossed padlock is Firefox-specific waiting for upstream fix, as noted in https://bugzilla.mozilla.org/show_bug.cgi?id=1220810#c23 |
8af259f
to
ba96bba
Compare
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.
Looking nice.
Updated The time for 🚲🏚️ is NOW ;-) |
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.
Review of the config documentation only. From what I can tell, the design looks good. I assume most of my comments are just bugs.
ccdbf99
to
c9dd755
Compare
(rebased and updated Next: finish writing tests (proxy+dnslink) |
This switches to peer.Decode as suggested in #6096 (comment) and adds tests for missing PeerID types. License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>
namesys/routing.go
Outdated
close(out) | ||
cancel() | ||
return out | ||
} |
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.
?
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.
We should probably have a test for this case as well.
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 remember removing this on purpose, we should restore this.
Digression: I'd do it myself but I need to fix my go dev env, go-ipfs requires go 1.14 now, and on my OS that requires kernel > 5.2 due to a known bug (golang/go#37436), so need to upgrade a few things 🙃
(commenting so we don't forget)
core/corehttp/hostname.go
Outdated
// Example: given "dist.ipfs.io.ipns.dweb.link": | ||
// 1. Lookup "link" TLD in knownGateways: negative | ||
// 2. Lookup "dweb.link" in knownGateways: positive | ||
for i := range labels { |
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.
for i := len(labels) - 1; i >= 2; i-- {
...
}
I've pushed a patch for this.
core/corehttp/hostname.go
Outdated
if err == nil { | ||
return host | ||
} | ||
// noop: this should never happen |
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.
How so? There may not be a port.
(we're implementing an _actual_ proxy) License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>
Instead of adding a new fake header (that could be spoofed by the client...), just read the original request URI from the request object. This also removes support for suborigins. They have never been implemented in browsers and it looks like efforts have stalled. We can add support back if we need it but, well, maintaining support was going to be more trouble than it was worth. License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>
Allows static DNSLink mappings with IPFS_NS_MAP. License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>
8287c2c
to
b805817
Compare
I've squashed this into reasonable commits. |
And I've updated go-ipfs-config to the release version. |
namesys/routing.go
Outdated
close(out) | ||
cancel() | ||
return out | ||
} |
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 remember removing this on purpose, we should restore this.
Digression: I'd do it myself but I need to fix my go dev env, go-ipfs requires go 1.14 now, and on my OS that requires kernel > 5.2 due to a known bug (golang/go#37436), so need to upgrade a few things 🙃
(commenting so we don't forget)
License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>
When request is sent to http://localhost:8080/ipfs/$cid response has HTTP 301 status code and "Location" header with redirect destination at $cid.ipfs.localhost:8080 Redirect is followed by browsersi, but not by commandline tools. Status 301 is ignored by curl in default mode: it will print response and won't follow redirect, user needs to add -L for that. To fix curl, we return correct payload in body of HTTP 301 response, but set Clear-Site-Data header to ensure Origin sandbox can't be abused. This requires a surgical workaround: If Location header is present in ResponseWriter's Header map, we ensure http.ServeContent() returns HTTP 301 Context: #6982 License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>
1cf8077
to
05fe308
Compare
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.
Approving because @Stebalien can't approve PR he initially created 🙃
I can't wrap my mind around this currently, so I just kindly ask you not to forget the idea of @Stebalien about permanent links if it's appropriate here:
Source: ipld/specs#19 (comment) If it's not appropriate, just ignore me. |
This change won't conflict with that feature. |
In the screenshots above I see Could you, please, elaborate why won't there be a conflict: will be this service launched on special port (e.g. 8030) different from permanent mutable links? |
We'd use one of the proposals in the comment you linked: either (a) use a different subdomain |
This change adds support for DNSLink subdomains on localhost gateway (ipfs/kubo#6096) Example: en.wikipedia-on-ipfs.org.ipfs.localhost:8080 BREAKING CHANGE: `isIPFS.subdomain` now returns true for <domain.tld>.ipns.localhost BREAKING CHANGE: `isIPFS.subdomainPattern` changed License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>
* feat: support DNSLink subdomains This change adds support for DNSLink subdomains on localhost gateway (ipfs/kubo#6096) Example: en.wikipedia-on-ipfs.org.ipfs.localhost:8080 BREAKING CHANGE: `isIPFS.subdomain` now returns true for <domain.tld>.ipns.localhost BREAKING CHANGE: `isIPFS.subdomainPattern` changed * test: support peer multiaddr with /p2p/ Context: libp2p/libp2p#79 * fix: explicitly ignore URL param and hash .url and .path now return true when validating: https://ipfs.io/ipfs/<CID>?filename=name.png#foo * refactor: simplify dnslinkSubdomain License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org> * fix: url() check should include subdomain() When .url was created we only had path gateways. When .subdomain was added, we did not update .url to test for subdomain gateways, which in the long run will confuse people and feels like a bug. Let's fix this: .url() will now check for both subdomain and path gateways #32 (comment) BREAKING CHANGE: .url(url) now returns true if .subdomain(url) is true * refactor: merge DNSLink check into ipnsSubdomain() This makes subdomain checks follow what path gateway checks do, removing confusion. In both cases (IPNS and DNSLink) user needs to perform online record check, so this is just a handy way of detecting potential matches. * docs: update examples * refactor: switch to iso-url * refactor: lint-package-json * chore: update deps License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>
Seems to break on Linux => #7290 |
feat: gateway subdomains + http proxy mode This commit was moved from ipfs/kubo@0114869
Depends on ipfs/go-ipfs-config#30
What does this do?
http://ipfs.io/ipfs/...
to the local gateway without redirecting (or touching the URL bar).http://CID.ipfs.localhost:8080
.fixes #5982
fixes #6498
cc ipfs/in-web-browsers#89, ipfs/ipfs-companion#667