ZOOKEEPER-2649 - Add more session and authentication information to S… by Ghatage · Pull Request #1498 · apache/zookeeper

Ghatage · 2020-10-12T22:29:56Z

…ASL success and failure logs.

Another one from the backlogs - created in 2016, no activity since 6 months.

JIRA: https://issues.apache.org/jira/browse/ZOOKEEPER-2649

cc @eolivelli

…ASL success and failure logs

Ghatage · 2020-10-12T23:00:10Z

Needs more work, nvm. Will close, rework and open a new PR later.

ztzg · 2020-10-14T13:50:33Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java

                if (saslServer.isComplete()) {
                    String authorizationID = saslServer.getAuthorizationID();
-                    LOG.info("adding SASL authorization for authorizationID: {}", authorizationID);
+                    LOG.info("Session 0x{}: adding SASL authorization for authorizationID: {}",


+1.

I'd like to have this in the tree, and was about to submit a patch with something like this.

(Arguably, it'd be sufficient to augment logging here and not on the other path of ZooKeeperServer, as the "pluggable" authentication providers can and should do their own, "domain-specific," logging.)

@Ghatage: are you planning to give this another go?

ztzg · 2020-10-14T13:54:06Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java

+                ZooKeeperSaslServer saslServer = cnxn.zooKeeperSaslServer;
                LOG.debug("Authentication succeeded for scheme: {}", scheme);
-                LOG.info("auth success {}", cnxn.getRemoteSocketAddress());
+                LOG.info("Session 0x{}: auth success for authorizationID={}/{}",
+                        Long.toHexString(cnxn.getSessionId()), saslServer.getAuthorizationID(),
+                        cnxn.getRemoteSocketAddress());


This is the "traditional" authentication path, used by providers others than SASL. Unless I am missing something, saslServer is likely to be NULL here.

(I suppose this is the reason for your "Needs more work" comment.)

That's right @ztzg.
I was just helping clear out the backlog when I took this up but I think I'll need to add in more code so took a breather on this.
I would still like to give it a shot till the weekend if you don't mind. Will tag you for updated codes' review :)

Sure; no hurry/pressure. Was just checking as I was working in the same area…

@ztzg I added some code to protect the use of sasl in the log statement only if its not null.
Not sure if that is sufficient / appropriate.. please advise!

Hi @Ghatage,

No, I don't think that's what we want—sorry for not being clearer in my previous comment.

This is the code path where "traditional" auth packets are being handled. While it is technically possible to send a sasl auth packet to a server, the handleAuthentication method of SASLAuthenticationProvider always returns AUTHFAILED. So you'll never enter that block in response to SASL auth.

Now, while I think it would be good to add at least the sessionId and scheme to that message, adding the "user name" corresponding to a traditional AuthenticationProvider is quite a bit more complicated—and doing it "very cleanly" would arguably require incompatible infrastructure changes.

(For example, DigestAuthenticationProvider can add up to two Ids to a connection, and the existing interface does not allow it to return them; you'd have to "diff" the before and after Id sets to know what has changed.)

So I'm not sure it's worth the effort, given that everybody is moving towards SASL and/or X509 anyway. And even if one wants to keep using a traditional provider, it is usually more useful to let it do its own (domain-specific) logging.

In conclusion: I'd just add the sessionId and scheme there, and ignore the actual Id(s).

Hope this helps, -D

Cc: @eolivelli & @symat.

In conclusion: I'd just add the sessionId and scheme there, and ignore the actual Id(s).

make sense to me.
(for SASL I think we already have log outputs about the authenticated principal names)

Ghatage · 2020-10-22T15:55:25Z

Thank you for the detailed explanation, @ztzg!
I've made the recommended changes, PTAL and let me know if that's appropriate.

cc @eolivelli, @symat

ztzg · 2020-10-22T17:47:36Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java

            if (authReturn == KeeperException.Code.OK) {
-                LOG.debug("Authentication succeeded for scheme: {}", scheme);
-                LOG.info("auth success {}", cnxn.getRemoteSocketAddress());
+                ZooKeeperSaslServer saslServer = cnxn.zooKeeperSaslServer;


Almost… but I'm afraid you still have a leftover saslServer variable here :)

Sorry about that! Fixed it now.

No problem! LGTM.

Ghatage · 2020-10-22T19:27:00Z

@eolivelli @symat PTAL

eolivelli

LGTM

symat

LGTM, +1

ztzg · 2020-10-28T15:32:49Z

Merged into master. Thank you for this and #1497, @Ghatage!

Ghatage · 2020-10-28T15:36:12Z

Thanks for the guidance and accepting my contributions @ztzg and @symat.
I absolutely love the project and hope to make more contributions going forward!

…SL success and failure logs …ASL success and failure logs. Another one from the backlogs - created in 2016, no activity since 6 months. JIRA: https://issues.apache.org/jira/browse/ZOOKEEPER-2649 cc eolivelli Author: Ghatage <ghatageanup@gmail.com> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mate Szalay-Beko <symat@apache.org>, Damien Diederen <ddiederen@apache.org> Closes apache#1498 from Ghatage/ZOOKEEPER-2649

ZOOKEEPER-2649 - Add more session and authentication information to S…

f0923ea

…ASL success and failure logs

Ghatage closed this Oct 12, 2020

ztzg reviewed Oct 14, 2020

View reviewed changes

Only print added information if using SASL

f0dd94a

Ghatage reopened this Oct 17, 2020

Change log message to only use scheme instead of sasl details

f7d445a

ztzg requested changes Oct 22, 2020

View reviewed changes

Remove leftover variable

72569f2

ztzg approved these changes Oct 22, 2020

View reviewed changes

eolivelli approved these changes Oct 22, 2020

View reviewed changes

symat approved these changes Oct 24, 2020

View reviewed changes

ztzg closed this in 8853b33 Oct 28, 2020

Conversation

Ghatage commented Oct 12, 2020

Uh oh!

Ghatage commented Oct 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ztzg Oct 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ghatage commented Oct 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ghatage commented Oct 22, 2020

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

symat left a comment

Choose a reason for hiding this comment

Uh oh!

ztzg commented Oct 28, 2020

Uh oh!

Ghatage commented Oct 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ztzg Oct 19, 2020 •

edited

Loading