webauthn: couple caBLE discoveries with an EventStream.

When PaaSK for 2nd-factor is launched, we don't want to ping phones
automatically because we'll be getting linking information from Sync and
that would be surprising (and annoying) to users. So we only want to
kick off a caBLEv2 transaction when the user selects a phone from the
transport-selection dialog.

That begs the question of how to plumb that click into the
cablev2::Discovery. There are calls from the UI to the
FidoRequestHandlerBase (such as |StartAuthenticatorRequest|) but it
seems awkward that the request handler should have to know about a
discovery-specific concept and should have to enumerate its discoveries
to try and find the one to forward it onto.

Rather we would like to be able to configure this on the
FidoDiscoveryFactory like all the other caBLE data. But the
cablev2::Discovery itself hasn't been created at that point, so we can't
get a callback on it. What we need is a “late-binding” callback. Like
Mojo's PendingRemote, but much (much) lighter.

This change adds |EventSource| to do that. It also solves a smaller
problem: the V1 discovery holders a pointer to the V2 discovery so that
it can forward BLE adverts. This is fine because we know that they'll be
destroyed together, but it's slightly bothersome. With an |EventSource|
in hand we can do something less coupled.

BUG=1002262

Change-Id: I93708ab7803b5f073c9bb5db49a51139d6ab61c2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2792062
Commit-Queue: Adam Langley <agl@chromium.org>
Reviewed-by: Martin Kreichgauer <martinkr@google.com>
Cr-Commit-Position: refs/heads/master@{#867900}

content/browser/webauth/authenticator_impl_unittest.cc

@@ -6728,6 +6728,9 @@ class CableV2AuthenticatorImplTest : public AuthenticatorImplTest {
                  p256.get(), EC_KEY_get0_public_key(peer_identity.get()),
                  POINT_CONVERSION_UNCOMPRESSED, peer_identity_x962_,
                  sizeof(peer_identity_x962_), /*ctx=*/nullptr));
+
+    std::tie(ble_advert_callback_, ble_advert_events_) =
+        device::cablev2::Discovery::AdvertEventStream::New();
   }
 
   base::RepeatingCallback<void(device::cablev2::PairingEvent)>
@@ -6806,24 +6809,27 @@ class CableV2AuthenticatorImplTest : public AuthenticatorImplTest {
       base::span<const uint8_t, device::cablev2::kClientNonceSize>
           client_nonce)>
       contact_callback_;
