Skip to content

Commit

Permalink
Make sure we don't advertise with CM=1 when we're not accepting PASE …
Browse files Browse the repository at this point in the history 
…connections. (#13813)

* Make sure we don't advertise with CM=1 when we're not accepting PASE connections.

Right now it's possible for a device to both be advertising over
dns-sd with CM=1 and not be accepting PASE connections.  Which makes
the CM=1 not exactly true.

Summary of changes:

1. Change DnssdServer to clearly differentiate between "Start with
default commissioning mode behavior based on whether we have
operational credentials" and "Start with commissioning mode disabled",
instead of treating the two as synonyms.  I audited all callsites that
explicitly passed kDisabled to StartServer() instead of no args, and
they all seem to want to actually disable commissioning mode.

2. Change CommissioningWindowManager to start/stop DNS-SD
commissioning advertising in the same codepaths as BLE advertising.
This means we stop advertising with CM=1 or CM=2 when we stop
accepting PASE connections, and start doing it again when we start
accepting PASE connections again (e.g. if commissioning fails).

* Apply review suggestion.

Co-authored-by: chrisdecenzo <[email protected]>

Co-authored-by: chrisdecenzo <[email protected]>
  • Loading branch information
@bzbarsky-apple @chrisdecenzo @pull
2 people authored and pull[bot] committed Aug 4, 2023
1 parent 310d0df commit 1701673
Show file tree
Hide file tree
Showing 4 changed files with 64 additions and 26 deletions.
45 changes: 29 additions & 16 deletions src/app/server/CommissioningWindowManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,13 @@ void CommissioningWindowManager::OnPlatformEvent(const DeviceLayer::ChipDeviceEv

void CommissioningWindowManager::Shutdown()
{
StopAdvertisement();
StopAdvertisement(/* aShuttingDown = */ true);

ResetState();
}

void CommissioningWindowManager::ResetState()
{
mUseECM = false;

mECMDiscriminator = 0;
Expand All @@ -88,10 +93,9 @@ void CommissioningWindowManager::Shutdown()

void CommissioningWindowManager::Cleanup()
{
Shutdown();
StopAdvertisement(/* aShuttingDown = */ false);

// reset all advertising
app::DnssdServer::Instance().StartServer(Dnssd::CommissioningMode::kDisabled);
ResetState();
}

void CommissioningWindowManager::OnSessionEstablishmentError(CHIP_ERROR err)
Expand Down Expand Up @@ -151,7 +155,7 @@ void CommissioningWindowManager::OnSessionEstablished()

DeviceLayer::PlatformMgr().AddEventHandler(OnPlatformEventWrapper, reinterpret_cast<intptr_t>(this));

StopAdvertisement();
StopAdvertisement(/* aShuttingDown = */ true);
ChipLogProgress(AppServer, "Device completed Rendezvous process");
}

Expand All @@ -171,17 +175,12 @@ CHIP_ERROR CommissioningWindowManager::OpenCommissioningWindow()
ReturnErrorOnFailure(mServer->GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(
Protocols::SecureChannel::MsgType::PBKDFParamRequest, &mPairingSession));

ReturnErrorOnFailure(StartAdvertisement());

if (mUseECM)
{
ReturnErrorOnFailure(SetTemporaryDiscriminator(mECMDiscriminator));
ReturnErrorOnFailure(
mPairingSession.WaitForPairing(mECMPASEVerifier, mECMIterations, ByteSpan(mECMSalt, mECMSaltLength), mECMPasscodeID,
keyID, Optional<ReliableMessageProtocolConfig>::Value(gDefaultMRPConfig), this));

// reset all advertising, indicating we are in commissioningMode
app::DnssdServer::Instance().StartServer(Dnssd::CommissioningMode::kEnabledEnhanced);
}
else
{
Expand All @@ -192,11 +191,10 @@ CHIP_ERROR CommissioningWindowManager::OpenCommissioningWindow()
pinCode, kSpake2p_Iteration_Count,
ByteSpan(reinterpret_cast<const uint8_t *>(kSpake2pKeyExchangeSalt), strlen(kSpake2pKeyExchangeSalt)), keyID,
Optional<ReliableMessageProtocolConfig>::Value(gDefaultMRPConfig), this));

// reset all advertising, indicating we are in commissioningMode
app::DnssdServer::Instance().StartServer(Dnssd::CommissioningMode::kEnabledBasic);
}

