Chip tool operational credentials cluster fix #6873

vivien-apple · 2021-05-17T12:51:12Z

Problem

chip-tool can not pair anymore since the Operational Credentials cluster has landed because the response to the AddOptCert command is not enabled.
In order to enable it, it requires a few fixes, which is what this PR is about.

Summary of Changes

Enable the AddOptCert command/response
Do the necessary changes to the templates
Update gen/ folders.

todo · 2021-05-17T12:51:14Z

(#5098) We should pass the Command Object and EndpointId to the cluster callbacks.

connectedhomeip/examples/chip-tool/gen/IMClusterCommandHandler.cpp

Lines 4119 to 4129 in 2d0e18f

    
                   // TODO(#5098) We should pass the Command Object and EndpointId to the cluster callbacks. 
        
                   wasHandled = emberAfOperationalCredentialsClusterOpCSRResponseCallback(apCommandObj, CSR, CSRNonce, VendorReserved1, 
        
                                                                                          VendorReserved2, VendorReserved3, Signature); 
        
               } 
        
               break; 
        
           } 
        
           case ZCL_OP_CERT_RESPONSE_COMMAND_ID: { 
        
               expectArgumentCount = 3; 
        
               uint8_t StatusCode; 
        
               uint64_t FabricIndex; 
        
               const uint8_t * DebugText;

This comment was generated by todo based on a `TODO` comment in `2d0e18f` in #6873. cc @vivien-apple.

todo · 2021-05-17T12:51:17Z

(#5542): The cluster handlers should accept a ByteSpan for all string types.

connectedhomeip/examples/chip-tool/gen/IMClusterCommandHandler.cpp

Lines 4166 to 4176 in 2d0e18f

    
               // TODO(#5542): The cluster handlers should accept a ByteSpan for all string types. 
        
               TLVUnpackError = aDataTlv.GetDataPtr(DebugText); 
        
               break; 
        
           default: 
        
               // Unsupported tag, ignore it. 
        
               ChipLogProgress(Zcl, "Unknown TLV tag during processing."); 
        
               break; 
        
           } 
        
           if (CHIP_NO_ERROR != TLVUnpackError) 
        
           { 
        
               break;

This comment was generated by todo based on a `TODO` comment in `2d0e18f` in #6873. cc @vivien-apple.

todo · 2021-05-17T12:51:20Z

(#5098) We should pass the Command Object and EndpointId to the cluster callbacks.

connectedhomeip/examples/chip-tool/gen/IMClusterCommandHandler.cpp

Lines 4188 to 4196 in 2d0e18f

    
                   // TODO(#5098) We should pass the Command Object and EndpointId to the cluster callbacks. 
        
                   wasHandled = emberAfOperationalCredentialsClusterOpCertResponseCallback(apCommandObj, StatusCode, FabricIndex, 
        
                                                                                           const_cast<uint8_t *>(DebugText)); 
        
               } 
        
               break; 
        
           } 
        
           case ZCL_SET_FABRIC_RESPONSE_COMMAND_ID: { 
        
               expectArgumentCount = 1; 
        
               chip::FabricId FabricId;

This comment was generated by todo based on a `TODO` comment in `2d0e18f` in #6873. cc @vivien-apple.

tcarmelveilleux · 2021-05-17T12:59:44Z

examples/chip-tool/commands/clusters/Commands.h

+| * AddOpCert                                                         |   0x06 |
+| * OpCSRRequest                                                      |   0x04 |


It's odd that those are not in order. Not for this PR to fix, but it's odd.

tcarmelveilleux · 2021-05-17T13:00:32Z

examples/chip-tool/commands/clusters/Commands.h

+    OperationalCredentialsAddOpCert() : ModelCommand("add-op-cert")
+    {
+        AddArgument("noc", &mNoc);
+        AddArgument("iCACertificate", &mICACertificate);


iCACertificate is very odd casig. Suggest ICACertificate.

Will provide a fix in a followup. This is because the template is doing {{asCamelCased name}} omitting the second argument of https://github.com/project-chip/zap/blob/7c928747ef6c32671d39901c881b82f57a6e1c08/src-electron/generator/helper-c.js#L200 which force the first letter to be lower cased.

tcarmelveilleux · 2021-05-17T13:02:34Z

examples/chip-tool/commands/clusters/Commands.h

+
+        chip::Controller::OperationalCredentialsCluster cluster;
+        cluster.Associate(device, endpointId);
+        return cluster.AddOpCert(


Add TODO about needing to send the NOC and ICA certs in a CHIP certificate array.

tcarmelveilleux · 2021-05-17T13:03:12Z

examples/chip-tool/commands/clusters/Commands.h

+public:
+    OperationalCredentialsOpCSRRequest() : ModelCommand("op-csrrequest")
+    {
+        AddArgument("cSRNonce", &mCSRNonce);


Rename to CSRNonce.

examples/chip-tool/commands/clusters/Commands.h

tcarmelveilleux · 2021-05-17T13:11:05Z

examples/chip-tool/gen/CHIPClientCallbacks.cpp

+    ChipLogProgress(Zcl, "  CSR: %s", CSR);
+    ChipLogProgress(Zcl, "  CSRNonce: %s", CSRNonce);
+    ChipLogProgress(Zcl, "  VendorReserved1: %s", VendorReserved1);
+    ChipLogProgress(Zcl, "  VendorReserved2: %s", VendorReserved2);
+    ChipLogProgress(Zcl, "  VendorReserved3: %s", VendorReserved3);
+    ChipLogProgress(Zcl, "  Signature: %s", Signature);


None of these are human readable strings. Is there a reason they are printed as strings here? How does a chip::ByteSpan print? I could not find the path used to convert it to a const char*

It has mostly been used for debugging. I will remove that in a followup, likely just printing the length of the chip::ByteSpan for OCTET_STRING, while still printing the content for CHAR_STRING (which are chip::ByteSpan under the hood).

So what will happen here today, though?

I did replace the code that uses %s in order to just prints the size and removes a few other places where chip::Bytespan was printed badly.

As discussed I will use

connectedhomeip/src/lib/support/BytesToHex.h

Line 73 in 4f0ade5

CHIP_ERROR BytesToHex(const uint8_t * src_bytes, size_t src_size, char * dest_hex, size_t dest_size_max, BitFlags<HexFlags> flags);

in a followup in order to facilitate debugging. I was originally hoping to do it into this PR but I got distracted by a few other things and since chip-tool is broken without this patch I guess it is more urgent to repair it...

tcarmelveilleux · 2021-05-17T13:14:23Z

examples/chip-tool/commands/clusters/Commands.h

+        chip::Controller::OperationalCredentialsCluster cluster;
+        cluster.Associate(device, endpointId);
+        return cluster.OpCSRRequest(onSuccessCallback->Cancel(), onFailureCallback->Cancel(),
+                                    chip::ByteSpan(chip::Uint8::from_char(mCSRNonce), strlen(mCSRNonce)));


This will crash: mCSRNonce is not a string, it's a byte string. strlen should not be called on any CSR fields.

chip-tool lacks support of real OCTET_STRING.
Ideally it should take a string as input specified as hex byte and convert it before sending. At the moment, everything is a string passed from the command line. So strlen is called on the content of the command line argument. It will likely not crash here, since mCSRNonce is a char *, but it will probably not work as you may expect.

TODO comment then? or will this stop compiling once we support octed_string and will get fixed as a result?

lazarkov · 2021-05-17T16:04:30Z

@andreilitvin can it be merged as hotfix?

tcarmelveilleux

Approved, conditional on not logging any ByteSpan

woody-apple · 2021-05-17T18:28:27Z

@msandstedt @saurabhst ?

woody-apple · 2021-05-17T18:29:25Z

@andreilitvin can it be merged as hotfix?

@cjandhyala or @vivien-apple how has this been tested?

@vivien-apple are there any tests we can add to make sure this doesn't regress here? Thanks

tcarmelveilleux · 2021-05-17T19:37:53Z

examples/chip-tool/templates/commands.zapt

        {{/chip_attribute_list_entryTypes}}
        {{else}}
+        {{#if (isOctetString type)}}
+        ChipLogProgress(Zcl, "  {{asSymbol label}}: %zu", entries[i].size());


%zu is not available on all embedded platforms.

In a future PR, suggest:

Suggest having the code eventually static cast size() to uint32_t in the log call and use PRIu32 for the specifier.

andy31415 · 2021-05-17T19:54:54Z

@msandstedt @saurabhst @Damian-Nordic @jepenven-silabs ?

woody-apple · 2021-05-17T23:49:51Z

/rebase

…als cluster

vivien-apple self-assigned this May 17, 2021

boring-cyborg bot added app darwin examples labels May 17, 2021

pullapprove bot requested review from andy31415, bzbarsky-apple, chrisdecenzo, Damian-Nordic, hawk248, jepenven-silabs, msandstedt and shana-apple May 17, 2021 12:55

tcarmelveilleux reviewed May 17, 2021

View reviewed changes

examples/chip-tool/commands/clusters/Commands.h Show resolved Hide resolved

tcarmelveilleux reviewed May 17, 2021

View reviewed changes

vivien-apple force-pushed the ChipTool_OperationalCredentialsClusterFix branch from 2d0e18f to f65cad4 Compare May 17, 2021 13:49

andy31415 approved these changes May 17, 2021

View reviewed changes

woody-apple approved these changes May 17, 2021

View reviewed changes

tcarmelveilleux approved these changes May 17, 2021

View reviewed changes

vivien-apple force-pushed the ChipTool_OperationalCredentialsClusterFix branch from f65cad4 to 4c37838 Compare May 17, 2021 19:26

boring-cyborg bot added the controller label May 17, 2021

tcarmelveilleux reviewed May 17, 2021

View reviewed changes

saurabhst approved these changes May 17, 2021

View reviewed changes

vivien-apple added 2 commits May 18, 2021 00:23

Update templates to get chip-tool working with the operation credenti…

dea5d6e

…als cluster

Update gen/ folders

b038fdd

woody-apple force-pushed the ChipTool_OperationalCredentialsClusterFix branch from 4c37838 to b038fdd Compare May 18, 2021 00:23

pullapprove bot added the review - approved label May 18, 2021

woody-apple merged commit 6687e7d into project-chip:master May 18, 2021

todo bot mentioned this pull request May 18, 2021

(#5098) We should pass the Command Object and EndpointId to the cluster callback... #6912

Closed

vivien-apple mentioned this pull request May 18, 2021

Update the zap template used for chip-tool to not forced the first le… #6930

Merged

gjc13 mentioned this pull request May 26, 2021

[chip-tool] OpCSRResponse parsing failure in chip-tool #6867

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chip tool operational credentials cluster fix #6873

Chip tool operational credentials cluster fix #6873

vivien-apple commented May 17, 2021

todo bot commented May 17, 2021

todo bot commented May 17, 2021

todo bot commented May 17, 2021

tcarmelveilleux May 17, 2021

tcarmelveilleux May 17, 2021

vivien-apple May 17, 2021

tcarmelveilleux May 17, 2021

tcarmelveilleux May 17, 2021

tcarmelveilleux May 17, 2021

vivien-apple May 17, 2021

tcarmelveilleux May 17, 2021 •

edited

Loading

vivien-apple May 17, 2021

tcarmelveilleux May 17, 2021

vivien-apple May 17, 2021

andy31415 May 17, 2021

lazarkov commented May 17, 2021

tcarmelveilleux left a comment

woody-apple commented May 17, 2021

woody-apple commented May 17, 2021

tcarmelveilleux May 17, 2021

andy31415 commented May 17, 2021

woody-apple commented May 17, 2021

Chip tool operational credentials cluster fix #6873

Chip tool operational credentials cluster fix #6873

Conversation

vivien-apple commented May 17, 2021

Problem

Summary of Changes

todo bot commented May 17, 2021

(#5098) We should pass the Command Object and EndpointId to the cluster callbacks.

This comment was generated by todo based on a TODO comment in 2d0e18f in #6873. cc @vivien-apple.

todo bot commented May 17, 2021

(#5542): The cluster handlers should accept a ByteSpan for all string types.

This comment was generated by todo based on a TODO comment in 2d0e18f in #6873. cc @vivien-apple.

todo bot commented May 17, 2021

(#5098) We should pass the Command Object and EndpointId to the cluster callbacks.

This comment was generated by todo based on a TODO comment in 2d0e18f in #6873. cc @vivien-apple.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcarmelveilleux May 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazarkov commented May 17, 2021

tcarmelveilleux left a comment

Choose a reason for hiding this comment

woody-apple commented May 17, 2021

woody-apple commented May 17, 2021

Choose a reason for hiding this comment

andy31415 commented May 17, 2021

woody-apple commented May 17, 2021

This comment was generated by todo based on a `TODO` comment in `2d0e18f` in #6873. cc @vivien-apple.

This comment was generated by todo based on a `TODO` comment in `2d0e18f` in #6873. cc @vivien-apple.

This comment was generated by todo based on a `TODO` comment in `2d0e18f` in #6873. cc @vivien-apple.

tcarmelveilleux May 17, 2021 •

edited

Loading