Assume token value without prefix in password if tokenEndpointUri is not set

[elakito]

Hi, This is a follow up to the slack channel thread regarding the oauth bearer token over SASL_PLAIN https://cloud-native.slack.com/archives/CMH3Q3SNP/p1616692769193700

Regarding the way how the two variants (i.e., username+password and username+token over PLAIN are distinguished) and how this was changed between 0.7.0 and 0.7.1, I would like to propose a minor change to it to make the processing more flexible to support more clients.

The proposal is to make tokenEndpointUri be required only when the username + password combination is sent over PLAIN to require a token lookup. When tokenEndpointUri is absent, the password is assumed to be a token. In this way, the broker can support the following cases

• The current 0.7.1 case where tokenEndpointUri is set and the password value is checked against prefix $accessToken to switch between the username+password or username+token handling
• The case where tokenEndpointUri is not set and the client is sending a token as password without prefixing it with $accessToken.

I tested this change with a modified strimzi-operator which allows an empty tokenEndpointUri. I would appreciate for your comments.

Thanks, aki

Signed-off-by: Aki Yoshida elakito@gmail.com

[mstruk]

This is a tough one. Currently, the client has a choice of using either clientId + secret or userId + access_token. After this change, the client can make this choice only if tokenEndpointUri is set. But if the server is misconfigured or deliberately disabled for clientId and secret (by not having tokenEndpointUri configured), the clientId + secret would not work.

On the other hand would it be of some value to someone to enable PLAIN and limit it to authentication with access tokens only, maybe so that the clients never store clientId and secret in their configurations, or send it to the server? Your specific motivation is backwards compatibility with 0.7.0 but full compatibility is not really possible, as you have to set a different username. And if you can set password to $ACCESS_TOKEN via configuration, why can't you set it to '$accessToken:'$ACCCESS_TOKEN?

My concern is that this will make troubleshooting authentication issues more difficult since there's now another variable in the equation that can very easily be flipped accidentally (by omission). Maybe if we add another server-side config oauth.disable.client.credentials.over.plain that would have to be set to true when tokenEndpointUri is not set? That would also require adding it to 'oauth' authentication in Strimzi Operator as disableClientCredentialsOverPlain.

We would also need additional documentation to explain the different behaviour.

But then is this really a feature anyone is looking for?

I don't know, what do others think?

[elakito]

@mstruk I have custom clients written in node and java that use a token over PLAIN and and these clients will be able to interact with strimzi if there is a way to configure strimzi to accept non-prefixed tokens. These clients have been used for some years and I have no control.

Regarding the poetical difficulty in troubleshooting, if the authentication error is reported, I don't think adding this fallback behavior (i.e., when tokenEndpointUri is not set, the password is taken as a token) would make the troubleshooting harder, wouldn't it?

Besides the use of prefix $accessToken within the password value could already introduce a confusion, as the intended password value won't get interpreted as password if it indeed starts with $accessToken. In that sense, providing a configuration option to only enable username+token actually will reduce the potential difficulty in troubleshooting.

I hope this explains the motivation behind this change and also answers your concern. regards, aki

[mstruk]

@scholzj @tombentley @ppatierno Any strong opinion about this?

[scholzj]

@mstruk TBH, as long as it does not brake backwards compatibility, I do not have a strong opinion. You are the expert here I guess :-).

In any case, I think it needs to have some tests because that is the only way how we can ensure this does not break later.

[elakito]

I would like to mention that I will be more than happy to work on the tests and the documentation.

[mstruk]

Actually there are tests in the testsuite for current functionality. We just have to add another test to make sure the client credentials will work without $accessToken: prefix when tokenEndpointUri is not configured. And some corrections to the docs yes.

Integration into Strimzi Operator will require an extra PR as well.

[scholzj]

Actually there are tests in the testsuite for current functionality. We just have to add another test to make sure the client credentials will work without $accessToken: prefix when tokenEndpointUri is not configured. And some corrections to the docs yes.

Right, so the test for this here should be added in this PR I guess.

[mstruk]

I created another PR for this where I added the tests, the documentation updates, and some additional logging.

@elakito Can you review that one?

[elakito]

@mstruk I looked at the updated PR. It looks good. I created a PR for strimizi-kafka-operator https://github.com/strimzi/strimzi-kafka-operator/pull/4746 to use this feature. thanks

[scholzj]

So, should we close this PR if it was replaced by #107?

[mstruk]

Yes.