Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Annotation of access control metadata to two clusters #11542

Closed
wants to merge 2 commits into from
Closed

Annotation of access control metadata to two clusters #11542

wants to merge 2 commits into from

Conversation

hubTab
Copy link
Contributor

@hubTab hubTab commented Nov 8, 2021 •
edited by tcarmelveilleux
Loading

Problem

Started annotating the XML cluster files with Access Control XML elements.
Normalized Normalized fabric_idx usage across the two updated cluster xml files.

Issue #11204
Issue #11286
Issue #10246

Change overview

Annotated the operational-credentials-cluster.xml and onoff-cluster.xml based on the Matter Specs, section 11.18, and appclusters, respectively.

  • Normalized the elements. Note that the writeable attribute indicates if a field is writeable, or not. The elements qualifies provileges on a per operation basis:

    • updated all elements to include fieldId;
    • updated NodeOperationalCertStatus to include missing item elements;
    • updated NOCStruct to include a missing item element;

  • Normalized fabric_idx usage across the two updated cluster xml files.

Testing

How was this tested? (at least one bullet point required)

  • Not applicable, as it is a draft yet.

https://github.com/project-chip/connectedhomeip/issues/10246
7b25c6f 
project-chip#11204
project-chip#11286

Annotated the operational-credentials-cluster.xml and onoff-cluster.xml based on the Matter Specs, section 11.18, and appclusters, respectively.
Normalized the <access> elements. Note that the writeable <attribute> attribute indicates if a field is writeable, or not. The <access> elements qualifies provileges on a per operation basis.
    updated all <struct> elements to include <item> fieldId;
    updated NodeOperationalCertStatus to include missing item elements;
    updated NOCStruct to include a missing item element;
Normalized fabric_idx usage across the two updated cluster xml files.
@boring-cyborg boring-cyborg bot added the app label Nov 8, 2021
tcarmelveilleux
tcarmelveilleux reviewed Nov 12, 2021
View reviewed changes
src/app/zap-templates/zcl/data-model/chip/operational-credentials-cluster.xml
<item name="FabricIndex" type="INT8U"/>
<item name="NOC" type="OCTET_STRING"/>
<item fieldId="0" name="FabricIndex" type="fabric_idx"/>
<item fieldId="1" name="NOC" type="OCTET_STRING" length="400" isFabricSensitive="true"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is isFabricSensitive an alias for the equivalent of an access tag?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was suggested on Nov 1st: https://csamembers.slack.com/archives/C02FTN42KEE/p1635797498010800?thread_ts=1635795804.009300&cid=C02FTN42KEE

tcarmelveilleux
tcarmelveilleux reviewed Nov 12, 2021
View reviewed changes
src/app/zap-templates/zcl/data-model/chip/operational-credentials-cluster.xml
Comment on lines 78 to +79
<!-- 400 = 400 bytes for root cert -->
<attribute side="server" code="0x0004" define="TRUSTED_ROOTS" type="ARRAY" entryType="OCTET_STRING" length="400" writable="false" optional="false">TrustedRootCertificates</attribute>
<attribute side="server" code="0x0005" define="CURRENT_FABRIC_INDEX" type="fabric_idx" writable="false" optional="false">CurrentFabricIndex</attribute>
<attribute side="server" code="0x0004" define="TRUSTED_ROOTS" type="ARRAY" entryType="OCTET_STRING" length="400" writable="false" optional="false">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 400 length for an array here doesn't make sense

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the topic that needs group resolution :)

tcarmelveilleux
tcarmelveilleux reviewed Nov 12, 2021
View reviewed changes
src/app/zap-templates/zcl/data-model/chip/operational-credentials-cluster.xml
</command>

<command source="server" code="0x01" name="AttestationResponse" optional="false">
<description>An attestation information confirmation from the server.</description>
<arg name="AttestationElements" type="OCTET_STRING"/>
<arg name="Signature" type="OCTET_STRING"/>
<access op="invoke" privilege="administer"/>
</command>

<command source="client" code="0x02" name="CertificateChainRequest" optional="false">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add response="CertificateChainResponse"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did the comment refer to the already existing response?

    <command source="server" code="0x03" name="CertificateChainResponse" optional="false">
      <description>A device attestation certificate (DAC) or product attestation intermediate (PAI) certificate from the server.</description>
      <arg name="Certificate" type="OCTET_STRING"/>
    </command>

tcarmelveilleux
tcarmelveilleux reviewed Nov 12, 2021
View reviewed changes
src/app/zap-templates/zcl/data-model/chip/operational-credentials-cluster.xml
Comment on lines 146 to 148
<arg name="StatusCode" type="INT8U"/>
<arg name="FabricIndex" type="INT8U"/>
<arg name="FabricIndex" type="fabric_idx"/>
<arg name="DebugText" type="CHAR_STRING"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need the field IDs for these args

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the cluster xml files include a fieldId in the attributes. To make sure I understand the requirement, is this format correct?

    <command source="server" code="0x08" name="NOCResponse" optional="false">
      <description>Response to AddNOC or UpdateNOC commands.</description>
      <arg fieldId="0" name="StatusCode" type="INT8U"/>
      <arg fieldId="1" name="FabricIndex" type="fabric_idx"/>
      <arg fieldId="2" name="DebugText" type="CHAR_STRING"/>
    </command>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last commit made the above assumption. If incorrect, we can rollback the changes and work on determining the right format.

Removed multiple op invoke privileges, as there can only be one privi…
09376a8 
…lege. Removed <access> for response commands, as there are no privileges for command responses.
@tcarmelveilleux tcarmelveilleux changed the title https://github.com/project-chip/connectedhomeip/issues/10246 Annotation of access control metadata to two clusters Nov 15, 2021
@stale
Copy link

stale bot commented Nov 22, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale Stale issue or PR label Nov 22, 2021
@stale
Copy link

stale bot commented Nov 29, 2021

This stale pull request has been automatically closed. Thank you for your contributions.

@stale stale bot closed this Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tcarmelveilleux tcarmelveilleux tcarmelveilleux left review comments

Assignees
No one assigned
Labels
app stale Stale issue or PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hubTab @tcarmelveilleux