Multi versioning in SignedExchangeHandler

SignedExchangeHandler parses the content-type header to determine the
version of signed exchange (currently it accepts only version b0), and
passes the version enum to other classes that need versioned code.

Bug: 803774
Change-Id: I90fe2c94d4a5dec61000650338ff4cd0461d2c7d
Reviewed-on: https://chromium-review.googlesource.com/1056940Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558604}

content/browser/web_package/signed_exchange_cert_fetcher.cc

@@ -70,15 +70,16 @@ SignedExchangeCertFetcher::CreateAndStart(
     const GURL& cert_url,
     url::Origin request_initiator,
     bool force_fetch,
+    SignedExchangeVersion version,
     CertificateCallback callback,
     SignedExchangeDevToolsProxy* devtools_proxy) {
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("loading"),
                "SignedExchangeCertFetcher::CreateAndStart");
   std::unique_ptr<SignedExchangeCertFetcher> cert_fetcher(
-      new SignedExchangeCertFetcher(std::move(shared_url_loader_factory),
-                                    std::move(throttles), cert_url,
-                                    std::move(request_initiator), force_fetch,
-                                    std::move(callback), devtools_proxy));
+      new SignedExchangeCertFetcher(
+          std::move(shared_url_loader_factory), std::move(throttles), cert_url,
+          std::move(request_initiator), force_fetch, version,
+          std::move(callback), devtools_proxy));
   cert_fetcher->Start();
   return cert_fetcher;
 }
@@ -89,11 +90,13 @@ SignedExchangeCertFetcher::SignedExchangeCertFetcher(
     const GURL& cert_url,
     url::Origin request_initiator,
     bool force_fetch,
+    SignedExchangeVersion version,
     CertificateCallback callback,
     SignedExchangeDevToolsProxy* devtools_proxy)
     : shared_url_loader_factory_(std::move(shared_url_loader_factory)),
       throttles_(std::move(throttles)),
       resource_request_(std::make_unique<network::ResourceRequest>()),
+      version_(version),
       callback_(std::move(callback)),
       devtools_proxy_(devtools_proxy) {
   // TODO(https://crbug.com/803774): Revisit more ResourceRequest flags.
@@ -179,12 +182,10 @@ void SignedExchangeCertFetcher::OnDataComplete() {
   body_.reset();
   handle_watcher_ = nullptr;
 
-  // TODO(https://crbug.com/803774): Take SignedExchangeVersion as a
-  // parameter of CreateAndStart() and use it here.
   std::unique_ptr<SignedExchangeCertificateChain> cert_chain =
       SignedExchangeCertificateChain::Parse(
-          SignedExchangeVersion::kB0,
-          base::as_bytes(base::make_span(body_string_)), devtools_proxy_);
+          version_, base::as_bytes(base::make_span(body_string_)),
+          devtools_proxy_);
   body_string_.clear();
   if (!cert_chain) {
     signed_exchange_utils::ReportErrorAndEndTraceEvent(

content/browser/web_package/signed_exchange_cert_fetcher.h

@@ -52,6 +52,7 @@ class CONTENT_EXPORT SignedExchangeCertFetcher
       const GURL& cert_url,
       url::Origin request_initiator,
       bool force_fetch,
+      SignedExchangeVersion version,
       CertificateCallback callback,
       SignedExchangeDevToolsProxy* devtools_proxy);
 
@@ -73,6 +74,7 @@ class CONTENT_EXPORT SignedExchangeCertFetcher
       const GURL& cert_url,
       url::Origin request_initiator,
       bool force_fetch,
+      SignedExchangeVersion version,
       CertificateCallback callback,
       SignedExchangeDevToolsProxy* devtools_proxy);
   void Start();
@@ -99,6 +101,7 @@ class CONTENT_EXPORT SignedExchangeCertFetcher
   scoped_refptr<network::SharedURLLoaderFactory> shared_url_loader_factory_;
   std::vector<std::unique_ptr<URLLoaderThrottle>> throttles_;
   std::unique_ptr<network::ResourceRequest> resource_request_;
+  const SignedExchangeVersion version_;
   CertificateCallback callback_;
 
   std::unique_ptr<ThrottlingURLLoader> url_loader_;

content/browser/web_package/signed_exchange_cert_fetcher_factory.cc

@@ -25,6 +25,7 @@ class SignedExchangeCertFetcherFactoryImpl
   std::unique_ptr<SignedExchangeCertFetcher> CreateFetcherAndStart(
       const GURL& cert_url,
       bool force_fetch,
+      SignedExchangeVersion version,
       SignedExchangeCertFetcher::CertificateCallback callback,
       SignedExchangeDevToolsProxy* devtools_proxy) override;
 
@@ -38,6 +39,7 @@ std::unique_ptr<SignedExchangeCertFetcher>
 SignedExchangeCertFetcherFactoryImpl::CreateFetcherAndStart(
     const GURL& cert_url,
     bool force_fetch,
+    SignedExchangeVersion version,
     SignedExchangeCertFetcher::CertificateCallback callback,
     SignedExchangeDevToolsProxy* devtools_proxy) {
   DCHECK(url_loader_factory_);
@@ -46,7 +48,7 @@ SignedExchangeCertFetcherFactoryImpl::CreateFetcherAndStart(
       std::move(url_loader_throttles_getter_).Run();
   return SignedExchangeCertFetcher::CreateAndStart(
       std::move(url_loader_factory_), std::move(throttles), cert_url,
-      std::move(request_initiator_), force_fetch, std::move(callback),
+      std::move(request_initiator_), force_fetch, version, std::move(callback),
       devtools_proxy);
 }
 

content/browser/web_package/signed_exchange_cert_fetcher_factory.h

@@ -32,6 +32,7 @@ class CONTENT_EXPORT SignedExchangeCertFetcherFactory {
   virtual std::unique_ptr<SignedExchangeCertFetcher> CreateFetcherAndStart(
       const GURL& cert_url,
       bool force_fetch,
+      SignedExchangeVersion version,
       SignedExchangeCertFetcher::CertificateCallback callback,
       SignedExchangeDevToolsProxy* devtools_proxy) = 0;
 

content/browser/web_package/signed_exchange_cert_fetcher_unittest.cc

@@ -210,7 +210,8 @@ class SignedExchangeCertFetcherTest : public testing::Test {
         base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
             &mock_loader_factory_),
         std::move(throttles_), url_, request_initiator_, force_fetch,
-        std::move(callback), nullptr /* devtools_proxy */);
+        SignedExchangeVersion::kB0, std::move(callback),
+        nullptr /* devtools_proxy */);
   }
 
   void CallOnReceiveResponse() {

content/browser/web_package/signed_exchange_handler.cc

@@ -10,7 +10,6 @@
 #include "content/browser/loader/merkle_integrity_source_stream.h"
 #include "content/browser/web_package/signed_exchange_cert_fetcher_factory.h"
 #include "content/browser/web_package/signed_exchange_certificate_chain.h"
-#include "content/browser/web_package/signed_exchange_consts.h"
 #include "content/browser/web_package/signed_exchange_devtools_proxy.h"
 #include "content/browser/web_package/signed_exchange_header.h"
 #include "content/browser/web_package/signed_exchange_signature_verifier.h"
@@ -89,10 +88,10 @@ SignedExchangeHandler::SignedExchangeHandler(
   TRACE_EVENT_BEGIN0(TRACE_DISABLED_BY_DEFAULT("loading"),
                      "SignedExchangeHandler::SignedExchangeHandler");
 
-  base::Optional<std::string> content_type_version_param;
-  if (!SignedExchangeHeaderParser::GetVersionParamFromContentType(
-          content_type, &content_type_version_param) ||
-      !content_type_version_param || *content_type_version_param != "b0") {
+  // Currently, only 'v=b0' is supported.
+  if (!SignedExchangeHeaderParser::GetVersionParamFromContentType(content_type,
+                                                                  &version_) ||
+      version_ != SignedExchangeVersion::kB0) {
     base::SequencedTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::BindOnce(&SignedExchangeHandler::RunErrorCallback,
                                   weak_factory_.GetWeakPtr(), net::ERR_FAILED));
@@ -237,11 +236,12 @@ bool SignedExchangeHandler::ParseHeadersAndFetchCertificate() {
   // TODO(https://crbug.com/819467): When we will support ed25519Key, |cert_url|
   // may be empty.
   DCHECK(cert_url.is_valid());
+  DCHECK(version_.has_value());
 
   DCHECK(cert_fetcher_factory_);
   cert_fetcher_ = std::move(cert_fetcher_factory_)
                       ->CreateFetcherAndStart(
-                          cert_url, false,
+                          cert_url, false, *version_,
                           base::BindOnce(&SignedExchangeHandler::OnCertReceived,
                                          base::Unretained(this)),
                           devtools_proxy_.get());

content/browser/web_package/signed_exchange_handler.h

@@ -10,6 +10,7 @@
 #include "base/callback.h"
 #include "base/optional.h"
 #include "base/time/time.h"
+#include "content/browser/web_package/signed_exchange_consts.h"
 #include "content/browser/web_package/signed_exchange_header.h"
 #include "content/common/content_export.h"
 #include "mojo/public/cpp/system/data_pipe.h"
@@ -96,6 +97,7 @@ class CONTENT_EXPORT SignedExchangeHandler {
   void OnCertVerifyComplete(int result);
 
   ExchangeHeadersCallback headers_callback_;
+  base::Optional<SignedExchangeVersion> version_;
   std::unique_ptr<net::SourceStream> source_;
 
   State state_ = State::kReadingHeadersLength;

content/browser/web_package/signed_exchange_handler_unittest.cc

@@ -60,13 +60,13 @@ class MockSignedExchangeCertFetcherFactory
   std::unique_ptr<SignedExchangeCertFetcher> CreateFetcherAndStart(
       const GURL& cert_url,
       bool force_fetch,
+      SignedExchangeVersion version,
       SignedExchangeCertFetcher::CertificateCallback callback,
       SignedExchangeDevToolsProxy* devtools_proxy) override {
     EXPECT_EQ(cert_url, expected_cert_url_);
 
     auto cert_chain = SignedExchangeCertificateChain::Parse(
-        SignedExchangeVersion::kB0, base::as_bytes(base::make_span(cert_str_)),
-        devtools_proxy);
+        version, base::as_bytes(base::make_span(cert_str_)), devtools_proxy);
     EXPECT_TRUE(cert_chain);
 
     base::SequencedTaskRunnerHandle::Get()->PostTask(

content/browser/web_package/signed_exchange_header_parser.cc

@@ -286,7 +286,7 @@ SignedExchangeHeaderParser::ParseSignature(
 // static
 bool SignedExchangeHeaderParser::GetVersionParamFromContentType(
     base::StringPiece content_type,
-    base::Optional<std::string>* version_param) {
+    base::Optional<SignedExchangeVersion>* version_param) {
   DCHECK(version_param);
   StructuredHeaderParser parser(content_type);
   ParameterisedLabel parameterised_label;
@@ -297,7 +297,12 @@ bool SignedExchangeHeaderParser::GetVersionParamFromContentType(
   if (it == parameterised_label.params.end()) {
     *version_param = base::nullopt;
   } else {
-    *version_param = it->second;
+    if (it->second == "b0")
+      *version_param = SignedExchangeVersion::kB0;
+    else if (it->second == "b1")
+      *version_param = SignedExchangeVersion::kB1;
+    else
+      return false;
   }
   return true;
 }

content/browser/web_package/signed_exchange_header_parser.h

@@ -13,6 +13,7 @@
 #include "base/macros.h"
 #include "base/optional.h"
 #include "base/strings/string_piece.h"
+#include "content/browser/web_package/signed_exchange_consts.h"
 #include "content/common/content_export.h"
 #include "net/base/hash_value.h"
 #include "url/gurl.h"
@@ -49,11 +50,11 @@ class CONTENT_EXPORT SignedExchangeHeaderParser {
       SignedExchangeDevToolsProxy* devtools_proxy);
 
   // Parses |content_type| to get the value of "v=" parameter of the signed
-  // exchange. Example: "b0" for "application/signed-exchange;v=b0". Returns
-  // false if failed to parse.
+  // exchange, and converts to SignedExchangeVersion. Returns false if failed to
+  // parse.
   static bool GetVersionParamFromContentType(
       base::StringPiece content_type,
-      base::Optional<std::string>* version_param);
+      base::Optional<SignedExchangeVersion>* version_param);
 };
 
 }  // namespace content

content/browser/web_package/signed_exchange_header_parser_unittest.cc

@@ -231,7 +231,7 @@ TEST_F(SignedExchangeHeaderParserTest, OpenQuoteAtEnd) {
 
 TEST_F(SignedExchangeHeaderParserTest, VersionParam_None) {
   const char content_type[] = "application/signed-exchange";
-  base::Optional<std::string> version;
+  base::Optional<SignedExchangeVersion> version;
   EXPECT_TRUE(SignedExchangeHeaderParser::GetVersionParamFromContentType(
       content_type, &version));
   EXPECT_FALSE(version);
@@ -239,43 +239,43 @@ TEST_F(SignedExchangeHeaderParserTest, VersionParam_None) {
 
 TEST_F(SignedExchangeHeaderParserTest, VersionParam_NoneWithSemicolon) {
   const char content_type[] = "application/signed-exchange;";
-  base::Optional<std::string> version;
+  base::Optional<SignedExchangeVersion> version;
   EXPECT_FALSE(SignedExchangeHeaderParser::GetVersionParamFromContentType(
       content_type, &version));
 }
 
 TEST_F(SignedExchangeHeaderParserTest, VersionParam_EmptyString) {
   const char content_type[] = "application/signed-exchange;v=";
-  base::Optional<std::string> version;
+  base::Optional<SignedExchangeVersion> version;
   EXPECT_FALSE(SignedExchangeHeaderParser::GetVersionParamFromContentType(
       content_type, &version));
 }
 
 TEST_F(SignedExchangeHeaderParserTest, VersionParam_Simple) {
   const char content_type[] = "application/signed-exchange;v=b0";
-  base::Optional<std::string> version;
+  base::Optional<SignedExchangeVersion> version;
   EXPECT_TRUE(SignedExchangeHeaderParser::GetVersionParamFromContentType(
       content_type, &version));
   ASSERT_TRUE(version);
-  EXPECT_EQ(*version, "b0");
+  EXPECT_EQ(*version, SignedExchangeVersion::kB0);
 }
 
 TEST_F(SignedExchangeHeaderParserTest, VersionParam_SimpleWithSpace) {
-  const char content_type[] = "application/signed-exchange; v=b0";
-  base::Optional<std::string> version;
+  const char content_type[] = "application/signed-exchange; v=b1";
+  base::Optional<SignedExchangeVersion> version;
   EXPECT_TRUE(SignedExchangeHeaderParser::GetVersionParamFromContentType(
       content_type, &version));
   ASSERT_TRUE(version);
-  EXPECT_EQ(*version, "b0");
+  EXPECT_EQ(*version, SignedExchangeVersion::kB1);
 }
 
 TEST_F(SignedExchangeHeaderParserTest, VersionParam_SimpleWithDoublequotes) {
   const char content_type[] = "application/signed-exchange;v=\"b0\"";
-  base::Optional<std::string> version;
+  base::Optional<SignedExchangeVersion> version;
   EXPECT_TRUE(SignedExchangeHeaderParser::GetVersionParamFromContentType(
       content_type, &version));
   ASSERT_TRUE(version);
-  EXPECT_EQ(*version, "b0");
+  EXPECT_EQ(*version, SignedExchangeVersion::kB0);
 }
 
 }  // namespace content