add support for OAuth #442

jsurany-bloomberg · 2022-02-07T19:24:52Z

This PR is meant to provide the user a way to build a Client that uses the oauthbearer_refresh_token_cb functionality in rdkafka-sys. It also provides the plumbing to define their own callback through the ClientContext trait.

ClientConfig has a pub(crate) flag that tells the Client to set the callback function to that of the given <impl>ClientContext type.
Default implementation of the callback just logs a message at the error level, the user needs to create their own callback function.

Example

// Define a type that overrides the default refresh_oauth_token method
struct RefreshContext {
    // relevant fields
}

impl RefreshContext {
    // other impl details
}

impl ClientContext for RefreshContext {
    fn refresh_oauth_token(&self, client: &mut RDKafka, oauthbearer_config: *const i8) {
         // set the token with rdkafka_sys::rd_kafka_oauthbearer_set_token
    }
}

let config = ClientConfig::new();
config.use_oauth_token_refresh_cb();
// other settings

let context = RefreshContext::new();
let producer = FutureProducer<RefreshContext> = config
    .create_with_context(context)
    .expect("Producer creation failed");

// etc.

Let me know what you think. The prototyping I've done for this has been on an older version of this crate, so testing is needed for these changes, particularly for Consumer types.

jsurany-bloomberg · 2022-02-07T20:45:35Z

Should resolve issue #415

jsurany-bloomberg · 2022-02-09T22:19:13Z

cc @benesch

benesch · 2022-02-22T04:39:27Z

Sorry, I don't have time to look at this at the moment! I'll ping some of the other maintainers.

src/client.rs

duarten · 2022-02-22T20:58:36Z

src/client.rs

+    opaque: *mut c_void,
+) {
+    let context = &mut *(opaque as *mut C);
+    context.refresh_oauth_token(client, oauthbearer_config);


Should it return a boolean so we can then call oauthbearer_set_token() or
oauthbearer_set_token_failure()?

Hoping that I'm understanding what you want here...

It'd be cool to put the unsafe calls to those functions in this block, but I'm not sure it makes sense.

For instance, I don't think we should assume what the value of parameters like md_lifetime_ms should be, and I don't think we should complicate ClientContext so that the user has a way to provide that value through context by default.

Also, I don't really want to assume anything about how the user handles errors in this process. By moving the call to oauthbearer_set_token into this block, we make it so that the user has a lot of flexibility in handling errors in generating the token (since that's code that they are going to write), but they have no flexibility in handling errors that get reported back via the errstr value - they would be bound to whatever we decide to do in this block.

I can write a pseudo prototype of a callback function to make things a bit clearer if needed.

The callback could return the information needed for us to call oauthbearer_set_token here. One option would be to return Result<SetTokenData, Error>. The user can still have some custom error handling logic in the callback, but we'll take care of enforcing the contract that we must call oauthbearer_set_token_failure or oauthbearer_set_token in the callback.

Alternatively, if we really want the user to have all the flexibility in handling errors, we can return Option<SetTokenData>.

In any case, should this PR also include bindings for oauthbearer_set_token_failure or oauthbearer_set_token?

Take a look at the most recent commit and let me know what you think. The only thing that's missing right now is support for SASL extensions, for which I would be happy to add support in my free time after this PR.

Looks great!

…oken function

jsurany-bloomberg · 2022-03-01T21:31:20Z

@duarten Tested these changes and discovered some issues. I've fixed them in the latest commit, along with a small QoL change to the OAuthTokenError struct.

jsurany-bloomberg · 2022-03-01T21:48:59Z

src/client.rs

