ACL/Transport: Add structured subject #15418
Closed
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
pullapprove
bot
requested review from
andy31415,
anush-apple,
austinh0,
balducci-apple,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
franck-apple,
gjc13,
hawk248,
harsha-rajendran,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
lazarkov,
LuDuda and
lzgrablic02
pullapprove
bot
requested review from
tecimovic,
turon,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21 and
yunhanw-google
|
PR #15418: Size comparison from cbdecce to c39ca5d Increases (31 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Full report (34 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
andy31415
reviewed
Feb 22, 2022
|
PR #15418: Size comparison from 1fc5eed to 1d75d6b Increases (40 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Full report (43 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
bzbarsky-apple
requested changes
Feb 23, 2022
|
|
||
| struct PaseSubject | ||
| { | ||
| static constexpr uint16_t kPasscodeId = 0; |
There was a problem hiding this comment.
This is not used except in logging, right? Why do we need this at all?
There was a problem hiding this comment.
This field follows chip spec.
Subject ID SHALL be of type uint64 with semantics depending on the entry’s AuthMode as follows:
PASE: Lower 16-bits → Passcode ID Upper 48-bits → all bits clear
And also in spec:
Note that any Passcode ID other than 0, which is the default commissioning passcode, is reserved for future use.
So kPasscodeId is a const of 0 now.
There was a problem hiding this comment.
This does not answer my question. What is this field actually used for? If it's not used for anything, why is it there?
There was a problem hiding this comment.
It is used to construct the SubjectId, which is well defined in the spec.
The SubjectId is not only used for logging, it can be used to fill CaseAdminNode field in AddNOC commands (maybe in the future PR).
Although SubjectId for PASE subject is not being used to encode messages, it doesn't prevent us from building a SubjectId for PASE subject. The GetSubjectId function is fully aligned with spec.
There was a problem hiding this comment.
Used to construct the SubjectId from what?
The SubjectId is not only used for logging, it can be used to fill CaseAdminNode field in AddNOC commands
That value is just a NodeId. It does not need any of the Subject machinery, and I can't see any situation where it would.
The GetSubjectId function is fully aligned with spec.
No, it's just not. But if you think it is, please walk me through step by step exactly how it's aligned.
src/access/Subjects.h
Outdated
Comment on lines
178
to
179
| * OperationalNodeId identifies an individual Node on a Fabric. It is a special type of Subject targeting to NodeSubject. It is | ||
| * interchangeable with the generic Subject type but uses less memory. |
There was a problem hiding this comment.
A few comments:
- This identifies an individual node on a fabric from the point of view of some specific node, not in general.
- "targeting to NodeSubject" does not make sense. What is that trying to say?
- In what sense is this interchangeable with the generic Subject type? Is the idea that you can compare these to Subject and can create a Subject from one of these? So you can store this instead of a Subject if you happen to have a Subject representing a node?
There was a problem hiding this comment.
Yes, the idea is that:
- you can compare these to Subject
- you can create a Subject from OperationalNodeId or vice-versa.
- you can store this instead of a Subject if you happen to have a Subject representing a node.
| * OperationalNodeId identifies an individual Node on a Fabric. It is a special type of Subject targeting to NodeSubject. It is | ||
| * interchangeable with the generic Subject type but uses less memory. | ||
| */ | ||
| class OperationalNodeId |
There was a problem hiding this comment.
Again, I am not convinced by the use of "Operational" here. It does not match any of the spec's uses of "Operational". This is a ScopedNodeId.
There was a problem hiding this comment.
Maybe we can find a better name for it. But ScopedNodeId is also not proper. Because NodeId is too broad, it also contains CASE Authenticated Tag or Group Node ID.
Here, it can only contain an Operational Node ID .
There was a problem hiding this comment.
The fact that we can have those things stuck into a NodeId is so broken.... :( It leads to really poor naming all around. The vast majority of places that talk about NodeId really mean "in the operational range".
I would rather we stuck to that convention and if we really want a type that represents "any of these things" we had a different name for it than "NodeId".
| * A subject is a global unique identifier. It suite 2 purpose: | ||
| * | ||
| * 1. Identify an entity. operator== can be used to check if 2 subjects are identical. | ||
| * 2. Associate to ACL entries to grant privileges |
There was a problem hiding this comment.
But it can't actually do that, because it does not carry the information that ACL needs (e.g. CATs, for CASE).
There was a problem hiding this comment.
CATs are another type of subjects, I'll add it in the PR.
There was a problem hiding this comment.
This brings us back to the question of "what is this thing trying to represent and why?". For ACL purposes a "subject" in the sense of an ACL entry can be one of a variety of things, but a "subject" in the sense of "what gets matched against an ACL entry" can be a list of things.
We don't have a singular concept of "subject" around.
There was a problem hiding this comment.
Like Boris, I'd like to understand what problem these new classes are trying to solve. Currently (and not judging whether it's ideal), a NodeId is a 64-bit value that, via spec, has its range partitioned into a few "kinds" of NodeId. We have some headers (NodeId.h and friends) with constants and functions for manipulating these NodeIds.
A subject is similar, but not exactly the same thing. ACL entries have subjects, and the SubjectDescriptor is provided by the messaging layer (those session objects and so on) through the IM to the Access Control system module, for performing checks during actions. Part of performing those checks is comparing ACL entry subjects against the SubjectDescriptor.
The other place subjects appear is in cluster operations (read/write entries), where they are just numbers, and the auth mode serves as a discriminant. There's no need for more type richness there, because we're not trying to protect naive users from writing a wrong subject, we expect admins to know what they are doing, and the values are validated regardless.
Looking at the linked issue, it seems the desire is to ensure all the info in those various sessions (in the messaging layer) is correct. This is good, but I think (for the subject parts of that task, aside from other things that may also need to be audited) we only need to check all the places where a SubjectDescriptor is created, to ensure it is being fed appropriate messaging layer data.
The key thing to remember from a design perspective is this: subject is an access control construct, not a messaging construct. It is derived from messaging constructs, but is not the same thing. That's why it's defined in the access module, but created in the messaging module. It's an input API to the access control system module.
|
The potential of this PR is providing a better, easy to use API interface It also defines some foundational classed for ACL to use. |
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
Better in what specific way? Easy to use in what ways?
Does ACL plan to use them? Have you talked to @mlepage-google about these? |
|
PR #15418: Size comparison from f2c450c to 9086755 Increases above 0.2%:
Increases (41 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Full report (44 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
| class PaseSubject | ||
| { | ||
| public: | ||
| static constexpr uint16_t kPasscodeId = 0; |
There was a problem hiding this comment.
There is a PasscodeId typedef.
| * A subject is a global unique identifier. It suite 2 purpose: | ||
| * | ||
| * 1. Identify an entity. operator== can be used to check if 2 subjects are identical. | ||
| * 2. Associate to ACL entries to grant privileges |
There was a problem hiding this comment.
Like Boris, I'd like to understand what problem these new classes are trying to solve. Currently (and not judging whether it's ideal), a NodeId is a 64-bit value that, via spec, has its range partitioned into a few "kinds" of NodeId. We have some headers (NodeId.h and friends) with constants and functions for manipulating these NodeIds.
A subject is similar, but not exactly the same thing. ACL entries have subjects, and the SubjectDescriptor is provided by the messaging layer (those session objects and so on) through the IM to the Access Control system module, for performing checks during actions. Part of performing those checks is comparing ACL entry subjects against the SubjectDescriptor.
The other place subjects appear is in cluster operations (read/write entries), where they are just numbers, and the auth mode serves as a discriminant. There's no need for more type richness there, because we're not trying to protect naive users from writing a wrong subject, we expect admins to know what they are doing, and the values are validated regardless.
Looking at the linked issue, it seems the desire is to ensure all the info in those various sessions (in the messaging layer) is correct. This is good, but I think (for the subject parts of that task, aside from other things that may also need to be audited) we only need to check all the places where a SubjectDescriptor is created, to ensure it is being fed appropriate messaging layer data.
The key thing to remember from a design perspective is this: subject is an access control construct, not a messaging construct. It is derived from messaging constructs, but is not the same thing. That's why it's defined in the access module, but created in the messaging module. It's an input API to the access control system module.
|
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. |
|
This stale pull request has been automatically closed. Thank you for your contributions. |
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.
Problem
SubjectDescriptor is not easy to use. Prepare to fix #13397
Change overview
Subjectcontains information about SubjectDescriptorOperationalNodeIdcontains tuple<FabricIndex, NodeId>GetSubjectin session interface which returns a structuredSubjectDescriptorobjectChipLogFormatSubjectandChipLogValueSubjectto help printSubjectstructureTesting
Passed unit-tests