ARTEMIS-4210 audit connection creation & destruction #4408
Conversation
| @@ -2639,4 +2644,18 @@ static void isAutoDelete(Object source) { | |||
|
|
|||
| @LogMessage(id = 601766, value = "User {} is getting auto-delete property on target resource: {}", level = LogMessage.Level.INFO) | |||
| void isAutoDelete(String user, Object source); | |||
|
|
|||
| static void createdConnection(String protocol, Object connectionID, String remoteAddress) { | |||
| CONNECTION_LOGGER.createdConnection(protocol, connectionID.toString(), String.format("unknown%s", formatRemoteAddress(remoteAddress))); | |||
There was a problem hiding this comment.
Why does the user value always start with 'unknown'?
If there is no way to find the user here, maybe we could review the
@LogMessage(id = 601767, value = "{} connection {} for user {} created", level = LogMessage.Level.INFO) void createdConnection(String protocol, String connectionID, String user);
log template, replacing the 'user' info with some other field... what do you think?
There was a problem hiding this comment.
At the point where this is logged the user isn't actually known yet since that data hasn't been parsed. We definitely don't want to add any additional parsing here as this is on the hot path for every connection. However, I left the user information in the log for 2 main reasons:
- To show the IP address
- To be consistent with the rest of the audit logging
The connection ID is also logged here as well as in the authentication message so these audit messages can be correlated.
| @@ -2639,4 +2644,18 @@ static void isAutoDelete(Object source) { | |||
|
|
|||
| @LogMessage(id = 601766, value = "User {} is getting auto-delete property on target resource: {}", level = LogMessage.Level.INFO) | |||
| void isAutoDelete(String user, Object source); | |||
|
|
|||
| static void createdConnection(String protocol, Object connectionID, String remoteAddress) { | |||
There was a problem hiding this comment.
Why is the 'connectionID' an Object, while in other methods it is a String?
If it is possible, it would be better to manage it in the same way, and having a String seems to me a cleaner way.
Furthermore, managing it as an Object could bring to unexpected NPE, in case it is null, once the 'toString' method will be invoked.
There was a problem hiding this comment.
The difference is to disambiguate the two createdConnection methods from each other otherwise the code wouldn't even compile. Also, while there is a technical risk of NPE there is no practical risk because if the connection ID is null the code will fail before it even gets to the audit logging.
| @@ -380,7 +380,7 @@ private void authenticationFailed(String user, RemotingConnection connection) th | |||
| ActiveMQServerLogger.LOGGER.securityProblemWhileAuthenticating(e.getMessage()); | |||
|
|
|||
| if (AuditLogger.isResourceLoggingEnabled()) { | |||
| AuditLogger.userFailedAuthenticationInAudit(null, e.getMessage()); | |||
| AuditLogger.userFailedAuthenticationInAudit(null, e.getMessage(), connection.getID().toString()); | |||
There was a problem hiding this comment.
I have a doubt here: why is the user (first parameter) always null? Is there no way to find it?
There was a problem hiding this comment.
That's a fair question, but it's not really germane to this PR since it hasn't been changed. I recommend you follow-up on the ActiveMQ users list about this.
No description provided.