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

feat: add SASL security scheme types for use by Kafka specs #502

Merged
merged 3 commits into from May 28, 2021
Merged

feat: add SASL security scheme types for use by Kafka specs #502

merged 3 commits into from May 28, 2021

Conversation

dalelane
Copy link
Collaborator

@dalelane dalelane commented Feb 25, 2021

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.

Fixes #466

Signed-off-by: Dale Lane dale.lane@uk.ibm.com

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide and the instructions about a basic recommended setup useful for opening a pull request.

Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@dalelane dalelane changed the title Add SASL security scheme types for use by Kafka specs feat: Add SASL security scheme types for use by Kafka specs Feb 25, 2021
@dalelane dalelane changed the title feat: Add SASL security scheme types for use by Kafka specs feat: add SASL security scheme types for use by Kafka specs Feb 25, 2021
@nictownsend
Copy link
Contributor

nictownsend commented Feb 25, 2021

Originally posted by @dalelane in asyncapi/bindings#56 (comment)

AsyncAPI:
server protocol
AsyncAPI:
security scheme type
meaning
encryption?
meaning
auth?
maps to Kafka client parameter
sasl.mechanism
maps to Kafka client parameter
security.protocol
kafka no no unset PLAINTEXT
kafka plain * no yes, using SASL/PLAIN PLAIN SASL_PLAINTEXT
kafka scramSha256 * no yes, using SASL/SCRAM SCRAM-SHA-256 SASL_PLAINTEXT
kafka scramSha512 * no yes, using SASL/SCRAM SCRAM-SHA-512 SASL_PLAINTEXT
kafka oauth2 no yes, using OAuth OAUTHBEARER SASL_PLAINTEXT
kafka gssapi * no yes, using GSSAPI GSSAPI SASL_PLAINTEXT
kafka-secure yes no unset SSL
kafka-secure plain * yes yes, using SASL/PLAIN PLAIN SASL_SSL
kafka-secure scramSha256 * yes yes, using SASL/SCRAM SCRAM-SHA-256 SASL_SSL
kafka-secure scramSha512 * yes yes, using SASL/SCRAM SCRAM-SHA-512 SASL_SSL
kafka-secure oauth2 yes yes, using OAuth OAUTHBEARER SASL_SSL
kafka-secure gssapi * yes yes, using GSSAPI GSSAPI SASL_SSL
kafka-secure X509 yes yes, using mutual TLS unset SSL

* - a new security scheme type to be added

@nictownsend
Copy link
Contributor

This is all working on the assumption that kafka/kafka-secure is equivalent to http/https - https://asyncapi.slack.com/archives/C34F2JV0U/p1614255214067200 @fmvilas for reference

If it's not, then there will need to be more to achieve the table above.

@@ -1961,7 +1961,7 @@ Defines a security scheme that can be used by the operations. Supported schemes
##### Fixed Fields
Field Name | Type | Applies To | Description
---|:---:|---|---
<a name="securitySchemeObjectType"></a>type | `string` | Any | **REQUIRED**. The type of the security scheme. Valid values are `"userPassword"`, `"apiKey"`, `"X509"`, `"symmetricEncryption"`, `"asymmetricEncryption"`, `"httpApiKey"`, `"http"`, `oauth2`, and `openIdConnect`.
<a name="securitySchemeObjectType"></a>type | `string` | Any | **REQUIRED**. The type of the security scheme. Valid values are `"userPassword"`, `"apiKey"`, `"X509"`, `"symmetricEncryption"`, `"asymmetricEncryption"`, `"httpApiKey"`, `"http"`, `"oauth2"`, and `"openIdConnect"`, `"plain"`, `"scramSha256"`, `"scramSha512"`, `"gssapi"`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't plain be covered already by the userPassword?

Copy link
Member

@smoya smoya Mar 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to reuse the userPassword schema, adding an optional field for the encryption? Sorry for the back and forth 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that depends whether we want userPassword and X509 to be reusable across protocol.

