[BUG] Audit logging fails to log the request if Rest request is invalid

[shikharj05]

What is the bug?
Audit logging doesn't log a failed request if request body fails to match content-type header or required parameters. For example, if request body logging is enabled and passed content-type is unsupported, the following method can see OpenSearchParseException or IllegalStateException thrown from here. Since the exception is not handled by caller, the log method fails to log the request.

How can one reproduce the bug?
Steps to reproduce the behavior:

1. Launch an OpenSearch cluster with security plugin and audit-logging enabled.
2. Send a curl request like-curl -XGET 'https://localhost:9200/?source=abc' --insecure -H 'content-type: '
3. This request fails with 401, however this is not logged in audit logs. e.g. Exception trace in logs-

[2022-05-17T20:18:05,268][WARN ][r.suppressed             ] [] path: /, params: {source=abc}
java.lang.IllegalStateException: source and source_content_type parameters are required
	at org.opensearch.rest.RestRequest.contentOrSourceParam(RestRequest.java:545) ~[opensearch-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT]
	at org.opensearch.security.auditlog.impl.AuditMessage.addRestRequestInfo(AuditMessage.java:384) ~[?:?]
	at org.opensearch.security.auditlog.impl.AbstractAuditLog.logFailedLogin(AbstractAuditLog.java:146) ~[?:?]
	at org.opensearch.security.auditlog.impl.AuditLogImpl.logFailedLogin(AuditLogImpl.java:134) ~[?:?]
	at org.opensearch.security.auth.BackendRegistry.authenticate(BackendRegistry.java:273) ~[?:?]
	at org.opensearch.security.filter.SecurityRestFilter.checkAndAuthenticateRequest(SecurityRestFilter.java:192) ~[?:?]
	at org.opensearch.security.filter.SecurityRestFilter$1.handleRequest(SecurityRestFilter.java:125) ~[?:?]

What is the expected behavior?
These exceptions should be handled in the method here

What is your host/environment?

• OS: NA
• Version: 2.0
• Plugins: Security plugin

Do you have any screenshots?
NA

Do you have any additional context?
NA

[DarshitChanpura]

[Triage] We wouldn't recommend logging invalid requests by default. However, we'd be interested to review a pull request.

@jimishs Can you please provide more inputs here?

[cliu123]

@jimishs To be more clear - Invalid requests is pretty common. For example, a request in wrong syntax(could even be caused by a typo or anything) is an invalid request. It's not even reach server because it's a bad request. So it seems not related to security at all. In this case, we'd like to have your inputs on how much value to support this case in Audit Logging.

[shikharj05]

@jimishs To be more clear - Invalid requests is pretty common. For example, a request in wrong syntax(could even be caused by a typo or anything) is an invalid request. It's not even reach server because it's a bad request. So it seems not related to security at all. In this case, we'd like to have your inputs on how much value to support this case in Audit Logging.

Adding further comments-

Correct, not all invalid requests can be logged. However, if a request is reaching security plugin and it's AuthN is checked, plugin should not fail at logging it.

[cwperks]

[Triage] @shikharj05 Closing this issue as its been inactive for over 6 months. Please re-open if this is still an issue to be addressed.

[shikharj05]

Yes, this is still not fixed. The method catches IOException, however, the downstream function can throw other exceptions as well. Request will not be logged in such cases. @cwperks can you reopen this issue?

[stephen-crawford]

[Triage] Hi @shikharj05, thank you for confirming this issue still persists. We would be happy to review a PR looking to introduce this behavior as a configurable option for a cluster.

[Aayush8394]

Here if request body Audit logging is enabled, we are trying to log the request body. This check ensures that there is request body present.

contentOrSourceParam method throws OpenSearchParseException using the same hasContentOrSourceParam check which was already done by caller method (as explained above). Hence we won't see OpenSearchParseException in caller method

But in case there is source query string parameter present in the request, source_content_type is required to parse the request body. source parameter is already validated above. Hence contentOrSourceParam method will throw IllegalStateException if source_content_type is missing or value of source_content_type is invalid.

Proposed Solution:
We should handle IllegalStateException in caller method here with a error message as ERROR: Unable to generate request body with invalid source_content_type
We should also modify catch IOException to catch Exception as a catch all error case, in case some other exception gets added in future in the underlying method

[shikharj05]

closing the issue as #4232 is merged