Skip to content

feat!: add dynamic authorization support#194

Open
EddieHouston wants to merge 2 commits intobitcoindevkit:masterfrom
EddieHouston:rpc-auth
Open

feat!: add dynamic authorization support#194
EddieHouston wants to merge 2 commits intobitcoindevkit:masterfrom
EddieHouston:rpc-auth

Conversation

@EddieHouston
Copy link

@EddieHouston EddieHouston commented Jan 16, 2026

Description

This PR adds support for dynamic authorization in Electrum RPC requests.
This enables authentication with authorization-protected Electrum servers and API gateways.

Use cases:

  • Bearer token authentication (JWT, OAuth, API keys)
  • API gateways requiring authorization headers
  • Any dynamic authorization scenario where tokens need to be updated during the client's lifetime

Notes to the reviewers

The implementation uses a callback pattern rather than a static token to support dynamic scenarios like automatic token refresh. The AuthProvider is called for each RPC request, allowing the token to be updated without reconnecting the client.

Both Config and RawClient structs needed custom Debug implementations since function pointers don't implement Debug - this is handled by displaying <provider> when an auth provider is present, which prevents leaking sensitive token values in debug output.

Thread safety is ensured through Arc wrapping and Send + Sync bounds, making this safe for concurrent use across the client.

Tests added:
14 new tests, all passing

Changelog notice

Added: Dynamic authorization support via AuthProvider callback in ConfigBuilder. Enables authentication with authorization-protected Electrum servers and automatic token refresh patterns for Bearer tokens, API keys, and other authorization schemes.

Checklists

All Submissions:

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@notmandatory
Copy link
Member

concept ACK

Thanks for this, adding an "authorization" header is the most common way to authenticate to a http endpoints so this should be generally useful. Appreciate the comprehensive examples and unit tests.

The team is spread a bit thin so it may take some time to get it reviewed and tested.

Copy link
Collaborator

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

cACK

I did an initial round of review. Overall, it looks good! I'll give it a try testing it.

@luisschwab
Copy link
Member

@EddieHouston needs rebase

@EddieHouston
Copy link
Author

@EddieHouston needs rebase

@oleonardolima Was this still planned to release before the Electrum protocol v1.6 changes?

@oleonardolima
Copy link
Collaborator

@EddieHouston needs rebase

@oleonardolima Was this still planned to release before the Electrum protocol v1.6 changes?

I'm planning to have both under the same release, as the version negotiation is should work fine for new v1.6 and old servers. Appreciate it, if you could do a rebase.

@oleonardolima
Copy link
Collaborator

Also, you could use the opportunity to squash into a single commit, as it touches the same files.

@EddieHouston

This comment was marked as outdated.

@EddieHouston

This comment was marked as outdated.

@oleonardolima

This comment was marked as outdated.

@oleonardolima

This comment was marked as outdated.

@EddieHouston

This comment was marked as outdated.

Copy link
Collaborator

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

tACK 020f193

It's looking good! I still think the API feels a bit odd, and we could just update the existing methods to have a new auth parameter, though it can be addressed in a follow-up (maybe in another release even).

I gave it a shot and tested it with non-authenticated (testnet) servers that support v1.6 by syncing/broadcasting w/ bdk_wallet, it's still working as expected.

I also tested it with authenticated (mainnet) servers, but didn't tested it by syncing/broadcasting w/ bdk_wallet, still need to test it in testnet/signet if possible.

@oleonardolima oleonardolima requested a review from luisschwab March 6, 2026 00:17
Copy link
Member

@luisschwab luisschwab left a comment

Choose a reason for hiding this comment

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

These should be removed, since electrum-client is unaware of bdk_electrum. This is a dependency loop.

@EddieHouston EddieHouston requested a review from luisschwab March 10, 2026 18:38
@luisschwab
Copy link
Member

luisschwab commented Mar 10, 2026

I still think the API feels a bit odd, and we could just update the existing methods to have a new auth parameter, though it can be addressed in a follow-up (maybe in another release even).

@EddieHouston how hard would it be run this in regtest on CI? Since we'll probably move some stuff around for the next release, would be nice to be able to test it out there and catch any regressions.

@luisschwab
Copy link
Member

@EddieHouston can you also provide complete instructions on testing this with your servers?

@oleonardolima
Copy link
Collaborator

@EddieHouston can you also provide complete instructions on testing this with your servers?

@luisschwab You can probably start from this: oleonardolima@4f6bc16

I think that having it as a full example is better than have it all in just docs, will update to the latest changes and probably push that commit, wdyt @EddieHouston

@luisschwab
Copy link
Member

@oleonardolima go ahead. While you're at it, can you squash the last 3 commits and fix formatting?

@EddieHouston
Copy link
Author

I still think the API feels a bit odd, and we could just update the existing methods to have a new auth parameter, though it can be addressed in a follow-up (maybe in another release even).

