Prefer privacy_mode in QuicSessionKey over QuicServerId

This fixes some easy instances of the privacy_mode_enabled boolean.
Rather than reconstruct the enum out of QuicServerId's boolean, use the
Chromium accessor. (The Chromium accessor currently reconstructs things
itself, but I'm hoping we can change that. Either way, this avoids some
duplicated code.)

Where privacy_mode_enabled was logged as a boolean, I've switched to a
string rperesentation pulled out of url_request_netlog_params.cc. Also
I've filled in a few missing parts of QuicSessionKey in one of the log
events and sync'd up with the comments in net/log.

Bug: 1103350
Change-Id: I6906fa802f6ebb0a8b6c187519df68a74a1d3b13
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2296629
Commit-Queue: David Benjamin <davidben@chromium.org>
Commit-Queue: Nick Harper <nharper@chromium.org>
Auto-Submit: David Benjamin <davidben@chromium.org>
Reviewed-by: Nick Harper <nharper@chromium.org>
Cr-Commit-Position: refs/heads/master@{#788410}

net/BUILD.gn

@@ -158,6 +158,7 @@ component("net") {
     "base/parse_number.h",
     "base/port_util.cc",
     "base/port_util.h",
+    "base/privacy_mode.cc",
     "base/privacy_mode.h",
     "base/rand_callback.h",
     "base/registry_controlled_domains/registry_controlled_domain.cc",

net/base/privacy_mode.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/base/privacy_mode.h"
+
+#include "base/notreached.h"
+
+namespace net {
+
+const char* PrivacyModeToDebugString(PrivacyMode privacy_mode) {
+  switch (privacy_mode) {
+    case PRIVACY_MODE_DISABLED:
+      return "disabled";
+    case PRIVACY_MODE_ENABLED:
+      return "enabled";
+    case PRIVACY_MODE_ENABLED_WITHOUT_CLIENT_CERTS:
+      return "enabled without client certs";
+  }
+  NOTREACHED();
+  return "";
+}
+
+}  // namespace net

net/base/privacy_mode.h

@@ -18,6 +18,8 @@ enum PrivacyMode {
   PRIVACY_MODE_ENABLED_WITHOUT_CLIENT_CERTS = 2,
 };
 
+const char* PrivacyModeToDebugString(PrivacyMode privacy_mode);
+
 }  // namespace net
 
 #endif  // NET_BASE_PRIVACY_MODE_H_

net/log/net_log_event_type_list.h

@@ -1670,7 +1670,10 @@ EVENT_TYPE(HTTP2_PROXY_CLIENT_SESSION)
 // Measures the time taken to execute the QuicStreamFactory::Job.
 // The event parameters are:
 //   {
-//     "server_id": <The quic::QuicServerId that the Job serves>,
+//     "host": <The origin hostname that the Job serves>,
+//     "port": <The origin port>,
+//     "privacy_mode": <The privacy mode of the Job>,
+//     "network_isolation_key": <The NetworkIsolationKey of the Job>,
 //   }
 EVENT_TYPE(QUIC_STREAM_FACTORY_JOB)
 
@@ -1714,7 +1717,14 @@ EVENT_TYPE(QUIC_STREAM_FACTORY_JOB_STALE_HOST_RESOLUTION_MATCHED)
 
 // The start/end of a quic::QuicSession.
 //   {
-//     "host": <The host-port string>,
+//     "host": <The origin hostname string>,
+//     "port": <The origin port>,
+//     "privacy_mode": <The privacy mode of the session>,
+//     "network_isolation_key": <The NetworkIsolationKey of the session>,
+//     "require_confirmation": <True if the session will wait for a successful
+//                              QUIC handshake before vending streams>,
+//     "cert_verify_flags": <The certificate verification flags for the
+//                           session>,
 //   }
 EVENT_TYPE(QUIC_SESSION)
 

net/quic/quic_chromium_client_session.cc

@@ -26,6 +26,7 @@
 #include "net/base/net_errors.h"
 #include "net/base/network_activity_monitor.h"
 #include "net/base/network_isolation_key.h"
+#include "net/base/privacy_mode.h"
 #include "net/base/url_util.h"
 #include "net/http/transport_security_state.h"
 #include "net/log/net_log_event_type.h"
@@ -270,25 +271,28 @@ std::string MigrationCauseToString(MigrationCause cause) {
 }
 
 base::Value NetLogQuicClientSessionParams(
-    const quic::QuicServerId* server_id,
+    const QuicSessionKey* session_key,
     const quic::QuicConnectionId& connection_id,
     const quic::QuicConnectionId& client_connection_id,
     const quic::ParsedQuicVersionVector& supported_versions,
     int cert_verify_flags,
     bool require_confirmation) {
-  base::DictionaryValue dict;
-  dict.SetString("host", server_id->host());
-  dict.SetInteger("port", server_id->port());
-  dict.SetBoolean("privacy_mode", server_id->privacy_mode_enabled());
-  dict.SetBoolean("require_confirmation", require_confirmation);
-  dict.SetInteger("cert_verify_flags", cert_verify_flags);
-  dict.SetString("connection_id", connection_id.ToString());
+  base::Value dict(base::Value::Type::DICTIONARY);
+  dict.SetStringKey("host", session_key->server_id().host());
+  dict.SetIntKey("port", session_key->server_id().port());
+  dict.SetStringKey("privacy_mode",
+                    PrivacyModeToDebugString(session_key->privacy_mode()));
+  dict.SetStringKey("network_isolation_key",
+                    session_key->network_isolation_key().ToDebugString());
+  dict.SetBoolKey("require_confirmation", require_confirmation);
+  dict.SetIntKey("cert_verify_flags", cert_verify_flags);
+  dict.SetStringKey("connection_id", connection_id.ToString());
   if (!client_connection_id.IsEmpty()) {
-    dict.SetString("client_connection_id", client_connection_id.ToString());
+    dict.SetStringKey("client_connection_id", client_connection_id.ToString());
   }
-  dict.SetString("versions",
-                 ParsedQuicVersionVectorToString(supported_versions));
-  return std::move(dict);
+  dict.SetStringKey("versions",
+                    ParsedQuicVersionVectorToString(supported_versions));
+  return dict;
 }
 
 base::Value NetLogQuicPushPromiseReceivedParams(
@@ -881,9 +885,8 @@ QuicChromiumClientSession::QuicChromiumClientSession(
   migrate_back_to_default_timer_.SetTaskRunner(task_runner_);
   net_log_.BeginEvent(NetLogEventType::QUIC_SESSION, [&] {
     return NetLogQuicClientSessionParams(
-        &session_key.server_id(), connection_id(),
-        connection->client_connection_id(), supported_versions(),
-        cert_verify_flags, require_confirmation_);
+        &session_key, connection_id(), connection->client_connection_id(),
+        supported_versions(), cert_verify_flags, require_confirmation_);
   });
   IPEndPoint address;
   if (socket_raw && socket_raw->GetLocalAddress(&address) == OK &&

net/quic/quic_stream_factory.cc

@@ -109,14 +109,15 @@ enum class ConnectionStateAfterDNS {
 };
 
 base::Value NetLogQuicStreamFactoryJobParams(
-    const quic::QuicServerId* server_id) {
-  base::DictionaryValue dict;
-  dict.SetString(
-      "server_id",
-      "https://" +
-          HostPortPair(server_id->host(), server_id->port()).ToString() +
-          (server_id->privacy_mode_enabled() ? "/private" : ""));
-  return std::move(dict);
+    const QuicStreamFactory::QuicSessionAliasKey* key) {
+  base::Value dict(base::Value::Type::DICTIONARY);
+  dict.SetStringKey("host", key->server_id().host());
+  dict.SetIntKey("port", key->server_id().port());
+  dict.SetStringKey("privacy_mode", PrivacyModeToDebugString(
+                                        key->session_key().privacy_mode()));
+  dict.SetStringKey("network_isolation_key",
+                    key->session_key().network_isolation_key().ToDebugString());
+  return dict;
 }
 
 std::string QuicPlatformNotificationToString(
@@ -501,9 +502,8 @@ QuicStreamFactory::Job::Job(
       connection_retried_(false),
       session_(nullptr),
       network_(NetworkChangeNotifier::kInvalidNetworkHandle) {
-  net_log_.BeginEvent(NetLogEventType::QUIC_STREAM_FACTORY_JOB, [&] {
-    return NetLogQuicStreamFactoryJobParams(&key_.server_id());
-  });
+  net_log_.BeginEvent(NetLogEventType::QUIC_STREAM_FACTORY_JOB,
+                      [&] { return NetLogQuicStreamFactoryJobParams(&key_); });
   // Associate |net_log_| with |net_log|.
   net_log_.AddEventReferencingSource(
       NetLogEventType::QUIC_STREAM_FACTORY_JOB_BOUND_TO_HTTP_STREAM_JOB,
@@ -1180,14 +1180,17 @@ int QuicStreamFactory::Create(const QuicSessionKey& session_key,
                       session_key.server_id().port())
              .Equals(HostPortPair::FromURL(url)));
   // Enforce session affinity for promised streams.
+  //
+  // TODO(https://crbug.com/1105544): This logic should also handle
+  // NetworkIsolationKey.
   quic::QuicClientPromisedInfo* promised =
       push_promise_index_.GetPromised(url.spec());
   if (promised) {
     QuicChromiumClientSession* session =
         static_cast<QuicChromiumClientSession*>(promised->session());
     DCHECK(session);
-    if (session->server_id().privacy_mode_enabled() ==
-        session_key.server_id().privacy_mode_enabled()) {
+    if (session->quic_session_key().privacy_mode() ==
+        session_key.privacy_mode()) {
       request->SetSession(session->CreateHandle(destination));
       ++num_push_streams_created_;
       return OK;
@@ -1229,10 +1232,7 @@ int QuicStreamFactory::Create(const QuicSessionKey& session_key,
       QuicChromiumClientSession* session = key_value.second;
       if (destination.Equals(all_sessions_[session].destination()) &&
           session->CanPool(session_key.server_id().host(),
-                           session_key.server_id().privacy_mode_enabled()
-                               ? PRIVACY_MODE_ENABLED
-                               : PRIVACY_MODE_DISABLED,
-                           session_key.socket_tag(),
+                           session_key.privacy_mode(), session_key.socket_tag(),
                            session_key.network_isolation_key(),
                            session_key.disable_secure_dns())) {
         request->SetSession(session->CreateHandle(destination));

net/url_request/url_request_netlog_params.cc

@@ -16,22 +16,6 @@
 
 namespace net {
 
-namespace {
-
-std::string PrivacyModeDebugString(PrivacyMode privacy_mode) {
-  switch (privacy_mode) {
-    case PRIVACY_MODE_DISABLED:
-      return "disabled";
-    case PRIVACY_MODE_ENABLED:
-      return "enabled";
-    case PRIVACY_MODE_ENABLED_WITHOUT_CLIENT_CERTS:
-      return "enabled without client certs";
-  }
-  return "";
-}
-
-}  // namespace
-
 base::Value NetLogURLRequestConstructorParams(
     const GURL& url,
     RequestPriority priority,
@@ -56,7 +40,7 @@ base::Value NetLogURLRequestStartParams(
   dict.SetStringKey("url", url.possibly_invalid_spec());
   dict.SetStringKey("method", method);
   dict.SetIntKey("load_flags", load_flags);
-  dict.SetStringKey("privacy_mode", PrivacyModeDebugString(privacy_mode));
+  dict.SetStringKey("privacy_mode", PrivacyModeToDebugString(privacy_mode));
   dict.SetStringKey("network_isolation_key",
                     network_isolation_key.ToDebugString());
   dict.SetStringKey("site_for_cookies", site_for_cookies.ToDebugString());