Prevent use of fallback DoH provider when ESB disabled [chromium/src : main]
Andrew Williams (Gerrit)
Andrew Williams added 1 comment![]( "Open in Gerrit")
Kenichi, can you take a look at this overall?
Javier, can you take a look at the ESB pref tracking logic in ProfileNetworkContextService to confirm this will sufficiently track the state of whether a given profile has ESB enabled?
Takashi, PTAL for owners approval of tools/metrics/histograms/metadata/net/enums.xml
Thanks everyone!
Related details
- Chromium IPC Reviews
- Javier Castro
- Kenichi Ishibashi
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: d...@chromium.org, toyo...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): d...@chromium.org, toyo...@chromium.org
Reviewer source(s):
d...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Dominic Farolino
- Javier Castro
- Kenichi Ishibashi
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi added 8 comments![]( "Open in Gerrit")
Thank you for working on this, mostly bikeshedding.
void UpdateDohFallbackAllowedForContext();What does this "ForContext" mean? Simply name UpdateDohFallbackAllowed()?
void UpdateDohFallbackAllowedForContext();As I wrote in the other comment, the term "fallback" is confusing. We use "fallback" to refer to both the DoH provider fallback and the act of falling back to insecure (in the DNS stack).
Can we figure out somewhat less confusing terms?
void SetDohFallbackAllowedForContext(bool allowed) override;Ditto. Since ContextHostResolver is already per context, "ForContext" might be redundant.
if (context->IsDohFallbackConfigPresent() &&The term 'fallback' is indeed overloaded here (referring to both the DoH provider type and the act of failing back to insecure). This logic correctly prioritizes the ESB-based restriction.
void set_doh_fallback_allowed_for_context(bool allowed) {By `for_context`, do you want to distinguish per-profile vs per-context here? Or we may just remove it?
bool doh_fallback_allowed_for_context = false;Ditto.
SetDohFallbackAllowedForContext(bool allowed);Ditto.
Related details
- Andrew Williams
- Dominic Farolino
- Javier Castro
- Takashi Toyoshima
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. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 4 comments![]( "Open in Gerrit")
fallback-to-default-DoH-provider logic for Enhanced Safe Browsing users,may say "ESB: Enhanced Safe Browsing" for other references.
Can we add some context in a function comment, as this is net/ in //Chrome/. So, it's nice to have more kind comments for other developers.
pref_change_registrar_.Add(
prefs::kSafeBrowsingEnhanced,
base::BindRepeating(
&ProfileNetworkContextService::UpdateDohFallbackAllowedForContext,
base::Unretained(this)));mistakenly registered twice?
void SetDohFallbackAllowedForContext(bool allowed) override;Ditto. Since ContextHostResolver is already per context, "ForContext" might be redundant.
+1
Related details
- Andrew Williams
- Dominic Farolino
- Javier Castro
Andrew Williams (Gerrit)
Andrew Williams added 2 comments![]( "Open in Gerrit")
void UpdateDohFallbackAllowedForContext();As I wrote in the other comment, the term "fallback" is confusing. We use "fallback" to refer to both the DoH provider fallback and the act of falling back to insecure (in the DNS stack).
Can we figure out somewhat less confusing terms?
I'll think about this some more... These confusing terms are already in use, maybe we can proactively address those as well: https://source.chromium.org/chromium/chromium/src/+/main:net/dns/dns_config.h;l=108-116;drc=ab614c36f6203d6e3a6fbbe163136e5384f4a69d
void set_doh_fallback_allowed_for_context(bool allowed) {By `for_context`, do you want to distinguish per-profile vs per-context here? Or we may just remove it?
Yeah I originally thought about using a name for these like "doh_fallback_allowed_by_profile_prefs", but from looking at existing naming it didn't seem like net/ had the concept of profiles. I figured "for_network_context" was the closest thing, but I shortened it to "for_context". I tried to explicitly avoid just using `doh_fallback_allowed` for this to disambiguate it from the case where the functionality is disabled by feature flag... I'll have to think more about this
Related details
- Dominic Farolino
- Javier Castro
- Kenichi Ishibashi
Andrew Williams (Gerrit)
Andrew Williams added 1 comment![]( "Open in Gerrit")
pref_change_registrar_.Add(
prefs::kSafeBrowsingEnhanced,
base::BindRepeating(
&ProfileNetworkContextService::UpdateDohFallbackAllowedForContext,
base::Unretained(this)));mistakenly registered twice?
I thought this as well, but these are slightly different - kSafeBrowsingEnabled vs. kSafeBrowsingEnhanced. We use the same handler when either of those change, and we listen for changes to both because that's what safe_browsing::IsEnhancedProtectionEnabled uses: https://source.chromium.org/chromium/chromium/src/+/main:components/safe_browsing/core/common/safe_browsing_prefs.cc;l=165-174;drc=9740839778fe4760edd07198ea924c880a9055d4
@jaca...@chromium.org to confirm this is all we need, though.
Related details
- Dominic Farolino
- Javier Castro
- Kenichi Ishibashi
- Takashi Toyoshima
Andrew Williams (Gerrit)
Andrew Williams added 2 comments![]( "Open in Gerrit")
void UpdateDohFallbackAllowedForContext();Andrew WilliamsAs I wrote in the other comment, the term "fallback" is confusing. We use "fallback" to refer to both the DoH provider fallback and the act of falling back to insecure (in the DNS stack).
Can we figure out somewhat less confusing terms?
I'll think about this some more... These confusing terms are already in use, maybe we can proactively address those as well: https://source.chromium.org/chromium/chromium/src/+/main:net/dns/dns_config.h;l=108-116;drc=ab614c36f6203d6e3a6fbbe163136e5384f4a69d
WDYT about the following plan for naming? "Secondary" is the best I could come up with, but definitely open to suggestions.
In this CL:
```
ResolveContext::IsDohFallbackConfigPresent -> ResolveContext::IsDohConfigFromSecondaryDohNameservers
NetworkContext::SetDohFallbackAllowedForContext -> NetworkContext::SetSecondaryDohAllowed
HostResolver::SetDohFallbackAllowedForContext -> HostResolver::SetSecondaryDohAllowed
FallbackFromSecureTransactionPreferredReason::kFallbackPreferredDohFallbackNotAllowed -> FallbackFromSecureTransactionPreferredReason::kFallbackPreferredSecondaryDohNotAllowed
... etc ...
```
In a follow-up CL:
```
DnsConfig::fallback_doh_nameservers -> DnsConfig::secondary_doh_nameservers
DnsConfig::should_perform_doh_fallback_upgrade -> DnsConfig::doh_config_from_secondary_doh_nameservers
ResolveContext::IsDohFallbackProbeEnabled -> ResolveContext::IsSecondaryDohProbeEnabled
ResolveContext::doh_fallback_canary_domain_check_status -> ResolveContext::secondary_doh_canary_domain_check_status
... etc ...
```
Maybe a less drastic change is using the phrase "DoH Fallback Upgrade" everywhere to denote this behavior and differentiate it from fallback to insecure DNS.
void set_doh_fallback_allowed_for_context(bool allowed) {Andrew WilliamsBy `for_context`, do you want to distinguish per-profile vs per-context here? Or we may just remove it?
Yeah I originally thought about using a name for these like "doh_fallback_allowed_by_profile_prefs", but from looking at existing naming it didn't seem like net/ had the concept of profiles. I figured "for_network_context" was the closest thing, but I shortened it to "for_context". I tried to explicitly avoid just using `doh_fallback_allowed` for this to disambiguate it from the case where the functionality is disabled by feature flag... I'll have to think more about this
OK I thought about this some more, I think using "allowed" sufficiently differentiates it and we can try to use "enabled"/"disabled" to represent state based on feature flags. So I'll drop _for_context everywhere
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi added 3 comments![]( "Open in Gerrit")
Thanks!
void UpdateDohFallbackAllowedForContext();Andrew WilliamsAs I wrote in the other comment, the term "fallback" is confusing. We use "fallback" to refer to both the DoH provider fallback and the act of falling back to insecure (in the DNS stack).
Can we figure out somewhat less confusing terms?
Andrew WilliamsI'll think about this some more... These confusing terms are already in use, maybe we can proactively address those as well: https://source.chromium.org/chromium/chromium/src/+/main:net/dns/dns_config.h;l=108-116;drc=ab614c36f6203d6e3a6fbbe163136e5384f4a69d
WDYT about the following plan for naming? "Secondary" is the best I could come up with, but definitely open to suggestions.
In this CL:
```
ResolveContext::IsDohFallbackConfigPresent -> ResolveContext::IsDohConfigFromSecondaryDohNameservers
NetworkContext::SetDohFallbackAllowedForContext -> NetworkContext::SetSecondaryDohAllowed
HostResolver::SetDohFallbackAllowedForContext -> HostResolver::SetSecondaryDohAllowed
FallbackFromSecureTransactionPreferredReason::kFallbackPreferredDohFallbackNotAllowed -> FallbackFromSecureTransactionPreferredReason::kFallbackPreferredSecondaryDohNotAllowed
... etc ...
```In a follow-up CL:
```
DnsConfig::fallback_doh_nameservers -> DnsConfig::secondary_doh_nameservers
DnsConfig::should_perform_doh_fallback_upgrade -> DnsConfig::doh_config_from_secondary_doh_nameservers
ResolveContext::IsDohFallbackProbeEnabled -> ResolveContext::IsSecondaryDohProbeEnabled
ResolveContext::doh_fallback_canary_domain_check_status -> ResolveContext::secondary_doh_canary_domain_check_status
... etc ...
```Maybe a less drastic change is using the phrase "DoH Fallback Upgrade" everywhere to denote this behavior and differentiate it from fallback to insecure DNS.
AI suggested following options as well:
1. "Alternative"
2. "Supplemental"
3. Public DoH or Default DoH, e.g. `SetPublicDohAllowed()`, `IsPublicDohConfigPresent()`
4. Explicitly qualify with "Upgrade", e.g. `SetDohFallbackUpgradeAllowed()`
Secondary sounds reasonable to me though.
void set_doh_fallback_allowed_for_context(bool allowed) {Andrew WilliamsBy `for_context`, do you want to distinguish per-profile vs per-context here? Or we may just remove it?
Andrew WilliamsYeah I originally thought about using a name for these like "doh_fallback_allowed_by_profile_prefs", but from looking at existing naming it didn't seem like net/ had the concept of profiles. I figured "for_network_context" was the closest thing, but I shortened it to "for_context". I tried to explicitly avoid just using `doh_fallback_allowed` for this to disambiguate it from the case where the functionality is disabled by feature flag... I'll have to think more about this
OK I thought about this some more, I think using "allowed" sufficiently differentiates it and we can try to use "enabled"/"disabled" to represent state based on feature flags. So I'll drop _for_context everywhere
- Andrew Williams
- Dominic Farolino
- Javier Castro
- Takashi Toyoshima
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
pref_change_registrar_.Add(
prefs::kSafeBrowsingEnhanced,
base::BindRepeating(
&ProfileNetworkContextService::UpdateDohFallbackAllowedForContext,
base::Unretained(this)));Andrew Williamsmistakenly registered twice?
I thought this as well, but these are slightly different - kSafeBrowsingEnabled vs. kSafeBrowsingEnhanced. We use the same handler when either of those change, and we listen for changes to both because that's what safe_browsing::IsEnhancedProtectionEnabled uses: https://source.chromium.org/chromium/chromium/src/+/main:components/safe_browsing/core/common/safe_browsing_prefs.cc;l=165-174;drc=9740839778fe4760edd07198ea924c880a9055d4
@jaca...@chromium.org to confirm this is all we need, though.
Ah, Enabled and Enhanced!
So, as these names are very similar and confusing, can you add a short comment above these two calls, and say "register a callback for A and B"?
Related details
- Andrew Williams
- Dominic Farolino
- Javier Castro
Andrew Williams (Gerrit)
Andrew Williams added 9 comments![]( "Open in Gerrit")
fallback-to-default-DoH-provider logic for Enhanced Safe Browsing users,may say "ESB: Enhanced Safe Browsing" for other references.
Done
Thanks for the suggestions, I'd be interested to hear what bcb@ thinks as well when he returns. Let's go with the "DoH Fallback Upgrade" naming for this CL and consider using renaming more broadly in a follow-up CL after talking with Brian. I've implemented this
What does this "ForContext" mean? Simply name UpdateDohFallbackAllowed()?
Done
Can we add some context in a function comment, as this is net/ in //Chrome/. So, it's nice to have more kind comments for other developers.
Done
pref_change_registrar_.Add(
prefs::kSafeBrowsingEnhanced,
base::BindRepeating(
&ProfileNetworkContextService::UpdateDohFallbackAllowedForContext,
base::Unretained(this)));Andrew Williamsmistakenly registered twice?
Takashi ToyoshimaI thought this as well, but these are slightly different - kSafeBrowsingEnabled vs. kSafeBrowsingEnhanced. We use the same handler when either of those change, and we listen for changes to both because that's what safe_browsing::IsEnhancedProtectionEnabled uses: https://source.chromium.org/chromium/chromium/src/+/main:components/safe_browsing/core/common/safe_browsing_prefs.cc;l=165-174;drc=9740839778fe4760edd07198ea924c880a9055d4
@jaca...@chromium.org to confirm this is all we need, though.
Ah, Enabled and Enhanced!
So, as these names are very similar and confusing, can you add a short comment above these two calls, and say "register a callback for A and B"?
Good thinking, done
void SetDohFallbackAllowedForContext(bool allowed) override;Takashi ToyoshimaDitto. Since ContextHostResolver is already per context, "ForContext" might be redundant.
+1
Done
The term 'fallback' is indeed overloaded here (referring to both the DoH provider type and the act of failing back to insecure). This logic correctly prioritizes the ESB-based restriction.
Done
bool doh_fallback_allowed_for_context = false;Andrew WilliamsDitto.
Done
SetDohFallbackAllowedForContext(bool allowed);Andrew WilliamsDitto.
Done
Related details
- Dominic Farolino
- Javier Castro
- Kenichi Ishibashi
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi voted and added 3 comments![]( "Open in Gerrit")
Votes added by Kenichi Ishibashi
| Code-Review | +1 |
3 comments
Ack
// Returns true if the the current DoH configuration was added from theTypo: "the the" -> "the".
Related details
- Andrew Williams
- Dominic Farolino
- Javier Castro
- Takashi Toyoshima
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. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Andrew Williams
- Dominic Farolino
- Javier Castro
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. |
Javier Castro (Gerrit)
Javier Castro voted and added 3 comments![]( "Open in Gerrit")
Votes added by Javier Castro
| Code-Review | +1 |
3 comments
lgtm for ESB pref handling. I left some comments just outlining some of the issues we're taking on with this change -- but I don't expect the ESB pref implementation to change any time soon so I think this tech debt is ok. I filed crbug.com/498905576 to add an observer interface so that interested features will have an easier time with this in the future.
prefs::kSafeBrowsingEnabled,I know it's not ideal, but listening to both prefs should work.
We could probably just register for changes to prefs::kSafeBrowsingEnhanced, because changes to safe browsing state are supposed to pass through SetSafeBrowsingState which ensures that both values get updated. But since it's not easy to tell that from the code (and maybe there's a policy case that I'm not considering), listening to both sgtm.
Also it looks like when setting the prefs, kSafeBrowsingEnhanced is consistently updated first and followed by kSafeBrowsingEnabled. Considering the state transitions, we should be getting consistent answers for IsEnhancedProtectionEnabled except for the transition from NO_PROTECTION to ENHANCED_PROTECTION -- in that case the `UpdateDohFallbackUpgradeAllowed` handler will be called twice and the first call will say enhanced protection is disabled and the second will say that it is enabled. But ultimately it gets the correct answer.
profile_prefs->SetBoolean(prefs::kSafeBrowsingEnhanced, true);
profile_prefs->SetBoolean(prefs::kSafeBrowsingEnabled, false);nit: enhanced true with enabled false isn't really a supported state, but it makes sense to test it here since if the safe browsing state transitions from NO_PROTECTION to ENHANCED_PROTECTION, the individual prefs will transition like this:
```
kSafeBrowsingEnhanced -> false true true
kSafeBrowsingEnabled -> false false true
```
And I think that the handler we're registering in this changelist gets triggered twice and the first call gets ESB disabled and the second gets ESB enabled.
Related details
- Andrew Williams
- Dominic Farolino
Andrew Williams (Gerrit)
Andrew Williams added 3 comments![]( "Open in Gerrit")
lgtm for ESB pref handling. I left some comments just outlining some of the issues we're taking on with this change -- but I don't expect the ESB pref implementation to change any time soon so I think this tech debt is ok. I filed crbug.com/498905576 to add an observer interface so that interested features will have an easier time with this in the future.
Sounds great, thank you! At least for this code we shouldn't need it beyond the experiment (later we can just rely on the value of the pref specifically for this feature), so the tech debt burden should be shortlived.
prefs::kSafeBrowsingEnabled,I know it's not ideal, but listening to both prefs should work.
We could probably just register for changes to prefs::kSafeBrowsingEnhanced, because changes to safe browsing state are supposed to pass through SetSafeBrowsingState which ensures that both values get updated. But since it's not easy to tell that from the code (and maybe there's a policy case that I'm not considering), listening to both sgtm.
Also it looks like when setting the prefs, kSafeBrowsingEnhanced is consistently updated first and followed by kSafeBrowsingEnabled. Considering the state transitions, we should be getting consistent answers for IsEnhancedProtectionEnabled except for the transition from NO_PROTECTION to ENHANCED_PROTECTION -- in that case the `UpdateDohFallbackUpgradeAllowed` handler will be called twice and the first call will say enhanced protection is disabled and the second will say that it is enabled. But ultimately it gets the correct answer.
Thanks for confirming this, and good find on the two calls. As long as the value is correct after the last call this should be OK since all we do in the network service in response to these is update a boolean (we don't kick off probes or reschedule canary domain checks or anything)
// Returns true if the the current DoH configuration was added from theTypo: "the the" -> "the".
- Dominic Farolino
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
10 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: net/dns/resolve_context.h
Insertions: 2, Deletions: 3.
The diff is too large to show. Please review the diff.
```
Change information
[DoH] Prevent use of fallback DoH provider when ESB disabled
For an upcoming experiment we want to only enable the
fallback-to-default-DoH-provider logic for Enhanced Safe Browsing (ESB)
users, and since we intend to use a starts_active=false experiment we
need the check of the experiment feature flag to be once we know a user
is actually eligible for the functionality. Since the ESB prefs are
per-profile, we need to make that check per-NetworkContext, so it's
implemented where a ResolveContext is available.
This functionality will continue to be disabled by default using the
kForceSecureDnsDohFallback feature flag.
In follow-on CLs we will also use this new pref-derived value to disable
actually sending the DoH probes and canary domain check query when it's
set to false.
This CL was based on the following by bcb@:
https://chromium-review.git.corp.google.com/c/chromium/src/+/7689192
- M chrome/browser/net/profile_network_context_service.cc
- M chrome/browser/net/profile_network_context_service.h
- M chrome/browser/net/profile_network_context_service_browsertest.cc
- M net/dns/canary_domain_service_unittest.cc
- M net/dns/context_host_resolver.cc
- M net/dns/context_host_resolver.h
- M net/dns/context_host_resolver_unittest.cc
- M net/dns/dns_client.cc
- M net/dns/dns_client_unittest.cc
- M net/dns/host_resolver.cc
- M net/dns/host_resolver.h
- M net/dns/mapped_host_resolver.cc
- M net/dns/mapped_host_resolver.h
- M net/dns/mock_host_resolver.cc
- M net/dns/mock_host_resolver.h
- M net/dns/resolve_context.cc
- M net/dns/resolve_context.h
- M net/dns/stale_host_resolver.cc
- M net/dns/stale_host_resolver.h
- M services/network/network_context.cc
- M services/network/network_context.h
- M services/network/network_context_unittest.cc
- M services/network/public/mojom/network_context.mojom
- M services/network/test/test_network_context.h
- M tools/metrics/histograms/metadata/net/enums.xml
Code-Review: +1 by Takashi Toyoshima, +1 by Kenichi Ishibashi, +1 by Javier Castro
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |