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/OAUTHTOKEN support #253

Merged
merged 2 commits into from
Jan 6, 2025

Conversation

einarmo
Copy link
Contributor

@einarmo einarmo commented Jan 3, 2025

Closes #252.

This adds SASL/OAUTHBEARER support to the client.

Since SaslConfig no longer always contains Credentials I did some minor refactoring and moved the rsasl SASLConfig building into a separate method on SaslConfig. There is no convenient method for doing Oauth, so we have to implement an rsasl callback for providing a bearer token.

I've chosen the token callback to be asynchronous, since users of this library are likely to use an asynchronous HTTP library, or other OAUTH mechanism, and mixing sync and async is a very bad idea.

One thing I have not done is write any tests, mostly since writing an actual integration test supporting this would be very complicated. Since I have now done some of that myself I have a vague idea of what would be needed:

  • You would need an IDP, we used Azure AD, but you could make that self contained using Keycloak or similar, though configuring that as part of tests is also non-trivial.

  • You would need some form of Oauth plugin, which I believe isn't part of the docker image you use in tests (not entirely sure about that, we use the strimzi image).

  • I've read the contributing section of the project CONTRIBUTING.md.

  • Signed CLA (if not already signed).

@einarmo einarmo changed the title Add SASL/OAUTHTOKEN support feat: Add SASL/OAUTHTOKEN support Jan 3, 2025
@einarmo einarmo force-pushed the oauth-support branch 2 times, most recently from bb76719 to 3a5e65e Compare January 3, 2025 06:50
@einarmo
Copy link
Contributor Author

einarmo commented Jan 3, 2025

Looks like audit is failing due to rust version. A simple fix would be to lock cargo-deny to 0.16.2.

Copy link
Collaborator

@crepererum crepererum left a comment

Choose a reason for hiding this comment

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

Looks pretty good for the first round. Couple of smaller nitpicks. I've fixed the CI in #254, so if you rebase this branch, you should be fine.

Cargo.toml Outdated Show resolved Hide resolved
src/protocol/frame.rs Show resolved Hide resolved
src/protocol/frame.rs Show resolved Hide resolved
src/messenger.rs Outdated
Comment on lines 207 to 208
#[error("Other SASL error: {0}")]
Other(String),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess you've introduced this variant so that you have a catch-all version for the callback? I'm wondering if the callback should even use SaslError in the first place or if it should rather return Box<dyn std::error::Error + Send + Sync> and you convert this to a SaslError variant like

#[derive(Error, Debug)]
pub enum SaslError {
    // ...

    #[error("OAuth Callback: {0}")]
    OauthCallback(Box<dyn std::error::Error + Send + Sync>),

    // ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's probably a good idea. I'll change it 👍

)?),
Self::Oauthbearer(credentials) => {
// Fetch the token first, since that's an async call.
// The user can and should cache the token as appropriate to their OAUTH provider.
Copy link
Collaborator

Choose a reason for hiding this comment

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

// The user can and should cache the token as appropriate to their OAUTH provider.

Could you move this particular note into the docstring for OauthCallback? I think this is important for the user.

@einarmo einarmo force-pushed the oauth-support branch 2 times, most recently from 6239627 to 4c6ad64 Compare January 3, 2025 11:52
This was a bug in the existing implementation that sometimes caused
issues, but that some kafka brokers appear to tolerate.
@einarmo
Copy link
Contributor Author

einarmo commented Jan 3, 2025

I added a second commit, since I ran into a bug in the existing SASL implementation. It seems like rsasl sometimes (for PLAIN auth, among others), expects us to send a final message even when state is marked as finished.

I changed the code there a bit to account for this, I really don't understand how this could work at all, it should have completely broken PLAIN auth.

@crepererum
Copy link
Collaborator

I changed the code there a bit to account for this, I really don't understand how this could work at all, it should have completely broken PLAIN auth.

me neither, but if it works now it works 🤷

Copy link
Collaborator

@crepererum crepererum left a comment

Choose a reason for hiding this comment

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

thank you

@crepererum crepererum merged commit e65584d into influxdata:main Jan 6, 2025
11 checks passed
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.

SASL/OAUTHBEARER support
2 participants