[Feature/Identity] Select AuthenticationManager via configuration #5953
Conversation
Signed-off-by: Craig Perkins <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Craig Perkins <[email protected]>
cwperks
force-pushed
the
configurable-auth-manager
branch
from
7de61c0 to
49270b1
Compare
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Craig Perkins <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Craig Perkins <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
@@ Coverage Diff @@
## feature/identity #5953 +/- ##
======================================================
+ Coverage 70.73% 70.87% +0.14%
- Complexity 58964 59067 +103
======================================================
Files 4824 4821 -3
Lines 281960 281956 -4
Branches 40638 40645 +7
======================================================
+ Hits 199438 199843 +405
+ Misses 66184 65821 -363
+ Partials 16338 16292 -46
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
This looks good to me. I have left only one comment.
Thank you for this PR @cwperks ! Will the ExtensibleIdentityPlugin be limited to modifying Auth manager or is there additional scope?
| } | ||
|
|
||
| dependencies { | ||
| implementation project(':sandbox:libs:opensearch-authn') | ||
| compileOnly project(':sandbox:libs:opensearch-authn') |
There was a problem hiding this comment.
Are we sure that we don't need it as runtime also. If not, we should leave it as implementation
There was a problem hiding this comment.
In this PR, the identity module extends the extensible-identity module which has this as an implementation dependency. It will get the dependency at runtime because of this this relationship.
|
Thank you @DarshitChanpura. @peternied Presented another solution for Identity plugins in this PR: #5925 against main. I think the new plugin extension point |
| identity.auth_manager_class: org.opensearch.identity.authmanager.internal.InternalAuthenticationManager | ||
| ``` | ||
|
|
||
| Note that `identity.auth_manager_class` is optional and will default to the InternalAuthenticationManager. Use this setting if you would like to use a non-default AuthenticationManager. |
There was a problem hiding this comment.
I really like that you added steps for people to be able to test Identity out!
| } | ||
|
|
||
| test { | ||
| systemProperty 'opensearch.set.netty.runtime.available.processors', 'false' |
There was a problem hiding this comment.
When this gets merged will it have any impacts outside of our use or is it a relatively minor difference? I know it sets the number of processors and I think that setting it to false will make it not restrict the netty thread pool size. Is this intended?
| @@ -38,6 +42,13 @@ | |||
| // TODO not sure why ThreadLeakScope.NONE is required | |||
| @ThreadLeakScope(ThreadLeakScope.Scope.NONE) | |||
There was a problem hiding this comment.
I know this is old but this is overriding thread leak behavior to do no checks. I assume that if NONE is required it is detecting either a SUITE level leak (the whole suite may not have leaks but the individual tests can) or a TEST level leak (no test may have a leak).
I do not know if we need to be concerned about this. I am not sure where we would be swapping threads in this test case but imagine it is probably when it starts the second node or when it goes to check the cluster health.
|
@cwperks |
|
@peternied I plan to close this PR. I also thinks it's best to add a new extension point in |
Description
Uses the
ExtensiblePluginextension point to dynamically set the Authentication Manager based on settings. The default authentication manager is theInternalAuthenticationManagerin the Identity module that uses the internal identity implementations of the APIs for AuthenticationManager and Subject. Other plugins can extend theextensible-identityplugin and define implementations of AuthenticationManager to allow for other implementations of the Identity APIs. See opensearch-project/security@main...cwperks:security:initial-feature-identity for an example of the security plugin implementing thegetSubject()method.PluginsServicewas modified to allow plugins defined in tests to be able to extend plugins and testloadExtensions.A file called
IDENTITY_TESTING.mdwas added to outline the steps of running a cluster with identity enabled and the security plugin installed.The
extensible-identitymodule is responsible for setting the Auth Manager on Identity which can then be used to get the Subject and perform calls on the current Subject.Issues Resolved
#5884
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.