@EddieHouston how hard would it be run this in regtest on CI? Since we'll probably move some stuff around for the next release, would be nice to be able to test it out there and catch any regressions.

In theory not difficult, just time. You could maybe mock up a test server that takes an API key or something.

@oleonardolima
Copy link
Collaborator

I cherry-picked my changes for the examples and squashed everything into a single commit.

I changed the jwt_dynamic_auth example to use bitreq instead of reqwest, as reqwest was bringing a bunch of MSRV issues on CI.

Adding a regtest auth infra for testing in CI can be done in a follow-up.

Copy link
Collaborator

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

tACK 1c7ad73

I've tested using this version of the electrum-client for FullScan/Sync and broadcasting with a bdk_wallet on Testnet3. I did it for both unauthenticated and authenticated servers, and it's working as expected.

Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

Thanks for this work. This is only a partial review.

params.to_vec(),
);

// Servers expect auth token only in the first request in batched requests
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately this library doesn't actually use JSON-RPC batch arrays - the "batch" here just means "write all in one go".

We need to add auth token to all requests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@EddieHouston Are there any issues on the server side if we choose to send the authorization within every request ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Anyway, I fixed this in 72dc88a

Copy link
Author

Choose a reason for hiding this comment

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

@EddieHouston Are there any issues on the server side if we choose to send the authorization within every request ?

yeah, it's a huge amount of extra unneeded data over the wire.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@EddieHouston Are there any issues on the server side if we choose to send the authorization within every request ?

yeah, it's a huge amount of extra unneeded data over the wire.

Oh, got it. However, we should probably follow with above's suggestion to guarantee general compatibility.

Copy link
Author

Choose a reason for hiding this comment

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

I don't see the problem, the original code adds the auth only on the first request in the batch, even though its using newline-delimited JSON-RPC, they are all flushed to the wire together and it has auth only on the first one as expected.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can keep as it's for now, if leads to any performance issues we can change it and release in a patch release, as it'd be internal and non-breaking.

src/types.rs Outdated
pub params: Vec<Param>,
/// Optional authorization token (e.g., JWT Bearer token)
#[serde(skip_serializing_if = "Option::is_none")]
pub authorization: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentionally made public? RawClient seems to overwrite this regardless (raw_client.rs:973).

Either make this pub(crate), or change logic to not override a caller-set value (however, does this feature even make sense?).

Copy link
Collaborator

@oleonardolima oleonardolima Mar 16, 2026

Choose a reason for hiding this comment

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

If I got it right, RawClient overwrites it because Request is always being created with an authorization as None.

Yes, it probably can be pub(crate) and already assigned when creating a new Request, that would also fix the issue of only adding the authorization to the first one in batch scenarios.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I fixed this in 72dc88a

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I changed it to private in the latest changes, there's no need to be either pub or pub(crate), you should set it through the new .with_auth() method.

Self::new_with_auth(socket_addrs, timeout, None)
}

pub(crate) fn new_with_auth<A: ToSocketAddrs>(
Copy link
Member

Choose a reason for hiding this comment

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

All of the new .._with_auth methods seem to only populate the auto_provider field. Is this correct?

Instead of having all these duplicate methods - we can use a builder pattern by introducing a single method:

pub(crate) with_auth(mut self, auth_provider: AuthProvider) -> Self {
    self.auth_provider = Some(auth_provider);
    self
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this goes back to my previous comment.

Yes, we should give it a try to builder pattern, it'll give us a better API.
cc @EddieHouston

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright, I did it in the latest commits. It now has a better API and follows the builder pattern.

EddieHouston and others added 2 commits March 20, 2026 15:11
- add new `AuthProvider` public type to handle an `authorization` token.
- add new `Option<AuthProvider>` as `authorization_provider` in
  `Config`, it holds the dynamic authentication token callback provider.
- add new `Option<AuthProvider>` as `auth_provider` in `RawClient`, it
  holds the dynamic authentication token callback provider.
- add new `authorization` private field in `Request`.
- add new `pub fn with_auth()` to `RawClient` in order to set the
  authorization provider into the client, it should be used following a
  builder patter, the client defaults to no authentication otherwise.
- add new `.with_auth()` method to `Request`, it sets the
  `authorization` token, if any.

- update `pub fn negotiate_protocol_version()` in `RawClient` to return
  the `RawClient`, this way it SHOULD no be used following a builder
  pattern.
- update `.call()` and `.batch_call()` to use the newly added
  `.with_auth()`, instead of updating request's authorization field
  directly.

BREAKING CHANGE: all changes above are API breaking changes.
- add two new examples: `jwt_auth.rs` and `jwt_dynamic_auth.rs` as
  references on how to build a client with authentication.
@oleonardolima oleonardolima changed the title feat: add dynamic authorization support via callback provider feat!: add dynamic authorization support Mar 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api API breaking change new feature New feature or request

Projects

Status: Needs Review

Development

Successfully merging this pull request may close these issues.

5 participants