feat: Support SCRAM and OAuth mechanisms in Kafka plugin #479
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mkanoor
force-pushed
the
extend_sasl_mechanisms
branch
from
f86faaf to
6f9576f
Compare
github-actions
bot
added
feat
mkanoor
force-pushed
the
extend_sasl_mechanisms
branch
from
6f9576f to
9e1524b
Compare
github-actions
bot
added
feat
mkanoor
force-pushed
the
extend_sasl_mechanisms
branch
from
9e1524b to
e64fdc9
Compare
github-actions
bot
added
feat
mkanoor
force-pushed
the
extend_sasl_mechanisms
branch
from
e64fdc9 to
07c66f9
Compare
github-actions
bot
added
feat
github-actions
bot
added
feat
mkanoor
force-pushed
the
extend_sasl_mechanisms
branch
from
44d8317 to
6a44eb2
Compare
github-actions
bot
added
feat
mkanoor
force-pushed
the
extend_sasl_mechanisms
branch
from
d865131 to
644010d
Compare
github-actions
bot
added
feat
github-actions
bot
added
feat
mkanoor
force-pushed
the
extend_sasl_mechanisms
branch
from
a701145 to
ba437d9
Compare
github-actions
bot
added
feat
github-actions
bot
added
feat
Added support for fetching tokens from external Auth Servers. When fetching tokens supports * client_secret_basic * client_secret_post * client_secret_jwt * private_key_jwt These authentication methods are described in RFC 6749 and RFC 7523 Added validations for SCRAM-SHA-256 and SCRAM-SHA-512 With this PR we have complete support for all SASL Mechanisms needed in Kafka * PLAIN * SCRAM-SHA-256 * SCRAM-SHA-512 * GSSAPI * OAUTHBEARER
mkanoor
force-pushed
the
extend_sasl_mechanisms
branch
from
bbd7d4d to
b8c025d
Compare
github-actions
bot
added
feat
for more information, see https://pre-commit.ci
github-actions
bot
added
feat
| files: ^.config\/.*requirements.*$ | ||
| language: python | ||
| language_version: "3.9" # minimal we support officially | ||
| language_version: "3.10" # minimal we support officially |
There was a problem hiding this comment.
let's do this in a different PR and set 3.11 as minimal.
| description: | ||
| - The kerberos REALM | ||
| type: str | ||
| sasl_oauth_token_endpoint: |
There was a problem hiding this comment.
We would have already 17 sasl related parameters, this is a awful and bad interface IMO. We should nest all these params into their own sasl config object, with backward compatibility of the existing params with deprecation in favor of the new system. Which probably should be correlated with an internal schema for sasl config because dealing with some many fields with a regular dict becomes hard to read and maintain.
| if offset not in ("latest", "earliest"): | ||
| msg = f"Invalid offset option: {offset}" | ||
| raise ValueError(msg) | ||
| _validate_args(args) |
There was a problem hiding this comment.
defaults management is inconsistent because we already set the default values also in main, the default values should be set only in one place.
Encapsulating validation is great but it should be called before declaring any other variable that comes from the args.
I suggest a validate args object, where we can centralize parsing, validation and defaults.
Comment on lines
+319
to
+342
| if sasl_oauth_method not in OAUTH_CLASSES_MAP: | ||
| msg = ( | ||
| f"OAUTHBEARER invalid sasl_oauth_method: {sasl_oauth_method}. " | ||
| f"Should be one of : {','.join(OAUTH_CLASSES_MAP)}" | ||
| ) | ||
| raise ValueError(msg) | ||
|
|
||
| if sasl_oauth_method in PRIVATE_KEY_METHODS and not args.get( | ||
| "sasl_oauth_private_keyfile" | ||
| ): | ||
| msg = ( | ||
| f"When using {sasl_oauth_method} a private key file is needed. " | ||
| "Please provide sasl_oauth_private_keyfile" | ||
| ) | ||
| raise ValueError(msg) | ||
|
|
||
| if sasl_oauth_method in CLIENT_SECRET_METHODS and not args.get( | ||
| "sasl_oauth_client_secret" | ||
| ): | ||
| msg = ( | ||
| f"When using {sasl_oauth_method} a client secret is needed. " | ||
| "Please provide sasl_oauth_client_secret" | ||
| ) | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
This looks like something that should be also encapsulated at the validation
|
|
||
| except aiohttp.ClientError as e: | ||
| LOGGER.error("Failed to obtain OAuth2 token: %s", str(e)) | ||
| LOGGER.error("Response text %s", str(response_text)) |
There was a problem hiding this comment.
Are you sure response_text will not contain sensitive data?
| ) | ||
| raise ValueError(msg) | ||
|
|
||
| if sasl_mechanism in USER_PASSWORD_MECHANISMS: |
There was a problem hiding this comment.
USER_PASSWORD_MECHANISMS does not include PLAIN
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added support for fetching tokens from external Auth Servers. When fetching tokens supports
These authentication methods are described in RFC 6749 and RFC 7523
Added validations for SCRAM-SHA-256 and SCRAM-SHA-512
With this PR we have complete support for all SASL Mechanisms needed in Kafka