Skip to content
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

ARTEMIS-1884 add plugin API for message level authorization policies #3281

Closed
wants to merge 1 commit into from

Conversation

ryeats
Copy link
Contributor

@ryeats ryeats commented Sep 29, 2020

No description provided.

@gtully
Copy link
Contributor

gtully commented Sep 30, 2020

this looks nice but it seem too many hoops to go through to have a plugin implement a filter and a very specific new plugin type. A generic filter may be more generally applicable. canAccept behaving like filter.match and gating delivery.

maybe the existing broker plugin can have a new:

  • boolean canAccept(ServerConsumer, MessageReference)

and with the new getSubject accessor that can implement authorization.

@@ -2444,6 +2446,25 @@ public void callBrokerMessagePlugins(final ActiveMQPluginRunnable<ActiveMQServer
callBrokerPlugins(getBrokerMessagePlugins(), pluginRun);
}

@Override
public boolean callBrokerMessagePluginsCanAccept(ServerConsumer serverConsumer, MessageReference messageReference) throws ActiveMQException {
for (ActiveMQServerMessagePlugin plugin : getBrokerMessagePlugins()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Default return should surely be false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am probably not following what you are getting at. If no MessagePlugins are present or if all the plugins are using default canAccept implementation this returns true and no messages are filtered out. I am making an assumption that the results of all MessagePlugin#canAccept calls should be combined as an AND though so if any single plugin returns false the message will not be delivered to that consumer.

*/
@Override
public Subject getSessionSubject(SecurityAuth session) {
if (securityManager instanceof ActiveMQSecurityManager5) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a little specific, why only 5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ActiveMQSecurityManager5 is the only ActiveMQSecurityManager API that exposes returning a users subject the other APIs abstracts away the details of JAAS to a greater extent.

@ryeats ryeats force-pushed the ARTEMIS-1884 branch 3 times, most recently from 8a3e5c2 to f58ccf5 Compare October 1, 2020 22:40
@clebertsuconic
Copy link
Contributor

@ryeats for big features like this, it would be nice to also have a DISCUSS thread on the dev list explaining how things works, some designs.. etc...

I'm not trying to pedantic or anything.. just saying it would be nice.. (like if you look at the dev-list now, you will see a thread I started myself for a PR I am going to make next week).

@ryeats
Copy link
Contributor Author

ryeats commented Oct 1, 2020

Yeah this turned into a bigger change than i was thinking i am requesting permission to post to the dev forums now.

@jbertram
Copy link
Contributor

@ryeats, I think this looks good for the most part, but it's a little hard to review with the commits separated out. Can you squash all the commits together?

@ryeats ryeats force-pushed the ARTEMIS-1884 branch 3 times, most recently from 4768227 to 52e97b0 Compare November 20, 2020 22:25
@ryeats
Copy link
Contributor Author

ryeats commented Dec 7, 2020

Hacked at the Perf test to try out a few cases. Turns out Roles are stored in the javax.security.auth.Subject as a "Set" thats really backed by linked list so they do effect performance which is disappointing. I tried to run the test in a way that made the worst case take place every time so canAccept returns true on the 100th role on the 100th consumer.

Performance Test on a Queue:
Baseline performance
4743.47 msg/s (250000 messages in 52.70s

Role\Consumers 1 11 101
2 56.10s 56.64s 66.37s
11 57.53s 58.96s 68.48s
101 56.23s 60.53s 84.66s

I also tested against a topic worst case 50 consumers have the role and it is their 100th role. Had some trouble hitting memory or other system limits on this one so adjusted the consumers down to avoid that noise but it still wasn't super consistent.
Performance Test on a Topic with durability off:
Baseline Performance
1 consumers 83084.08 msg/s (250000 messages in 3.01s)
10 consumers 21347.45 msg/s (250000 messages in 11.71s)
50 consumers 21396.78 msg/s (250000 messages in 11.68s)

Performance Test on a Topic:

Role\Consumers 1 11 51
2 3.84s 8.16s 13.15s
11 2.49s 12.20s 18.93s
101 3.08s 11.58s 25.01s

The numbers were not super consistent but do seem to show a trend towards higher with more consumers and roles. I think there has to be some expectation of a trade off in performance when doing message level authentication though. It is disappointing that java's role principles are not backed by a hash lookup but the number of roles is normally small.

Copy link
Contributor

@jbertram jbertram left a comment

Choose a reason for hiding this comment

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

You a need "negative" queue test in tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/management/MessageAuthorizationTest.java (i.e. a test where the consumers can't receive the messages due to the incorrect role).

@ryeats ryeats force-pushed the ARTEMIS-1884 branch 2 times, most recently from 7a8e6d9 to c103946 Compare December 9, 2020 19:33
@ryeats
Copy link
Contributor Author

ryeats commented Jan 13, 2021

@jbertram don't know a better way to touch base but wanted to checkin and see if there was anything else holding this PR up and if so what are the next steps to push it along?

@jbertram
Copy link
Contributor

@ryeats, sorry it took me so long to get back to this. Everything looks good. I'll merge it ASAP.

@asfgit asfgit closed this in 0845ff2 Jan 25, 2021
@jbertram
Copy link
Contributor

@ryeats, nice work, BTW!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants