New issue
Advanced search Search tips

Issue 855690 link

Starred by 1 user

Issue metadata

Status: Closed
Owner:
Closed: Sep 5
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocking:
issue 875053
issue 849201



Sign in to add a comment

Add Enterprise Policy to allow HTTP/2 connection coalescing on connections with client certs

Project Member Reported by rsleevi@chromium.org, Jun 22 2018

Issue description

Currently, SpdySession::CanPool rejects coalescing multiple hosts onto the same HTTP/2 connection if a client cert has been used.

This is an important security property, to prevent ambient auth on connections, particularly with the Web Platform (e.g. this would defeat some of CORS' protections).

However, as part of a temporary enterprise migration, there's been a feature request to allow pooling H/2 sessions even if a client cert has been set, if an appropriate Enterprise policy has been set.

Given the security implications to the overall platform, pooling should only be enabled for an explicit list of hosts and subdomains, rather than a blanket enabling (which would otherwise be Web-visible)
 
Note: We would not want this policy generally exposed.

The work for this would likely involve:
- Add a policy to https://cs.chromium.org/chromium/src/components/policy/resources/policy_templates.json
  - Support on all platforms (although more time-critical for ChromeOS)
  - per_profile, dynamic_refresh, and future set in the features set
- Add a per-profile pref that is wired to the policy
- Expose configuration to //services/network/public/mojom/network_context
  - SSLConfig/SSLConfigClient or ProxyConfigWithAnnotation/ProxyConfigClient may be a suitable example here
- Introduce a new object to allow callers to check whether host X can be pooled with a client-cert based connection
  - Object can be updated whenever the NetworkContext is updated (e.g. a vector of hosts or URL patterns that can be pooled). Same approach as a PrefObserver-style solution
- Pass that object to SpdySession::CanPool
  - Also means making sure all the CanPool sites have this object, which likely means threading it through all of the HttpNetworkSession and URLRequestContextBuilder sites
Cc: nhar...@chromium.org
Blocking: 849201
Labels: -Target-69 Target-70
(Changing target to 70, since feature freeze for 69 is today.)

Would a list of hostname suffixes work for the policy, or do we need to support more than that?

What's the reason for needing dynamic_refresh? (It seems like supporting that would require more work)
@nharper: Does this also mean you see us supporting Channel ID through Chrome 69 as well? Just making sure - there's still the possibility of getting this in before branch, so we should be targeting 69 unless we're agreeing to hold on exposing Channel ID to the Web for another release, which would be ideal not to.

From a syntax standpoint, I think we're talking about the standard syntax for expressing hosts (as captured in the URLBlacklist format, but simplified re: scheme/port/path)
foo.com -> (match to foo and subdomains)
.foo.com -> (match to subdomains)

Per-profile policies should support dynamic_refresh, given that they can be sync'd post-profile creation. Per-profile without dynamic_refresh is rare and violates the principle of least surprise. Supporting dynamic_refresh is not substantially more work - it's more or less the copy-pasta of our existing browser<->network_context configuration (like the two mentioned). Without dynamic_refresh, we'd post in a vector of strings, but it'd still need to be plumbed through in the same fashion (and ideally, encapsulated in an interface rather than just as a raw string vector), and by that point, you've done 90% of the same work, and the only extra work to do is to have the NetworkContext signal that object when it receives an update :)
I know this means postponing Channel ID deprecation by one release, and while I'd prefer not do, I think it's reasonable to do that instead of rushing this implementation and re-deprecation of Channel ID.

I did not know about the URLBlacklist format - that sounds like a fine mechanism to reuse here.

I'm not familiar with the details of how policies work. If it's only 10% more work to support dynamic_refresh (and better matches user expectations), I'll do it. It seems like there are plenty of examples to work off of here.
From looking closer at URLBlacklist - it lives in //components/policy/core/browser - can //net depend on that, or is that going to prevent reusing that code for this?
//net can't use that code, but that's also more heavyweight than you need. Sorry, I did a bad job capturing - I was suggesting the format for expressing hosts and subdomains (see https://dev.chromium.org/administrators/url-blacklist-filter-format ), rather than the implementation. The syntax is rather easy to parse directly, but just making sure we don't introduce yet-another-syntax is the goal :)

(And of course, I expressed the syntax wrong - foo.com matches foo and subdomains, .foo.com only matches foo.com)
In this syntax, "*" is only used to mean "all hostnames", and not used in a context like "*.foo.com"? I also take it that "foo.com" matches "foo.com", "bar.foo.com", but not "barfoo.com"?

AutoSelectCertificateForUrls appears to have a different syntax for its patterns (it uses ContentSettingsPattern), and AuthNegotiateDelegateWhitelist has yet another syntax (using ProxyBypassRules). I wouldn't be surprised if there are other formats used for other policies.

I think the URLBlacklist format (using just the bit for hosts/subdomains) is the simplest format for the constraints we want, but I wouldn't be opposed to using a slightly different format if that meant more code reuse.
Ah, good spot.

Nick and I had an offline chat, where we explored the potential implications for the use case of the syntax, and agreed that the syntax doesn't really matter here, especially with it being a temporary policy. We chatted about whether we needed Include+Exclude syntax, or whether just "This name and all subdomains" suffice - and from both the immediate use case and the transition to the longer term solution (see related internal bug linked in Issue 849201), we can get away with just "this name and all subdomains".

If there are any issues with the policy (only supporting includes + subdomains), the server operators can control that behaviour by the names expressed in the cert, so we're good there.

As the URLBlacklist also relies on the URLFixer to format URLs (same as the omnibox), that's another compelling reason to skip that exact syntax, and just make it a 'host list that includes subdomains' and require the policy admin to configure it correctly.
Labels: Enterprise-Triaged
Owner: atwilson@chromium.org
Assigning to atwilson@ for CrOS triage.
Project Member

Comment 12 by bugdroid1@chromium.org, Jul 31

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/89bc721d488add3b806c6bcc5c05b00fcad22451

commit 89bc721d488add3b806c6bcc5c05b00fcad22451
Author: Nick Harper <nharper@chromium.org>
Date: Tue Jul 31 19:07:57 2018

Add field to SSLConfig to allow H2 connection coalescing with client certificates

Bug:  855690 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I092d9f3bc7fcf5eb507bbc3f4e9f27ce09931532
Reviewed-on: https://chromium-review.googlesource.com/1125296
Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Reviewed-by: Chris Palmer <palmer@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Commit-Queue: Nick Harper <nharper@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579515}
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/android_webview/browser/net/aw_url_request_context_getter.cc
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/net/http/http_network_transaction_ssl_unittest.cc
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/net/http/http_network_transaction_unittest.cc
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/net/http/http_proxy_client_socket_wrapper_unittest.cc
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/net/quic/chromium/bidirectional_stream_quic_impl_unittest.cc
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/net/quic/chromium/quic_chromium_client_session.cc
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/net/quic/chromium/quic_chromium_client_session.h
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/net/quic/chromium/quic_chromium_client_session_test.cc
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/net/quic/chromium/quic_http_stream_test.cc
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/net/quic/chromium/quic_proxy_client_socket_unittest.cc
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/net/quic/chromium/quic_stream_factory.cc
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/net/quic/chromium/quic_stream_factory_test.cc
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/net/spdy/spdy_session.cc
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/net/spdy/spdy_session.h
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/net/spdy/spdy_session_pool.cc
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/net/spdy/spdy_session_unittest.cc
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/net/ssl/ssl_config_service.cc
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/net/ssl/ssl_config_service.h
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/net/ssl/ssl_config_service_defaults.cc
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/net/ssl/ssl_config_service_defaults.h
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/net/ssl/ssl_config_service_unittest.cc
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/net/url_request/url_request_unittest.cc
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/services/network/public/mojom/ssl_config.mojom
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/services/network/ssl_config_service_mojo.cc
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/services/network/ssl_config_service_mojo.h
[modify] https://crrev.com/89bc721d488add3b806c6bcc5c05b00fcad22451/services/network/ssl_config_service_mojo_unittest.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Aug 6

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f2e551fa5ca7acca74bfee0ccf0d94f3f609eaa8

