Skip to content

Commit

Permalink
Revert "Use uncredentialed requests to Gaia"
Browse files Browse the repository at this point in the history
This reverts commit c262371.

Reason for revert: The changes in this CL trigger a DCHECK when running Chrome. See below for callstack and the command I used to build Chrome.

Callstack:
2023-02-17T20:24:19.978808Z FATAL chrome[1213141:1213233]: [feature_list.cc(671)] Check failed: CheckFeatureIdentity(feature). GaiaCredentialsModeOmitBug_775438_Workaround
#0 0x7f7c3d2a77f2 base::debug::CollectStackTrace()
#1 0x7f7c3d289e03 base::debug::StackTrace::StackTrace()
#2 0x7f7c3d1654f0 logging::LogMessage::~LogMessage()
#3 0x7f7c3d165fce logging::LogMessage::~LogMessage()
#4 0x7f7c3d154849 base::FeatureList::GetOverrideState()
#5 0x7f7c3d15337d base::FeatureList::IsEnabled()
#6 0x7f7c34ed2797 google_apis::GetOmitCredentialsModeForGaiaRequests()
#7 0x7f7c34ec0e69 gcm::RegistrationRequest::Start()
#8 0x562a357cded1 gcm::GCMClientImpl::Register()
#9 0x562a357d5f23 gcm::GCMDriverDesktop::IOWorker::Register()
#10 0x7f7c3d1daf5a base::TaskAnnotator::RunTaskImpl()
#11 0x7f7c3d2051f8 base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWorkImpl()
#12 0x7f7c3d204633 base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWork()
#13 0x7f7c3d205d15 base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWork()
#14 0x7f7c3d2b34e3 base::MessagePumpEpoll::Run()
#15 0x7f7c3d206232 base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::Run()
#16 0x7f7c3d1ab82a base::RunLoop::Run()
#17 0x7f7c3d239687 base::Thread::Run()
#18 0x7f7c36876359 content::BrowserProcessIOThread::IOThreadRun()
#19 0x7f7c368762ec content::BrowserProcessIOThread::Run()
#20 0x7f7c3d2399eb base::Thread::ThreadMain()
#21 0x7f7c3d25810b base::(anonymous namespace)::ThreadFunc()
#22 0x7f7c29aa7fd4 (/usr/lib/x86_64-linux-gnu/libc.so.6+0x88fd3)
#23 0x7f7c29b2866c (/usr/lib/x86_64-linux-gnu/libc.so.6+0x10966b)

The specific command I used to build Chrome:
```
cd ~/chromium/src; autoninja -C \${out_dir} \
chrome && \${out_dir}/chrome \
--user-data-dir=~/.config/cros \
--use-system-clipboard \
--ash-host-window-bounds="40+40-1920x1080" \
--enable-logging=stderr \
--enable-features=ShimlessRMAFlow,OsFeedback,ShortcutCustomizationApp,ShortcutCustomization,EnableInputInDiagnosticsApp,SearchInShortcutsApp
```

Original change's description:
> Use uncredentialed requests to Gaia
>
> Adopting kOmitBug_775438_Workaround makes the browser spec-compliant and
> avoids that developers need to use workarounds such as
> --ignore-urlfetcher-cert-requests in order to communicate with non-prod
> server environments.
>
> The change is guarded with a kill switch but is otherwise enabled by
> default.
>
> Below a summary extracted from devidben@'s nice explanation of what this
> patch does (readers may find the full Gerrit comment thread
> interesting):
>
> The concrete behavior change is around client certs. Client certs work as follows:
>
> 1. Either the origin server (depends on what URL you're connecting to)
> or the proxy server (depends on the user's network config) can, in a TLS
> connection request client certs.
>
> 2. If we already have a client cert decision recorded for that server, we just use it.
>
> 3. Otherwise, we show a prompt to the user.
>
> 4. If the request isn't associated with some tab, we have no place to
> show the prompt and we just abort the request. (We cannot continue
> without a certificate because that'll persist the "send no certificate"
> decision in the net stack... this auth mechanism is somewhat unavoidably
> sticky.)
>
> This CL will change the behavior for just origin-requested client certs (not proxy-requested) to, instead of steps (2-4), unconditionally continue the request with no client certificate.
>
> Change-Id: I34908fa81c1688ebaf7feb801408d9080207453c
> Fixed: 1221565
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4255589
> Reviewed-by: Maks Orlovich <morlovich@chromium.org>
> Commit-Queue: Mikel Astiz <mastiz@chromium.org>
> Reviewed-by: Rohit Rao <rohitrao@chromium.org>
> Reviewed-by: Alex Ilin <alexilin@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1106846}

Change-Id: Ib67b7801b007b8bf56a3f65fb0677b94c4719ab9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4265133
Owners-Override: David Baron <dbaron@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: David Baron <dbaron@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1106998}
  • Loading branch information
Camden Bickel authored and Chromium LUCI CQ committed Feb 17, 2023
1 parent 5b9fc36 commit f392ad5
Show file tree
Hide file tree
Showing 25 changed files with 41 additions and 136 deletions.
4 changes: 1 addition & 3 deletions chrome/browser/ash/login/gaia_reauth_token_fetcher.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
#include "chrome/browser/browser_process.h"
#include "chrome/browser/net/system_network_context_manager.h"
#include "chromeos/ash/components/login/auth/recovery/service_constants.h"
#include "google_apis/credentials_mode.h"
#include "google_apis/google_api_keys.h"
#include "net/base/load_flags.h"
#include "net/base/url_util.h"
Expand Down Expand Up @@ -60,8 +59,7 @@ void GaiaReauthTokenFetcher::Fetch() {
resource_request->url = GetFetchReauthTokenUrl();
resource_request->load_flags =
net::LOAD_DISABLE_CACHE | net::LOAD_DO_NOT_SAVE_COOKIES;
resource_request->credentials_mode =
google_apis::GetOmitCredentialsModeForGaiaRequests();
resource_request->credentials_mode = network::mojom::CredentialsMode::kOmit;
resource_request->method = "GET";

// TODO(b/197615068): Update the "policy" field in the traffic
Expand Down
4 changes: 1 addition & 3 deletions chrome/browser/ash/login/marketing_backend_connector.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
#include "components/signin/public/identity_manager/scope_set.h"
#include "components/user_manager/user.h"
#include "components/user_manager/user_manager.h"
#include "google_apis/credentials_mode.h"
#include "net/base/load_flags.h"
#include "net/http/http_request_headers.h"
#include "net/http/http_status_code.h"
Expand Down Expand Up @@ -66,8 +65,7 @@ std::unique_ptr<network::ResourceRequest> GetResourceRequest() {
auto resource_request = std::make_unique<network::ResourceRequest>();
resource_request->url = GetChromebookServiceEndpoint();
resource_request->load_flags = net::LOAD_DISABLE_CACHE;
resource_request->credentials_mode =
google_apis::GetOmitCredentialsModeForGaiaRequests();
resource_request->credentials_mode = network::mojom::CredentialsMode::kOmit;
resource_request->method = "POST";
return resource_request;
}
Expand Down
4 changes: 1 addition & 3 deletions chrome/browser/ash/login/saml/password_sync_token_fetcher.cc
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
#include "components/user_manager/known_user.h"
#include "content/public/browser/browser_context.h"
#include "content/public/common/url_constants.h"
#include "google_apis/credentials_mode.h"
#include "google_apis/gaia/gaia_auth_fetcher.h"
#include "google_apis/gaia/gaia_constants.h"
#include "google_apis/gaia/google_service_auth_error.h"
Expand Down Expand Up @@ -242,8 +241,7 @@ void PasswordSyncTokenFetcher::FetchSyncToken(const std::string& access_token) {
}
resource_request->load_flags =
net::LOAD_DISABLE_CACHE | net::LOAD_BYPASS_CACHE;
resource_request->credentials_mode =
google_apis::GetOmitCredentialsModeForGaiaRequests();
resource_request->credentials_mode = network::mojom::CredentialsMode::kOmit;
if (request_type_ == RequestType::kCreateToken) {
resource_request->method = net::HttpRequestHeaders::kPostMethod;
} else {
Expand Down
4 changes: 1 addition & 3 deletions chrome/browser/ash/login/users/avatar/user_image_loader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
#include "chrome/browser/browser_process.h"
#include "chrome/browser/ui/ash/image_downloader_impl.h"
#include "components/user_manager/user_image/user_image.h"
#include "google_apis/credentials_mode.h"
#include "ipc/ipc_channel.h"
#include "net/traffic_annotation/network_traffic_annotation.h"
#include "services/data_decoder/public/cpp/data_decoder.h"
Expand Down Expand Up @@ -439,8 +438,7 @@ void StartWithGURLAnimated(const GURL& default_image_url,

auto request = std::make_unique<network::ResourceRequest>();
request->url = default_image_url;
request->credentials_mode =
google_apis::GetOmitCredentialsModeForGaiaRequests();
request->credentials_mode = network::mojom::CredentialsMode::kOmit;

auto loader = network::SimpleURLLoader::Create(std::move(request),
kNetworkTrafficAnnotationTag);
Expand Down
1 change: 0 additions & 1 deletion components/signin/internal/identity_manager/DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,5 @@ include_rules = [
"+components/signin/public/webdata",
"+components/signin/public/android/jni_headers",
"+components/signin/public/android/test_support_jni_headers",
"+google_apis",
"+mojo/public",
]
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
#include "components/signin/public/base/signin_metrics.h"
#include "components/signin/public/base/signin_pref_names.h"
#include "components/signin/public/identity_manager/set_accounts_in_cookie_result.h"
#include "google_apis/credentials_mode.h"
#include "google_apis/gaia/gaia_constants.h"
#include "google_apis/gaia/gaia_urls.h"
#include "net/base/load_flags.h"
Expand Down Expand Up @@ -348,8 +347,7 @@ GaiaCookieManagerService::ExternalCcResultFetcher::CreateAndStartLoader(

auto request = std::make_unique<network::ResourceRequest>();
request->url = url;
request->credentials_mode =
google_apis::GetOmitCredentialsModeForGaiaRequests();
request->credentials_mode = network::mojom::CredentialsMode::kOmit;

std::unique_ptr<network::SimpleURLLoader> loader =
network::SimpleURLLoader::Create(std::move(request), traffic_annotation);
Expand Down
4 changes: 1 addition & 3 deletions components/sync/driver/sync_stopped_reporter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
#include "base/metrics/histogram_functions.h"
#include "base/strings/stringprintf.h"
#include "components/sync/protocol/sync.pb.h"
#include "google_apis/credentials_mode.h"
#include "net/base/load_flags.h"
#include "net/http/http_status_code.h"
#include "net/traffic_annotation/network_traffic_annotation.h"
Expand Down Expand Up @@ -111,8 +110,7 @@ void SyncStoppedReporter::ReportSyncStopped(const std::string& access_token,
resource_request->url = sync_event_url_;
resource_request->load_flags =
net::LOAD_BYPASS_CACHE | net::LOAD_DISABLE_CACHE;
resource_request->credentials_mode =
google_apis::GetOmitCredentialsModeForGaiaRequests();
resource_request->credentials_mode = network::mojom::CredentialsMode::kOmit;
resource_request->method = "POST";
resource_request->headers.SetHeader(
net::HttpRequestHeaders::kAuthorization,
Expand Down
4 changes: 1 addition & 3 deletions components/sync/engine/net/http_bridge.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
#include "base/task/thread_pool.h"
#include "base/threading/thread_restrictions.h"
#include "components/variations/net/variations_http_headers.h"
#include "google_apis/credentials_mode.h"
#include "net/base/load_flags.h"
#include "net/base/net_errors.h"
#include "net/http/http_request_headers.h"
Expand Down Expand Up @@ -243,8 +242,7 @@ void HttpBridge::MakeAsynchronousPost() {
resource_request->method = "POST";
resource_request->load_flags =
net::LOAD_BYPASS_CACHE | net::LOAD_DISABLE_CACHE;
resource_request->credentials_mode =
google_apis::GetOmitCredentialsModeForGaiaRequests();
resource_request->credentials_mode = network::mojom::CredentialsMode::kOmit;

if (!extra_headers_.empty())
resource_request->headers.AddHeadersFromString(extra_headers_);
Expand Down
4 changes: 1 addition & 3 deletions components/sync/trusted_vault/trusted_vault_request.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
#include "components/sync/driver/trusted_vault_histograms.h"
#include "components/sync/trusted_vault/trusted_vault_access_token_fetcher.h"
#include "components/sync/trusted_vault/trusted_vault_server_constants.h"
#include "google_apis/credentials_mode.h"
#include "net/base/url_util.h"
#include "net/http/http_request_headers.h"
#include "net/http/http_status_code.h"
Expand Down Expand Up @@ -179,8 +178,7 @@ std::unique_ptr<network::SimpleURLLoader> TrustedVaultRequest::CreateURLLoader(
net::AppendQueryParameter(request_url_, kQueryParameterAlternateOutputKey,
kQueryParameterAlternateOutputProto);
request->method = GetHttpMethodString(http_method_);
request->credentials_mode =
google_apis::GetOmitCredentialsModeForGaiaRequests();
request->credentials_mode = network::mojom::CredentialsMode::kOmit;
request->headers.SetHeader(
kAuthorizationHeader,
/*value=*/base::StringPrintf("Bearer %s", access_token.c_str()));
Expand Down
4 changes: 1 addition & 3 deletions google_apis/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,6 @@ config("key_defines") {
template("google_apis_tmpl") {
source_set(target_name) {
sources = [
"credentials_mode.cc",
"credentials_mode.h",
"gaia/core_account_id.cc",
"gaia/core_account_id.h",
"gaia/gaia_access_token_fetcher.cc",
Expand Down Expand Up @@ -168,7 +166,6 @@ template("google_apis_tmpl") {
public_deps = [
":buildflags",
"//build:chromeos_buildflags",
"//services/network/public/cpp",
]

deps = [
Expand All @@ -179,6 +176,7 @@ template("google_apis_tmpl") {
"//build:chromeos_buildflags",
"//crypto",
"//mojo/public/cpp/bindings:struct_traits",
"//services/network/public/cpp",
]

if (_use_official_google_keys_and_generate_metrics_key_header) {
Expand Down
3 changes: 1 addition & 2 deletions google_apis/common/base_requests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
#include "base/values.h"
#include "google_apis/common/request_sender.h"
#include "google_apis/common/task_util.h"
#include "google_apis/credentials_mode.h"
#include "net/base/load_flags.h"
#include "net/http/http_util.h"
#include "services/network/public/cpp/resource_request.h"
Expand Down Expand Up @@ -175,7 +174,7 @@ void UrlFetchRequestBase::StartAfterPrepare(
request->url = url;
request->method = GetRequestType();
request->load_flags = net::LOAD_DISABLE_CACHE;
request->credentials_mode = GetOmitCredentialsModeForGaiaRequests();
request->credentials_mode = network::mojom::CredentialsMode::kOmit;

// Add request headers.
// Note that SetHeader clears the current headers and sets it to the passed-in
Expand Down
27 changes: 0 additions & 27 deletions google_apis/credentials_mode.cc

This file was deleted.

19 changes: 0 additions & 19 deletions google_apis/credentials_mode.h

This file was deleted.

31 changes: 14 additions & 17 deletions google_apis/gaia/gaia_auth_fetcher.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
#include "base/system/sys_info.h"
#include "base/types/optional_util.h"
#include "base/values.h"
#include "google_apis/credentials_mode.h"
#include "google_apis/gaia/gaia_auth_consumer.h"
#include "google_apis/gaia/gaia_auth_util.h"
#include "google_apis/gaia/gaia_constants.h"
Expand Down Expand Up @@ -267,9 +266,7 @@ void GaiaAuthFetcher::CreateAndStartGaiaFetcher(
resource_request->url = gaia_gurl;
original_url_ = gaia_gurl;

if (credentials_mode != network::mojom::CredentialsMode::kOmit &&
credentials_mode !=
network::mojom::CredentialsMode::kOmitBug_775438_Workaround) {
if (credentials_mode != network::mojom::CredentialsMode::kOmit) {
CHECK(gaia::HasGaiaSchemeHostPort(gaia_gurl)) << gaia_gurl;

url::Origin origin = GaiaUrls::GetInstance()->gaia_origin();
Expand Down Expand Up @@ -413,10 +410,10 @@ void GaiaAuthFetcher::StartRevokeOAuth2Token(const std::string& auth_token) {
}
}
})");
CreateAndStartGaiaFetcher(
request_body_, kFormEncodedContentType, std::string(),
oauth2_revoke_gurl_, google_apis::GetOmitCredentialsModeForGaiaRequests(),
traffic_annotation);
CreateAndStartGaiaFetcher(request_body_, kFormEncodedContentType,
std::string(), oauth2_revoke_gurl_,
network::mojom::CredentialsMode::kOmit,
traffic_annotation);
}

void GaiaAuthFetcher::StartAuthCodeForOAuth2TokenExchange(
Expand Down Expand Up @@ -461,7 +458,7 @@ void GaiaAuthFetcher::StartAuthCodeForOAuth2TokenExchangeWithDeviceId(
})");
CreateAndStartGaiaFetcher(
request_body_, kFormEncodedContentType, std::string(), oauth2_token_gurl_,
google_apis::GetOmitCredentialsModeForGaiaRequests(), traffic_annotation);
network::mojom::CredentialsMode::kOmit, traffic_annotation);
}

void GaiaAuthFetcher::StartMergeSession(const std::string& uber_token,
Expand Down Expand Up @@ -552,7 +549,7 @@ void GaiaAuthFetcher::StartTokenFetchForUberAuthExchange(
})");
CreateAndStartGaiaFetcher(
std::string(), std::string(), authentication_header, uberauth_token_gurl_,
google_apis::GetOmitCredentialsModeForGaiaRequests(), traffic_annotation);
network::mojom::CredentialsMode::kOmit, traffic_annotation);
}

void GaiaAuthFetcher::StartListAccounts() {
Expand Down Expand Up @@ -750,9 +747,9 @@ void GaiaAuthFetcher::StartCreateReAuthProofTokenForParent(
DCHECK(reauth_url.is_valid());

// Start the request.
CreateAndStartGaiaFetcher(
post_body, kJsonContentType, headers, reauth_url,
google_apis::GetOmitCredentialsModeForGaiaRequests(), traffic_annotation);
CreateAndStartGaiaFetcher(post_body, kJsonContentType, headers, reauth_url,
network::mojom::CredentialsMode::kOmit,
traffic_annotation);
}

void GaiaAuthFetcher::StartGetCheckConnectionInfo() {
Expand Down Expand Up @@ -783,10 +780,10 @@ void GaiaAuthFetcher::StartGetCheckConnectionInfo() {
}
}
})");
CreateAndStartGaiaFetcher(
std::string(), std::string(), std::string(),
get_check_connection_info_url_,
google_apis::GetOmitCredentialsModeForGaiaRequests(), traffic_annotation);
CreateAndStartGaiaFetcher(std::string(), std::string(), std::string(),
get_check_connection_info_url_,
network::mojom::CredentialsMode::kOmit,
traffic_annotation);
}

// static
Expand Down

0 comments on commit f392ad5

Please sign in to comment.