Skip to content

Commit

Permalink
Extend our fail-safe a bit before we try to enable wifi/thread networks.
Browse files Browse the repository at this point in the history 
Two changes here:

1. Automatic fail-safe extension should not make the fail-safe _shorter_.  This
   should prevent, for example, a short fail-safe on attestation failure
   overriding a longer initial fail-safe.

2. Before we try to enable network (which is the last time we're guaranteed to
   be able to talk to a device over PASE, for non-concurrent commissioning flow
   devices), extend our fail-safe to try to cover the time it will take us to do
   operational discovery and sending CommissioningComplete.
  • Loading branch information
@bzbarsky-apple
bzbarsky-apple committed Mar 13, 2023
1 parent d04e396 commit e4349e4
Show file tree
Hide file tree
Showing 5 changed files with 119 additions and 18 deletions.
10 changes: 8 additions & 2 deletions src/app/DeviceProxy.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include <lib/core/CHIPCallback.h>
#include <lib/core/CHIPCore.h>
#include <lib/support/DLLUtil.h>
#include <system/SystemClock.h>

namespace chip {

Expand All @@ -54,7 +55,12 @@ class DLL_EXPORT DeviceProxy

virtual CHIP_ERROR SetPeerId(ByteSpan rcac, ByteSpan noc) { return CHIP_ERROR_NOT_IMPLEMENTED; }

const ReliableMessageProtocolConfig & GetRemoteMRPConfig() const { return mRemoteMRPConfig; }
/**
* Facilities for keeping track of the latest point we can expect the
* fail-safe to last through. These timestamp values use the monotonic clock.
*/
void SetFailSafeExpirationTimestamp(System::Clock::Timestamp timestamp) { mFailSafeExpirationTimestamp = timestamp; }
System::Clock::Timestamp GetFailSafeExpirationTimestamp() const { return mFailSafeExpirationTimestamp; }

/**
* @brief
Expand All @@ -69,7 +75,7 @@ class DLL_EXPORT DeviceProxy
protected:
virtual bool IsSecureConnected() const = 0;

ReliableMessageProtocolConfig mRemoteMRPConfig = GetDefaultMRPConfig();
System::Clock::Timestamp mFailSafeExpirationTimestamp = System::Clock::kZero;
};

} // namespace chip
3 changes: 3 additions & 0 deletions src/controller/AutoCommissioner.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,9 @@ void AutoCommissioner::SetCASEFailsafeTimerIfNeeded()
// CASE establishment, and receipt of the commissioning complete command.
// We know that the mCommissioneeDeviceProxy is still valid at this point since it gets cleared during cleanup
// and SetCASEFailsafeTimerIfNeeded is always called before that stage.
//
// A false return from ExtendArmFailSafe is fine; we don't want to make
// the fail-safe shorter here.
mCommissioner->ExtendArmFailSafe(mCommissioneeDeviceProxy, CommissioningStage::kFindOperational,
mParams.GetCASEFailsafeTimerSeconds().Value(),
GetCommandTimeout(mCommissioneeDeviceProxy, CommissioningStage::kArmFailsafe),
Expand Down
108 changes: 97 additions & 11 deletions src/controller/CHIPDeviceController.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1131,16 +1131,42 @@ void DeviceCommissioner::OnFailedToExtendedArmFailSafeDeviceAttestation(void * c
commissioner->CommissioningStageComplete(CHIP_ERROR_INTERNAL, report);
}

void DeviceCommissioner::ExtendArmFailSafe(DeviceProxy * proxy, CommissioningStage step, uint16_t armFailSafeTimeout,
bool DeviceCommissioner::ExtendArmFailSafe(DeviceProxy * proxy, CommissioningStage step, uint16_t armFailSafeTimeout,
Optional<System::Clock::Timeout> commandTimeout, OnExtendFailsafeSuccess onSuccess,
OnExtendFailsafeFailure onFailure)
{
using namespace System;
using namespace System::Clock;
auto now = SystemClock().GetMonotonicTimestamp();
auto newFailSafeTimeout = now + Seconds16(armFailSafeTimeout);
if (newFailSafeTimeout < proxy->GetFailSafeExpirationTimestamp())
{
ChipLogProgress(
Controller, "Skipping arming failsafe: new time (%u seconds from now) before old time (%u seconds from now)",
armFailSafeTimeout, std::chrono::duration_cast<Seconds16>(proxy->GetFailSafeExpirationTimestamp() - now).count());
return false;
}

uint64_t breadcrumb = static_cast<uint64_t>(step);
GeneralCommissioning::Commands::ArmFailSafe::Type request;
request.expiryLengthSeconds = armFailSafeTimeout;
request.breadcrumb = breadcrumb;
ChipLogProgress(Controller, "Arming failsafe (%u seconds)", request.expiryLengthSeconds);
SendCommand(proxy, request, onSuccess, onFailure, kRootEndpointId, commandTimeout);
CHIP_ERROR err = SendCommand(proxy, request, onSuccess, onFailure, kRootEndpointId, commandTimeout);
if (err != CHIP_NO_ERROR)
{
// TODO: What's the right error-handling here? We used to just silently
// ignore this failure... Scheduling a lambda to call
// onFailure(this, err) is hard, because a lambda we can schedule can't
// capture that much state.
}
else
{
// TODO: Handle the situation when our command ends up erroring out
// asynchronously?
proxy->SetFailSafeExpirationTimestamp(newFailSafeTimeout);
}
return true;
}

void DeviceCommissioner::ExtendArmFailSafeForDeviceAttestation(const Credentials::DeviceAttestationVerifier::AttestationInfo & info,
Expand All @@ -1154,20 +1180,24 @@ void DeviceCommissioner::ExtendArmFailSafeForDeviceAttestation(const Credentials
mAttestationDeviceInfo = Platform::MakeUnique<Credentials::DeviceAttestationVerifier::AttestationDeviceInfo>(info);

auto expiryLengthSeconds = deviceAttestationDelegate->FailSafeExpiryTimeoutSecs();
if (expiryLengthSeconds.HasValue())
bool failSafeSkipped = expiryLengthSeconds.HasValue();
if (failSafeSkipped)
{
GeneralCommissioning::Commands::ArmFailSafe::Type request;
request.expiryLengthSeconds = expiryLengthSeconds.Value();
request.breadcrumb = mCommissioningStage;
ChipLogProgress(Controller, "Changing fail-safe timer to %u seconds to handle DA failure", request.expiryLengthSeconds);
ChipLogProgress(Controller, "Changing fail-safe timer to %u seconds to handle DA failure", expiryLengthSeconds.Value());
// Per spec, anything we do with the fail-safe armed must not time out
// in less than kMinimumCommissioningStepTimeout.
SendCommand(mDeviceBeingCommissioned, request, OnArmFailSafeExtendedForDeviceAttestation,
OnFailedToExtendedArmFailSafeDeviceAttestation, MakeOptional(kMinimumCommissioningStepTimeout));
failSafeSkipped =
ExtendArmFailSafe(mDeviceBeingCommissioned, mCommissioningStage, expiryLengthSeconds.Value(),
MakeOptional(kMinimumCommissioningStepTimeout), OnArmFailSafeExtendedForDeviceAttestation,
OnFailedToExtendedArmFailSafeDeviceAttestation);
}
else
{
ChipLogProgress(Controller, "Proceeding without changing fail-safe timer value as delegate has not set it");
}

if (failSafeSkipped)
{
// Callee does not use data argument.
const GeneralCommissioning::Commands::ArmFailSafeResponse::DecodableType data;
OnArmFailSafeExtendedForDeviceAttestation(this, data);
Expand Down Expand Up @@ -1782,6 +1812,8 @@ void DeviceCommissioner::OnDeviceConnectionRetryFn(void * context, const ScopedN
{
failsafeTimeout = static_cast<uint16_t>(retryTimeout.count() + kDefaultFailsafeTimeout);
}
// A false return from ExtendArmFailSafe is fine; we don't want to make the
// fail-safe shorter here.
self->ExtendArmFailSafe(commissioneeDevice, CommissioningStage::kFindOperational, failsafeTimeout,
MakeOptional(kMinimumCommissioningStepTimeout), OnExtendFailsafeForCASERetrySuccess,
OnExtendFailsafeForCASERetryFailure);
Expand Down Expand Up @@ -2168,8 +2200,11 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
{
case CommissioningStage::kArmFailsafe: {
VerifyOrDie(endpoint == kRootEndpointId);
ExtendArmFailSafe(proxy, step, params.GetFailsafeTimerSeconds().ValueOr(kDefaultFailsafeTimeout), timeout, OnArmFailSafe,
OnBasicFailure);
// Make sure the fail-safe value we set here actually ends up being used
// no matter what.
proxy->SetFailSafeExpirationTimestamp(System::Clock::kZero);
VerifyOrDie(ExtendArmFailSafe(proxy, step, params.GetFailsafeTimerSeconds().ValueOr(kDefaultFailsafeTimeout), timeout,
OnArmFailSafe, OnBasicFailure));
}
break;
case CommissioningStage::kReadCommissioningInfo: {
Expand Down Expand Up @@ -2471,6 +2506,10 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT);
return;
}
// Before we try to do network enablement, make sure that our fail-safe
// is set far enough out that we can later try to do operational
// discovery without it timing out.
ExtendFailsafeBeforeNetworkEnable(proxy, params, step);
NetworkCommissioning::Commands::ConnectNetwork::Type request;
request.networkID = params.GetWiFiCredentials().Value().ssid;
request.breadcrumb.Emplace(breadcrumb);
Expand All @@ -2488,6 +2527,10 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT);
return;
}
// Before we try to do network enablement, make sure that our fail-safe
// is set far enough out that we can later try to do operational
// discovery without it timing out.
ExtendFailsafeBeforeNetworkEnable(proxy, params, step);
NetworkCommissioning::Commands::ConnectNetwork::Type request;
request.networkID = extendedPanId;
request.breadcrumb.Emplace(breadcrumb);
Expand Down Expand Up @@ -2522,6 +2565,49 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
}
}

// No specific action to take on either success or failure here; we're just
// trying to bump the fail-safe, and if that fails it's not clear there's much
// we can to with that.
static void OnExtendFailsafeBeforeNetworkEnableFailure(void * context, CHIP_ERROR error)
{
ChipLogError(Controller, "Failed to extend fail-safe before network enable: %" CHIP_ERROR_FORMAT, error.Format());
}
static void OnExtendFailsafeBeforeNetworkEnableSuccess(
void * context, const app::Clusters::GeneralCommissioning::Commands::ArmFailSafeResponse::DecodableType & data)
{
ChipLogProgress(Controller, "Status of extending fail-safe before network enable: %u", to_underlying(data.errorCode));
}

void DeviceCommissioner::ExtendFailsafeBeforeNetworkEnable(DeviceProxy * device, CommissioningParameters & params,
CommissioningStage step)
{
auto * commissioneeDevice = FindCommissioneeDevice(device->GetDeviceId());
if (device != commissioneeDevice)
{
// Not a commissionee device; just return.
return;
}

// Try to make sure we have at least enough time for our expected
// commissioning bits plus the MRP retries for a Sigma1.
uint16_t failSafeTimeoutSecs = params.GetFailsafeTimerSeconds().ValueOr(kDefaultFailsafeTimeout);
auto sigma1Timeout = CASESession::ComputeSigma1ResponseTimeout(commissioneeDevice->GetPairing().GetRemoteMRPConfig());
uint16_t sigma1TimeoutSecs = std::chrono::duration_cast<System::Clock::Seconds16>(sigma1Timeout).count();
if (UINT16_MAX - failSafeTimeoutSecs < sigma1TimeoutSecs)
{
failSafeTimeoutSecs = UINT16_MAX;
}
else
{
failSafeTimeoutSecs += sigma1TimeoutSecs;
}

// A false return from ExtendArmFailSafe is fine; we don't want to make the
// fail-safe shorter here.
ExtendArmFailSafe(commissioneeDevice, step, failSafeTimeoutSecs, MakeOptional(kMinimumCommissioningStepTimeout),
OnExtendFailsafeBeforeNetworkEnableSuccess, OnExtendFailsafeBeforeNetworkEnableFailure);
}

CHIP_ERROR DeviceController::GetCompressedFabricIdBytes(MutableByteSpan & outBytes) const
{
const auto * fabricInfo = GetFabricInfo();
Expand Down
12 changes: 10 additions & 2 deletions src/controller/CHIPDeviceController.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -683,8 +683,11 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
return mDefaultCommissioner == nullptr ? NullOptional : MakeOptional(mDefaultCommissioner->GetCommissioningParameters());
}

// Reset the arm failsafe timer during commissioning.
void ExtendArmFailSafe(DeviceProxy * proxy, CommissioningStage step, uint16_t armFailSafeTimeout,
// Reset the arm failsafe timer during commissioning. If this returns
// false, that means that the timer was already set for a longer time period
// than the new time we are trying to set. In this case, neither
// onSuccess nor onFailure will be called.
bool ExtendArmFailSafe(DeviceProxy * proxy, CommissioningStage step, uint16_t armFailSafeTimeout,
Optional<System::Clock::Timeout> commandTimeout, OnExtendFailsafeSuccess onSuccess,
OnExtendFailsafeFailure onFailure);

Expand Down Expand Up @@ -900,6 +903,11 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
// to the PairingDelegate upon arm failsafe command completion.
void CleanupCommissioning(DeviceProxy * proxy, NodeId nodeId, const CompletionStatus & completionStatus);

// Extend the fail-safe before trying to do network-enable (since after that
// point, for non-concurrent-commissioning devices, we may not have a way to
// extend it).
void ExtendFailsafeBeforeNetworkEnable(DeviceProxy * device, CommissioningParameters & params, CommissioningStage step);

chip::Callback::Callback<OnDeviceConnected> mOnDeviceConnectedCallback;
chip::Callback::Callback<OnDeviceConnectionFailure> mOnDeviceConnectionFailureCallback;
#if CHIP_DEVICE_CONFIG_ENABLE_AUTOMATIC_CASE_RETRIES
Expand Down
4 changes: 1 addition & 3 deletions src/controller/CommissioneeDeviceProxy.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,9 @@ CHIP_ERROR CommissioneeDeviceProxy::UpdateDeviceData(const Transport::PeerAddres
{
mDeviceAddress = addr;

mRemoteMRPConfig = config;

// Initialize PASE session state with any MRP parameters that DNS-SD has provided.
// It can be overridden by PASE session protocol messages that include MRP parameters.
mPairing.SetRemoteMRPConfig(mRemoteMRPConfig);
mPairing.SetRemoteMRPConfig(config);

if (!mSecureSession)
{
Expand Down

0 comments on commit e4349e4

Please sign in to comment.