You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2023/01/07 15:41:43 UTC
[GitHub] [kafka] curie71 opened a new pull request, #13090: KAFKA-14605 Change the log level to info when logIfAllowed is set, warn when logIfDenied is set.
curie71 opened a new pull request, #13090:
URL: https://github.com/apache/kafka/pull/13090
KAFKA-14605 StandardAuthorizer log at INFO level when logIfDenied is set(otherwise, we log at TRACE), but at debug level when logIfAllowed is set.
Since audit log is security log, it should be logged at default verbosity level, not debug or trace when logIfAllowed is set.
So I think, log at INFO when allow, and log at WARN when deny is better.
```java
private void logAuditMessage(
...... ) {
switch (rule.result()) {
case ALLOWED:
if (action.logIfAllowed() && auditLog.isDebugEnabled()) {
auditLog.debug(......); // info maybe better
} else if (auditLog.isTraceEnabled()) {
auditLog.trace(buildAuditMessage(principal, requestContext, action, rule));
}
return;
case DENIED:
if (action.logIfDenied()) {
auditLog.info(......); // warn maybe better
} else if (auditLog.isTraceEnabled()) {
auditLog.trace(buildAuditMessage(principal, requestContext, action, rule));
}
}
}
```
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] curie71 commented on a diff in pull request #13090: KAFKA-14605 Change the log level to info when logIfAllowed is set, warn when logIfDenied is set.
Posted by GitBox <gi...@apache.org>.
curie71 commented on code in PR #13090:
URL: https://github.com/apache/kafka/pull/13090#discussion_r1070537837
##########
metadata/src/main/java/org/apache/kafka/metadata/authorizer/StandardAuthorizerData.java:
##########
@@ -329,7 +329,7 @@ private void logAuditMessage(
// authorized operations or a filter (e.g for regex subscriptions) to filter out authorized resources.
// In this case, log only at trace level.
if (action.logIfDenied()) {
- auditLog.info(buildAuditMessage(principal, requestContext, action, rule));
+ auditLog.warn(buildAuditMessage(principal, requestContext, action, rule));
Review Comment:
Thanks for your reply!
Firstly, audit log is **security log**, not ~debug log~. Security log should be enabled by default during production such as fatal, error, warn, info verbosity levels. But now, we only enable **deny** log by default in KAFKA.
```java
case DENIED:
if (action.logIfDenied()) {
auditLog.info(......); // deny log at verbose level
} else if (auditLog.isTraceEnabled()) {
auditLog.trace(buildAuditMessage(principal, requestContext, action, rule)); // debug log for deny
}
```
Secondly, the debug level is close to trace level, the `if (action.logIfAllowed)` branch is **redundant**.
```java
case ALLOWED:
if (action.logIfAllowed() && auditLog.isDebugEnabled()) { // redundant branch
auditLog.debug(......); // debug log
} else if (auditLog.isTraceEnabled()) {
auditLog.trace(buildAuditMessage(principal, requestContext, action, rule)); // debug log again
}
```
In my opinion, if we don' t think "allow log" is as important as "deny log", we should **delete** the redundant branch here.
Or we could log allow at **info** level with a switch `action.logIfAllowed = false/true` for performance consideration.
```java
private void logAuditMessage(
...... ) {
switch (rule.result()) {
case ALLOWED:
if (action.logIfAllowed() && auditLog.isDebugEnabled()) {
auditLog.debug(......); // info maybe better
} else if (auditLog.isTraceEnabled()) {
auditLog.trace(buildAuditMessage(principal, requestContext, action, rule));
}
return;
case DENIED:
if (action.logIfDenied()) {
auditLog.info(......); // warn maybe better
} else if (auditLog.isTraceEnabled()) {
auditLog.trace(buildAuditMessage(principal, requestContext, action, rule));
}
}
}
```
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a diff in pull request #13090: KAFKA-14605 Change the log level to info when logIfAllowed is set, warn when logIfDenied is set.
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #13090:
URL: https://github.com/apache/kafka/pull/13090#discussion_r1066631471
##########
metadata/src/main/java/org/apache/kafka/metadata/authorizer/StandardAuthorizerData.java:
##########
@@ -329,7 +329,7 @@ private void logAuditMessage(
// authorized operations or a filter (e.g for regex subscriptions) to filter out authorized resources.
// In this case, log only at trace level.
if (action.logIfDenied()) {
- auditLog.info(buildAuditMessage(principal, requestContext, action, rule));
+ auditLog.warn(buildAuditMessage(principal, requestContext, action, rule));
Review Comment:
The `AclAuthorizer` (as shown below) has the same implementation. This is long-standing behavior and there would have to be a strong reason to change it at this point.
> if (authorized) {
> // logIfAllowed is true if access is granted to the resource as a result of this authorization.
> // In this case, log at debug level. If false, no access is actually granted, the result is used
> // only to determine authorized operations. So log only at trace level.
> if (action.logIfAllowed)
> authorizerLogger.debug(logMessage)
> else
> authorizerLogger.trace(logMessage)
> } else {
> // logIfDenied is true if access to the resource was explicitly requested. Since this is an attempt
> // to access unauthorized resources, log at info level. If false, this is either a request to determine
> // authorized operations or a filter (e.g for regex subscriptions) to filter out authorized resources.
> // In this case, log only at trace level.
> if (action.logIfDenied)
> authorizerLogger.info(logMessage)
> else
> authorizerLogger.trace(logMessage)
> }
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] github-actions[bot] commented on pull request #13090: KAFKA-14605 Change the log level to info when logIfAllowed is set, warn when logIfDenied is set.
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13090:
URL: https://github.com/apache/kafka/pull/13090#issuecomment-1585816423
This PR is being marked as stale since it has not had any activity in 90 days. If you would like to keep this PR alive, please ask a committer for review. If the PR has merge conflicts, please update it with the latest from trunk (or appropriate release branch) <p> If this PR is no longer valid or desired, please feel free to close it. If no activity occurrs in the next 30 days, it will be automatically closed.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org