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

Do not pass raw RFC-8484 urls when make DOH override in IPFS #24706

Closed
cypt4 opened this issue Aug 16, 2022 · 4 comments · Fixed by brave/brave-core#14631
Closed

Do not pass raw RFC-8484 urls when make DOH override in IPFS #24706

cypt4 opened this issue Aug 16, 2022 · 4 comments · Fixed by brave/brave-core#14631

Comments

@cypt4
Copy link

cypt4 commented Aug 16, 2022

Kubo(go-ipfs) only supports POST DOH requests.
https://github.com/libp2p/go-doh-resolver/blob/master/request.go#L28
If DOH is overriden by url with {?dns} template then DNS resolution errors happens.
So we should clear the provided DOH url from {?dns} templates or not override DOH if such url is selected.

@cypt4 cypt4 added this to the 1.43.x - Beta milestone Aug 16, 2022
@cypt4 cypt4 added this to Web3 Aug 16, 2022
cypt4 added a commit to brave/brave-core that referenced this issue Aug 16, 2022
Fixes brave/brave-browser#24706
Currently Kubo doesn't support such template and only uses POST requests
@cypt4 cypt4 removed this from the 1.43.x - Beta milestone Aug 16, 2022
@ShivanKaul
Copy link
Collaborator

ShivanKaul commented Aug 16, 2022

@cypt4 we discussed this in privacy confab today but we didn't understand what the issue was
Taking a look at the PR, seems like it's simply replacing the {?dns} substring from the DoH server URL if it exists. It's confusing that the PR fixes two separate issues under one title.

@cypt4 cypt4 moved this to Done in Web3 Aug 17, 2022
@brave-builds brave-builds added this to the 1.44.x - Nightly milestone Aug 17, 2022
@cypt4 cypt4 added the QA/Yes label Aug 18, 2022
@cypt4
Copy link
Author

cypt4 commented Aug 18, 2022

@cypt4 we discussed this in privacy confab today but we didn't understand what the issue was Taking a look at the PR, seems like it's simply replacing the {?dns} substring from the DoH server URL if it exists. It's confusing that the PR fixes two separate issues under one title.

Yep, sorry for merging 2 issues in the one pr, this is made to push this changes to 1.43 faster.
I've fixed PR's description.

@kjozwiak
Copy link
Member

The above will require 1.43.81 or higher for 1.43.x verification.

@stephendonner stephendonner added bug QA/Test-Plan-Specified QA/In-Progress Indicates that QA is currently in progress for that particular issue labels Aug 24, 2022
@stephendonner
Copy link

Verified PASSED using

Brave 1.43.81 Chromium: 104.0.5112.102 (Official Build) (x86_64)
Revision 8e5396254975ef939f2ef7d0bd334e48a052b536-refs/branch-heads/5112@{#1478}
OS macOS Version 11.6.8 (Build 20G730)

Followed the steps to reproduce from brave/brave-core#14631 (comment).

Confirmed I was able to load ipns://ipfs.io

example example example example
Screen Shot 2022-08-24 at 2 14 38 PM Screen Shot 2022-08-24 at 2 18 42 PM Screen Shot 2022-08-24 at 2 17 11 PM Screen Shot 2022-08-24 at 2 17 24 PM

@stephendonner stephendonner added QA Pass-macOS and removed QA/In-Progress Indicates that QA is currently in progress for that particular issue labels Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment