Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change doc level query name validation #630

Merged
merged 6 commits into from
Apr 12, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ data class DocLevelQuery(

init {
// Ensure the name and tags have valid characters
validateQuery(name)
validateQueryName(name)
for (tag in tags) {
validateQuery(tag)
validateQueryTag(tag)
}
}

Expand Down Expand Up @@ -80,6 +80,7 @@ data class DocLevelQuery(
const val QUERY_FIELD_NAMES_FIELD = "query_field_names"
const val NO_ID = ""
val INVALID_CHARACTERS: List<String> = listOf(" ", "[", "]", "{", "}", "(", ")")
val QUERY_NAME_REGEX = "^.{0,256}$".toRegex() // regex to restrict string length 256 chars
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doubt: How did we decide to cap the length at 256?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another comment - this regex differs from the one in alerting: https://github.com/opensearch-project/alerting/pull/1506/files#diff-c8b7d067c22deb1581efc7a7286769d1c24d1a96146f8a4ecb92ef0dec16c44aR53

What's the reason for a difference?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose 256 as the cap based on security analytics front end validation for a rule name in hopes of remaining somewhat consistent:
https://github.com/opensearch-project/security-analytics-dashboards-plugin/blob/3e01e1631fa8aaff4e0968c68424a81abff03981/public/utils/validation.ts#L14

I had to change the length to 0-256 because when security analytics creates findings from bucket level monitors, it will create a doc level query with an empty name.
https://github.com/opensearch-project/security-analytics/blob/0507239054d238dac1e9cf53cfde488aeb4be1c2/src/main/java/org/opensearch/securityanalytics/findings/FindingsService.java#L302

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex is different from alerting and security analytics because when security analytics converts the prepackaged sigma rules into doc level queries, there are some existing prepackaged rules that have some of the special characters like ( so we can't include all the validation at the doc level query level.


@JvmStatic
@Throws(IOException::class)
Expand All @@ -100,7 +101,7 @@ data class DocLevelQuery(
QUERY_ID_FIELD -> id = xcp.text()
NAME_FIELD -> {
name = xcp.text()
validateQuery(name)
validateQueryName(name)
}

QUERY_FIELD -> query = xcp.text()
Expand All @@ -112,7 +113,7 @@ data class DocLevelQuery(
)
while (xcp.nextToken() != XContentParser.Token.END_ARRAY) {
val tag = xcp.text()
validateQuery(tag)
validateQueryTag(tag)
tags.add(tag)
}
}
Expand Down Expand Up @@ -159,16 +160,20 @@ data class DocLevelQuery(
return DocLevelQuery(sin)
}

// TODO: add test for this
private fun validateQuery(stringVal: String) {
private fun validateQueryTag(stringVal: String) {
for (inValidChar in INVALID_CHARACTERS) {
if (stringVal.contains(inValidChar)) {
throw IllegalArgumentException(
"They query name or tag, $stringVal, contains an invalid character: [' ','[',']','{','}','(',')']"
"The query tag, $stringVal, contains an invalid character: [' ','[',']','{','}','(',')']"
)
}
}
}
private fun validateQueryName(stringVal: String) {
if (!stringVal.matches(QUERY_NAME_REGEX)) {
throw IllegalArgumentException("The query name, $stringVal, can be max 256 characters.")
}
}
}

// constructor for java plugins' convenience to optionally avoid passing empty list for 'fieldsBeingQueried' field
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ import java.util.regex.Pattern
// Valid ID characters = (All Base64 chars + "_-") to support UUID format and Base64 encoded IDs
private val VALID_ID_CHARS: Set<Char> = (('a'..'z') + ('A'..'Z') + ('0'..'9') + '+' + '/' + '_' + '-').toSet()

// Invalid characters in a new name field: [* ? < > | #]
private val INVALID_NAME_CHARS = "^\\*\\?<>|#"

fun validateUrl(urlString: String) {
require(isValidUrl(urlString)) { "Invalid URL or unsupported" }
}
Expand Down Expand Up @@ -53,3 +56,15 @@ fun validateIamRoleArn(roleArn: String) {
val roleArnRegex = Pattern.compile("^arn:aws(-[^:]+)?:iam::([0-9]{12}):([a-zA-Z_0-9+=,.@\\-_/]+)$")
require(roleArnRegex.matcher(roleArn).find()) { "Invalid AWS role ARN: $roleArn " }
}

fun isValidName(name: String): Boolean {
// Regex to restrict string so that it cannot start with [_, -, +],
// contain two consecutive periods or contain invalid chars
val regex = Regex("""^(?![_\-\+])(?!.*\.\.)[^$INVALID_NAME_CHARS]+$""")

return name.matches(regex)
}

fun getInvalidNameChars(): String {
return INVALID_NAME_CHARS
}
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,19 @@ class DocLevelMonitorInputTests {
}

@Test
fun `test create Doc Level Query with invalid characters for name`() {
val badString = "query with space"
fun `test create Doc Level Query with invalid name length`() {
val stringBuilder = StringBuilder()
repeat(257) {
stringBuilder.append("a")
}
val badString = stringBuilder.toString()

try {
randomDocLevelQuery(name = badString)
Assertions.fail("Expecting an illegal argument exception")
} catch (e: IllegalArgumentException) {
Assertions.assertEquals(
"They query name or tag, $badString, contains an invalid character: [' ','[',']','{','}','(',')']",
"The query name, $badString, can be max 256 characters.",
e.message
)
}
Expand All @@ -64,7 +69,7 @@ class DocLevelMonitorInputTests {
Assertions.fail("Expecting an illegal argument exception")
} catch (e: IllegalArgumentException) {
Assertions.assertEquals(
"They query name or tag, $badString, contains an invalid character: [' ','[',']','{','}','(',')']",
"The query tag, $badString, contains an invalid character: [' ','[',']','{','}','(',')']",
e.message
)
}
Expand Down
Loading