commit f2e551fa5ca7acca74bfee0ccf0d94f3f609eaa8
Author: Ryan Sleevi <rsleevi@chromium.org>
Date: Mon Aug 06 21:32:51 2018

Fix use-after-move when setting the SSLConfig

Bug:  855690 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I9029b2c3fe66cfb56423ee3fc6e8590547401533
Reviewed-on: https://chromium-review.googlesource.com/1163926
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Ryan Sleevi <rsleevi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580993}
[modify] https://crrev.com/f2e551fa5ca7acca74bfee0ccf0d94f3f609eaa8/services/network/ssl_config_service_mojo.cc

Nick, are you driving this? i'm trying to understand what my team's role should be (if any).
Cc: -nhar...@chromium.org atwilson@chromium.org
Owner: nhar...@chromium.org
Status: Assigned (was: Untriaged)
not nick, but chiming in for timezones: Nick's got a CL out for wiring up the policy ( https://chromium-review.googlesource.com/c/chromium/src/+/1159840 ). Beyond reviews, that "should" cover it. The flag is marked 'future' because it's really only intended for one specific organization as they migrate off Channel ID, and my understanding is that because it's only one specific organization, it doesn't require any server-side changes. Does that match your understanding? If so, it 'sounds' like wiring it up w/ "future" should be sufficient - correct?
As long as we have a way to set that policy directly in the policy store, agreed, we shouldn't need server changes beyond syncing the new proto definition.
Project Member

