Skip to content

aaa: enforce per-list authorization for private lists (fail closed) - #331

Open
zeroscience wants to merge 1 commit into
apache:masterfrom
zeroscience:fix/private-list-authz
Open

aaa: enforce per-list authorization for private lists (fail closed)#331
zeroscience wants to merge 1 commit into
apache:masterfrom
zeroscience:fix/private-list-authz

Conversation

@zeroscience

Copy link
Copy Markdown

Makes authoritative necessary but not sufficient in can_access_list(), adds a per-list authorization hook, and fails closed - so a single login can no longer read every private list. Reported first via security@apache.org and raised here at the PMC's request; see #330. Happy to iterate on where the per-list membership source should come from.

Refs #330

Comment thread server/plugins/aaa.py
# authorized for. Anything else fails closed.
if not (session.credentials and session.credentials.authoritative):
return False
if not listid:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check should be first, as it is cheaper.
Though I wonder if listid should be optional here.

Comment thread server/plugins/aaa.py
owner/moderator/subscriber roster, or an explicit config ACL). Until a
membership model exists, only globally-configured admins are granted, and
everyone else is denied. Never grant a private list to every authoritative
user again.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last sentence (Never … again) does not belong in the code.

Comment thread server/plugins/aaa.py
"""
user = getattr(session.credentials, "email", None) or getattr(
session.credentials, "uid", None
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above check - and the rest of the code - makes assumptions about the permission scheme, and may not be appropriate in all cases.

It should be up to the installer to provide the necessary code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants