feat: Better support of Gateway Subdomain spec #546

ppodolsky · 2022-11-23T12:47:00Z

Added basic subdomain support by uplifting parameters extraction from Host header required for handlers.
DnsLink part of spec is not supported in this PR.

Related to n0-computer/beetle#78

Spec:

HEAD, GET methods
DNSLink support
Redirection from Path methods to Subdomain methods (behaviour is configured by option)
X-Forwarded-Host support
X-Forwarded-Proto support

Current PR won't cover URI router, I believe it is better to implement in a separate PR.

iroh-gateway/src/handlers.rs

ppodolsky · 2022-11-25T13:06:05Z

Can't figure out if we should do anything here with the note from spec:

Note: Base36 must be used to ensure CIDv1 with ED25519 fits in a single DNS label (63 characters).

Decode and then encode IPNS name? Or what?

lidel · 2022-11-28T13:21:48Z

iroh-gateway/src/core.rs

+        let res = do_request(
+            "GET",
+            &format!("localhost:{}", test_setup.gateway_addr.port()),
+            "/ipns/k51qzi5uqu5dlvj2baxnqndepeb86cbk3ng7n3i46uzyxzyqj2xjonzllnv0v8",


nit: needs a separate test for legacy notation that was not a valid CID (e.g., 12D3KooWJHxkQKX8C5KAyqEPhn2ssT2in4TExyG9SXxi519tycL9)
See explainer in multiformats/cid-utils-website#48

This is important feature of subdomain gateway: when a request is made to example.com/ipns/id, subdomain set up on example.com will return HTTP 301 redirect to proper subdomain.
Before redirect is made, id is converted to cidv1-libp2p-key-base36.

Done

I had to add nasty from_peer_id function for dealing with discrepancy between multihash:0.16 and multihash:0.17. This func may be safely deleted after updating to multihash:0.17

cc @dignifiedquire Would you mind to release multiaddr 0.17 for bringing new multihash crate to rust-libp2p?

@vmx what't the status here?

@dignifiedquire I'm not maintaining rust-multiaddr, that's a question for @mxinden :)

Kicked off release process here. Please approve. multiformats/rust-multiaddr#66

lidel · 2022-11-28T13:25:45Z

iroh-gateway/src/core.rs

+        let res = do_request(
+            "GET",
+            &format!("localhost:{}", test_setup.gateway_addr.port()),
+            "/ipns/ipfs-subdomain.ipfs-domain.eth",


nit: change to en.wikipedia-on-ipfs.org? this is our usual test case for handling - (i suggest avoiding using .eth in tests unrelated to ENS interop).

Done.

Btw, while I was implementing code around - processing with regular expressions I had faced that it is hard or even impossible to properly process cases like aaaa-b-aaaa.com (a single symbol between 2 dashes) because captured groups were overlapping there (a-b and b-a with b belonging to both groups) and it is forbidden in Regex crate.

I mean, it may be useful to consider in E2E and other tests this case too cause it belongs to a separate equivalence class.

fwiw Kubo also started with regex, realized it comes with footguns, and replaced with a plain function :)

And it doesn't bring extra crate. You are right, dunno why I've decided regex would be better. It might be useful for complicated cases or expected future changes in format, but replacing dashes are definitely not this case.

iroh-gateway/src/core.rs

lidel · 2022-11-28T13:36:07Z

iroh-gateway/src/regex.rs

