-
-
Notifications
You must be signed in to change notification settings - Fork 527
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
Conversation
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. |
One thing I think this is missing is that the return type of |
Hi, @Nevon When can we expect this to be released? Thanks. |
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. |
…f response to decide instead
@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 :) |
Hi, I am really anxious to be able to use this. Do you know when it might be merged? Thanks |
@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 I will commit to doing another pass at reviewing this tomorrow. |
There was a problem hiding this 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.
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 }) => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
kafkajs/src/broker/saslAuthenticator/scram.js
Lines 171 to 174 in 4246f92
return this.saslAuthenticate({ | |
authExpectResponse: true, | |
request, | |
response, |
You need to wait for some responses to continue with the handshake.
There was a problem hiding this comment.
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.
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. |
@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 |
@michael-hardeman Did you have any luck implementing the GSSAPI mechanism? I myself have spent a great deal of time in the implementation but couldn't get it to work. I went as far as issuing a ticket, encoding it but always get an "Invalid credentials" error. @Nevon Can you please point us in the right direction on how to implement GSSAPI mechanism in KafkaJS? |
No. I had issues as well and there was a severe lack of documentation on kerberos auth and, at the time, the authentication portion here. I ended up taking a different route in design instead. |
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 becomesFixes #840