Skip to content

MINOR: Add DEBUG level logs for successful/failed authentications wit…#5856

Merged
ijuma merged 3 commits intoapache:trunkfrom
stanislavkozlovski:log-auth-failure-ips
Jan 9, 2019
Merged

MINOR: Add DEBUG level logs for successful/failed authentications wit…#5856
ijuma merged 3 commits intoapache:trunkfrom
stanislavkozlovski:log-auth-failure-ips

Conversation

@stanislavkozlovski
Copy link
Copy Markdown
Contributor

I believe that it'll be useful for debugging purposes to know which IP addresses are consistently failing authentication.
Applications that consistently fail authentication can introduce CPU pressure to the broker and as such, I think it is useful for Ops teams to have the needed information to quickly block.

I've left these logs on DEBUG level since I presume that most common cases won't require to know this information. I'm sure we shouldn't log successful auths in INFO but I'm hesitant about unsuccessful auths

cc @rajinisivaram @ijuma @rondagostino

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Oct 31, 2018

I think failed authentications should be info given that they're pretty expensive for people using TLS which is very common.

@omkreddy
Copy link
Copy Markdown
Contributor

Related JIRA for server side logging: https://issues.apache.org/jira/browse/KAFKA-5810

channel.prepare();
} catch (AuthenticationException e) {
if (channel.successfulAuthentications() == 0)
boolean isReAuthentication = channel.successfulAuthentications() > 0;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: capitalization isn't quite right, see reauthenticationLatencyMs for example.

sensors.failedAuthentication.record();
else
sensors.failedReauthentication.record();
log.debug("Address {} failed {}authentication ({})",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per my PR comment, I think this should be info.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I must have missed it. As I stated originally, I was hesitant on what it should be. I'll change it to info

@stanislavkozlovski
Copy link
Copy Markdown
Contributor Author

@omkreddy thanks for sharing that JIRA. It has the very reasonable suggestion of splitting the auth logs in log4j for configuration.
I don't want to change this PR for that since it would require a lot more changes, inspection and work. I'll put that JIRA in my backlog and get to it eventually if nobody beats it to me

if (!isReauthentication)
sensors.failedAuthentication.record();
else
sensors.failedReauthentication.record();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we swap the if/else so that we don't have to negate the first if? That seems unnecessarily confusing.

@ijuma ijuma merged commit 66a9416 into apache:trunk Jan 9, 2019
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
…pache#5856)

Use `info` for failed authentications and `debug` for successful ones.

Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com>, Ismael Juma <ismael@juma.me.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants