Implement SASL/OAUTHBEARER support

[mk6i]

Hello,

This commit implements SASL/OAUTHBEARER client authentication as described in KIP-255 and issue #1223.

To get started, the user must set c.Net.SASL.Mechanism: OAUTHBEARER and assign a user-defined token generator to config.Net.SASL.TokenProvider. For backwards compatibility, a blank value for c.Net.SASL.Mechanism defaults to PLAIN.

Example:

type TokenProvider struct {
}

func (t *TokenProvider) Token() (*sarama.AccessToken, error) {
	return &sarama.AccessToken{Token: "access-token-string"}, nil
}
// ...
config.Net.SASL.Enable = true
config.Net.SASL.Mechanism = sarama.SASLTypeOAuth
config.Net.SASL.TokenProvider = &TokenProvider{}

cc @ggrcha

[mk6i]

Working on unit test to improve test coverage

[varun06]

@mkaminski1988 PR is looking great now. But you might have to add some more tests for codecoverage to pass.

[mk6i]

Cool, tests in progress. I also mentioned this PR to @rondagostino, who implemented SASL/OAUTHBEARER in Kafka. He said he'll take a look once he gets back from vacation after the new year.

[varun06]

Makes sense and Thanks a lot for all the work. Very excited for it.

[mk6i]

@rondagostino I've implemented extensions. Couple of questions:

1. Is it necessary for the client to send an authzid header? I see that it's application-dependent according to RFC-7628. If I'm understanding the Kafka Java SASL auth code correctly, the broker can deserialize the authorization ID if the client sends one. The Java Kafka client, however, doesn't appear to send one. I'm assuming that was done to keep the Kafka SASL server compliant with RFC-7628?

2. In this implementation, the SASL extensions are assigned to the client configuration and remain static for the duration of the client. This seems to mirror the behavior of the Java client, where the extensions are set in the JAAS configuration. Am I correct in my assumption that the SASL extension parameters shouldn't change after initial configuration?

[rondagostino]

@mkaminski1988 Yes, you are correct about the authzid -- it is optional as per the spec, and the Java client currently does not send one. The broker does check for it in case it is there, and it ensures that any authzid matches the principal name as defined by the token.

With regard to any extensions that the Java client sends to the broker, it is implementation-dependent whether they are constant across token retrievals. The login callback handler is tasked with retrieving the extensions, and the default is for there to be no extensions if the login callback handler doesn't know how to handle SaslExtensionsCallback. The unsecured login callback handler does look for extensions in the JAAS config as you point out, but that of course can't be used in production, so it falls to the production login callback handler implementation that is plugged in to decide what (if anything) to do with SaslExtensionsCallback when it sees it (and it can throw UnsupportedCallbackException since that will be caught and empty extensions will be injected by default).

[mk6i]

@rondagostino Thanks for that explanation. I think for this implementation it would be better to create a new ExtensionsProvider interface that the user can optionally implement in order to dynamically retrieve extensions, as opposed to hard-coding them in the config.

[mk6i]

Upon further investigation I've decided to make the TokenProvider .Token() method return a struct that contains both the access token and SASL extensions instead of a string, which gives flexibility for future changes.

[mk6i]

@varun06 This is ready to merge.

[varun06]

🕺

This is great @mkaminski1988 . I am going to have a look and may be @bai or @jprobinson can also have one more look. This is a big change and thanks a lot of all the work.

[jprobinson]

It may be just the style of the project, but I've noticed there's no use of "sub tests" via the t.Run() method: https://golang.org/pkg/testing/#T.Run

This would allow users to run individual tests cases within table tests from the CLI and also prevent the need to prefix all Errorf messages with a test.name.

Just something to consider.

[mk6i]

Good to know about t.Run(). I think I'll match the style of the project, unless the maintainers would like it otherwise.

[jprobinson]

I just called out a few style things but other than that the logic looks solid.

Great work, @mkaminski1988! 🌮!

[mk6i]

Thanks for your expertise @jprobinson!

[bsvingen]

This is great, would very much like to see this merged.

[varun06]

@bai What is the strategy to get this merged. It's a big change and might need a release?

[bai]

Thanks for getting this done, amazing work! I'm going to merge this one now and will compile changelog and do a release this week.

[mk6i]

My pleasure, thanks for your help everyone!

[bai]

Sorry for the noise here, just to clarify: 1.20.1 with deadlock fix is coming up in a moment, this one will go into 1.21.

[mk6i]

@bai Do you have an idea of when the next release will be cut?

[bai]

I intend to cut it tomorrow or Friday latest.