I have gotten a lot of good review comments and I think the PR is in a good
place if anyone wants to take another look. I have time to do some work to get
this in the main branch if there are more changes needed. I did some
performance testing on my machine to compare performance of message level
authentication with various amounts or roles and consumers
https://github.com/apache/activemq-artemis/pull/3281#issuecomment-740243430.
There is a performance hit when more consumers are subscribed to topics or
non-authorized consumers on queues. In my view however it's acceptable as its
the cost of doing message level authentication. The only thing that could
really be improved is the role caching, since java stores role(Principals)
internally as a linked list. I am open to ideas, but I think this PR has made
the API changes and a plugin that should cover most normal cases. If end-users
need a ton of roles or are doing authorization slightly different, they now
have the API's to optimize for their own solution.
Thanks for your time,
Ryan Yeats
On 10/5/20, 8:53 AM, "Ryan Yeats" <ryan.ye...@octoconsulting.com> wrote:
Please be cautious
This email was sent from outside of your organization
________________________________
I have a PR up for adding a plugin API and a plugin to support message
level authentication but there are some assumptions and API changes that would
be better explained and discussed here.
The premise for this change is that address level permissions while
suitable for a majority of use cases are not suitable for cases where message
permissions are more dynamic and or coordination with consumers to subscribe on
different topics can't happen before hand.
First issue that prevents this feature is that it needs access to a cached
instance of authenticated users and roles in order to make message policy
decisions. Instead of re-authenticate it made sense to reuse already
authenticated users from the SecurityStore but it does not expose an API to
retrieve the Subject or RolePrinciples. Therefore, I propose adding Subject
SecurityStore#getSessionSubject(SecurityAuth session). In leu of changing the
SecurityStore API it is entirely possible for plugins to use session
information to reauthenticate using the ActiveMQSecurityManager5 API and then
cache the Roles themselves, but this seems too burdensome.
The second issue I found is that none of the existing APIs can affect
delivery of a message. ActiveMQServerMessagePlugin#beforeDelivery is close
since it has all the needed information, but it happens after the handle
decision and can't affect delivery without the negative side effect of making
it not deliverable to other consumers. The ActiveMQSecurityManager API would
also make sense but it doesn't have access to the SecurityStore and the
ServerConsumer is already calling out to ActiveMQServerPlugin API so extending
that makes more sense. Therefore, I propose modifying the ActiveMQServerPlugin
specifically extending ActiveMQServerMessagePlugin to add a boolean
canAccept(ServerConsumer consumer, MessageReference reference).
However, the drawback to this is that Artemis allows multiple plugins all
of which could implement canAccept. This means the results of multiple
plugin#canAccept need to be taken into account by either requiring ALL(logical
AND) or ANY(logical OR) canAccept to be true before delivering a message to a
consumer. Both are acceptable but ANY makes it hard to have a default
implantation of canAccept because a true would override any other false and
false would filter all messages if a plugin doesn't care about implementing
canAccept. So, I am proposing that the results of ALL plugin#canAccepts be true
before the ServerConsumer delivers a message to a consumer.
Hopefully this gives more context to my PR and can help reviewers.
Ryan Yeats
[Octo | Emerging Technology. Human
Impact.]<https://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.octoconsulting.com%2F&data=01%7C01%7Cryan.yeats%40octoconsulting.com%7Ce13bea9f995b4235a0c108d86946c12b%7Cfd88ebfaa896461c92df64d031ea9a37%7C0&sdata=aqQNaZUMn%2F1fCCVuL54MwO2Ks47enGXEcGurLQUPcQ4%3D&reserved=0>
