-
Notifications
You must be signed in to change notification settings - Fork 116
Clarify extended permission evaluation #48
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
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for the PR!
I think the overall idea that this needs more clarification is good, but I do have some comments hoping to clarify and sharpen the explanation a bit before we merge.
src/xperm_rules.md
Outdated
|
||
Extended permission rules are evaluated as follows: | ||
|
||
* If no extended permissions are defined, only the resource-level policy is |
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.
I don't think "resource-level policy" is clear or exactly precise in this case. Maybe something like "The standard SELinux checks around AVC rules and constraints will be performed".
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.
Thanks for the clarification!; that makes sense, and I have updated the documentation with this wording
src/xperm_rules.md
Outdated
considered. | ||
|
||
* If an extended permission rule is defined, the policy is first evaluated | ||
according to the high-level resource policy. For example: |
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.
See above regarding "high-level resource policy". Additionally, I think this could spell out the situation more explicitly. After the AVC and constraint checks are performed, then the xperm checks will be applied.
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.
That makes sense, thank you. I have updated this to clarify "standard AVC checks".
* If an extended permission rule is defined, the policy is first evaluated | ||
according to the high-level resource policy. For example: | ||
|
||
* If an *allowxperm* rule is defined, extended permissions will only be |
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 sort of makes it sound like "allow" is sufficient (and xperm is totally worthless). Maybe something about "both checks must pass" or similar would be clearer?
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.
That's a good point. I've updated this to state that "both the standard AVC checks and the extended permissions must pass". Please let me know if this statement needs any more clarification.
src/xperm_rules.md
Outdated
granted if *allow* is granted to the resource. | ||
|
||
* If an *auditallowxperm* rule is defined, extended audit permissions will only | ||
be granted if *auditallow* is granted to the resource. |
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.
I think we should avoid the words "granted" and "permissions" in this context when we're not talking about permissions, just auditing. "extended auditing will only be performed..."
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.
Good point; done.
src/xperm_rules.md
Outdated
be granted if *auditallow* is granted to the resource. | ||
|
||
* If any extended permission rule is defined, the resource and operation are fully | ||
evaluated according to extended access rules. All undefined permissions within |
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.
I think "unspecified" would be clearer than "undefined" here. It might be worth it at the end to take an extra sentence to reiterate that xperms are deny-by-default-allow-by-exception.
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.
Sounds good, thanks. I have updated the wording, and added an extra sentence to reiterate this.
Adding documentation to clarify the automatic-deny evaluation when extended permissions are defined, as well as the overall evaluation logic. Signed-off-by: Liz Prucka <[email protected]>
Great, thank you for the review! I have attempted to clarify the explanation as per your comments, but please let me know if anything could use further clarification. |
Adding documentation to clarify the automatic-deny evaluation when extended permissions are defined,
as well as the overall evaluation logic.