Add Enterprise Policy to allow HTTP/2 connection coalescing on connections with client certs |
||||||||||
Issue descriptionCurrently, 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)
,
Jun 22 2018
,
Jun 22 2018
,
Jun 22 2018
(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)
,
Jun 22 2018
@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 :)
,
Jun 22 2018
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.
,
Jun 22 2018
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?
,
Jun 22 2018
//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)
,
Jun 22 2018
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.
,
Jun 22 2018
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.
,
Jul 17
Assigning to atwilson@ for CrOS triage.
,
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
,
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
,
Aug 7
Nick, are you driving this? i'm trying to understand what my team's role should be (if any).
,
Aug 7
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?
,
Aug 7
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.
,
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
,
Aug 16
,
Sep 5
,
Sep 11
Hi.. Could you please provide steps, if the issue requires manual verification.
,
Oct 6
Ping.
,
Oct 9
,
Oct 19
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 |
||||||||||
Comment 1 by rsleevi@chromium.org
, Jun 22 2018