#2616 bwcPlugin extension setting and getTokenManager implementation #2938
Conversation
…nsion settings bwcPluginMode Signed-off-by: scosta <[email protected]>
… for extensions backwards compatibility using BWC_PLUGIN_MODE extension setting Signed-off-by: Sam <[email protected]>
samuelcostae
requested review from
cliu123,
cwperks,
DarshitChanpura,
davidlago,
peternied,
RyanL1997,
stephen-crawford,
reta and
willyborankin
as code owners
… for extensions backwards compatibility using BWC_PLUGIN_MODE extension setting Signed-off-by: Sam <[email protected]>
… for extensions backwards compatibility using BWC_PLUGIN_MODE extension setting Signed-off-by: Sam <[email protected]>
…xtension-setting' into #2616add-bwcPluginMode-extension-setting # Conflicts: # src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java # src/main/java/org/opensearch/security/auth/SecurityTokenManager.java
Signed-off-by: Sam <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #2938 +/- ##
============================================
- Coverage 62.31% 62.16% -0.15%
+ Complexity 3337 3332 -5
============================================
Files 266 267 +1
Lines 19650 19675 +25
Branches 3329 3330 +1
============================================
- Hits 12244 12231 -13
- Misses 5779 5812 +33
- Partials 1627 1632 +5
|
|
Hi Sam, thanks for taking this on. I think we may want to split the extension setting away from the tokenManager. If you look here: #2787, you can see some of what I had been working on with the TokenManager but for the time being, we need to wait until the feature branch is at a state appropriate to be merged in main. |
There was a problem hiding this comment.
Hey Sam, I took an initial pass. I see that you have a usage of the setting in the token manager introduced here, but I think we need to combine this PR with the feature/extensions branch which introduces a JWTVendor which was created for issuing JWTs (on behalf of tokens) for an extension. You are on the right track, but issueToken should vend a JWT using the vendor and this setting will help determine the payload of that JWT and whether backend roles are included in the payload or not. The purpose of including backend roles in the payload is for plugins that are being rewritten as extensions and currently rely on the knowledge of a user's backend roles.
| import java.util.Set; | ||
| import java.util.StringJoiner; | ||
|
|
||
| public class SecurityTokenManager implements TokenManager { |
There was a problem hiding this comment.
Hey @samuelcostae , as @scrawfor99 pointed out if this PR is limited to the settings then wdyt about making this be a Noop version of the token manager and focus the PR on the settings?
Edit: Can this be incorporated into the feature/extensions branch and shown how its utilized in the JWTVendor?
There was a problem hiding this comment.
@cwperks Either way is fine by me, I agree that is beyond the scope originally stated in the issue description, but I thought you had previously asked me(In the other PR) to include some usage of the setting. Were you referencing some other part of the implementation?
| @@ -1098,6 +1108,21 @@ public Settings additionalSettings() { | |||
| return builder.build(); | |||
| } | |||
|
|
|||
| @Override | |||
| public List<Setting<?>> getExtensionSettings() { | |||
There was a problem hiding this comment.
This looks fine to me, but I'd also like to see the usage of the setting inside the JWTVendor in feature/extensions branch to show how this setting impacts the payload of the JWT created of the obo token for forwarding to an extension.
| @@ -1876,6 +1901,16 @@ private static String handleKeyword(final String field) { | |||
| return field; | |||
| } | |||
|
|
|||
| @Override | |||
| public Subject getSubject() { | |||
| return null; | |||
There was a problem hiding this comment.
This will also need to be addressed as we implement IdentityPlugin for the security plugin. Subject should never be null and if no subject is present in the current context then its assumed to be unauthenticated.
You can see an example implementation of SecuritySubject in this draft PR: https://github.com/opensearch-project/security/pull/2773/files#diff-f21aa0165f9d0633658e7c99a1375b84258d1ef2beecaad8df286cfe5ba2a7d2R1-R49
Signed-off-by: Stephen Crawford <[email protected]>
|
Closing as will create a PR from a different branch based on feature/extensions |
Description
This Draft PR includes the new setting bwcPluginMode (backward compatible plugin mode for extensions ) and an tentaptive/draft implementation of OpenSearchSecurityPlugin.java.getTokenManager().
getTokenManager() is a overriden method from the IdentityPlugin Interface and is called by core in RestSendToExtensionAction.java.
The issued token will be sent to the extension in the payload and should include the user and its mapped roles if the setting is set to true.
Issues Resolved
#2616
Testing
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.