Add security tests and workflow plus minor fix #470

lezzago · 2022-07-05T13:57:51Z

Signed-off-by: Ashish Agrawal ashisagr@amazon.com

Description

Add security tests
Add security github workflow
Fix createNotificationConfig API requiring create indices permission

Issues Resolved

Check List

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: Ashish Agrawal <ashisagr@amazon.com>

.../notifications/src/main/kotlin/org/opensearch/notifications/index/NotificationConfigIndex.kt

qreshi · 2022-07-11T20:32:51Z

notifications/notifications/src/test/kotlin/org/opensearch/integtest/SecurityNotificationIT.kt

+        val error = sendResponse.get("error").asJsonObject
+        Assert.assertNotNull(error.get("reason").asString)


Should we check the error explicitly since this is technically testing an expected INTERNAL_SERVER_ERROR because the Slack channel doesn't exist.

Just want to make sure an actual issue presenting itself as an INTERNAL_SERVER_ERROR doesn't slip by in this case.

.github/workflows/security-notifications-test-workflow.yml

Signed-off-by: Ashish Agrawal <ashisagr@amazon.com>

codecov-commenter · 2022-08-04T01:51:24Z

Codecov Report

Merging #470 (2287642) into main (4fe6a39) will decrease coverage by 0.33%.
The diff coverage is 16.66%.

@@             Coverage Diff              @@
##               main     #470      +/-   ##
============================================
- Coverage     61.77%   61.43%   -0.34%     
  Complexity      112      112              
============================================
  Files            73       73              
  Lines          2451     2466      +15     
  Branches        264      266       +2     
============================================
+ Hits           1514     1515       +1     
- Misses          761      773      +12     
- Partials        176      178       +2

Flag	Coverage Δ
opensearch-notifications	`61.43% <16.66%> (-0.34%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...notifications/action/SendTestNotificationAction.kt	`90.90% <ø> (ø)`
...rch/notifications/index/NotificationConfigIndex.kt	`69.67% <16.66%> (-6.76%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

qreshi · 2022-08-04T19:07:16Z

The test and build dashboards CI is failing because it needs to build against 2.2 if we're using OSD branch 2.x. We bumped our 2.x branch to 2.2 but didn't do it to main since we were planning on bumping it to 3.0 anyway. The bump to 3.0 is currently blocked on a bug being tracked in the OpenSearch repo as mentioned in #481

If the bump to 3.0 remains blocked, we'll get main on to 2.2 in the meantime or update the CI to properly build against OSD 2.1. The former would be the more consistent option since it would be strange to have our 2.x branch ahead of main in version.

getsaurabh02 · 2022-08-04T22:29:10Z

.../notifications/src/main/kotlin/org/opensearch/notifications/index/NotificationConfigIndex.kt

+ * @param block a function to process this [AutoCloseable] resource.
+ * @return the result of [block] function invoked on this resource.
+ */
+@Suppress("TooGenericExceptionCaught")


Let's see if we can get rid of these Suppress by having more specific exceptions raised. Unless it is breaking bwc.

getsaurabh02 · 2022-08-04T22:31:25Z

notifications/notifications/src/test/kotlin/org/opensearch/integtest/AccessRoles.kt

+val ALL_ACCESS_ROLE = "all_access"
+val NOTIFICATION_FULL_ACCESS_ROLE = "notifications_full_access"
+val NOTIFICATION_READ_ONLY_ACCESS = "notifications_read_access"
+val NOTIFICATION_NO_ACCESS_ROLE = "no_access"
+val NOTIFICATION_CREATE_CONFIG_ACCESS = "notifications_create_config_access"
+val NOTIFICATION_UPDATE_CONFIG_ACCESS = "notifications_update_config_access"
+val NOTIFICATION_DELETE_CONFIG_ACCESS = "notifications_delete_config_access"
+val NOTIFICATION_GET_CONFIG_ACCESS = "notifications_get_config_access"
+val NOTIFICATION_GET_PLUGIN_FEATURE_ACCESS = "notifications_get_plugin_access"
+val NOTIFICATION_GET_CHANNEL_ACCESS = "notifications_get_channel_access"
+val NOTIFICATION_TEST_SEND_ACCESS = "notifications_test_send_access"


Why do we need to define these access roles separately/specifically for tests, and not import from the actual source for consistency ?

This is purely as a helper class. Only the ALL_ACCESS_ROLE, NOTIFICATION_FULL_ACCESS_ROLE, and NOTIFICATION_READ_ONLY_ACCESS exist, but that is in the security plugin. We have set the others so it easier for the security integration tests to use them.

getsaurabh02 · 2022-08-04T22:32:34Z

notifications/notifications/src/test/kotlin/org/opensearch/integtest/IntegTestHelpers.kt

@@ -99,6 +101,68 @@ fun getStatusText(response: JsonObject): String {
        .get("status_text").asString
 }

+private val charPool: List<Char> = ('a'..'z') + ('A'..'Z') + ('0'..'9')
+
+fun getCreateRequestJsonString(


nit : should we be more specific in naming such as getCreateNotificationRequestJsonString

getsaurabh02 · 2022-08-04T22:35:19Z

notifications/notifications/src/test/kotlin/org/opensearch/integtest/IntegTestHelpers.kt

+        ConfigType.SLACK -> """
+            "slack":{"url":"https://slack.domain.com/sample_slack_url#$randomString"}
+        """.trimIndent()
+        ConfigType.CHIME -> """
+            "chime":{"url":"https://chime.domain.com/sample_chime_url#$randomString"}
+        """.trimIndent()
+        ConfigType.WEBHOOK -> """
+            "webhook":{"url":"https://web.domain.com/sample_web_url#$randomString"}
+        """.trimIndent()
+        ConfigType.SMTP_ACCOUNT -> """
+            "smtp_account":{
+                "host":"smtp.domain.com",
+                "port":"4321",
+                "method":"ssl",
+                "from_address":"$randomString@from.com"


Wondering if these all can be resolved via predefined Map <ConfigType, Value> for better maintainability and reusability ?

The problem is that we need to pass in values to the config definition and abstracting this out would end up complicating the code as no other function currently needs this.

getsaurabh02 · 2022-08-04T22:38:00Z

notifications/notifications/src/test/kotlin/org/opensearch/integtest/SecurityNotificationIT.kt

+        deleteUserWithCustomRole(user, NOTIFICATION_DELETE_CONFIG_ACCESS)
+    }
+
+    fun `test delete slack notification config without get Notification permission`() {


Wondering if we should randomize on the channels; since all the scenarios will fit across different channels (slack/chime)

I agree. In a future PR, we can make the tests run for each channel type

Lets create a backlog.

Signed-off-by: Ashish Agrawal <ashisagr@amazon.com>

* Add security tests and workflow plus minor fix Signed-off-by: Ashish Agrawal <ashisagr@amazon.com> * fix test and update workflow Signed-off-by: Ashish Agrawal <ashisagr@amazon.com> * apply cleanup comments Signed-off-by: Ashish Agrawal <ashisagr@amazon.com> (cherry picked from commit 7f3c6a8)

* Add security tests and workflow plus minor fix Signed-off-by: Ashish Agrawal <ashisagr@amazon.com> * fix test and update workflow Signed-off-by: Ashish Agrawal <ashisagr@amazon.com> * apply cleanup comments Signed-off-by: Ashish Agrawal <ashisagr@amazon.com> (cherry picked from commit 7f3c6a8) Co-authored-by: Ashish Agrawal <ashisagr@amazon.com>

Add security tests and workflow plus minor fix

0aa556d

Signed-off-by: Ashish Agrawal <ashisagr@amazon.com>

lezzago requested a review from a team July 5, 2022 13:57

qreshi reviewed Jul 11, 2022

View reviewed changes

qreshi added the backport 2.x label Aug 3, 2022

qreshi reviewed Aug 3, 2022

View reviewed changes

.github/workflows/security-notifications-test-workflow.yml Show resolved Hide resolved

lezzago added 2 commits August 3, 2022 16:53

Merge remote-tracking branch 'origin/main' into security-tests

8b3ce6b

Signed-off-by: Ashish Agrawal <ashisagr@amazon.com>

fix test and update workflow

e9c8122

Signed-off-by: Ashish Agrawal <ashisagr@amazon.com>

qreshi previously approved these changes Aug 4, 2022

View reviewed changes

qreshi mentioned this pull request Aug 4, 2022

Resolve hosts when checking against host deny list #496

Merged

1 task

getsaurabh02 requested changes Aug 4, 2022

View reviewed changes

apply cleanup comments

2287642

Signed-off-by: Ashish Agrawal <ashisagr@amazon.com>

lezzago dismissed qreshi’s stale review via 2287642 August 5, 2022 00:50

qreshi approved these changes Aug 5, 2022

View reviewed changes

lezzago requested a review from getsaurabh02 August 5, 2022 01:12

getsaurabh02 approved these changes Aug 5, 2022

View reviewed changes

lezzago merged commit 7f3c6a8 into opensearch-project:main Aug 5, 2022

opensearch-trigger-bot bot mentioned this pull request Aug 5, 2022

[Backport 2.x] Add security tests and workflow plus minor fix #497

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add security tests and workflow plus minor fix #470

Add security tests and workflow plus minor fix #470

lezzago commented Jul 5, 2022

qreshi Jul 11, 2022

lezzago Aug 4, 2022

codecov-commenter commented Aug 4, 2022 •

edited

Loading

qreshi commented Aug 4, 2022 •

edited

Loading

getsaurabh02 Aug 4, 2022

getsaurabh02 Aug 4, 2022

lezzago Aug 4, 2022

getsaurabh02 Aug 4, 2022

getsaurabh02 Aug 4, 2022

lezzago Aug 5, 2022

getsaurabh02 Aug 4, 2022

lezzago Aug 5, 2022

getsaurabh02 Aug 5, 2022

		val error = sendResponse.get("error").asJsonObject
		Assert.assertNotNull(error.get("reason").asString)

Add security tests and workflow plus minor fix #470

Add security tests and workflow plus minor fix #470

Conversation

lezzago commented Jul 5, 2022

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 4, 2022 • edited Loading

Codecov Report

qreshi commented Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 4, 2022 •

edited

Loading

qreshi commented Aug 4, 2022 •

edited

Loading