iwa: On ChromeOS, only verify signatures during installation

This is essentially a revert of crrev.com/4051065. The explicit check
during installation landed in https://crrev.com/c/4173793.

For reference, as documented in the design doc [1], we only want to
check signatures during installation on ChromeOS, and not during every
Chrome session.

[1]: go/isolated-apps-when-to-verify-signatures

Bug: 1366309
Change-Id: I7c55a10cebd08105ea858f9f48d9df828400cf5a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4180724
Commit-Queue: Sonja Laurila <laurila@google.com>
Auto-Submit: Christian Flach <cmfcmf@chromium.org>
Reviewed-by: Sonja Laurila <laurila@google.com>
Cr-Commit-Position: refs/heads/main@{#1097892}

chrome/browser/web_applications/isolated_web_apps/install_isolated_web_app_command_unittest.cc

@@ -196,6 +196,8 @@ class FakeResponseReaderFactory : public IsolatedWebAppResponseReaderFactory {
                             const web_package::SignedWebBundleId& web_bundle_id,
                             bool skip_signature_verification,
                             Callback callback) override {
+    // Signatures _must_ be verified during installation.
+    CHECK(!skip_signature_verification);
     if (bundle_error_) {
       std::move(callback).Run(base::unexpected(std::move(*bundle_error_)));
     } else {

chrome/browser/web_applications/isolated_web_apps/isolated_web_app_reader_registry.cc

@@ -106,14 +106,17 @@ void IsolatedWebAppReaderRegistry::ReadResponse(
   cache_entry_it->second.pending_requests.emplace_back(resource_request,
                                                        std::move(callback));
 
+#if BUILDFLAG(IS_CHROMEOS)
+  // On ChromeOS, signatures are only verified at install-time. The location of
+  // the installed bundles inside of cryptohome is deemed secure enough to not
+  // necessitate re-verification of signatures once per session.
+  bool skip_signature_verification = true;
+#else
   // If we already verified the signatures of this Signed Web Bundle during
   // the current browser session, we trust that the Signed Web Bundle has not
   // been tampered with and don't re-verify signatures.
-  //
-  // TODO(crbug.com/1366309): On ChromeOS, we should only verify signatures at
-  // install-time. Until this is implemented, we will verify signatures on
-  // ChromeOS once per session.
   bool skip_signature_verification = verified_files_.contains(web_bundle_path);
+#endif
 
   reader_factory_->CreateResponseReader(
       web_bundle_path, web_bundle_id, skip_signature_verification,

chrome/browser/web_applications/isolated_web_apps/isolated_web_app_reader_registry_unittest.cc

@@ -373,7 +373,11 @@ TEST_F(IsolatedWebAppReaderRegistryTest, TestSignedWebBundleReaderLifetime) {
     EXPECT_EQ(result->head()->response_code, 200);
   }
 
+#if BUILDFLAG(IS_CHROMEOS)
+  EXPECT_EQ(num_signature_verifications, 0ul);
+#else
   EXPECT_EQ(num_signature_verifications, 1ul);
+#endif
 
   // Verify that the cache cleanup timer has started.
   EXPECT_EQ(task_environment_.GetPendingMainThreadTaskCount(), 1ul)
@@ -394,7 +398,11 @@ TEST_F(IsolatedWebAppReaderRegistryTest, TestSignedWebBundleReaderLifetime) {
     EXPECT_EQ(result->head()->response_code, 200);
   }
 
+#if BUILDFLAG(IS_CHROMEOS)
+  EXPECT_EQ(num_signature_verifications, 0ul);
+#else
   EXPECT_EQ(num_signature_verifications, 1ul);
+#endif
 
   // Verify that the cache cleanup timer is still running.
   EXPECT_EQ(task_environment_.GetPendingMainThreadTaskCount(), 1ul)
@@ -427,9 +435,13 @@ TEST_F(IsolatedWebAppReaderRegistryTest, TestSignedWebBundleReaderLifetime) {
     EXPECT_EQ(result->head()->response_code, 200);
   }
 
+#if BUILDFLAG(IS_CHROMEOS)
+  EXPECT_EQ(num_signature_verifications, 0ul);
+#else
   // Signatures should not have been verified again, since we only verify them
   // once per session per file path.
   EXPECT_EQ(num_signature_verifications, 1ul);
+#endif
 
   // Verify that the cache cleanup timer has started again.
   EXPECT_EQ(task_environment_.GetPendingMainThreadTaskCount(), 1ul)
@@ -550,6 +562,21 @@ TEST_P(IsolatedWebAppReaderRegistrySignatureVerificationErrorTest,
 
   FulfillIntegrityBlock();
 
+#if BUILDFLAG(IS_CHROMEOS)
+  // On ChromeOS, signatures are only verified at installation-time, thus the
+  // `FakeSignatureVerifier` set up above will never be called.
+  FulfillMetadata();
+  FulfillResponse(resource_request);
+
+  ReadResult result = read_response_future.Take();
+  ASSERT_TRUE(result.has_value()) << result.error().message;
+
+  histogram_tester.ExpectBucketCount(
+      "WebApp.Isolated.ReadIntegrityBlockAndMetadataStatus",
+      IsolatedWebAppResponseReaderFactory::ReadIntegrityBlockAndMetadataStatus::
+          kSuccess,
+      1);
+#else
   ReadResult result = read_response_future.Take();
   ASSERT_FALSE(result.has_value());
   EXPECT_EQ(result.error().type,
@@ -563,6 +590,7 @@ TEST_P(IsolatedWebAppReaderRegistrySignatureVerificationErrorTest,
       IsolatedWebAppResponseReaderFactory::ReadIntegrityBlockAndMetadataStatus::
           kSignatureVerificationError,
       1);
+#endif  // BUILDFLAG(IS_CHROMEOS)
 }
 
 INSTANTIATE_TEST_SUITE_P(

docs/webapps/isolated_web_apps.md

@@ -45,9 +45,9 @@ content from an Isolated Web App:
 7. If the Integrity Block is valid, then:
    - On non-ChromeOS: The signatures contained in the Isolated Web App are
      verified using `web_package::SignedWebBundleSignatureVerifier`.
-   - On ChromeOS: The signatures are only verified during installation
-     (TODO(crbug.com/1366309)), because the cryptohome is deemed secure enough
-     to prevent tampering with an already installed Isolated Web App.
+   - On ChromeOS: The signatures are only verified during installation, because
+     the cryptohome is deemed secure enough to prevent tampering with an already
+     installed Isolated Web App.
 7. If the signatures are valid, the metadata of the Signed Web Bundle is read
    and validated using `IsolatedWebAppValidator::ValidateMetadata`. This
    includes a check that validates that URLs contained in the Signed Web Bundle