ReturnErrorOnFailure(StartAdvertisement());

return CHIP_NO_ERROR;
}

Expand Down Expand Up @@ -284,23 +282,29 @@ CHIP_ERROR CommissioningWindowManager::StartAdvertisement()
mAppDelegate->OnPairingWindowOpened();
}

Dnssd::CommissioningMode commissioningMode;
if (mUseECM)
{
mWindowStatus = app::Clusters::AdministratorCommissioning::CommissioningWindowStatus::kEnhancedWindowOpen;
mWindowStatus = app::Clusters::AdministratorCommissioning::CommissioningWindowStatus::kEnhancedWindowOpen;
commissioningMode = Dnssd::CommissioningMode::kEnabledEnhanced;
}
else
{
mWindowStatus = app::Clusters::AdministratorCommissioning::CommissioningWindowStatus::kBasicWindowOpen;
mWindowStatus = app::Clusters::AdministratorCommissioning::CommissioningWindowStatus::kBasicWindowOpen;
commissioningMode = Dnssd::CommissioningMode::kEnabledBasic;
}

// reset all advertising, indicating we are in commissioningMode
app::DnssdServer::Instance().StartServer(commissioningMode);

#if CHIP_DEVICE_CONFIG_ENABLE_SED
DeviceLayer::ConnectivityMgr().RequestSEDFastPollingMode(true);
#endif

return CHIP_NO_ERROR;
}

CHIP_ERROR CommissioningWindowManager::StopAdvertisement()
CHIP_ERROR CommissioningWindowManager::StopAdvertisement(bool aShuttingDown)
{
RestoreDiscriminator();

Expand All @@ -313,6 +317,15 @@ CHIP_ERROR CommissioningWindowManager::StopAdvertisement()
DeviceLayer::ConnectivityMgr().RequestSEDFastPollingMode(false);
#endif

// If aShuttingDown, don't try to change our DNS-SD advertisements.
if (!aShuttingDown)
{
// Stop advertising commissioning mode, since we're not accepting PASE
// connections right now. If we start accepting them again (via
// OpenCommissioningWindow) that will call StartAdvertisement as needed.
app::DnssdServer::Instance().StartServer(Dnssd::CommissioningMode::kDisabled);
}

if (mIsBLE)
{
ReturnErrorOnFailure(chip::DeviceLayer::ConnectivityMgr().SetBLEAdvertisingEnabled(false));
Expand Down
7 changes: 6 additions & 1 deletion src/app/server/CommissioningWindowManager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,15 @@ class CommissioningWindowManager : public SessionEstablishmentDelegate

CHIP_ERROR StartAdvertisement();

CHIP_ERROR StopAdvertisement();
CHIP_ERROR StopAdvertisement(bool aShuttingDown);

CHIP_ERROR OpenCommissioningWindow();

// Helper for Shutdown and Cleanup. Does not do anything with
// advertisements, because Shutdown and Cleanup want to handle those
// differently.
void ResetState();

AppDelegate * mAppDelegate = nullptr;
Server * mServer = nullptr;

Expand Down
25 changes: 19 additions & 6 deletions src/app/server/Dnssd.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -402,9 +402,22 @@ CHIP_ERROR DnssdServer::AdvertiseCommissionableNode(chip::Dnssd::CommissioningMo
return Advertise(true /* commissionableNode */, mode);
}

void DnssdServer::StartServer(chip::Dnssd::CommissioningMode mode)
void DnssdServer::StartServer()
{
ChipLogDetail(Discovery, "DNS-SD StartServer mode=%d", static_cast<int>(mode));
return StartServer(NullOptional);
}

void DnssdServer::StartServer(Dnssd::CommissioningMode mode)
{
return StartServer(MakeOptional(mode));
}

void DnssdServer::StartServer(Optional<Dnssd::CommissioningMode> mode)
{
// Default to CommissioningMode::kDisabled if no value is provided.
Dnssd::CommissioningMode modeValue = mode.ValueOr(Dnssd::CommissioningMode::kDisabled);

ChipLogDetail(Discovery, "DNS-SD StartServer modeHasValue=%d modeValue=%d", mode.HasValue(), static_cast<int>(modeValue));

ClearTimeouts();

Expand All @@ -431,9 +444,9 @@ void DnssdServer::StartServer(chip::Dnssd::CommissioningMode mode)
if (HaveOperationalCredentials())
{
ChipLogProgress(Discovery, "Have operational credentials");
if (mode != chip::Dnssd::CommissioningMode::kDisabled)
if (modeValue != chip::Dnssd::CommissioningMode::kDisabled)
{
err = AdvertiseCommissionableNode(mode);
err = AdvertiseCommissionableNode(modeValue);
if (err != CHIP_NO_ERROR)
{
ChipLogError(Discovery, "Failed to advertise commissionable node: %s", chip::ErrorStr(err));
Expand All @@ -443,7 +456,7 @@ void DnssdServer::StartServer(chip::Dnssd::CommissioningMode mode)
#if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY
else if (GetExtendedDiscoveryTimeoutSecs() != CHIP_DEVICE_CONFIG_DISCOVERY_DISABLED)
{
err = AdvertiseCommissionableNode(mode);
err = AdvertiseCommissionableNode(modeValue);
if (err != CHIP_NO_ERROR)
{
ChipLogError(Discovery, "Failed to advertise extended commissionable node: %s", chip::ErrorStr(err));
Expand All @@ -457,7 +470,7 @@ void DnssdServer::StartServer(chip::Dnssd::CommissioningMode mode)
{
#if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DISCOVERY
ChipLogProgress(Discovery, "Start dns-sd server - no current nodeId");
err = AdvertiseCommissionableNode(chip::Dnssd::CommissioningMode::kEnabledBasic);
err = AdvertiseCommissionableNode(mode.ValueOr(chip::Dnssd::CommissioningMode::kEnabledBasic));
if (err != CHIP_NO_ERROR)
{
ChipLogError(Discovery, "Failed to advertise unprovisioned commissionable node: %s", chip::ErrorStr(err));
Expand Down
13 changes: 10 additions & 3 deletions src/app/server/Dnssd.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#pragma once

#include <lib/core/CHIPError.h>
#include <lib/core/Optional.h>
#include <lib/dnssd/Advertiser.h>
#include <platform/CHIPDeviceLayer.h>
#include <stddef.h>
Expand Down Expand Up @@ -78,9 +79,12 @@ class DLL_EXPORT DnssdServer
CHIP_ERROR AdvertiseOperational();

/// (Re-)starts the Dnssd server
/// - if device has not yet been commissioned, then commissioning mode will show as enabled (CM=1, AC=0)
/// - if device has been commissioned, then commissioning mode will reflect the state of mode argument
void StartServer(chip::Dnssd::CommissioningMode mode = chip::Dnssd::CommissioningMode::kDisabled);
/// - if device has not yet been commissioned, then commissioning mode will show as enabled (CM=1)
/// - if device has been commissioned, then commissioning mode will be disabled.
void StartServer();

/// (Re-)starts the Dnssd server, using the provided commissioning mode.
void StartServer(Dnssd::CommissioningMode mode);

CHIP_ERROR GenerateRotatingDeviceId(char rotatingDeviceIdHexBuffer[], size_t rotatingDeviceIdHexBufferSize);

Expand Down Expand Up @@ -111,6 +115,9 @@ class DLL_EXPORT DnssdServer
#endif // CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY
}

// Helper for StartServer.
void StartServer(Optional<Dnssd::CommissioningMode> mode);

uint16_t mSecuredPort = CHIP_PORT;
uint16_t mUnsecuredPort = CHIP_UDC_PORT;

Expand Down

0 comments on commit 1701673

Please sign in to comment.