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

Ability to use custom authenticators #1372

Merged
merged 18 commits into from Jul 7, 2022
Merged

Conversation

markgaylard
Copy link
Contributor

@markgaylard markgaylard commented May 25, 2022

Starting from #1101, this is a rework to allow custom authentication mechanisms to be written based on the discussions in that PR around the desired interface for custom authentication mechanisms.

To use a custom authenticator, the sasl object becomes

{ 
      mechanism: string; 
      authenticationProvider: ( host: string, port: number, logger, saslAuthenticate) => { authenticate: () => Promise<void> }
}

Fixes #840

@markgaylard markgaylard changed the title Pluggable auth Ability to use custom authenticators May 25, 2022
types/index.d.ts Outdated Show resolved Hide resolved
@markgaylard markgaylard marked this pull request as ready for review May 25, 2022 10:10
@Nevon
Copy link
Collaborator

Nevon commented Jun 1, 2022

I haven't updated the existing authenticators at this stage as was done in the previous PR, but I can if that is desired.

When I was working on this, I found that to be the best way to test out the interface and how awkward or not awkward it was to use it, so I would highly recommend doing that.

@Nevon
Copy link
Collaborator

Nevon commented Jun 1, 2022

One thing I think this is missing is that the return type of SaslAuthenticateRequest["encode"] is Encoder, not Buffer as it says in the type definition here. Encoder is not exported and part of the public API, which is why in #1101 I changed that to instead return a Buffer. From what I recall it was a rather simple change, just updating the SaslAuthenticator to expect a Buffer instead of Encoder, and then returning the buffer from the encoder in each of the built-in mechanisms.

@mehul-sg
Copy link

mehul-sg commented Jun 2, 2022

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

@Nevon
Copy link
Collaborator

Nevon commented Jun 2, 2022

When can we expect this to be released?

Once it's ready. I will be going on vacations tomorrow, however, and won't be back until June 25th, so very unlikely to be before then.

@markgaylard
Copy link
Contributor Author

@Nevon I think I'm done with this now. I've ported all the changes from your PR to this one. I'm glad you had done most of the hard work otherwise this would have taken me a lot longer :)

@christianpurple
Copy link

Hi, I am really anxious to be able to use this. Do you know when it might be merged? Thanks

@Nevon
Copy link
Collaborator

Nevon commented Jun 28, 2022

@christianpurple: If you are in a rush to use code that has not been merged, you can always use a branch reference by changing your dependency to "kafkajs": "markgaylard/kafkajs#pluggable-auth". Like I said before, it will be merged when it is ready, and when I've had the time to properly review it and both me and @markgaylard are happy with it.

I will commit to doing another pass at reviewing this tomorrow.

Copy link
Collaborator

@Nevon Nevon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, and tests are passing, so it seems to be working well. The only thing I think is important to do before merging is to turn the arguments of the authenticationProvider into an object instead of positional arguments, to make it easier to evolve the interface if needed.

docs/CustomAuthenticationMechanism.md Show resolved Hide resolved
const handshake = await this.connection.send(this.saslHandshake({ mechanism }))
if (!handshake.enabledMechanisms.includes(mechanism)) {
throw new KafkaJSSASLAuthenticationError(
`SASL ${mechanism} mechanism is not supported by the server`
)
}

const saslAuthenticate = async ({ request, response, authExpectResponse }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why authExpectResponse existed? It makes sense to me that we expect a response if we're given a way to decode responses, and not otherwise, but it seems a bit silly that we didn't notice that from the start.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having read through the original PR (#72) again, I don't think there was a good reason.

Copy link
Owner

@tulios tulios Jun 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Nevon It's used as part of the SCRAM handshake

return this.saslAuthenticate({
authExpectResponse: true,
request,
response,

You need to wait for some responses to continue with the handshake.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, no I get what it was being used for, just not if there was a good reason to not just use the presence of response to determine whether or not to wait for a response (like if there would be a response that we didn't want to decode, for some weird reason), but that doesn't seem to be the case.

src/broker/saslAuthenticator/index.js Outdated Show resolved Hide resolved
docs/CustomAuthenticationMechanism.md Outdated Show resolved Hide resolved
docs/Configuration.md Outdated Show resolved Hide resolved
@Nevon Nevon merged commit 7e9970b into tulios:master Jul 7, 2022
@Nevon
Copy link
Collaborator

Nevon commented Jul 7, 2022

Thanks for the contribution. Let's get some people to try out this new interface before we make a stable release out of it, to see if we missed anything in how this works.

@michael-hardeman
Copy link

michael-hardeman commented Aug 25, 2022

@Nevon This is a great feature.

I'm trying to use this new feature to implement a GSSAPI mechanism. I've looked at https://github.com/adaltas/node-krb5 and https://github.com/mongodb-js/kerberos for native kerberos bindings but at initial glance neither of them seem suited for your custom auth mechanisms? in node-krb5 I can get a ticket granting ticket and spnego token and it says I should add it as an Authorization: Negociate <token> header on all http requests to services similar to a bearer token but I don't really see how that would be done. Do you have any pointers?

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
6 participants