search

ungoogled-software / ungoogled-chromium

Google Chromium, sans integration with Google
BSD 3-Clause "New" or "Revised" License
20.82k stars 846 forks source link

DNS fixes, clone patch update, and config cleanup #2970

Closed Ahrotahn closed 3 months ago

Ahrotahn commented 3 months ago

This PR contains a handful of smaller fixes:

PF4Public commented 3 months ago

But that wasn't the problem! The domain itself is!

rany2 commented 3 months ago

I have IPv6 connectivity and still had the .works issue. I don't think that was the cause of the problem.

rany2 commented 3 months ago

I noticed this about howhttps.works' AAAA responses, it returns a SERVFAIL. It seems like a DNSSEC issue on their end:

~ $ dig howhttps.works aaaa

; <<>> DiG 9.16.41 <<>> howhttps.works aaaa
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: SERVFAIL, id: 22456
;; flags: qr rd ra; QUERY: 1, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 1

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 512
; EDE: 12 (NSEC Missing): (Invalid denial of existence of howhttps.works/aaaa)
;; QUESTION SECTION:
;howhttps.works.                        IN      AAAA

;; Query time: 92 msec
;; SERVER: 8.8.8.8#53(8.8.8.8)
;; WHEN: Thu Aug 01 18:12:18 EEST 2024
;; MSG SIZE  rcvd: 99

~ $ dig nsa.gov aaaa

; <<>> DiG 9.16.41 <<>> nsa.gov aaaa
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 58717
;; flags: qr rd ra ad; QUERY: 1, ANSWER: 0, AUTHORITY: 1, ADDITIONAL: 1

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 512
;; QUESTION SECTION:
;nsa.gov.                       IN      AAAA

;; AUTHORITY SECTION:
nsa.gov.                1800    IN      SOA     w000-dsid-u05.nsa.gov. hostmaster.nsa.gov. 2024030527 3600 1080 2419200 900

;; Query time: 108 msec
;; SERVER: 8.8.8.8#53(8.8.8.8)
;; WHEN: Thu Aug 01 18:12:33 EEST 2024
;; MSG SIZE  rcvd: 97
PF4Public commented 3 months ago

it returns a SERVFAIL. It seems like a DNSSEC issue on their end

It is exactly what I wrote: https://github.com/ungoogled-software/ungoogled-chromium/issues/2963#issuecomment-2259120412

Ahrotahn commented 3 months ago

Yup! The core issue is some kind of misconfiguration on the site's end and that was exposed by our changes.

But it's also true that vanilla Chromium has no issue visiting the site, and it's also true that the patch causes the probe to always return true when the flag is not set. It should only be changing the value to false when the flag is set unless the goal is to remove the probe entirely.

If that is the case then we should look at removing patches/core/inox-patchset/0009-disable-google-ipv6-probes.patch since add-ipv6-probing-option.patch currently removes that change entirely.

PF4Public commented 3 months ago

unless the goal is to remove the probe entirely.

I thought that was the idea behind this change. May we have following options here: "IPv4", "IPv6", "probes enabled / default behaviour", and the last one if it would make any sense: "default behaviour, but without any probes"?

Ahrotahn commented 3 months ago

Sorry for the delay, I just returned from a trip a day ago.

I can remove the inox patch and we can keep things how they are since it would only affect misconfigured sites. If that site is the only one that's been found since the patch has been in place then I don't think it's too big of an issue.

Another option that keeps the probe removed would be to update the patch to have the result return false by default and change the flag to SetIpv6ProbeTrue. This should allow misconfigured sites to function correctly like in vanilla chromium while not changing behavior for anyone that still has SetIpv6ProbeFalse set. The only difference is that people on IPv6-only networks might have to set the the flag to function correctly.

PF4Public commented 3 months ago

I'm just trying to wrap my head around it and cannot make sense of it :(

[…] to have the result return false by default and change the flag to SetIpv6Probe True. […] for anyone that still has SetIpv6Probe False set.

It easily gets sooooo confusing. I suggest we change the name of the option to IPv6Reachability. With the following description: Forces the result of the browser's IPv6 probing (i.e. IPv6 connectivity test) to be successful without causing additional network activity. This makes IPv6 addresses to be prioritized over IPv4 addresses. With this flag disabled, the probing result is set to be unsuccessful, which causes IPv4 to be used over IPv6 when possible. ungoogled-chromium flag.. Enabled by default. (I think, it is what @Ahrotahn suggested, I'm just trying to rephrase it and make the statement more clear).

Why do we keep this chunk? Maybe remove it too?

  // Don't bother checking if the request will use WiFi and IPv6 is assumed to
  // not work on WiFi.
  if (!check_ipv6_on_wifi_ && RequestWillUseWiFi(target_network_)) {
    probing_ipv6_ = false;
    last_ipv6_probe_result_ = false;
    last_ipv6_probe_time_ = base::TimeTicks();
    return OK;
  }
Ahrotahn commented 3 months ago

Yeah, the whole function would be changed to the inside of that chunk and last_ipv6_probe_result_ would be set to the state of the flag.

For now I've updated the PR to keep the patch as it was but remove the redundant patch. I guess if we encounter more sites that cause problems we could change the flag, but I think it might cause too much confusion if we don't have stronger reasons to make that change.

rany2 commented 3 months ago

Can you make a -2 release?

Ahrotahn commented 3 months ago

Sure, I'll push the tag in a bit