i.e it's inferred (or we state) - that for kafka, userPassword = SASL plain, X509 = mutual TLS, scramShaX = SASL scram etc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it becomes a challenge if kafka (or other protocols) add support for clashing ideas (e.g SASL x509 + mutual tls)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding an optional field for the encryption

Can we assume that protocol kafka-secure means encrypted and kafka is plain?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e it's inferred (or we state) - that for kafka, userPassword = SASL plain, X509 = mutual TLS, scramShaX = SASL scram etc

Perhaps it makes sense to add the concept of aliases that at the end are just reusing the same JSON Schema types? What I would like to avoid is redefining the same concept with a different name and maintain both at the same time.

Can we assume that protocol kafka-secure means encrypted and kafka is plain?

Yeah, that's a good point 👍 . What does it should mean -secure suffix here? I guess the question is: Do we talk about encryption or the protocol?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it should mean -secure suffix here? I guess the question is: Do we talk about encryption or the protocol?

If we compare http/https - the s represents secure protocol (via SSL encryption) - at least, I believe that's the case. If so, then that maps accordingly to kafka/kafka-secure - the secure means it's a secure version of the protocol?

Copy link
Collaborator Author

@dalelane dalelane Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we compare http/https - the s represents secure protocol (via SSL encryption)

I agree with this. I think being consistent with the distinction between http and https would be best - so in the same way that using https means encryption and does not require authentication, I think kafka-secure should mean encryption but not require auth.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to reuse the userPassword schema, adding an optional field for the encryption?

I don't really mind.

The root requirement is that there at least three SASL mechanisms typically used for Kafka that could all be described as username/password-based (SASL/PLAIN, SASL/SCRAM-SHA-256, SASL/SCRAM-SHA-512) so we either need to describe them as different security scheme types, or make this SASL mechanism information a new additional attribute of the security scheme.

Either is totally fine with me, as long as we have a way to articulate the required config.

@fmvilas Any strong preferences?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really. Either way is fine for me. Like you said, as long as we have a way to articulate the config, everything is fine. Just let's make sure it's not too verbose and it's maintainable in the long-term.

@sonarcloud
Copy link

sonarcloud bot commented Mar 16, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@dalelane
Copy link
Collaborator Author

Updated the PR to address merge conflicts.

I think all the comments above are addressed and this is ready for re-review now, please.

@dalelane
Copy link
Collaborator Author

@fmvilas @derberg Is there anything else needed for this one, please?

@fmvilas fmvilas added the 💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md) label Apr 21, 2021
@fmvilas
Copy link
Member

fmvilas commented Apr 21, 2021

I think it's pretty much done on the spec side. The only missing part for me is to update the JSON Schema definition and the JS Parser.

Regarding the JSON Schema definition, create a new document called 2.1.0.json and add these new security schemes to the definition here. Just create a pull request, doesn't have to be merged.

Regarding the JS Parser, similar thing: create a PR and make the @asyncapi/specs (this is asyncapi-node above) point to the branch you created above. Again, PR doesn't have to be merged.

If you meet these two criteria, we'll move this to the next stage (Draft RFC 2).

If you're not familiar with JS, we're happy to help on that front.

@fmvilas fmvilas added 📣 RFC document PR creates or changes document inside "rfc" folder and removed 📣 RFC document PR creates or changes document inside "rfc" folder labels Apr 21, 2021
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 <dale.lane@uk.ibm.com>
Moved the "and" to the end of the list.

Signed-off-by: Dale Lane <dale.lane@uk.ibm.com>
@fmvilas fmvilas changed the base branch from master to 2021-06-release April 23, 2021 19:56
@fmvilas fmvilas added 📄 Draft (RFC 2) RFC Stage 2 (See CONTRIBUTING.md) and removed 💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md) labels Apr 23, 2021
@fmvilas
Copy link
Member

