-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
ACL/Transport: Add structured subject #15418
Conversation
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)
|
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)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What problems does this actually solve that GetSubjectDescriptor does not solve? The PR rationale claims these exist, but does not say what they are.
|
||
struct PaseSubject | ||
{ | ||
static constexpr uint16_t kPasscodeId = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not used except in logging, right? Why do we need this at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CATs
are another type of subjects, I'll add it in the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Problem
SubjectDescriptor is not easy to use. Prepare to fix #13397
Change overview
Subject
contains information about SubjectDescriptorOperationalNodeId
contains tuple<FabricIndex, NodeId>
GetSubject
in session interface which returns a structuredSubjectDescriptor
objectChipLogFormatSubject
andChipLogValueSubject
to help printSubject
structureTesting
Passed unit-tests