Skip to content

ARTEMIS-2886 optimize security auth#3246

Merged
asfgit merged 1 commit into
apache:masterfrom
jbertram:ARTEMIS-2886
Aug 26, 2020
Merged

ARTEMIS-2886 optimize security auth#3246
asfgit merged 1 commit into
apache:masterfrom
jbertram:ARTEMIS-2886

Conversation

@jbertram

@jbertram jbertram commented Aug 25, 2020

Copy link
Copy Markdown
Contributor

Both authentication and authorization will hit the underlying security
repository (e.g. files, LDAP, etc.). For example, creating a JMS
connection and a consumer will result in 2 hits with the same
authentication request. This can cause unwanted (and unnecessary)
resource utilization, especially in the case of networked configuration
like LDAP.

There is already a rudimentary cache for authorization, but it is
cleared totally every 10 seconds by default (controlled via the
security-invalidation-interval setting), and it must be populated
initially which still results in duplicate auth requests.

This commit optimizes authentication and authorization via the following
changes:

  • Replace our home-grown cache with Google Guava's cache. This provides
    simple caching with both time-based and size-based LRU eviction. See more
    at https://github.com/google/guava/wiki/CachesExplained. I also thought
    about using Caffeine, but we already have a dependency on Guava and the
    cache implementions look to be negligibly different for this use-case.
  • Add caching for authentication. Both successful and unsuccessful
    authentication attempts will be cached to spare the underlying security
    repository as much as possible. Authenticated Subjects will be cached
    and re-used whenever possible.
  • Authorization will used Subjects cached during authentication. If the
    required Subject is not in the cache it will be fetched from the
    underlying security repo.
  • Caching can be disabled by setting the security-invalidation-interval
    to 0.
  • Cache sizes are configurable.
  • Management operations exist to inspect cache sizes at runtime.

@jbertram jbertram force-pushed the ARTEMIS-2886 branch 2 times, most recently from 4baa5d5 to 8db16f8 Compare August 26, 2020 01:20
}, 2000, 100);

// Removing the Role... the check should be cached, so the next createConsumer shouldn't fail
securityManager.getConfiguration().removeRole("auser", "receiver");

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.

I don't understand this...

Why can't we just use the previous operation on clearing the cache.

I don't think this is a valid test change. The test was written to validate the cache cleared.

if caching becomes a valid option now, we should rather just remove the test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That test was meant to (among other things) exercise the authorization cache. However, with the introduction of the authentication cache the semantics changed a bit. I've updated the test to make this more clear. Thanks for the review.

@jbertram jbertram force-pushed the ARTEMIS-2886 branch 7 times, most recently from b8e38d3 to 8c45fee Compare August 26, 2020 18:19
Both authentication and authorization will hit the underlying security
repository (e.g. files, LDAP, etc.). For example, creating a JMS
connection and a consumer will result in 2 hits with the *same*
authentication request. This can cause unwanted (and unnecessary)
resource utilization, especially in the case of networked configuration
like LDAP.

There is already a rudimentary cache for authorization, but it is
cleared *totally* every 10 seconds by default (controlled via the
security-invalidation-interval setting), and it must be populated
initially which still results in duplicate auth requests.

This commit optimizes authentication and authorization via the following
changes:

 - Replace our home-grown cache with Google Guava's cache. This provides
simple caching with both time-based and size-based LRU eviction. See more
at https://github.com/google/guava/wiki/CachesExplained. I also thought
about using Caffeine, but we already have a dependency on Guava and the
cache implementions look to be negligibly different for this use-case.
 - Add caching for authentication. Both successful and unsuccessful
authentication attempts will be cached to spare the underlying security
repository as much as possible. Authenticated Subjects will be cached
and re-used whenever possible.
 - Authorization will used Subjects cached during authentication. If the
required Subject is not in the cache it will be fetched from the
underlying security repo.
 - Caching can be disabled by setting the security-invalidation-interval
to 0.
 - Cache sizes are configurable.
 - Management operations exist to inspect cache sizes at runtime.
@asfgit asfgit closed this in 4e33b53 Aug 26, 2020
@asfgit asfgit merged commit 4e33b53 into apache:master Aug 26, 2020
@luisalves00

Copy link
Copy Markdown

The new API looks much cleaner, but on the:

boolean authorize(Subject subject, Set<Role> roles, CheckType checkType);

the address was removed and I don't understand why. Address is the Resource you are trying to perform the operation to, so it doesn't seem possible anymore to have a fine grain control. (reference: https://www.keycloak.org/docs/latest/authorization_services/#_overview_terminology)

@jbertram

jbertram commented Sep 2, 2020

Copy link
Copy Markdown
Contributor Author

The address parameter was removed as part of the API simplification because it wasn't required. It wasn't actually being used anywhere. The authorize method gets the subject which contains all the roles that the user has, a Set of Role objects (which contain the role's name and its permissions) from the broker's security-settings, and the type of check that needs to be authorized. The name of the address or FQQN isn't really needed. The security manager just needs to sort through the parameters and see if the Subject has the right roles for the requested check.

Do you have a use-case which requires the address/FQQN? If so, please elaborate.

@luisalves00

Copy link
Copy Markdown

My use case is quite simple. I'll try to describe as best as I can, but please note that not all is decided yet.

Clients (services) will connect to the broker using an access token (https://auth0.com/docs/flows/client-credentials-flow) for a given realm [domain] (Keycloak is the OpenID provider).
The token contains the identification of the service (ClientID) and their roles (if needed). I want everything to be dynamic, so every authenticated client can create addresses under a certain domain that include their clientID. E.g I'm app-a then I can create (checkType.CREATE_ADDRESS) the address "org.artemis.app-a.event". So on ActiveMQSecurityManager4 this was already in place. Now I want to be able to control who subscribes to my address (their might be some sensible data and I don't want all clients snooping arround). Here is the part that I requested on the ticket of the FQQN. The flow will be the same as the client that created the address but the operation will be CheckType.CREATE_DURABLE_QUEUE (as it's subscribing) over the resource "org.artemis.app-a.event::app-c". As you've might guessed the clientID is "app-c" and on his access token it will have a role that allow it to subscribe. Something like "org.artemis.app-a.event::app-c::subscribe". This was granted by AppA on Keycloak. This last part is not yet defined as it's also possible to use a Request Party Token (RPT), but in each case I need the CLIENTID (maps to Subject), RESOURCE (maps to an FQQN), SCOPE (maps to the CheckType) and the Permission (maps to RESOURCE::CHECK_TYPE) where I can apply policies to grant or not access (reference: https://www.keycloak.org/docs/latest/authorization_services/#_overview_terminology).

Does it make sense?

@jbertram

jbertram commented Sep 3, 2020

Copy link
Copy Markdown
Contributor Author

@luisalves00, yes that makes sense. I'll put the address/FQQN into the interface. Thanks for the explanation.

@luisalves00

Copy link
Copy Markdown

@jbertram Thanks, that would be great for me and hopefully others. Do you know when you'll do the next release?

@jbertram

jbertram commented Sep 4, 2020

Copy link
Copy Markdown
Contributor Author

@luisalves00, I just sent #3254 to deal with this. We just released 2.15.0 so I'm not sure when we'll do the next release. My guess would be in the next month or so given all the changes that have already been made. In any case, you can build your own release if necessary.

@luisalves00

Copy link
Copy Markdown

@jbertram didn't realize that you've already released 2.15.0. Now it make sense that the master is on 2.16.0-SNAPSHOT as I have built it on my machine to test this feature.

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.

4 participants