From d3107fb4cdb06e960ec96583f0e674f21008c85c Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 15 Jun 2022 20:35:05 -0400 Subject: [PATCH] Add a way to send a CloseSession StatusReport via chip-tool. (#19641) Specific changes: 1) Add a way to create a Protocols::Id from a 32-bit spec representation. 2) Change StatusReport to take Protocols::Id, not uint32_t. 3) Fix chip-tool interactive mode to log failures when a command fails to run. 4) Add a chip-tool command to send a CloseSession StatusReport. --- examples/chip-tool/BUILD.gn | 2 + .../chip-tool/commands/common/Commands.cpp | 7 +- .../commands/pairing/CloseSessionCommand.cpp | 88 +++++++++++++++++++ .../commands/pairing/CloseSessionCommand.h | 57 ++++++++++++ .../chip-tool/commands/pairing/Commands.h | 2 + src/protocols/Protocols.h | 10 ++- src/protocols/bdx/BdxTransferSession.cpp | 6 +- .../bdx/tests/TestBdxTransferSession.cpp | 2 +- src/protocols/secure_channel/PairingSession.h | 6 +- src/protocols/secure_channel/StatusReport.cpp | 13 +-- src/protocols/secure_channel/StatusReport.h | 9 +- .../secure_channel/tests/TestStatusReport.cpp | 4 +- 12 files changed, 185 insertions(+), 21 deletions(-) create mode 100644 examples/chip-tool/commands/pairing/CloseSessionCommand.cpp create mode 100644 examples/chip-tool/commands/pairing/CloseSessionCommand.h diff --git a/examples/chip-tool/BUILD.gn b/examples/chip-tool/BUILD.gn index fbe554ed741318..694d88a7ceadd0 100644 --- a/examples/chip-tool/BUILD.gn +++ b/examples/chip-tool/BUILD.gn @@ -65,6 +65,8 @@ static_library("chip-tool-utils") { # TODO - enable CommissionedListCommand once DNS Cache is implemented # "commands/pairing/CommissionedListCommand.cpp", # "commands/pairing/CommissionedListCommand.h", + "commands/pairing/CloseSessionCommand.cpp", + "commands/pairing/CloseSessionCommand.h", "commands/pairing/OpenCommissioningWindowCommand.cpp", "commands/pairing/OpenCommissioningWindowCommand.h", "commands/pairing/PairingCommand.cpp", diff --git a/examples/chip-tool/commands/common/Commands.cpp b/examples/chip-tool/commands/common/Commands.cpp index b7bd6f03db466f..16e188c1dee2ca 100644 --- a/examples/chip-tool/commands/common/Commands.cpp +++ b/examples/chip-tool/commands/common/Commands.cpp @@ -58,7 +58,12 @@ int Commands::Run(int argc, char ** argv) int Commands::RunInteractive(int argc, char ** argv) { CHIP_ERROR err = RunCommand(argc, argv, true); - return (err == CHIP_NO_ERROR) ? EXIT_SUCCESS : EXIT_FAILURE; + if (err == CHIP_NO_ERROR) + { + return EXIT_SUCCESS; + } + ChipLogError(chipTool, "Run command failure: %s", chip::ErrorStr(err)); + return EXIT_FAILURE; } CHIP_ERROR Commands::RunCommand(int argc, char ** argv, bool interactive) diff --git a/examples/chip-tool/commands/pairing/CloseSessionCommand.cpp b/examples/chip-tool/commands/pairing/CloseSessionCommand.cpp new file mode 100644 index 00000000000000..b30d0accae1639 --- /dev/null +++ b/examples/chip-tool/commands/pairing/CloseSessionCommand.cpp @@ -0,0 +1,88 @@ +/* + * Copyright (c) 2022 Project CHIP Authors + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#include "CloseSessionCommand.h" +#include +#include + +using namespace chip; +using namespace chip::Protocols; + +CHIP_ERROR CloseSessionCommand::RunCommand() +{ + CommissioneeDeviceProxy * commissioneeDeviceProxy = nullptr; + if (CHIP_NO_ERROR == CurrentCommissioner().GetDeviceBeingCommissioned(mDestinationId, &commissioneeDeviceProxy)) + { + return CloseSession(commissioneeDeviceProxy); + } + + return CurrentCommissioner().GetConnectedDevice(mDestinationId, &mOnDeviceConnectedCallback, + &mOnDeviceConnectionFailureCallback); +} + +CHIP_ERROR CloseSessionCommand::CloseSession(DeviceProxy * device) +{ + VerifyOrReturnError(device->GetSecureSession().HasValue(), CHIP_ERROR_INCORRECT_STATE); + + // TODO perhaps factor out this code into something on StatusReport that + // takes an exchange and maybe a SendMessageFlags? + SecureChannel::StatusReport statusReport(SecureChannel::GeneralStatusCode::kSuccess, SecureChannel::Id, + SecureChannel::kProtocolCodeCloseSession); + + size_t reportSize = statusReport.Size(); + Encoding::LittleEndian::PacketBufferWriter bbuf(MessagePacketBuffer::New(reportSize), reportSize); + statusReport.WriteToBuffer(bbuf); + + System::PacketBufferHandle msg = bbuf.Finalize(); + VerifyOrReturnError(!msg.IsNull(), CHIP_ERROR_NO_MEMORY); + + auto * exchange = device->GetExchangeManager()->NewContext(device->GetSecureSession().Value(), nullptr); + VerifyOrReturnError(exchange != nullptr, CHIP_ERROR_NO_MEMORY); + + // Per spec, CloseSession reports are always sent with MRP disabled. + CHIP_ERROR err = + exchange->SendMessage(SecureChannel::MsgType::StatusReport, std::move(msg), Messaging::SendMessageFlags::kNoAutoRequestAck); + if (err == CHIP_NO_ERROR) + { + SetCommandExitStatus(CHIP_NO_ERROR); + } + else + { + exchange->Close(); + } + + return err; +} + +void CloseSessionCommand::OnDeviceConnectedFn(void * context, OperationalDeviceProxy * device) +{ + auto * command = reinterpret_cast(context); + VerifyOrReturn(command != nullptr, ChipLogError(chipTool, "OnDeviceConnectedFn: context is null")); + + CHIP_ERROR err = command->CloseSession(device); + VerifyOrReturn(CHIP_NO_ERROR == err, command->SetCommandExitStatus(err)); +} + +void CloseSessionCommand::OnDeviceConnectionFailureFn(void * context, PeerId peerId, CHIP_ERROR err) +{ + LogErrorOnFailure(err); + + auto * command = reinterpret_cast(context); + VerifyOrReturn(command != nullptr, ChipLogError(chipTool, "OnDeviceConnectionFailureFn: context is null")); + command->SetCommandExitStatus(err); +} diff --git a/examples/chip-tool/commands/pairing/CloseSessionCommand.h b/examples/chip-tool/commands/pairing/CloseSessionCommand.h new file mode 100644 index 00000000000000..782e334ebfe74a --- /dev/null +++ b/examples/chip-tool/commands/pairing/CloseSessionCommand.h @@ -0,0 +1,57 @@ +/* + * Copyright (c) 2022 Project CHIP Authors + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#pragma once + +#include "../common/CHIPCommand.h" +#include +#include +#include + +class CloseSessionCommand : public CHIPCommand +{ +public: + CloseSessionCommand(CredentialIssuerCommands * credIssuerCommands) : + CHIPCommand("close-session", credIssuerCommands), mOnDeviceConnectedCallback(OnDeviceConnectedFn, this), + mOnDeviceConnectionFailureCallback(OnDeviceConnectionFailureFn, this) + { + AddArgument("destination-id", 0, UINT64_MAX, &mDestinationId); + AddArgument("timeout", 0, UINT64_MAX, &mTimeoutSecs, + "Time, in seconds, before this command is considered to have timed out."); + } + + /////////// CHIPCommand Interface ///////// + CHIP_ERROR RunCommand() override; + chip::System::Clock::Timeout GetWaitDuration() const override + { + return chip::System::Clock::Seconds16(mTimeoutSecs.ValueOr(10)); + } + +private: + chip::NodeId mDestinationId; + chip::Optional mTimeoutSecs; + + static void OnDeviceConnectedFn(void * context, chip::OperationalDeviceProxy * device); + static void OnDeviceConnectionFailureFn(void * context, PeerId peerId, CHIP_ERROR error); + + // Try to send the action CloseSession status report. + CHIP_ERROR CloseSession(chip::DeviceProxy * device); + + chip::Callback::Callback mOnDeviceConnectedCallback; + chip::Callback::Callback mOnDeviceConnectionFailureCallback; +}; diff --git a/examples/chip-tool/commands/pairing/Commands.h b/examples/chip-tool/commands/pairing/Commands.h index ef06ca2f4dd654..80506e6df0e9e0 100644 --- a/examples/chip-tool/commands/pairing/Commands.h +++ b/examples/chip-tool/commands/pairing/Commands.h @@ -18,6 +18,7 @@ #pragma once +#include "CloseSessionCommand.h" #include "CommissionedListCommand.h" #include "OpenCommissioningWindowCommand.h" #include "PairingCommand.h" @@ -218,6 +219,7 @@ void registerCommandsPairing(Commands & commands, CredentialIssuerCommands * cre // make_unique(), make_unique(credsIssuerConfig), make_unique(credsIssuerConfig), + make_unique(credsIssuerConfig), }; commands.Register(clusterName, clusterCommands); diff --git a/src/protocols/Protocols.h b/src/protocols/Protocols.h index 7f87ce0aea59d1..faadaeedf652d3 100644 --- a/src/protocols/Protocols.h +++ b/src/protocols/Protocols.h @@ -58,8 +58,16 @@ class Id constexpr VendorId GetVendorId() const { return mVendorId; } constexpr uint16_t GetProtocolId() const { return mProtocolId; } + static constexpr uint32_t kVendorIdShift = 16; + + static Id FromFullyQualifiedSpecForm(uint32_t aSpecForm) + { + return Id(static_cast(aSpecForm >> kVendorIdShift), + static_cast(aSpecForm & ((1 << kVendorIdShift) - 1))); + } + private: - constexpr uint32_t ToUint32() const { return (static_cast(mVendorId) << 16) | mProtocolId; } + constexpr uint32_t ToUint32() const { return (static_cast(mVendorId) << kVendorIdShift) | mProtocolId; } chip::VendorId mVendorId; uint16_t mProtocolId; diff --git a/src/protocols/bdx/BdxTransferSession.cpp b/src/protocols/bdx/BdxTransferSession.cpp index 945381e2fefa2c..391511c52e0bd7 100644 --- a/src/protocols/bdx/BdxTransferSession.cpp +++ b/src/protocols/bdx/BdxTransferSession.cpp @@ -516,7 +516,7 @@ CHIP_ERROR TransferSession::HandleStatusReportMessage(const PayloadHeader & head Protocols::SecureChannel::StatusReport report; ReturnErrorOnFailure(report.Parse(std::move(msg))); - VerifyOrReturnError((report.GetProtocolId() == Protocols::BDX::Id.ToFullyQualifiedSpecForm()), CHIP_ERROR_INVALID_MESSAGE_TYPE); + VerifyOrReturnError((report.GetProtocolId() == Protocols::BDX::Id), CHIP_ERROR_INVALID_MESSAGE_TYPE); mStatusReportData.statusCode = static_cast(report.GetProtocolCode()); @@ -871,8 +871,8 @@ void TransferSession::PrepareStatusReport(StatusCode code) { mStatusReportData.statusCode = code; - Protocols::SecureChannel::StatusReport report(Protocols::SecureChannel::GeneralStatusCode::kFailure, - Protocols::BDX::Id.ToFullyQualifiedSpecForm(), to_underlying(code)); + Protocols::SecureChannel::StatusReport report(Protocols::SecureChannel::GeneralStatusCode::kFailure, Protocols::BDX::Id, + to_underlying(code)); size_t msgSize = report.Size(); Encoding::LittleEndian::PacketBufferWriter bbuf(chip::MessagePacketBuffer::New(msgSize), msgSize); VerifyOrExit(!bbuf.IsNull(), mPendingOutput = OutputEventType::kInternalError); diff --git a/src/protocols/bdx/tests/TestBdxTransferSession.cpp b/src/protocols/bdx/tests/TestBdxTransferSession.cpp index f18487e2373af2..0242b88daaebe6 100644 --- a/src/protocols/bdx/tests/TestBdxTransferSession.cpp +++ b/src/protocols/bdx/tests/TestBdxTransferSession.cpp @@ -124,7 +124,7 @@ void VerifyStatusReport(nlTestSuite * inSuite, void * inContext, const System::P err = report.Parse(std::move(msgCopy)); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); NL_TEST_ASSERT(inSuite, report.GetGeneralCode() == SecureChannel::GeneralStatusCode::kFailure); - NL_TEST_ASSERT(inSuite, report.GetProtocolId() == Protocols::BDX::Id.ToFullyQualifiedSpecForm()); + NL_TEST_ASSERT(inSuite, report.GetProtocolId() == Protocols::BDX::Id); NL_TEST_ASSERT(inSuite, report.GetProtocolCode() == static_cast(expectedCode)); } diff --git a/src/protocols/secure_channel/PairingSession.h b/src/protocols/secure_channel/PairingSession.h index bc9d75892b2439..f8ac1d3afcd4ba 100644 --- a/src/protocols/secure_channel/PairingSession.h +++ b/src/protocols/secure_channel/PairingSession.h @@ -136,12 +136,11 @@ class DLL_EXPORT PairingSession : public SessionDelegate Protocols::SecureChannel::GeneralStatusCode generalCode = (protocolCode == Protocols::SecureChannel::kProtocolCodeSuccess) ? Protocols::SecureChannel::GeneralStatusCode::kSuccess : Protocols::SecureChannel::GeneralStatusCode::kFailure; - uint32_t protocolId = Protocols::SecureChannel::Id.ToFullyQualifiedSpecForm(); ChipLogDetail(SecureChannel, "Sending status report. Protocol code %d, exchange %d", protocolCode, exchangeCtxt->GetExchangeId()); - Protocols::SecureChannel::StatusReport statusReport(generalCode, protocolId, protocolCode); + Protocols::SecureChannel::StatusReport statusReport(generalCode, Protocols::SecureChannel::Id, protocolCode); Encoding::LittleEndian::PacketBufferWriter bbuf(System::PacketBufferHandle::New(statusReport.Size())); statusReport.WriteToBuffer(bbuf); @@ -161,8 +160,7 @@ class DLL_EXPORT PairingSession : public SessionDelegate Protocols::SecureChannel::StatusReport report; CHIP_ERROR err = report.Parse(std::move(msg)); ReturnErrorOnFailure(err); - VerifyOrReturnError(report.GetProtocolId() == Protocols::SecureChannel::Id.ToFullyQualifiedSpecForm(), - CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrReturnError(report.GetProtocolId() == Protocols::SecureChannel::Id, CHIP_ERROR_INVALID_ARGUMENT); if (report.GetGeneralCode() == Protocols::SecureChannel::GeneralStatusCode::kSuccess && report.GetProtocolCode() == Protocols::SecureChannel::kProtocolCodeSuccess && successExpected) diff --git a/src/protocols/secure_channel/StatusReport.cpp b/src/protocols/secure_channel/StatusReport.cpp index 781df11d308f65..18df6f7383e516 100644 --- a/src/protocols/secure_channel/StatusReport.cpp +++ b/src/protocols/secure_channel/StatusReport.cpp @@ -32,14 +32,15 @@ namespace chip { namespace Protocols { namespace SecureChannel { -StatusReport::StatusReport() : mGeneralCode(GeneralStatusCode::kSuccess), mProtocolId(0), mProtocolCode(0), mProtocolData(nullptr) +StatusReport::StatusReport() : + mGeneralCode(GeneralStatusCode::kSuccess), mProtocolId(SecureChannel::Id), mProtocolCode(0), mProtocolData(nullptr) {} -StatusReport::StatusReport(GeneralStatusCode generalCode, uint32_t protocolId, uint16_t protocolCode) : +StatusReport::StatusReport(GeneralStatusCode generalCode, Protocols::Id protocolId, uint16_t protocolCode) : mGeneralCode(generalCode), mProtocolId(protocolId), mProtocolCode(protocolCode), mProtocolData(nullptr) {} -StatusReport::StatusReport(GeneralStatusCode generalCode, uint32_t protocolId, uint16_t protocolCode, +StatusReport::StatusReport(GeneralStatusCode generalCode, Protocols::Id protocolId, uint16_t protocolCode, System::PacketBufferHandle protocolData) : mGeneralCode(generalCode), mProtocolId(protocolId), mProtocolCode(protocolCode), mProtocolData(std::move(protocolData)) @@ -54,7 +55,9 @@ CHIP_ERROR StatusReport::Parse(System::PacketBufferHandle buf) uint8_t * bufStart = buf->Start(); LittleEndian::Reader bufReader(bufStart, buf->DataLength()); - ReturnErrorOnFailure(bufReader.Read16(&tempGeneralCode).Read32(&mProtocolId).Read16(&mProtocolCode).StatusCode()); + uint32_t protocolId; + ReturnErrorOnFailure(bufReader.Read16(&tempGeneralCode).Read32(&protocolId).Read16(&mProtocolCode).StatusCode()); + mProtocolId = Protocols::Id::FromFullyQualifiedSpecForm(protocolId); mGeneralCode = static_cast(tempGeneralCode); // Any data that exists after the required fields is considered protocol-specific data. @@ -78,7 +81,7 @@ CHIP_ERROR StatusReport::Parse(System::PacketBufferHandle buf) Encoding::LittleEndian::BufferWriter & StatusReport::WriteToBuffer(Encoding::LittleEndian::BufferWriter & buf) const { - buf.Put16(to_underlying(mGeneralCode)).Put32(mProtocolId).Put16(mProtocolCode); + buf.Put16(to_underlying(mGeneralCode)).Put32(mProtocolId.ToFullyQualifiedSpecForm()).Put16(mProtocolCode); if (!mProtocolData.IsNull()) { buf.Put(mProtocolData->Start(), mProtocolData->DataLength()); diff --git a/src/protocols/secure_channel/StatusReport.h b/src/protocols/secure_channel/StatusReport.h index 875c8fda0de279..c0704d5bca4a99 100644 --- a/src/protocols/secure_channel/StatusReport.h +++ b/src/protocols/secure_channel/StatusReport.h @@ -19,6 +19,7 @@ #pragma once #include +#include #include namespace chip { @@ -44,7 +45,7 @@ class DLL_EXPORT StatusReport * @param protocolId Must specify a ProtocolId which consists of Vendor Id (upper 16 bits) and ProtocolId (lower 16 bits) * @param protocolCode A code defined by the specified protocol which provides more information about the status */ - StatusReport(GeneralStatusCode generalCode, uint32_t protocolId, uint16_t protocolCode); + StatusReport(GeneralStatusCode generalCode, Protocols::Id protocolId, uint16_t protocolCode); // /** @@ -55,7 +56,7 @@ class DLL_EXPORT StatusReport * @param protocolCode A code defined by the specified protocol which provides more information about the status * @param protocolData A \c PacketBufferHandle containing the protocol-specific data */ - StatusReport(GeneralStatusCode generalCode, uint32_t protocolId, uint16_t protocolCode, + StatusReport(GeneralStatusCode generalCode, Protocols::Id protocolId, uint16_t protocolCode, System::PacketBufferHandle protocolData); /** @@ -88,13 +89,13 @@ class DLL_EXPORT StatusReport size_t Size() const; GeneralStatusCode GetGeneralCode() const { return mGeneralCode; } - uint32_t GetProtocolId() const { return mProtocolId; } + Protocols::Id GetProtocolId() const { return mProtocolId; } uint16_t GetProtocolCode() const { return mProtocolCode; } const System::PacketBufferHandle & GetProtocolData() const { return mProtocolData; } private: GeneralStatusCode mGeneralCode; - uint32_t mProtocolId; + Protocols::Id mProtocolId; uint16_t mProtocolCode; System::PacketBufferHandle mProtocolData; diff --git a/src/protocols/secure_channel/tests/TestStatusReport.cpp b/src/protocols/secure_channel/tests/TestStatusReport.cpp index fe2181194d0369..35cbe1b5bca65e 100644 --- a/src/protocols/secure_channel/tests/TestStatusReport.cpp +++ b/src/protocols/secure_channel/tests/TestStatusReport.cpp @@ -33,7 +33,7 @@ using namespace chip::Protocols::SecureChannel; void TestStatusReport_NoData(nlTestSuite * inSuite, void * inContext) { GeneralStatusCode generalCode = GeneralStatusCode::kSuccess; - uint32_t protocolId = SecureChannel::Id.ToFullyQualifiedSpecForm(); + auto protocolId = SecureChannel::Id; uint16_t protocolCode = kProtocolCodeSuccess; StatusReport testReport(generalCode, protocolId, protocolCode); @@ -59,7 +59,7 @@ void TestStatusReport_NoData(nlTestSuite * inSuite, void * inContext) void TestStatusReport_WithData(nlTestSuite * inSuite, void * inContext) { GeneralStatusCode generalCode = GeneralStatusCode::kFailure; - uint32_t protocolId = SecureChannel::Id.ToFullyQualifiedSpecForm(); + auto protocolId = SecureChannel::Id; uint16_t protocolCode = static_cast(StatusCode::InvalidFabricConfig); uint8_t data[6] = { 42, 19, 3, 1, 3, 0 }; const uint16_t dataLen = 6;