Update brod_gssapi_v1 to use sasl_auth 2.0 #5

starbelly · 2022-05-07T18:21:52Z

no longer define error codes and instead depend on atoms provided by sasl_auth 2.0
bump version
Remove call to list mechanisms as there isn't a need unless since we always want gssapi.

Co-authored-by: Vikas Bhardwaj [email protected]

Notes

Depends on Rewrite sasl_auth nif sasl_auth#8
Changes were kept to a minimum, there's a lot of refactoring to be done in the v1 module though IMO, it's a bit hard to follow right now.
There are still no tests. We could do what was in done in sasl_auth v2 (i.e., tests that depend on a kdc and kafka instance). Long term some mock/fake servers would serve as well as far as being able to run simple integration tests.
It may be we want to just make the v1 module the default (i.e., just overwrite brod_gssapi.erl) since it won't even work with pulling in sasl_auth v2. It may be that we want to update that module to use sasl_auth v2, @vikas15bhardwaj Might have thoughts on this.
An improvement that could be made would be to put the list mechs call back in, but verify that gssapi is in there.

vikas15bhardwaj · 2022-05-10T13:24:40Z

thanks @starbelly for putting this out here.
On your point if we should make v1 default: here are my thoughts:

brod_gssapi_v1 was added to support Kafka protocol version 1 and above. In my view, we should have one brod_gssapi plugin which should work for old Kafka protocols (older 0.0.9) or 1+. Users shouldn't be aware of this difference. Internally main plugin can bifurcate to either v1 or v0... depending on Kafka version it's dealing with. I think regardless of Kafka protocol sasl_auth version should be same

starbelly · 2022-05-10T16:02:27Z

thanks @starbelly for putting this out here. On your point if we should make v1 default: here are my thoughts:

brod_gssapi_v1 was added to support Kafka protocol version 1 and above. In my view, we should have one brod_gssapi plugin which should work for old Kafka protocols (older 0.0.9) or 1+. Users shouldn't be aware of this difference. Internally main plugin can bifurcate to either v1 or v0... depending on Kafka version it's dealing with. I think regardless of Kafka protocol sasl_auth version should be same

That makes sense. So we'll need to update the main plugin and test it.

Edit:

I'll also meck out the nif calls and so forth so we can at least test the login in the modules.

starbelly · 2022-05-22T19:42:50Z

What's needed is the handshake vsn passed in from brod to the backend auth plugin. Should we send a patch upstream or not let perfect be the enemy of good here?

I'm also taking a stab at refactoring both modules since they are hard to follow. I can tuck those changes to the side, but imo that can will just keep getting kicked down the road.

@kjellwinblad @zmstone Any thoughts here?

starbelly · 2022-05-24T14:09:01Z

src/brod_gssapi_v1.erl

+ensure_binary(Bin) when is_binary(Bin) ->
+    Bin.
+
+maybe_interact_continue(sasl_interact) ->


This needs to be removed all together. We should never hit sasl_interact. I think this was originally based off librdkafka it seems. librdkafka may have a case for interacting with a user (i.e., gui or cli app), but we have none or rather AFAIK we have no way of interacting with the user currently.

When you provide callbacks to cyrus-sasl for additional information that may needed, you're opting out of getting sasl_interact back as a code AFAIK. What's more even if you do get that back at client step, you're supposed to go back to calling client start after you get the information asked for.

kjellwinblad · 2022-05-25T09:00:08Z

What's needed is the handshake vsn passed in from brod to the backend auth plugin. Should we send a patch upstream or not let perfect be the enemy of good here?

Unfortunately, I have not been able to understand how this plugin works well enough yet to have an intelligent opinion. However, I think a patch to broad that sends the version to the plugin sounds like a good idea, if it is needed so that users don't need to configure what version to use manually. It is probably good if it is possible to do the change so other authentication plugins don't need to be changed. However, I'm not sure if that is a big deal since I'm don't know which other authentication plugins exists. @zmstone maybe you know more about this?

I'm also taking a stab at refactoring both modules since they are hard to follow. I can tuck those changes to the side, but imo that can will just keep getting kicked down the road.

I think this sounds good as it is probably something that will pay off in the long run.

My current plan is to spend some more time with the sasl_auth repository to make sure it is production ready (maybe add some more test cases and study the code to make sure I can't find anything that is broken). After that, I plan to add test cases to this repo as well (probably by utilizing docker-compose to run brod, kafka and a Kerberos KDC in different Docker containers).

zmstone · 2022-05-25T09:15:35Z

What's needed is the handshake vsn passed in from brod to the backend auth plugin. Should we send a patch upstream or not let perfect be the enemy of good here?

https://github.com/kafka4beam/kafka_protocol/blob/713a00e30a5f7ab0d19558c502a7e5cc5b0bfa68/src/kpro_sasl.erl#L36-L39
the version is not passed down to the callback, we can extend the callback to remain backward compatible.

starbelly · 2022-05-30T22:19:09Z

Unfortunately, I have not been able to understand how this plugin works well enough yet to have an intelligent opinion.

@kjellwinblad Hopefully what I've pushed up helps code wise, but need to add updated docs that covers the flow. Interestingly, what I smell here is a behaviour where sasl_auth has a function that does the flow and depends on a few callbacks from the caller, but premature as well.

https://github.com/kafka4beam/kafka_protocol/blob/713a00e30a5f7ab0d19558c502a7e5cc5b0bfa68/src/kpro_sasl.erl#L36-L39
the version is not passed down to the callback, we can extend the callback to remain backward compatible.

@zmstone I was thinking maybe adding a new callback function that passes in a map or record such that future changes don't require plugins to worry about different functions with different arity. See https://github.com/kafka4beam/brod_gssapi/pull/5/files#diff-6d37c08aa5bef5ce84cd840e06fe6c363227ed626e77af4010732b54948a1555R122

I mainly did this to help clean up what's going on in this plugin, but then I figured I'd raise it as an option before I send up a patch to kafka_protocol

Obviously, only brod_gssapi v2 and future backend auth plugins would utilize this. Thoughts?

starbelly · 2022-06-09T03:02:18Z

The changes at this point should require : kafka4beam/kafka_protocol#102 .

Technically, we can skip that, and the auth/8 as part of the kafka_protocol PR, will just live doormant in the code until such time that it can be merged.

kjellwinblad

I think this looks very good!

src/brod_gssapi.erl

kjellwinblad · 2022-06-09T04:57:52Z

src/brod_gssapi.erl

-%%====================================================================
+dispatch(#{handshake_vsn := 1} = State) ->
+    brod_gssapi_v1:auth(State);
+dispatch(#{handshake_vsn := 0} = State) ->


When is the the handshake_vsn = 0?

This would be passed in from kafka_protocol, but it's probably better as undefined since there is no handshake 0. Thanks for calling it out :)

Edit:

I think that's wrong actually. So kafka_protocol will pass down either 0 or 1. Kafka did not support a handshake at one point in time, which is what brod_gssapi_v0 actually supports. Thus brod_gssapi_v0 should really be called something else and brod_gssapi_v1 should be adjusted to handle both v0 and v1 handshakes. See https://github.com/kafka4beam/kafka_protocol/blob/713a00e30a5f7ab0d19558c502a7e5cc5b0bfa68/src/kpro_sasl.erl#L75

Also see https://kafka.apache.org/protocol.html#sasl_handshake

and https://cwiki.apache.org/confluence/display/KAFKA/KIP-43%3A+Kafka+SASL+enhancements

It may be we don't want to support the non-handshake version, though brokers still do support it. Especially if kafka_protocol doesn't support it (i.e., kafka_protocol will always send 0 or 1 for the handshake version).

Though, there's no harm in keeping it around as brod_gssapi_legacy for users who are using versions of kafka_protocol that don't pass the handshake in.

Thoughts?

@zmstone Can you comment on the above? Either way, I'll have to get this merged tomorrow.

Actulally, here's what I'll do, I'll go ahead and merge this and open a few issues :)

#8

Hated to merge it in the state it was in and with a few things being up in the air, but didn't want to block anymore + kaizen 😁

I don't have any strong feeling either way. Perhaps @zmstone also wants to comment on this as he has much more experience with Kafka.

kjellwinblad · 2022-06-09T11:09:33Z

I think I found a bug that I think should be fixed before merge (or we us the fix in PR #6). See #6 (comment).

- no longer define error codes and instead depend on atoms provided by sasl_auth 2.0 - bump version - Remove call to list mechanisms as there isn't a need unless since we always want gssapi. Co-authored-by: Vikas Bhardwaj <[email protected]>

- first pass at refactoring brod_gssapi_v1.erl which will become brod_gssapi - add test suite, currently mecks all externals funs - added elvis.config - added erlang_ls.config - added the following project plugins - rebar3_hex - rebar3_format - erlfmt (used by rebar3_format0 - rebar3_ex_doc - rebar3_lint - rebar3_hank

- rewrite brod_gssapi to handle auth/6 and auth/7 and delegate to appropriate versions - temporarily pull in my branch of brod

…version

starbelly marked this pull request as draft May 23, 2022 14:24

starbelly commented May 24, 2022

View reviewed changes

starbelly force-pushed the use-sasl-auth-2 branch from bb92fd3 to c38225f Compare May 30, 2022 20:50

starbelly marked this pull request as ready for review May 31, 2022 18:17

starbelly force-pushed the use-sasl-auth-2 branch 7 times, most recently from 49370d0 to 6f4ca80 Compare May 31, 2022 22:18

starbelly marked this pull request as draft June 1, 2022 22:18

kjellwinblad mentioned this pull request Jun 3, 2022

Add example to make it easy to test brod_gssapi #6

Merged

kjellwinblad approved these changes Jun 9, 2022

View reviewed changes

starbelly and others added 8 commits June 9, 2022 22:56

Update brod_gssapi_v1 to use sasl_auth 2.0

59aebaf

- no longer define error codes and instead depend on atoms provided by sasl_auth 2.0 - bump version - Remove call to list mechanisms as there isn't a need unless since we always want gssapi. Co-authored-by: Vikas Bhardwaj <[email protected]>

reduce and hit 100% coverage

6bed5de

Dialyzer fixups

cff091c

Add github workflow

3b52bbc

Add this branch as a test

0aa3d0e

Move install deps above setup-beam and add sasl_auth 2.0.0-rc1

ce39261

Add missing sudo for apt-get install

d8db83d

starbelly added 6 commits June 9, 2022 22:56

fix up test unit

96ba132

add backwards compatability

e4ace67

- rewrite brod_gssapi to handle auth/6 and auth/7 and delegate to appropriate versions - temporarily pull in my branch of brod

Replace LICENSE with LICENSE.md

c3fc24f

Add ex_doc config and bump rebar3_ex_doc to 0.2.11

e5cba64

Make modules as private for docs

c7fcd6d

re-add doc

d05bb96

starbelly force-pushed the use-sasl-auth-2 branch from f60eead to d05bb96 Compare June 10, 2022 03:58

starbelly added 5 commits June 9, 2022 23:00

undo undefined change, kafka_protocol should pass 0 for no handshake …

facd997

…version

return error if handshake v0 is given for now

ae8e3c3

remove pointer to my kafka_protocol branch

be90b70

pass undefined as version for old callback

1740242

handshake vsn can be undefined

29ccac3

starbelly marked this pull request as ready for review June 10, 2022 04:55

starbelly merged commit 5f23624 into kafka4beam:master Jun 10, 2022

kjellwinblad mentioned this pull request Jun 10, 2022

Add test cases for different handshake versions #13

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update brod_gssapi_v1 to use sasl_auth 2.0 #5

Update brod_gssapi_v1 to use sasl_auth 2.0 #5

starbelly commented May 7, 2022 •

edited

Loading

vikas15bhardwaj commented May 10, 2022 •

edited

Loading

starbelly commented May 10, 2022 •

edited

Loading

starbelly commented May 22, 2022

starbelly May 24, 2022

kjellwinblad commented May 25, 2022

zmstone commented May 25, 2022

starbelly commented May 30, 2022

starbelly commented Jun 9, 2022

kjellwinblad left a comment

kjellwinblad Jun 9, 2022

starbelly Jun 10, 2022 •

edited

Loading

starbelly Jun 10, 2022

starbelly Jun 10, 2022

starbelly Jun 10, 2022 •

edited

Loading

kjellwinblad Jun 10, 2022

kjellwinblad commented Jun 9, 2022

Update brod_gssapi_v1 to use sasl_auth 2.0 #5

Update brod_gssapi_v1 to use sasl_auth 2.0 #5

Conversation

starbelly commented May 7, 2022 • edited Loading

Notes

vikas15bhardwaj commented May 10, 2022 • edited Loading

starbelly commented May 10, 2022 • edited Loading

starbelly commented May 22, 2022

starbelly May 24, 2022

Choose a reason for hiding this comment

kjellwinblad commented May 25, 2022

zmstone commented May 25, 2022

starbelly commented May 30, 2022

starbelly commented Jun 9, 2022

kjellwinblad left a comment

Choose a reason for hiding this comment

kjellwinblad Jun 9, 2022

Choose a reason for hiding this comment

starbelly Jun 10, 2022 • edited Loading

Choose a reason for hiding this comment

starbelly Jun 10, 2022

Choose a reason for hiding this comment

starbelly Jun 10, 2022

Choose a reason for hiding this comment

starbelly Jun 10, 2022 • edited Loading

Choose a reason for hiding this comment

kjellwinblad Jun 10, 2022

Choose a reason for hiding this comment

kjellwinblad commented Jun 9, 2022

starbelly commented May 7, 2022 •

edited

Loading

vikas15bhardwaj commented May 10, 2022 •

edited

Loading

starbelly commented May 10, 2022 •

edited

Loading

starbelly Jun 10, 2022 •

edited

Loading

starbelly Jun 10, 2022 •

edited

Loading