+
+  std::unique_ptr<device::cablev2::Discovery::AdvertEventStream>
+      ble_advert_events_;
+  device::cablev2::Discovery::AdvertEventStream::Callback ble_advert_callback_;
 };
 
 TEST_F(CableV2AuthenticatorImplTest, QRBasedWithNoPairing) {
   auto discovery = std::make_unique<device::cablev2::Discovery>(
-      network_context_.get(), qr_generator_key_,
+      network_context_.get(), qr_generator_key_, std::move(ble_advert_events_),
       /*pairings=*/std::vector<std::unique_ptr<device::cablev2::Pairing>>(),
       /*extension_contents=*/std::vector<device::CableDiscoveryData>(),
       GetPairingCallback());
-  auto* const discovery_ptr = discovery.get();
 
   AuthenticatorEnvironmentImpl::GetInstance()
       ->ReplaceDefaultDiscoveryFactoryForTesting(
           std::make_unique<DiscoveryFactory>(std::move(discovery)));
 
   std::unique_ptr<device::cablev2::authenticator::Transaction> transaction =
       device::cablev2::authenticator::TransactFromQRCode(
-          device::cablev2::authenticator::NewMockPlatform(discovery_ptr,
-                                                          &virtual_device_),
+          device::cablev2::authenticator::NewMockPlatform(
+              std::move(ble_advert_callback_), &virtual_device_),
           network_context_.get(), root_secret_, "Test Authenticator",
           zero_qr_secret_, peer_identity_x962_,
           /*contact_id=*/base::nullopt);
@@ -6835,20 +6841,19 @@ TEST_F(CableV2AuthenticatorImplTest, QRBasedWithNoPairing) {
 TEST_F(CableV2AuthenticatorImplTest, PairingBased) {
   // First do unpaired exchange to get pairing data.
   auto discovery = std::make_unique<device::cablev2::Discovery>(
-      network_context_.get(), qr_generator_key_,
+      network_context_.get(), qr_generator_key_, std::move(ble_advert_events_),
       /*pairings=*/std::vector<std::unique_ptr<device::cablev2::Pairing>>(),
       /*extension_contents=*/std::vector<device::CableDiscoveryData>(),
       GetPairingCallback());
-  auto* discovery_ptr = discovery.get();
 
   AuthenticatorEnvironmentImpl::GetInstance()
       ->ReplaceDefaultDiscoveryFactoryForTesting(
           std::make_unique<DiscoveryFactory>(std::move(discovery)));
 
   std::unique_ptr<device::cablev2::authenticator::Transaction> transaction =
       device::cablev2::authenticator::TransactFromQRCode(
-          device::cablev2::authenticator::NewMockPlatform(discovery_ptr,
-                                                          &virtual_device_),
+          device::cablev2::authenticator::NewMockPlatform(
+              std::move(ble_advert_callback_), &virtual_device_),
           network_context_.get(), root_secret_, "Test Authenticator",
           zero_qr_secret_, peer_identity_x962_,
           /*contact_id=*/std::vector<uint8_t>({1, 2, 3}));
@@ -6857,11 +6862,14 @@ TEST_F(CableV2AuthenticatorImplTest, PairingBased) {
   EXPECT_EQ(pairings_.size(), 1u);
 
   // Now do a pairing-based exchange.
+  std::tie(ble_advert_callback_, ble_advert_events_) =
+      device::cablev2::Discovery::EventStream<
+          base::span<const uint8_t, device::cablev2::kAdvertSize>>::New();
   discovery = std::make_unique<device::cablev2::Discovery>(
-      network_context_.get(), qr_generator_key_, std::move(pairings_),
+      network_context_.get(), qr_generator_key_, std::move(ble_advert_events_),
+      std::move(pairings_),
       /*extension_contents=*/std::vector<device::CableDiscoveryData>(),
       GetPairingCallback());
-  discovery_ptr = discovery.get();
 
   const std::array<uint8_t, device::cablev2::kRoutingIdSize> routing_id = {0};
   bool contact_callback_was_called = false;
@@ -6871,16 +6879,15 @@ TEST_F(CableV2AuthenticatorImplTest, PairingBased) {
   // This simulates the tunnel server sending a cloud message to a phone. Given
   // the information from the connection, a transaction can be created.
   contact_callback_ = base::BindLambdaForTesting(
-      [this, &transaction, discovery_ptr, routing_id,
-       &contact_callback_was_called](
+      [this, &transaction, routing_id, &contact_callback_was_called](
           base::span<const uint8_t, device::cablev2::kTunnelIdSize> tunnel_id,
           base::span<const uint8_t> pairing_id,
           base::span<const uint8_t, device::cablev2::kClientNonceSize>
               client_nonce) -> void {
         contact_callback_was_called = true;
         transaction = device::cablev2::authenticator::TransactFromFCM(
-            device::cablev2::authenticator::NewMockPlatform(discovery_ptr,
-                                                            &virtual_device_),
+            device::cablev2::authenticator::NewMockPlatform(
+                std::move(ble_advert_callback_), &virtual_device_),
             network_context_.get(), root_secret_, routing_id, tunnel_id,
             pairing_id, client_nonce);
       });
@@ -6913,7 +6920,8 @@ TEST_F(CableV2AuthenticatorImplTest, ContactIDDisabled) {
   // rejected.
   auto network_context = device::cablev2::NewMockTunnelServer(base::nullopt);
   auto discovery = std::make_unique<device::cablev2::Discovery>(
-      network_context.get(), qr_generator_key_, std::move(pairings),
+      network_context.get(), qr_generator_key_, std::move(ble_advert_events_),
+      std::move(pairings),
       /*extension_contents=*/std::vector<device::CableDiscoveryData>(),
       GetPairingCallback());
 

device/fido/cable/fido_cable_discovery.cc

@@ -147,12 +147,10 @@ FidoCableDiscovery::ObservedDeviceData::~ObservedDeviceData() = default;
 // FidoCableDiscovery ---------------------------------------------------------
 
 FidoCableDiscovery::FidoCableDiscovery(
-    std::vector<CableDiscoveryData> discovery_data,
-    FidoDeviceDiscovery::BLEObserver* ble_observer)
+    std::vector<CableDiscoveryData> discovery_data)
     : FidoDeviceDiscovery(
           FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy),
-      discovery_data_(std::move(discovery_data)),
-      ble_observer_(ble_observer) {
+      discovery_data_(std::move(discovery_data)) {
 // Windows currently does not support multiple EIDs, thus we ignore any extra
 // discovery data.
 // TODO(https://crbug.com/837088): Add support for multiple EIDs on Windows.
@@ -183,6 +181,16 @@ FidoCableDiscovery::~FidoCableDiscovery() {
     adapter_->RemoveObserver(this);
 }
 
+std::unique_ptr<FidoDeviceDiscovery::EventStream<base::span<const uint8_t, 20>>>
+FidoCableDiscovery::GetV2AdvertStream() {
+  DCHECK(!advert_callback_);
+
+  std::unique_ptr<EventStream<base::span<const uint8_t, 20>>> ret;
+  std::tie(advert_callback_, ret) =
+      EventStream<base::span<const uint8_t, 20>>::New();
+  return ret;
+}
+
 std::unique_ptr<FidoCableHandshakeHandler>
 FidoCableDiscovery::CreateV1HandshakeHandler(
     FidoCableDevice* device,
@@ -619,7 +627,7 @@ FidoCableDiscovery::GetCableDiscoveryData(const BluetoothDevice* device) {
   // Try all combinations of 16- and 4-byte UUIDs to form 20-byte advert
   // payloads. (We don't know if something in the BLE stack might add other
   // short UUIDs to a BLE advert message).
-  if (ble_observer_) {
+  if (advert_callback_) {
     std::array<uint8_t, 16 + 4> v2_advert;
     for (const auto& uuid128 : uuid128s) {
       static_assert(EXTENT(uuid128) == 16, "");
@@ -628,7 +636,7 @@ FidoCableDiscovery::GetCableDiscoveryData(const BluetoothDevice* device) {
       for (const auto& uuid32 : uuid32s) {
         static_assert(EXTENT(uuid32) >= 4, "");
         memcpy(v2_advert.data() + 16, uuid32.data(), 4);
-        ble_observer_->OnBLEAdvertSeen(v2_advert);
+        advert_callback_.Run(v2_advert);
       }
     }
   }

device/fido/cable/fido_cable_discovery.h

@@ -35,13 +35,18 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoCableDiscovery
       public BluetoothAdapter::Observer,
       public FidoCableDevice::Observer {
  public:
-  FidoCableDiscovery(std::vector<CableDiscoveryData> discovery_data,
-                     FidoDeviceDiscovery::BLEObserver* ble_observer);
+  explicit FidoCableDiscovery(std::vector<CableDiscoveryData> discovery_data);
   ~FidoCableDiscovery() override;
 
   // FidoDeviceDiscovery:
   bool MaybeStop() override;
 
+  // GetV2AdvertStream returns a stream of caBLEv2 BLE adverts. Only a single
+  // stream is supported.
+  std::unique_ptr<FidoDeviceDiscovery::EventStream<
+      base::span<const uint8_t, cablev2::kAdvertSize>>>
+  GetV2AdvertStream();
+
   const std::map<CableEidArray, scoped_refptr<BluetoothAdvertisement>>&
   AdvertisementsForTesting() const {
     return advertisements_;
@@ -140,7 +145,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoCableDiscovery
   std::unique_ptr<BluetoothDiscoverySession> discovery_session_;
 
   std::vector<CableDiscoveryData> discovery_data_;
-  FidoDeviceDiscovery::BLEObserver* const ble_observer_;
+  base::RepeatingCallback<void(base::span<const uint8_t, cablev2::kAdvertSize>)>
+      advert_callback_;
 
   // active_authenticator_eids_ contains authenticator EIDs for which a
   // handshake is currently running. Further advertisements for the same EIDs

device/fido/cable/fido_cable_discovery_unittest.cc

@@ -325,8 +325,7 @@ class FakeFidoCableDiscovery : public FidoCableDiscovery {
  public:
   explicit FakeFidoCableDiscovery(
       std::vector<CableDiscoveryData> discovery_data)
-      : FidoCableDiscovery(std::move(discovery_data),
-                           /*ble_observer=*/nullptr) {}
+      : FidoCableDiscovery(std::move(discovery_data)) {}
   ~FakeFidoCableDiscovery() override = default;
 
  private:

device/fido/cable/v2_discovery.cc

@@ -46,6 +46,7 @@ void RecordEvent(CableV2DiscoveryEvent event) {
 Discovery::Discovery(
     network::mojom::NetworkContext* network_context,
     base::Optional<base::span<const uint8_t, kQRKeySize>> qr_generator_key,
+    std::unique_ptr<AdvertEventStream> advert_stream,
     std::vector<std::unique_ptr<Pairing>> pairings,
     const std::vector<CableDiscoveryData>& extension_contents,
     base::Optional<base::RepeatingCallback<void(PairingEvent)>>
@@ -55,9 +56,12 @@ Discovery::Discovery(
       network_context_(network_context),
       qr_keys_(KeysFromQRGeneratorKey(qr_generator_key)),
       extension_keys_(KeysFromExtension(extension_contents)),
+      advert_stream_(std::move(advert_stream)),
       pairings_(std::move(pairings)),
       pairing_callback_(std::move(pairing_callback)) {
   static_assert(EXTENT(*qr_generator_key) == kQRSecretSize + kQRSeedSize, "");
+  advert_stream_->Connect(
+      base::BindRepeating(&Discovery::OnBLEAdvertSeen, base::Unretained(this)));
 }
 
 Discovery::~Discovery() = default;
@@ -96,23 +100,25 @@ void Discovery::StartInternal() {
   }
 }
 
-void Discovery::OnBLEAdvertSeen(
-    const std::array<uint8_t, kAdvertSize>& advert) {
+void Discovery::OnBLEAdvertSeen(base::span<const uint8_t, kAdvertSize> advert) {
+  const std::array<uint8_t, kAdvertSize> advert_array =
+      fido_parsing_utils::Materialize<kAdvertSize>(advert);
+
   if (!started_) {
-    pending_adverts_.push_back(advert);
+    pending_adverts_.push_back(advert_array);
     return;
   }
 
-  if (base::Contains(observed_adverts_, advert)) {
+  if (base::Contains(observed_adverts_, advert_array)) {
     return;
   }
-  observed_adverts_.insert(advert);
+  observed_adverts_.insert(advert_array);
 
   // Check whether the EID satisfies any pending tunnels.
   for (std::vector<std::unique_ptr<FidoTunnelDevice>>::iterator i =
            tunnels_pending_advert_.begin();
        i != tunnels_pending_advert_.end(); i++) {
-    if (!(*i)->MatchAdvert(advert)) {
+    if (!(*i)->MatchAdvert(advert_array)) {
       continue;
     }
 
@@ -128,7 +134,7 @@ void Discovery::OnBLEAdvertSeen(
   if (qr_keys_) {
     // Check whether the EID matches a QR code.
     base::Optional<CableEidArray> plaintext =
-        eid::Decrypt(advert, qr_keys_->eid_key);
+        eid::Decrypt(advert_array, qr_keys_->eid_key);
     if (plaintext) {
       FIDO_LOG(DEBUG) << "  (" << base::HexEncode(advert)
                       << " matches QR code)";
@@ -144,7 +150,7 @@ void Discovery::OnBLEAdvertSeen(
   // Check whether the EID matches the extension.
   if (extension_keys_) {
     base::Optional<CableEidArray> plaintext =
-        eid::Decrypt(advert, extension_keys_->eid_key);
+        eid::Decrypt(advert_array, extension_keys_->eid_key);
     if (plaintext) {
       FIDO_LOG(DEBUG) << "  (" << base::HexEncode(advert)
                       << " matches extension)";

device/fido/cable/v2_discovery.h

@@ -29,13 +29,14 @@ class FidoTunnelDevice;
 // Discovery creates caBLEv2 devices, either based on |pairings|, or when a BLE
 // advert is seen that matches |qr_generator_key|. It does not actively scan for
 // BLE adverts itself. Rather it depends on |OnBLEAdvertSeen| getting called.
-class COMPONENT_EXPORT(DEVICE_FIDO) Discovery
-    : public FidoDeviceDiscovery,
-      public FidoDeviceDiscovery::BLEObserver {
+class COMPONENT_EXPORT(DEVICE_FIDO) Discovery : public FidoDeviceDiscovery {
  public:
+  using AdvertEventStream = EventStream<base::span<const uint8_t, kAdvertSize>>;
+
   Discovery(
       network::mojom::NetworkContext* network_context,
       base::Optional<base::span<const uint8_t, kQRKeySize>> qr_generator_key,
+      std::unique_ptr<AdvertEventStream> advert_stream,
       std::vector<std::unique_ptr<Pairing>> pairings,
       const std::vector<CableDiscoveryData>& extension_contents,
       // pairing_callback will be called when a QR-initiated connection
@@ -50,9 +51,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) Discovery
   // FidoDeviceDiscovery:
   void StartInternal() override;
 
-  // BLEObserver:
-  void OnBLEAdvertSeen(const std::array<uint8_t, kAdvertSize>& advert) override;
-
  private:
   // UnpairedKeys are keys that are conveyed by QR code or that come from the
   // server, i.e. keys that enable interactions with unpaired phones.
@@ -62,6 +60,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) Discovery
     std::array<uint8_t, kEIDKeySize> eid_key;
   };
 
+  void OnBLEAdvertSeen(base::span<const uint8_t, kAdvertSize> advert);
   void AddPairing(std::unique_ptr<Pairing> pairing);
   void PairingIsInvalid(
       std::array<uint8_t, kP256X962Length> peer_public_key_x962);
@@ -73,6 +72,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) Discovery
   network::mojom::NetworkContext* const network_context_;
   const base::Optional<UnpairedKeys> qr_keys_;
   const base::Optional<UnpairedKeys> extension_keys_;
+  std::unique_ptr<AdvertEventStream> advert_stream_;
   std::vector<std::unique_ptr<Pairing>> pairings_;
   const base::Optional<base::RepeatingCallback<void(PairingEvent)>>
       pairing_callback_;

device/fido/cable/v2_test_util.cc

@@ -8,7 +8,7 @@
 #include <vector>
 
 #include "base/base64url.h"
-#include "base/bind.h"
+#include "base/callback.h"
 #include "base/check.h"
 #include "base/memory/weak_ptr.h"
 #include "base/strings/string_number_conversions.h"
@@ -348,8 +348,10 @@ class DummyBLEAdvert
 // messages to the given |VirtualCtap2Device|.
 class TestPlatform : public authenticator::Platform {
  public:
-  TestPlatform(Discovery* discovery, device::VirtualCtap2Device* ctap2_device)
-      : discovery_(discovery), ctap2_device_(ctap2_device) {}
+  TestPlatform(Discovery::AdvertEventStream::Callback ble_advert_callback,
+               device::VirtualCtap2Device* ctap2_device)
+      : ble_advert_callback_(ble_advert_callback),
+        ctap2_device_(ctap2_device) {}
 
   void MakeCredential(std::unique_ptr<MakeCredentialParams> params) override {
     std::vector<device::PublicKeyCredentialParams::CredentialInfo> cred_infos;
@@ -393,15 +395,16 @@ class TestPlatform : public authenticator::Platform {
     base::SequencedTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
         base::BindOnce(
-            [](Discovery* discovery, std::array<uint8_t, kAdvertSize> payload) {
-              discovery->OnBLEAdvertSeen(payload);
-            },
-            base::Unretained(discovery_),
+            &TestPlatform::DoSendBLEAdvert, weak_factory_.GetWeakPtr(),
             device::fido_parsing_utils::Materialize<EXTENT(payload)>(payload)));
     return std::make_unique<DummyBLEAdvert>();
   }
 
  private:
+  void DoSendBLEAdvert(base::span<const uint8_t, kAdvertSize> advert) {
+    ble_advert_callback_.Run(advert);
+  }
+
   std::vector<uint8_t> ToCTAP2Command(
       const std::pair<device::CtapRequestCommand, base::Optional<cbor::Value>>&
           parts) {
@@ -451,7 +454,7 @@ class TestPlatform : public authenticator::Platform {
         *attestation_obj);
   }
 
-  Discovery* const discovery_;
+  Discovery::AdvertEventStream::Callback ble_advert_callback_;
   device::VirtualCtap2Device* const ctap2_device_;
   base::WeakPtrFactory<TestPlatform> weak_factory_{this};
 };
@@ -466,9 +469,9 @@ std::unique_ptr<network::mojom::NetworkContext> NewMockTunnelServer(
 namespace authenticator {
 
 std::unique_ptr<authenticator::Platform> NewMockPlatform(
-    Discovery* discovery,
+    Discovery::AdvertEventStream::Callback ble_advert_callback,
     device::VirtualCtap2Device* ctap2_device) {
-  return std::make_unique<TestPlatform>(discovery, ctap2_device);
+  return std::make_unique<TestPlatform>(ble_advert_callback, ctap2_device);
 }
 
 }  // namespace authenticator