Skip to content

Validate auth entries before signing#2530

Open
mootz12 wants to merge 6 commits intomainfrom
validate-auth-entries
Open

Validate auth entries before signing#2530
mootz12 wants to merge 6 commits intomainfrom
validate-auth-entries

Conversation

@mootz12
Copy link
Copy Markdown
Contributor

@mootz12 mootz12 commented Apr 29, 2026

What

The CLI currently relies on the RPC to check that no non-root auths are included in simulation results. This PR adds an explicit, per-entry validation step inside sign_soroban_authorizations that classifies every Address-credential auth entry against the transaction's host function before signing. Entries that cannot be replayed safely (or are malformed) are rejected with the offending entry rendered inline.

Example error output:

$ stellar contract invoke --source alice --id CDONQZPLRAHWMXLUCSVYJ2NGQIPNH2AFCJOR5XQGR4RY3FFCCBWYCMBY -- diff_auth_sub_auth --addr bob --val "Test" --subcall CAX4O7UT5DG35UAOOXCWBWJMQLXSB7ZLDL3PL52WLI5DIGR22XEMIIIK
ℹ️  Simulating transaction…
❌ error: Signing authorization entries that could be submitted outside the context of the transaction is not supported in the CLI:
  Auth Entry:
    Signer: GCFEAQ5A5BOO4NYTCDN63TF2UX2DS3T3KLNZDHPDPC3IVGCPR37RSRCC
    Invocation:
      Contract: CDONQZPLRAHWMXLUCSVYJ2NGQIPNH2AFCJOR5XQGR4RY3FFCCBWYCMBY
      Fn: diff_auth_sub_auth
      Args:
        "1"
        "2"
      Sub-invocation #0:
        Contract: CAX4O7UT5DG35UAOOXCWBWJMQLXSB7ZLDL3PL52WLI5DIGR22XEMIIIK
        Fn: do_auth
        Args:
          "GCFEAQ5A5BOO4NYTCDN63TF2UX2DS3T3KLNZDHPDPC3IVGCPR37RSRCC"
          Test

Why

The CLI eagerly signs authorization entries returned from the user-specified RPC. If an unsafe auth entry is included, the user might unexpectedly sign for something they did not intend. This check ensures everything the CLI signs is bound to the exact host function invocation in the transaction.

Close https://github.com/stellar/stellar-cli-internal/issues/50

Known limitations

require_auth_for_args for non-source accounts

The check blocks contracts that use require_auth_for_args(custom_args) at the root for non-source accounts. The auth tree's root carries custom_args, not the host function's args, so the strict-match check fails even though the auth is genuinely rooted at the operation. A tampered auth entry with the same custom args at root could otherwise be signed and replayed. Source-account auth via SorobanCredentials::SourceAccount is unaffected.

No bypass flag

This check currently cannot be bypassed. Most Stellar devtools (CLI and Stellar Lab) simulate in recording mode, which blocks non-root authorizations by default. If we add a flag to bypass this, we should also switch to simulating in AuthMode::RecordAllowNonRoot. To keep CLI and Lab behavior consistent, that change is deferred.

Copilot AI review requested due to automatic review settings April 29, 2026 16:43
@github-project-automation github-project-automation Bot moved this to Backlog (Not Ready) in DevX Apr 29, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens soroban-cli transaction signing by explicitly validating each Address-credential Soroban auth entry against the transaction’s host function before signing, rejecting entries that are unsafe to replay or malformed, and improving the error output by rendering the offending auth entry inline.

Changes:

  • Added a host-function vs auth-root-invocation classifier and enforced “strict” auth validation in sign_soroban_authorizations.
  • Introduced pretty-print formatting for auth entries to improve CLI error messages.
  • Added coverage via a new auth fixture contract + integration tests, and added missing RPC network-passphrase verification in extend/restore commands.