+pub static SUBDOMAIN_REGEX: Lazy<Regex> = Lazy::new(|| {
+    Regex::new(r"^(?P<cid_or_domain>[A-Za-z\d\-.]+)\.(?P<scheme>ip(?:ns|fs))\.(?P<hostname>[^.]+)$")
+        .unwrap()


Just to make sure there are no regressions in the future, add tests for edge cases where hostname is the same as scheme, e.g.:

bafy.ipfs.ipfs.com
bafy.ipns.ipfs.com
bafy.ipns.ipns.com
bafy.ipns.ipfs.com

Arqu · 2022-11-30T17:52:06Z

iroh-resolver/src/resolver.rs

@@ -47,6 +47,25 @@ pub struct Block {
    links: Vec<Cid>,
 }

+// ToDo: Replace to PeerId::from_str after updating multihash crate to 0.17


I'd like to have this handled before we merge.

I need a moment to fix it, but we should decide who will release multiaddr 0.17 and bring it through rust-libp2p and possibly other deps

Arqu

In regards to the gateway changes, this looks great. Regex is a bit scary looking.
Only thing I think we should do before the merge is resolve the multihash dependency.

ppodolsky · 2022-11-30T19:49:45Z

In regards to the gateway changes, this looks great. Regex is a bit scary looking. Only thing I think we should do before the merge is resolve the multihash dependency.

No need to scare, I replace regex with function :)

ppodolsky · 2022-12-10T08:27:32Z

Almost done, but should not merge until I set proper version of libp2p in Cargo.toml

dignifiedquire · 2022-12-13T18:25:35Z

I would love to merge this sooner rather than later, but I am not sure how long another libp2p release is out

ppodolsky · 2022-12-13T19:08:50Z

We may create an issue for cleaning things out after libp2p release and merge this PR as is. If it is ok, I'll prepare proper commits tomorrow

dignifiedquire · 2022-12-13T19:40:20Z

We may create an issue for cleaning things out after libp2p release and merge this PR as is. If it is ok, I'll prepare proper commits tomorrow

Yeah I think this makes the most sense, appreciated

ppodolsky · 2022-12-14T10:03:54Z

We may create an issue for cleaning things out after libp2p release and merge this PR as is. If it is ok, I'll prepare proper commits tomorrow

Yeah I think this makes the most sense, appreciated

Done. I've created reminding issue with "What-to-do" description
n0-computer/beetle#47

dignifiedquire · 2022-12-14T23:19:23Z

Cargo.toml

@@ -45,3 +45,6 @@ incremental = false

 [workspace.metadata.release]
 consolidate-commits = true
+
+[workspace.dependencies]
+libp2p = { version = "0.50" }


can you remove this from this PR here please, see discussion on #595

Arqu

Just some very minor comments.
Need to get CI to pass, otherwise I'm happy to merge.

Arqu · 2022-12-15T11:51:06Z

iroh-gateway/src/config.rs

@@ -48,6 +52,9 @@ pub struct Config {
    /// set of user provided headers to attach to all responses
    #[serde(with = "http_serde::header_map")]
    pub headers: HeaderMap,
+    /// Redirects to subdomains for path requests
+    #[serde(default = "set_true")]
+    pub redirect_to_subdomain: bool,


We should default this to false. This requires additional config on the DNS side and making sure certificates are valid for anyone hosting it. Since there's the burden of configuration anyways, we should have this off by default and those interested can then just enable it.

Arqu · 2022-12-15T11:54:16Z

iroh-gateway/src/constants.rs

@@ -1,6 +1,8 @@
 use axum::http::{header::HeaderName, HeaderValue};

 // Headers
+pub static HEADER_X_FORWARDED_HOST: HeaderName = HeaderName::from_static("x-forwarded-host");


This overlaps with #588, we should get both of these in. Think same tests are affected.

Let's merge this one firstly and then I'll rebase and refine left one

Arqu · 2022-12-15T12:00:20Z

iroh-gateway/src/client.rs

    pub format: ResponseFormat,
    pub cid: CidOrDomain,
    pub resolved_path: iroh_resolver::resolver::Path,
    pub query_file_name: String,
    pub download: bool,
    pub query_params: GetParams,
+    pub is_subdomain_mode: bool,


Nitpick, can we make this subdomain_mode

Arqu · 2022-12-15T12:01:23Z

iroh-gateway/src/handlers.rs

@@ -46,27 +53,56 @@ use crate::{
    templates::{icon_class_name, ICONS_STYLESHEET, STYLESHEET},
 };

+enum RequestPreprocessingResult {
+    ResponseImmediately(GatewayResponse),


Can we rename this to RespondImmediately is a little bit more readable.

ppodolsky · 2022-12-15T12:41:25Z

Argh, something happened during rebase, let me check what is wrong with the test

ppodolsky · 2022-12-15T13:20:28Z

Fixed tests

Arqu

LGTM!

* [feat] Support subdomains * pr: fixes * pr: fix test * fix: remove mut in iroh/src/run.rs

ppodolsky force-pushed the feature/subdomain-support branch from da4ddbc to 90c37d3 Compare November 23, 2022 13:09

ppodolsky changed the title ~~[WIP] [feat] Support subdomains~~ [feat] Support subdomains Nov 23, 2022

ppodolsky changed the title ~~[feat] Support subdomains~~ [feat] Support parsing Host header for aligning with Gateway Subdomain spec Nov 23, 2022

b5 added the c-gateway label Nov 23, 2022

b5 requested a review from Arqu November 23, 2022 14:27

dignifiedquire reviewed Nov 23, 2022

View reviewed changes

iroh-gateway/src/handlers.rs Outdated Show resolved Hide resolved

ppodolsky changed the title ~~[feat] Support parsing Host header for aligning with Gateway Subdomain spec~~ [feat] Better support of Gateway Subdomain spec Nov 23, 2022

ppodolsky changed the title ~~[feat] Better support of Gateway Subdomain spec~~ feat: Better support of Gateway Subdomain spec Nov 24, 2022

b5 assigned ppodolsky Nov 24, 2022

ppodolsky force-pushed the feature/subdomain-support branch from f084ea6 to e6806ad Compare November 28, 2022 09:28

lidel reviewed Nov 28, 2022

View reviewed changes

ppodolsky force-pushed the feature/subdomain-support branch from e6806ad to 6669a68 Compare November 29, 2022 11:50

ppodolsky mentioned this pull request Nov 30, 2022

[WIP] feat: Basic IPNS support #539

Closed

Arqu reviewed Nov 30, 2022

View reviewed changes

b5 mentioned this pull request Dec 6, 2022

deps!: Update multiaddr & multihash to 0.17.0 libp2p/rust-libp2p#3196

Merged

ppodolsky force-pushed the feature/subdomain-support branch from 4b58fd6 to 3ad58b7 Compare December 10, 2022 08:09

ppodolsky force-pushed the feature/subdomain-support branch 3 times, most recently from c9f6b0e to db13ab9 Compare December 14, 2022 10:03

ppodolsky force-pushed the feature/subdomain-support branch 3 times, most recently from 5053dfe to 1dd0e23 Compare December 14, 2022 12:54

dignifiedquire reviewed Dec 14, 2022

View reviewed changes

ppodolsky force-pushed the feature/subdomain-support branch 3 times, most recently from 6596d4e to 1e07829 Compare December 15, 2022 07:05

[feat] Support subdomains

b0076e8

ppodolsky force-pushed the feature/subdomain-support branch from 1e07829 to b0076e8 Compare December 15, 2022 07:09

Arqu requested changes Dec 15, 2022

View reviewed changes

pr: fixes

f50d262

pr: fix test

9f6ceec

ppodolsky force-pushed the feature/subdomain-support branch from 52916de to 9f6ceec Compare December 15, 2022 13:19

fix: remove mut in iroh/src/run.rs

dbcb206

Arqu approved these changes Dec 15, 2022

View reviewed changes

Arqu merged commit dfe3134 into n0-computer:main Dec 15, 2022

ppodolsky added a commit to izihawa/iroh that referenced this pull request Dec 28, 2022

feat: support of Gateway Subdomain spec (n0-computer#546)

4ce7115

* [feat] Support subdomains * pr: fixes * pr: fix test * fix: remove mut in iroh/src/run.rs

feat: Better support of Gateway Subdomain spec #546

feat: Better support of Gateway Subdomain spec #546

Conversation

ppodolsky commented Nov 23, 2022 • edited Loading

ppodolsky commented Nov 25, 2022

lidel Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

ppodolsky Nov 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arqu left a comment

Choose a reason for hiding this comment

ppodolsky commented Nov 30, 2022

ppodolsky commented Dec 10, 2022

dignifiedquire commented Dec 13, 2022

ppodolsky commented Dec 13, 2022

dignifiedquire commented Dec 13, 2022

ppodolsky commented Dec 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arqu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppodolsky commented Dec 15, 2022

ppodolsky commented Dec 15, 2022

Arqu left a comment

Choose a reason for hiding this comment

ppodolsky commented Nov 23, 2022 •

edited

Loading

lidel Nov 28, 2022 •

edited

Loading

ppodolsky Nov 29, 2022 •

edited

Loading

ppodolsky commented Dec 14, 2022 •

edited

Loading