search

opensearch-project / security

🔐 Secure your cluster with TLS, numerous authentication backends, data masking, audit logging as well as role-based access control on indices, documents, and fields
https://opensearch.org/docs/latest/security-plugin/index/
Apache License 2.0
199 stars 279 forks source link

[BUG] ComplianceConfig read fields does not expire values from the cache correctly #3950

Open peternied opened 10 months ago

peternied commented 10 months ago

What is the bug? ComplianceConfig supports auditing for rolling audit logs, such as audit-logs-YYYY-MM-dd. There is a cache for field data, and this caches does not know how to handle data rollover correctly.

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

  1. Create a config with * pattern for read fields
  2. Use a rolling log config
  3. Perform an access with todays config
  4. Have the date roll over
  5. Perform an access with the same config as 3, however

Easier repro:

  1. Go to ComplianceConfigTest, and uncomment the following assertion https://github.com/opensearch-project/security/blob/c06365ca94c9a1d15e85b578a1ae48168bf0bca7/src/test/java/org/opensearch/security/auditlog/compliance/ComplianceConfigTest.java#L203-L204
  2. Run the unit test ./gradlew test --tests org.opensearch.security.auditlog.compliance.ComplianceConfigTest

What is the expected behavior? The check will be done against the current config, all other configs should be looked up and reported on.

Do you have any additional context? The bug is in ComplianceConfig.java, see how getFieldsForIndex is used to build the cache; however, the entry should be invalid after a roll over happens. How this is determined could be complex and requires investigation.

stephen-crawford commented 10 months ago

[Triage] Hi @peternied, thank you for filing this issue. This seems like a good thing for us to correct since it makes the rollover feature problematic. We can close this issue when we correct the cache behavior and add tests to show this.

stephen-crawford commented 10 months ago

Hi @peternied, could you add some more specific details on how to reproduce the failure you encountered? I am having a hard time figuring out how to reproduce it since I have not set up a compliance configuration before and the documentation does not show anything about it: https://opensearch.org/docs/latest/security/audit-logs/index/

I see there is an option in the audit logging page of dashboards but should I be looking for a specific "compliance" field? I am not sure where to specify the wildcard read fields pattern...

peternied commented 10 months ago

Thanks @scrawfor99 - I've updated the description with an easier reproduction, there is already a unit test that can catch this behavior

  1. Go to ComplianceConfigTest, and uncomment the following assertion https://github.com/opensearch-project/security/blob/c06365ca94c9a1d15e85b578a1ae48168bf0bca7/src/test/java/org/opensearch/security/auditlog/compliance/ComplianceConfigTest.java#L203-L204
  2. Run the unit test ./gradlew test --tests org.opensearch.security.auditlog.compliance.ComplianceConfigTest
ksankeerth commented 9 months ago

Hi @peternied and @scrawfor99 ,

I'm new to OpenSearch, would like to work on this issue if this is open for community.

I've done some analysis. The LoadingCache.get(String key) method has to invalidate some entries(need a criteria to decide some) in cache if rollover happens. As this logic is not available in the default method, We may extend LoadingCache and implement this logic.

Thanks, Sankeerthan

peternied commented 9 months ago

@ksankeerth Thanks for volunteering - I'd be happy to review a PR that addresses this issue.