Reviewed changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
cmd/soroban-cli/src/tx.rs Updates comment to reflect new validation behavior prior to signing.
cmd/soroban-cli/src/signer/validation.rs Adds auth root-invocation classification logic + unit tests.
cmd/soroban-cli/src/signer/mod.rs Enforces strict validation before signing; adds new errors and source-account credential guard; adds unit tests.
cmd/soroban-cli/src/log/auth.rs Replaces prior debug helper with structured formatting for auth entries.
cmd/soroban-cli/src/commands/contract/restore.rs Adds verify_network_passphrase call on RPC client.
cmd/soroban-cli/src/commands/contract/extend.rs Adds verify_network_passphrase call on RPC client.
cmd/crates/soroban-test/tests/it/integration/util.rs Adds AUTH fixture constant.
cmd/crates/soroban-test/tests/it/integration/auth.rs Adds integration coverage for strict vs non-strict/non-root auth scenarios.
cmd/crates/soroban-test/tests/it/integration.rs Wires in the new auth integration test module.
cmd/crates/soroban-test/tests/fixtures/test-wasms/auth/src/lib.rs Adds a Soroban test contract to generate various auth-tree shapes.
cmd/crates/soroban-test/tests/fixtures/test-wasms/auth/Cargo.toml Adds the new test fixture crate manifest.
Cargo.lock Records the new test_auth fixture crate dependency.

Comment thread cmd/soroban-cli/src/log/auth.rs
Comment thread cmd/soroban-cli/src/signer/validation.rs
Comment thread cmd/soroban-cli/src/signer/mod.rs
Copy link
Copy Markdown
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

.

Copy link
Copy Markdown
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

.

Copy link
Copy Markdown
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

.

Copy link
Copy Markdown
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

.

Copy link
Copy Markdown
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

.

Copy link
Copy Markdown
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

I haven't finished reviewing this, but it's not obvious to me that this is the appropriate action, it's rather restrictive, especially in the context of the known limitations in the pr description.

I think given the stellar-cli is a developer tool it would be more appropriate to do the following instead of rejecting outright:

  • validate the safe case and auto sign in that case (most commands other than invoke should have relatively predictable auth entries required)

  • identify the unsafe cases and display those to the user to confirm if they wish to continue

And for a tool that has narrower use, such as a wallet cli rather than a developer focused cli, to provide narrower restrictions like this to protect its use case.

Comment on lines 118 to 120
ScAddress::MuxedAccount(_) => todo!("muxed accounts are not supported"),
ScAddress::ClaimableBalance(_) => todo!("claimable balance not supported"),
ScAddress::LiquidityPool(_) => todo!("liquidity pool not supported"),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do these panics mean that a malicious RPC could return invalid entries and cause the cli to panic?

I don't think these types of addresses are supported all, so a todo! is probably less appropriate and instead return an error.

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.

I agree, but left it as is since a proper fix is likely adding MuxedAccount support.

See here: #2534

Strict,
/// `root_invocation` does not match the host function exactly. Signing this
/// could produce a portable authorization that could be submitted
/// outside the context of this transaction.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
/// outside the context of this transaction.
/// outside the context of the other contract calls in this transaction.

Even for a root invocation the invoke is still portable if it uses a detached auth entry, so I think indicating the disconnect with the other contract calls rather than the transaction is more appropriate.

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.

Yeah I can fix the wording here a bit. Explained my intentions for the split in the larger comment.

@mootz12
Copy link
Copy Markdown
Contributor Author

mootz12 commented Apr 30, 2026

I think given the stellar-cli is a developer tool it would be more appropriate to do the following instead of rejecting outright:

  • validate the safe case and auto sign in that case (most commands other than invoke should have relatively predictable auth entries required)
  • identify the unsafe cases and display those to the user to confirm if they wish to continue

Thanks for the feedback @leighmcculloch !

This PR is basically step 1, identify "safe" auth. In my mind, I defined this as any auth entry that is tied to the root invocation exactly. Since the idea of this is not to limit user actions, we can assume the user input the contract invocation as intended. Thus, even if the auth could be detached, the only way its valid is if the exact contract invocation the user intended was the root invocation.

To add, I'd be shocked if this was actually restrictive. The only use case it blocks is non-source accounts signing require_auth_for_args, when the only way to provide additional signers is via arguments as it stands.

Step 2 will be adding in bypass logic. It will be shaped roughly as:

  • --auth-mode strict/approve/auto default is approve (can be set by env STELLAR_AUTH_MODE=) and exact naming tbd
    • strict - as this PR is implemented (RPC sims blocking non-root auth)
    • approve - requests approval for any signatures that are not strict (RPC sims allowing non-root auth)
    • auto - just sign everything, add some warning that (RPC sims allowing non-root auth)

I thought doing 1 and 2 together would be a large PR, but happy to do it all at once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog (Not Ready)

Development

Successfully merging this pull request may close these issues.

3 participants