feat: require signatures to prove control of signer-key in pox

[hstove]

This closes #4247, which is to ensure that all Signers are using signing keys that they either control, or are controlled by the account they're delegating to.

• closes Signer should control which stackers can delegate to the signer  #4247

This is done by adding a signer-sig param to:

• stack-stx
• delegate-stack-stx
• stack-aggregation-commit

The signature follows SIP018 for signed structured data. Following SIP018 has the benefit of built-in wallet support, so signers can use hardware wallets and apps to generate these signatures.

Following SIP018, the structured data is { reward-cycle, reward-cycle }, where reward-cycle is the current stacking cycle where the transaction is made. The domain is { app: "pox-4-signer", version: "1.0.0", chain-id }.

Because we'll almost certainly be building the ability for stacks-signer to generate these signatures (ie via CLI), I added a SIP18 util library at /stacksib/src/lib_util/signed_structured_data. That library has tests that validate the functionality based on the test vectors in SIP018.

I added a function to_rsv to MessageSignature to match the byte order that Clarity uses for signatures.

Note that I added the new signer-sig param as the second-to-last argument because of other conversations around expected argument order, but I'm not really familiar with the requirements there. I also am not sure what other touch points use these functions where the arguments need to be updated. cc @zone117x regarding param order

[codecov]

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (d8136bc) 58.59% compared to head (e577816) 77.07%.

Files	Patch %	Lines
...tackslib/src/chainstate/stacks/boot/pox_4_tests.rs	98.77%	11 Missing ⚠️
stackslib/src/util_lib/signed_structured_data.rs	97.65%	7 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             next    #4277       +/-   ##
===========================================
+ Coverage   58.59%   77.07%   +18.47%     
===========================================
  Files         445      446        +1     
  Lines      317218   318455     +1237     
===========================================
+ Hits       185882   245435    +59553     
+ Misses     131336    73020    -58316     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[hstove]

Do we need to add the signature check to delegate-stack-increase and delegate-stack-extend?

[hstove]

After further thought, I think we should follow SIP018 (signed structured data) for this. That also unlocks the ability for signers to use a web app to request signatures from a wallet.

[friedger]

#4247 is about delegating signer duties, not delegating stx. As far as I understand.

Pool operators can already refuse to delegate-stack-stx for a pool member.

[hstove]

@friedger @kantai is it better to describe this ticket as "delegates and delegators need to prove ownership of the signing key"? For delegates, this is proving that the signer they're delegating to has control over the signing key.

[jcnelson]

Everyone needs to prove ownership of their signing key. This goes for solo stackers as well, via stack-stx and stack-extend. Basically, anywhere you see a signing key as an argument, you need to also require a signature argument and a signature verification (that should be stack-stx, delegate-stack-stx, stack-extend, and delegate-stack-extend).

[friedger]

Instead of delegate stack stx, pool operators now provide the key in stack aggregation-* function calls.

See #4274

[hstove]

Instead of delegate stack stx, pool operators now provide the key in stack aggregation-* function calls.

See #4274

Good to know - and I'll have to change this to build off the change from signer-key to signer too.

[hstove]

Everyone needs to prove ownership of their signing key. This goes for solo stackers as well, via stack-stx and stack-extend. Basically, anywhere you see a signing key as an argument, you need to also require a signature argument and a signature verification (that should be stack-stx, delegate-stack-stx, stack-extend, and delegate-stack-extend).

👍 that's reflected in the current state of the PR

[hstove]

There are a number of tests outside of the pox-4 tests related to nakamoto that are failing because of the change to stack-stx, I'm working on those still but the pox-4 code is ready for review.

[jcnelson]

Why does this belong in util_lib? Isn't this only used in testing? If so, then please remove this file altogether and put this into stackslib/src/chainstate/stacks/boot/mod.rs.

[hstove]

At the moment it's only used in tests, but we'll add it (in a separate PR) to stacks-signer so we can expose a CLI function like:

stacks-signer create-stacking-signature --stacker S --reward-cycle R

I don't have a strong opinion whether it should live here or not, and I'm not the best person to know. But I think it is helpful to expose somewhere that isn't just for tests.

[jcnelson]

Sounds like this belongs in libsigner then. libsigner can be a dev-dependency for stackslib.

[setzeus]

Generally pox_4.clar updates LGTM minus a few syntax issues & one ERR constant inconsistency.

[hstove]

FYI I'm going to rebase/build this off of #4269, because that changes the function signatures in pox-4. Shouldn't be too much work and then I'll move the ticket back to review.

[hstove]

I've updated this PR with changes since receiving Jude's feedback. Tagging @jcnelson @setzeus @MarvinJanssen @friedger for reviews.

• The signature message hash now includes two new fields
  • topic: one of stack-stx, stack-extend, or agg-commit. This prevents re-using signatures across methods
  • period: a uint reflecting lock-period for stacking or extend-count for extending. For stack-aggregation-commit, this value is u1.
• The get-signer-key-message-hash now takes all hash fields as args, instead of dynamically calling get-current-reward-cycle. This allows signers to easily call this read-only method to generate a message hash.
• In stack-aggregation-commit, the message hash uses the reward cycle provided as user input instead of the current reward cycle.

I've also added some code in util_lib::signed_structured_data::pox4 to allow re-using the Rust code for generating these signatures instead of re-writing a helper function in different places.

[kantai]

This LGTM -- I'd just suggest using the define_named_enum! macro for the topics, and you should add another unit test that asserts equality between the rust message hash constructor and the contract's read-only fn.

[setzeus]

Included a formatting/syntax NIT but outside of that LGTM! Great job on this @hstove.

[kantai]

LGTM -- just see if the ...Old enum can be removed.

[hstove]

LGTM -- just see if the ...Old enum can be removed.

Ah crap, thanks for catching that. CI should pass and then I think this is good to merge!

The message hash function now includes topic and period

[blockstack-devops]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.