Add policies to control the DNS over HTTPS identifiers feature [chromium/src : main]
Andreea Costinas (Gerrit)
Attention is currently required from: Andreea Costinas, Eric Orth, Kyle Horimoto, Pavol Marko, Roland Bock, Roman Sorokin.
Andreea Costinas uploaded patch set #7 to this change.
Add policies to control the DNS over HTTPS identifiers feature
This allows to set the policies DnsOverHttpsTemplatesWithIdentifiers and
DnsOverHttpsSalt to allow including variables in the template. Those
variables will then be evaluated when generating the final URI to be
passed to the DNS over HTTPS server. The server can use the content of
the variables for making logging or routing decisions.
The feature is only supported on ChromeOS. For other OSs the original
policy DnsOverHttpsTemplates will be used (if set) which does not allow
including variables. The cross platform policy DnsOverHttpsTemplates
was not extended with identifiers to preserve existing behavior and
because the client request asks for machine identifiers which are not
available on other platforms.
This CL only adds the policies, for the implementation see the followup CL:4013345.
This CL was originally reviewed as CL:3913063.
Bug: b/230468360
Change-Id: I41b976f35f4d9cccca3c8b05b4e2765c55d4570e
---
M ash/constants/ash_features.cc
M ash/constants/ash_features.h
M chrome/browser/ash/crosapi/prefs_ash.cc
M chrome/browser/extensions/api/settings_private/prefs_util.cc
M chrome/browser/lacros/prefs_ash_observer.cc
M chrome/browser/lacros/prefs_ash_observer.h
M chrome/browser/net/secure_dns_policy_handler.cc
M chrome/browser/net/secure_dns_policy_handler.h
M chrome/browser/net/secure_dns_policy_handler_unittest.cc
M chrome/browser/net/stub_resolver_config_reader.cc
M chrome/common/pref_names.cc
M chrome/common/pref_names.h
M chrome/test/data/policy/policy_test_cases.json
M chromeos/crosapi/mojom/prefs.mojom
M components/policy/resources/templates/policies.yaml
M components/policy/resources/templates/policy_definitions/Miscellaneous/DnsOverHttpsTemplates.yaml
A components/policy/resources/templates/policy_definitions/Network/DnsOverHttpsSalt.yaml
A components/policy/resources/templates/policy_definitions/Network/DnsOverHttpsTemplatesWithIdentifiers.yaml
M components/policy_strings.grdp
A components/policy_strings_grdp/IDS_POLICY_SECURE_DNS_SALT_INVALID_ERROR.png.sha1
A components/policy_strings_grdp/IDS_POLICY_SECURE_DNS_SALT_INVALID_SIZE_ERROR.png.sha1
M tools/metrics/histograms/enums.xml
22 files changed, 393 insertions(+), 2 deletions(-)
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Roman Sorokin (Gerrit)
Attention is currently required from: Andreea Costinas, Eric Orth, Kyle Horimoto, Pavol Marko, Roland Bock.
Patch set 7:Code-Review +1
Kyle Horimoto (Gerrit)
Attention is currently required from: Andreea Costinas, Eric Orth, Pavol Marko, Roland Bock.
Patch set 7:Code-Review +1
2 comments:
File chrome/browser/net/secure_dns_policy_handler.cc:
`std::string()`
File chrome/browser/net/secure_dns_policy_handler_unittest.cc:
Patch Set #7, Line 524:
EXPECT_EQ(templates, "");nit: `EXPECT_TRUE(templates.empty())`, same below.
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Andreea Costinas (Gerrit)
Attention is currently required from: Adam Rice, Eric Orth, Pavol Marko, Roland Bock.
Andreea Costinas would like Adam Rice to review this change.
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Andreea Costinas (Gerrit)
Attention is currently required from: Eric Orth, Pavol Marko, Roland Bock.
3 comments:
Patchset:
Thank you for the review!
File chrome/browser/net/secure_dns_policy_handler.cc:
`std::string()`
Done
File chrome/browser/net/secure_dns_policy_handler_unittest.cc:
Patch Set #7, Line 524:
EXPECT_EQ(templates, "");nit: `EXPECT_TRUE(templates.empty())`, same below.
Done
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Andreea Costinas (Gerrit)
Attention is currently required from: Adam Rice, Eric Orth, Pavol Marko, Roland Bock.
1 comment:
Patchset:
Hi Adam,
Would you have time to do an OWNER review for the chrome/browser/net/secure_dns_policy_handler* and chrome/browser/net/stub_resolver_config_reader.cc files?
Thank you!
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Adam Rice (Gerrit)
Attention is currently required from: Andreea Costinas, Eric Orth, Pavol Marko, Roland Bock.
Patch set 8:Code-Review +1
6 comments:
Patchset:
//chrome/browser/net lgtm with nits
File chrome/browser/net/secure_dns_policy_handler.cc:
Patch Set #8, Line 80:
const base::Value* templates_with_identifiers =Maybe use `IsPolicySet()` instead?
Patch Set #8, Line 87:
const base::Value* salt = policies.GetValueUnsafe(key::kDnsOverHttpsSalt);Looks like using `GetValue()` rather than `GetValueUnsafe()` here would avoid needing the type check below.
Patch Set #8, Line 161:
const base::Value* templatesWithIdentifier = policies.GetValue(Style: `templates_with_identifiers`
Patch Set #8, Line 163:
const base::Value* salt = policies.GetValueUnsafe(key::kDnsOverHttpsSalt);Using `GetValue()` here would save some logic.
File chrome/browser/net/secure_dns_policy_handler_unittest.cc:
Patch Set #8, Line 512:
auto it = errors().begin();Something like
```
EXPECT_THAT(errors(), ::testing::ElementsAre(
::testing::Pair(kDnsOverHttpsSalt, expected_error1),
...
));
```
might be more elegant.
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Roland Bock (Gerrit)
Attention is currently required from: Andreea Costinas, Eric Orth, Pavol Marko.
Patch set 8:Code-Review +1
2 comments:
Patchset:
Looks good to me, but see the comment.
File chrome/browser/lacros/prefs_ash_observer.cc:
Patch Set #8, Line 63:
LOG(WARNING)Note: I see that this is the current style, but this should be a `DCHECK`, IMO.
```
DCHECK(value.is_string()) << "Unexpected value type: "
<< base::Value::GetTypeName(value.type());
```The pref this is observing is a string-pref. If anything but a string gets here, it is a critical error. IOW, this just cannot and must not receive a non-string every now and then.
Can you add a `TOOD(link-to-bug)` here?
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Adam Rice (Gerrit)
Attention is currently required from: Andreea Costinas, Eric Orth, Pavol Marko, Roland Bock.
1 comment:
File chrome/browser/lacros/prefs_ash_observer.cc:
Patch Set #8, Line 63:
LOG(WARNING)
Note: I see that this is the current style, but this should be a `DCHECK`, IMO. […]
It looks like CrosapiPrefsObserver doesn't check the type of the value, and it comes from configuration outside of Chrome, so it's not an invariant of the code that the type is correct, therefore DCHECK() is not appropriate.
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Andreea Costinas (Gerrit)
Attention is currently required from: Adam Rice, Eric Orth, Pavol Marko, Roland Bock.
6 comments:
File chrome/browser/lacros/prefs_ash_observer.cc:
Patch Set #8, Line 63:
LOG(WARNING)
It looks like CrosapiPrefsObserver doesn't check the type of the value, and it comes from configurat […]
I know some CrosapiPrefObservers use DCHECKS (e.g. observers tracking quick_answers prefs) but Adam makes a good point.
As a Lacros code owner, Roland, if you feel strongly about this, I will add a DCHECK, otherwise I'll leave the warning.
File chrome/browser/net/secure_dns_policy_handler.cc:
Patch Set #8, Line 80:
const base::Value* templates_with_identifiers =Maybe use `IsPolicySet()` instead?
Done
Patch Set #8, Line 87:
const base::Value* salt = policies.GetValueUnsafe(key::kDnsOverHttpsSalt);
Looks like using `GetValue()` rather than `GetValueUnsafe()` here would avoid needing the type check […]
You're right, thank you!
Patch Set #8, Line 161:
const base::Value* templatesWithIdentifier = policies.GetValue(Style: `templates_with_identifiers`
Done
Patch Set #8, Line 163:
const base::Value* salt = policies.GetValueUnsafe(key::kDnsOverHttpsSalt);Using `GetValue()` here would save some logic.
Done
File chrome/browser/net/secure_dns_policy_handler_unittest.cc:
Patch Set #8, Line 512:
auto it = errors().begin();
Something like […]
You're right but testing::Pair doesn't work well with `PolicyErrorMap`.
`PolicyErrorMap` has a `GetErrorMessages()` methods which allows more elegant testing.
Thanks for the suggestion!!
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Roland Bock (Gerrit)
Attention is currently required from: Adam Rice, Eric Orth, Pavol Marko.
Patch set 8:Code-Review +1
1 comment:
File chrome/browser/lacros/prefs_ash_observer.cc:
Patch Set #8, Line 63:
LOG(WARNING)It looks like CrosapiPrefsObserver doesn't check the type of the value, and it comes from configurat […]
IIUC, it comes from a PrefService. And that ensures the type.
But then again, it comes through crosapi and there *could* be disruptions due to version skew. So agreed, `DCHECK` might be too eager.
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Eric Orth (Gerrit)
Attention is currently required from: Adam Rice, Andreea Costinas, Pavol Marko.
11 comments:
Patchset:
Only reviewed chrome/browser/net/ files.
I'm a little sad to see ChromeOS-only policies added to the DoH functionality, but it seems reasonable in this specific case since this appears to all be about sending client identity to DoH servers and Chrome browser has generally taken the position that the browser shouldn't allow sending any such information because only the OS is positioned to know if such behaviors are safe for user privacy.
File chrome/browser/net/secure_dns_policy_handler.cc:
Nit: Over
Patch Set #9, Line 47:
CheckDnsOVerHttpsSaltPolicyThis should be in an anonymous namespace, and preferably placed at the top of the policy namespace since net code tends to prefer a single anonymous namespace at the top. Would also be good to merge the constants above into that new anonymous namespace.
namespace policy {
namespace {
#if BUILDFLAG(IS_CHROMEOS_ASH)
constantsCheckDnsOVerHttpsSaltPolicy()
#endif // BUILDFLAG(IS_CHROMEOS_ASH)} // namespace
SecureDnsPolicyHandler::SecureDnsPolicyHandler() {}
// If the templates_with_identifiers is there it overrides the original
// setting on ChromeOS, only.I'm curious about the decision to just ignore kDnsOverHttpsTemplates when kDnsOverHttpsTemplatesWithIdentifiers is present. Since kDnsOverHttpsTemplates is completely unusable in that case, would it be better to make it an error to have both set? Or are there cases here where an admin would need to set both to let one take effect in ChromeOS cases and the other take effect for non-ChromeOS?
Similar question about whether or not we should make setting the ChromeOS-only policies errors for non-ChromeOS.
Patch Set #9, Line 118:
CheckDnsOVerHttpsSaltPolicyCan we have an error on setting kDnsOverHttpsSalt when kDnsOverHttpsTemplatesWithIdentifiers is not set?
Patch Set #9, Line 176:
prefs->SetString(prefs::kDnsOverHttpsTemplates, templates->GetString());Since we're now not validating kDnsOverHttpsTemplates as much when kDnsOverHttpsTemplatesWithIdentifiers is set, and appearing to intend to just only use kDnsOverHttpsTemplatesWithIdentifiers in that case, would it be safer to not set or may clear the kDnsOverHttpsTemplates pref? Safer to avoid accidentally using kDnsOverHttpsTemplates when it shouldn't be used.
File chrome/browser/net/secure_dns_policy_handler_unittest.cc:
Patch Set #9, Line 37:
kDoHSaltNit: kDohSalt. Style guide preference it to treat acryonyms as a single word for cammelcasing. (https://google.github.io/styleguide/cppguide.html#General_Naming_Rules)
Patch Set #9, Line 463:
CheckAndApplyPolicySettings();Validate errors() is empty.
Patch Set #9, Line 487:
CheckAndApplyPolicySettings();Validate errors() is empty.
Patch Set #9, Line 521:
#endifMissing some important cases:
- kDnsOverHttpsTemplatesWithIdentifiers and kDnsOverHttpsTemplates both set
- Valid kDnsOverHttpsTemplatesWithIdentifiers but missing kDnsOverHttpsSalt
- kDnsOverHttpsTemplatesWithIdentifiers when mode is off or missing
- (If you add an error per the comment in secure_dns_policy_handler.cc) kDnsOverHttpsSalt set without kDnsOverHttpsTemplatesWithIdentifiers
File chrome/browser/net/stub_resolver_config_reader.cc:
Patch Set #9, Line 204:
IS_CHROMEOSShould this be IS_CHROMEOS or IS_CHROMEOS_ASH? The policy handler is only setting this for ash.
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Pavol Marko (Gerrit)
Attention is currently required from: Andreea Costinas.
Patch set 9:Code-Review +1
Andreea Costinas (Gerrit)
Attention is currently required from: Adam Rice, Eric Orth, Kyle Horimoto, Pavol Marko, Roland Bock, Roman Sorokin.
13 comments:
Patchset:
Only reviewed chrome/browser/net/ files. […]
Ack and agree. Unfortunately most enterprise features go against user privacy, partly to protect the company's interest, partly because of legal requirements (EDU).
Patchset:
Thanks a lot for the review, Eric!
Please take another look!
File chrome/browser/lacros/prefs_ash_observer.cc:
Patch Set #8, Line 63:
LOG(WARNING)
IIUC, it comes from a PrefService. And that ensures the type. […]
Ack
File chrome/browser/net/secure_dns_policy_handler.cc:
Patch Set #9, Line 47:
CheckDnsOVerHttpsSaltPolicyThis should be in an anonymous namespace, and preferably placed at the top of the policy namespace s […]
True, thank you!
Nit: Over
Done
// If the templates_with_identifiers is there it overrides the original
// setting on ChromeOS, only.
I'm curious about the decision to just ignore kDnsOverHttpsTemplates when kDnsOverHttpsTemplatesWith […]
I can only speak for myself (CL is a cherry pick with different original author - see comment) but the validation logic becomes very complex and I wasn't sure if we need to deal with all this complexity just to validate something that we don't use (e.g. we need to check values of both policies before deciding if `mode=secure` is an ok value).
I also didn't fully understand the validation logic i.e. why we consider templates for which `IsTemplatesPolicyNotSpecified()=false` as applicable.
I've changed the code now to validate both policies also for Chrome OS.
>Or are there cases here where an admin would need to set both to let one take effect in ChromeOS cases and the other take effect for non-ChromeOS?
Yes, this is a supported case.
>Similar question about whether or not we should make setting the ChromeOS-only policies errors for non-ChromeOS.
I'm not sure this makes sense.
1. On other OSes the policy is not read.
2. Consistency - AFAIK we don't do this for any other Chrome OS only policy.
Patch Set #9, Line 118:
CheckDnsOVerHttpsSaltPolicy
Can we have an error on setting kDnsOverHttpsSalt when kDnsOverHttpsTemplatesWithIdentifiers is not […]
Done
Patch Set #9, Line 176:
prefs->SetString(prefs::kDnsOverHttpsTemplates, templates->GetString());
Since we're now not validating kDnsOverHttpsTemplates as much when kDnsOverHttpsTemplatesWithIdentif […]
I've updates the code and now we validate both policies.
File chrome/browser/net/secure_dns_policy_handler_unittest.cc:
Patch Set #9, Line 37:
kDoHSalt
Nit: kDohSalt. Style guide preference it to treat acryonyms as a single word for cammelcasing. […]
Done
Patch Set #9, Line 463:
CheckAndApplyPolicySettings();Validate errors() is empty.
Done
Patch Set #9, Line 487:
CheckAndApplyPolicySettings();Validate errors() is empty.
Done
Patch Set #9, Line 521:
#endif
Missing some important cases: […]
Done and thank you!
File chrome/browser/net/stub_resolver_config_reader.cc:
Patch Set #9, Line 204:
IS_CHROMEOSShould this be IS_CHROMEOS or IS_CHROMEOS_ASH? The policy handler is only setting this for ash.
IS_CHROMEOS_ASH
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Stefan Radig (Gerrit)
Attention is currently required from: Adam Rice, Andreea Costinas, Eric Orth, Kyle Horimoto, Pavol Marko, Roland Bock, Roman Sorokin.
1 comment:
File chrome/browser/net/secure_dns_policy_handler.cc:
// If the templates_with_identifiers is there it overrides the original
// setting on ChromeOS, only.
I can only speak for myself (CL is a cherry pick with different original author - see comment) but t […]
The idea behind ignoring the old policy if the new is set is that only on ChromeOS the new policy is respected. It would typically contain placeholders for variables. On other OSs the same string would not work because the other OSs would not evaluate the variables and thus the resulting URLs would be garbage for the DNS provider. Thus on any other OS the original policy is used which is then understandable by the DNS provider because it would not contain the placeholders.
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Andreea Costinas (Gerrit)
Attention is currently required from: Adam Rice, Eric Orth, Kyle Horimoto, Pavol Marko, Roland Bock, Roman Sorokin, Stefan Radig.
1 comment:
File chrome/browser/net/secure_dns_policy_handler.cc:
// If the templates_with_identifiers is there it overrides the original
// setting on ChromeOS, only.
The idea behind ignoring the old policy if the new is set is that only on ChromeOS the new policy is […]
Thank you, Stefan!
I understood Eric's question as "why don't we still validate kDnsOverHttpsTemplates on Chrome OS even though the other policy has precedence".
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Adam Rice (Gerrit)
Attention is currently required from: Andreea Costinas, Eric Orth, Kyle Horimoto, Pavol Marko, Roland Bock, Roman Sorokin, Stefan Radig.
Patch set 10:Code-Review +1
Kyle Horimoto (Gerrit)
Attention is currently required from: Andreea Costinas, Eric Orth, Garrick Evans, Pavol Marko, Roland Bock.
Kyle Horimoto would like Garrick Evans to review this change authored by Andreea Costinas.
A components/policy_strings_grdp/IDS_POLICY_SECURE_DNS_SALT_IRRELEVANT_ERROR.png.sha1
A components/policy_strings_grdp/IDS_POLICY_SECURE_DNS_TEMPLATES_WITH_IDENTIFIERS_IRRELEVANT_ERROR.png.sha1
M tools/metrics/histograms/enums.xml
24 files changed, 624 insertions(+), 40 deletions(-)
Kyle Horimoto (Gerrit)
Attention is currently required from: Andreea Costinas, Eric Orth, Garrick Evans, Pavol Marko, Roland Bock.
Patch set 11:Code-Review +1
2 comments:
File chrome/browser/net/secure_dns_policy_handler.h:
bool is_templates_policy_valid_ = false;
#if BUILDFLAG(IS_CHROMEOS_ASH)
bool is_templates_with_identifiers_policy_valid_ = false;
#endifCan we add comments here to make it clear why we need these two separate fields and how they interact?
File chrome/browser/net/secure_dns_policy_handler_unittest.cc:
Patch Set #11, Line 453:
#if BUILDFLAG(IS_CHROMEOS_ASH)Thanks for all of these tests! Made it a lot easier to understand the implementation.
Garrick Evans (Gerrit)
Attention is currently required from: Andreea Costinas, Eric Orth, Pavol Marko, Roland Bock.
1 comment:
Patchset:
lgtm
Pavol Marko (Gerrit)
Attention is currently required from: Andreea Costinas, Eric Orth, Roland Bock.
1 comment:
File chrome/browser/net/stub_resolver_config_reader.cc:
Patch Set #9, Line 204:
IS_CHROMEOS
IS_CHROMEOS_ASH
I'm confused - this is still IS_CHROMEOS but your comment is saying IS_CHROMEOS_ASH :-)
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Pavol Marko (Gerrit)
Attention is currently required from: Andreea Costinas, Eric Orth, Roland Bock.
Patch set 11:Code-Review +1
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Andreea Costinas (Gerrit)
Attention is currently required from: Eric Orth, Roland Bock.
3 comments:
Patchset:
Thanks again for the review!!
File chrome/browser/net/secure_dns_policy_handler.h:
bool is_templates_policy_valid_ = false;
#if BUILDFLAG(IS_CHROMEOS_ASH)
bool is_templates_with_identifiers_policy_valid_ = false;
#endif
Can we add comments here to make it clear why we need these two separate fields and how they interac […]
Done
File chrome/browser/net/stub_resolver_config_reader.cc:
Patch Set #9, Line 204:
IS_CHROMEOS
I'm confused - this is still IS_CHROMEOS but your comment is saying IS_CHROMEOS_ASH :-)
True, I forgot to update. We need `IS_CHROMEOS` because the DNS secure prefs are synced with Lacros.
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Eric Orth (Gerrit)
Attention is currently required from: Andreea Costinas, Eric Orth, Roland Bock, Stefan Radig.
13 comments:
File chrome/browser/net/secure_dns_policy_handler.h:
Patch Set #12, Line 34:
IsTemplatesPolicyNotSpecifiedNow that template processing is a little more complicated, I think this method needs a bit more comment explanation. Maybe a comment that this returns true iff templates are expected (due to the mode) but not set or not valid.
Patch Set #12, Line 41:
is_templates_policy_valid_Here and the other one below, I'd also add to the comment that CheckPolicySettings() is responsible for setting this.
File chrome/browser/net/secure_dns_policy_handler.cc:
// If the templates_with_identifiers is there it overrides the original
// setting on ChromeOS, only.
Thank you, Stefan! […]
It was kinda implicitly a two parter comment...
1) Can we make setting both policies an error? Stefan explained that this was a no because admins will need to set both policies so that the correct one can be used for the OS that loads the policies.
2) Can we still validate the unused policy. (I figure better to more strictly validate these things to ensure an admin will see if something is configured wrong. Don't want an admin to set things up, only test it on ChromeOS, and miss that Chrome is rejecting the policies on other platforms.) Looks like the code is now doing this validation. Would be nice if we could also validate the templates policy on non-ChromeOS before ignoring it, but that's reasonable to skip since we don't have the full ability to validate it on non-ChromeOS.
File chrome/browser/net/secure_dns_policy_handler.cc:
Patch Set #12, Line 38:
an error messageTechnically, this has a non-error case (both policies unset) that will return false without adding an error message, so this comment isn't accurate.
Patch Set #12, Line 54:
IDS_POLICY_SECURE_DNS_TEMPLATES_WITH_IDENTIFIERS_IRRELEVANT_ERRORAdd to the comment at the top of this function that it is also responsible for all checks regarding one policy not being set without the other, since that is a responsibility not obvious from the function name.
Patch Set #12, Line 61:
!salt->is_string()GetValue() will return nullptr if the type doesn't match, so this is a potential nullptr deref. So either need this to check `!salt` or change the GetValue() to a GetValueUnsafe().
if (templates_set && salt->GetString().empty())
errors->AddError(
policy::key::kDnsOverHttpsTemplatesWithIdentifiers,
IDS_POLICY_SECURE_DNS_TEMPLATES_WITH_IDENTIFIERS_IRRELEVANT_ERROR);Should this return false? Or is it intended for this to fall through and also set the INVALID_SIZE_ERROR? And if not intended to fall through, should something return false in the empty salt case?
Also, nit: Per style guide, if statements need to use curly braces unless the whole thing including body can fit on one or two lines.
if (!templates_set)
errors->AddError(policy::key::kDnsOverHttpsSalt,
IDS_POLICY_SECURE_DNS_SALT_IRRELEVANT_ERROR);Nit: Curly braces required.
Patch Set #12, Line 88:
adds an error messageAgain, this is not technically true since it doesn't add an error message in the non-error unset case.
Patch Set #12, Line 107:
if (!templates_str.empty() &&When `templates_str` is empty, should something return false?
Patch Set #12, Line 182:
!templates || templates->is_string()The only time IsTemplatesPolicyNotSpecified() would return true without templates being unset or non-string is if templates is empty. Is it deliberate to make empty templates no longer an error?
Patch Set #12, Line 247:
const base::Value* templatesNow that we have the `is_templates_..._valid_` flags, would it make more sense for this function to just check those instead of interpretting the templates string again?
File chrome/browser/net/secure_dns_policy_handler_unittest.cc:
Why this change? Empty string is a string, but this test is supposed to be the test for non-strings.
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Andreea Costinas (Gerrit)
Attention is currently required from: Eric Orth, Eric Orth, Roland Bock, Stefan Radig.
13 comments:
Patchset:
Thanks a lot, Eric!
Please take another look!
File chrome/browser/net/secure_dns_policy_handler.h:
Patch Set #12, Line 34:
IsTemplatesPolicyNotSpecified
Now that template processing is a little more complicated, I think this method needs a bit more comm […]
Done
Patch Set #12, Line 41:
is_templates_policy_valid_
Here and the other one below, I'd also add to the comment that CheckPolicySettings() is responsible […]
Done
File chrome/browser/net/secure_dns_policy_handler.cc:
Patch Set #12, Line 38:
an error message
Technically, this has a non-error case (both policies unset) that will return false without adding a […]
Done
Patch Set #12, Line 54:
IDS_POLICY_SECURE_DNS_TEMPLATES_WITH_IDENTIFIERS_IRRELEVANT_ERROR
Add to the comment at the top of this function that it is also responsible for all checks regarding […]
Done
Patch Set #12, Line 61:
!salt->is_string()
GetValue() will return nullptr if the type doesn't match, so this is a potential nullptr deref. […]
Done (changed to `GetValueUnsafe()`)
if (templates_set && salt->GetString().empty())
errors->AddError(
policy::key::kDnsOverHttpsTemplatesWithIdentifiers,
IDS_POLICY_SECURE_DNS_TEMPLATES_WITH_IDENTIFIERS_IRRELEVANT_ERROR);
Should this return false? Or is it intended for this to fall through and also set the INVALID_SIZE_E […]
If the policy is set to "", it can just fall through the INVALID_SIZE_ERROR case.
Added curly braces.
if (!templates_set)
errors->AddError(policy::key::kDnsOverHttpsSalt,
IDS_POLICY_SECURE_DNS_SALT_IRRELEVANT_ERROR);Nit: Curly braces required.
Done
Patch Set #12, Line 88:
adds an error message
Again, this is not technically true since it doesn't add an error message in the non-error unset cas […]
Done
Patch Set #12, Line 107:
if (!templates_str.empty() &&When `templates_str` is empty, should something return false?
I'm following the existing logic where an error message is appended to `errors` if templates is empty but `templates_is_applicable` remains "true".
I hope I'm reading the code correctly, but it looks like it should only return false when (templates are empty && DNS mode is secure) [1].
Since it's enough to have one of the templates URI policy set,the case when DNS mode is secure is evaluated outside this method.
Patch Set #12, Line 182:
!templates || templates->is_string()
The only time IsTemplatesPolicyNotSpecified() would return true without templates being unset or non […]
>Is it deliberate to make empty templates no longer an error?
If you mean an error message no longer being added to `errors`:
If `templates` is null or an empty string, it adds IDS_POLICY_SECURE_DNS_TEMPLATES_NOT_SPECIFIED_ERROR to `errors`. This "if" only excludes the template values which are not string (this case being evaluated in `CheckDnsOverHttpsTemplatePolicy`).
The test cases "TemplatesEmptyWithModeSecure" and "NoTemplatesWithIdentifiers" verify this and I now added "TemplatesWithIdentifiersEmpty" to distinguish between "the policy not set" and "the policy set to an empty string".
If you mean `CheckDnsOverHttpsTemplatePolicy` doesn't return "false" for this specific case, this is the existing logic.
Patch Set #12, Line 247:
const base::Value* templates
Now that we have the `is_templates_... […]
I think yes, but you know better. This would slightly change the expectations for the case where a policy value is set and invalid (not string) + the mode is automatic.
In this case, in `ApplyPolicySettings`, both `IsTemplatesPolicyNotSpecified` and `ShouldSetTemplatesPref` would be false and the policy is ignored.
If we replace `IsTemplatesPolicyNotSpecified` with `is_templates_..._valid_` then the pref associated with the invalid policy is cleared.
In this CL, `is_templates_..._valid_` replaces `ShouldSetTemplatesPref` so the logic doesn't change.
If you decide we can replace `IsTemplatesPolicyNotSpecified`, I would rather do it in a followup CL. Is that ok?
File chrome/browser/net/secure_dns_policy_handler_unittest.cc:
Why this change? Empty string is a string, but this test is supposed to be the test for non-strings.
You're right, I just looked at the expected error which was `IDS_POLICY_SECURE_DNS_TEMPLATES_NOT_SPECIFIED_ERROR` and changed the value... but the error message was wrong.
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Eric Orth (Gerrit)
Attention is currently required from: Andreea Costinas, Eric Orth, Roland Bock, Stefan Radig.
4 comments:
File chrome/browser/net/secure_dns_policy_handler.cc:
if (templates_set && salt->GetString().empty())
errors->AddError(
policy::key::kDnsOverHttpsTemplatesWithIdentifiers,
IDS_POLICY_SECURE_DNS_TEMPLATES_WITH_IDENTIFIERS_IRRELEVANT_ERROR);
If the policy is set to "", it can just fall through the INVALID_SIZE_ERROR case. […]
Fallthrough SGTM, but please add a comment to point out that it is intended.
Patch Set #12, Line 107:
if (!templates_str.empty() &&
I'm following the existing logic where an error message is appended to `errors` if templates is empt […]
It was previously `true`, but the result didn't really matter since IsTemplatesPolicyNotSpecified() still checks for empty and treats it the same as unset. So I think if we converted these empty cases to returning `false`, that would make this logic all a little more consistent without having any behavior change.
And then if we do that, it helps with my suggestion below of converting IsTemplatesPolicyNotSpecified() to just check the templates valid flags since those flags would then match up with the current IsTemplatesPolicyNotSpecified() behavior.
Patch Set #12, Line 182:
!templates || templates->is_string()
>Is it deliberate to make empty templates no longer an error? […]
Hmm... sorry, I was misreading this as `!templates || !templates->is_string()` and thus wondering why you would do that when it is mostly redundant with the logic in IsTemplatesPolicyNotSpecified(). But I see now that it is actually `templates->is_string()`, so I take it that this is to avoid emitting IDS_POLICY_SECURE_DNS_TEMPLATES_NOT_SPECIFIED_ERROR alongside the invalid errors when templates is a non-string?
I think it would be better to keep it simple and just always emit IDS_POLICY_SECURE_DNS_TEMPLATES_NOT_SPECIFIED_ERROR when IsTemplatesPolicyNotSpecified() returns true, regardless of the type of `templates` and regardless of whether or not we also emitted an invalid error. Yes, that means emitting two errors for invalid non-strings, but that's allowed.
Patch Set #12, Line 247:
const base::Value* templates
I think yes, but you know better. […]
Sorry, I'm not suggesting we completely replace IsTemplatesPolicyNotSpecified() with the valid_ flags, only that we use them instead of most of the logic internal to the function.
So it becomes...
```
bool SecureDnsPolicyHandler::IsTemplatesPolicyNotSpecified(
base::StringPiece mode_str) {
if (mode_str == SecureDnsConfig::kModeSecure) {
#if BUILDFLAG(IS_CHROMEOS_ASH)
return !is_templates_with_identifiers_policy_valid_ &&
!is_templates_policy_valid_;
#else
return !is_templates_policy_valid_;
#endif
}
return false;
}
```
I believe that should result in the same behavior (at least as far as whether or not there is an error, but maybe some slight unimportant changes in which errors are emitted if this then results in the missing-templates error being added to invalid templates errors).
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Andreea Costinas (Gerrit)
Attention is currently required from: Eric Orth, Eric Orth, Roland Bock, Stefan Radig.
5 comments:
Patchset:
Thanks for the review, Eric! PTAL!
File chrome/browser/net/secure_dns_policy_handler.cc:
if (templates_set && salt->GetString().empty())
errors->AddError(
policy::key::kDnsOverHttpsTemplatesWithIdentifiers,
IDS_POLICY_SECURE_DNS_TEMPLATES_WITH_IDENTIFIERS_IRRELEVANT_ERROR);
Fallthrough SGTM, but please add a comment to point out that it is intended.
Done
Patch Set #12, Line 107:
if (!templates_str.empty() &&
It was previously `true`, but the result didn't really matter since IsTemplatesPolicyNotSpecified() […]
True, thank you for the suggestion!
Patch Set #12, Line 182:
!templates || templates->is_string()
Hmm... […]
>so I take it that this is to avoid emitting IDS_POLICY_SECURE_DNS_TEMPLATES_NOT_SPECIFIED_ERROR alongside the invalid errors when templates is a non-string?
Yes. I've tried to keep the existing logic, but code readability definitely worsened :).
Patch Set #12, Line 247:
const base::Value* templates
Sorry, I'm not suggesting we completely replace IsTemplatesPolicyNotSpecified() with the valid_ flag […]
Done (with a small change)
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Roland Bock (Gerrit)
Attention is currently required from: Andreea Costinas, Eric Orth, Eric Orth, Stefan Radig.
Patch set 14:Code-Review +1
1 comment:
Patchset:
Still LGTM :-)
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Eric Orth (Gerrit)
Attention is currently required from: Andreea Costinas, Eric Orth, Stefan Radig.
Patch set 14:Code-Review +1
2 comments:
Patchset:
LGTM for chrome/browser/net/
File chrome/browser/net/secure_dns_policy_handler_unittest.cc:
EXPECT_EQ(errors().size(), 2U);
EXPECT_EQ(errors().GetErrorMessages(key::kDnsOverHttpsTemplates),
base::JoinString({expected_error1, expected_error2}, u"\n"));Optional... might be a little less brittle if this didn't depend on a specific order for the errors by splitting the actual string rather than joining the expected strings:
```
#include "base/strings/string_split.h"EXPECT_THAT(
base::SplitString(
errors().GetErrorMessages(key::kDnsOverHttpsTemplates),
base::kWhitespaceASCIIAs16,
base::TRIM_WHITESPACE,
base::SPLIT_WANT_NONEMPTY),
testing::UnorderedElementsAre(
expected_error1, expected_error2));
```
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Andreea Costinas (Gerrit)
Attention is currently required from: Andreea Costinas, Eric Orth, Stefan Radig.
Patch set 15:Commit-Queue +2
Andreea Costinas (Gerrit)
Attention is currently required from: Eric Orth, Stefan Radig.
1 comment:
File chrome/browser/net/secure_dns_policy_handler_unittest.cc:
EXPECT_EQ(errors().size(), 2U);
EXPECT_EQ(errors().GetErrorMessages(key::kDnsOverHttpsTemplates),
base::JoinString({expected_error1, expected_error2}, u"\n"));
Optional... […]
Good point, thanks!
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
14 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: tools/metrics/histograms/enums.xml
Insertions: 3, Deletions: 2.
The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/browser/net/secure_dns_policy_handler_unittest.cc
Insertions: 6, Deletions: 6.
The diff is too large to show. Please review the diff.
```
```
The name of the file: components/policy/resources/templates/policies.yaml
Insertions: 3, Deletions: 2.
The diff is too large to show. Please review the diff.
```
Add policies to control the DNS over HTTPS identifiers feature
This allows to set the policies DnsOverHttpsTemplatesWithIdentifiers and
DnsOverHttpsSalt to allow including variables in the template. Those
variables will then be evaluated when generating the final URI to be
passed to the DNS over HTTPS server. The server can use the content of
the variables for making logging or routing decisions.
The feature is only supported on ChromeOS. For other OSs the original
policy DnsOverHttpsTemplates will be used (if set) which does not allow
including variables. The cross platform policy DnsOverHttpsTemplates
was not extended with identifiers to preserve existing behavior and
because the client request asks for machine identifiers which are not
available on other platforms.
This CL only adds the policies, for the implementation see the followup CL:4013345.
This CL was originally reviewed as CL:3913063.
Bug: b/230468360
Change-Id: I41b976f35f4d9cccca3c8b05b4e2765c55d4570e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3990794
Reviewed-by: Eric Orth <eric...@chromium.org>
Reviewed-by: Roland Bock <rb...@google.com>
Reviewed-by: Pavol Marko <pma...@chromium.org>
Reviewed-by: Adam Rice <ri...@chromium.org>
Commit-Queue: Andreea Costinas <acos...@google.com>
Reviewed-by: Kyle Horimoto <khor...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1075090}
24 files changed, 685 insertions(+), 61 deletions(-)
Lei Zhang (Gerrit)
Attention is currently required from: Andreea Costinas.
1 comment:
File chrome/browser/net/secure_dns_policy_handler.cc:
Patch Set #16, Line 12:
#include "chrome/browser/ash/arc/policy/managed_configuration_variables.h"It's not obvious if the new headers added here are useful. For example, everything declared in this header is in namespace arc, but I don't see any "arc::" references in this file. Can this be removed?
If this header is needed, then it should be placed down by line 28, given it's got "ash" in its path. I'm trying to help prevent Ash-only headers from being included in non-Ash builds. So trying to nudge folks to fix the existing cases.
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.
Andreea Costinas (Gerrit)
1 comment:
File chrome/browser/net/secure_dns_policy_handler.cc:
Patch Set #16, Line 12:
#include "chrome/browser/ash/arc/policy/managed_configuration_variables.h"
It's not obvious if the new headers added here are useful. […]
Thanks for flagging this! Please see https://chromium-review.googlesource.com/c/chromium/src/+/4238496.
To view, visit change 3990794. To unsubscribe, or for help writing mail filters, visit settings.