Store Scheduled Job User Information in an index owned by the Security plugin #2773

cwperks · 2023-05-15T18:45:54Z

Description

Companion PR in Core: opensearch-project/OpenSearch#7573
Companion PR in Job Scheduler: opensearch-project/job-scheduler#394

This PR implements one of the methods introduced in a new interface in core (See companion PR) around Scheduled Job Identity Management. This PR shows an implementation of saveUserDetails which is present on an interface called ScheduledJobIdentityManager. This method can be called within Job Scheduler to save user info into an index owned by the security plugin that tracks user info alongside the job_id and job_index which is a reference to where the scheduled job details are stored in indices owned by respective plugins that extend job scheduler.

With this security model, plugins and extensions will not need to store user information alongside job details as many plugins do today. With this model, an Identity plugin can implement 3 methods:

ScheduledJobIdentityManager.saveUserDetails(String jobId, String jobIndex)
ScheduledJobIdentityManager.deleteUserDetails(String jobId, String jobIndex)
ScheduledJobIdentityManager.issueAccessTokenOnBehalfOfUser(String jobId, String jobIndex)

to implement scheduled job security. For the security plugin, that will mean creating a new protected index (.opendistro-security-scheduled-job-identity in this Draft PR) and keeping a full catalog of all registered scheduled jobs (jobId and jobIndex as compound key for referential integrity) and the User information of the user that owns the job.

Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)

New Feature

Issues Resolved

#2528
#2626

Is this a backport? If so, please add backport PR # and/or commits #

Check List

New functionality includes testing
New functionality has been documented
Commits are signed per the DCO using --signoff

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.

Signed-off-by: Craig Perkins <cwperx@amazon.com>

…ross indices Signed-off-by: Craig Perkins <cwperx@amazon.com>

Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks · 2023-05-15T18:47:10Z

@peternied I'm curious on your thoughts to this approach for scheduled job security - this approach offloads the responsibility of storing user info on a plugin/extension.

Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks · 2023-05-16T17:21:07Z

@joshpalis Tagging to keep you in the loop. This is a Draft PR that shows the approach to scheduled job security for extensions. It would be useful for plugins today as well and offloads plugins needing to keep track of user info themselves.

Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks · 2023-05-17T16:05:08Z

src/main/java/org/opensearch/security/identity/SecurityScheduledJobIdentityManager.java

+        } else if (totalHits == 1) {
+            logger.info("Scheduled Job Identity already exists in " + SCHEDULED_JOB_IDENTITY_INDEX + " for job with jobId " + jobId);
+        } else {
+            final User user = threadPool.getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER);


@davidlago Made a good comment that if an extension/plugin is writing to its Jobs Index that the user in the threadcontext may not be the right user to persist for the job identity index. I'm looking into ways that the correct user information can be provided.

DarshitChanpura

took a first pass and left some comments.

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java

src/main/java/org/opensearch/security/identity/SecurityIndex.java

DarshitChanpura · 2023-05-18T15:58:13Z

src/main/java/org/opensearch/security/identity/SecurityIndex.java

