[authentication] Validate tokens for binary connections

[addisonj]

Motivation

Currently, binary connects aren't checked to see if they provide a
token.

This results in a NPE in the JWT validation as well as a whole bunch of
log spam.

Modifications

By explicitly checking for a null/empty token here, we can
avoid some exceptions and clean up log spam.

Verifying this change

• Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

• Dependencies (does it add or upgrade a dependency): no
• The public API: no
• The schema: n
• The default values of configurations: n
• The wire protocol: n
• The rest endpoints: n
• The admin cli options: n
• Anything that affects deployment:n

Documentation

• Does this pull request introduce a new feature? n
• If yes, how is the feature documented? not applicable
• If a feature is not applicable for documentation, explain why?
• If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

[sijie]

Interestingly, it seems that we don't have a test to catch that. @addisonj it is possible to add a test case for this.

[merlimat]

Currently, binary connects aren't checked to see if they provide a token.

So it results in NPE, though the client is still not being allowed in, right?

[addisonj]

correct. It fails in the JWT validation with an NPE and leaves a lot of chattery logs. I don't believe this will actually makes the logs be less noisy as it still throws an exception that is logged, but I figured this is still much better than an NPE

…

On Thu, Feb 6, 2020 at 6:05 PM Matteo Merli ***@***.***> wrote: Currently, binary connects aren't checked to see if they provide a token. So it results in NPE, though the client is still not being allowed in, right? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#6233?email_source=notifications&email_token=AAE3LA2D3VLZTA4GEIQIRT3RBSXURA5CNFSM4KQSLQKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELBLOSY#issuecomment-583186251>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE3LA4KRQQB6DN2ZBBMVEDRBSXURANCNFSM4KQSLQKA> .

[merlimat]

I don't believe this will actually makes the logs be less
noisy as it still throws an exception that is logged,

It should, since we don't print the full stack trace in case of an AuthenticationException (since it's a "known" failure mode).

[merlimat]

Interestingly, it seems that we don't have a test to catch that. @addisonj it is possible to add a test case for this.

It might be tricky to test since the connection gets rejected. Maybe a way would be to check the exception type when creating a producers (eg: AuthenticationError vs Timeout)

[addisonj]

@merlimat it still results in an AuthenticationError because it gets caught by this try/catch here:

pulsar/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java

Line 124 in 8e599fd

throw new AuthenticationException("Failed to authentication token: " + e.getMessage());

But if that is how it should behave, that would be great and much less noisy

[sijie]

@merlimat can you check the last comment?

[sijie]

@merlimat @addisonj any thoughts about this?

[addisonj]

@sijie I just say we merge it, it should at least give a more clear error message, but we can always clean up the logs more later