From 9001c8ca3fa4ccd1dee1c54dbedf3e2619179f11 Mon Sep 17 00:00:00 2001 From: "cbentzel@chromium.org" Date: Thu, 13 May 2010 16:21:40 +0000 Subject: Added authentication scheme as key to HttpAuthCache. Behavioral changes are small; this is mostly a syntactic sugar change. But there are a few behavioral changes: * If a web site replies with different schemes for the same realm, we'll have two entries in the cache. * There will not be a log entry in HttpNetworkTransaction::SelectNextAuthIdentityToTry when we have the wrong authentication scheme (we don't see that entry any more) * We will no longer return ERR_TUNNEL_CONNECTION_FAILED from SocketStream::HandleAuthChallenge when there's an entry in the cache with a non-basic authentication scheme (we won't know it's there). Contributed by rdsmith@chromium.org BUG=33433 TEST=HttpAuthCacheTest.* (as modified in this commit), HttpNetworkTransactionTest.*, SocketStreamTest.*, only on Linux. Review URL: http://codereview.chromium.org/1949004 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@47149 0039d316-1c4b-4281-b951-d872f2087c98 --- net/http/http_auth_cache.cc | 17 ++++-- net/http/http_auth_cache.h | 42 +++++++------ net/http/http_auth_cache_unittest.cc | 112 +++++++++++++++++++++++++---------- net/http/http_network_transaction.cc | 32 +++------- net/socket_stream/socket_stream.cc | 12 ++-- 5 files changed, 131 insertions(+), 84 deletions(-) (limited to 'net') diff --git a/net/http/http_auth_cache.cc b/net/http/http_auth_cache.cc index a7e2309..2be2198 100644 --- a/net/http/http_auth_cache.cc +++ b/net/http/http_auth_cache.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2008 The Chromium Authors. All rights reserved. +// Copyright (c) 2010 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. @@ -60,13 +60,15 @@ struct IsEnclosedBy { namespace net { // Performance: O(n), where n is the number of realm entries. -HttpAuthCache::Entry* HttpAuthCache::LookupByRealm(const GURL& origin, - const std::string& realm) { +HttpAuthCache::Entry* HttpAuthCache::Lookup(const GURL& origin, + const std::string& realm, + const std::string& scheme) { CheckOriginIsValid(origin); // Linear scan through the realm entries. for (EntryList::iterator it = entries_.begin(); it != entries_.end(); ++it) { - if (it->origin() == origin && it->realm() == realm) + if (it->origin() == origin && it->realm() == realm && + it->scheme() == scheme) return &(*it); } return NULL; // No realm entry found. @@ -103,7 +105,8 @@ HttpAuthCache::Entry* HttpAuthCache::Add(const GURL& origin, CheckPathIsValid(path); // Check for existing entry (we will re-use it if present). - HttpAuthCache::Entry* entry = LookupByRealm(origin, handler->realm()); + HttpAuthCache::Entry* entry = Lookup(origin, handler->realm(), + handler->scheme()); if (!entry) { // Failsafe to prevent unbounded memory growth of the cache. @@ -155,10 +158,12 @@ bool HttpAuthCache::Entry::HasEnclosingPath(const std::string& dir) { bool HttpAuthCache::Remove(const GURL& origin, const std::string& realm, + const std::string& scheme, const std::wstring& username, const std::wstring& password) { for (EntryList::iterator it = entries_.begin(); it != entries_.end(); ++it) { - if (it->origin() == origin && it->realm() == realm) { + if (it->origin() == origin && it->realm() == realm && + it->scheme() == scheme) { if (username == it->username() && password == it->password()) { entries_.erase(it); return true; diff --git a/net/http/http_auth_cache.h b/net/http/http_auth_cache.h index 62c09e9..7062c76 100644 --- a/net/http/http_auth_cache.h +++ b/net/http/http_auth_cache.h @@ -1,4 +1,4 @@ -// Copyright (c) 2008 The Chromium Authors. All rights reserved. +// Copyright (c) 2010 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. @@ -16,28 +16,28 @@ namespace net { -// TODO(eroman): Can we change the key from (origin, realm) to -// (origin, realm, auth_scheme)? - // HttpAuthCache stores HTTP authentication identities and challenge info. -// For each realm the cache stores a HttpAuthCache::Entry, which holds: -// - the realm name -// - the origin server {scheme, host, port} +// For each (origin, realm, scheme) triple the cache stores a +// HttpAuthCache::Entry, which holds: +// - the origin server {protocol scheme, host, port} // - the last identity used (username/password) -// - the last auth handler used +// - the last auth handler used (contains realm and authentication scheme) // - the list of paths which used this realm -// Entries can be looked up by either (origin, realm) or (origin, path). +// Entries can be looked up by either (origin, realm, scheme) or (origin, path). class HttpAuthCache { public: class Entry; - // Find the realm entry on server |origin| for realm |realm|. + // Find the realm entry on server |origin| for realm |realm| and + // scheme |scheme|. // |origin| - the {scheme, host, port} of the server. // |realm| - case sensitive realm string. + // |scheme| - case sensitive authentication scheme, should be lower-case. // returns - the matched entry or NULL. - Entry* LookupByRealm(const GURL& origin, const std::string& realm); + Entry* Lookup(const GURL& origin, const std::string& realm, + const std::string& scheme); - // Find the realm entry on server |origin| whose protection space includes + // Find the entry on server |origin| whose protection space includes // |path|. This uses the assumption in RFC 2617 section 2 that deeper // paths lie in the same protection space. // |origin| - the {scheme, host, port} of the server. @@ -46,9 +46,10 @@ class HttpAuthCache { // returns - the matched entry or NULL. Entry* LookupByPath(const GURL& origin, const std::string& path); - // Add a realm entry on server |origin| for realm |handler->realm()|, If an - // entry for this realm already exists, update it rather than replace it -- - // this preserves the realm's paths list. + // Add an entry on server |origin| for realm |handler->realm()| and + // scheme |handler->scheme()|. If an entry for this (realm,scheme) + // already exists, update it rather than replace it -- this preserves the + // paths list. // |origin| - the {scheme, host, port} of the server. // |handler| - handler for the challenge. // |username| - login information for the realm. @@ -62,15 +63,17 @@ class HttpAuthCache { const std::wstring& password, const std::string& path); - // Remove realm entry on server |origin| for realm |realm| if one exists - // AND if the cached identity matches (|username|, |password|). + // Remove entry on server |origin| for realm |realm| and scheme |scheme| + // if one exists AND if the cached identity matches (|username|, |password|). // |origin| - the {scheme, host, port} of the server. // |realm| - case sensitive realm string. + // |scheme| - authentication scheme // |username| - condition to match. // |password| - condition to match. // returns - true if an entry was removed. bool Remove(const GURL& origin, const std::string& realm, + const std::string& scheme, const std::wstring& username, const std::wstring& password); @@ -98,6 +101,11 @@ class HttpAuthCache::Entry { return handler_->realm(); } + // The authentication scheme string of the challenge + const std::string scheme() const { + return handler_->scheme(); + } + // The handler for the challenge. HttpAuthHandler* handler() const { return handler_.get(); diff --git a/net/http/http_auth_cache_unittest.cc b/net/http/http_auth_cache_unittest.cc index c233f01..28cb138 100644 --- a/net/http/http_auth_cache_unittest.cc +++ b/net/http/http_auth_cache_unittest.cc @@ -69,58 +69,89 @@ TEST(HttpAuthCacheTest, Basic) { cache.Add(origin, realm2_handler, L"realm2-user", L"realm2-password", "/foo2/index.html"); - scoped_refptr realm3_handler = + scoped_refptr realm3_basic_handler = new MockAuthHandler("basic", "Realm3", HttpAuth::AUTH_PROXY); - cache.Add(origin, realm3_handler, L"realm3-user", L"realm3-password", ""); + cache.Add(origin, realm3_basic_handler, L"realm3-basic-user", + L"realm3-basic-password", ""); + + scoped_refptr realm3_digest_handler = + new MockAuthHandler("digest", "Realm3", HttpAuth::AUTH_PROXY); + cache.Add(origin, realm3_digest_handler, L"realm3-digest-user", + L"realm3-digest-password", "/baz/index.html"); // There is no Realm4 - entry = cache.LookupByRealm(origin, "Realm4"); + entry = cache.Lookup(origin, "Realm4", "basic"); EXPECT_TRUE(NULL == entry); // While Realm3 does exist, the origin scheme is wrong. - entry = cache.LookupByRealm(GURL("https://www.google.com"), "Realm3"); + entry = cache.Lookup(GURL("https://www.google.com"), "Realm3", + "basic"); EXPECT_TRUE(NULL == entry); - // Valid lookup by realm. - entry = cache.LookupByRealm(GURL("http://www.google.com:80"), "Realm3"); + // Realm, origin scheme ok, authentication scheme wrong + entry = cache.Lookup(GURL("http://www.google.com"), "Realm1", "digest"); + EXPECT_TRUE(NULL == entry); + + // Valid lookup by origin, realm, scheme. + entry = cache.Lookup(GURL("http://www.google.com:80"), "Realm3", "basic"); EXPECT_FALSE(NULL == entry); - EXPECT_TRUE(entry->handler() == realm3_handler.get()); - EXPECT_EQ(L"realm3-user", entry->username()); - EXPECT_EQ(L"realm3-password", entry->password()); + EXPECT_TRUE(entry->handler() == realm3_basic_handler.get()); + EXPECT_EQ(L"realm3-basic-user", entry->username()); + EXPECT_EQ(L"realm3-basic-password", entry->password()); + + // Valid lookup by origin, realm, scheme when there's a duplicate + // origin, realm in the cache + entry = cache.Lookup(GURL("http://www.google.com:80"), "Realm3", "digest"); + EXPECT_FALSE(NULL == entry); + EXPECT_TRUE(entry->handler() == realm3_digest_handler.get()); + EXPECT_EQ(L"realm3-digest-user", entry->username()); + EXPECT_EQ(L"realm3-digest-password", entry->password()); // Valid lookup by realm. - entry = cache.LookupByRealm(origin, "Realm2"); + entry = cache.Lookup(origin, "Realm2", "basic"); EXPECT_FALSE(NULL == entry); EXPECT_TRUE(entry->handler() == realm2_handler.get()); EXPECT_EQ(L"realm2-user", entry->username()); EXPECT_EQ(L"realm2-password", entry->password()); // Check that subpaths are recognized. - HttpAuthCache::Entry* realm2Entry = cache.LookupByRealm(origin, "Realm2"); - EXPECT_FALSE(NULL == realm2Entry); + HttpAuthCache::Entry* realm2_entry = cache.Lookup(origin, "Realm2", "basic"); + EXPECT_FALSE(NULL == realm2_entry); // Positive tests: entry = cache.LookupByPath(origin, "/foo2/index.html"); - EXPECT_TRUE(realm2Entry == entry); + EXPECT_TRUE(realm2_entry == entry); entry = cache.LookupByPath(origin, "/foo2/foobar.html"); - EXPECT_TRUE(realm2Entry == entry); + EXPECT_TRUE(realm2_entry == entry); entry = cache.LookupByPath(origin, "/foo2/bar/index.html"); - EXPECT_TRUE(realm2Entry == entry); + EXPECT_TRUE(realm2_entry == entry); entry = cache.LookupByPath(origin, "/foo2/"); - EXPECT_TRUE(realm2Entry == entry); + EXPECT_TRUE(realm2_entry == entry); + // Negative tests: entry = cache.LookupByPath(origin, "/foo2"); - EXPECT_FALSE(realm2Entry == entry); + EXPECT_FALSE(realm2_entry == entry); entry = cache.LookupByPath(origin, "/foo3/index.html"); - EXPECT_FALSE(realm2Entry == entry); + EXPECT_FALSE(realm2_entry == entry); entry = cache.LookupByPath(origin, ""); - EXPECT_FALSE(realm2Entry == entry); + EXPECT_FALSE(realm2_entry == entry); entry = cache.LookupByPath(origin, "/"); - EXPECT_FALSE(realm2Entry == entry); + EXPECT_FALSE(realm2_entry == entry); + + // Confirm we find the same realm, different auth scheme by path lookup + HttpAuthCache::Entry* realm3_digest_entry = + cache.Lookup(origin, "Realm3", "digest"); + EXPECT_FALSE(NULL == realm3_digest_entry); + entry = cache.LookupByPath(origin, "/baz/index.html"); + EXPECT_TRUE(realm3_digest_entry == entry); + entry = cache.LookupByPath(origin, "/baz/"); + EXPECT_TRUE(realm3_digest_entry == entry); + entry = cache.LookupByPath(origin, "/baz"); + EXPECT_FALSE(realm3_digest_entry == entry); // Lookup using empty path (may be used for proxy). entry = cache.LookupByPath(origin, ""); EXPECT_FALSE(NULL == entry); - EXPECT_TRUE(entry->handler() == realm3_handler.get()); + EXPECT_TRUE(entry->handler() == realm3_basic_handler.get()); EXPECT_EQ("Realm3", entry->realm()); } @@ -170,7 +201,7 @@ TEST(HttpAuthCacheTest, AddToExistingEntry) { cache.Add(origin, handler, L"user2", L"password2", "/z/y/x/"); cache.Add(origin, handler, L"user3", L"password3", "/z/y"); - HttpAuthCache::Entry* entry = cache.LookupByRealm(origin, "MyRealm"); + HttpAuthCache::Entry* entry = cache.Lookup(origin, "MyRealm", "basic"); EXPECT_TRUE(entry == orig_entry); EXPECT_EQ(L"user3", entry->username()); @@ -190,32 +221,51 @@ TEST(HttpAuthCacheTest, Remove) { scoped_refptr realm2_handler = new MockAuthHandler("basic", "Realm2", HttpAuth::AUTH_SERVER); - scoped_refptr realm3_handler = + scoped_refptr realm3_basic_handler = new MockAuthHandler("basic", "Realm3", HttpAuth::AUTH_SERVER); + scoped_refptr realm3_digest_handler = + new MockAuthHandler("digest", "Realm3", HttpAuth::AUTH_SERVER); + HttpAuthCache cache; cache.Add(origin, realm1_handler, L"alice", L"123", "/"); cache.Add(origin, realm2_handler, L"bob", L"princess", "/"); - cache.Add(origin, realm3_handler, L"admin", L"password", "/"); + cache.Add(origin, realm3_basic_handler, L"admin", L"password", "/"); + cache.Add(origin, realm3_digest_handler, L"root", L"wilecoyote", "/"); // Fails, because there is no realm "Realm4". - EXPECT_FALSE(cache.Remove(origin, "Realm4", L"alice", L"123")); + EXPECT_FALSE(cache.Remove(origin, "Realm4", "basic", L"alice", L"123")); // Fails because the origin is wrong. EXPECT_FALSE(cache.Remove( - GURL("http://foobar2.com:100"), "Realm1", L"alice", L"123")); + GURL("http://foobar2.com:100"), "Realm1", "basic", L"alice", L"123")); // Fails because the username is wrong. - EXPECT_FALSE(cache.Remove(origin, "Realm1", L"alice2", L"123")); + EXPECT_FALSE(cache.Remove(origin, "Realm1", "basic", L"alice2", L"123")); // Fails because the password is wrong. - EXPECT_FALSE(cache.Remove(origin, "Realm1", L"alice", L"1234")); + EXPECT_FALSE(cache.Remove(origin, "Realm1", "basic", L"alice", L"1234")); + + // Fails because the authentication type is wrong. + EXPECT_FALSE(cache.Remove(origin, "Realm1", "digest", L"alice", L"123")); // Succeeds. - EXPECT_TRUE(cache.Remove(origin, "Realm1", L"alice", L"123")); + EXPECT_TRUE(cache.Remove(origin, "Realm1", "basic", L"alice", L"123")); // Fails because we just deleted the entry! - EXPECT_FALSE(cache.Remove(origin, "Realm1", L"alice", L"123")); + EXPECT_FALSE(cache.Remove(origin, "Realm1", "basic", L"alice", L"123")); + + // Succeed when there are two authentication types for the same origin,realm. + EXPECT_TRUE(cache.Remove(origin, "Realm3", "digest", L"root", L"wilecoyote")); + + // Succeed as above, but when entries were added in opposite order + cache.Add(origin, realm3_digest_handler, L"root", L"wilecoyote", "/"); + EXPECT_TRUE(cache.Remove(origin, "Realm3", "basic", L"admin", L"password")); + + // Make sure that removing one entry still leaves the other available + // for lookup + HttpAuthCache::Entry* entry = cache.Lookup(origin, "Realm3", "digest"); + EXPECT_FALSE(NULL == entry); } // Test fixture class for eviction tests (contains helpers for bulk @@ -246,7 +296,7 @@ class HttpAuthCacheEvictionTest : public testing::Test { void CheckRealmExistence(int realm_i, bool exists) { const HttpAuthCache::Entry* entry = - cache_.LookupByRealm(origin_, GenerateRealm(realm_i)); + cache_.Lookup(origin_, GenerateRealm(realm_i), "basic"); if (exists) { EXPECT_FALSE(entry == NULL); EXPECT_EQ(GenerateRealm(realm_i), entry->realm()); diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc index 6ac135e..eeb9941 100644 --- a/net/http/http_network_transaction.cc +++ b/net/http/http_network_transaction.cc @@ -1877,6 +1877,7 @@ void HttpNetworkTransaction::InvalidateRejectedAuthFromCache( // since the entry in the cache may be newer than what we used last time. session_->auth_cache()->Remove(auth_origin, auth_handler_[target]->realm(), + auth_handler_[target]->scheme(), auth_identity_[target].username, auth_identity_[target].password); } @@ -1937,31 +1938,16 @@ bool HttpNetworkTransaction::SelectNextAuthIdentityToTry( } // Check the auth cache for a realm entry. - HttpAuthCache::Entry* entry = session_->auth_cache()->LookupByRealm( - auth_origin, auth_handler_[target]->realm()); + HttpAuthCache::Entry* entry = + session_->auth_cache()->Lookup(auth_origin, auth_handler_[target]->realm(), + auth_handler_[target]->scheme()); if (entry) { - // Disallow re-using of identity if the scheme of the originating challenge - // does not match. This protects against the following situation: - // 1. Browser prompts user to sign into DIGEST realm="Foo". - // 2. Since the auth-scheme is not BASIC, the user is reasured that it - // will not be sent over the wire in clear text. So they use their - // most trusted password. - // 3. Next, the browser receives a challenge for BASIC realm="Foo". This - // is the same realm that we have a cached identity for. However if - // we use that identity, it would get sent over the wire in - // clear text (which isn't what the user agreed to when entering it). - if (entry->handler()->scheme() == auth_handler_[target]->scheme()) { - auth_identity_[target].source = HttpAuth::IDENT_SRC_REALM_LOOKUP; - auth_identity_[target].invalid = false; - auth_identity_[target].username = entry->username(); - auth_identity_[target].password = entry->password(); - return true; - } - LOG(WARNING) << "The scheme of realm " << auth_handler_[target]->realm() - << " has changed from " << entry->handler()->scheme() - << " to " << auth_handler_[target]->scheme(); - // Fall through. + auth_identity_[target].source = HttpAuth::IDENT_SRC_REALM_LOOKUP; + auth_identity_[target].invalid = false; + auth_identity_[target].username = entry->username(); + auth_identity_[target].password = entry->password(); + return true; } // Use default credentials (single sign on) if this is the first attempt diff --git a/net/socket_stream/socket_stream.cc b/net/socket_stream/socket_stream.cc index b957d8a..68fbbf3 100644 --- a/net/socket_stream/socket_stream.cc +++ b/net/socket_stream/socket_stream.cc @@ -862,6 +862,7 @@ int SocketStream::HandleAuthChallenge(const HttpResponseHeaders* headers) { if (auth_identity_.source != HttpAuth::IDENT_SRC_PATH_LOOKUP) auth_cache_.Remove(auth_origin, auth_handler_->realm(), + auth_handler_->scheme(), auth_identity_.username, auth_identity_.password); auth_handler_ = NULL; @@ -877,14 +878,11 @@ int SocketStream::HandleAuthChallenge(const HttpResponseHeaders* headers) { return ERR_TUNNEL_CONNECTION_FAILED; } if (auth_handler_->NeedsIdentity()) { - HttpAuthCache::Entry* entry = auth_cache_.LookupByRealm( - auth_origin, auth_handler_->realm()); + // We only support basic authentication scheme now. + // TODO(ukai): Support other authentication scheme. + HttpAuthCache::Entry* entry = + auth_cache_.Lookup(auth_origin, auth_handler_->realm(), "basic"); if (entry) { - if (entry->handler()->scheme() != "basic") { - // We only support basic authentication scheme now. - // TODO(ukai): Support other authentication scheme. - return ERR_TUNNEL_CONNECTION_FAILED; - } auth_identity_.source = HttpAuth::IDENT_SRC_REALM_LOOKUP; auth_identity_.invalid = false; auth_identity_.username = entry->username(); -- cgit v1.1