Comment 17 by bugdroid1@chromium.org, Aug 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/598b280c5f47a8fc4d6a6a71b2fd8337026a8240

commit 598b280c5f47a8fc4d6a6a71b2fd8337026a8240
Author: Nick Harper <nharper@chromium.org>
Date: Fri Aug 10 19:57:11 2018

Add enterpise policy to control SSLConfig.client_cert_pooling_policy

Bug:  855690 
Change-Id: I19d0b1ca0770c9016b0197bf435cf47affe49a94
Reviewed-on: https://chromium-review.googlesource.com/1159840
Commit-Queue: Nick Harper <nharper@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582300}
[modify] https://crrev.com/598b280c5f47a8fc4d6a6a71b2fd8337026a8240/chrome/browser/policy/configuration_policy_handler_list_factory.cc
[modify] https://crrev.com/598b280c5f47a8fc4d6a6a71b2fd8337026a8240/chrome/browser/ssl/ssl_config_service_manager_pref.cc
[modify] https://crrev.com/598b280c5f47a8fc4d6a6a71b2fd8337026a8240/chrome/browser/ssl/ssl_config_service_manager_pref_unittest.cc
[modify] https://crrev.com/598b280c5f47a8fc4d6a6a71b2fd8337026a8240/chrome/common/pref_names.cc
[modify] https://crrev.com/598b280c5f47a8fc4d6a6a71b2fd8337026a8240/chrome/common/pref_names.h
[modify] https://crrev.com/598b280c5f47a8fc4d6a6a71b2fd8337026a8240/chrome/test/data/policy/policy_test_cases.json
[modify] https://crrev.com/598b280c5f47a8fc4d6a6a71b2fd8337026a8240/components/policy/resources/policy_templates.json
[modify] https://crrev.com/598b280c5f47a8fc4d6a6a71b2fd8337026a8240/tools/metrics/histograms/enums.xml

Blocking: 875053
Status: Fixed (was: Assigned)
Hi..
Could you please provide steps, if the issue requires manual verification.
Ping.
Labels: Needs-Feedback
Status: Closed (was: Fixed)
 No manually verification was done on this issue. Closing due to no new issues arising from this new policy

Sign in to add a comment