Add filter by backend roles setting

[markdboyd]

Description

This PR adds a new plugin setting, plugins.alerting.filter_by_backend_roles_access_strategy, which allows users to control how filtering by backend roles works to determine access to alerting objects (monitors). The options for this setting are:

• exact - Users have access to alerting objects if they have exactly the same (with no additional) backend roles as the user who created the object
• intersect - Users have access to alerting objects if they share at least one backend role with the user who created the object
• all - Users have access to alerting objects if their backend roles contain all of the backend roles of the user who created the object

Related Issues

Resolves #1940

Check List

• New functionality includes testing.
• New functionality has been documented.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff.
• Public documentation issue/PR created.

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.

[AWSHurneyt]

@markdboyd with the ALL strategy, what is the intended behavior when a user has all of the same backend roles but also some additional roles? With this implementation, it looks like a user with more roles would not match the resource.

If I'm understanding the use case correctly, are the below behaviors the intention?

1. user roles = [role1, role2, role3] and resource roles = [role1, role2, role3] should match
2. user roles = [role1, role2, role3, role4, role5] and resource roles = [role1, role2, role3] should match
3. user roles = [role1, role2, role3] and resource roles = [role1, role2, role3, role4, role5] should not match

[markdboyd]

@AWSHurneyt

what is the intended behavior when a user has all of the same backend roles but also some additional roles

In this case with the ALL strategy, the user would not have access.

If I'm understanding the use case correctly, are the below behaviors the intention?

1. user roles = [role1, role2, role3] and resource roles = [role1, role2, role3] should match
2. user roles = [role1, role2, role3, role4, role5] and resource roles = [role1, role2, role3] should match
3. user roles = [role1, role2, role3] and resource roles = [role1, role2, role3, role4, role5] should not match

For scenario 2, the behavior would also be not match and the user would not have access

Perhaps ALL would be better named MATCH_EXACTLY to specify this behavior?

[AWSHurneyt]

Ah thanks for clarifying. Yeah, EXACT makes sense in my mind given that additional context.

However, that intended behavior seems odd to me. Could you help me understand the use case for not permitting access to the resource when a user has more roles than are necessary?

[markdboyd]

@AWSHurneyt - I thought more about this and I agree: ALL should provide access if the user roles contain all of the object backend roles, even if the user has additional roles. So for ALL, the user would have access in scenarios 1 and 2.

I added new logic to handle the ALL strategy. And I added EXACT as a separate strategy where the roles must match exactly with no additional roles

[markdboyd]

@AWSHurneyt OK, I believe that I have added all of the requested tests

[AWSHurneyt]

@markdboyd I've rerun the failing security test a few times, but it still failing unfortunately. Could you take a look?

[markdboyd]

@AWSHurneyt OK. I've run 5 of the failing tests from that workflow on my local machine, but none are failing so far.

[markdboyd]

@AWSHurneyt OK, I think I have resolved the test failures. The cause of the failures was the changes that I have made in the TransportAcknowledgeAlertAction.kt code.

Initially, the problem was that I was having failures on tests acknowledging alerts when filtering by backend roles is enabled. Specifically, the code was returning this error: https://github.com/cloud-gov/opensearch-alerting/blob/dcb6f946bf12da4e9d6124db2f9fa7c059d1bf24/alerting/src/main/kotlin/org/opensearch/alerting/transport/SecureTransportAction.kt#L86-L92

I traced out the error to the fact that TransportAcknowledgeAlertAction is doing a request to get a monitor: https://github.com/cloud-gov/opensearch-alerting/blob/dcb6f946bf12da4e9d6124db2f9fa7c059d1bf24/alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportAcknowledgeAlertAction.kt#L112. And when filtering by backend role is enabled, this request needs the user information from the parent context, otherwise it fails with the above error.

My initial fix was to pass along just the user information from the parent context manually: 16a21e3. But it seems that solution was not thread safe, so inspired by

alerting/alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportGetRemoteIndexesAction.kt

Lines 116 to 120 in ab154d2

	client.threadPool().threadContext.stashContext().use {
	scope.launch(TenantContext(tenantId)) {
	val singleThreadContext = newSingleThreadContext("GetRemoteIndexesActionThread")
	withContext(singleThreadContext) {
	it.restore()

, I refactored the code to 4c908c0.

However, with that change, we started seeing several integration test failures related to acknowledging alerts. I believe that the problem with 4c908c0 is that it was including the user information from the parent context when invoking the AcknowledgeHandler. However, the AcknowledgeHandler wants to run in the system context without any information about the authenticated user, otherwise it fails to retrieve information about alerts.

So I have now refactored the code again to 298276a. The gist of the behavior is now:

• The parent context is stashed here: https://github.com/cloud-gov/opensearch-alerting/blob/dcb6f946bf12da4e9d6124db2f9fa7c059d1bf24/alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportAcknowledgeAlertAction.kt#L95C10-L95C57
• We explicitly restore the parent context here: https://github.com/cloud-gov/opensearch-alerting/blob/dcb6f946bf12da4e9d6124db2f9fa7c059d1bf24/alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportAcknowledgeAlertAction.kt#L102 so that the get monitor request has access to the user stored in the parent context. the user must be accessible in the context for the get monitor request when filtering by backend role is enabled, otherwise you get this error: https://github.com/cloud-gov/opensearch-alerting/blob/dcb6f946bf12da4e9d6124db2f9fa7c059d1bf24/alerting/src/main/kotlin/org/opensearch/alerting/transport/SecureTransportAction.kt#L86-L92
• Before running the acknowledge handler, we re-stash the parent context, because the AcknowledgeHandler should be acting as the system, not the authenticated user: https://github.com/cloud-gov/opensearch-alerting/blob/dcb6f946bf12da4e9d6124db2f9fa7c059d1bf24/alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportAcknowledgeAlertAction.kt#L129-L131. Without this change, all those security tests fail because they cannot properly acknowledge alerts when acting as the authenticated user

With these changes, I believe that the code should function as expected when filtering by backend role is enabled or not. And that all tests should now pass.

[AWSHurneyt]

Do we expect capitalized and mixed-case inputs to be valid (e.g., ALL, and aLl)?

[markdboyd]

I made the validation case insensitive: 2412be4, however, does this change alone allow a capitalized input to be accepted and used by the system?

[cwperks]

@AWSHurneyt since this is a cluster setting, I believe adding mixed case here allows all these permutations for flexibility. Similar to a CLI program accepting y|Y|YES|yes for more flexibility.

[cwperks]

Same as alerting PR, let's add Setting.Property.Sensitive here.

[cwperks]

I'm not an expert in this repo, but why must this be copied to all of these transport actions?

[markdboyd]

@cwperks I think the reason this variable has to be copied into all of the transport actions is that filterByAccessStrategy is defined as a member on the SecureTransportAction interface which all of these actions implement: https://github.com/cloud-gov/opensearch-alerting/blob/2412be4a761f203a7cf1aac2a37321f3e6b7d1eb/alerting/src/main/kotlin/org/opensearch/alerting/transport/SecureTransportAction.kt#L42. So in order to satisfy the interface, all of the transport actions must include that member.

If I remove this line from the code then try to compile the plugin, I get this error:

Class 'TransportIndexAlertingCommentAction' is not abstract and does not implement abstract member:
var filterByAccessStrategy: String

[cwperks]

nit: add this near related FILTER_BY_BACKEND_ROLES setting in this list.

[cwperks]

btw I think you can add something like

alerting/alerting/src/test/kotlin/org/opensearch/alerting/transport/PluggableDataFormatMonitorBlockIT.kt

Lines 30 to 36 in 1b33dbe

	@Before
	fun checkPluggableDataformatEnabled() {
	assumeTrue(
	"Skipping — pluggable dataformat tests require feature flag enabled on cluster",
	System.getProperty("tests.pluggable_dataformat_enabled", "false").toBoolean()
	)
	}

at the top of the test suite to skip all tests if running ITs without security enabled.

[markdboyd]

@cwperks Not all of the tests in this file seem to require that https is enabled. Should they and then I can apply your suggested change?

[markdboyd]

Related documentation changes: opensearch-project/documentation-website#12794

[AWSHurneyt]

Confirmed with @cwperks offline that his comments have been addressed. Merging.