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

add support for aws mks iam #2192

Closed
wants to merge 1 commit into from
Closed

add support for aws mks iam #2192

wants to merge 1 commit into from

Conversation

sethpollack
Copy link

fixes #1985

usage would look like:

cfg.Net.SASL.Mechanism = sarama.SASLMechanism(sarama.SASLTypeAWSMSKIAM)
cfg.Net.SASL.AWSMSKIAM = sarama.AWSMSKIAMConfig{
    Region: "us-east-1",
}

@ghost ghost added the cla-needed label Mar 22, 2022
@dnwe
Copy link
Collaborator

dnwe commented Mar 28, 2022

@sethpollack thanks for raising this

I wasn't aware that AWS had implented their own custom SASL mechanism for this, rather than adopting one of the existing standards. Whilst I can understand that people using AWS would like this support, ideally we wouldn't want to a) introduce a direct dependency on awk sdks for everyone or b) have service-specific configuration parameters etc. — this is also quite hard for us to regression test without provisioning a live service

I notice that other kafka clients had this support added using a pluggable "opt-in" sasl provider architecture. I wonder how much work it would be to achieve a similar result with Sarama? i.e., keeping the dependencies and configuration in a separate module path that people can opt-in to using by pulling it in as a dependency

@sethpollack
Copy link
Author

@dnwe Another option would be to use SCRAMClientGeneratorFunc but we would need to pass along the broker addr to the function.

@lomluca
Copy link

lomluca commented Apr 9, 2022

I was implementing the same feature for a Kafka Terraform provider that relies on Sarama and I went for this last solution @sethpollack mentioned: using a custom SCRAMClientGeneratorFunc. I think passing a pluggable function is the best solution, in this way Sarama could be very flexible supporting many (potential) SASL/SCRAM-based mechanism.

The problem is Sarama is not flexible right now: in many points the code checks if the mechanism is strictly "SASLTypeSCRAMSHA256" or "SASLTypeSCRAMSHA512", there is no room to add new mechanisms. I would propose to enforce a generalized "SASLTypeSCRAM" mechanism and, in addition to that, introduce a flexible mechanism name (i.e. "SCRAM-SHA-256, "SCRAM-SHA-512", "AWS_MSK_IAM") that could be externally specified. I think these modifications, with the already-in-place possibility to specify a custom SCRAMClientGeneratorFunc, allow what we want to achieve.

What do you think?

@michaelwagler
Copy link

Any updates on this? We use Sarama for a large number of our services, and would love to keep using it, but are considering moving over to kafka-go because it has IAM support.

@IBM IBM deleted a comment from tooptoop4 Jul 26, 2022
@tooptoop4
Copy link

tooptoop4 commented Oct 20, 2022

@sethpollack for SASL.User/SASL.Password do you have to provide a static accesskey/secretkey? https://github.com/Shopify/sarama/blob/f16c9d8fbe4866c970b20a08be14d57553b0b660/broker.go#L1417 mentions user/pw

@sethpollack
Copy link
Author

bpaquet added a commit to bpaquet/sarama that referenced this pull request Nov 17, 2022
bpaquet added a commit to bpaquet/sarama that referenced this pull request Nov 17, 2022
bpaquet added a commit to bpaquet/sarama that referenced this pull request Nov 17, 2022
@kashifkhan0771
Copy link

Any updates on this? When can Sarama provide IAM support?

@bpaquet
Copy link

bpaquet commented Dec 9, 2022

Please :)

@fuyar
Copy link

fuyar commented Jan 11, 2023

Up for 2023 NY ;)

@galamit1
Copy link

galamit1 commented Apr 9, 2023

Any updates? Do you know when it will be merged?

@austinorth
Copy link

@sethpollack would you have a moment to sign the CLA so we can get this merged? 🙏🏻

@sethpollack
Copy link
Author

@austinorth unfortunately that CLA is asking for way too much personal information, I don't feel comfortable signing it.

@austinorth
Copy link

@sethpollack ahhh worddd

@bpaquet
Copy link

bpaquet commented Apr 17, 2023

@sethpollack can I fork you PR to get it merged?

@sethpollack
Copy link
Author

@bpaquet Yes

@joaocc
Copy link

joaocc commented Aug 31, 2023

Hi! Is there any update on this PR? Thanks

@mauriciottc
Copy link

+1

@cjireland
Copy link

+1

@dnwe
Copy link
Collaborator

dnwe commented Sep 14, 2023

This PR was superseded by #2482 which is waiting for more testing feedback from AWS users and the /v2 version boundary of Sarama (because it will change the client API for auth)

@dnwe dnwe closed this Sep 14, 2023
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.

[Feature request] Support for MSK & IAM integration