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

Allow injecting authentication mechanisms #1101

Closed
wants to merge 10 commits into from

Conversation

Nevon
Copy link
Collaborator

@Nevon Nevon commented May 13, 2021

This allows users to inject their own custom authentication mechanisms, similar to what's done for compression codecs.

const { AuthenticationMechanisms } = require('kafkajs')
const { Mechanism, Type } = require('customauth')

AuthenticationMechanisms[Type] = () => Mechanism

The existing authentication mechanisms use the same interface, which meant I had to rework them some. Primarily, I moved them from being classes to functions. This isn't strictly necessary, but I find it easier to have people implement a function than to have them extend a class or similar - especially if they are using different compile-to-js languages.

The other thing was that I changed the interface to not expose any internal classes. That means moving from Encoders/Decoders to just buffers, and to not pass in the Connection anymore, and instead provide the sasl options and the broker host + port.

Fixes #840

Nevon added 2 commits May 13, 2021 19:40
AuthenticationMechanisms are now exported from the top level, and allows users
to add their own custom implementations as needed.
@Nevon Nevon requested review from tulios and JaapRood May 13, 2021 17:47
@Nevon
Copy link
Collaborator Author

Nevon commented May 14, 2021

Tested out and documented the typescript usage. The documentation has been updated accordingly.

@jvincilione
Copy link

It seems this change has not had any updates and has not been merged in a while. @Nevon will this end up getting added? I'm going to be using Kerberos, so I could definitely use this...

@Nevon
Copy link
Collaborator Author

Nevon commented Jul 15, 2021

I think it should definitely end up getting added, as there are already several use-cases we know of where this is necessary (aws-iam-msk & kerberos), but since this is a public API that people will be building on top of, we have to make sure that the interface works and makes sense.

I tried implementing aws-iam-msk authentication on top of this interface to test it out, and ran into some rough edges when using Typescript. It was a few weeks ago now, so the details a fussy, but from what I recall the issues were around making sure that if a user adds a new authentication mechanism, the options they pass in to sasl should still be typechecked according to the types of the custom authentication mechanism. I expected this to be doable using module augmentation of the AuthenticationMechanisms type, but I ran into some issues there, as seen by this example usage:

import { Kafka, AuthenticationMechanisms, AuthenticationMechanism } from 'kafkajs'
import { MskIamMechanism } from './MskIamMechanism'

// Augmenting the SASLMechanismsOptionsMap with the new mechanism
declare module 'kafkajs' {
  interface SASLMechanismOptionsMap {
    AWS_MSK_IAM: {
      credentialProvider: CredentialProvider
    }
  }
}

/**
 * Here's the ugly part where in order to be able to actually add the custom authentication mechanism
 * to the mechanisms, I have to cast AuthenticationMechanisms to my own custom type, which first
 * requires casting to unknown. This has to be done by the user, which sucks, but it's the only way I've found
 * that allows `sasl` to use the type provided by the custom mechanism.
 */
type ExtraAuthenticationMechanisms = AuthenticationMechanism & {
  AWS_MSK_IAM: () => typeof MskIamMechanism
}
;(AuthenticationMechanisms as unknown as ExtraAuthenticationMechanisms)['AWS_MSK_IAM'] = () =>
  MskIamMechanism

// On the bright side, now when I select `AWS_MSK_IAM` as a mechanism, it tells me I need to provide the
// "credentialProvider" property and what type it should have
const kafka = new Kafka({
  brokers: [
    'b-1.fake-test-broker.abc123.kafka.eu-north-1.amazonaws.com:9098',
  ],
  ssl: true,
  sasl: {
    mechanism: 'AWS_MSK_IAM',
    credentialProvider: defaultProvider(),
  },
  connectionTimeout: 5000,
})

My suggestion would be to attempt to implement Kerberos authentication on top of this interface, and then come back with concrete feedback on what worked well, what didn't, and what the user experience will be like. Maybe we can make some improvements to make it less of a hassle for the user.

The current implementation should work just fine for the built-in authentication mechanisms, so as a pure refactor of the existing functionality, I consider it good enough. But before we cut a stable release containing this, I'd like to improve the interface. Maybe @tulios could help out with a review so that we can get this merged in the current state, and then refactor the plugin interface once we have some feedback to work from.

@blacha
Copy link

blacha commented Aug 13, 2021

I am wanting to run a few nodejs lambda's against a MSK Kafka with IAM auth enabled, so I am very interested in this pull request.

I am wondering if a slightly different approach would make the new API slightly easier to type.

Would it be possible to instead of using a JSON config, passing a instantiated Mechanisim class into the client

const kafka = new Kafka({
  brokers: [
    'b-1.fake-test-broker.abc123.kafka.eu-north-1.amazonaws.com:9098',
  ],
  ssl: true,
  sasl: new AwsMSKIamMechanisim({ credentials } ),
  connectionTimeout: 5000,
})

This would simplify the type definitions as you could do:

export type sasl = SASLOptions | Mechanisim

This concept is similar to how the aws elstaic search connector works, where you are allowed to override the Transport and Connection classes in the constructor of the client ref: https://github.com/compwright/aws-elasticsearch-connector

@ultrarunner
Copy link

ultrarunner commented Sep 21, 2021

Hello, I just have a question about this PR regarding the ability to implement a custom auth mechanism to set the username / password based on an async call. Our kafka servers have ssl in place but we need to retrieve the username / password from a vault since they cannot be stored in the stash repo, very similar to the way the "oauthBearer" option is implemented. Below would be a very crude idea (interface and type taken from the index.d.ts file). Would the above PR allow for this? Thank you.

export interface PlainProviderResponse { username: string, password: string }

type SASLMechanismOptionsMap = {'plainprovider': { plainProvider: () => Promise<PlainProviderResponse> }

@damaral-Fuze
Copy link

Hi, any chance of this PR being merged?

@s4bdufabricio
Copy link

@Nevon I would like to use this feature ! :)

@ludov04
Copy link

ludov04 commented Dec 21, 2021

This would be immensely useful

@amitruls1
Copy link

Hi, @Nevon When can we expect this to be released? Thanks.

@leoddias
Copy link

@tulios @JaapRood @Nevon Does anybody know when it will be released?
Any alternatives so far?
Thanks

@Nevon
Copy link
Collaborator Author

Nevon commented Feb 23, 2022

It'll happen when I have time to work on it. These things don't happen by magic, but by someone spending the hours of work to make it happen, and right now my priority is #1258. 🙂

I like @blacha's suggestion and will likely refactor this in that direction once I have time to come back to this.

@terrancej
Copy link

bump

@Nevon
Copy link
Collaborator Author

Nevon commented Apr 12, 2022

I'm going to close this PR since it's not particularly useful at the moment. The branch will remain, in case someone wants to take a crack at reworking it.

I spent some time last week trying to refactor it with a better interface, and also make the necessary adjustments from the changes to the Connection that are in master, but didn't finish. One thing to note about blacha's suggestion is that the authentication mechanism needs to expose a name that we can use in the protocol negotiation, so the sasl interface would have to be something like { mechanism: string; authenticate: (stuff: { saslAuthenticate, aFewOtherThingsICantRemember: ...unknown }) => Promise<void> } (as a union with the existing interface, for backwards compatibility).

@Nevon Nevon closed this Apr 12, 2022
@ro-ka
Copy link

ro-ka commented May 18, 2022

Please keep the branch available, we’re using that branch already successfully in our project. 😬 Hope there will be a final solution someday in the main branch. 🤞 Thanks for your work here!

@Nevon
Copy link
Collaborator Author

Nevon commented May 18, 2022

That seems incredibly dangerous. I would suggest that you fork the repo so that you have your own copy of the branch, rather than rely on me not clicking that "Delete branch" button.

@antonwintergerst
Copy link

FYI, @markgaylard has completed the work to allow custom authentication mechanisms and has raised a PR here: #1372

@Nevon
Copy link
Collaborator Author

Nevon commented Jul 7, 2022

This is available now in a beta release. See #1372 (comment) for details.

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.

Allow for injecting authentication mechanisms