Repository navigation
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // CombineSignShares combines SignShares to produce a valid signature. Players | ||
| // and threshold must be trusted protocol parameters; the function checks them | ||
| // against every share before combining. | ||
| func CombineSignShares(pub *rsa.PublicKey, players, threshold uint, shares []SignShare, msg []byte) (Signature, error) { | ||
| if err := validateParams(players, threshold); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🟡 Signature-combining function changes its public call signature without a compatible alternative or deprecation path
The publicly exported signature-combining entry point gains two required arguments (CombineSignShares at tss/rsa/rsa_threshold.go:204) with no additive alternative kept around, so every existing user of this library stops working until they rewrite their calls.
Impact: Downstream projects that call the threshold RSA combine step break on upgrade with no deprecation warning or migration shim.
Repository rules on breaking public API changes
REVIEW.md (API and compatibility) requires: "Additive changes are preferred. Breaking changes are called out in the PR description and paired with // Deprecated: where feasible". AGENTS.md also says to stop and escalate before changing a public API signature. Here CombineSignShares(pub, shares, msg) becomes CombineSignShares(pub, players, threshold, shares, msg) at tss/rsa/rsa_threshold.go:204, with no // Deprecated: wrapper and no additively named alternative (e.g. a separate function accepting trusted parameters). The docs in tss/rsa/README.md also still describe the old flow.
Prompt for agents
CombineSignShares is an exported API whose signature changed in a breaking way (two new required parameters players/threshold). Repository policy (REVIEW.md "API and compatibility", AGENTS.md) prefers additive changes and requires breaking changes to be documented and paired with a `// Deprecated:` marker on the old API where feasible. Consider keeping the old three-argument entry point (marked `// Deprecated:`, delegating to the new logic using the share-derived parameters) and exposing the parameter-validating variant under a new name, or explicitly document the break in the PR description and package docs (tss/rsa/README.md still describes the old flow).
Was this helpful? React with 👍 or 👎 to provide feedback.
| } | ||
|
|
||
| players := shares[0].Players | ||
| threshold := shares[0].Threshold | ||
|
|
||
| for i := range shares { | ||
| if shares[i].Players != players { | ||
| return nil, errors.New("rsa_threshold: shares didn't have consistent players") | ||
| return nil, errors.New("rsa_threshold: share players did not match protocol players") | ||
| } | ||
| if shares[i].Threshold != threshold { | ||
| return nil, errors.New("rsa_threshold: shares didn't have consistent threshold") | ||
| return nil, errors.New("rsa_threshold: share threshold did not match protocol threshold") | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🟨 Share index from untrusted shares is still not validated against the trusted player count
The PR makes players/threshold trusted inputs and checks each share's Players/Threshold against them (tss/rsa/rsa_threshold.go:210-219), but each share's Index remains fully attacker-controlled and is never checked to be within 1..players or to be distinct across shares. computeLambda (tss/rsa/rsa_threshold.go:312) only requires that the index appears in the provided set, so out-of-range or duplicated indices flow into the Lagrange computation; the combine only fails later via the generic rsa: internal error self-check (tss/rsa/rsa_threshold.go:280-282) or ErrInvalidCalc.
(Refers to lines 210-221)
Was this helpful? React with 👍 or 👎 to provide feedback.
Uh oh!
There was an error while loading. Please reload this page.