@@ -515,7 +516,11 @@ pub(crate) unsafe extern "C" fn native_oauth_refresh_cb<C: ClientContext>(
 ) {
    // generate the token using generate_oauth_token
    let context = &mut *(opaque as *mut C);
-    let oauthbearer_config = CStr::from_ptr(oauthbearer_config).to_string_lossy();
+    let oauthbearer_config = match oauthbearer_config.is_null() {


oauthbearer_config may be null if there is no configuration. This check protects against the resulting seg fault.

jsurany-bloomberg · 2022-03-01T21:52:17Z

src/client.rs

@@ -542,7 +547,7 @@ pub(crate) unsafe extern "C" fn native_oauth_refresh_cb<C: ClientContext>(
        }
    };

-    let errstr = match CString::new(vec![0_u8; token_info.errstr_size]) {
+    let errstr = match CString::new(vec![u8::MAX; token_info.errstr_size]) {


from documentation CString::new returns an error if the bytes contain a 0, causing this part of the code to only error.

from_vec_unchecked may be a better choice. I would think that we want the vector to have only zeros.

Tweak the support for the OAuth token refresh callback added in #442 to avoid some memory unsafety and to be more in line with crate conventions. In particular: * Don't pass `&str` to C code expecting a C string directly, as the Rust string slice is not guaranteed to be null terminated and so the C code might read off the end of a string. Instead always construct a CString. * Don't allow the user to specify an `errstr_size`. Crate convention is to use an `ErrBuf` with a fixed size of 512. * Rename `generate_oauth_token` to `refresh_oauth_token`, to hew closer to the librdkafka naming for the feature. * Move the `Config::use_token_refresh_cb` method to an associated constant on the `ClientContext`, so that the configuration of the token refresh callback is entirely contained within `ClientContext.`

This PR is meant to provide the user a way to build a Client that uses the oauthbearer_refresh_token_cb functionality in rdkafka-sys. It also provides the plumbing to define their own callback through the ClientContext trait. ClientConfig has a pub(crate) flag that tells the Client to set the callback function to that of the given <impl>ClientContext type. Default implementation of the callback just logs a message at the error level, the user needs to create their own callback function. Example ```rust // Define a type that overrides the default refresh_oauth_token method struct RefreshContext { // relevant fields } impl RefreshContext { // other impl details } impl ClientContext for RefreshContext { fn refresh_oauth_token(&self, client: &mut RDKafka, oauthbearer_config: *const i8) { // set the token with rdkafka_sys::rd_kafka_oauthbearer_set_token } } let config = ClientConfig::new(); config.use_oauth_token_refresh_cb(); // other settings let context = RefreshContext::new(); let producer = FutureProducer<RefreshContext> = config .create_with_context(context) .expect("Producer creation failed"); // etc. ``` Let me know what you think. The prototyping I've done for this has been on an older version of this crate, so testing is needed for these changes, particularly for Consumer types.

Tweak the support for the OAuth token refresh callback added in #442 to avoid some memory unsafety and to be more in line with crate conventions. In particular: * Don't pass `&str` to C code expecting a C string directly, as the Rust string slice is not guaranteed to be null terminated and so the C code might read off the end of a string. Instead always construct a CString. * Don't allow the user to specify an `errstr_size`. Crate convention is to use an `ErrBuf` with a fixed size of 512. * Move the `Config::use_token_refresh_cb` method to an associated constant on the `ClientContext`, so that the configuration of the token refresh callback is entirely contained within `ClientContext.`

add token refresh cb to Client and ClientConfig

93057f2

jsurany-bloomberg mentioned this pull request Feb 7, 2022

support for OAuth #415

Open

jsurany-bloomberg changed the title ~~add token refresh callback functionality~~ add support for OAuth Feb 16, 2022

duarten reviewed Feb 22, 2022

View reviewed changes

added OAuthTokenData struct, created by user-defined generate_oauth_t…

2a14da5

…oken function

duarten approved these changes Feb 25, 2022

View reviewed changes

fix discovered runtime issues

a0d4045

jsurany-bloomberg commented Mar 1, 2022

View reviewed changes

duarten merged commit 04b634a into fede1024:master Mar 24, 2022

benesch mentioned this pull request Apr 4, 2022

Rework OAuth callback to avoid memory unsafety and follow conventions #457

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for OAuth #442

add support for OAuth #442

jsurany-bloomberg commented Feb 7, 2022

jsurany-bloomberg commented Feb 7, 2022

jsurany-bloomberg commented Feb 9, 2022

benesch commented Feb 22, 2022

duarten Feb 22, 2022

jsurany-bloomberg Feb 23, 2022

duarten Feb 23, 2022

jsurany-bloomberg Feb 25, 2022

duarten Feb 25, 2022

jsurany-bloomberg commented Mar 1, 2022

jsurany-bloomberg Mar 1, 2022

jsurany-bloomberg Mar 1, 2022

jsurany-bloomberg Mar 1, 2022

add support for OAuth #442

add support for OAuth #442

Conversation

jsurany-bloomberg commented Feb 7, 2022

jsurany-bloomberg commented Feb 7, 2022

jsurany-bloomberg commented Feb 9, 2022

benesch commented Feb 22, 2022

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

jsurany-bloomberg commented Mar 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment