Storing user object in all APIs and enabling filter of response based on user #115

thalurur · 2021-08-11T07:00:45Z

Signed-off-by: Ravi Thaluru [email protected]

Issue #, if available:
#75

Description of changes:

Storing user information populated by security plugin in the APIs
- For transforms, rollups, policies there is a high level attribute called user which stores the user information
- For managed indices, the attached policies of the managed index will have the user information
- None of the indexed job metadata has the user information
Using the stored user information to run the background jobs - rollups, transforms, ism jobs
- The user information is loaded into thread context to do this and this is only done for specific transport actions called as part of the background job - e.g. when writing data to ism config index the threadcontext will not have user information but when reading data from the source index in rollup job will have the user information in threadcontext. This will ensure an less privileged user will not be able to read/write data from other indices they don't have permissions for by creating rollup/transform jobs. Similarly when executing the policy actions the user information is added to thread context
- Any data that was written prior to this change will not have user information in them and they are regarded as jobs with full permissions
Enabling new setting to filter stored information based on the appropriate use permissions
- There is a new setting which enables filtering of data returned by the IndexManagement plugin APIs, if this setting is enabled it is required that all users updating/creating jobs using IM plugin have backend roles. Other wise the request will fail
- If the setting is enabled a user will only see data that matches their backend roles. All the existing data that is written without backend roles will not be visible to anyone (This is a limitation)

Call outs:

One can no longer create a managed index job with a non existent policy, add policy API will verify that the policy exists and is stored with the managed index at the time of creation
Permission check for managed indices is implemented based on a custom permission implemented as part of new Transport action called ManagedIndexAction, i.e if user has this permission on an index they will be able to manage the index - this check is implemented in AddPolicy, removePolicy, ChangePolicy, RetryFailedIndex API and the cluster event sweeper to determine the policy that should be applied to an index
Explain API will always fail to return if user attempts to fetch indices they don't have permissions for - i.e on every index in the response the user needs to have the permission
User information stored in the jobs is not being returned as part of the REST API responses

CheckList:
[ ] 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.

… on user Signed-off-by: Ravi Thaluru <[email protected]>

src/main/resources/mappings/opendistro-ism-config.json

codecov-commenter · 2021-08-14T00:37:50Z

Codecov Report

Merging #115 (e2b58a9) into main (5f6dd66) will decrease coverage by 1.75%.
The diff coverage is 57.25%.

@@             Coverage Diff              @@
##               main     #115      +/-   ##
============================================
- Coverage     77.26%   75.51%   -1.76%     
+ Complexity     1900     1893       -7     
============================================
  Files           258      259       +1     
  Lines         10523    10905     +382     
  Branches       1676     1730      +54     
============================================
+ Hits           8131     8235     +104     
- Misses         1474     1721     +247     
- Partials        918      949      +31

Impacted Files	Coverage Δ
...agement/indexstatemanagement/model/ChangePolicy.kt	`80.39% <0.00%> (ø)`
...temanagement/opensearchapi/OpenSearchExtensions.kt	`79.26% <ø> (-2.06%)`	⬇️
...ement/step/notification/AttemptNotificationStep.kt	`0.00% <0.00%> (ø)`
...ent/transport/action/explain/ExplainAllResponse.kt	`92.00% <0.00%> (ø)`
...ent/indexstatemanagement/util/NotificationUtils.kt	`0.00% <0.00%> (ø)`
...ment/indexstatemanagement/util/RestHandlerUtils.kt	`100.00% <ø> (+6.25%)`	⬆️
...arch/indexmanagement/rollup/RollupSearchService.kt	`57.40% <0.00%> (-3.38%)`	⬇️
...arch/indexmanagement/transform/TransformIndexer.kt	`71.73% <0.00%> (-6.84%)`	⬇️
...ch/indexmanagement/transform/TransformValidator.kt	`58.00% <0.00%> (-2.00%)`	⬇️
...g/opensearch/indexmanagement/util/SecurityUtils.kt	`20.00% <9.67%> (-24.45%)`	⬇️
... and 62 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f6dd66...e2b58a9. Read the comment docs.

…d enabled user permission checks on top of managed index APIs

dbbaughe · 2021-08-16T15:48:40Z

build.gradle

@@ -154,6 +154,7 @@ dependencies {
    compile group: 'commons-codec', name: 'commons-codec', version: '1.13'
    compile "org.jetbrains.kotlin:kotlin-stdlib:${kotlin_version}"
    compile "org.jetbrains.kotlin:kotlin-stdlib-common:${kotlin_version}"
+    compile "org.jetbrains.kotlin:kotlin-stdlib-jdk8:${kotlin_version}"


nit: Are we adding a whole dependency just for the "use" helper function? Is it not possible to just copy that helper function over? Seems a bit overkill.

Yes, its added only for use method. Can copy the method instead - do we follow any rule of thumb on when to copy vs add dependency?

I don't think there's a rule of thumb we follow.. I'd just look at dependency size (+ maintenance overhead, maintainer activity, etc.) vs what we're using from it. If it's just a simple method that we can import ourselves then might be able to not add a whole dependency, but if you find you're having to pull in a lot of things it's fine to add.

...ain/kotlin/org/opensearch/indexmanagement/indexstatemanagement/model/ManagedIndexMetaData.kt

src/test/kotlin/org/opensearch/indexmanagement/IndexManagementIndicesIT.kt

src/main/kotlin/org/opensearch/indexmanagement/indexstatemanagement/ManagedIndexCoordinator.kt

.../indexmanagement/indexstatemanagement/transport/action/addpolicy/TransportAddPolicyAction.kt

src/main/kotlin/org/opensearch/indexmanagement/opensearchapi/OpenSearchExtensions.kt

dbbaughe · 2021-08-16T17:27:19Z

src/main/kotlin/org/opensearch/indexmanagement/util/SecurityUtils.kt

            val injectedUser: User? = User.parse(threadContext.getTransient<String>(ConfigConstants.OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT))
            return if (injectedUser == null) {
                null
            } else {
                User(injectedUser.name, injectedUser.backendRoles, injectedUser.roles, injectedUser.customAttNames)
            }
        }
+


Do we have unit tests for all the below new methods?

no, don't have the tests, will add

dbbaughe · 2021-08-16T17:35:47Z

Few top level comments:

Lots of changes in APIs, just confirm we are not introducing any breaking changes by removing or renaming fields or changing models etc.
Lots of new behavior added for security, but I know we have issues running security tests locally w/ the local integ Cluster that runs. Can we still add integration tests that get filtered out if remote cluster is not used? i.e. we can still write integration tests, but they only run if isRemoteCluster && isSecurityEnabled or something.

src/main/kotlin/org/opensearch/indexmanagement/util/SecurityUtils.kt

dbbaughe · 2021-08-16T17:43:24Z

...management/indexstatemanagement/transport/action/removepolicy/TransportRemovePolicyAction.kt

-                            val indexMetadatas = response.state.metadata.indices
-                            indexMetadatas.forEach {
-                                indicesToRemove.putIfAbsent(it.value.indexUUID, it.key)
+            client.threadPool().threadContext.stashContext().use {


Curious, does this throw an exception if user is trying to access an index they can't access?

No, this is a miss. Updating the check for ChangePolicy and RemovePolicy

bowenlan-amzn · 2021-08-16T17:46:07Z

If an existing ISM job doesn't have user info saved, what are the ways for user to adopt the new security model?
By calling change policy API?

And same question for Rollup and Transform jobs without user.

dbbaughe · 2021-08-16T17:46:57Z

src/main/kotlin/org/opensearch/indexmanagement/rollup/RollupRunner.kt

@@ -257,7 +259,12 @@ object RollupRunner :
                }
            }

-            when (val result = rollupMapperService.attemptCreateRollupTargetIndex(updatableJob, clusterConfigurationProvider.hasLegacyPlugin)) {
+            val result = withClosableContext(
+                IndexManagementSecurityContext(job.id, settings, threadPool.threadContext, job.user)


Do we need to create new IndexManagementSecurityContext each time (here and below in while loop etc) or can we just reuse a single one?

...arch/indexmanagement/indexstatemanagement/transport/action/explain/TransportExplainAction.kt

… and cleanup

thalurur · 2021-08-16T22:02:26Z

If an existing ISM job doesn't have user info saved, what are the ways for user to adopt the new security model?
By calling change policy API?

And same question for Rollup and Transform jobs without user.

Yes, updating would be the option for user to fix

…o manage index

* Add integTest script to the repo (#94) Signed-off-by: Peter Zhu <[email protected]> * Removing Usages of Action Get Call and using listeners (#100) Signed-off-by: Aditya Jindal <[email protected]> * Enhance ISM template (#105) Signed-off-by: bowenlan-amzn <[email protected]> * Explain response still use old opendistro policy id (#109) * Explain response still use old opendistro policy id * Use hardcoded policyid setting in tests for explain response * Trying to fix flaky tests * Storing user information as part of the job when security plugin is installed (#113) Signed-off-by: Ravi Thaluru <[email protected]> * ISM/Notification channel support (#117) * Updates NAME of transport actions * Upgrades Kotlin version, updates dependencies on other OS plugins, adds notification plugin as a test resource and includes it in test clusters * Adds support for Channels in error notifications and notification actions * Adds support for sending notifications to channels * Adds support for publishing notifications to the legacy destinations through the Notification plugin and some cleanup * Removes notification alerting jar dependency * Adds compile only dep on commons codec for digest utils sha1 method in ism rollup * Updates Error Notification to make channel/destination nullable, and adds helper methods for publish calls * Constructs URL for legacy custom webhook Signed-off-by: Drew Baugher <[email protected]> * Fixes Feature enum and dep Signed-off-by: Drew Baugher <[email protected]> * Trying something else Signed-off-by: Drew Baugher <[email protected]> * Addresses comments Signed-off-by: Drew Baugher <[email protected]> * Storing user object in all APIs and enabling filter of response based on user (#115) Signed-off-by: Ravi Thaluru <[email protected]> * Upgrade dependencies to 1.1 and build snapshot by default. (#121) Signed-off-by: dblock <[email protected]> * Security improvements (#126) Signed-off-by: Ravi Thaluru <[email protected]> * Removes support for notification plugin (#136) Signed-off-by: Drew Baugher <[email protected]> * Updating security filtering logic (#137) Signed-off-by: Ravi Thaluru <[email protected]> * Release note for 1.1.0.0 release. (#139) * Release note for 1.1.0.0 release. Signed-off-by: bowenlan-amzn <[email protected]> * Correct copyright notices * Uses published daily snapshot dependencies (#141) Signed-off-by: Drew Baugher <[email protected]> * License header check (#142) * Provide default copyright header using IDE feature Signed-off-by: bowenlan-amzn <[email protected]> * Address #103 history write index is rolled over even if the history indices are disabled * Removed integtest.sh. (#148) Signed-off-by: dblock <[email protected]> * Adds mavenLocal back to repositories (#158) Signed-off-by: Drew Baugher <[email protected]> * Making snapshot name to scripted input in template (#77) Signed-off-by: Ravi Thaluru <[email protected]> * Fix issues with security changes in rollup runnner (#161) * Updates index management version to 1.2 (#157) * Updates index management version to 1.2 * Updates job scheduler snapshot to 1.2 in test resources Signed-off-by: Robert Downs <[email protected]> * Adds setting to search all rollup jobs on a target index (#165) * Adds cluster setting to search all rollup jobs Signed-off-by: Clay Downs <[email protected]> * Adds implementation for the delay feature in rollup jobs (#147) * Adds delay implementation for rollup jobs * Removes non-continuous delay implementation * Adds additional rollup delay tests Signed-off-by: Clay Downs <[email protected]> * Updates testCompile mockito version, adds AwaitsFix annotation to MetadataRegressionIT tests (#168) * Updates testCompile mockito version to match OpenSearch changes * AwaitsFix on the failing MetadataRegressionIT tests Signed-off-by: Robert Downs <[email protected]> * Adds cluster setting to configure index state management jitter (#153) * Adds jitter cluster setting, sets jitter to 0 for ISM tests Signed-off-by: Clay Downs <[email protected]> * Allows out of band rollovers on an index without causing ISM to fail (#180) * Allows out of band rollovers on an index without causing ISM to fail Signed-off-by: Drew Baugher <[email protected]> * Fixes detekt issue Signed-off-by: Drew Baugher <[email protected]> * Remove policy API on read only indices (#182) * In explain API not showing the total count to all users (#185) Co-authored-by: Peter Zhu <[email protected]> Co-authored-by: Aditya Jindal <[email protected]> Co-authored-by: Bowen Lan <[email protected]> Co-authored-by: Ravi <[email protected]> Co-authored-by: Drew Baugher <[email protected]> Co-authored-by: Daniel Doubrovkine (dB.) <[email protected]>

… on user (opensearch-project#115) Signed-off-by: Ravi Thaluru <[email protected]>

thalurur marked this pull request as draft August 11, 2021 07:00

thalurur force-pushed the main branch 4 times, most recently from b7b611e to 1838cc6 Compare August 12, 2021 22:26

Storing user object in all APIs and enabling filter of response based…

0fbf841

… on user Signed-off-by: Ravi Thaluru <[email protected]>

thalurur force-pushed the main branch from 1838cc6 to 0fbf841 Compare August 12, 2021 23:00

thalurur marked this pull request as ready for review August 12, 2021 23:33

thalurur requested review from bowenlan-amzn and dbbaughe August 12, 2021 23:33

correction

5c8792b

dbbaughe reviewed Aug 13, 2021

View reviewed changes

src/main/resources/mappings/opendistro-ism-config.json Show resolved Hide resolved

Merge branch 'main' into main

77b8159

Applying policies on indices based on the user permission if exist an…

6ee2601

…d enabled user permission checks on top of managed index APIs