[CASESession] refactoring and improving testability of Sigma1 sending and handling, and Sigma2 Sending #36679
+818
−370
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review changes with |
Alami-Amine
force-pushed
the
AA/CASERefactorSigma1
branch
from
3b59b16 to
13a8f48
Compare
|
PR #36679: Size comparison from d37eae1 to 13a8f48 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #36679: Size comparison from d37eae1 to 224a626 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Alami-Amine
force-pushed
the
AA/CASERefactorSigma1
branch
from
cf3ac6a to
a2624fa
Compare
|
PR #36679: Size comparison from 5d42d92 to a2624fa Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Alami-Amine
force-pushed
the
AA/CASERefactorSigma1
branch
2 times, most recently
from
a70c7c5 to
673c53b
Compare
|
PR #36679: Size comparison from 41a9dea to 673c53b Full report (11 builds for cc13x4_26x4, cc32xx, qpg, stm32, tizen)
|
Alami-Amine
force-pushed
the
AA/CASERefactorSigma1
branch
from
673c53b to
7d0b4a6
Compare
|
PR #36679: Size comparison from 41a9dea to 7d0b4a6 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Alami-Amine
force-pushed
the
AA/CASERefactorSigma1
branch
from
0013419 to
6c13347
Compare
|
PR #36679: Size comparison from 75ab4c9 to 6c13347 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Alami-Amine
changed the title
Comment on lines
+973
to
+974
| // TODO should I keep this? | ||
| case Step::kSendStatusReport: |
There was a problem hiding this comment.
Yes. If kSendStatusReport is the outcome from Sigma1, it means there is a status report to send, and we need to send it:
// FindLocalNodeFromDestinationId returns CHIP_ERROR_KEY_NOT_FOUND if validation of DestinationID fails, which will trigger
// status Report with ProtocolCode = NoSharedTrustRoots.
mNextStep = Step::kSendStatusReport;
So I recommend moving the following block in kSendStatusReport case:
if (err == CHIP_ERROR_KEY_NOT_FOUND)
{
SendStatusReport(mExchangeCtxt, kProtocolCodeNoSharedRoot);
mState = State::kInitialized;
}
There was a problem hiding this comment.
Actually I made the case Step::kSendStatusReport automatically redirect us to exit through ExitNow() and kept the SendStatusReport with NoSharedTrustRoots in exit because:
-
Technically, it is possible to get a
CHIP_ERROR_KEY_NOT_FOUNDin other Code Paths than the one you quoted (where I defined the NextStep); such as through calls toSignWithOpKeypair, called inPrepareSigma2(). -
I don't know if getting to that codepath is impossible or not, so I thought I'll just keep that
SendStatusReportinexitand redirect thatcaseto exit (which does make theNextStepkind of useless. -
What do you think?
Comment on lines
+1142
to
+1143
| // FindLocalNodeFromDestinationId returns CHIP_ERROR_KEY_NOT_FOUND if validation of DestinationID fails, which will trigger | ||
| // status Report with ProtocolCode = NoSharedTrustRoots. |
There was a problem hiding this comment.
Is this always true? We need to make sure that SendStatusReport always at least returns invalid parameters where needed (or nothing at all in some cases). This comment indicates something brittle about the implementation, which means we should try to ensure the error is CHIP_ERROR_KEY_NOT_FOUND if FindLocalNodeFromDestinationId failed, but perhaps a log of error be done to help diagnose at the DUT.
There was a problem hiding this comment.
- Actually my comment isn't correct, we can get other return values such as
CHIP_ERROR_INCORRECT_STATE. - are you suggesting that we add a
ChipLogErrorwith some more parameters to be added insideFindLocalNodeFromDestinationIdwhenfound == false, or do you want to modify theSendStatusReportitself to communicate those?
There was a problem hiding this comment.
ChipLogError with some more parameters to be added inside FindLocalNodeFromDestinationId when found == false
That
| VerifyOrReturnError(mLocalMRPConfig.HasValue(), CHIP_ERROR_INCORRECT_STATE); | ||
| output.responderMrpConfig = &mLocalMRPConfig.Value(); | ||
|
|
||
| mState = State::kSentSigma2Resume; |
There was a problem hiding this comment.
It's not sent yet. Perhaps kSendSigma2Resume?
There was a problem hiding this comment.
This is a mistake! will remove it (it is already present inside SendSigma2Resume)
| // Check if length of msg_R2_Encrypted is set and is at least larger than the MIC length | ||
| VerifyOrReturnError(input.encrypted2Length > CHIP_CRYPTO_AEAD_MIC_LENGTH_BYTES, CHIP_ERROR_INCORRECT_STATE); | ||
|
|
||
| ReturnErrorOnFailure(tlvWriterMsg2.PutBytes(TLV::ContextTag(ToRaw(Sigma2Tags::kEncrypted2)), input.msg_R2_Encrypted.Get(), |
There was a problem hiding this comment.
After this PutBytes, consider freeing the input.msg_R2_Encrypted message buffer. How is it released right now, otherwise?
There was a problem hiding this comment.
It will be freed once we EncodeSigma2Inputs goes out of scope and we exit HandleSigma1_and_SendSigma2, wouldn't it? since it is a ScopedMemoryBuffer
There was a problem hiding this comment.
Yes, it will be.
I want implying that perhaps it should be scoped shorter.
| msg_r2_signed_enc_len = static_cast<size_t>(tlvWriter.GetLengthWritten()); | ||
|
|
||
| msg_r2_signed_enc_len = static_cast<size_t>(tlvWriter.GetLengthWritten()); | ||
| output.encrypted2Length = msg_r2_signed_enc_len + CHIP_CRYPTO_AEAD_MIC_LENGTH_BYTES; |
There was a problem hiding this comment.
It's too bad the data type of the encrypted message doesn't maintain its length on its own and it has to be ridden-along here.
There was a problem hiding this comment.
I thought of using ScopedMemoryBufferWithSize for msg_R2_Encrypted but it gives the actual allocated size and there is no support for resizing, and using the LengthWritten by the TLVWriter. Should I add resize function to ScopedMemoryBufferWithSize ?
tcarmelveilleux
approved these changes
Dec 16, 2024
|
PR #36679: Size comparison from 1c92162 to 3060675 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
bzbarsky-apple
requested changes
Dec 18, 2024
| @@ -211,6 +190,116 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler, | |||
| // If this function returns true, the CASE session has been reset and is ready for a new session establishment. | |||
| bool InvokeBackgroundWorkWatchdog(); | |||
|
|
|||
| protected: | |||
| // Helper Enum for usage in HandleSigma1_and_SendSigma2 | |||
There was a problem hiding this comment.
Suggested change
| // Helper Enum for usage in HandleSigma1_and_SendSigma2 | |
| // Helper Enum for use in HandleSigma1_and_SendSigma2 |
| kSendStatusReport | ||
| }; | ||
|
|
||
| Step mNextStep = Step::kNone; |
There was a problem hiding this comment.
Why does this need to be a member? Seems like we could have a Step * outparam on HandleSigma1, since this value is just computed in that function, used immediately in its caller, and does not need to be persisted past that point....
| ByteSpan destinationId; | ||
| bool sessionResumptionRequested = false; | ||
| ByteSpan resumptionId; | ||
| ByteSpan initiatorResumeMICSpan; |
There was a problem hiding this comment.
Why does this field's name have a "Span" suffix? None of the other spans here do....
|
|
||
| Step mNextStep = Step::kNone; | ||
|
|
||
| // This struct only serves as a base struct for EncodeSigma1 and ParseSigma1 |
There was a problem hiding this comment.
Suggested change
| // This struct only serves as a base struct for EncodeSigma1 and ParseSigma1 | |
| // This struct only serves as a base struct for EncodedSigma1Inputs and ParsedSigma1 |
|
|
||
| struct EncodeSigma1Inputs : Sigma1Param | ||
| { | ||
| const Crypto::P256PublicKey * pEphPubKey = nullptr; |
There was a problem hiding this comment.
Suggested change
| const Crypto::P256PublicKey * pEphPubKey = nullptr; | |
| const Crypto::P256PublicKey * ephPubKey = nullptr; |
Please follow local naming style.
| encodeParams.responderSessionId = 7315; | ||
|
|
||
| // Generate Ephemeral Public Key | ||
| Crypto::P256Keypair * EphemeralKey = gDeviceOperationalKeystore.AllocateEphemeralKeypairForCASE(); |
There was a problem hiding this comment.
Suggested change
| Crypto::P256Keypair * EphemeralKey = gDeviceOperationalKeystore.AllocateEphemeralKeypairForCASE(); | |
| Crypto::P256Keypair * ephemeralKey = gDeviceOperationalKeystore.AllocateEphemeralKeypairForCASE(); |
|
|
||
| { | ||
| System::PacketBufferHandle msg; | ||
| EXPECT_EQ(CHIP_NO_ERROR, EncodeSigma2(msg, encodeParams)); |
There was a problem hiding this comment.
Again, if EncodeSigma2 does not touch state it should be static, and if it does this needs to be done on a clean CASESession?
|
|
||
| { | ||
| System::PacketBufferHandle msg; | ||
| EXPECT_EQ(CHIP_NO_ERROR, EncodeSigma2Resume(msg, encodeParams)); |
There was a problem hiding this comment.
Again: static if it does not use member state, use a clear CASESession for each call if it does.
| encodeParams.responderSessionId = 7315; | ||
|
|
||
| // responder Session Parameters | ||
| ReliableMessageProtocolConfig MRPConfig = GetDefaultMRPConfig(); |
There was a problem hiding this comment.
Suggested change
| ReliableMessageProtocolConfig MRPConfig = GetDefaultMRPConfig(); | |
| ReliableMessageProtocolConfig mrpConfig = GetDefaultMRPConfig(); |
| encodeParams.msgR2Encrypted.Alloc(encodeParams.encrypted2Length); | ||
|
|
||
| // responder Session Parameters | ||
| ReliableMessageProtocolConfig MRPConfig = GetDefaultMRPConfig(); |
There was a problem hiding this comment.
Suggested change
| ReliableMessageProtocolConfig MRPConfig = GetDefaultMRPConfig(); | |
| ReliableMessageProtocolConfig mrpConfig = GetDefaultMRPConfig(); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes in the PR
Sending Sigma1
EncodeSigma1()method; removing the bits from Networking and Messaging layers --> that way it could be tested without needingSessionManagerandExchangeContext.HandleSigma1_and_SendSigma2HandleSigma1_and_SendSigma2:previously, this method was primarily a wrapper to
HandleSigma1(); which in turn handled with all the validation and parsing of Sigma1 as well as the generation, and logic involved in sending Sigma2 or Sigma2Resume...in a very sequential manner.Fix: This was changed to be more modular, by factoring out the
Sending Sigma2calls out ofHandleSigma1(), and encapsulating it in a switch-case statement, and then breaking downSigma2 Sendingmethods into two more methods: one for generating and preparing the Logic for the Sigma2, and one for Encoding it into TLV-format.ParseSigma1(): made the output in the form of a struct, and decoupling it from exchange context, which was moved toHandleSigma1()EncodeSigma2()andEncodeSigma2Resume(): created and refactored into them the TLV encoding for the final Sigma2/Sigma2Resume messages that will be sent.PrepareSigma2()andPrepareSigma2Resume(): refactored into them all the steps that happen before Encoding the Sigma2/Sigma2Resume messages.Testing