openthread
Coap blockwise issue - token missing
Describe the bug A clear and concise description of what the bug is. Coap clockwise- doesn't work. Issue is all Coap block's header should carry same token number. But in our observation from client side first Coap block was having token number while same token number was not set for 2nd and consecutive token numbers. Version #define OT_THREAD_VERSION_INVALID 0 #define OT_THREAD_VERSION_1_1 2 #define OT_THREAD_VERSION_1_2 3
We have used Silicon lab MG24 chip.
TO reproduce issue
- Just define block size - may be 1024 bytes
- Try to send 1500 bytes of data. This requires two blocks of data to be sent.
- We can observe that only 1024 bytes of data received by server, while 2nd block and remaining bytes doesn't reach server.
We added below fix. coap.zip File name: src\core\coap.cpp method name: CoapBase::PrepareNextBlockRequest SuccessOrExit(error = iterator.Init(aRequestOld)); ++ SuccessOrExit(error = aRequest.SetTokenFromMessage(aRequestOld));
Thanks @dilipzi The fix/change seems reasonable to me. Please feel free to submit a PR with this change, or if you want I can help submit this (please let me know).
I went ahead and submitted https://github.com/openthread/openthread/pull/7980. Thanks.
Thanks Abtin,
Regards Dilip
From: Abtin Keshavarzian @.> Sent: 04 August 2022 00:01 To: openthread/openthread @.> Cc: Dilip Kumar Agarwal @.>; Mention @.> Subject: Re: [openthread/openthread] Coap blockwise issue - token missing (Issue #7976)
EXTERNAL EMAIL
Reply to this email directly, view it on GitHubhttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fopenthread%2Fopenthread%2Fissues%2F7976%23issuecomment-1204330267&data=05%7C01%7Cdilipkumar.agarwal%40nagra.com%7C312e0681a1864d20ef3308da757e5d8b%7Ce573309544254f08b6ba487b9a46a425%7C0%7C0%7C637951482839410994%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=mgFQOwk17H5mL47Is1iCGO6%2FPnLslJJKyquwT6KO%2FDc%3D&reserved=0, or unsubscribehttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAXL26LDS4HBKFKE65VIV7K3VXK3HTANCNFSM55OF7TGA&data=05%7C01%7Cdilipkumar.agarwal%40nagra.com%7C312e0681a1864d20ef3308da757e5d8b%7Ce573309544254f08b6ba487b9a46a425%7C0%7C0%7C637951482839410994%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=zVFsK6pO2H0dgOVOGIwXpdovdd31azTIfCdZw7DmsJk%3D&reserved=0. You are receiving this because you were mentioned.Message ID: @.@.>>
@dilipzi , thanks for raising this issue.
@dilipzi @abtink , can you help provide a pointer to RFC text that requires the CoAP Token to be the same across different GET requests for different blocks? As far as I know, the ETag is intended to be used for correlating blocks of a given representation - see RFC 7959 Section 2.4.
I have also encountered this issue. I can't seem to find a definitive answer on the use of tokens for block correlation, though it seems that they are not intended to be used as such per RFC7959 - 3.4 (see paragraph under figure 12).
I have been working through a few changes to get enough functionality for our FTD to communicate with a linux host running libcoap. Most block transfers do not work in this case, mostly due to the options not being appropriately copied in certain circumstances.
libcoap generates stateless tokens per each block past the original, such that each block in the transfer has a different token. I ended up generating random tokens for each block, though I don't think it is necessary.
As is, it is possible for options to be copied out of order especially when user defined options with number higher than block1/2 are present. Here's what I've done wrt option processing. I have tested this only between a nrf52840dk FTD <-> linux host running libcoap, all of block2 tx/rx and block1 tx/rx work with these changes. Previously only block1 rx worked. This has NOT been tested for simultaneous use of block1/2 in a transfer.
@@ -577,7 +585,6 @@ Error CoapBase::PrepareNextBlockRequest(Message::BlockType aType,
Message & aMessage)
{
Error error = kErrorNone;
- bool isOptionSet = false;
uint64_t optionBuf = 0;
uint16_t blockOption = 0;
Option::Iterator iterator;
@@ -585,55 +592,42 @@ Error CoapBase::PrepareNextBlockRequest(Message::BlockType aType,
blockOption = (aType == Message::kBlockType1) ? kOptionBlock1 : kOptionBlock2;
aRequest.Init(kTypeConfirmable, static_cast<ot::Coap::Code>(aRequestOld.GetCode()));
+ IgnoreReturnValue(aRequest.GenerateRandomToken(aRequestOld.GetTokenLength()));
+
+ /// NOTE: Options MUST be added in ascending order
SuccessOrExit(error = iterator.Init(aRequestOld));
-
- // Copy options from last response to next message
- for (; !iterator.IsDone() && iterator.GetOption()->GetLength() != 0; error = iterator.Advance())
+ while (!iterator.IsDone())
{
- uint16_t optionNumber = iterator.GetOption()->GetNumber();
-
- SuccessOrExit(error);
-
- // Check if option to copy next is higher than or equal to Block1 option
- if (optionNumber >= blockOption && !isOptionSet)
+ if(iterator.GetOption()->GetLength() != 0)
{
- // Write Block1 option to next message
- SuccessOrExit(error = aRequest.AppendBlockOption(aType, aMessage.GetBlockWiseBlockNumber() + 1, aMoreBlocks,
- aMessage.GetBlockWiseBlockSize()));
- aRequest.SetBlockWiseBlockNumber(aMessage.GetBlockWiseBlockNumber() + 1);
- aRequest.SetBlockWiseBlockSize(aMessage.GetBlockWiseBlockSize());
- aRequest.SetMoreBlocksFlag(aMoreBlocks);
-
- isOptionSet = true;
+ uint16_t optionNumber = iterator.GetOption()->GetNumber();
- // If option to copy next is Block1 or Block2 option, option is not copied
- if (optionNumber == kOptionBlock1 || optionNumber == kOptionBlock2)
+ if (optionNumber == blockOption)
{
- continue;
+ // Replace the block option with params for the next block
+ SuccessOrExit(error = aRequest.AppendBlockOption(aType, aMessage.GetBlockWiseBlockNumber() + 1, aMoreBlocks,
+ aMessage.GetBlockWiseBlockSize()));
+ aRequest.SetBlockWiseBlockNumber(aMessage.GetBlockWiseBlockNumber() + 1);
+ aRequest.SetBlockWiseBlockSize(aMessage.GetBlockWiseBlockSize());
+ aRequest.SetMoreBlocksFlag(aMoreBlocks);
+ }
+ else
+ {
+ // Copy option
+ SuccessOrExit(error = iterator.ReadOptionValue(&optionBuf));
+ SuccessOrExit(error = aRequest.AppendOption(optionNumber, iterator.GetOption()->GetLength(), &optionBuf));
}
}
- // Copy option
- SuccessOrExit(error = iterator.ReadOptionValue(&optionBuf));
- SuccessOrExit(error = aRequest.AppendOption(optionNumber, iterator.GetOption()->GetLength(), &optionBuf));
- }
-
- if (!isOptionSet)
- {
- // Write Block1 option to next message
- SuccessOrExit(error = aRequest.AppendBlockOption(aType, aMessage.GetBlockWiseBlockNumber() + 1, aMoreBlocks,
- aMessage.GetBlockWiseBlockSize()));
- aRequest.SetBlockWiseBlockNumber(aMessage.GetBlockWiseBlockNumber() + 1);
- aRequest.SetBlockWiseBlockSize(aMessage.GetBlockWiseBlockSize());
- aRequest.SetMoreBlocksFlag(aMoreBlocks);
+ IgnoreReturnValue(iterator.Advance());
}
exit:
return error;
}
As well as here, where other options were not copied at all. Exiting on iterator.Advance() also drops options, though I cannot remember what the deal was.
@@ -909,8 +926,12 @@ Error CoapBase::ProcessBlock2Request(Message & aMessage,
SuccessOrExit(error = iterator.ReadOptionValue(&optionBuf));
SuccessOrExit(error = response->AppendOption(optionNumber, iterator.GetOption()->GetLength(), &optionBuf));
}
+ else {
+ SuccessOrExit(error = iterator.ReadOptionValue(&optionBuf));
+ SuccessOrExit(error = response->AppendOption(optionNumber, iterator.GetOption()->GetLength(), &optionBuf));
+ }
- SuccessOrExit(error = iterator.Advance());
+ IgnoreReturnValue(iterator.Advance());
}
SuccessOrExit(error = response->SetPayloadMarker());
@@ -1135,15 +1158,16 @@ void CoapBase::ProcessReceivedResponse(Message &aMessage, const Ip6::MessageInfo
break;
case kOptionSize2:
- // ToDo: wait for method to read uint option values
- totalTransfereSize = 0;
+ uint64_t value;
+ SuccessOrExit(error = otCoapOptionIteratorGetOptionUintValue(&iterator, &value));
+ totalTransfereSize = (uint32_t)(value & 0xFFFFFFFF);
break;
default:
break;
}
- SuccessOrExit(error = iterator.Advance());
+ IgnoreReturnValue(iterator.Advance());
}
}
switch (blockOptionType)
@@ -1304,15 +1328,16 @@ void CoapBase::ProcessReceivedRequest(Message &aMessage, const Ip6::MessageInfo
break;
case kOptionSize1:
- // ToDo: wait for method to read uint option values
- totalTransfereSize = 0;
+ uint64_t value;
+ SuccessOrExit(error = otCoapOptionIteratorGetOptionUintValue(&iterator, &value));
+ totalTransfereSize = (uint32_t)(value & 0xFFFFFFFF);
break;
default:
break;
}
- SuccessOrExit(error = iterator.Advance());
+ IgnoreReturnValue(iterator.Advance());
}
curUriPath[0] = '\0';
Finally, the api does not provide the user with enough information to properly create cache keys (perhaps the real issue here). I opted to generate cache-keys similar to libcoap. The keys allow correlation of messages based on peer addr/port and cache eligible options.
static const char kBlockCacheSha256KeyString[] = "openthread-coap-blockwise-cache";
static const otCryptoKey kBlockCacheKey = {
.mKey = (const uint8_t*)kBlockCacheSha256KeyString,
.mKeyLength = sizeof(kBlockCacheSha256KeyString),
.mKeyRef = 0
};
static bool IsCacheKey(uint16_t aOptionNumber) {
// https://tools.ietf.org/html/rfc7252#section-5.4.6 Definition of NoCacheKey
if ((aOptionNumber & 0x1e) == 0x1c)
{
return false;
}
// https://tools.ietf.org/html/rfc7641#section-2
if (aOptionNumber == OT_COAP_OPTION_OBSERVE)
{
return false;
}
// Ignoring particular options in the cache per https://tools.ietf.org/html/rfc7959#section-2.10
/// TODO: Cache-ignore options would be better to be assignable by the user
if (aOptionNumber == OT_COAP_OPTION_BLOCK1 ||
aOptionNumber == OT_COAP_OPTION_BLOCK2 ||
aOptionNumber == OT_COAP_OPTION_MAX_AGE ||
aOptionNumber == OT_COAP_OPTION_IF_NONE_MATCH)
{
return false;
}
return true;
}
otError otCoapDeriveCacheKey(const otMessage* aMessage, const otMessageInfo* aInfo, otCryptoSha256Hash* aHashOut)
{
Error error = kErrorNone;
Crypto::HmacSha256 hmac;
Coap::Option::Iterator iterator;
hmac.Start(static_cast<const Crypto::Key &>(kBlockCacheKey));
/// Include messageInfo peer data in the hash
hmac.Update((const void*)aInfo->mPeerAddr.mFields.m8, sizeof(aInfo->mPeerAddr.mFields.m8));
hmac.Update((const void*)&aInfo->mPeerPort, sizeof(aInfo->mPeerPort));
/// Include all Cache-Key eligible options in the hash
SuccessOrExit(error = iterator.Init(AsCoapMessage(aMessage)));
while (!iterator.IsDone())
{
uint16_t optionNumber = iterator.GetOption()->GetNumber();
uint16_t optionLength = iterator.GetOption()->GetLength();
if (IsCacheKey(optionNumber))
{
hmac.Update((const void*)&optionNumber, sizeof(optionNumber));
if (optionLength != 0)
{
uint8_t buf[optionLength];
SuccessOrExit(iterator.ReadOptionValue((void*)buf));
hmac.Update((const void*)buf, sizeof(optionLength));
}
}
IgnoreReturnValue(iterator.Advance());
}
exit:
if(error == kErrorNone) {
hmac.Finish(*static_cast<Crypto::HmacSha256::Hash*>(aHashOut));
return error;
}
return kErrorParse;
}
I then call the new otCoapDeriveCacheKey before each blockwise RX/TX hook call and pass the cache key up to the user so the cache can be implemented there. I'm not sure if this is the best way to do it but it does work for me. With this approach, otCoapDeriveCacheKey should be added to the api as well to allow the user to generate cache keys for their TX blockwise hook setup.
Just adding a note to confirm that Token is not used by CoAP blockwise transfers for indication of a "session" of multiple block transfers. Rather, it is just used as defined by RFC 7252 so a client may use a token or may use the default token (0 bytes token) in case it doesn't need the token value in the response. The token can be selected individually for each single transaction (CoAP req). It is used to match a received response to the proper (single) request.
The real issue flagged here seems to be that Block1 blockwise transfer isn't working. If that works or not should not depend on the Token values used by the client.
If a packet capture could be attached, that would be helpful in the analysis.