+    private final String indexName;
+    private final String mapping;
+
+    SecurityIndex(String name, Supplier<String> mappingSupplier) {


Why use Supplier here? Asking because we are just returning a String and not a custom class object

DarshitChanpura · 2023-05-18T16:01:08Z

src/main/java/org/opensearch/security/identity/SecurityIndices.java

+    // keep track of whether the mapping version is up-to-date
+    private EnumMap<SecurityIndex, IndexState> indexStates;
+
+    class IndexState {


Could you please explain the purpose of this class?

This is based off some similar code in AD. I will take a deeper dive and reply back with findings.

DarshitChanpura · 2023-05-18T16:03:00Z

src/main/java/org/opensearch/security/identity/SecurityIndices.java

+            if (createdResponse.isAcknowledged()) {
+                IndexState indexState = indexStates.computeIfAbsent(index, IndexState::new);
+                if (Boolean.FALSE.equals(indexState.mappingUpToDate)) {
+                    indexState.mappingUpToDate = Boolean.TRUE;


Why set it to true if it was false? Asking because i'm not sure where are we updating the mapping

src/main/java/org/opensearch/security/identity/SecurityIndices.java

Signed-off-by: Craig Perkins <cwperx@amazon.com>

peternied

Nice work getting Security to support Identity!

peternied · 2023-06-05T21:37:10Z

src/main/java/org/opensearch/security/identity/SecuritySubject.java

+        }
+        final User user = (User) threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER);
+        if (user == null) {
+            return NamedPrincipal.UNAUTHENTICATED;


When there is no user in the thread context isn't this when we are running as 'root' in OpenSearch? I really like that we'd be patching this hole, but want to double check there aren't implications

That's partially true, what really defines superuser is:

Lack of User in the threadcontext

Lack of Origin in the threadcontext or Origin === LOCAL

No injected roles

See https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/filter/SecurityFilter.java#L258-L266 - this is the block that skips PrivilegeEvaluation after a plugin stashes its threadcontext and executes a TransportAction

I think you are correct though that if a plugin calls on getSubject() and there is no User in the threadcontext that that would mean that the plugin is in superuser mode.

While I don't like the behavior of falling back to superuser on a null, I think we should keep the relationship subject <-> security user consistent otherwise we might have logging that says the user is unauthenticated alongside system-only changes.

Note; I think changing the default behavior would be very good for the security posture, but we would likely need to make this a breaking change ala 3.0.

What about None since that's technically the case? If the threadcontext is stashed then there is no subject. Contrast that to doing clientcert auth with a certificate that matches an admin_dn configured in opensearch.yml. That would truly be the root user.

I'm fine with either approach - I had to implement the IdentityPlugin as part of this PR so this code is tangentially related to the meat of the PR

While tangentially related there are a lot of implications of these values that can create confusion, if you'd prefer not to address this now, how about a follow up issue?

Changed this to ROOT and introduced that NamedPrincipal in the companion PR for core.

Working on providing those data flow diagrams now for job creation, job execution and job deletion.

Signed-off-by: Craig Perkins <cwperx@amazon.com>

RyanL1997

Hi @cwperks, thanks for putting this together! According to this comment, I would like to ask some of the general questions about this approach for information storage.

RyanL1997 · 2023-06-13T22:55:47Z

src/main/java/org/opensearch/security/identity/ScheduledJobIdentity.java

+    private final Instant lastUpdateTime;
+    private final User user;
+
+    public ScheduledJobIdentity(


Refer to the idea of information storage, is this the identity of the job? (including all the basic informations). I'm asking this because I was thinking, if we create a token management index, what kinds of information we should store? Should it be some general information from the claims? (such as, iat, default exp or the token itself?)

Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks · 2023-07-27T21:13:02Z

Closing for now. Will re-open at a later date with updates.

cwperks added 10 commits May 10, 2023 22:07

WIP on extension settings

18417b3

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Merge branch 'main' into extension-security-settings

38b27c8

Merge branch 'main' into extension-security-settings

b3ba5b4

WIP on Scheduled Job Identity Manager

1957eb3

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Add ToXContent for user

eb2051b

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Use Search Query instead of Get Request since docIds aren't unique ac…

369d9cc

…ross indices Signed-off-by: Craig Perkins <cwperx@amazon.com>

Change indentation

e6005c1

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Remove extensions settings changes

3b5b028

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Remove extensions settings changes

14d9111

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Merge branch 'main' into scheduled-job-identity

fd64dc8

Remove references

f1513de

Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks mentioned this pull request May 15, 2023

Create ScheduledJobIdentityManager interface in IdentityPlugin opensearch-project/OpenSearch#7573

Closed

6 tasks

cwperks added 2 commits May 16, 2023 12:05

Remove reference to AD

1a183a3

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Update ScheduledJobIdentity

9dc058f

Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks added 2 commits May 16, 2023 13:44

Add SecurityIndices for security index related methods

871b057

Signed-off-by: Craig Perkins <cwperx@amazon.com>

WIP on tests

7ad9dd6

Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks commented May 17, 2023

View reviewed changes

DarshitChanpura reviewed May 18, 2023

View reviewed changes

cwperks added 6 commits May 23, 2023 15:15

Merge branch 'main' into scheduled-job-identity

c75c65b

Use ScheduledJobOperator from core

f4c55bf

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Create BearerToken

fa21481

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Implemente deleteUserDetails

83e8f3c

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Merge branch 'main' into scheduled-job-identity

ec6d843

Run spotlessApply

0203554

Signed-off-by: Craig Perkins <cwperx@amazon.com>

peternied reviewed Jun 5, 2023

View reviewed changes

cwperks added 3 commits June 6, 2023 13:55

Merge branch 'main' into scheduled-job-identity

9fd6350

Return ROOT for user not in threadcontext

34887e8

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Update bearer token

8c2d18b

Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks mentioned this pull request Jun 13, 2023

Adding end to end endpoint with various changes RyanL1997/security#1

Merged

3 tasks

RyanL1997 reviewed Jun 13, 2023

View reviewed changes

cwperks added 12 commits June 20, 2023 11:12

Merge branch 'main' into scheduled-job-identity

34195a7

Run spotlessApply

84121b1

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Merge branch 'main' into scheduled-job-identity

609fa51

Use durable threadcontext

c02c40c

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Merge branch 'main' into scheduled-job-identity

85ce526

Update extension point names

238c103

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Merge branch 'main' into scheduled-job-identity

c485095

Create convertOperatorToUser

dddfc6a

Signed-off-by: Craig Perkins <cwperx@amazon.com>

WIP on extracting user info from token

612a691

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Merge branch 'main' into scheduled-job-identity

699eeba

Run spotlessApply

28098f7

Signed-off-by: Craig Perkins <cwperx@amazon.com>

No header overlap

0fcd476

Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks closed this Jul 27, 2023

Store Scheduled Job User Information in an index owned by the Security plugin #2773

Store Scheduled Job User Information in an index owned by the Security plugin #2773

Uh oh!

Conversation

cwperks commented May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

cwperks commented May 15, 2023

Uh oh!

cwperks commented May 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarshitChanpura left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

peternied left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwperks Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanL1997 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwperks commented Jul 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cwperks commented May 15, 2023 •

edited

Loading

cwperks Jun 6, 2023 •

edited

Loading