Fix DoH Fallback Check and Short Study Implementation [chromium/src : main]
Brian Begnoche (Gerrit)
Brian Begnoche added 2 comments![]( "Open in Gerrit")
if (!context->ShouldAllowDohFallback()) {We've been talking about the fact that we don't want DoH probes and canary domain check to fire because of the unconditionally populated DoH config.
We could add this same check to NetworkContext::ActivateDohProbes, although it would only want to do that when `should_perform_doh_fallback_upgrade` is true. Maybe it can get to the DnsConfig already, or this needs to be surfaced via ResourceContext in some fashion.
And CanaryDomainService could do the same check via ResourceContext.
I think I would propose a new method that checks should_perform_doh_fallback_upgrade and ShouldAllowDohFallback to return whether DoH probes (including canary domain) should be active.
Something along those lines.
virtual void SetDohFallbackAllowed(bool allowed);Maybe this should only be added to ContextHostResolver.
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Williams (Gerrit)
Andrew Williams added 5 comments![]( "Open in Gerrit")
Posting some updates for knowledge sharing
if (secure_dns_mode != net::SecureDnsMode::kAutomatic) {I think for simplicity we should just check `safe_browsing::IsEnhancedProtectionEnabled(profile_prefs)` here, because all the other cases will be handled elsewhere... I've implemented it this way (but correct me if I'm wrong on this!)
void GeneratedSafeBrowsingPref::OnSafeBrowsingPreferencesChanged() {From chatting with Gemini, IIUC GeneratedSafeBrowsingPref is only used for the settings but this pref can change via other means as well, for instance via the pref syncing. I ended up monitoring for changes to the ESB prefs in ProfileNetworkContextService
if (!context->ShouldAllowDohFallback()) {We've been talking about the fact that we don't want DoH probes and canary domain check to fire because of the unconditionally populated DoH config.
We could add this same check to NetworkContext::ActivateDohProbes, although it would only want to do that when `should_perform_doh_fallback_upgrade` is true. Maybe it can get to the DnsConfig already, or this needs to be surfaced via ResourceContext in some fashion.
And CanaryDomainService could do the same check via ResourceContext.
I think I would propose a new method that checks should_perform_doh_fallback_upgrade and ShouldAllowDohFallback to return whether DoH probes (including canary domain) should be active.
Something along those lines.
Thanks, that makes sense. I don't know that NetworkContext::ActivateDohProbes is the right spot because the DnsConfig is not easily available there IIUC... I ended up adding it into DnsOverHttpsProbeRunner::ContinueProbe
virtual void SetDohFallbackAllowed(bool allowed);Maybe this should only be added to ContextHostResolver.
I tried this and it turns out that the per-profile NetworkContext uses more than just ContextHostResolver, specifically for tests. There's like three different classes that get used. I ended up adding the HostResolver method
Related details
- Brian Begnoche
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Williams (Gerrit)
Andrew Williams added 1 comment![]( "Open in Gerrit")
I ended up splitting this up into the following CLs:
- https://chromium-review.googlesource.com/c/chromium/src/+/7704626 - Move kForceSecureDnsDohFallback feature to net/
- https://chromium-review.googlesource.com/c/chromium/src/+/7723198 - [DoH] Remove fallback upgrade managed preference logic
- https://chromium-review.googlesource.com/c/chromium/src/+/7704042 - [DoH] Prevent use of fallback DoH provider when ESB disabled
- https://chromium-review.googlesource.com/c/chromium/src/+/7710806 - [DoH] Prevent DoH probes and canary domain check if pref disabled
- https://chromium-review.googlesource.com/c/chromium/src/+/7718918 - [DoH] Move kForceSecureDnsDohFallback to the network service
Thanks for putting this together!
Related details
- Brian Begnoche
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |