dlmarion commented on PR #5483: URL: https://github.com/apache/accumulo/pull/5483#issuecomment-2809389258
Regarding the removal of the new log message, in https://github.com/apache/accumulo/pull/5480#discussion_r2045674000 you said, `Given that the appropriate place to log this would be in the audit logs, and the authorizations are in fact logged there already, I'm going to revert this new log message.` I think that's not the case, and is likely a bug. `ThriftScanClientHandler.startScan` calls `SecurityOperation.canScan(TCredentials, TableId, NamespaceId, TRange, List<TColumn>, List<IterInfo>, Map<String,Map<String,String>>, List<ByteBuffer>)` which is overridden in `AuditedSecurityOperation`. `ThriftScanClientHandler.startMultiScan` calls `SecurityOperation.canScan(TCredentials, TableId, NamespaceId)` and `SecurityOperation.authenticatedUserHasAuthorizations(TCredentials, List<ByteBuffer>)`, neither of which are overridden by `AuditedSecurityOperation`. It looks like there are several methods in SecurityOperation that are not overridden by AuditedSecurityOperation. You also said `... this extra log message adds risk that routine client requests can fill the server-side logs by merely submitting an invalid request.` Doesn't the audit.log pose the same risk? If users wanted to mitigate this risk, wouldn't they turn the audit log off? The audit log might be more useful if `denied` operations were logged at a higher level than `permitted` operations. For example, if `denied` were logged at `warn` and `permitted` logged at `info`, then the user could easily change the log level to reduce the noise in the audit log. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
