Skip to content

Sign auth HMAC over request bodies#207

Merged
tnull merged 1 commit into
lightningdevkit:mainfrom
benthecarman:auth-body
May 18, 2026
Merged

Sign auth HMAC over request bodies#207
tnull merged 1 commit into
lightningdevkit:mainfrom
benthecarman:auth-body

Conversation

@benthecarman
Copy link
Copy Markdown
Collaborator

@benthecarman benthecarman commented May 12, 2026

Require authenticated gRPC requests to bind the HMAC to both the timestamp and the request body. This prevents a valid header from being replayed with different request contents during the allowed timestamp window.

Update the client and docs so callers generate signatures that match the new server contract.

Originally didn't do this to keep things simpler and we were exploring other auth options. Now that we've seemed to settle on this for now, may as well improve it.

Require authenticated gRPC requests to bind the HMAC to both the timestamp and the request body. This prevents a valid header from being replayed with different request contents during the allowed timestamp window.

Update the client and docs so callers generate signatures that match the new server contract.
@ldk-reviews-bot
Copy link
Copy Markdown

ldk-reviews-bot commented May 12, 2026

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@benthecarman benthecarman requested a review from tnull May 12, 2026 21:36
Comment thread ldk-server/src/service.rs
let bytes = match limited_body.collect().await {
Ok(collected) => collected.to_bytes(),
Err(_) => {
return Err(GrpcStatus::new(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

maybe worthwhile logging instead of discarding the error?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No, logging an error here pre-auth would introduce a potential DoS vector, as a malicious client could craft messages that fail to decode, hence spamming our logs.

Comment thread ldk-server/src/service.rs
Comment on lines +531 to +532
fn compute_hmac(api_key: &str, timestamp: u64, body: &[u8]) -> String {
compute_auth_hmac(api_key, timestamp, body).to_string()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

useless indirection?

Copy link
Copy Markdown

@lorenzolfm lorenzolfm left a comment

Choose a reason for hiding this comment

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

(not worth much)ACK 819bed8

@ldk-reviews-bot
Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Comment thread ldk-server/src/service.rs
let bytes = match limited_body.collect().await {
Ok(collected) => collected.to_bytes(),
Err(_) => {
return Err(GrpcStatus::new(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No, logging an error here pre-auth would introduce a potential DoS vector, as a malicious client could craft messages that fail to decode, hence spamming our logs.

@tnull tnull merged commit c25167f into lightningdevkit:main May 18, 2026
10 checks passed
@benthecarman benthecarman deleted the auth-body branch May 18, 2026 17:00
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.

4 participants