[Follow-Up] Updated TC_ACL_2_7 and TC_ACL_2_8 python3 test module to include fabric sensitive checks #38763
Conversation
This file contains hidden or 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
- Updated to conform these 2 test modules to the follow-up task here: project-chip/matter-test-scripts#549 - Added fabric_filtered/fabricFiltered args to test steps 7-10 for TC_ACL_2_7 and TC_ACL_2_8 to include these checks to make sure that with fabric sensitive reads that the writes dont pass over to other fabrics data
|
PR #38763: Size comparison from 4fbe950 to 9286dea Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #38763: Size comparison from 8386c0d to 9475ddd Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
cecille
reviewed
May 20, 2025
| asserts.assert_equal(found_valid_events, expected_events_count, | ||
| f"Expected {expected_events_count} valid events for TH1, found {found_valid_events}") | ||
|
|
||
| if is_filtered: |
There was a problem hiding this comment.
Are you getting back events for TH2 when you don't use fabric filtering? These are fabric sensitive, so they shouldn't be emitted either way....
There was a problem hiding this comment.
Hi Cecille,
So yes when not using fabric filtered we get back results for TH1 when filtering with TH2, it appears that the data section is just obscured or filtered out for the extensions attribute, below is an example from test step 8 output for TH2 with fabric filtered = false:
[AccessControl.Structs.AccessControlExtensionStruct(data=b'', fabricIndex=1), AccessControl.Structs.AccessControlExtensionStruct(data=b'\x17\xd0\x00\x00\xf1\xff\x01\x00=Hello World. This is a single element living as a charstring\x00\x18', fabricIndex=2)]
There was a problem hiding this comment.
WOW. OK...uh...do ou have any idea whether that's coming over the wire or substituted by the client?
There was a problem hiding this comment.
Sorry, I see the issue here now, the above was from running TC_ACL_2_7 test module test step 8:
TH2 read result (fabricFiltered=False): [AccessControl.Structs.AccessControlExtensionStruct(data=b'', fabricIndex=1), AccessControl.Structs.AccessControlExtensionStruct(data=b'\x17\xd0\x00\x00\xf1\xff\x01\x00=Hello World. This is a single element living as a charstring\x00\x18', fabricIndex=2)]
The result we get from ACL_2_8 test step 8 when TH2 reads ACL result (fabric_filtered=True): [AccessControl.Structs.AccessControlEntryStruct(privilege=<AccessControlEntryPrivilegeEnum.kAdminister: 5>, authMode=<AccessControlEntryAuthModeEnum.kCase: 2>, subjects=[2, 2222], targets=Null, fabricIndex=2)]
However, we get the following during ACL_2_8 test step 8 when TH2 reads ACL result (fabric_filtered=False): [AccessControl.Structs.AccessControlEntryStruct(privilege=0, authMode=0, subjects=Null, targets=Null, fabricIndex=1), AccessControl.Structs.AccessControlEntryStruct(privilege=<AccessControlEntryPrivilegeEnum.kAdminister: 5>, authMode=<AccessControlEntryAuthModeEnum.kCase: 2>, subjects=[2, 2222], targets=Null, fabricIndex=2)]
There was a problem hiding this comment.
Additionally, regarding the data output b'\x17\xd0\x00\x00\xf1\xff\x01\x00=Hello World. This is a single element living as a charstring\x00\x18' this as mentioned above came from ACL_2_7 test step 8, I apologize for having provided the incorrect data to this comment previously. However, I investigated this further and discussed it with Amine to verify what's happening here just to make certain that this behavior is correct and expected. The data is coming over the wire as it should be; it's not being substituted or modified by the client. Here's a breakdown of what is occurring:
The D_OK_SINGLE constant represents valid TLV (Tag-Length-Value) encoded data that containing:
- TLV structure bytes: \x17\xd0\x00\x00\xf1\xff\x01\x00=
- An embedded character string element: Hello World. This is a single element living as a charstring
- TLV termination bytes: \x00\x18
When this TLV-encoded payload is transmitted and received, we see both the binary TLV structure and the embedded string content in its human-readable form. This is the standard behavior for TLV data that contains string elements - the string content remains visible within the binary representation.
The ACL_2_7 test step 8 is functioning correctly, and this output confirms that the TLV encoding/decoding is working as designed.
There was a problem hiding this comment.
As discussed in Slack after you have reviewed the trace log output from ACL_2_7 python3 test module test run for test steps 7 and 8, added the additional checks to make sure that data was empty for other fabrics in unfiltered response:
- Added check to make sure that TH1 is empty bytespan in response if TH2 is reading extensions unfiltered
- Added check to make sure that TH2 has empty bytespan in response if TH1 is reading extension data unfiltered
There was a problem hiding this comment.
OK - just to double check - 'cause I want to be sure before i start a fire...
You're getting fabric sensitive events back for the non-accessing fabric, but there's no data inside. But you're still getting the events.
This still feels like an implementation bug that I'd like to chase down. Because it's still leaking information about the other fabric's ACL management.
There was a problem hiding this comment.
Hi Cecille,
That is correct, and I agree that even the presence of an event for another fabric (even with empty data) could be considered an information leak.
Current Behavior:
When reading with fabricFiltered=False, the implementation returns events for both the accessing fabric and the other fabric, but the event data for the other fabric is empty.
For example, during the ACL_2_8 test step 8, when TH2 reads the ACL result with fabric_filtered=False, the result is:
[AccessControl.Structs.AccessControlEntryStruct(privilege=0, authMode=0, subjects=Null, targets=Null, fabricIndex=1), AccessControl.Structs.AccessControlEntryStruct(privilege=<AccessControlEntryPrivilegeEnum.kAdminister: 5>, authMode=<AccessControlEntryAuthModeEnum.kCase: 2>, subjects=[2, 2222], targets=Null, fabricIndex=2)]
cecille
reviewed
May 20, 2025
src/python_testing/TC_ACL_2_7.py
Outdated
| fabric_filtered=False | ||
| ) | ||
| logging.info("TH2 read result (fabricFiltered=False): %s", str(result2_unfiltered)) | ||
| asserts.assert_equal(len(result2_unfiltered), 2, "Should have two extensions when not fabric filtered") |
There was a problem hiding this comment.
two things
- you don't know for sure that there's no other fabric on the DUT, so this might not actually be two. There should be at least two
- The important check here isn't so much about non-fabric filtered reads returning data. The data should be returned, yes, but the fabric sensitive fields in the struct aren't supposed to be readable from other fabrics. So if you read here with fabric_filtered = False, you should get back an entry for TH2, but it shouldn't tell you anything interesting. I don't love the spec wording here - it's not nearly specific enough. The subject and target are nullable, but I'd actually be quite interested to know what happens with the privilege and auth mode fields. Actually, what ARE you getting back here for the
There was a problem hiding this comment.
Hi Cecille,
Understood, I have updated this part for the 1st point to:
- asserts.assert_true(len(result2_unfiltered) >= 2, "Should have two extensions when not fabric filtered")
Additionally, I have provided the output from this test step here which shows acec output with fabric unfiltered for TH2 from ACL_2_7 test step 8:
[AccessControl.Structs.AccessControlExtensionStruct(data=b'', fabricIndex=1), AccessControl.Structs.AccessControlExtensionStruct(data=b'\x17\xd0\x00\x00\xf1\xff\x01\x00=Hello World. This is a single element living as a charstring\x00\x18', fabricIndex=2)]
However, I think this might be a bit more interesting regarding what you had mentioned for point 2 and auth fabric filtering received when test ACL_2_8, when we read the AccessControlList attribute with TH1 and fabric filtered = False: [AccessControl.Structs.AccessControlEntryStruct(privilege=<AccessControlEntryPrivilegeEnum.kAdminister: 5>, authMode=<AccessControlEntryAuthModeEnum.kCase: 2>, subjects=[112233, 1111], targets=Null, fabricIndex=1), AccessControl.Structs.AccessControlEntryStruct(privilege=0, authMode=0, subjects=Null, targets=Null, fabricIndex=2)]
Then If we look and compare to the result of the AccessControlList attribute for TH1 with fabric filtered = True:
[AccessControl.Structs.AccessControlEntryStruct(privilege=<AccessControlEntryPrivilegeEnum.kAdminister: 5>, authMode=<AccessControlEntryAuthModeEnum.kCase: 2>, subjects=[112233, 1111], targets=Null, fabricIndex=1)]
There was a problem hiding this comment.
As discussed in Slack after you have reviewed the trace log output from ACL_2_7 python3 test module test run for test steps 7 and 8, added the additional checks to make sure that data was empty for other fabrics in unfiltered response:
- Added check to make sure that TH1 is empty binary data in response if TH2 is reading extensions unfiltered
- Added check to make sure that TH2 has empty binary data in response if TH1 is reading extension data unfiltered
- changed from equal to a greater than value to verify that we got more than 2 fabrics results returned during test step 8
- Adding check to make sure that TH1 is empty binary data in response if TH2 is reading extensions unfiltered - Added check to make sure that TH2 has empty binary data in response if TH1 is reading extension data unfiltered
|
PR #38763: Size comparison from c198b25 to 4b86808 Increases above 0.2%:
Full report (70 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, stm32, telink, tizen)
|
cecille
reviewed
Jun 17, 2025
| ) | ||
| logging.info("TH1 read ACL result (fabric_filtered=False): %s", str(acl_list_unfiltered)) | ||
| asserts.assert_equal(len(acl_list_unfiltered), 2, "Should have two ACL entries when not fabric filtered") | ||
|
|
There was a problem hiding this comment.
Can you please add a check here that the data for the non-accessing fabric is empty. You should get back a response, but it won't have any fields, and the client will fill in the defaults, which is empty everything.
Please also add a comment as to why you're checking for empty everything.
There was a problem hiding this comment.
and wherever else this happens.
There was a problem hiding this comment.
Thanks, Cecille!
I’ll add explicit checks to ensure that the data for the non-accessing fabric is empty (i.e., all fields are defaults/empty), and I’ll include a comment explaining why we’re checking for empty values.
I’ll make sure to do this wherever else it happens in the test as well.
| asserts.assert_equal(found_valid_events, expected_events_count, | ||
| f"Expected {expected_events_count} valid events for TH1, found {found_valid_events}") | ||
|
|
||
| if is_filtered: |
There was a problem hiding this comment.
OK - just to double check - 'cause I want to be sure before i start a fire...
You're getting fabric sensitive events back for the non-accessing fabric, but there's no data inside. But you're still getting the events.
This still feels like an implementation bug that I'd like to chase down. Because it's still leaking information about the other fabric's ACL management.
- Adding asserts to verify that non-accessing fabric entry is empty or values are defaults to validate no data leak
- Updating events var for self._get_relevant_acl_events() from events to events_filtered due to new fabric filtered versus unfiltered logic
|
PR #38763: Size comparison from 91928f0 to 6a3100f Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
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.
Testing
Updated TC_ACL_2_7 and TC_ACL_2_8 python3 test modules: