Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Oauthbearer support #680

Merged
merged 21 commits into from
Jul 20, 2020
Merged

Oauthbearer support #680

merged 21 commits into from
Jul 20, 2020

Conversation

uwburn
Copy link
Contributor

@uwburn uwburn commented Mar 28, 2020

#674

Implemented the SASL OAUTHBEARER request, the reauthentication mechanism seems to be handled automatically by already present isConnected() check in src/broker/index.js, as it evalueates the sessionLifetime that is replied back from Kafka when proper options (connections.max.reauth.ms) are configured in the broker.

A base spec ha been introduced to check the most obvious errors, more elaborate testing seems difficult to set up, as requires plenty of configuration on the broker.

Some tests are failing, but they do seem unrelated.

Updates to the documentation are missing at the moment.

@uwburn
Copy link
Contributor Author

uwburn commented Mar 30, 2020

I wrote a test using for OAUTHBEARER login, but the problem is that Kafka fails to start with exception java.lang.IllegalArgumentException: Must supply exactly 1 non-null JAAS mechanism configuration (size was 3) when configured to use the embedded OAUTHBEARER handler.

Unfortunately i found other users having the same issue, by looking at the Java class at https://github.com/apache/kafka/blob/b8651d4e82d45463d2c71798bd5852f8a605b440/clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/unsecured/OAuthBearerUnsecuredValidatorCallbackHandler.java it looks like it doesn't properly lookup the JAAS entry but just checks that there's only one and pick it, so it gets confused by PLAN and SCRAM entries. I also tried declaring a dedicated listener for the test but it still refuse to start with the same error.

I opened a branch oauthbearer-support-test in my fork if you want to check it. From it you can manually check that it works, but you have to remove non OAUTHBEARER SASL mechanisms and remove non OAUTHBEARER entries from KafkaServer JAAS section. Should i include the test keeping it commented, in case in future we can use it?

In the meantime i added the docs.

@uwburn
Copy link
Contributor Author

uwburn commented May 10, 2020

Do you have any plan for merging the PR and having the funcionality available in GA releases?

@uwburn
Copy link
Contributor Author

uwburn commented May 26, 2020

Rebased.

@Nevon
Copy link
Collaborator

Nevon commented May 29, 2020

Do you have any plan for merging the PR and having the funcionality available in GA releases?

Sorry about the delay. It seems all the maintainers, including myself, are busy with life and work lately.

Should i include the test keeping it commented, in case in future we can use it?

I think that sounds good, but rather than commenting it out, let's do what we do for running tests with different versions of Kafka:

const testIfKafkaVersion = version => (description, callback, testFn = test) => {
return semver.gte(semver.coerce(process.env.KAFKA_VERSION), semver.coerce(version))
? testFn(description, callback)
: test.skip(description, callback)
}
const testIfKafka_0_11 = testIfKafkaVersion('0.11')

So something like:

const testIfEnv = (key, value) => (description, callback, testFn = test) => {
  return value === process.env[key]
    ? testFn(description, callback)
    : test.skip(description, callback)
}

/**
 *  <Explanation why oauthbearer is conditionally enabled and how to enable it>
 */
const testIfOauthbearerEnabled = testIfEnv('OAUTHBEARER_ENABLED', '1')

Then you test would be just the following, and it would run if OAUTHBEARER_ENABLED is 1, and be skipped otherwise.

testIfOauthbearerEnabled('support SASL OAUTHBEARER connections', async () => {
  // ...
})

docs/Configuration.md Outdated Show resolved Hide resolved
@uwburn
Copy link
Contributor Author

uwburn commented Jun 3, 2020

Sorry about the delay. It seems all the maintainers, including myself, are busy with life and work lately.

Sorry, i was not meaning to be harsh, i am about to start a project in the company where i work that would include kafkajs, so i'm looking forward to avoid using git npm dependencies, but that's not really an issue.

Anyway, i followed your suggestion, also i rearranged a bit SASL tests to avoid code duplications.

By looking at it i noted that probably some other conditional tests about 1.1 and 0.11 must be rechecked, if i have some time to spare eventually i will submit another PR about that.

Not ideal, but because we have to scan for matching test names, the job
ends up taking 5+ minutes in CI. The tests could be reorganized to instead
move all OauthBearer tests into a single spec (or specs with a specific name)
to be able to use `testPathPattern` instead of `testNamePattern`, at
which point we could probably enable this again.
@Nevon
Copy link
Collaborator

Nevon commented Jul 20, 2020

I added a job to the pipeline that runs the Oauthbearer tests. It's configured to only run in master, as it takes more than 5 minutes to search through all tests for the SASL OAUTHBEARER ones. Not great, and could be fixed by reorganizing the tests so that all the oauthbearer ones are in files with a certain suffix or something, but I'm not gonna do that right now. I did run the job through CI once, and it passed.

@Nevon Nevon merged commit f8d13b9 into tulios:master Jul 20, 2020
@Palmy523 Palmy523 mentioned this pull request Sep 11, 2020
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.

2 participants