Skip to content

fix: validate username and truncate worker name at authorize time#197

Closed
average-gary wants to merge 1 commit intostratum-mining:mainfrom
fossatmara:fix/user-identity-validate-authorize
Closed

fix: validate username and truncate worker name at authorize time#197
average-gary wants to merge 1 commit intostratum-mining:mainfrom
fossatmara:fix/user-identity-validate-authorize

Conversation

@average-gary
Copy link
Copy Markdown
Contributor

The UserIdentity TLV field has a protocol-specified maximum of 32 bytes. When downstream miners send mining.authorize with long worker names, the translator would panic on unwrap().

This fix:

  • Rejects mining.authorize if the username portion (before the dot) exceeds 32 bytes, as this indicates an invalid configuration
  • Truncates only the worker name portion if the total length exceeds 32 bytes, preserving the username
  • Adds defensive error handling in sv1_server.rs as a fallback

@average-gary average-gary force-pushed the fix/user-identity-validate-authorize branch from 67bf1cf to 190ec4c Compare January 21, 2026 19:20
@average-gary
Copy link
Copy Markdown
Contributor Author

Rebased onto #162

Pr ot that branch here:
Shourya742#1

@average-gary average-gary force-pushed the fix/user-identity-validate-authorize branch from 190ec4c to 30d4718 Compare January 22, 2026 17:06
MAX_USER_IDENTITY_LENGTH,
username.len()
);
return false;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is there a more specific error type we should use here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The return type is defined upstream in stratum_core::sv2_api

@average-gary average-gary force-pushed the fix/user-identity-validate-authorize branch from 8acdf30 to 9583132 Compare January 23, 2026 15:16
@GitGab19
Copy link
Copy Markdown
Member

I think we have some misconceptions, and that's probably due to how we called and managed some fields in the DownstreamData in the past.

We currently have authorized_worker_name and user_identity there, but we are currently setting both values with the same string we receive from the mining.authorize message.

Here the changes I would apply:

  • rename authorized_worker_name into worker_name_from_authorize to better reflect that this field contains the exact string we receive from the Sv1 mining.authorize
  • assign to user_identity only the suffix part of what we receive from the Sv1 mining.authorize (e.g. in case of gitgab.worker1 the user_identity will contain only worker1)
    • if there's no . delimiter, assign the same value as authorized_worker_name
  • we don't check the length of any of these fields in the authorize step of the initial "Sv1 handshake", because we don't even know if we need to use the extension or not there
  • in the Sv1Server, where we create the TLV using the user_identity field of the DownstreamData we handle the result and, if we get an error from the new() (the check on the length is already made there), we return an error and we disconnect the client

Does it make sense to you @average-gary ?

Refactor user identity handling in the translator to properly separate
channel identity from worker identity for TLV extension.

Changes:
- Rename authorized_worker_name to worker_name_from_authorize for clarity
- Store only worker suffix (part after '.') in user_identity for TLV use
- Remove username length validation at authorize time (username is ignored)
- Disconnect client at share submission if worker suffix exceeds 32 bytes
- Remove config-based user_identity overwrite in open_extended_mining_channel
- Update integration tests to reflect new behavior

The user_identity in TLV now contains only the worker portion from
mining.authorize (e.g., 'worker1' from 'user.worker1'), enabling
pass-through of worker identification for hashrate tracking.

The channel identity sent in OpenExtendedMiningChannel continues to use
the translator's config value, which is separate from the TLV worker identity.
@average-gary average-gary force-pushed the fix/user-identity-validate-authorize branch from 9583132 to 1010f9a Compare February 2, 2026 20:08
@average-gary
Copy link
Copy Markdown
Contributor Author

Yes. I see now. But why are we waiting for share submission to fail?

we don't check the length of any of these fields in the authorize step of the initial "Sv1 handshake", because we don't even know if we need to use the extension or not there

We should know. The sv1_server.start() doesn't execute until Upstream is connected and started (included extension negotiation).

@GitGab19
Copy link
Copy Markdown
Member

GitGab19 commented Feb 2, 2026

(included extension negotiation)

I'm not sure about this assertion, are you?

In general, if there's an easy way to get the negotiated extensions from there, I guess we can eventually refuse the mining.authorize in case the username exceeds the max 32 bytes,only when the extension is required and negotiated.

@average-gary
Copy link
Copy Markdown
Contributor Author

I'm not sure about this assertion, are you?

initialize_upstream() of TranlatorSv2 -> try_initialize_upstream() -> creates new() Upstream and then start() -> setup_connection() which has extension negotiation messaging.
Once we're done awaiting try_initialize_upstream() for TranslatorSv2, we then sv1_server_instance.start().

Ah, I see, it's just a request extension message we're sending and then we handle messages.

Is there a reason we don't fully negotiate extensions in connection setup?

@GitGab19
Copy link
Copy Markdown
Member

GitGab19 commented Feb 3, 2026

Is there a reason we don't fully negotiate extensions in connection setup?

I don't remember exactly why I implemented it in that way tbh.

@average-gary
Copy link
Copy Markdown
Contributor Author

Closing in favor of #276.

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.

3 participants