Repository navigation
Record the creator on a workflow's sharing record - #2300
DarshitChanpura wants to merge 7 commits into
Conversation
Adds secure integration tests that follow all_shared_principals through share, update and revoke for monitors and for workflows, and that check search returns exactly the monitors and workflows a user owns or is currently shared, replacing the two earlier search tests. The workflow tests need the security plugin to resolve the workflow resource type and alerting to record the workflow's creator, so they pass only once both are fixed. Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
TransportIndexWorkflowAction captured the caller's thread context inside a coroutine on Dispatchers.IO, which does not carry the thread context. The captured context held no user, so under resource sharing the security plugin wrote no sharing record for a new workflow, and the creator could not see it in search. When an earlier request had left a context on that worker, the record could name that request's user instead. The context is now captured in doExecute, on the request thread, before the coroutine starts. It is applied only around the workflow write, through a new withStoredContext helper that restores the coroutine's own context afterwards, so the metadata and monitor lookups that follow still run as the plugin rather than as the caller. Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
PR Reviewer Guide 🔍(Review updated until commit bb19a0f)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to bb19a0f Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit f3dcd02
Suggestions up to commit d88ce61
Suggestions up to commit e13ec29
Suggestions up to commit 60b457e
Suggestions up to commit 069a8a3
|
The security plugin now identifies a resource's type by the value its typeField holds, which defaults to the registered type name. Workflows are registered as alerting-workflow while their documents hold workflow in workflow.type, so the workflow provider declares that value through typeFieldValue(). Monitors store monitor and need no change. Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
|
Persistent review updated to latest commit 60b457e |
Workflow documents hold "workflow" in workflow.type, while the type is registered as alerting-workflow, so the workflow provider overrides matchesResource to match on that value. Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
|
Persistent review updated to latest commit e13ec29 |
…ce-sharing-context Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
|
Persistent review updated to latest commit d88ce61 |
riysaxen-amzn
left a comment
riysaxen-amzn
left a comment
There was a problem hiding this comment.
The core fix looks right: capturing the context in doExecute, on the request thread, before scope.launch explains both the missing sharing record and the wrong-owner records. Making storedThreadContext non-null also fits, since the only call site now always passes it.
My main concern is inline on withStoredContext: as written, I think it can leave the caller's persistent auth header on the IO worker where the write started, which is the same leak class this PR is fixing. A suggested shape is in that comment.
On CI: the short failures (including the security-enabled resource-sharing job) are docker pull rate limits (toomanyrequests), so they never ran the code. The ARM failures (468 tests, 52 failed, region must be set before starting and OpenSearchSingleNodeTestCase.NODE is null) match main's Test Workflow run from Oct 21 exactly, and main has been green since Oct 27. So a rebase on main and a re-run should clear them, and would let SecureResourceSharingMonitorRestApiIT actually run.
It would also help to link an issue for the monitor create/update/delete restore-without-reset follow-up, since that leak is what makes a regression here intermittent.
| * Runs [block] under [stored], then restores the context the coroutine had, on whichever thread it resumes. This keeps | ||
| * the caller's context from reaching the calls that follow [block]. | ||
| */ | ||
| suspend fun <T> ThreadContext.withStoredContext(stored: ThreadContext.StoredContext, block: suspend () -> T): T { |
There was a problem hiding this comment.
This can leave the caller's identity on a worker thread
ThreadContext is per thread, and block suspends. Tracing the write path:
- On IO worker A, this saves
own, thenstored.restore()puts the caller's context on A. indexStashedcallsstashContext()on A. A stash drops transient headers but keeps persistent ones, and the caller's auth is a persistent header (the comment removed in this PR said the write relies on that). It then suspends onclient.index.- On
Dispatchers.IOthe coroutine may resume on worker B, so bothstored.close()inindexStashedandown.restore()here run on B. - Nothing resets A. It keeps the stash state, which still carries the caller's persistent auth header.
The next coroutine that captures context on A can then pick up this caller, which is the "Alice's workflow recorded as Bob's" symptom from the description. The KDoc's promise ("keeps the caller's context from reaching the calls that follow") holds for the coroutine, but not for thread A.
One way to avoid it is to do the restore, stash and dispatch synchronously on A before suspending, and reset A right away:
suspendCoroutine { cont ->
val own = ctx.newStoredContext(false)
stored.restore()
try {
ctx.stashContext().use { client.index(request, listenerFor(cont)) }
} finally {
own.restore()
}
}The listener resumes the coroutine, and no thread keeps the caller's context after this returns. I haven't reproduced the leak; this is from the ThreadContext semantics, so please correct me if the security plugin clears the persistent header somewhere I missed.
There was a problem hiding this comment.
You're right. stashContext() keeps persistent headers, so after the coroutine suspended, the thread that started the write kept the caller's persistent headers, and the thread it resumed on received the starting thread's context.
In bb19a0f I replaced withStoredContext and indexStashed with indexWithContext, which follows your shape: it sets the caller's context, stashes, dispatches the index request, and gives the calling thread its own context back before suspending. SdkClientExtensionsTests starts the write on one thread and answers it on another. It fails on the previous helper (the starting thread still held the caller's header) and passes now.
| storedThreadContext?.restore() | ||
| try { | ||
| val indexResponse: IndexResponse = client.indexStashed(indexRequest) | ||
| val indexResponse: IndexResponse = client.threadPool().threadContext.withStoredContext(storedThreadContext) { |
There was a problem hiding this comment.
Question: on update, this restores the context captured on the request thread, which is the updater's, not the creator's. That's fine if the security plugin only records createdBy on the first index of a doc. Could you confirm that an update by a user the workflow is shared with doesn't rewrite the owner? I couldn't tell from the new tests whether "update" there means updating the workflow doc or updating the sharing.
There was a problem hiding this comment.
Confirmed. The security plugin writes the sharing record only when the document is created. On a later write to the resource, ResourceIndexListener.postIndex only restores all_shared_principals from the record and does not change created_by.
"Update" in the existing test meant an update of the workflow document by the owner. In bb19a0f I added test workflow update by a read-write shared user keeps the owner: bob, with read-write access to the workflow and its delegate monitor, renames the workflow, and the sharing record still names alice as the owner.
| override fun resourceType(): String = ResourceSharingUtils.WORKFLOW_RESOURCE_TYPE | ||
| override fun resourceIndexName(): String = SCHEDULED_JOBS_INDEX | ||
| override fun typeField(): String = "workflow.type" | ||
| override fun matchesResource(fieldValue: Function<String, String>): Boolean = |
There was a problem hiding this comment.
nit: this only compiles against a security SPI that has matchesResource (security#6627), and build.gradle doesn't change, so it relies on the current security snapshot. Since the description says the two must ship together, would a minimum security version check or a release-notes entry be worth adding so that dependency is enforced rather than only described?
There was a problem hiding this comment.
Alerting compiles against the security SPI of its own OpenSearch version (opensearch-security-spi:${opensearch_build}), and the two ship in the same release. A build against an SPI without matchesResource does not compile, so that is the check. Release notes are generated from the PR metadata now, so the description carries the dependency.
…ce-sharing-context Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
|
Persistent review updated to latest commit f3dcd02 |
withStoredContext restored the caller's context, then indexStashed stashed it and suspended on the write. A stash keeps persistent headers, so the thread that started the write kept the caller's persistent headers, and the thread the coroutine resumed on received the starting thread's context when the stash and the stored context were closed. indexWithContext replaces both. It sets the caller's context, stashes, dispatches the index request, and gives the calling thread its own context back before the coroutine suspends, so no thread keeps the caller's context. Adds a unit test that starts the write on one thread and answers it on another, and an integration test that a read-write shared user's update to a workflow keeps the owner on its sharing record. Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
|
Thanks for the review. I merged the current |
|
Persistent review updated to latest commit bb19a0f |
Codecov Report❌ Patch coverage is
❌ Your project check has failed because the head coverage (67.83%) is below the target coverage (70.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2300 +/- ##
============================================
+ Coverage 67.72% 67.83% +0.10%
- Complexity 105 1813 +1708
============================================
Files 160 170 +10
Lines 10343 14322 +3979
Branches 1522 2088 +566
============================================
+ Hits 7005 9715 +2710
- Misses 2672 3727 +1055
- Partials 666 880 +214 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Description
Bug fix. Under resource sharing, creating a workflow wrote no sharing record, so the workflow's creator could not find it by search, and it could not be shared.
TransportIndexWorkflowActioncaptured the caller's thread context insidevalidateMonitorAccess's listener, which runs afterscope.launchonDispatchers.IO. Coroutines do not carry the thread context, so the captured context held no user. The security plugin then logged "no authenticated user found in the thread context" and skipped the record. When an earlier request had left a context on the same worker, the record could name that request's user instead: in one test run, a workflow Alice created was recorded as Bob's. The capture placement came from PR 2180. The monitor action captures before its coroutine, and works.The context is now captured in
doExecute, on the request thread, before the coroutine starts. It is applied only around the workflow write, through a newwithStoredContexthelper that afterwards restores the coroutine's own context. Restoring it without that scope made workflow create fail with "User doesn't have permissions to execute this action": the metadata and monitor lookups after the write then ran as the user against the system index, instead of as the plugin.The other context captures that PR 2180 added all run before any thread change.
Known, and left for a follow-up: the monitor create, update and delete actions restore the caller's context on a
Dispatchers.IOworker and never reset it. As a result, a capture made inside a coroutine picks up whatever an earlier request left there. That is why a regression of this fix shows up in the tests some of the time rather than every time.Related Issues
Needs PR opensearch-project/security#6627, now merged and published in the security snapshot. Without it, the security plugin resolves the workflow's type as
workflow, which matches no provider, so no sharing record is written either way. The two workflow tests below pass only with both changes. The two changes should be released together: without this one, the security change makes workflow records whose owner can be wrong.Testing
The integration tests, the context fix, and a
matchesResourceoverride on the workflow provider. In PR 6627 the security plugin asks each provider whether a document is of its type, throughResourceProvider.matchesResource, which by default compares thetypeFieldwith the type name. Workflows are registered asalerting-workflowbut storeworkflowinworkflow.type, so the override matches on that value; it compiles only against a security SPI that has the method.SecureResourceSharingMonitorRestApiITadds tests that followall_shared_principalsthrough share, update and revoke for monitors and workflows. They also check that search returns exactly what a user owns or is currently shared. With a local security build carrying PR 6627: 33 of 33, onmainand on3.9. The node log no longer shows "no authenticated user found" for workflows.matchesResourceoverride, the two workflow tests fail and the other 31 pass.main, against the published security snapshot that has PR 6627, the resource-sharing suite that CI runs (SecureResourceSharingMonitorRestApiITandRscMigrateE2ERestApiIT,-Dsecurity=true -Dhttps=true -Dresource_sharing.enabled=true): 61 of 61.WorkflowRestApiIT: 30 of 30, both without security and on a secured cluster with resource sharing off.SecureWorkflowRestApiITis marked AwaitsFix, so it does not run.3.9, hence thebackport 3.9label. The backport needs the security 3.9 backport, PR [Backport 3.9] Resolve a resource's type from its provider security#6628, which addsmatchesResourcethere.Check List
--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.