Add AWS_MSK_IAM SASL Mechanism #763

cmaher · 2021-10-21T19:19:01Z

Addresses #661

I've verified this against a live MSK cluster and confirmed the behavior relative to other implementations. Unfortunately, live connections can't be unit-tested, so I'm testing the signature generation, which is the heart of this mechanism. Also, as discussed in the original ticket, I've made this a submodule, with the intent that it won't cause conflicts with general user code. Please let me know if I need to change/add anything.

.circleci/config.yml

sasl/aws_msk_iam/go.mod

achille-roussel

Exciting to see this shape up!

sasl/aws_msk_iam/msk_iam.go

sasl/aws_msk_iam/go.mod

sasl/aws_msk_iam/msk_iam.go

cmaher · 2021-10-22T14:47:21Z

Thanks for the quick response! I think I've addressed all the feedback here.

sasl/aws_msk_iam/msk_iam.go

sasl/aws_msk_iam/msk_iam_test.go

cmaher · 2021-10-26T14:31:29Z

Just checking in to see if you want anything else done / what the next steps are

dominicbarnes · 2021-10-26T16:16:02Z

@cmaher sorry about that, I had a couple other questions I wanted to raise but the last few days have been a little hectic for me.

dominicbarnes · 2021-10-26T16:22:43Z

sasl/aws_msk_iam/msk_iam.go

+	// The sigv4.Signer to use when signing the request. Required.
+	Signer *sigv4.Signer
+	// The host of the kafka broker to connect to. Required.
+	Host string


Any time I've gone to set up an MSK cluster, each broker (which requires more than 1 in all cases) has it's own distinct host name. So far, I have not found a unified host name like I see in other services with clusters (eg: Redshift, Elasticache) so should we be supporting multiple values here? (it looks like the host name is part of the signing process, so maybe that isn't as easy at it seems?)

Additionally, it seems like the MSK API exposes endpoints to retrieve the broker host names, depending on the specific authentication strategy being used. I think it would be more future-proof to allow using the MSK API to perform this look-up internally, but I think we can pass on this bit of "magic" for now and make it a later enhancement.

You're right. I was using a single broker in my experiments, but the host needs to be dynamic. I think the best way to do this while maintaining the existing sasl interface is to set the host on the context. This approach raises some questions, however, which I'll point out in my commit

cmaher · 2021-10-26T21:48:35Z

dialer.go

@@ -258,6 +258,8 @@ func (d *Dialer) connectTLS(ctx context.Context, conn net.Conn, config *tls.Conf
 	return
 }

+const ContextKeyBrokerAddr = "brokerAddr"


I was wondering where the proper place to put this is, assuming you want to use constants instead of literals

It looks like this is actually unnecessary now that sasl.MetadataFromContext is being used. Would you mind removing this const and updating the tests that still reference it?

cmaher · 2021-10-26T21:50:15Z

sasl/aws_msk_iam/go.mod

@@ -6,3 +6,5 @@ require (
 	github.com/aws/aws-sdk-go v1.41.3
 	github.com/segmentio/kafka-go v0.4.21
 )
+
+replace github.com/segmentio/kafka-go => ../..


This is needed for the tests to pass in this pull-request, but it should not be merged. How should this be handled? 2 separate pull-requests?

Ah yeah, in that case I would think we can just remove this line before merging, once we've gotten tests to pass. I would rather avoid 2 PRs.

Addresses segmentio#661

Indent json for godoc Co-authored-by: Achille <achille.roussel@gmail.com>

dominicbarnes

Looks like we're getting closer!

dominicbarnes · 2021-11-22T08:21:31Z

dialer.go

@@ -258,6 +258,8 @@ func (d *Dialer) connectTLS(ctx context.Context, conn net.Conn, config *tls.Conf
 	return
 }

+const ContextKeyBrokerAddr = "brokerAddr"


It looks like this is actually unnecessary now that sasl.MetadataFromContext is being used. Would you mind removing this const and updating the tests that still reference it?

dominicbarnes · 2021-11-22T08:22:35Z

sasl/aws_msk_iam/go.mod

@@ -6,3 +6,5 @@ require (
 	github.com/aws/aws-sdk-go v1.41.3
 	github.com/segmentio/kafka-go v0.4.21
 )
+
+replace github.com/segmentio/kafka-go => ../..


Ah yeah, in that case I would think we can just remove this line before merging, once we've gotten tests to pass. I would rather avoid 2 PRs.

dominicbarnes · 2021-11-22T08:27:22Z

sasl/aws_msk_iam/msk_iam.go

+	}
+	signUrl := url.URL{
+		Scheme:   "kafka",
+		Host:     saslMeta.Host,


Locally, my tests panic because saslMeta does not exist, which I think will be corrected by updating your test cases.

That being said, should we guard against this and return an explicit error message if we detect this? (rather than panicking)

dominicbarnes · 2021-11-22T08:28:04Z

sasl/aws_msk_iam/msk_iam_test.go

+	}{
+		{
+			ctx: func() context.Context {
+				return context.WithValue(context.Background(),


I think these need to be updated to use sasl.WithMetadata.

dominicbarnes · 2021-11-22T08:28:51Z

Howdy again! Sorry about the delay, I've been trying to get this code to work with a manually-provisioned MSK cluster just to make sure I've at least seen it in action for myself (since as you pointed out, unit-testing this is hard/unfeasible). Thankfully, after some trial and error of my own, I was able to get it to work! That makes me more comfortable moving forward with merging even without some added test coverage beyond what we have here.

I'll follow up on the existing threads, as I think we have a few outstanding ones to finish addressing. That being said, this is looking to be really close.

dominicbarnes · 2021-11-22T08:39:56Z

.circleci/config.yml

@@ -40,6 +40,7 @@ jobs:
        paths:
        - /go/pkg/mod
    - run: go test -race -cover ./...
+    - run: find ./sasl -name go.mod -execdir go test -race -cover ./... \;


I know this was my idea, but it looks like it was a bad one, sorry about that! It looks like the exit code from the -execdir is being lost, so CircleCI is treating these as passing even though they panic. :(

If we don't have any other ideas, feel free to revert back to what you had before.

cmaher · 2021-11-23T15:06:45Z

Hey, thanks for getting back to this. I probably don't have time this week, but I'll be able to get to it within the next two weeks after the holiday.

dominicbarnes · 2021-11-24T04:26:28Z

This PR has been addressed by #798 so I will close this. Thank you so much for your contribution!

dominicbarnes reviewed Oct 21, 2021

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

dominicbarnes reviewed Oct 21, 2021

View reviewed changes

sasl/aws_msk_iam/go.mod Outdated Show resolved Hide resolved

achille-roussel reviewed Oct 21, 2021

View reviewed changes

achille-roussel reviewed Oct 22, 2021

View reviewed changes

sasl/aws_msk_iam/msk_iam.go Outdated Show resolved Hide resolved

sasl/aws_msk_iam/msk_iam_test.go Outdated Show resolved Hide resolved

dominicbarnes reviewed Oct 26, 2021

View reviewed changes

cmaher commented Oct 26, 2021

View reviewed changes

achille-roussel mentioned this pull request Nov 5, 2021

add port to SASL metadata #780

Merged

cmaher and others added 8 commits November 5, 2021 16:08

Add AWS_MSK_IAM SASL Mechanism

9debc76

Addresses segmentio#661

Update sasl/aws_msk_iam/msk_iam.go

3b466c2

Indent json for godoc Co-authored-by: Achille <achille.roussel@gmail.com>

Respond to PR feedback

e0b80a8

Clearer comment

0231dea

Clarify some comments. More straightforward code.

8afeca3

Test compares bytes instead of maps

8cdaac5

Auth works for multiple brokers

d16a17a

Update with sasl meta

fb5d688

cmaher force-pushed the cmaher/add-aws-msk-iam-sasl branch from c054939 to fb5d688 Compare November 5, 2021 20:15

Move module version back until new version is cut

4eb1653

achille-roussel assigned dominicbarnes and derekbassett Nov 12, 2021

yolken-segment mentioned this pull request Nov 18, 2021

AWS IAM Auth support segmentio/topicctl#48

Closed

dominicbarnes requested changes Nov 22, 2021

View reviewed changes

dominicbarnes reviewed Nov 22, 2021

View reviewed changes

dominicbarnes mentioned this pull request Nov 23, 2021

SASL Mechanism: AWS MSK IAM (making requested edits) #798

Merged

dominicbarnes closed this Nov 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AWS_MSK_IAM SASL Mechanism #763

Add AWS_MSK_IAM SASL Mechanism #763

cmaher commented Oct 21, 2021 •

edited

Loading

achille-roussel left a comment

cmaher commented Oct 22, 2021

cmaher commented Oct 26, 2021

dominicbarnes commented Oct 26, 2021

dominicbarnes Oct 26, 2021 •

edited

Loading

cmaher Oct 26, 2021

cmaher Oct 26, 2021

dominicbarnes Nov 22, 2021

cmaher Oct 26, 2021

dominicbarnes Nov 22, 2021

dominicbarnes left a comment

dominicbarnes Nov 22, 2021

dominicbarnes Nov 22, 2021

dominicbarnes Nov 22, 2021

dominicbarnes Nov 22, 2021

dominicbarnes commented Nov 22, 2021

dominicbarnes Nov 22, 2021

cmaher commented Nov 23, 2021

dominicbarnes commented Nov 24, 2021

Add AWS_MSK_IAM SASL Mechanism #763

Add AWS_MSK_IAM SASL Mechanism #763

Conversation

cmaher commented Oct 21, 2021 • edited Loading

achille-roussel left a comment

Choose a reason for hiding this comment

cmaher commented Oct 22, 2021

cmaher commented Oct 26, 2021

dominicbarnes commented Oct 26, 2021

dominicbarnes Oct 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominicbarnes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominicbarnes commented Nov 22, 2021

Choose a reason for hiding this comment

cmaher commented Nov 23, 2021

dominicbarnes commented Nov 24, 2021

cmaher commented Oct 21, 2021 •

edited

Loading

dominicbarnes Oct 26, 2021 •

edited

Loading