-
-
Notifications
You must be signed in to change notification settings - Fork 74
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
docs: add essential Kafka client security parameters to bindings #56
Conversation
There are a huge number of Kafka client configs we could add to server bindings, but I'm skeptical of the value of just copying them all across. Most client config values have sensible defaults that are suitable in most cases. cf. https://kafka.apache.org/documentation/#consumerconfigs Two values are essential for code generation with secured Kafka clusters, however - identifying the security mechanism and protocol to use. This commit adds these two most commonly-used parameters to the Kafka server bindings. I suggest we start with these and gauge usage before starting to add many more, as these are enough to enable code generation for secured Kafka clusters. Closes: asyncapi/spec#466 Signed-off-by: Dale Lane <[email protected]>
Hi @dalelane! Thanks for making this PR. [Edit] I see @fmvilas suggested this already as per your notes in asyncapi/spec#466 |
Could you give me a bit more detail about how that'd work, please? Or maybe an example of what this would look like in security schemes?
|
We already have implemented one of the available SASL mechanisms: x509. type: saslScramSha512 Or create a global one for all SHA, which means we would have x509 duplicated, not so ideal I think. type: sasl
saslMechanism: ScramSha512 |
|
||
Field Name | Type | Description | Applicability [default] | Constraints |
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.
If the whole "Constraints" column is empty, shouldn't we just get rid of it entirely?
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.
I was doing that to avoid the merge conflict if both this and #55 get approved
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.
Oh, I see. Thanks for the explanation 👍
|
||
Field Name | Type | Description | Applicability [default] | Constraints | ||
---|:---:|:---:|:---:|--- | ||
`saslMechamism` | string | Specifies the [`sasl.mechanism`](https://kafka.apache.org/documentation/#consumerconfigs_sasl.mechanism) config property that must be specified by Kafka clients connecting to this server. <br>Example values include `GSSAPI`, `PLAIN`, `OAUTHBEARER`, `SCRAM-SHA-256`, `SCRAM-SHA-512` | OPTIONAL [`GSSAPI`] | - |
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.
Really the default value is GSSAPI
? Isn't it PLAIN
?
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.
Also, shouldn't we have something like a NONE
value to disable the saslMechanism?
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.
I did that to mirror what Kafka does by default - which is GSSAPI
But I think adding a NONE option for when using kafka
instead of kafka-secure
might make sense, and make a better default.
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. In any case, if we move this to the Security Schemes Object, we would not need the NONE
value anymore. Sorry, for the messed-up review. I realized about the other comments after I made the review.
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.
no problem at all - it's been a useful thread, thanks to both you and @smoya !
I just saw Sergio's comment. Yeah, it's true it would make more sense to add this to Security Schemes. The way to proceed is similar. If something is missing on the Security Schemes Object, then feel free to add new keys, like |
The problem with |
You are totally right @nictownsend. x509 would need to be a shared mechanism, in this case between TLS/SSL and SASL.
|
That's been really useful, thanks @smoya @nictownsend I think where we end up is:
* - a new security scheme type to be added |
Closing as I think we've agreed this is better captured in security schemes, as described in #56 (comment) so I've opened asyncapi/spec#502 for this |
* feat: add SASL security scheme types for use by Kafka specs As discussed in asyncapi/bindings#56 this adds additional security scheme types. The motivation for adding them is to enable description of secured Kafka clusters, however the security protocols and mechanisms being added are not unique to Kafka, so this commit adds them as generic security schemes so they could be used by other protocols as well. Contributes to: #466 Signed-off-by: Dale Lane <[email protected]> * chore: update for grammar Moved the "and" to the end of the list. Signed-off-by: Dale Lane <[email protected]> * docs: additional SASL documentation Contributes to: #466 Signed-off-by: Dale Lane <[email protected]>
There are a huge number of Kafka client configs we could add to
server bindings, but I'm skeptical of the value of just copying
them all across. Most client config values have sensible
defaults that are suitable in most cases.
cf. https://kafka.apache.org/documentation/#consumerconfigs
Two values are essential for code generation with secured Kafka
clusters, however - identifying the security mechanism and
protocol to use. This commit adds these two most commonly-used
parameters to the Kafka server bindings. I suggest we start with
these and gauge usage before starting to add many more, as these
are enough to enable code generation for secured Kafka clusters.
Closes: asyncapi/spec#466
Signed-off-by: Dale Lane [email protected]