fix: access control to fail-closed when owner attributes are missing #4273
Conversation
derekhiggins
requested review from
ashwinb,
bbrowning,
cdoern,
ehhuang,
franciscojavierarceo,
leseb,
mattf and
raghotham
as code owners
meta-cla
bot
added
the
CLA Signed
|
@grs would you mind double checking if my thinking is correct here |
|
|
||
| def matches(self, resource: ProtectedResource, user: User) -> bool: | ||
| required = self.owners_values(resource) | ||
| if not required: |
There was a problem hiding this comment.
Maybe change the name of the required variable, as it now doesn't indicate whether a check is required or not?
| class UserIsOwner: | ||
| def matches(self, resource: ProtectedResource, user: User) -> bool: | ||
| return resource.owner.principal == user.principal if resource.owner else False | ||
| return resource.owner.principal == user.principal if resource.owner else True |
There was a problem hiding this comment.
I'm questioning the wisdom of this, As is above
UserIsOwner passes if User is the owner OR the resource has no owner (public)
I wonder if we should have a new rule(UserIsNull)
UserIsOwner: passes if User is the owner
UserIsNull:the resource has no owner (public)
There was a problem hiding this comment.
Actually, I agree with you, sorry I missed that! I think it would be unexpected for a rule enabled only if the user is the owner to apply when there is no owner.
I think UserIsNull should be OwnerIsNull though, or perhaps even better ResourceIsUnowned.
There was a problem hiding this comment.
Ack, I'll go with ResourceIsUnowned so ,
CI though up changes I need to make to the prebuild SQL query in _build_default_policy_where_clause (for the default policy), I'm currently going through that, I'll update this once I'm happy with what I have.
The tests are failing because of differences in how ProtectedResource/SqlRecord sets owners on public resources vs ResourceWithOwner So that its clear whats I'm proposing to fix this specific issue I've propose a seperate PR #4284 Once its fixed, I'll have to rebase this PR (with some changes in AuthorizedSqlStore) |
Changed UserInOwnersList.matches() to return False instead of True when a resource's owner attributes are None. This prevents unintended access when resource when owner attributes aren't present. For example, checking "user in owners teams" now returns False if the resource has no teams attribute, rather than defaulting to True. Added "ResourceIsUnowned" to allow access to unowned resources. Updated default_policy to use multiple separate "user in owners" AccessRules instead of a single rule with multiple when clauses. With the new fail-closed behavior, only one rule needs to match. Added a "user is owner" rule to handle resources. Also added ResourceIsUnowned to allow access to unowned resources. Updated the SQL generated by _build_default_policy_where_clause to reflect the above changes. Closes: llamastack#4272 Signed-off-by: Derek Higgins <[email protected]>
derekhiggins
force-pushed
the
GHSA-phmq-84v6-rhg9
branch
from
c75f085 to
bd14fa5
Compare
3 participants
Changed UserInOwnersList.matches() to return False instead of True when a resource's owner attributes are None. This prevents unintended access when resource when owner attributes arn't present.
For example, checking "user in owners teams" now returns False if the resource has no teams attribute, rather than defaulting to True.
Changed UserIsOwner.matches() to return True when a resource has no owner attribute set. This allows access to resources that don't use the owner attribute.
Updated default_policy to use multiple separate "user in owners" AccessRules instead of a single rule with multiple when clauses. With the new fail-closed behavior, only one rule needs to match. Added a "user is owner" rule to handle resources without attribute-based access.
Closes: #4272