Add audit log assertions to RemoteClusterSecurityCrossClusterApiKeySi… #137225
Conversation
gmjehovich
added
Team:Security
|
Hi @gmjehovich, I've created a changelog YAML for you. |
|
Pinging @elastic/es-security (Team:Security) |
jfreden
left a comment
jfreden
left a comment
There was a problem hiding this comment.
Looks like this PR was made from your forks main branch. It might make it difficult to work on other stuff so I'd suggest opening a new PR from a branch.
| throw new UncheckedIOException(e); | ||
| } | ||
| } | ||
| }, 10, java.util.concurrent.TimeUnit.SECONDS); |
There was a problem hiding this comment.
I think you can drop 10, java.util.concurrent.TimeUnit.SECONDS since that's the default.
docs/changelog/137225.yaml
Outdated
| @@ -0,0 +1,5 @@ | |||
| pr: 137225 | |||
| summary: Add audit log assertions to `RemoteClusterSecurityCrossClusterApiKeySi…` | |||
There was a problem hiding this comment.
Looks like both the title and this was truncated.
| } | ||
| } | ||
|
|
||
| private void assertCrossClusterSearchSuccessfulWithoutResult() throws IOException { |
There was a problem hiding this comment.
This should also do some audit logging on the fulfilling cluster since the auth fails, the only difference here is that the query cluster ignores the problem.
|
|
||
| try { | ||
| // Call the generic assertion function with the expected 'authentication_success' event | ||
| assertAuditLogContainsNewEvent(startTimeMillis, "authentication_success"); |
There was a problem hiding this comment.
There are constants for authentication_success and authentication_failed in AuditLevel that would be nice to use here.
| assertAuditLogContainsNewEvent(startTimeMillis, "authentication_success"); | ||
| } catch (Exception e) { | ||
| // Re-throw the exception so the test fails if the log isn't found | ||
| throw new RuntimeException("Audit log assertion failed after client-side success check passed.", e); |
There was a problem hiding this comment.
I think this is a little confusing because I don't think the AssertionError thrown by assertAuditLogContainsNewEvent would be caught here (since it doesn't extend Exception). This would catch the IOException thrown by Stream.readAllLines. If readAllLines fail, I think failing the test with fail(e, msg) is better.
| equalTo(true) | ||
| ); | ||
| } catch (IOException e) { | ||
| throw new UncheckedIOException(e); |
There was a problem hiding this comment.
Should this happen in the test? Why not just throws IOException on the method instead?
| return client().performRequest(request); | ||
| } | ||
|
|
||
| private String extractJsonValue(String jsonLine, String fieldName) { |
There was a problem hiding this comment.
Instead of this custom json logic I think it would be cleaner to use XContentHelper.convertToMap and also the constants from here. It will make the test easier to maintain.
| private long parseLogTimestamp(String timestamp) { | ||
| try { | ||
| String standardized = timestamp.replace(',', '.'); | ||
| java.time.format.DateTimeFormatter formatter = java.time.format.DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSZ"); |
There was a problem hiding this comment.
AuditIT has some code to do this, I wonder if that can be reused? See executeAndVerifyAudit.
There was a problem hiding this comment.
It may be possible to move it into a shared utility method, but I think the code there does more testing than we would need (extra assertions for each field, since it's testing general audit logging).
I went ahead and copied the date/time pattern used there. Let me know if that is sufficient, or if you think I should try to extract more shared logic to a utility
| try (var auditLog = fulfillingCluster.getNodeLog(i, LogType.AUDIT)) { | ||
| final List<String> allLines = Streams.readAllLines(auditLog); | ||
|
|
||
| String expectedLogFragment = "event.action\":\"" + eventAction + "\""; |
There was a problem hiding this comment.
It would be nice to use the constants from here to match against.
3 participants
Add audit log assertions to
RemoteClusterSecurityCrossClusterApiKeySigningIT.