Skip to content

Commit

Permalink
Try multiple transports in SetUpCodePairer. (project-chip#16802)
Browse files Browse the repository at this point in the history 
* Try multiple transports in SetUpCodePairer.

When a device gets removed from a fabric and then factory reset, we
can get into a situation where it's only listening over BLE but there
are stale DNS-SD records for it.  In that case we should try BLE once
we discover that doing PASE over IP does not work.

* Address review comments

* Address review comments
  • Loading branch information
@bzbarsky-apple @rochaferraz
bzbarsky-apple authored and rochaferraz committed Mar 31, 2022
1 parent 023caff commit 86e7ab5
Show file tree
Hide file tree
Showing 4 changed files with 208 additions and 21 deletions.
28 changes: 24 additions & 4 deletions src/controller/CHIPDeviceController.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,7 @@ CHIP_ERROR DeviceCommissioner::Shutdown()
if (device != nullptr && device->IsSessionSetupInProgress())
{
ChipLogDetail(Controller, "Setup in progress, stopping setup before shutting down");
OnSessionEstablishmentError(CHIP_ERROR_CONNECTION_ABORTED);
PairingFailed(CHIP_ERROR_CONNECTION_ABORTED);
}
// TODO: If we have a commissioning step in progress, is there a way to cancel that callback?

Expand Down Expand Up @@ -812,7 +812,27 @@ void DeviceCommissioner::RendezvousCleanup(CHIP_ERROR status)

void DeviceCommissioner::OnSessionEstablishmentError(CHIP_ERROR err)
{
// PASE session establishment failure.
// Need to null out mDeviceInPASEEstablishment before maybe trying to
// establish PASE again, so EstablishPASEConnection won't error out.
auto * oldDeviceInPASEEstablishment = mDeviceInPASEEstablishment;
mDeviceInPASEEstablishment = nullptr;

if (mSetUpCodePairer.TryNextRendezvousParameters())
{
ChipLogProgress(Controller, "Ignoring PASE error; will try commissioning over a different transport");
// We don't need oldDeviceInPASEEstablishment anymore.
ReleaseCommissioneeDevice(oldDeviceInPASEEstablishment);
return;
}

// Reset mDeviceInPASEEstablishment because PairingFailed checks for that to
// send the right notifications.
mDeviceInPASEEstablishment = oldDeviceInPASEEstablishment;
PairingFailed(err);
}

void DeviceCommissioner::PairingFailed(CHIP_ERROR err)
{
mSystemState->SystemLayer()->CancelTimer(OnSessionEstablishmentTimeoutCallback, this);

if (mPairingDelegate != nullptr)
Expand All @@ -831,7 +851,7 @@ void DeviceCommissioner::OnSessionEstablished()
// We are in the callback for this pairing. Reset so we can pair another device.
mDeviceInPASEEstablishment = nullptr;

VerifyOrReturn(device != nullptr, OnSessionEstablishmentError(CHIP_ERROR_INVALID_DEVICE_DESCRIPTOR));
VerifyOrReturn(device != nullptr, PairingFailed(CHIP_ERROR_INVALID_DEVICE_DESCRIPTOR));

PASESession * pairing = &device->GetPairing();

Expand All @@ -842,7 +862,7 @@ void DeviceCommissioner::OnSessionEstablished()
if (err != CHIP_NO_ERROR)
{
ChipLogError(Controller, "Failed in setting up secure channel: err %s", ErrorStr(err));
OnSessionEstablishmentError(err);
PairingFailed(err);
return;
}

Expand Down
5 changes: 5 additions & 0 deletions src/controller/CHIPDeviceController.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -623,6 +623,11 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,

static void OnSessionEstablishmentTimeoutCallback(System::Layer * aLayer, void * aAppState);

// Helper to call once we decide that pairing has failed. This might be
// called from OnSessionEstablishmentError if we have no other transports to
// try, or in various other failure cases.
void PairingFailed(CHIP_ERROR err);

/* This function sends a Device Attestation Certificate chain request to the device.
The function does not hold a reference to the device object.
*/
Expand Down
149 changes: 134 additions & 15 deletions src/controller/SetUpCodePairer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ CHIP_ERROR SetUpCodePairer::PairDevice(NodeId remoteId, const char * setUpCode,
mRemoteId = remoteId;
mSetUpPINCode = payload.setUpPINCode;

ResetDiscoveryState();

return Connect(payload);
}

Expand Down Expand Up @@ -91,17 +93,29 @@ CHIP_ERROR SetUpCodePairer::StartDiscoverOverBle(SetupPayload & payload)
mCommissioner->ConnectBleTransportToSelf();
#endif // CHIP_DEVICE_CONFIG_ENABLE_BOTH_COMMISSIONER_AND_COMMISSIONEE
VerifyOrReturnError(mBleLayer != nullptr, CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE);
return mBleLayer->NewBleConnectionByDiscriminator(payload.discriminator, this, OnDiscoveredDeviceOverBleSuccess,
OnDiscoveredDeviceOverBleError);

ChipLogProgress(Controller, "Starting commissioning discovery over BLE");

// Handle possibly-sync callbacks.
mWaitingForDiscovery[kBLETransport] = true;
CHIP_ERROR err = mBleLayer->NewBleConnectionByDiscriminator(payload.discriminator, this, OnDiscoveredDeviceOverBleSuccess,
OnDiscoveredDeviceOverBleError);
if (err != CHIP_NO_ERROR)
{
mWaitingForDiscovery[kBLETransport] = false;
}
return err;
#else
return CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE;
#endif // CONFIG_NETWORK_LAYER_BLE
}

CHIP_ERROR SetUpCodePairer::StopConnectOverBle()
{
mWaitingForDiscovery[kBLETransport] = false;
#if CONFIG_NETWORK_LAYER_BLE
VerifyOrReturnError(mBleLayer != nullptr, CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE);
ChipLogDetail(Controller, "Stopping commissioning discovery over BLE");
return mBleLayer->CancelBleIncompleteConnection();
#else
return CHIP_NO_ERROR;
Expand All @@ -110,16 +124,28 @@ CHIP_ERROR SetUpCodePairer::StopConnectOverBle()

CHIP_ERROR SetUpCodePairer::StartDiscoverOverIP(SetupPayload & payload)
{
ChipLogProgress(Controller, "Starting commissioning discovery over DNS-SD");

currentFilter.type = payload.isShortDiscriminator ? Dnssd::DiscoveryFilterType::kShortDiscriminator
: Dnssd::DiscoveryFilterType::kLongDiscriminator;
currentFilter.code =
payload.isShortDiscriminator ? static_cast<uint16_t>((payload.discriminator >> 8) & 0x0F) : payload.discriminator;
return mCommissioner->DiscoverCommissionableNodes(currentFilter);
// Handle possibly-sync callbacks.
mWaitingForDiscovery[kIPTransport] = true;
CHIP_ERROR err = mCommissioner->DiscoverCommissionableNodes(currentFilter);
if (err != CHIP_NO_ERROR)
{
mWaitingForDiscovery[kIPTransport] = false;
}
return err;
}

CHIP_ERROR SetUpCodePairer::StopConnectOverIP()
{
currentFilter.type = Dnssd::DiscoveryFilterType::kNone;
ChipLogDetail(Controller, "Stopping commissioning discovery over DNS-SD");

mWaitingForDiscovery[kIPTransport] = false;
currentFilter.type = Dnssd::DiscoveryFilterType::kNone;
return CHIP_NO_ERROR;
}

Expand All @@ -130,30 +156,73 @@ CHIP_ERROR SetUpCodePairer::StartDiscoverOverSoftAP(SetupPayload & payload)

CHIP_ERROR SetUpCodePairer::StopConnectOverSoftAP()
{
mWaitingForDiscovery[kSoftAPTransport] = false;
return CHIP_NO_ERROR;
}

void SetUpCodePairer::OnDeviceDiscovered(RendezvousParameters & params)
bool SetUpCodePairer::ConnectToDiscoveredDevice()
{
if (mConnectionType == SetupCodePairerBehaviour::kCommission)
if (mWaitingForConnection)
{
LogErrorOnFailure(mCommissioner->PairDevice(mRemoteId, params.SetSetupPINCode(mSetUpPINCode)));
// Nothing to do. Just wait until we either succeed at that connection
// or fail and get asked to try another device.
return false;
}
else

for (auto & storedParams : mDiscoveredParameters)
{
LogErrorOnFailure(mCommissioner->EstablishPASEConnection(mRemoteId, params.SetSetupPINCode(mSetUpPINCode)));
if (!storedParams.HasPeerAddress())
{
continue;
}

// Clear out those params, since we will kick off a connection to them
// now.
RendezvousParameters params(storedParams);
storedParams = RendezvousParameters();

params.SetSetupPINCode(mSetUpPINCode);

#if CHIP_PROGRESS_LOGGING
char buf[Transport::PeerAddress::kMaxToStringSize];
params.GetPeerAddress().ToString(buf);
ChipLogProgress(Controller, "Attempting PASE connection to %s", buf);
#endif // CHIP_PROGRESS_LOGGING

CHIP_ERROR err;
if (mConnectionType == SetupCodePairerBehaviour::kCommission)
{
err = mCommissioner->PairDevice(mRemoteId, params);
}
else
{
err = mCommissioner->EstablishPASEConnection(mRemoteId, params);
}
LogErrorOnFailure(err);
if (err == CHIP_NO_ERROR)
{
mWaitingForConnection = true;
return true;
}
}

return false;
}

#if CONFIG_NETWORK_LAYER_BLE
void SetUpCodePairer::OnDiscoveredDeviceOverBle(BLE_CONNECTION_OBJECT connObj)
{
ChipLogProgress(Controller, "Discovered device to be commissioned over BLE");

mWaitingForDiscovery[kBLETransport] = false;

// Probably safe to stop connections over other transports at this point?
LogErrorOnFailure(StopConnectOverIP());
LogErrorOnFailure(StopConnectOverSoftAP());

Transport::PeerAddress peerAddress = Transport::PeerAddress::BLE();
RendezvousParameters params = RendezvousParameters().SetPeerAddress(peerAddress).SetConnectionObject(connObj);
OnDeviceDiscovered(params);
Transport::PeerAddress peerAddress = Transport::PeerAddress::BLE();
mDiscoveredParameters[kBLETransport] = RendezvousParameters().SetPeerAddress(peerAddress).SetConnectionObject(connObj);
ConnectToDiscoveredDevice();
}

void SetUpCodePairer::OnDiscoveredDeviceOverBleSuccess(void * appState, BLE_CONNECTION_OBJECT connObj)
Expand All @@ -163,6 +232,13 @@ void SetUpCodePairer::OnDiscoveredDeviceOverBleSuccess(void * appState, BLE_CONN

void SetUpCodePairer::OnDiscoveredDeviceOverBleError(void * appState, CHIP_ERROR err)
{
static_cast<SetUpCodePairer *>(appState)->OnBLEDiscoveryError(err);
}

void SetUpCodePairer::OnBLEDiscoveryError(CHIP_ERROR err)
{
ChipLogError(Controller, "Commissioning discovery over BLE failed: %" CHIP_ERROR_FORMAT, err.Format());
mWaitingForDiscovery[kBLETransport] = false;
LogErrorOnFailure(err);
}
#endif // CONFIG_NETWORK_LAYER_BLE
Expand Down Expand Up @@ -192,14 +268,57 @@ void SetUpCodePairer::NotifyCommissionableDeviceDiscovered(const Dnssd::Discover
{
return;
}
LogErrorOnFailure(StopConnectOverBle());

ChipLogProgress(Controller, "Discovered device to be commissioned over DNS-SD");

// Don't stop trying to connect over BLE, because we may be dealing with
// stale DNS-SD records.
LogErrorOnFailure(StopConnectOverIP());
LogErrorOnFailure(StopConnectOverSoftAP());

Inet::InterfaceId interfaceId = nodeData.ipAddress[0].IsIPv6LinkLocal() ? nodeData.interfaceId : Inet::InterfaceId::Null();
Transport::PeerAddress peerAddress = Transport::PeerAddress::UDP(nodeData.ipAddress[0], nodeData.port, interfaceId);
RendezvousParameters params = RendezvousParameters().SetPeerAddress(peerAddress);
OnDeviceDiscovered(params);
mDiscoveredParameters[kIPTransport] = RendezvousParameters().SetPeerAddress(peerAddress);
ConnectToDiscoveredDevice();
}

bool SetUpCodePairer::TryNextRendezvousParameters()
{
// Don't call PairDevice again; just do PASE as needed. If we called
// PairDevice before and PASE succeeds, the commmissioning that's been
// queued up will happen.
mConnectionType = SetupCodePairerBehaviour::kPaseOnly;
mWaitingForConnection = false;

if (ConnectToDiscoveredDevice())
{
ChipLogProgress(Controller, "Trying connection to commissionee over different transport");
return true;
}

for (const auto & waiting : mWaitingForDiscovery)
{
if (waiting)
{
ChipLogProgress(Controller, "Waiting to discover commissionees that match our filters");
return true;
}
}

return false;
}

void SetUpCodePairer::ResetDiscoveryState()
{
for (auto & waiting : mWaitingForDiscovery)
{
waiting = false;
}

for (auto & params : mDiscoveredParameters)
{
params = RendezvousParameters();
}
}

} // namespace Controller
Expand Down
47 changes: 45 additions & 2 deletions src/controller/SetUpCodePairer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ enum class SetupCodePairerBehaviour : uint8_t
class DLL_EXPORT SetUpCodePairer
{
public:
SetUpCodePairer(DeviceCommissioner * commissioner) : mCommissioner(commissioner) {}
SetUpCodePairer(DeviceCommissioner * commissioner) : mCommissioner(commissioner) { ResetDiscoveryState(); }
virtual ~SetUpCodePairer() {}

CHIP_ERROR PairDevice(chip::NodeId remoteId, const char * setUpCode,
Expand All @@ -66,6 +66,18 @@ class DLL_EXPORT SetUpCodePairer
void SetBleLayer(Ble::BleLayer * bleLayer) { mBleLayer = bleLayer; };
#endif // CONFIG_NETWORK_LAYER_BLE

// Called by the commissioner when PASE establishment fails.
//
// May start a new PASE establishment.
//
// Will return whether we might in fact have more rendezvous parameters to
// try (e.g. because we started a new PASE establishment or are waiting on
// more device discovery).
//
// The commissioner can use the return value to decide whether pairing has
// actually failed or not.
bool TryNextRendezvousParameters();

private:
CHIP_ERROR Connect(SetupPayload & paload);
CHIP_ERROR StartDiscoverOverBle(SetupPayload & payload);
Expand All @@ -75,11 +87,26 @@ class DLL_EXPORT SetUpCodePairer
CHIP_ERROR StartDiscoverOverSoftAP(SetupPayload & payload);
CHIP_ERROR StopConnectOverSoftAP();

void OnDeviceDiscovered(RendezvousParameters & params);
// Returns whether we have kicked off a new connection attempt.
bool ConnectToDiscoveredDevice();

// Reset our mWaitingForDiscovery/mDiscoveredParameters state to indicate no
// pending work.
void ResetDiscoveryState();

// Not an enum class because we use this for indexing into arrays.
enum TransportTypes
{
kBLETransport = 0,
kIPTransport,
kSoftAPTransport,
kTransportTypeCount,
};

#if CONFIG_NETWORK_LAYER_BLE
Ble::BleLayer * mBleLayer = nullptr;
void OnDiscoveredDeviceOverBle(BLE_CONNECTION_OBJECT connObj);
void OnBLEDiscoveryError(CHIP_ERROR err);
/////////// BLEConnectionDelegate Callbacks /////////
static void OnDiscoveredDeviceOverBleSuccess(void * appState, BLE_CONNECTION_OBJECT connObj);
static void OnDiscoveredDeviceOverBleError(void * appState, CHIP_ERROR err);
Expand All @@ -92,6 +119,22 @@ class DLL_EXPORT SetUpCodePairer
chip::NodeId mRemoteId;
uint32_t mSetUpPINCode = 0;
SetupCodePairerBehaviour mConnectionType = SetupCodePairerBehaviour::kCommission;

// Boolean will be set to true if we currently have an async discovery
// process happening via the relevant transport.
bool mWaitingForDiscovery[kTransportTypeCount] = { false };

// HasPeerAddress() for a given transport type will test true if we have
// discovered an address for that transport and not tried connecting to it
// yet. The general discovery/pairing process will terminate once all
// parameters test false for HasPeerAddress() and all the booleans in
// mWaitingForDiscovery are false.
RendezvousParameters mDiscoveredParameters[kTransportTypeCount];

// mWaitingForConnection is true if we have called either
// EstablishPASEConnection or PairDevice on mCommissioner and are now just
// waiting to see whether that works.
bool mWaitingForConnection = false;
};

} // namespace Controller
Expand Down

0 comments on commit 86e7ab5

Please sign in to comment.