fmvilas commented Apr 23, 2021

Moving this to the next stage 🎉 It's now a Draft, pending testing it on the JS parser.

@derberg
Copy link
Member

derberg commented Apr 26, 2021

In my opinion, we should also get examples of new schemes like in case of others from the list https://github.com/asyncapi/spec/blob/master/spec/asyncapi.md#security-scheme-object-example, also for me it is not clear what is the structure of information that one has to provide when using scramSha512 for example.

  • above the table that is modified in this PR is the list of supported schemes, would be good to add the new ones there and even link them to some vendor-neutral links (if available) that explain more.

Thoughts?

@dalelane
Copy link
Collaborator Author

In my opinion, we should also get examples of new schemes like in case of others from the list https://github.com/asyncapi/spec/blob/master/spec/asyncapi.md#security-scheme-object-example

Sure, I can add that.

it is not clear what is the structure of information that one has to provide when using scramSha512

For now, there isn't yet extra information - the crucial info is the type itself. Knowing that the SASL mechanism is SASL/SCRAM-SHA-512 is what you need to know to connect to the cluster (other than the actual credentials themselves)

above the table that is modified in this PR is the list of supported schemes, would be good to add the new ones there and even link them to some vendor-neutral links (if available) that explain more.

Good idea, I can add that

Contributes to: #466

Signed-off-by: Dale Lane <dale.lane@uk.ibm.com>
@sonarcloud
Copy link

sonarcloud bot commented Apr 26, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@derberg
Copy link
Member

derberg commented May 14, 2021

what do you think about extending the streetlight example?

@fmvilas fmvilas added this to the 2.1.0 Release milestone May 14, 2021
@derberg
Copy link
Member

derberg commented May 24, 2021

@dalelane can you have a look at my last comment?

@derberg
Copy link
Member

derberg commented May 26, 2021

@fmvilas I checked the streetlights example and I actually revert my request to @dalelane to extend it with SASL, as SASL is for Kafka and example is for MQTT. Problem is that this example, even though for mqtt, has kafka bindings. So the example is a bit messy and I would not like to block this PR because of it.

I had quick chat with @dalelane and he, as our Kafka ❤️ AsyncAPI expert, will work on another PR to fix the existing example and provide a separate Kafka example.

@fmvilas I think we are ready to go with these PRs related to SASL. The way to merge should be:

  1. spec PR merge
  2. asyncapi-node PR merge
  3. wait for asyncapi-node release candidate for NPM
  4. bump this candidate in PR in parser-js
  5. parser-js PR merge

I think this is it 🤔

@fmvilas fmvilas added 🏁 Accepted (RFC 3) RFC Stage 3 (See CONTRIBUTING.md) and removed 📄 Draft (RFC 2) RFC Stage 2 (See CONTRIBUTING.md) labels May 28, 2021
@fmvilas
Copy link
Member

fmvilas commented May 28, 2021

Yeah, agree. And since we all agree, I'm marking it as 🏁 RFC Stage 3 (Accepted) 🎉

Congrats, @dalelane! It's the first accepted RFC after v2.0.0.

@fmvilas fmvilas merged commit 8c8e211 into asyncapi:2021-06-release May 28, 2021
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.1.0-2021-06-release.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@dalelane dalelane deleted the security-scheme-for-kafka branch May 31, 2021 21:00
@dalelane
Copy link
Collaborator Author

@derberg FYI - I've had a go at preparing a new version of the Streetlight example in #548

fmvilas pushed a commit that referenced this pull request Jun 1, 2021
Main motivation of this was to provide a place to demonstrate
the new Kafka-specific security schemes introduced in
AsyncAPI 2.1.0. It was also a chance to remove the Kafka
bindings from the otherwise-MQTT sample.

The need for this was discussed in
#502

Contributes to: #466

Signed-off-by: Dale Lane <dale.lane@uk.ibm.com>
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.2.0-2021-06-release.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants